From 79beee711f5d716041ab54e1bf8d92a77890f6aa Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Sat, 8 Aug 2026 14:34:03 +0800 Subject: [PATCH] feat(mcp): capability profile and scope enforcement for the online /mcp surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A public bearer client previously reached the full 160-tool catalog, including save_document (fs::write at a caller-chosen path), the process-global codegen plan store, and host diagnostics. Online mode now classifies every tool by surface in one reviewable table and denies the 17 that touch the daemon host (filesystem, outbound fetch, unscoped globals, diagnostics, future process spawn) in both tools/list and tools/call — refusal happens before argument parsing, so traversal payloads never reach a handler. mcp:read/mcp:write scopes gate write tools ahead of dispatch (deny-then-scope, so a filesystem tool reports denial rather than inviting a bigger token); session cookies keep full scope and unclassified static tools fail the build. Local and managed catalogs are unchanged. The deployment-boundaries script now refuses direct invocation instead of green-exiting with every helper missing. --- crates/op-host-services/src/mcp_serve.rs | 66 +- .../op-host-services/src/mcp_serve/tests.rs | 41 +- .../src/mcp_serve/tool_profile.rs | 629 ++++++++++++++++++ .../src/mcp_serve/tool_profile_tests.rs | 253 +++++++ .../src/mcp_serve/tools_list.rs | 50 ++ .../src/web_canvas_server/connection.rs | 18 +- .../src/web_canvas_server/hub_verifier.rs | 39 +- .../src/web_canvas_server/online_mcp_tests.rs | 218 ++++++ .../src/web_canvas_server/online_run_loop.rs | 3 + .../online_run_loop_tests.rs | 4 + .../src/web_canvas_server/tenant_auth.rs | 37 +- .../src/web_canvas_server/tenant_tests.rs | 1 + tools/check-collab-deployment-boundaries.sh | 27 + 13 files changed, 1346 insertions(+), 40 deletions(-) create mode 100644 crates/op-host-services/src/mcp_serve/tool_profile.rs create mode 100644 crates/op-host-services/src/mcp_serve/tool_profile_tests.rs create mode 100644 crates/op-host-services/src/mcp_serve/tools_list.rs create mode 100644 crates/op-host-services/src/web_canvas_server/online_mcp_tests.rs diff --git a/crates/op-host-services/src/mcp_serve.rs b/crates/op-host-services/src/mcp_serve.rs index 1e9407e76..e958e402a 100644 --- a/crates/op-host-services/src/mcp_serve.rs +++ b/crates/op-host-services/src/mcp_serve.rs @@ -137,6 +137,32 @@ fn process_message( pub fn process_message_with_applier( state: &mut EditorState, line: &str, + apply: F, +) -> Result, McpServeError> +where + F: FnMut(&str, &mut EditorState, &EditorCommand) -> bool, +{ + // The unrestricted profile is the whole catalog with full authority — + // exactly what this function did before capability profiles existed, so + // every local and managed caller is unchanged. + process_message_with_applier_profiled( + state, + line, + tool_profile::McpAccessProfile::UNRESTRICTED, + apply, + ) +} + +/// [`process_message_with_applier`] under an explicit capability profile. +/// +/// The profile decides which tools the catalog advertises and which calls are +/// refused before they run. A refusal is a normal `tools/call` error envelope +/// carrying the originating request id — never a panic, and never a transport +/// error, because an MCP client has to be able to keep the session. +pub fn process_message_with_applier_profiled( + state: &mut EditorState, + line: &str, + profile: tool_profile::McpAccessProfile, mut apply: F, ) -> Result, McpServeError> where @@ -153,7 +179,7 @@ where } Some("tools/list") => { return Ok(sniff_id_raw(trimmed) - .map(|id| tools_list_response(&id, state, debug_tools_enabled()))); + .map(|id| tools_list_response(&id, state, debug_tools_enabled(), profile))); } Some("notifications/initialized") | Some("initialized") => { return Ok(None); // notification — no response required @@ -167,7 +193,25 @@ where // Fall through: tools/call or legacy direct dispatch. The // registry snapshots `state` at build time, so it no longer // borrows it once the applier closure mutates it. - let requested_tool = op_mcp::parse_tool_call(trimmed).map(|call| call.tool); + let call = op_mcp::parse_tool_call(trimmed); + // Enforcement, ahead of the registry build and therefore ahead of the + // tool's own argument parsing: a denied tool never sees the path it was + // asked to open. + if let Some(call) = call.as_ref() { + if let Some(refusal) = profile.refuse(&call.tool) { + // The ordinary tools/call error envelope (`isError:true`) with + // the originating id, so a client sees a refusal it can read + // rather than a transport failure that would drop the session. + return Ok(Some(op_mcp::tool_response_to_json( + &op_mcp::ToolResponse::Err { + id: call.id.clone(), + code: op_mcp::ToolErrorCode::ToolFailed, + message: refusal.message(&call.tool), + }, + ))); + } + } + let requested_tool = call.map(|call| call.tool); let registry = rebuild_registry(state, requested_tool.as_deref()); process_tool_message_with_registry(®istry, line, |tool_name, cmd| { apply(tool_name, state, cmd) @@ -716,22 +760,10 @@ pub use wire::*; mod doc_sync; pub use doc_sync::*; -fn tools_list_response(id_raw: &str, state: &EditorState, debug_enabled: bool) -> String { - let mut entries: Vec = TOOL_SCHEMAS.iter().map(|s| (*s).to_string()).collect(); - entries.extend(op_mcp::element_tools::element_tool_schemas(state)); - #[cfg(not(feature = "mcp-debug-tools"))] - let _ = debug_enabled; - #[cfg(feature = "mcp-debug-tools")] - if debug_enabled { - entries.extend(DEBUG_TOOL_SCHEMAS.iter().map(|s| (*s).to_string())); - } - format!( - r#"{{"jsonrpc":"2.0","id":{id_raw},"result":{{"tools":[{}]}}}}"#, - entries.join(",") - ) -} - pub(crate) mod schemas; +mod tools_list; +use tools_list::tools_list_response; +pub mod tool_profile; #[cfg(not(feature = "mcp-debug-tools"))] pub use schemas::TOOL_SCHEMAS; #[cfg(feature = "mcp-debug-tools")] diff --git a/crates/op-host-services/src/mcp_serve/tests.rs b/crates/op-host-services/src/mcp_serve/tests.rs index 30e51f480..a3cee4e1c 100644 --- a/crates/op-host-services/src/mcp_serve/tests.rs +++ b/crates/op-host-services/src/mcp_serve/tests.rs @@ -11,7 +11,12 @@ fn tools_list_response_includes_all_registered_tools() { // Debug gating is passed explicitly (no process-global env mutation, // so this test can't race other tests' env access). let state = op_editor_core::EditorState::new(); - let r = tools_list_response("3", &state, false); + let r = tools_list_response( + "3", + &state, + false, + tool_profile::McpAccessProfile::UNRESTRICTED, + ); // The production catalog excludes debug tools. Exact-count // assertion: any tool added without updating this test trips // the count first. Codex stop-gate: previous `contains`-only @@ -30,7 +35,12 @@ fn tools_list_response_includes_all_registered_tools() { ); #[cfg(not(feature = "mcp-debug-tools"))] { - let r_forced_debug = tools_list_response("3", &state, true); + let r_forced_debug = tools_list_response( + "3", + &state, + true, + tool_profile::McpAccessProfile::UNRESTRICTED, + ); for name in [ "debug_validation_report", "debug_logs_tail", @@ -193,7 +203,12 @@ fn tools_list_response_includes_all_registered_tools() { // Gate open (debug_enabled = true) — internal debug builds can opt in // to the debug tools catalog. - let r_debug = tools_list_response("3", &state, true); + let r_debug = tools_list_response( + "3", + &state, + true, + tool_profile::McpAccessProfile::UNRESTRICTED, + ); #[cfg(feature = "mcp-debug-tools")] for name in [ "debug_validation_report", @@ -215,9 +230,13 @@ fn tools_list_response_includes_all_registered_tools() { #[test] fn tools_list_design_content_schema_advertises_ts_layered_args() { let state = op_editor_core::EditorState::new(); - let response: serde_json::Value = - serde_json::from_str(&tools_list_response("3", &state, false)) - .expect("tools/list response should be JSON"); + let response: serde_json::Value = serde_json::from_str(&tools_list_response( + "3", + &state, + false, + tool_profile::McpAccessProfile::UNRESTRICTED, + )) + .expect("tools/list response should be JSON"); let tools = response["result"]["tools"] .as_array() .expect("tools/list result should contain tools"); @@ -255,9 +274,13 @@ fn tools_list_design_content_schema_advertises_ts_layered_args() { #[test] fn tools_list_schemas_advertise_ts_file_path_args() { let state = op_editor_core::EditorState::new(); - let response: serde_json::Value = - serde_json::from_str(&tools_list_response("3", &state, false)) - .expect("tools/list response should be JSON"); + let response: serde_json::Value = serde_json::from_str(&tools_list_response( + "3", + &state, + false, + tool_profile::McpAccessProfile::UNRESTRICTED, + )) + .expect("tools/list response should be JSON"); let tools = response["result"]["tools"] .as_array() .expect("tools/list result should contain tools"); diff --git a/crates/op-host-services/src/mcp_serve/tool_profile.rs b/crates/op-host-services/src/mcp_serve/tool_profile.rs new file mode 100644 index 000000000..9eb0be009 --- /dev/null +++ b/crates/op-host-services/src/mcp_serve/tool_profile.rs @@ -0,0 +1,629 @@ +//! What each MCP tool needs, and what a deployment is willing to give it. +//! +//! The tool catalog grew up serving one local operator, so a tool that writes +//! a caller-named path or reads the daemon's own config directory was simply a +//! feature. A public multi-account deployment shares one process — and one +//! filesystem — between mutually untrusting accounts, so those tools have to +//! be off, and a token that was issued read-only must not be able to drive a +//! write tool. +//! +//! Both questions are answered from ONE table, [`TOOL_PROFILES`], because the +//! failure mode of two tables is that they disagree. A test pins the table +//! against `schemas::TOOL_SCHEMAS` in both directions, so a tool added to the +//! catalog without being classified fails the build rather than defaulting to +//! something permissive. +//! +//! ## Where this is enforced +//! +//! Two places, and it must be both: +//! +//! - `tools/list` — a denied tool is filtered out of the catalog, so a client +//! never learns it exists or writes a plan around it. +//! - `tools/call` — a denied tool is answered with a refusal even when the +//! client asks for it by name anyway. Filtering the catalog is discovery, +//! not enforcement; this is enforcement. +//! +//! The refusal happens BEFORE the tool runs, so a path-traversal argument on +//! a denied tool never reaches the code that would open it. + +#[cfg(feature = "mcp-debug-tools")] +use super::schemas::DEBUG_TOOL_SCHEMAS; +use super::schemas::TOOL_SCHEMAS; + +/// Whether a tool mutates the document. +/// +/// Determined from the tool's own outcome contract: a `Write` tool is one +/// whose `call` can return `ToolOutcome::OkWithCommand` / `OkJsonWithCommand`, +/// i.e. one that hands the host an `EditorCommand` to apply. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ToolAccess { + Read, + Write, +} + +/// What the tool touches besides the in-memory document. +/// +/// Only [`ToolSurface::InMemory`] is safe to expose on a shared deployment; +/// every other variant names a resource that belongs to the daemon's host or +/// is shared process-wide with no tenant dimension. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ToolSurface { + /// Only the in-memory document and editor state. + InMemory, + /// Reads or writes the daemon host's filesystem, usually at a path the + /// caller chooses. + LocalFilesystem, + /// Performs outbound network from the daemon host. + OutboundNetwork, + /// Reads or writes process-global state that carries no tenant + /// dimension, so one account can observe or destroy another's. + ProcessGlobal, + /// Reports on the daemon host itself rather than on a document. + HostDiagnostics, + /// Spawns, or is specified to grow into spawning, host processes. + HostProcess, +} + +impl ToolSurface { + /// Whether this surface is safe to expose to an untrusted account. + pub const fn is_shareable(self) -> bool { + matches!(self, Self::InMemory) + } +} + +/// One tool's classification. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ToolProfile { + pub name: &'static str, + pub access: ToolAccess, + pub surface: ToolSurface, +} + +impl ToolProfile { + const fn new(name: &'static str, access: ToolAccess, surface: ToolSurface) -> Self { + Self { + name, + access, + surface, + } + } +} + +/// Why a tool call was refused before it ran. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ToolRefusal { + /// The tool reaches a host or process-global resource that a shared + /// deployment cannot partition between accounts. + LocalResourceDenied, + /// The caller's credential does not carry the scope this tool needs. + ScopeInsufficient, +} + +impl ToolRefusal { + /// Stable machine-readable code. It leads the wire message so a client + /// can branch on it without parsing prose. + pub const fn code(self) -> &'static str { + match self { + Self::LocalResourceDenied => "tool-not-available", + Self::ScopeInsufficient => "scope-insufficient", + } + } + + /// The full refusal text a client receives. + pub fn message(self, tool: &str) -> String { + match self { + Self::LocalResourceDenied => format!( + "{}: the tool '{tool}' is not available on this deployment", + self.code() + ), + Self::ScopeInsufficient => format!( + "{}: the tool '{tool}' requires the '{}' scope", + self.code(), + MCP_WRITE_SCOPE + ), + } + } +} + +impl std::fmt::Display for ToolRefusal { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(match self { + Self::LocalResourceDenied => "tool is not available on this deployment", + Self::ScopeInsufficient => "credential lacks the required scope", + }) + } +} + +impl std::error::Error for ToolRefusal {} + +/// Scope names, matching what op-hub issues on an API token. +pub const MCP_READ_SCOPE: &str = "mcp:read"; +pub const MCP_WRITE_SCOPE: &str = "mcp:write"; + +/// What a credential is allowed to do over MCP. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct McpScopes { + read: bool, + write: bool, +} + +impl McpScopes { + /// Unrestricted — what a local operator, a managed supervisor, and a + /// browser session all get. + pub const FULL: Self = Self { + read: true, + write: true, + }; + + /// Read-only. + pub const READ_ONLY: Self = Self { + read: true, + write: false, + }; + + /// Derive from the scope list on a hub token. + /// + /// A token that names NO `mcp:*` scope at all is treated as unrestricted. + /// That is the compatibility default for a token store that does not yet + /// issue scopes: the alternative would refuse every token ever minted + /// before scopes existed. Once a token names ANY `mcp:*` scope, the list + /// is authoritative and anything absent from it is denied — so an + /// explicitly read-only token is genuinely read-only. + pub fn from_scope_list>(scopes: &[S]) -> Self { + let mentions_mcp = scopes + .iter() + .any(|scope| scope.as_ref().trim().starts_with("mcp:")); + if !mentions_mcp { + return Self::FULL; + } + let has = |wanted: &str| scopes.iter().any(|scope| scope.as_ref().trim() == wanted); + Self { + read: has(MCP_READ_SCOPE), + write: has(MCP_WRITE_SCOPE), + } + } + + pub const fn allows(self, access: ToolAccess) -> bool { + match access { + ToolAccess::Read => self.read, + ToolAccess::Write => self.write, + } + } + + pub const fn can_write(self) -> bool { + self.write + } +} + +impl Default for McpScopes { + fn default() -> Self { + Self::FULL + } +} + +/// The MCP capability profile one request is served under. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct McpAccessProfile { + /// Refuse every tool whose surface is not shareable. + pub deny_unshareable_tools: bool, + pub scopes: McpScopes, +} + +impl McpAccessProfile { + /// What the local and managed daemons run under: the whole catalog, full + /// authority. Byte-for-byte the behaviour that predates this module. + pub const UNRESTRICTED: Self = Self { + deny_unshareable_tools: false, + scopes: McpScopes::FULL, + }; + + /// The public multi-account profile. + pub const fn online(scopes: McpScopes) -> Self { + Self { + deny_unshareable_tools: true, + scopes, + } + } + + /// Whether `tool` may appear in this profile's `tools/list`. + /// + /// Scope is deliberately NOT consulted here: a read-only token should + /// still see that a write tool exists, and be told why when it calls one. + /// Hiding it would look like the tool had been removed. + pub fn lists(&self, tool: &str) -> bool { + !self.deny_unshareable_tools || surface_of(tool).is_shareable() + } + + /// Why `tool` may not be called, if it may not. + /// + /// Denial ranks above scope: a tool that is off for everyone should say + /// so rather than suggesting a bigger token would help. + pub fn refuse(&self, tool: &str) -> Option { + if self.deny_unshareable_tools && !surface_of(tool).is_shareable() { + return Some(ToolRefusal::LocalResourceDenied); + } + if !self.scopes.allows(access_of(tool)) { + return Some(ToolRefusal::ScopeInsufficient); + } + None + } +} + +impl Default for McpAccessProfile { + fn default() -> Self { + Self::UNRESTRICTED + } +} + +/// The classification for `name`, if it is in the static catalog. +pub fn profile_for(name: &str) -> Option<&'static ToolProfile> { + TOOL_PROFILES.iter().find(|profile| profile.name == name) +} + +/// The surface of `name`, defaulting to the shareable one. +/// +/// A name outside the static catalog is an `element_tools` insert tool: +/// those are generated per-document from the document's OWN component kits, +/// so they touch nothing but the in-memory document. The table-parity test +/// is what keeps this default from silently covering a real omission — a +/// static tool that is never classified fails that test. +fn surface_of(name: &str) -> ToolSurface { + profile_for(name).map_or(ToolSurface::InMemory, |profile| profile.surface) +} + +/// The access level of `name`, defaulting to `Write`. +/// +/// Fail-closed on purpose, and in the direction that matters: an +/// unclassified tool is assumed to mutate, so a read-only credential cannot +/// reach it. The dynamic insert tools this covers really are writes. +fn access_of(name: &str) -> ToolAccess { + profile_for(name).map_or(ToolAccess::Write, |profile| profile.access) +} + +/// Every tool in `schemas::TOOL_SCHEMAS`, classified. +/// +/// Kept in the same alphabetical order as the catalog so the two are +/// diffable side by side. +pub const TOOL_PROFILES: &[ToolProfile] = &[ + ToolProfile::new("ToolSearch", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("add_node_effect", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("add_page", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("align_selected", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "apply_design_system", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new("batch_design", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("batch_get", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("clear_selection", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "codegen_assemble", + ToolAccess::Read, + ToolSurface::ProcessGlobal, + ), + ToolProfile::new( + "codegen_clean", + ToolAccess::Read, + ToolSurface::ProcessGlobal, + ), + ToolProfile::new("codegen_plan", ToolAccess::Read, ToolSurface::ProcessGlobal), + ToolProfile::new( + "codegen_submit_chunk", + ToolAccess::Read, + ToolSurface::ProcessGlobal, + ), + ToolProfile::new("conversion_status", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("copy_node", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("copy_selected", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("count_nodes", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("create_component", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("create_variable", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("cut_selected", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "cycle_active_axis_value", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new( + "debug_logs_tail", + ToolAccess::Read, + ToolSurface::LocalFilesystem, + ), + ToolProfile::new( + "debug_screenshot", + ToolAccess::Read, + ToolSurface::HostDiagnostics, + ), + ToolProfile::new( + "debug_validation_report", + ToolAccess::Read, + ToolSurface::HostDiagnostics, + ), + ToolProfile::new("delete_component", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("delete_node", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("delete_page", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("delete_selected", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("delete_variable", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("design_content", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("design_refine", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("design_skeleton", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("duplicate_page", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "duplicate_selected", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new("export_design_md", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("export_item", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("export_nodes", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("find_empty_space", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("find_node_by_name", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_active_theme", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_canvas_bounds", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_component", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_design_md", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_design_prompt", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_document_info", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_editor_state", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_guidelines", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_history_depth", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_node", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_node_children", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_node_parent", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_screenshot", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_selection", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_selection_set", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_style_guide", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new( + "get_style_guide_tags", + ToolAccess::Read, + ToolSurface::InMemory, + ), + ToolProfile::new("get_variables", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("get_viewport", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("group_selected", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "import_html", + ToolAccess::Write, + ToolSurface::LocalFilesystem, + ), + ToolProfile::new( + "import_html_url", + ToolAccess::Write, + ToolSurface::OutboundNetwork, + ), + ToolProfile::new( + "import_svg", + ToolAccess::Write, + ToolSurface::LocalFilesystem, + ), + ToolProfile::new( + "import_web_snapshot", + ToolAccess::Write, + ToolSurface::LocalFilesystem, + ), + ToolProfile::new("insert_node", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "instantiate_component", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new("lint_document", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("list_components", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("list_node_kinds", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("list_pages", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new( + "list_theme_presets", + ToolAccess::Read, + ToolSurface::LocalFilesystem, + ), + ToolProfile::new("list_variables", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new( + "load_theme_preset", + ToolAccess::Write, + ToolSurface::LocalFilesystem, + ), + ToolProfile::new("move_node", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("nudge_selected", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "open_document", + ToolAccess::Read, + ToolSurface::LocalFilesystem, + ), + ToolProfile::new("paste_clipboard", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("read_nodes", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("redo", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "remove_node_effect", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new("remove_page", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("rename_component", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("rename_page", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("rename_variable", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("reorder_page", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("reorder_selected", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "replace_all_matching_properties", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new("replace_node", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "save_document", + ToolAccess::Read, + ToolSurface::LocalFilesystem, + ), + ToolProfile::new( + "save_theme_preset", + ToolAccess::Read, + ToolSurface::LocalFilesystem, + ), + ToolProfile::new( + "search_all_unique_properties", + ToolAccess::Read, + ToolSurface::InMemory, + ), + ToolProfile::new( + "set_active_axis_value", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new("set_active_page", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("set_active_tool", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("set_design_md", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("set_ellipse_arc", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "set_node_collapsed", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new( + "set_node_corner_radius", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new( + "set_node_fill_hex", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new("set_node_flip", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "set_node_font_size", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new( + "set_node_font_weight", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new("set_node_hidden", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("set_node_locked", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("set_node_name", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "set_node_rotation", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new( + "set_node_stroke_hex", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new( + "set_node_stroke_side_width", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new( + "set_node_stroke_width", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new("set_node_text", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("set_selection", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "set_selection_set", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new("set_themes", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new( + "set_variable_boolean", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new( + "set_variable_color", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new( + "set_variable_number", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new( + "set_variable_string", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new("set_variables", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("set_viewport", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("snapshot_layout", ToolAccess::Read, ToolSurface::InMemory), + ToolProfile::new("spawn_agents", ToolAccess::Read, ToolSurface::HostProcess), + ToolProfile::new( + "toggle_node_selection", + ToolAccess::Write, + ToolSurface::InMemory, + ), + ToolProfile::new("undo", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("ungroup_selected", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("update_node", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("upsert_component", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("upsert_screen", ToolAccess::Write, ToolSurface::InMemory), + ToolProfile::new("upsert_variables", ToolAccess::Write, ToolSurface::InMemory), +]; + +/// Names denied on a shared deployment, for diagnostics and tests. +pub fn denied_tool_names() -> Vec<&'static str> { + TOOL_PROFILES + .iter() + .filter(|profile| !profile.surface.is_shareable()) + .map(|profile| profile.name) + .collect() +} + +/// Names that only exist in a build with the debug-tool feature. +/// +/// They stay classified in every build so the deny decision cannot be lost +/// by flipping a feature flag; the parity test knows they are absent from the +/// catalog when the feature is off. +pub const DEBUG_ONLY_TOOLS: &[&str] = &[ + "debug_logs_tail", + "debug_screenshot", + "debug_validation_report", +]; + +pub fn is_debug_only_tool(name: &str) -> bool { + DEBUG_ONLY_TOOLS.contains(&name) +} + +/// Every catalog name this build advertises. +/// +/// Only the parity tests consume this outside a debug-tool build; it stays +/// compiled either way so the two builds cannot drift. +#[cfg_attr(not(feature = "mcp-debug-tools"), allow(dead_code))] +pub(crate) fn catalog_tool_names() -> Vec { + #[cfg_attr(not(feature = "mcp-debug-tools"), allow(unused_mut))] + let mut names: Vec = TOOL_SCHEMAS + .iter() + .filter_map(|schema| schema_name(schema)) + .collect(); + #[cfg(feature = "mcp-debug-tools")] + names.extend( + DEBUG_TOOL_SCHEMAS + .iter() + .filter_map(|schema| schema_name(schema)), + ); + names +} + +/// Pull `"name":"…"` out of a schema entry. +/// +/// The schemas are pre-serialized JSON string constants, and the name is +/// always the first member, so this reads it without a JSON parse. +pub(crate) fn schema_name(schema: &str) -> Option { + let rest = schema.split_once(r#""name":"#)?.1.trim_start(); + let rest = rest.strip_prefix('"')?; + let end = rest.find('"')?; + Some(rest[..end].to_string()) +} + +#[cfg(test)] +#[path = "tool_profile_tests.rs"] +mod tests; diff --git a/crates/op-host-services/src/mcp_serve/tool_profile_tests.rs b/crates/op-host-services/src/mcp_serve/tool_profile_tests.rs new file mode 100644 index 000000000..a6aaed96a --- /dev/null +++ b/crates/op-host-services/src/mcp_serve/tool_profile_tests.rs @@ -0,0 +1,253 @@ +//! Tests for the MCP capability profile. +//! +//! The parity tests are the load-bearing ones: they are what turns "someone +//! added a tool and forgot to classify it" from a silent security hole into a +//! build failure. + +use super::*; + +#[test] +fn every_catalog_tool_is_classified() { + let missing: Vec = catalog_tool_names() + .into_iter() + .filter(|name| profile_for(name).is_none()) + .collect(); + assert!( + missing.is_empty(), + "these tools are in TOOL_SCHEMAS but carry no capability profile, so a \ + deployment would fall back to a default instead of a decision: {missing:?}" + ); +} + +#[test] +fn every_classified_tool_is_in_the_catalog() { + let catalog = catalog_tool_names(); + let stale: Vec<&str> = TOOL_PROFILES + .iter() + .map(|profile| profile.name) + // The debug tools are classified in every build but only present in + // the catalog of a debug-tool build; keeping their classification + // unconditional is what stops a feature flag from losing the deny. + .filter(|name| !is_debug_only_tool(name)) + .filter(|name| !catalog.iter().any(|entry| entry == name)) + .collect(); + assert!( + stale.is_empty(), + "these profiles name tools the catalog no longer has: {stale:?}" + ); +} + +#[test] +fn the_table_has_no_duplicate_entries() { + let mut names: Vec<&str> = TOOL_PROFILES.iter().map(|profile| profile.name).collect(); + let before = names.len(); + names.sort_unstable(); + names.dedup(); + assert_eq!(before, names.len(), "duplicate profile entries"); +} + +#[test] +fn the_denied_set_is_exactly_the_reviewed_list() { + // Pinned deliberately: widening this set is a product decision and + // narrowing it is a security decision. Either way it should be a diff a + // reviewer sees, not a side effect of editing a tool. + let mut denied = denied_tool_names(); + denied.sort_unstable(); + assert_eq!( + denied, + vec![ + "codegen_assemble", + "codegen_clean", + "codegen_plan", + "codegen_submit_chunk", + "debug_logs_tail", + "debug_screenshot", + "debug_validation_report", + "import_html", + "import_html_url", + "import_svg", + "import_web_snapshot", + "list_theme_presets", + "load_theme_preset", + "open_document", + "save_document", + "save_theme_preset", + "spawn_agents", + ] + ); +} + +#[test] +fn the_unrestricted_profile_refuses_nothing_and_lists_everything() { + let profile = McpAccessProfile::UNRESTRICTED; + for name in catalog_tool_names() { + assert!(profile.lists(&name), "{name} must stay listed"); + assert_eq!(profile.refuse(&name), None, "{name} must stay callable"); + } +} + +#[test] +fn the_online_profile_refuses_every_unshareable_tool() { + let profile = McpAccessProfile::online(McpScopes::FULL); + for name in denied_tool_names() { + assert!(!profile.lists(name), "{name} must not be advertised"); + assert_eq!( + profile.refuse(name), + Some(ToolRefusal::LocalResourceDenied), + "{name} must be refused even when asked for by name" + ); + } +} + +#[test] +fn the_online_profile_still_serves_the_in_memory_catalog() { + let profile = McpAccessProfile::online(McpScopes::FULL); + for name in [ + "add_page", + "insert_node", + "get_node", + "batch_design", + "undo", + ] { + assert!(profile.lists(name), "{name}"); + assert_eq!(profile.refuse(name), None, "{name}"); + } +} + +#[test] +fn a_read_only_credential_may_read_but_not_write() { + let profile = McpAccessProfile::online(McpScopes::READ_ONLY); + for read_tool in ["get_node", "list_pages", "snapshot_layout", "lint_document"] { + assert_eq!(profile.refuse(read_tool), None, "{read_tool}"); + } + for write_tool in [ + "add_page", + "insert_node", + "delete_node", + "undo", + "batch_design", + ] { + assert_eq!( + profile.refuse(write_tool), + Some(ToolRefusal::ScopeInsufficient), + "{write_tool}" + ); + } +} + +#[test] +fn a_read_only_credential_still_sees_the_write_tools_it_cannot_call() { + // Hiding them would read as "this deployment has no write tools" rather + // than "your token cannot use them", which is a worse diagnostic. + let profile = McpAccessProfile::online(McpScopes::READ_ONLY); + assert!(profile.lists("add_page")); +} + +#[test] +fn a_denied_tool_reports_denial_rather_than_a_scope_problem() { + // `save_document` is a document READ that writes the filesystem, so a + // scope-first check would wave it through for a read-only token. + let read_only = McpAccessProfile::online(McpScopes::READ_ONLY); + assert_eq!( + read_only.refuse("save_document"), + Some(ToolRefusal::LocalResourceDenied), + "a bigger token must not look like the fix" + ); + assert_eq!( + profile_for("save_document").map(|profile| profile.access), + Some(ToolAccess::Read), + "this is the trap the ordering exists for" + ); +} + +#[test] +fn an_unclassified_tool_is_assumed_to_write() { + // The dynamic `add_*` insert tools are generated per document and are + // never in the static catalog. They are writes, and an unclassified name + // must not be reachable by a read-only credential. + let profile = McpAccessProfile::online(McpScopes::READ_ONLY); + assert_eq!( + profile.refuse("add_some_kit_component"), + Some(ToolRefusal::ScopeInsufficient) + ); + // …but it is still in-memory, so a full-scope online credential may use it. + assert_eq!( + McpAccessProfile::online(McpScopes::FULL).refuse("add_some_kit_component"), + None + ); +} + +#[test] +fn scopes_are_derived_from_the_hub_scope_list() { + assert_eq!( + McpScopes::from_scope_list(&["mcp:read"]), + McpScopes::READ_ONLY + ); + assert!(McpScopes::from_scope_list(&["mcp:read", "mcp:write"]).can_write()); + // Write without read is honoured as written rather than normalised. + assert!(McpScopes::from_scope_list(&["mcp:write"]).can_write()); + assert!(!McpScopes::from_scope_list(&["mcp:write"]).allows(ToolAccess::Read)); + // A list that names no mcp scope predates scoping — see the doc comment. + assert_eq!( + McpScopes::from_scope_list(&["billing:read"]), + McpScopes::FULL + ); + assert_eq!(McpScopes::from_scope_list::<&str>(&[]), McpScopes::FULL); + // An unknown mcp scope does NOT grant anything. + assert_eq!( + McpScopes::from_scope_list(&["mcp:admin"]), + McpScopes { + read: false, + write: false + } + ); +} + +#[test] +fn a_refusal_message_leads_with_its_machine_code() { + let denied = ToolRefusal::LocalResourceDenied.message("save_document"); + assert!(denied.starts_with("tool-not-available:"), "{denied}"); + assert!(denied.contains("save_document"), "{denied}"); + + let scoped = ToolRefusal::ScopeInsufficient.message("add_page"); + assert!(scoped.starts_with("scope-insufficient:"), "{scoped}"); + assert!(scoped.contains("mcp:write"), "{scoped}"); +} + +#[test] +fn schema_names_are_read_out_of_the_catalog_entries() { + assert_eq!( + schema_name(r#"{"name":"get_node","description":"x"}"#).as_deref(), + Some("get_node") + ); + assert_eq!(schema_name("{}"), None); +} + +#[test] +fn every_tool_that_writes_the_local_filesystem_is_denied() { + // Independent of the pinned list above: these are the tools the audit + // found reaching the host filesystem, asserted by surface rather than by + // name so a reclassification cannot quietly re-expose one. + for name in [ + "save_document", + "save_theme_preset", + "load_theme_preset", + "list_theme_presets", + "import_html", + "import_svg", + "import_web_snapshot", + "debug_logs_tail", + "open_document", + ] { + assert_eq!( + profile_for(name).map(|profile| profile.surface), + Some(ToolSurface::LocalFilesystem), + "{name}" + ); + assert!(!ToolSurface::LocalFilesystem.is_shareable()); + } + assert_eq!( + profile_for("import_html_url").map(|profile| profile.surface), + Some(ToolSurface::OutboundNetwork) + ); +} diff --git a/crates/op-host-services/src/mcp_serve/tools_list.rs b/crates/op-host-services/src/mcp_serve/tools_list.rs new file mode 100644 index 000000000..b6e278e9e --- /dev/null +++ b/crates/op-host-services/src/mcp_serve/tools_list.rs @@ -0,0 +1,50 @@ +//! The `tools/list` catalog response. +//! +//! Split out of the `mcp_serve` spine at the 800-line cap. It lives next to +//! `tool_profile` because the two are one decision: the profile says which +//! tools a deployment offers, and this is where that answer becomes the +//! catalog a client sees. + +use op_editor_core::EditorState; + +#[cfg(feature = "mcp-debug-tools")] +use super::schemas::DEBUG_TOOL_SCHEMAS; +use super::schemas::TOOL_SCHEMAS; +use super::tool_profile; + +pub(super) fn tools_list_response( + id_raw: &str, + state: &EditorState, + debug_enabled: bool, + profile: tool_profile::McpAccessProfile, +) -> String { + // Discovery follows enforcement: a tool this profile would refuse is not + // advertised, so a client never plans around one it cannot call. + let listed = + |schema: &str| tool_profile::schema_name(schema).is_none_or(|name| profile.lists(&name)); + let mut entries: Vec = TOOL_SCHEMAS + .iter() + .filter(|schema| listed(schema)) + .map(|s| (*s).to_string()) + .collect(); + entries.extend( + op_mcp::element_tools::element_tool_schemas(state) + .into_iter() + .filter(|schema| listed(schema)), + ); + #[cfg(not(feature = "mcp-debug-tools"))] + let _ = debug_enabled; + #[cfg(feature = "mcp-debug-tools")] + if debug_enabled { + entries.extend( + DEBUG_TOOL_SCHEMAS + .iter() + .filter(|schema| listed(schema)) + .map(|s| (*s).to_string()), + ); + } + format!( + r#"{{"jsonrpc":"2.0","id":{id_raw},"result":{{"tools":[{}]}}}}"#, + entries.join(",") + ) +} diff --git a/crates/op-host-services/src/web_canvas_server/connection.rs b/crates/op-host-services/src/web_canvas_server/connection.rs index 0f8ce48a9..babe5b3ca 100644 --- a/crates/op-host-services/src/web_canvas_server/connection.rs +++ b/crates/op-host-services/src/web_canvas_server/connection.rs @@ -21,6 +21,10 @@ pub(super) struct ConnCtx<'a> { pub(super) state: &'a Mutex, pub(super) hub: &'a SseHub, pub(super) mode: ServeMode, + /// Which MCP tools this connection may see and call. `UNRESTRICTED` for + /// the local and managed daemons — the whole catalog, full authority, + /// exactly as before capability profiles existed. + pub(super) mcp_profile: crate::mcp_serve::tool_profile::McpAccessProfile, } /// Handle one connection against the single-user document authority. @@ -64,7 +68,16 @@ pub(super) fn serve_one_in_mode( mode: ServeMode, ) -> Result { let req = crate::mcp_serve::read_http_request(stream)?; - dispatch(stream, &req, &ConnCtx { state, hub, mode }) + dispatch( + stream, + &req, + &ConnCtx { + state, + hub, + mode, + mcp_profile: crate::mcp_serve::tool_profile::McpAccessProfile::UNRESTRICTED, + }, + ) } /// Route one already-parsed request against `ctx`. @@ -397,9 +410,10 @@ pub(super) fn dispatch( // call against a headless `op start` daemon also relays the // radar-scan to the browser shell) is tracked as follow-up // scope, not part of this pass. - let response = crate::mcp_serve::process_message_with_applier( + let response = crate::mcp_serve::process_message_with_applier_profiled( &mut guard.editor, &req.body, + ctx.mcp_profile, |_tool_name, editor, cmd| { if let Err(reason) = policy.check_command(cmd, op_editor_core::CollabEditSource::Mcp) diff --git a/crates/op-host-services/src/web_canvas_server/hub_verifier.rs b/crates/op-host-services/src/web_canvas_server/hub_verifier.rs index 5f14407d2..2d853f8a4 100644 --- a/crates/op-host-services/src/web_canvas_server/hub_verifier.rs +++ b/crates/op-host-services/src/web_canvas_server/hub_verifier.rs @@ -15,6 +15,7 @@ use crate::hub_auth_client::{HubAuthClient, HubToken, HubUser}; use crate::hub_auth_error::HubAuthError; +use crate::mcp_serve::tool_profile::McpScopes; use super::tenant_auth::{ IdentityVerifier, IdentityVia, OnlineAuthError, PresentedCredentials, ResolvedIdentity, @@ -71,27 +72,31 @@ fn identity_from_user(user: HubUser) -> ResolvedIdentity { username: user.username, display_name, via: IdentityVia::SessionCookie, + // A browser session IS the account, so it carries the account's own + // authority. Scopes exist to narrow an API token below that. + scopes: McpScopes::FULL, } } /// An API token's account. /// -/// Introspection carries no display name, so the username stands in. Scopes -/// are deliberately dropped here: M3 enforces them in the MCP dispatch, where -/// the tool being called is known, and smuggling them through the identity -/// type would invite a caller of `ResolvedIdentity` to assume they were -/// already checked. +/// Introspection carries no display name, so the username stands in. The +/// hub's scope list is reduced to the MCP capabilities here and enforced in +/// the MCP dispatch, where the tool being called — and therefore whether it +/// writes — is known. fn identity_from_token(token: HubToken) -> ResolvedIdentity { let username = if token.username.trim().is_empty() { token.user_id.clone() } else { token.username }; + let scopes = McpScopes::from_scope_list(&token.scopes); ResolvedIdentity { user_id: token.user_id, display_name: username.clone(), username, via: IdentityVia::ApiToken, + scopes, } } @@ -143,6 +148,30 @@ mod tests { assert_eq!(identity.username, "person_name"); assert_eq!(identity.display_name, "Person"); assert_eq!(identity.via, IdentityVia::SessionCookie); + assert_eq!(identity.scopes, McpScopes::FULL); + } + + #[test] + fn a_token_scope_list_narrows_what_it_may_drive() { + let read_only = identity_from_token(HubToken { + scopes: vec!["mcp:read".into()], + ..token() + }); + assert!(!read_only.scopes.can_write()); + + let both = identity_from_token(HubToken { + scopes: vec!["mcp:read".into(), "mcp:write".into()], + ..token() + }); + assert!(both.scopes.can_write()); + + // A token that names no mcp scope at all predates scoping and keeps + // full authority; see `McpScopes::from_scope_list`. + let unscoped = identity_from_token(HubToken { + scopes: vec!["billing:read".into()], + ..token() + }); + assert!(unscoped.scopes.can_write()); } #[test] diff --git a/crates/op-host-services/src/web_canvas_server/online_mcp_tests.rs b/crates/op-host-services/src/web_canvas_server/online_mcp_tests.rs new file mode 100644 index 000000000..3d42f8eb6 --- /dev/null +++ b/crates/op-host-services/src/web_canvas_server/online_mcp_tests.rs @@ -0,0 +1,218 @@ +//! The online MCP capability profile, exercised end to end through the +//! accept loop. +//! +//! Split out of `online_run_loop_tests.rs` at the 800-line cap; nested under +//! it so `use super::*` still reaches the request builder, the mock stream, +//! and the tenant/verifier helpers. + +use super::*; + +/// `tokR` is read-only; `tokA`/`tokB` carry full authority. +fn scoped_verifier() -> StaticVerifier { + StaticVerifier::parse("tokA=userA,tokB=userB,tokR=userR:read") +} + +fn mcp_call(tool: &str, arguments: &str) -> String { + format!( + r#"{{"jsonrpc":"2.0","id":7,"method":"tools/call","params":{{"name":"{tool}","arguments":{arguments}}}}}"# + ) +} + +/// Drive one JSON-RPC message at `/mcp` and return the decoded body. +fn mcp( + registry: &TenantRegistry, + verifier: &StaticVerifier, + token: &'static str, + message: &str, +) -> serde_json::Value { + let response = serve( + registry, + verifier, + Request::json("POST", "/mcp", message).with_bearer(token), + ); + assert_eq!( + status_line(&response), + "HTTP/1.1 200 OK", + "a refusal is still a 200 JSON-RPC envelope: {response}" + ); + body(&response) +} + +const TOOLS_LIST: &str = r#"{"jsonrpc":"2.0","id":1,"method":"tools/list"}"#; + +/// The text block of a `tools/call` response. +fn call_text(payload: &serde_json::Value) -> String { + payload["result"]["content"][0]["text"] + .as_str() + .unwrap_or_default() + .to_string() +} + +#[test] +fn the_online_tool_catalog_omits_every_local_resource_tool() { + let listed = mcp(®istry(), &verifier(), "tokA", TOOLS_LIST); + let names: Vec<&str> = listed["result"]["tools"] + .as_array() + .expect("a tools array") + .iter() + .filter_map(|tool| tool["name"].as_str()) + .collect(); + assert!(!names.is_empty(), "the catalog must not be empty"); + for denied in crate::mcp_serve::tool_profile::denied_tool_names() { + assert!( + !names.contains(&denied), + "{denied} must not be advertised to a public client" + ); + } + // The in-memory catalog is still there — this is a filter, not a shutdown. + for kept in ["get_node", "add_page", "insert_node", "batch_design"] { + assert!(names.contains(&kept), "{kept} must still be offered"); + } +} + +#[test] +fn calling_a_denied_tool_by_name_is_refused_rather_than_executed() { + let payload = mcp( + ®istry(), + &verifier(), + "tokA", + &mcp_call( + "save_document", + r#"{"filePath":"/tmp/op-m3-should-not-exist.op"}"#, + ), + ); + assert_eq!(payload["result"]["isError"], true, "{payload}"); + let text = call_text(&payload); + assert!(text.contains("tool-not-available"), "{text}"); + assert!(text.contains("save_document"), "{text}"); + // Filtering the catalog is discovery; this is the enforcement. + assert!( + !std::path::Path::new("/tmp/op-m3-should-not-exist.op").exists(), + "a denied tool must never reach the code that writes the path" + ); +} + +#[test] +fn a_path_traversal_argument_on_a_denied_tool_never_reaches_the_filesystem() { + let target = std::env::temp_dir().join("op-m3-traversal-probe.op"); + let _ = std::fs::remove_file(&target); + let payload = mcp( + ®istry(), + &verifier(), + "tokA", + &mcp_call( + "save_document", + &serde_json::json!({ + "filePath": format!("../../../../../../{}", target.display()), + }) + .to_string(), + ), + ); + assert_eq!(payload["result"]["isError"], true, "{payload}"); + assert!( + !target.exists(), + "the deny layer runs before the tool parses its path, so traversal is moot" + ); +} + +#[test] +fn every_denied_tool_is_refused_when_called_directly() { + let registry = registry(); + let verifier = verifier(); + for denied in crate::mcp_serve::tool_profile::denied_tool_names() { + let payload = mcp(®istry, &verifier, "tokA", &mcp_call(denied, "{}")); + assert_eq!(payload["result"]["isError"], true, "{denied}: {payload}"); + assert!( + call_text(&payload).contains("tool-not-available"), + "{denied}: {}", + call_text(&payload) + ); + } +} + +#[test] +fn a_read_scope_token_may_read_but_not_write() { + let registry = registry(); + let verifier = scoped_verifier(); + + let read = mcp( + ®istry, + &verifier, + "tokR", + &mcp_call("get_document_info", "{}"), + ); + assert_ne!( + read["result"]["isError"], true, + "a read must succeed: {read}" + ); + + let write = mcp( + ®istry, + &verifier, + "tokR", + &mcp_call("add_page", r#"{"name":"nope"}"#), + ); + assert_eq!(write["result"]["isError"], true, "{write}"); + assert!( + call_text(&write).contains("scope-insufficient"), + "{}", + call_text(&write) + ); +} + +#[test] +fn a_full_scope_token_may_write_the_same_tool_a_read_token_cannot() { + let registry = registry(); + let verifier = scoped_verifier(); + let write = mcp( + ®istry, + &verifier, + "tokA", + &mcp_call("add_page", r#"{"name":"ok"}"#), + ); + assert_ne!(write["result"]["isError"], true, "{write}"); +} + +#[test] +fn a_read_scope_token_is_refused_the_write_before_the_document_changes() { + let registry = registry(); + let verifier = scoped_verifier(); + let before = mcp( + ®istry, + &verifier, + "tokR", + &mcp_call("get_document_info", "{}"), + ); + let _ = mcp( + ®istry, + &verifier, + "tokR", + &mcp_call("add_page", r#"{"name":"nope"}"#), + ); + let after = mcp( + ®istry, + &verifier, + "tokR", + &mcp_call("get_document_info", "{}"), + ); + assert_eq!( + call_text(&before), + call_text(&after), + "a scope refusal must not have mutated anything" + ); +} + +#[test] +fn the_local_daemon_still_lists_and_calls_the_whole_catalog() { + // The other half of the contract: none of the above may leak into the + // single-user daemon, whose operator owns the filesystem it writes. + let profile = crate::mcp_serve::tool_profile::McpAccessProfile::UNRESTRICTED; + for denied in crate::mcp_serve::tool_profile::denied_tool_names() { + assert!(profile.lists(denied), "{denied} must stay listed locally"); + assert_eq!( + profile.refuse(denied), + None, + "{denied} must stay callable locally" + ); + } +} diff --git a/crates/op-host-services/src/web_canvas_server/online_run_loop.rs b/crates/op-host-services/src/web_canvas_server/online_run_loop.rs index bc1b9e1f5..5739d6266 100644 --- a/crates/op-host-services/src/web_canvas_server/online_run_loop.rs +++ b/crates/op-host-services/src/web_canvas_server/online_run_loop.rs @@ -294,6 +294,9 @@ pub(super) fn serve_one_online( state: lease.state(), hub: lease.hub(), mode: ServeMode::Online, + // The public tool profile, narrowed further by whatever scopes + // this particular credential carries. + mcp_profile: crate::mcp_serve::tool_profile::McpAccessProfile::online(identity.scopes), }, ) } diff --git a/crates/op-host-services/src/web_canvas_server/online_run_loop_tests.rs b/crates/op-host-services/src/web_canvas_server/online_run_loop_tests.rs index 539aa91f0..862ca0f04 100644 --- a/crates/op-host-services/src/web_canvas_server/online_run_loop_tests.rs +++ b/crates/op-host-services/src/web_canvas_server/online_run_loop_tests.rs @@ -743,3 +743,7 @@ fn a_disallowed_origin_gets_no_cors_header_at_all() { "omitting the header is what makes the browser withhold the body: {response}" ); } + +#[cfg(test)] +#[path = "online_mcp_tests.rs"] +mod mcp_profile; diff --git a/crates/op-host-services/src/web_canvas_server/tenant_auth.rs b/crates/op-host-services/src/web_canvas_server/tenant_auth.rs index 548f32327..4cba4f85b 100644 --- a/crates/op-host-services/src/web_canvas_server/tenant_auth.rs +++ b/crates/op-host-services/src/web_canvas_server/tenant_auth.rs @@ -12,6 +12,7 @@ use std::collections::HashMap; +use crate::mcp_serve::tool_profile::McpScopes; use crate::mcp_serve::HttpRequest; /// Development-only token table: `token1=user1,token2=user2`. @@ -45,6 +46,13 @@ pub struct ResolvedIdentity { pub username: String, pub display_name: String, pub via: IdentityVia, + /// What this credential may drive over MCP. + /// + /// A browser session is the account itself and carries full authority; an + /// API token carries whatever the hub issued it. Enforced in the MCP + /// dispatch, where the tool being called is known — see + /// `crate::mcp_serve::tool_profile`. + pub scopes: McpScopes, } /// Why a request could not be attributed to an account. @@ -149,8 +157,8 @@ pub trait IdentityVerifier: Send + Sync { /// with `--online` and no real verifier is a misconfiguration, and the /// online run loop says so on stderr at start-up. pub struct StaticVerifier { - /// token → user id. - entries: HashMap, + /// token → (user id, scopes). + entries: HashMap, } impl StaticVerifier { @@ -163,7 +171,12 @@ impl StaticVerifier { ) } - /// Parse a `token=user,token2=user2` table. + /// Parse a `token=user[,token2=user2:read]` table. + /// + /// The optional `:read` suffix on the account mints a read-only + /// credential, so the scope path can be exercised without a hub. Any + /// other suffix is rejected rather than silently granting write — a typo + /// must not widen authority. /// /// Malformed pairs are skipped rather than failing the whole table: the /// failure mode of a dropped entry is "that token does not authenticate", @@ -172,13 +185,17 @@ impl StaticVerifier { let entries = raw .split(',') .filter_map(|pair| { - let (token, user) = pair.split_once('=')?; + let (token, account) = pair.split_once('=')?; let token = token.trim(); - let user = user.trim(); + let (user, scopes) = match account.trim().rsplit_once(':') { + Some((user, "read")) => (user.trim(), McpScopes::READ_ONLY), + Some((_, _)) => return None, + None => (account.trim(), McpScopes::FULL), + }; (!token.is_empty() && !user.is_empty() && token.chars().count() <= MAX_CREDENTIAL_CHARS) - .then(|| (token.to_string(), user.to_string())) + .then(|| (token.to_string(), (user.to_string(), scopes))) }) .collect(); Self { entries } @@ -211,7 +228,7 @@ impl IdentityVerifier for StaticVerifier { if credential.chars().count() > MAX_CREDENTIAL_CHARS { return Err(OnlineAuthError::MalformedCredential); } - let user_id = self + let (user_id, scopes) = self .entries .get(credential.as_str()) .ok_or(OnlineAuthError::UnknownCredential)?; @@ -220,6 +237,12 @@ impl IdentityVerifier for StaticVerifier { username: user_id.clone(), display_name: user_id.clone(), via, + // A browser session is the account itself, so it carries full + // authority however the token table classified the same string. + scopes: match via { + IdentityVia::SessionCookie => McpScopes::FULL, + IdentityVia::ApiToken => *scopes, + }, }) } } diff --git a/crates/op-host-services/src/web_canvas_server/tenant_tests.rs b/crates/op-host-services/src/web_canvas_server/tenant_tests.rs index 57ec05f09..d73e22091 100644 --- a/crates/op-host-services/src/web_canvas_server/tenant_tests.rs +++ b/crates/op-host-services/src/web_canvas_server/tenant_tests.rs @@ -11,6 +11,7 @@ fn identity(user_id: &str) -> ResolvedIdentity { username: user_id.into(), display_name: user_id.into(), via: IdentityVia::ApiToken, + scopes: crate::mcp_serve::tool_profile::McpScopes::FULL, } } diff --git a/tools/check-collab-deployment-boundaries.sh b/tools/check-collab-deployment-boundaries.sh index d6696ce9f..9448a024e 100644 --- a/tools/check-collab-deployment-boundaries.sh +++ b/tools/check-collab-deployment-boundaries.sh @@ -1,6 +1,33 @@ # Deployment-specific collaboration security boundaries. # Sourced by check-collab-security-boundaries.sh after its shared assertions # and failure accumulator have been initialized. +# +# This file is a FRAGMENT, not a runnable gate. Every assertion below is a +# function the parent defines, so running this file directly used to print +# "require_file: command not found" once per assertion and then exit 0 — a +# green result from a script that checked nothing. Anyone who ran it by hand, +# or wired it into CI as its own step, got that silent pass. +# +# The guard below makes the mistake loud. It also refuses to run when the +# parent has not initialized the failure accumulator, so a future reordering +# of the parent cannot reintroduce the same hole. + +for _boundary_helper in \ + require_file \ + require_executable \ + require_literal \ + require_literal_count \ + record_failure; do + if ! command -v "$_boundary_helper" >/dev/null 2>&1; then + printf '%s\n' \ + "check-collab-deployment-boundaries.sh is not a standalone gate." \ + "It must be sourced by check-collab-security-boundaries.sh, which" \ + "defines the assertions it uses (missing: $_boundary_helper)." \ + "Run instead: bash tools/check-collab-security-boundaries.sh" >&2 + exit 2 + fi +done +unset _boundary_helper for required in \ .dockerignore \