From 4419ab0150ff648e89dd388b0b75811bb582ae02 Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 30 Sep 2026 16:15:23 -0400 Subject: [PATCH] fix(profile): classify every tool; boundary tests fail on unclassified ones (#45) The shell-capable list was hand-maintained: a new tool (say clipboard_write) would be scanned but treated as safe. Every served tool now has a class (observe / act / shell) and the tests fail on an unclassified tool, on a non-shell-equivalent profile allowing a shell tool, and on observe holding anything but observe tools. macOS: ToolCatalog is the single list main.swift serves and the tests check (source scan kept as a cross-check). Linux: same classes and checks over tool_list. observe now requires allowlist AND observe class at runtime. Docs: observe protects the computer, not the agent (screenshots can carry prompt injection). Mutation-checked on both sides. --- Sources/InstanceMCPCore/ToolCatalog.swift | 11 ++ Sources/InstanceMCPCore/ToolProfile.swift | 40 ++++++- Sources/oab-instance-mcp/main.swift | 2 +- .../ProfileBoundaryTests.swift | 58 +++++---- poc/reverse-attach-linux/src/mcp.rs | 111 +++++++++++++++++- 5 files changed, 196 insertions(+), 26 deletions(-) create mode 100644 Sources/InstanceMCPCore/ToolCatalog.swift diff --git a/Sources/InstanceMCPCore/ToolCatalog.swift b/Sources/InstanceMCPCore/ToolCatalog.swift new file mode 100644 index 0000000..f10a65c --- /dev/null +++ b/Sources/InstanceMCPCore/ToolCatalog.swift @@ -0,0 +1,11 @@ +import Foundation + +/// Every local tool this daemon registers, in one place. `main.swift` serves this +/// list and `ProfileBoundaryTests` checks it, so a tool cannot be served without +/// also being classified (`ToolProfile.localToolClass`). +public enum ToolCatalog { + public static func local(agentVersion: String) -> [any Tool] { + [SysInfoTool(agentVersion: agentVersion), ExecTool(), ExecStartTool(), ExecPollTool(), + ExecListTool(), ExecCancelTool(), ScreenshotTool(), MouseTool(), KeyTool(), OsascriptTool()] + } +} diff --git a/Sources/InstanceMCPCore/ToolProfile.swift b/Sources/InstanceMCPCore/ToolProfile.swift index 3b96476..c612778 100644 --- a/Sources/InstanceMCPCore/ToolProfile.swift +++ b/Sources/InstanceMCPCore/ToolProfile.swift @@ -44,9 +44,39 @@ public enum ToolProfile: String, Codable, Sendable, CaseIterable { /// upstream — is denied under `observe` until it is listed here. public static let observeTools: Set = ["sys_info", "screenshot"] - /// Local tools that reach a shell as the desktop user, directly or by driving - /// the GUI. Any profile with `isShellEquivalent == false` must allow none. - public static let shellCapableTools: Set = ["osascript", "key", "mouse"] + /// What a tool can do, as far as a boundary is concerned. + public enum ToolClass: String, Sendable, CaseIterable { + /// Reads only; changes nothing. + case observe + /// Changes state, but cannot run code as the desktop user. + case act + /// Reaches the desktop user's shell, directly or by driving the GUI. + case shell + } + + /// Every local tool, classified. **Exhaustive by test**: a tool in + /// `ToolCatalog.local` without an entry here fails `ProfileBoundaryTests`, + /// so a new tool cannot slip past the boundary check by simply not being + /// listed as dangerous. Classify conservatively: if it can open, type into, + /// or script anything that runs code, it is `.shell`. + public static let localToolClass: [String: ToolClass] = [ + "sys_info": .observe, + "screenshot": .observe, + "exec_poll": .observe, // reads a job's state and output; starts nothing + "exec_list": .observe, + "exec_cancel": .act, // signals a job's process group + "exec": .shell, + "exec_start": .shell, + "osascript": .shell, // `do shell script`, JXA, NSTask + "key": .shell, // can type into a terminal + "mouse": .shell, // can open one + ] + + /// Local tools that reach a shell as the desktop user. Any profile with + /// `isShellEquivalent == false` must allow none. + public static var shellCapableTools: Set { + Set(localToolClass.filter { $0.value == .shell }.keys) + } /// Match is on the tool name. public func allows(_ toolName: String) -> Bool { @@ -57,7 +87,9 @@ public enum ToolProfile: String, Codable, Sendable, CaseIterable { if toolName.hasPrefix("browser_") { return Self.desktopBrowserTools.contains(toolName) } return true case .observe: - return Self.observeTools.contains(toolName) + // Both: on the allowlist *and* classified as observation, so a tool added + // to the allowlist by mistake is still refused unless it changes nothing. + return Self.observeTools.contains(toolName) && Self.localToolClass[toolName] == .observe } } diff --git a/Sources/oab-instance-mcp/main.swift b/Sources/oab-instance-mcp/main.swift index 270bbca..d050e26 100644 --- a/Sources/oab-instance-mcp/main.swift +++ b/Sources/oab-instance-mcp/main.swift @@ -137,7 +137,7 @@ let server = MCPServer( no screenshots. Fall back to `screenshot` only for things outside the browser. """) """, - tools: [SysInfoTool(agentVersion: version), ExecTool(), ExecStartTool(), ExecPollTool(), ExecListTool(), ExecCancelTool(), ScreenshotTool(), MouseTool(), KeyTool(), OsascriptTool()], + tools: ToolCatalog.local(agentVersion: version), upstreams: opts.upstreams.map { UpstreamMCP(name: $0.0, url: $0.1, log: log) } ) let attachManager: AttachManager? = opts.attach diff --git a/Tests/InstanceMCPCoreTests/ProfileBoundaryTests.swift b/Tests/InstanceMCPCoreTests/ProfileBoundaryTests.swift index bde1dbe..496d817 100644 --- a/Tests/InstanceMCPCoreTests/ProfileBoundaryTests.swift +++ b/Tests/InstanceMCPCoreTests/ProfileBoundaryTests.swift @@ -8,16 +8,19 @@ import XCTest /// that hides `exec*` but keeps those is not narrower in privilege, only in /// convenience. These tests make that impossible to claim by accident. final class ProfileBoundaryTests: XCTestCase { - /// Every local tool this daemon ships, by name, straight from the source so a - /// new tool is covered without anyone remembering to list it here. - private func shippedLocalToolNames() throws -> Set { + /// The tools the daemon actually serves (the same list `main.swift` uses). + private var served: [String] { ToolCatalog.local(agentVersion: "test").map(\.name) } + + /// Tool names declared in the Tools sources, as a cross-check that nothing is + /// served from outside the catalog. + private func declaredToolNames() throws -> Set { let dir = URL(fileURLWithPath: #filePath) .deletingLastPathComponent().deletingLastPathComponent().deletingLastPathComponent() .appendingPathComponent("Sources/InstanceMCPCore/Tools") var names = Set() + let regex = try NSRegularExpression(pattern: #"\bname\s*=\s*"([a-z_]+)""#) for file in try FileManager.default.contentsOfDirectory(atPath: dir.path) where file.hasSuffix(".swift") { let text = try String(contentsOf: dir.appendingPathComponent(file), encoding: .utf8) - let regex = try NSRegularExpression(pattern: #"let name = "([a-z_]+)""#) for m in regex.matches(in: text, range: NSRange(text.startIndex..., in: text)) { if let r = Range(m.range(at: 1), in: text) { names.insert(String(text[r])) } } @@ -25,23 +28,33 @@ final class ProfileBoundaryTests: XCTestCase { return names } - func testTheToolScanSeesTheShellCapableTools() throws { - let shipped = try shippedLocalToolNames() - for tool in ["exec", "osascript", "key", "mouse", "screenshot", "sys_info"] { - XCTAssertTrue(shipped.contains(tool), "tool scan missed \(tool); shipped: \(shipped.sorted())") - } + /// The fix for "the dangerous list is hand-maintained": classification is + /// exhaustive. A new tool that nobody classified fails here instead of being + /// silently treated as safe. + func testEveryServedToolIsClassified() { + let unclassified = served.filter { ToolProfile.localToolClass[$0] == nil } + XCTAssertTrue(unclassified.isEmpty, + "classify in ToolProfile.localToolClass (observe / act / shell): \(unclassified.sorted())") + let stale = Set(ToolProfile.localToolClass.keys).subtracting(served) + XCTAssertTrue(stale.isEmpty, "classified but not served: \(stale.sorted())") + XCTAssertEqual(served.count, Set(served).count, "duplicate tool names") + } + + func testEveryDeclaredToolIsInTheCatalog() throws { + let declared = try declaredToolNames() + XCTAssertTrue(declared.isSuperset(of: ["exec", "osascript", "key", "mouse", "screenshot", "sys_info"]), + "source scan broke: \(declared.sorted())") + let outside = declared.subtracting(served) + XCTAssertTrue(outside.isEmpty, "declared but not in ToolCatalog.local: \(outside.sorted())") } /// The adversary check: a profile that does not declare itself shell-equivalent - /// must allow no tool that reaches a shell. Today no such profile exists and - /// `desktop` must say so; a future `observe` / `browser` profile is held to it. - func testNoProfileClaimsToBeNarrowerThanItIs() throws { - let shellReaching = try shippedLocalToolNames().filter { - $0.hasPrefix("exec") || ToolProfile.shellCapableTools.contains($0) - } - XCTAssertFalse(shellReaching.isEmpty) + /// must allow no tool classified `.shell`; one that does must actually allow one. + func testNoProfileClaimsToBeNarrowerThanItIs() { + let shell = served.filter { ToolProfile.localToolClass[$0] == .shell } + XCTAssertFalse(shell.isEmpty) for profile in ToolProfile.allCases { - let reachable = shellReaching.filter(profile.allows) + let reachable = shell.filter(profile.allows) if !profile.isShellEquivalent { XCTAssertTrue(reachable.isEmpty, "\(profile.rawValue) claims no shell but allows \(reachable.sorted())") @@ -51,6 +64,12 @@ final class ProfileBoundaryTests: XCTestCase { } } + /// `observe` may only hold tools that change nothing. + func testObserveAllowsOnlyObserveClassTools() { + let wrong = served.filter { ToolProfile.observe.allows($0) && ToolProfile.localToolClass[$0] != .observe } + XCTAssertTrue(wrong.isEmpty, "observe allows tools that act: \(wrong.sorted())") + } + func testDesktopIsDeclaredShellEquivalent() { XCTAssertTrue(ToolProfile.desktop.isShellEquivalent) for tool in ["osascript", "key", "mouse"] { @@ -60,10 +79,9 @@ final class ProfileBoundaryTests: XCTestCase { } /// `observe` is the one real boundary today: look, never act. - func testObserveCanOnlyLook() throws { + func testObserveCanOnlyLook() { XCTAssertFalse(ToolProfile.observe.isShellEquivalent) - let shipped = try shippedLocalToolNames() - XCTAssertEqual(Set(shipped.filter(ToolProfile.observe.allows)), ["sys_info", "screenshot"]) + XCTAssertEqual(Set(served.filter(ToolProfile.observe.allows)), ["sys_info", "screenshot"]) for tool in ["browser_navigate", "browser_snapshot", "browser_take_screenshot", "browser_something_new", "exec", "osascript"] { XCTAssertFalse(ToolProfile.observe.allows(tool), "\(tool) is an action, or unknown") } diff --git a/poc/reverse-attach-linux/src/mcp.rs b/poc/reverse-attach-linux/src/mcp.rs index b68058b..0822d79 100644 --- a/poc/reverse-attach-linux/src/mcp.rs +++ b/poc/reverse-attach-linux/src/mcp.rs @@ -185,11 +185,54 @@ pub(crate) fn upstream_tool_allowed(name: &str, profile: &str) -> bool { /// Local tools `observe` may call: look, never act (instance-mcp#45). pub(crate) const OBSERVE_TOOLS: &[&str] = &["sys_info", "screenshot"]; +/// What a tool can do, as far as a boundary is concerned (same classes as the +/// Swift `ToolProfile.ToolClass`). +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum ToolClass { + /// Reads only; changes nothing. + Observe, + /// Changes state, but cannot run code as the desktop user. + #[allow(dead_code)] + Act, + /// Reaches the desktop user's shell, directly or by driving the GUI. + #[cfg_attr(not(test), allow(dead_code))] + Shell, +} + +/// Every local tool, classified. Exhaustive by test: a tool in `LOCAL_TOOL_NAMES` +/// (or served by `tool_list`) without an entry here fails `profile_tests`, so a new +/// tool cannot pass the boundary check just by not being listed as dangerous. +pub(crate) const LOCAL_TOOL_CLASS: &[(&str, ToolClass)] = &[ + ("sys_info", ToolClass::Observe), + ("screenshot", ToolClass::Observe), + ("bash", ToolClass::Shell), + ("mouse", ToolClass::Shell), // can open a terminal + ("key", ToolClass::Shell), // can type into one +]; + +pub(crate) fn local_tool_class(name: &str) -> Option { + LOCAL_TOOL_CLASS + .iter() + .find(|(n, _)| *n == name) + .map(|(_, c)| *c) +} + +/// Whether a profile honestly grants the node user's shell (instance-mcp#45). +#[cfg(test)] +pub(crate) fn profile_is_shell_equivalent(profile: &str) -> bool { + matches!(normalize_profile(profile), Some("owner") | Some("desktop")) +} + /// Whether a local tool is visible/callable under `profile`. pub(crate) fn local_tool_allowed(name: &str, profile: &str) -> bool { match normalize_profile(profile) { Some("owner") | Some("desktop") => true, - Some("observe") => OBSERVE_TOOLS.contains(&name), + // Both: on the allowlist *and* classified as observation. A tool added to the + // allowlist by mistake is still refused unless it was also classified as + // changing nothing. + Some("observe") => { + OBSERVE_TOOLS.contains(&name) && local_tool_class(name) == Some(ToolClass::Observe) + } _ => false, } } @@ -433,6 +476,72 @@ mod profile_tests { assert!(upstream_tool_allowed("browser_navigate", "desktop")); } + /// Local tool names as actually served under `profile` (no upstream in tests). + fn served(profile: &str) -> Vec { + tool_list(profile) + .as_array() + .unwrap() + .iter() + .map(|t| t["name"].as_str().unwrap().to_owned()) + .filter(|n| !n.starts_with("browser_")) + .collect() + } + + #[test] + fn every_served_tool_is_classified_and_the_lists_agree() { + let served = served("owner"); + let mut sorted_served = served.clone(); + sorted_served.sort(); + let mut names: Vec = LOCAL_TOOL_NAMES.iter().map(|s| s.to_string()).collect(); + names.sort(); + assert_eq!( + sorted_served, names, + "tool_list and LOCAL_TOOL_NAMES disagree" + ); + let unclassified: Vec<_> = served + .iter() + .filter(|n| local_tool_class(n).is_none()) + .collect(); + assert!( + unclassified.is_empty(), + "classify in LOCAL_TOOL_CLASS: {unclassified:?}" + ); + let stale: Vec<_> = LOCAL_TOOL_CLASS + .iter() + .filter(|(n, _)| !served.iter().any(|s| s == n)) + .collect(); + assert!(stale.is_empty(), "classified but not served: {stale:?}"); + } + + #[test] + fn no_profile_claims_to_be_narrower_than_it_is() { + for profile in ["owner", "desktop", "observe"] { + let shell: Vec = served(profile) + .into_iter() + .filter(|n| local_tool_class(n) == Some(ToolClass::Shell)) + .collect(); + if profile_is_shell_equivalent(profile) { + assert!( + !shell.is_empty(), + "{profile} is marked shell-equivalent; keep it honest" + ); + } else { + assert!( + shell.is_empty(), + "{profile} claims no shell but serves {shell:?}" + ); + let acting: Vec = served(profile) + .into_iter() + .filter(|n| local_tool_class(n) != Some(ToolClass::Observe)) + .collect(); + assert!( + acting.is_empty(), + "{profile} serves tools that act: {acting:?}" + ); + } + } + } + #[test] fn observe_can_only_look() { for tool in ["sys_info", "screenshot"] {