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 \