From c2dc6d8b567f12b4d80f005f04b1fa8281207128 Mon Sep 17 00:00:00 2001 From: Halvor Reiten Date: Fri, 9 Oct 2026 15:31:08 +0200 Subject: [PATCH 1/3] feat: let admins choose which MCP server tools agents can use --- CHANGELOG.md | 10 + docs/CLAUDE_AGENT.md | 4 +- docs/HOW_IT_WORKS.md | 35 +++ package-lock.json | 97 ++++-- packages/control-plane/package.json | 1 + .../control-plane/src/db/mcp-servers.test.ts | 84 +++++ packages/control-plane/src/db/mcp-servers.ts | 54 +++- .../src/mcp/tool-discovery.test.ts | 290 +++++++++++++++++ .../control-plane/src/mcp/tool-discovery.ts | 150 +++++++++ .../control-plane/src/router.policy.test.ts | 12 +- .../control-plane/src/routes/mcp-servers.ts | 54 ++++ ...ono-route-catalog-conformance.test.ts.snap | 49 +-- .../route-admission-matrix.test.ts.snap | 2 + .../hono-route-catalog-conformance.test.ts | 2 +- .../test/integration/mcp-servers.test.ts | 114 +++++++ .../vitest.integration.config.ts | 54 ++++ .../content/docs/configure/mcp-servers.mdx | 52 +++- .../content/docs/models/agent-harnesses.mdx | 4 +- .../tests/test_web_api_create_sandbox.py | 23 ++ .../src/sandbox_runtime/harness/claude.py | 24 +- .../src/sandbox_runtime/mcp_tool_access.py | 189 ++++++++++++ .../src/sandbox_runtime/opencode_server.py | 30 ++ .../src/sandbox_runtime/runtime_manifest.json | 10 +- .../src/sandbox_runtime/types.py | 1 + .../tests/fixtures/mcp_stdio_server.py | 43 +++ .../tests/test_claude_harness.py | 80 +++++ packages/sandbox-runtime/tests/test_mcp.py | 69 +++++ .../tests/test_mcp_tool_access.py | 203 ++++++++++++ .../tests/test_opencode_reasoning_contract.py | 55 ++++ .../shared/src/types/integrations.test.ts | 34 ++ packages/shared/src/types/integrations.ts | 21 ++ .../app/api/mcp-servers/[id]/tools/route.ts | 23 ++ .../settings/mcp-servers-settings.test.tsx | 227 +++++++++++++- .../settings/mcp-servers-settings.tsx | 52 ++++ .../components/settings/mcp-tool-access.tsx | 291 ++++++++++++++++++ packages/web/src/hooks/use-mcp-servers.ts | 15 + .../0086_mcp_server_tool_allowlist.sql | 1 + 37 files changed, 2388 insertions(+), 71 deletions(-) create mode 100644 packages/control-plane/src/mcp/tool-discovery.test.ts create mode 100644 packages/control-plane/src/mcp/tool-discovery.ts create mode 100644 packages/sandbox-runtime/src/sandbox_runtime/mcp_tool_access.py create mode 100644 packages/sandbox-runtime/tests/fixtures/mcp_stdio_server.py create mode 100644 packages/sandbox-runtime/tests/test_mcp_tool_access.py create mode 100644 packages/web/src/app/api/mcp-servers/[id]/tools/route.ts create mode 100644 packages/web/src/components/settings/mcp-tool-access.tsx create mode 100644 terraform/d1/migrations/0086_mcp_server_tool_allowlist.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index 496d937b48..de108c8cef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,16 @@ New features, integrations, and notable improvements to Open-Inspect — newest first. +## October 9, 2026 + +**MCP tool selection.** Choose which of an MCP server's tools agents can use under **Settings > MCP +Servers**. Set a server to **Selected tools only**, click **Load tools** to list what a remote +server offers, and tick the tools to keep; for a local server, type tool names. OpenCode leaves the +other tools out of every request, so a large server no longer fills the context window. Claude Agent +allows only the selected tools and refuses the rest. Servers default to **All tools**, so existing +configurations are unchanged. Requires D1 migration 0086 and a sandbox image rebuild. See +[MCP tool allowlists](docs/HOW_IT_WORKS.md#mcp-tool-allowlists). + ## October 8, 2026 **Claude Agent for Slack sessions.** Choose OpenCode or Claude Agent for new Slack sessions under diff --git a/docs/CLAUDE_AGENT.md b/docs/CLAUDE_AGENT.md index a2bcb2c160..4bfdf73586 100644 --- a/docs/CLAUDE_AGENT.md +++ b/docs/CLAUDE_AGENT.md @@ -276,7 +276,9 @@ fix instead. - **Tools.** Open-Inspect's own tools are served to the Claude harness in-process as the `oi` MCP server: child sessions and `upload-media` always, `create-pull-request` when the session has a repository, and `slack-notify` when agent notifications are enabled for the repository. Session - MCP servers are passed through unchanged. + MCP servers are passed through, with a server's tool allowlist applied: only its selected tools + are allowed (`mcp____`). The control plane leaves out a server with an empty + allowlist. - **Sub-agents.** The child runs with `CLAUDE_CODE_DISABLE_BACKGROUND_TASKS=1`, so an `Agent` tool call returns only when its sub-agent has finished, and several sub-agents launched in one message still run concurrently. This is the same contract as OpenCode's `task` tool, and the timeline diff --git a/docs/HOW_IT_WORKS.md b/docs/HOW_IT_WORKS.md index 1832bc6322..c091d00064 100644 --- a/docs/HOW_IT_WORKS.md +++ b/docs/HOW_IT_WORKS.md @@ -404,6 +404,41 @@ reports preparation time on misses. This optimization removes redundant **global all MCP startup work: `npx` may still resolve registry metadata or populate its own execution cache, particularly with explicit `--package` commands. +#### MCP tool allowlists + +Each MCP server has an optional `toolAllowlist`. `null` (the default) allows every tool the server +offers; a list allows only those tool names. The control plane leaves a server with an empty list +out of the session's MCP servers, since it could offer nothing. Admins set it under Settings › MCP +Servers, where **Load tools** asks the control plane to connect to a saved remote server with its +stored headers and list its tools (`POST /mcp-servers/:id/tools`, which requires +`mcp_servers.manage`). + +The sandbox runtime applies the allowlist per harness (`mcp_tool_access.py`): + +- **OpenCode** names MCP tools `_`. A restricted server gets a `_*` deny rule + followed by an allow rule per selected tool, after the global allow. OpenCode leaves denied tools + out of the model request. Rules are ordered by name length so a restricted server whose name + extends another keeps its own rules, and OpenCode's built-in tools and custom tools + (Open-Inspect's and the repository's `.opencode/tool` files) that a deny pattern would match are + re-allowed. An unrestricted server whose name extends a restricted one (`docs_extra` next to a + restricted `docs`) gets no allow rule, because `docs_extra_*` would also match `docs`' own + `extra_*` tools; the runtime logs `mcp.tool_allowlist_shadowed_servers`. +- **Claude Agent** runs in `dontAsk` mode, so a restricted server's `mcp____*` approval is + replaced by one `mcp____` approval per selected tool and other calls are refused. + Claude Code removes a tool from context only when a deny rule names it, so the unselected tools + stay listed. Approvals add up across sources (the session's Claude settings, or the wildcard of a + server whose name overlaps, since Claude Code splits names on `__`), so a `PreToolUse` hook also + refuses every unselected `mcp____*` tool of a restricted server. +- Servers whose names differ only in characters both harnesses replace with `_` (`my.docs` and + `my_docs`) produce the same tool names. They are treated as one: restricted if any of them is, to + the tools every restricted one allows. +- Tools whose names render identically cannot be told apart by either harness, with or without an + allowlist: an MCP tool that renders as a built-in or Open-Inspect tool name (`apply` server, + `patch` tool versus OpenCode's `apply_patch`), or as another server's selected tool. The built-in + and the selected tool stay allowed. Likewise, OpenCode names a custom tool file's named exports + `_`, so a file named like a restricted server (`.opencode/tool/docs.ts`) shares its + prefix, and its named exports are hidden. + ### When Snapshots Are Taken - **After successful prompt completion**: Preserves the workspace state diff --git a/package-lock.json b/package-lock.json index 139399ec7a..12ade204f8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3521,6 +3521,36 @@ "node": ">=22.12.0" } }, + "node_modules/@modelcontextprotocol/client": { + "version": "2.3.1", + "resolved": "https://registry.npmjs.org/@modelcontextprotocol/client/-/client-2.3.1.tgz", + "integrity": "sha512-mIGZXpHsjnZ6lD+gD/WCMpR5k8yVQQ8nNFH1N0Srf7AvnwTUMYD6pvTY8ng2+He5FfV7YMZLmrjL7cLa/cc3dQ==", + "license": "Apache-2.0", + "dependencies": { + "@modelcontextprotocol/core": "2.3.1", + "cross-spawn": "^7.0.5", + "eventsource": "^3.0.2", + "eventsource-parser": "^3.0.8", + "jose": "^6.1.3", + "pkce-challenge": "^5.0.0", + "zod": "^4.2.0" + }, + "engines": { + "node": ">=20" + } + }, + "node_modules/@modelcontextprotocol/core": { + "version": "2.3.1", + "resolved": "https://registry.npmjs.org/@modelcontextprotocol/core/-/core-2.3.1.tgz", + "integrity": "sha512-laVmIhPGpWi7rG9q+0UigblRN0Hws/Yjlb6qLFtVqryXVLkc4h+yN8z731POD2LnAGQlFe7/tXhUm7Bi0YB91g==", + "license": "Apache-2.0", + "dependencies": { + "zod": "^4.2.0" + }, + "engines": { + "node": ">=20" + } + }, "node_modules/@napi-rs/wasm-runtime": { "version": "1.2.3", "resolved": "https://registry.npmjs.org/@napi-rs/wasm-runtime/-/wasm-runtime-1.2.3.tgz", @@ -10437,15 +10467,6 @@ "url": "https://paulmillr.com/funding/" } }, - "node_modules/better-auth/node_modules/jose": { - "version": "6.2.4", - "resolved": "https://registry.npmjs.org/jose/-/jose-6.2.4.tgz", - "integrity": "sha512-N8acGzVsQy6M/fjFcxtysNc4Q379TcM5dM/qKkNtsHFji88yANnXTr7BLeP75iPnFwBfQzM/jg2BZ9+HZrHCZA==", - "license": "MIT", - "funding": { - "url": "https://github.com/sponsors/panva" - } - }, "node_modules/better-call": { "version": "1.3.7", "resolved": "https://registry.npmjs.org/better-call/-/better-call-1.3.7.tgz", @@ -11127,7 +11148,6 @@ "version": "7.0.6", "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-7.0.6.tgz", "integrity": "sha512-uV2QOWP2nWzsy2aMp8aRibhi9dlzF5Hgh5SHaB9OiTGEyDTiJJyx0uy51QXdyWbtAHNua4XJzUKca3OzKUd3vA==", - "dev": true, "license": "MIT", "dependencies": { "path-key": "^3.1.0", @@ -12832,6 +12852,27 @@ "integrity": "sha512-mlsTRyGaPBjPedk6Bvw+aqbsXDtoAyAzm5MO7JgU+yVRyMQ5O8bD4Kcci7BS85f93veegeCPkL8R4GLClnjLFw==", "license": "MIT" }, + "node_modules/eventsource": { + "version": "3.0.7", + "resolved": "https://registry.npmjs.org/eventsource/-/eventsource-3.0.7.tgz", + "integrity": "sha512-CRT1WTyuQoD771GW56XEZFQ/ZoSfWid1alKGDYMmkt2yl8UXrVR4pspqWNEcqKvVIzg6PAltWjxcSSPrboA4iA==", + "license": "MIT", + "dependencies": { + "eventsource-parser": "^3.0.1" + }, + "engines": { + "node": ">=18.0.0" + } + }, + "node_modules/eventsource-parser": { + "version": "3.1.1", + "resolved": "https://registry.npmjs.org/eventsource-parser/-/eventsource-parser-3.1.1.tgz", + "integrity": "sha512-EKN1vKAMcZ8MlYMpaNuxN6R9yakzH6uajHcHVTqWJzvu5pWw9DyhbP35HH8MVBQ+dZjAfDxk+A8NiR9KWaXiyQ==", + "license": "MIT", + "engines": { + "node": ">=18.0.0" + } + }, "node_modules/execa": { "version": "5.1.1", "resolved": "https://registry.npmjs.org/execa/-/execa-5.1.1.tgz", @@ -15836,7 +15877,6 @@ "version": "2.0.0", "resolved": "https://registry.npmjs.org/isexe/-/isexe-2.0.0.tgz", "integrity": "sha512-RHxMLp9lnKHGHRng9QFhRCMbYAcVpn69smSGcq3f36xjgVVWThj4qqLbTLlq7Ssj8B+fIQ1EuCEGI2lKsyQeIw==", - "dev": true, "license": "ISC" }, "node_modules/istanbul-lib-coverage": { @@ -15921,6 +15961,15 @@ "jiti": "bin/jiti.js" } }, + "node_modules/jose": { + "version": "6.2.12", + "resolved": "https://registry.npmjs.org/jose/-/jose-6.2.12.tgz", + "integrity": "sha512-9NiFmJEex0sy2Dk58j2UGBSHgUs2ypF9eZSu4L6vjOX3Dp96Sw1F3uL+H+D1sx02jZZdzUT0HgvCy59CuvXcWw==", + "license": "MIT", + "funding": { + "url": "https://github.com/sponsors/panva" + } + }, "node_modules/js-tokens": { "version": "4.0.0", "resolved": "https://registry.npmjs.org/js-tokens/-/js-tokens-4.0.0.tgz", @@ -16555,6 +16604,12 @@ "url": "https://github.com/sponsors/wooorm" } }, + "node_modules/lru_map": { + "version": "0.4.1", + "resolved": "https://registry.npmjs.org/lru_map/-/lru_map-0.4.1.tgz", + "integrity": "sha512-I+lBvqMMFfqaV8CJCISjI3wbjmwVu/VyOoU7+qtu9d7ioW5klMgsTTiUOUp+DJvfTTzKXoPbyC6YfgkNcyPSOg==", + "license": "MIT" + }, "node_modules/lru-cache": { "version": "11.5.1", "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-11.5.1.tgz", @@ -16565,12 +16620,6 @@ "node": "20 || >=22" } }, - "node_modules/lru_map": { - "version": "0.4.1", - "resolved": "https://registry.npmjs.org/lru_map/-/lru_map-0.4.1.tgz", - "integrity": "sha512-I+lBvqMMFfqaV8CJCISjI3wbjmwVu/VyOoU7+qtu9d7ioW5klMgsTTiUOUp+DJvfTTzKXoPbyC6YfgkNcyPSOg==", - "license": "MIT" - }, "node_modules/lucide-react": { "version": "1.48.0", "resolved": "https://registry.npmjs.org/lucide-react/-/lucide-react-1.48.0.tgz", @@ -18657,7 +18706,6 @@ "version": "3.1.1", "resolved": "https://registry.npmjs.org/path-key/-/path-key-3.1.1.tgz", "integrity": "sha512-ojmeN0qd+y0jszEtoY48r0Peq5dwMEkIlCOu6Q5f41lfkswXuKtYrhgoTpLnyIcHm24Uhqx+5Tqm2InSwLhE6Q==", - "dev": true, "license": "MIT", "engines": { "node": ">=8" @@ -18736,6 +18784,15 @@ "node": ">= 6" } }, + "node_modules/pkce-challenge": { + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/pkce-challenge/-/pkce-challenge-5.0.1.tgz", + "integrity": "sha512-wQ0b/W4Fr01qtpHlqSqspcj3EhBvimsdh0KlHhH8HRZnMsEa0ea2fTULOXOS9ccQr3om+GcGRk4e+isrZWV8qQ==", + "license": "MIT", + "engines": { + "node": ">=16.20.0" + } + }, "node_modules/points-on-curve": { "version": "0.2.0", "resolved": "https://registry.npmjs.org/points-on-curve/-/points-on-curve-0.2.0.tgz", @@ -20141,7 +20198,6 @@ "version": "2.0.0", "resolved": "https://registry.npmjs.org/shebang-command/-/shebang-command-2.0.0.tgz", "integrity": "sha512-kHxr2zZpYtdmrN1qDjrrX/Z1rR1kG8Dx+gkpK1G4eXmvXswmcE1hTWBWYUzlraYw1/yZp6YuDY77YtvbN0dmDA==", - "dev": true, "license": "MIT", "dependencies": { "shebang-regex": "^3.0.0" @@ -20154,7 +20210,6 @@ "version": "3.0.0", "resolved": "https://registry.npmjs.org/shebang-regex/-/shebang-regex-3.0.0.tgz", "integrity": "sha512-7++dFhtcx3353uBaq8DDR4NuxBetBzC7ZQOhmTQInHEd6bSrXdiEyzCvG07Z44UYdLShWUyXt5M/yhz8ekcb1A==", - "dev": true, "license": "MIT", "engines": { "node": ">=8" @@ -21866,7 +21921,6 @@ "version": "2.0.2", "resolved": "https://registry.npmjs.org/which/-/which-2.0.2.tgz", "integrity": "sha512-BLI3Tl1TW3Pvl70l3yq3Y64i+awpwXqsGBYWkkqMtnbXgrMD+yj7rhW0kuEDxzJaYXGjEW5ogapKNMEKNMjibA==", - "dev": true, "license": "ISC", "dependencies": { "isexe": "^2.0.0" @@ -22345,6 +22399,7 @@ "@aws-sdk/client-s3": "^3.984.0", "@cloudflare/workers-types": "^4.20241230.0", "@hono/node-server": "^2.1.1", + "@modelcontextprotocol/client": "^2.3.1", "@open-inspect/shared": "file:../shared", "better-auth": "1.6.25", "hono": "^4.13.12", diff --git a/packages/control-plane/package.json b/packages/control-plane/package.json index 15faa785ba..88595abe80 100644 --- a/packages/control-plane/package.json +++ b/packages/control-plane/package.json @@ -20,6 +20,7 @@ "@aws-sdk/client-s3": "^3.984.0", "@cloudflare/workers-types": "^4.20241230.0", "@hono/node-server": "^2.1.1", + "@modelcontextprotocol/client": "^2.3.1", "@open-inspect/shared": "file:../shared", "better-auth": "1.6.25", "hono": "^4.13.12", diff --git a/packages/control-plane/src/db/mcp-servers.test.ts b/packages/control-plane/src/db/mcp-servers.test.ts index 5e57ebe58a..81d8ddb76f 100644 --- a/packages/control-plane/src/db/mcp-servers.test.ts +++ b/packages/control-plane/src/db/mcp-servers.test.ts @@ -67,6 +67,7 @@ const sampleRow = { url: null, env: JSON.stringify({ DEBUG: "1" }), repo_scope: null, + tool_allowlist: null, enabled: 1, created_at: 1000, updated_at: 1000, @@ -81,6 +82,7 @@ const remoteRow = { url: "https://mcp.example.com/sse", env: "{}", repo_scope: JSON.stringify(["carboncopyinc/habakkuk"]), + tool_allowlist: null, enabled: 1, created_at: 1001, updated_at: 1001, @@ -148,6 +150,25 @@ describe("McpServerStore", () => { expect(result).toBeNull(); }); + it.each([ + ["every tool", null, null], + ["no tools", "[]", []], + ["the stored tools", JSON.stringify(["fetch", "search"]), ["fetch", "search"]], + ])("reads a tool allowlist exposing %s", async (_case, stored, expected) => { + const { db } = createFakeD1({ firstResult: { ...remoteRow, tool_allowlist: stored } }); + const store = new McpServerStore(db, TEST_ENCRYPTION_KEY); + expect((await store.get("def456"))!.toolAllowlist).toEqual(expected); + }); + + it.each([["not json"], [JSON.stringify({ tools: ["search"] })], [JSON.stringify([""])]])( + "exposes no tools when the stored allowlist %s is unreadable", + async (stored) => { + const { db } = createFakeD1({ firstResult: { ...remoteRow, tool_allowlist: stored } }); + const store = new McpServerStore(db, TEST_ENCRYPTION_KEY); + expect((await store.get("def456"))!.toolAllowlist).toEqual([]); + } + ); + it("handles corrupted JSON in command gracefully", async () => { const corruptRow = { ...sampleRow, command: "not-json" }; const { db } = createFakeD1({ firstResult: corruptRow }); @@ -244,6 +265,25 @@ describe("McpServerStore", () => { } as unknown as ValidatedCreateMcpServerInput; await expect(store.create(invalid)).rejects.toThrow(McpServerValidationError); }); + + it.each([ + ["no allowlist", undefined, null], + ["an empty allowlist", [], "[]"], + ["named tools", ["fetch", "search"], JSON.stringify(["fetch", "search"])], + ])("stores %s", async (_case, toolAllowlist, stored) => { + const { db, statements } = createFakeD1({ firstResult: remoteRow }); + const store = new McpServerStore(db, TEST_ENCRYPTION_KEY); + await store.create({ + name: "docs", + type: "remote", + url: "https://mcp.example.com/mcp", + enabled: true, + toolAllowlist, + }); + const insert = statements.find((statement) => statement.sql.includes("INSERT INTO"))!; + expect(insert.sql).toContain("tool_allowlist"); + expect(insert.params[7]).toBe(stored); + }); }); describe("update()", () => { @@ -271,6 +311,22 @@ describe("McpServerStore", () => { expect(err).toBeInstanceOf(McpServerValidationError); expect(err.message).toMatch(/require a command/i); }); + + it.each([ + ["keeps the stored allowlist when the patch omits it", {}, '["search"]'], + ["restores every tool for a null allowlist", { toolAllowlist: null }, null], + ["stores an empty allowlist", { toolAllowlist: [] }, "[]"], + ["stores the patched tools", { toolAllowlist: ["fetch"] }, '["fetch"]'], + ])("%s", async (_case, patch, stored) => { + const { db, statements } = createFakeD1({ + firstResult: { ...remoteRow, tool_allowlist: '["search"]' }, + }); + const store = new McpServerStore(db, TEST_ENCRYPTION_KEY); + await store.update("def456", patch); + const update = statements.find((statement) => statement.sql.includes("UPDATE"))!; + expect(update.sql).toContain("tool_allowlist = ?"); + expect(update.params[6]).toBe(stored); + }); }); describe("getDecryptedForSession()", () => { @@ -327,6 +383,34 @@ describe("McpServerStore", () => { expect(remote.env).toBeUndefined(); }); + it("carries each server's tool allowlist into the session config", async () => { + const { db } = createFakeD1({ + allResults: [sampleRow, { ...remoteRow, tool_allowlist: '["search"]' }], + }); + const store = new McpServerStore(db, TEST_ENCRYPTION_KEY); + const results = await store.getDecryptedForSession([ + { repoOwner: "carboncopyinc", repoName: "habakkuk" }, + ]); + expect(results.map((server) => [server.name, server.toolAllowlist])).toEqual([ + ["playwright", null], + ["remote-mcp", ["search"]], + ]); + }); + + it.each(["[]", "not json"])( + "leaves out a server whose stored allowlist %s allows no tools", + async (stored) => { + const { db } = createFakeD1({ + allResults: [sampleRow, { ...remoteRow, tool_allowlist: stored }], + }); + const store = new McpServerStore(db, TEST_ENCRYPTION_KEY); + const results = await store.getDecryptedForSession([ + { repoOwner: "carboncopyinc", repoName: "habakkuk" }, + ]); + expect(results.map((server) => server.name)).toEqual(["playwright"]); + } + ); + it("returns env (not headers) for local servers", async () => { const { db } = createFakeD1({ allResults: [sampleRow] }); const store = new McpServerStore(db, TEST_ENCRYPTION_KEY); diff --git a/packages/control-plane/src/db/mcp-servers.ts b/packages/control-plane/src/db/mcp-servers.ts index 9ee696d446..f0a2f33ca3 100644 --- a/packages/control-plane/src/db/mcp-servers.ts +++ b/packages/control-plane/src/db/mcp-servers.ts @@ -2,6 +2,7 @@ import { mcpServerCommandSchema, mcpServerCredentialMapSchema, mcpServerTypeSchema, + mcpToolAllowlistSchema, type McpServerConfig, type McpServerMetadata, type ValidatedCreateMcpServerInput, @@ -36,6 +37,7 @@ interface McpServerRow { url: string | null; env: string; repo_scope: string | null; + tool_allowlist: string | null; enabled: number; created_at: number; updated_at: number; @@ -51,6 +53,33 @@ function parseRepoScopes(raw: string | null): string[] | null { } } +/** + * NULL means every tool. A stored value that no longer parses exposes no tools + * rather than all of them, so a damaged row cannot widen what the agent sees. + */ +function parseToolAllowlist(raw: string | null, serverId: string): string[] | null { + if (raw == null) return null; + let parsed: unknown; + let reason = "invalid_shape"; + try { + parsed = JSON.parse(raw); + } catch { + reason = "invalid_json"; + } + const result = mcpToolAllowlistSchema.safeParse(parsed); + if (result.success) return result.data; + log.warn("MCP server tool allowlist unreadable; exposing no tools", { + event: "mcp_server.tool_allowlist_unreadable", + server_id: serverId, + reason, + }); + return []; +} + +function serializeToolAllowlist(tools: string[] | null | undefined): string | null { + return tools == null ? null : JSON.stringify(tools); +} + function safeJsonParseCommand(raw: string | null): string[] | undefined { if (!raw) return undefined; let parsed: unknown; @@ -101,6 +130,7 @@ function rowToConfig(row: McpServerRow, payload: Record): McpSer url: type === "remote" ? (row.url ?? undefined) : undefined, ...envOrHeaders, repoScopes: parseRepoScopes(row.repo_scope), + toolAllowlist: parseToolAllowlist(row.tool_allowlist, row.id), enabled: row.enabled === 1, }; } @@ -118,6 +148,7 @@ function rowToMetadata(row: McpServerRow): McpServerMetadata { hasEnv: type === "local" && hasCredentials, hasHeaders: type === "remote" && hasCredentials, repoScopes: parseRepoScopes(row.repo_scope), + toolAllowlist: parseToolAllowlist(row.tool_allowlist, row.id), enabled: row.enabled === 1, }; } @@ -204,8 +235,8 @@ export class McpServerStore { try { await this.db .prepare( - `INSERT INTO mcp_servers (id, name, type, command, url, env, repo_scope, enabled, created_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)` + `INSERT INTO mcp_servers (id, name, type, command, url, env, repo_scope, tool_allowlist, enabled, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)` ) .bind( id, @@ -217,6 +248,7 @@ export class McpServerStore { config.repoScopes?.length ? JSON.stringify(config.repoScopes.map((r) => r.toLowerCase())) : null, + serializeToolAllowlist(config.toolAllowlist), config.enabled ? 1 : 0, now, now @@ -289,7 +321,7 @@ export class McpServerStore { try { const statement = this.db.prepare( - `UPDATE mcp_servers SET name = ?, type = ?, command = ?, url = ?, env = ?, repo_scope = ?, enabled = ?, updated_at = ?, revision = revision + 1 + `UPDATE mcp_servers SET name = ?, type = ?, command = ?, url = ?, env = ?, repo_scope = ?, tool_allowlist = ?, enabled = ?, updated_at = ?, revision = revision + 1 WHERE id = ? AND revision = COALESCE(?, revision) RETURNING *` ); @@ -305,6 +337,9 @@ export class McpServerStore { ? JSON.stringify(patch.repoScopes.map((r) => r.toLowerCase())) : null : row.repo_scope, + patch.toolAllowlist !== undefined + ? serializeToolAllowlist(patch.toolAllowlist) + : row.tool_allowlist, patch.enabled !== undefined ? (patch.enabled ? 1 : 0) : row.enabled, now, id, @@ -325,6 +360,15 @@ export class McpServerStore { } } + /** One server with decrypted credentials, for control-plane calls to the server itself. */ + async getDecrypted(id: string): Promise { + const row = await this.db + .prepare("SELECT * FROM mcp_servers WHERE id = ?") + .bind(id) + .first(); + return row ? this.decryptRow(row) : null; + } + async delete(id: string): Promise { const result = await this.db.prepare("DELETE FROM mcp_servers WHERE id = ?").bind(id).run(); return (result.meta?.changes ?? 0) > 0; @@ -334,6 +378,7 @@ export class McpServerStore { * Servers applicable to a session's member repositories: unscoped servers * always apply; scoped servers apply when ANY member matches a scope. * Pass an empty list for repo-less sessions (unscoped servers only). + * Servers allowed no tools are left out, since starting them gains nothing. */ async getDecryptedForSession( repositories: Array<{ repoOwner: string; repoName: string }> @@ -351,6 +396,7 @@ export class McpServerStore { return scopes.some((s) => repoFullNames.has(s.toLowerCase())); }); - return Promise.all(filtered.map((r) => this.decryptRow(r))); + const servers = await Promise.all(filtered.map((r) => this.decryptRow(r))); + return servers.filter((server) => server.toolAllowlist?.length !== 0); } } diff --git a/packages/control-plane/src/mcp/tool-discovery.test.ts b/packages/control-plane/src/mcp/tool-discovery.test.ts new file mode 100644 index 0000000000..613c06563e --- /dev/null +++ b/packages/control-plane/src/mcp/tool-discovery.test.ts @@ -0,0 +1,290 @@ +import type { FetchLike } from "@modelcontextprotocol/client"; +import { + MAX_MCP_SERVER_TOOLS, + type McpServerConfig, +} from "@open-inspect/shared/types/integrations"; +import { describe, expect, it, vi } from "vitest"; +import { + McpToolDiscoveryError, + authenticatedFetch, + discoverRemoteMcpTools, +} from "./tool-discovery"; + +const remoteServer: McpServerConfig = { + id: "mcp-1", + name: "docs", + type: "remote", + url: "https://mcp.example.com/mcp", + headers: { Authorization: "Bearer secret", "X-Tenant": "acme" }, + repoScopes: null, + toolAllowlist: null, + enabled: true, +}; + +function jsonResponse(body: unknown): Response { + return new Response(JSON.stringify(body), { headers: { "content-type": "application/json" } }); +} + +/** + * A Streamable HTTP MCP server that advertises `tools`, recording request + * headers. With `stallToolsList`, it opens the tools/list response stream and + * never answers. + */ +function streamableServer( + tools: Array<{ name: string; description?: string }>, + { stallToolsList = false } = {} +) { + const requestHeaders: Headers[] = []; + const fetchMock = vi.fn(async (_input: RequestInfo | URL, init?: RequestInit) => { + requestHeaders.push(new Headers(init?.headers)); + if (init?.method === "GET") return new Response(null, { status: 405 }); + if (init?.method === "DELETE") return new Response(null, { status: 200 }); + const message = JSON.parse(String(init?.body)) as { + id?: number; + method: string; + params?: { protocolVersion?: string }; + }; + if (message.method === "initialize") { + return jsonResponse({ + jsonrpc: "2.0", + id: message.id, + result: { + protocolVersion: message.params?.protocolVersion, + capabilities: { tools: {} }, + serverInfo: { name: "test-server", version: "1.0.0" }, + }, + }); + } + if (message.method === "notifications/initialized") { + return new Response(null, { status: 202 }); + } + if (message.method === "tools/list" && stallToolsList) { + return new Response(new ReadableStream(), { + headers: { "content-type": "text/event-stream" }, + }); + } + if (message.method === "tools/list") { + return jsonResponse({ + jsonrpc: "2.0", + id: message.id, + result: { + tools: tools.map((tool) => ({ ...tool, inputSchema: { type: "object" } })), + }, + }); + } + throw new Error(`Unexpected MCP request: ${message.method}`); + }); + return { fetchMock: fetchMock as unknown as FetchLike, requestHeaders }; +} + +/** + * A legacy HTTP+SSE MCP server: Streamable HTTP POSTs to the URL get 405, a GET + * opens the event stream, and answers to POSTed messages arrive on it. + */ +function legacySseServer(tools: Array<{ name: string }>) { + const encoder = new TextEncoder(); + let stream: ReadableStreamDefaultController | undefined; + const send = (event: string, data: string) => + stream?.enqueue(encoder.encode(`event: ${event}\ndata: ${data}\n\n`)); + const fetchMock = vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = new URL(String(input)); + if (url.pathname === "/sse" && init?.method === "POST") { + return new Response(null, { status: 405 }); + } + if (url.pathname === "/sse") { + const body = new ReadableStream({ + start(controller) { + stream = controller; + init?.signal?.addEventListener("abort", () => controller.error(init.signal?.reason)); + send("endpoint", "/messages"); + }, + }); + return new Response(body, { headers: { "content-type": "text/event-stream" } }); + } + const message = JSON.parse(String(init?.body)) as { + id?: number; + method: string; + params?: { protocolVersion?: string }; + }; + const result = + message.method === "initialize" + ? { + protocolVersion: message.params?.protocolVersion, + capabilities: { tools: {} }, + serverInfo: { name: "legacy-server", version: "1.0.0" }, + } + : { tools: tools.map((tool) => ({ ...tool, inputSchema: { type: "object" } })) }; + if (message.id !== undefined) { + send("message", JSON.stringify({ jsonrpc: "2.0", id: message.id, result })); + } + return new Response(null, { status: 202 }); + }); + return { fetchMock }; +} + +describe("authenticatedFetch", () => { + it("adds the server headers, honours both signals and drops the cache mode", async () => { + const timeout = new AbortController(); + const transport = new AbortController(); + let forwarded: RequestInit | undefined; + const baseFetch: FetchLike = vi.fn(async (_input, init) => { + forwarded = init; + return new Response(null, { status: 204 }); + }); + + // The SSE transport sets `cache`, which the Workers RequestInit type does not declare. + const init: RequestInit & { cache?: string } = { + headers: { Accept: "application/json" }, + signal: transport.signal, + cache: "no-store", + }; + await authenticatedFetch( + { Authorization: "Bearer secret" }, + timeout.signal, + baseFetch + )("https://mcp.example.com", init); + + const headers = new Headers(forwarded?.headers); + expect(headers.get("authorization")).toBe("Bearer secret"); + expect(headers.get("accept")).toBe("application/json"); + expect(forwarded).not.toHaveProperty("cache"); + expect(forwarded?.signal?.aborted).toBe(false); + transport.abort(); + expect(forwarded?.signal?.aborted).toBe(true); + }); +}); + +describe("discoverRemoteMcpTools", () => { + it("lists tools trimmed, deduplicated and sorted, sending the stored headers", async () => { + const { fetchMock, requestHeaders } = streamableServer([ + { name: " zebra ", description: " Last tool " }, + { name: "alpha", description: "First tool" }, + { name: "alpha", description: "Duplicate" }, + { name: " " }, + { name: "plain" }, + ]); + + await expect(discoverRemoteMcpTools(remoteServer, { fetch: fetchMock })).resolves.toEqual([ + { name: "alpha", description: "First tool" }, + { name: "plain" }, + { name: "zebra", description: "Last tool" }, + ]); + expect(requestHeaders).not.toHaveLength(0); + for (const headers of requestHeaders) { + expect(headers.get("authorization")).toBe("Bearer secret"); + expect(headers.get("x-tenant")).toBe("acme"); + } + }); + + it("falls back to the legacy SSE transport without the cache mode Workers rejects", async () => { + const { fetchMock } = legacySseServer([{ name: "search" }]); + + await expect( + discoverRemoteMcpTools( + { ...remoteServer, url: "https://mcp.example.com/sse" }, + { fetch: fetchMock as unknown as FetchLike } + ) + ).resolves.toEqual([{ name: "search" }]); + const streamRequest = fetchMock.mock.calls.find(([, init]) => init?.method !== "POST"); + expect(streamRequest?.[1]).toBeDefined(); + expect(streamRequest?.[1]).not.toHaveProperty("cache"); + expect(new Headers(streamRequest?.[1]?.headers).get("authorization")).toBe("Bearer secret"); + }); + + it("rejects a server advertising more tools than an allowlist can hold", async () => { + const tools = Array.from({ length: MAX_MCP_SERVER_TOOLS + 1 }, (_, i) => ({ name: `t${i}` })); + const { fetchMock } = streamableServer(tools); + + const err = await discoverRemoteMcpTools(remoteServer, { fetch: fetchMock }).catch((e) => e); + expect(err).toBeInstanceOf(McpToolDiscoveryError); + expect(err.message).toMatch(/more than 1000 tools/); + }); + + it.each([ + [401, 401, "The server returned HTTP 401; check its headers"], + [405, 403, "The server returned HTTP 403; check its headers"], + [404, 404, "The server returned HTTP 404"], + ])( + "falls back to SSE after Streamable HTTP %i and reports SSE %i by status only", + async (streamableStatus, sseStatus, message) => { + const fetchMock = vi.fn(async (_input: RequestInfo | URL, init?: RequestInit) => + init?.method === "POST" + ? new Response("internal detail", { status: streamableStatus }) + : new Response("internal detail", { status: sseStatus }) + ); + + const err = await discoverRemoteMcpTools(remoteServer, { + fetch: fetchMock as unknown as FetchLike, + }).catch((e) => e); + expect(err).toBeInstanceOf(McpToolDiscoveryError); + expect(err.message).toBe(message); + expect(fetchMock.mock.calls.some(([, init]) => init?.method !== "POST")).toBe(true); + } + ); + + it("reports a Streamable HTTP server error without trying SSE", async () => { + const fetchMock = vi.fn( + async (_input: RequestInfo | URL, _init?: RequestInit) => + new Response("internal detail", { status: 500 }) + ); + + const err = await discoverRemoteMcpTools(remoteServer, { + fetch: fetchMock as unknown as FetchLike, + }).catch((e) => e); + expect(err).toBeInstanceOf(McpToolDiscoveryError); + expect(err.message).toBe("The server returned HTTP 500"); + expect(fetchMock.mock.calls.every(([, init]) => init?.method === "POST")).toBe(true); + }); + + it("rethrows a failure without an HTTP status unchanged and does not try SSE", async () => { + const failure = new TypeError("fetch failed"); + const fetchMock = vi.fn(async (_input: RequestInfo | URL, _init?: RequestInit) => { + throw failure; + }); + + const err = await discoverRemoteMcpTools(remoteServer, { + fetch: fetchMock as unknown as FetchLike, + }).catch((e) => e); + expect(err).not.toBeInstanceOf(McpToolDiscoveryError); + expect(fetchMock.mock.calls.every(([, init]) => init?.method === "POST")).toBe(true); + }); + + it("reports a timeout without trying SSE", async () => { + const fetchMock = vi.fn( + (_input: RequestInfo | URL, init?: RequestInit) => + new Promise((_resolve, reject) => { + init?.signal?.addEventListener("abort", () => reject(init.signal?.reason)); + }) + ); + + const err = await discoverRemoteMcpTools(remoteServer, { + fetch: fetchMock as unknown as FetchLike, + timeoutMs: 20, + }).catch((e) => e); + expect(err).toBeInstanceOf(McpToolDiscoveryError); + expect(err.message).toMatch(/did not respond within/); + expect(fetchMock.mock.calls.every(([, init]) => init?.method === "POST")).toBe(true); + }); + + it("bounds a tools/list response that never arrives by the timeout", async () => { + const { fetchMock } = streamableServer([], { stallToolsList: true }); + + const err = await discoverRemoteMcpTools(remoteServer, { + fetch: fetchMock, + timeoutMs: 50, + }).catch((e) => e); + expect(err).toBeInstanceOf(McpToolDiscoveryError); + expect(err.message).toMatch(/did not respond within/); + }, 5_000); + + it.each<[string, McpServerConfig]>([ + ["a local server", { ...remoteServer, type: "local", url: undefined, command: ["npx"] }], + ["a non-HTTP URL", { ...remoteServer, url: "ftp://mcp.example.com" }], + ])("refuses %s without connecting", async (_case, server) => { + const fetchMock = vi.fn() as unknown as FetchLike; + await expect(discoverRemoteMcpTools(server, { fetch: fetchMock })).rejects.toBeInstanceOf( + McpToolDiscoveryError + ); + expect(fetchMock).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/control-plane/src/mcp/tool-discovery.ts b/packages/control-plane/src/mcp/tool-discovery.ts new file mode 100644 index 0000000000..27a232963e --- /dev/null +++ b/packages/control-plane/src/mcp/tool-discovery.ts @@ -0,0 +1,150 @@ +import { + Client, + SdkHttpError, + SseError, + SSEClientTransport, + StreamableHTTPClientTransport, + type FetchLike, + type Transport, +} from "@modelcontextprotocol/client"; +import { + MAX_MCP_SERVER_TOOLS, + type McpServerConfig, + type McpToolMetadata, +} from "@open-inspect/shared/types/integrations"; + +const DISCOVERY_TIMEOUT_MS = 15_000; +const MAX_LIST_TOOLS_PAGES = 64; +const MAX_TOOL_DESCRIPTION_LENGTH = 2000; +// Statuses meaning the URL does not speak Streamable HTTP, so the SSE status explains more. +const NOT_STREAMABLE_HTTP_STATUSES = new Set([404, 405]); + +export class McpToolDiscoveryError extends Error { + constructor(message: string, options?: ErrorOptions) { + super(message, options); + this.name = "McpToolDiscoveryError"; + } +} + +/** + * Sends the server's configured headers on every request, bounded by `signal`. + * Drops `cache`: the SSE transport's EventSource sets `cache: "no-store"`, which + * the Workers runtime rejects at the control plane's compatibility date. + */ +export function authenticatedFetch( + headers: Record, + signal: AbortSignal, + baseFetch: FetchLike +): FetchLike { + return (input, init) => { + // Typed explicitly: the Workers RequestInit type has no `cache` field. + const { cache: _cache, ...forwarded }: RequestInit & { cache?: unknown } = init ?? {}; + const requestHeaders = new Headers(headers); + for (const [name, value] of new Headers(forwarded.headers)) requestHeaders.set(name, value); + const requestSignal = forwarded.signal; + return baseFetch(input, { + ...forwarded, + headers: requestHeaders, + signal: requestSignal ? AbortSignal.any([signal, requestSignal]) : signal, + }); + }; +} + +async function listTools(transport: Transport, signal: AbortSignal): Promise { + const client = new Client( + { name: "open-inspect-settings", version: "1.0.0" }, + { listMaxPages: MAX_LIST_TOOLS_PAGES } + ); + try { + // Aborting a fetch does not settle a request whose response stream stalls. + await client.connect(transport, { signal }); + const { tools } = await client.listTools(undefined, { signal }); + if (tools.length > MAX_MCP_SERVER_TOOLS) { + throw new McpToolDiscoveryError( + `The server advertises more than ${MAX_MCP_SERVER_TOOLS} tools` + ); + } + const byName = new Map(); + for (const tool of tools) { + const name = tool.name.trim(); + if (!name || byName.has(name)) continue; + const description = tool.description?.trim().slice(0, MAX_TOOL_DESCRIPTION_LENGTH); + byName.set(name, description ? { name, description } : { name }); + } + return [...byName.values()].sort((a, b) => a.name.localeCompare(b.name)); + } finally { + await client.close().catch(() => undefined); + } +} + +function httpStatus(err: unknown): number | undefined { + if (err instanceof SdkHttpError) return err.status; + if (err instanceof SseError) return err.code; + return undefined; +} + +/** + * Reports only the HTTP status, because SDK error messages can quote the + * server's response body. + */ +function statusError(status: number, cause: unknown): McpToolDiscoveryError { + const hint = status === 401 || status === 403 ? "; check its headers" : ""; + return new McpToolDiscoveryError(`The server returned HTTP ${status}${hint}`, { cause }); +} + +interface DiscoveryOptions { + fetch?: FetchLike; + timeoutMs?: number; +} + +/** + * Lists the tools a remote MCP server advertises, connecting with its stored + * headers. Tries Streamable HTTP first and, when that is rejected with a 4xx + * status, falls back to the legacy SSE transport, as the MCP specification + * describes for backwards compatibility. + */ +export async function discoverRemoteMcpTools( + server: McpServerConfig, + { fetch: baseFetch = fetch, timeoutMs = DISCOVERY_TIMEOUT_MS }: DiscoveryOptions = {} +): Promise { + if (server.type !== "remote" || !server.url) { + throw new McpToolDiscoveryError("Only remote MCP servers can list their tools"); + } + const url = new URL(server.url); + if (url.protocol !== "https:" && url.protocol !== "http:") { + throw new McpToolDiscoveryError("The MCP server URL must use HTTP or HTTPS"); + } + + const timeout = AbortSignal.timeout(timeoutMs); + const timedOut = (cause: unknown) => + new McpToolDiscoveryError(`The server did not respond within ${timeoutMs / 1000} seconds`, { + cause, + }); + const request = authenticatedFetch(server.headers ?? {}, timeout, baseFetch); + + let streamableError: unknown; + let streamableStatus: number; + try { + return await listTools(new StreamableHTTPClientTransport(url, { fetch: request }), timeout); + } catch (err) { + if (err instanceof McpToolDiscoveryError) throw err; + if (timeout.aborted) throw timedOut(err); + const status = httpStatus(err); + // Only a 4xx can mean a legacy SSE server; any other failure is the real one. + if (status === undefined) throw err; + if (status < 400 || status >= 500) throw statusError(status, err); + streamableError = err; + streamableStatus = status; + } + + try { + return await listTools(new SSEClientTransport(url, { fetch: request }), timeout); + } catch (err) { + if (err instanceof McpToolDiscoveryError) throw err; + if (timeout.aborted) throw timedOut(err); + const status = NOT_STREAMABLE_HTTP_STATUSES.has(streamableStatus) + ? (httpStatus(err) ?? streamableStatus) + : streamableStatus; + throw statusError(status, new AggregateError([streamableError, err])); + } +} diff --git a/packages/control-plane/src/router.policy.test.ts b/packages/control-plane/src/router.policy.test.ts index 6a438902f5..61baa1a800 100644 --- a/packages/control-plane/src/router.policy.test.ts +++ b/packages/control-plane/src/router.policy.test.ts @@ -30,6 +30,12 @@ describe("route policy table", () => { expect(routeFor("POST", "/github/route")).toBeUndefined(); }); + it("requires MCP management to list a server's tools", () => { + expect(routeFor("POST", "/mcp-servers/server-1/tools")?.authorization).toMatchObject({ + allOf: [{ kind: "permission", permission: "mcp_servers.manage" }], + }); + }); + it("does not expose a member-facing team activity route", () => { expect(routeFor("GET", "/teams/team-1/activity")).toBeUndefined(); expect(routeFor("GET", "/audit-events")?.authorization).toMatchObject({ @@ -39,11 +45,11 @@ describe("route policy table", () => { }); it("publishes the complete canonical route catalog", () => { - expect(routes).toHaveLength(232); + expect(routes).toHaveLength(233); const paths = routes.map((route) => route.path); - expect(new Set(paths).size).toBe(176); - expect(new Set(routes.map((route) => `${route.method}:${route.path}`)).size).toBe(232); + expect(new Set(paths).size).toBe(177); + expect(new Set(routes.map((route) => `${route.method}:${route.path}`)).size).toBe(233); expect(routeFor("POST", "/sessions/session-1/sandbox-memory/search")).toMatchObject({ authentication: { kind: "sandbox" }, supportedScmProviders: "all", diff --git a/packages/control-plane/src/routes/mcp-servers.ts b/packages/control-plane/src/routes/mcp-servers.ts index bc4123bced..9072d7096b 100644 --- a/packages/control-plane/src/routes/mcp-servers.ts +++ b/packages/control-plane/src/routes/mcp-servers.ts @@ -11,6 +11,7 @@ import { import { Hono } from "hono"; import type { Env } from "../types"; import { createLogger } from "../logger"; +import { McpToolDiscoveryError, discoverRemoteMcpTools } from "../mcp/tool-discovery"; import { requireRepoSecretsEncryptionKey } from "../env-validation"; import { admit, dispatch } from "../routing/admit"; import type { ControlPlaneHonoEnv } from "../routing/hono-env"; @@ -165,6 +166,55 @@ async function handleDeleteMcpServer( return json({ ok: true }); } +/** Connects to a saved remote server with its stored headers and lists its tools. */ +/** The underlying error messages, for logs only: they can quote the server's response. */ +function causeMessage(cause: unknown): string | undefined { + if (cause instanceof AggregateError) { + return cause.errors.map((error) => causeMessage(error)).join("; "); + } + return cause instanceof Error ? cause.message : undefined; +} + +async function handleDiscoverMcpTools( + _request: Request, + env: Env, + params: { id: string }, + ctx: RequestContext +): Promise { + const { id } = params; + if (!ctx.db) return error("Database not configured", 503); + + const store = new McpServerStore(ctx.db, requireRepoSecretsEncryptionKey(env)); + const server = await store.getDecrypted(id); + if (!server) return error("MCP server not found", 404); + if (server.type !== "remote") { + return error("Only remote MCP servers can list their tools", 400); + } + + try { + const tools = await discoverRemoteMcpTools(server); + logger.info("MCP server tools discovered", { + event: "mcp_server.tools_discovered", + request_id: ctx.request_id, + trace_id: ctx.trace_id, + id, + tool_count: tools.length, + }); + return json({ tools }); + } catch (err) { + logger.warn("MCP server tool discovery failed", { + event: "mcp_server.tool_discovery_failed", + request_id: ctx.request_id, + trace_id: ctx.trace_id, + id, + error: err instanceof Error ? err.message : String(err), + cause: err instanceof Error ? causeMessage(err.cause) : undefined, + }); + const reason = err instanceof McpToolDiscoveryError ? `: ${err.message}` : ""; + return error(`Could not load tools from the MCP server${reason}`, 502); + } +} + const MCP_READ = admit({ ...GITHUB_USER_OR_SERVICE_ROUTE, authorization: requirePermission("mcp_servers.read"), @@ -181,3 +231,7 @@ mcpServerRoutes.post("/mcp-servers", MCP_MANAGE, (c) => dispatch(c, handleCreate mcpServerRoutes.get("/mcp-servers/:id", MCP_READ, (c) => dispatch(c, handleGetMcpServer)); mcpServerRoutes.put("/mcp-servers/:id", MCP_MANAGE, (c) => dispatch(c, handleUpdateMcpServer)); mcpServerRoutes.delete("/mcp-servers/:id", MCP_MANAGE, (c) => dispatch(c, handleDeleteMcpServer)); +// Manage, not read: the control plane connects out with the server's stored credentials. +mcpServerRoutes.post("/mcp-servers/:id/tools", MCP_MANAGE, (c) => + dispatch(c, handleDiscoverMcpTools) +); diff --git a/packages/control-plane/test/integration/__snapshots__/hono-route-catalog-conformance.test.ts.snap b/packages/control-plane/test/integration/__snapshots__/hono-route-catalog-conformance.test.ts.snap index ddd9b1d7f6..f94f60002a 100644 --- a/packages/control-plane/test/integration/__snapshots__/hono-route-catalog-conformance.test.ts.snap +++ b/packages/control-plane/test/integration/__snapshots__/hono-route-catalog-conformance.test.ts.snap @@ -181,6 +181,7 @@ exports[`Hono route catalog conformance > dispatches every frozen method/path/po "{"identity":"GET /mcp-servers/:id","pathname":"/mcp-servers/fixture-176-id%2Fraw","groups":{"id":"fixture-176-id%2Fraw"},"authentication":"user-or-service","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"mcp_servers.read"}],"auditAllowed":false,"service":{"kind":"actor"}},"supportedScmProviders":["github"],"cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"PUT /mcp-servers/:id","pathname":"/mcp-servers/fixture-177-id%2Fraw","groups":{"id":"fixture-177-id%2Fraw"},"authentication":"user-or-service","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"mcp_servers.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":["github"],"cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"DELETE /mcp-servers/:id","pathname":"/mcp-servers/fixture-178-id%2Fraw","groups":{"id":"fixture-178-id%2Fraw"},"authentication":"user-or-service","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"mcp_servers.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":["github"],"cacheControl":null,"hasServiceActorClaims":false}", + "{"identity":"POST /mcp-servers/:id/tools","pathname":"/mcp-servers/fixture-179-id%2Fraw/tools","groups":{"id":"fixture-179-id%2Fraw"},"authentication":"user-or-service","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"mcp_servers.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":["github"],"cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"GET /analytics/dashboard","pathname":"/analytics/dashboard","groups":{},"authentication":"user-or-service","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"analytics.read"}],"auditAllowed":false,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"GET /analytics/summary","pathname":"/analytics/summary","groups":{},"authentication":"user-or-service","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"analytics.read"}],"auditAllowed":false,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"GET /analytics/timeseries","pathname":"/analytics/timeseries","groups":{},"authentication":"user-or-service","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"analytics.read"}],"auditAllowed":false,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", @@ -192,19 +193,19 @@ exports[`Hono route catalog conformance > dispatches every frozen method/path/po "{"identity":"GET /skills","pathname":"/skills","groups":{},"authentication":"user-or-service","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.read"}],"auditAllowed":false,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"POST /skills/preview","pathname":"/skills/preview","groups":{},"authentication":"user-or-service","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.read"}],"auditAllowed":false,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"POST /skills/resolve-preview","pathname":"/skills/resolve-preview","groups":{},"authentication":"user-or-service","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.read"}],"auditAllowed":false,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", - "{"identity":"GET /skills/:id","pathname":"/skills/fixture-190-id%2Fraw","groups":{"id":"fixture-190-id%2Fraw"},"authentication":"user-or-service","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.read"}],"auditAllowed":false,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", + "{"identity":"GET /skills/:id","pathname":"/skills/fixture-191-id%2Fraw","groups":{"id":"fixture-191-id%2Fraw"},"authentication":"user-or-service","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.read"}],"auditAllowed":false,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"POST /skills","pathname":"/skills","groups":{},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"POST /skills/import/preview","pathname":"/skills/import/preview","groups":{},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"POST /skills/import","pathname":"/skills/import","groups":{},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", - "{"identity":"POST /skills/:id/reimport/preview","pathname":"/skills/fixture-194-id%2Fraw/reimport/preview","groups":{"id":"fixture-194-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", - "{"identity":"POST /skills/:id/reimport","pathname":"/skills/fixture-195-id%2Fraw/reimport","groups":{"id":"fixture-195-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", - "{"identity":"PATCH /skills/:id","pathname":"/skills/fixture-196-id%2Fraw","groups":{"id":"fixture-196-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", - "{"identity":"PUT /skills/:id","pathname":"/skills/fixture-197-id%2Fraw","groups":{"id":"fixture-197-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", - "{"identity":"DELETE /skills/:id","pathname":"/skills/fixture-198-id%2Fraw","groups":{"id":"fixture-198-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", + "{"identity":"POST /skills/:id/reimport/preview","pathname":"/skills/fixture-195-id%2Fraw/reimport/preview","groups":{"id":"fixture-195-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", + "{"identity":"POST /skills/:id/reimport","pathname":"/skills/fixture-196-id%2Fraw/reimport","groups":{"id":"fixture-196-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", + "{"identity":"PATCH /skills/:id","pathname":"/skills/fixture-197-id%2Fraw","groups":{"id":"fixture-197-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", + "{"identity":"PUT /skills/:id","pathname":"/skills/fixture-198-id%2Fraw","groups":{"id":"fixture-198-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", + "{"identity":"DELETE /skills/:id","pathname":"/skills/fixture-199-id%2Fraw","groups":{"id":"fixture-199-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skills.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"GET /skill-profiles","pathname":"/skill-profiles","groups":{},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skill_profiles.manage_own"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", "{"identity":"POST /skill-profiles","pathname":"/skill-profiles","groups":{},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skill_profiles.manage_own"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", - "{"identity":"PATCH /skill-profiles/:id","pathname":"/skill-profiles/fixture-201-id%2Fraw","groups":{"id":"fixture-201-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skill_profiles.manage_own"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", - "{"identity":"DELETE /skill-profiles/:id","pathname":"/skill-profiles/fixture-202-id%2Fraw","groups":{"id":"fixture-202-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skill_profiles.manage_own"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", + "{"identity":"PATCH /skill-profiles/:id","pathname":"/skill-profiles/fixture-202-id%2Fraw","groups":{"id":"fixture-202-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skill_profiles.manage_own"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", + "{"identity":"DELETE /skill-profiles/:id","pathname":"/skill-profiles/fixture-203-id%2Fraw","groups":{"id":"fixture-203-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"skill_profiles.manage_own"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"GET /keyboard-shortcuts","pathname":"/keyboard-shortcuts","groups":{},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":false},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", "{"identity":"PUT /keyboard-shortcuts","pathname":"/keyboard-shortcuts","groups":{},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"GET /memory-preferences","pathname":"/memory-preferences","groups":{},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", @@ -212,25 +213,25 @@ exports[`Hono route catalog conformance > dispatches every frozen method/path/po "{"identity":"POST /memories/preview","pathname":"/memories/preview","groups":{},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"sessions.create"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", "{"identity":"GET /memories","pathname":"/memories","groups":{},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", "{"identity":"POST /memories","pathname":"/memories","groups":{},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", - "{"identity":"GET /memories/:id","pathname":"/memories/fixture-210-id%2Fraw","groups":{"id":"fixture-210-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", - "{"identity":"PATCH /memories/:id","pathname":"/memories/fixture-211-id%2Fraw","groups":{"id":"fixture-211-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", - "{"identity":"GET /memories/:id/revisions","pathname":"/memories/fixture-212-id%2Fraw/revisions","groups":{"id":"fixture-212-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", - "{"identity":"POST /memories/:id/approve","pathname":"/memories/fixture-213-id%2Fraw/approve","groups":{"id":"fixture-213-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", - "{"identity":"POST /memories/:id/reject","pathname":"/memories/fixture-214-id%2Fraw/reject","groups":{"id":"fixture-214-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", - "{"identity":"POST /memories/:id/archive","pathname":"/memories/fixture-215-id%2Fraw/archive","groups":{"id":"fixture-215-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", - "{"identity":"POST /memories/:id/restore","pathname":"/memories/fixture-216-id%2Fraw/restore","groups":{"id":"fixture-216-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", + "{"identity":"GET /memories/:id","pathname":"/memories/fixture-211-id%2Fraw","groups":{"id":"fixture-211-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", + "{"identity":"PATCH /memories/:id","pathname":"/memories/fixture-212-id%2Fraw","groups":{"id":"fixture-212-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", + "{"identity":"GET /memories/:id/revisions","pathname":"/memories/fixture-213-id%2Fraw/revisions","groups":{"id":"fixture-213-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", + "{"identity":"POST /memories/:id/approve","pathname":"/memories/fixture-214-id%2Fraw/approve","groups":{"id":"fixture-214-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", + "{"identity":"POST /memories/:id/reject","pathname":"/memories/fixture-215-id%2Fraw/reject","groups":{"id":"fixture-215-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", + "{"identity":"POST /memories/:id/archive","pathname":"/memories/fixture-216-id%2Fraw/archive","groups":{"id":"fixture-216-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", + "{"identity":"POST /memories/:id/restore","pathname":"/memories/fixture-217-id%2Fraw/restore","groups":{"id":"fixture-217-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-self","auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", "{"identity":"GET /me/authorization","pathname":"/me/authorization","groups":{},"authentication":"user","authorization":{"kind":"authenticated","auditAllowed":false},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", "{"identity":"GET /roles","pathname":"/roles","groups":{},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"workspace.roles.read"}],"auditAllowed":false,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", - "{"identity":"GET /roles/:id","pathname":"/roles/fixture-219-id%2Fraw","groups":{"id":"fixture-219-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"workspace.roles.read"}],"auditAllowed":false,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", + "{"identity":"GET /roles/:id","pathname":"/roles/fixture-220-id%2Fraw","groups":{"id":"fixture-220-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"workspace.roles.read"}],"auditAllowed":false,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", "{"identity":"GET /members","pathname":"/members","groups":{},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"workspace.members.read"}],"auditAllowed":false,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", - "{"identity":"PUT /members/:id/role","pathname":"/members/fixture-221-id%2Fraw/role","groups":{"id":"fixture-221-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"workspace.members.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", - "{"identity":"PUT /members/:id/status","pathname":"/members/fixture-222-id%2Fraw/status","groups":{"id":"fixture-222-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"workspace.members.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", - "{"identity":"GET /teams/:id/secrets","pathname":"/teams/fixture-223-id%2Fraw/secrets","groups":{"id":"fixture-223-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"team","teamIdParam":"id","need":"canManageSecrets"}],"service":{"kind":"deny"},"auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", - "{"identity":"PUT /teams/:id/secrets","pathname":"/teams/fixture-224-id%2Fraw/secrets","groups":{"id":"fixture-224-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"team","teamIdParam":"id","need":"canManageSecrets"}],"service":{"kind":"deny"},"auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", - "{"identity":"DELETE /teams/:id/secrets/:key","pathname":"/teams/fixture-225-id%2Fraw/secrets/fixture-225-key%2Fraw","groups":{"id":"fixture-225-id%2Fraw","key":"fixture-225-key%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"team","teamIdParam":"id","need":"canManageSecrets"}],"service":{"kind":"deny"},"auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", - "{"identity":"POST /webhooks/sentry/:id","pathname":"/webhooks/sentry/fixture-226-id%2Fraw","groups":{"id":"fixture-226-id%2Fraw"},"authentication":"handler-authenticated","authorization":{"kind":"none","auditAllowed":false},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", - "{"identity":"POST /webhooks/automation/:id","pathname":"/webhooks/automation/fixture-227-id%2Fraw","groups":{"id":"fixture-227-id%2Fraw"},"authentication":"handler-authenticated","authorization":{"kind":"none","auditAllowed":false},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", - "{"identity":"GET /webhooks/automation/:id/invocations/:invocationId","pathname":"/webhooks/automation/fixture-228-id%2Fraw/invocations/fixture-228-invocationId%2Fraw","groups":{"id":"fixture-228-id%2Fraw","invocationId":"fixture-228-invocationId%2Fraw"},"authentication":"handler-authenticated","authorization":{"kind":"none","auditAllowed":false},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", + "{"identity":"PUT /members/:id/role","pathname":"/members/fixture-222-id%2Fraw/role","groups":{"id":"fixture-222-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"workspace.members.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", + "{"identity":"PUT /members/:id/status","pathname":"/members/fixture-223-id%2Fraw/status","groups":{"id":"fixture-223-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"workspace.members.manage"}],"auditAllowed":true,"service":{"kind":"actor"}},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", + "{"identity":"GET /teams/:id/secrets","pathname":"/teams/fixture-224-id%2Fraw/secrets","groups":{"id":"fixture-224-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"team","teamIdParam":"id","need":"canManageSecrets"}],"service":{"kind":"deny"},"auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", + "{"identity":"PUT /teams/:id/secrets","pathname":"/teams/fixture-225-id%2Fraw/secrets","groups":{"id":"fixture-225-id%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"team","teamIdParam":"id","need":"canManageSecrets"}],"service":{"kind":"deny"},"auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", + "{"identity":"DELETE /teams/:id/secrets/:key","pathname":"/teams/fixture-226-id%2Fraw/secrets/fixture-226-key%2Fraw","groups":{"id":"fixture-226-id%2Fraw","key":"fixture-226-key%2Fraw"},"authentication":"user","authorization":{"kind":"active-user","allOf":[{"kind":"team","teamIdParam":"id","need":"canManageSecrets"}],"service":{"kind":"deny"},"auditAllowed":true},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", + "{"identity":"POST /webhooks/sentry/:id","pathname":"/webhooks/sentry/fixture-227-id%2Fraw","groups":{"id":"fixture-227-id%2Fraw"},"authentication":"handler-authenticated","authorization":{"kind":"none","auditAllowed":false},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", + "{"identity":"POST /webhooks/automation/:id","pathname":"/webhooks/automation/fixture-228-id%2Fraw","groups":{"id":"fixture-228-id%2Fraw"},"authentication":"handler-authenticated","authorization":{"kind":"none","auditAllowed":false},"supportedScmProviders":"all","cacheControl":null,"hasServiceActorClaims":false}", + "{"identity":"GET /webhooks/automation/:id/invocations/:invocationId","pathname":"/webhooks/automation/fixture-229-id%2Fraw/invocations/fixture-229-invocationId%2Fraw","groups":{"id":"fixture-229-id%2Fraw","invocationId":"fixture-229-invocationId%2Fraw"},"authentication":"handler-authenticated","authorization":{"kind":"none","auditAllowed":false},"supportedScmProviders":"all","cacheControl":"private, no-store","hasServiceActorClaims":false}", "{"identity":"POST /internal/github-event","pathname":"/internal/github-event","groups":{},"authentication":"service","authorization":{"kind":"service","services":["github-bot"],"actor":"optional","auditAllowed":true},"supportedScmProviders":["github"],"cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"POST /internal/slack-event","pathname":"/internal/slack-event","groups":{},"authentication":"service","authorization":{"kind":"service","services":["slack-bot"],"actor":"optional","auditAllowed":true},"supportedScmProviders":["github"],"cacheControl":null,"hasServiceActorClaims":false}", "{"identity":"GET /github/route","pathname":"/github/route","groups":{},"authentication":"service","authorization":{"kind":"active-user","allOf":[{"kind":"permission","permission":"repositories.read"}],"auditAllowed":false,"service":{"kind":"actor","actorlessGrants":[{"service":"github-bot"}]}},"supportedScmProviders":["github"],"cacheControl":"private, no-store","hasServiceActorClaims":true}", diff --git a/packages/control-plane/test/integration/__snapshots__/route-admission-matrix.test.ts.snap b/packages/control-plane/test/integration/__snapshots__/route-admission-matrix.test.ts.snap index 47eb80b017..40da59172e 100644 --- a/packages/control-plane/test/integration/__snapshots__/route-admission-matrix.test.ts.snap +++ b/packages/control-plane/test/integration/__snapshots__/route-admission-matrix.test.ts.snap @@ -201,6 +201,7 @@ exports[`route admission matrix > admits the workspace owner through every brows "GET /mcp-servers/:id owner=404", "PUT /mcp-servers/:id owner=404", "DELETE /mcp-servers/:id owner=404", + "POST /mcp-servers/:id/tools owner=404", "GET /analytics/dashboard owner=200", "GET /analytics/summary owner=200", "GET /analytics/timeseries owner=200", @@ -432,6 +433,7 @@ exports[`route admission matrix > rejects every credentialed route anonymously b "GET /mcp-servers/:id anonymous=401", "PUT /mcp-servers/:id anonymous=401", "DELETE /mcp-servers/:id anonymous=401", + "POST /mcp-servers/:id/tools anonymous=401", "GET /analytics/dashboard anonymous=401", "GET /analytics/summary anonymous=401", "GET /analytics/timeseries anonymous=401", diff --git a/packages/control-plane/test/integration/hono-route-catalog-conformance.test.ts b/packages/control-plane/test/integration/hono-route-catalog-conformance.test.ts index 2d2a26cbee..2800b922ab 100644 --- a/packages/control-plane/test/integration/hono-route-catalog-conformance.test.ts +++ b/packages/control-plane/test/integration/hono-route-catalog-conformance.test.ts @@ -45,7 +45,7 @@ describe("Hono route catalog conformance", () => { }; }); - expect(manifest).toHaveLength(232); + expect(manifest).toHaveLength(233); // One compact, reviewable line per frozen route keeps the fixture explicit // without thousands of snapshot-only formatting lines. expect(manifest.map((entry) => JSON.stringify(entry))).toMatchSnapshot(); diff --git a/packages/control-plane/test/integration/mcp-servers.test.ts b/packages/control-plane/test/integration/mcp-servers.test.ts index 7d58fd8648..066dd46ef2 100644 --- a/packages/control-plane/test/integration/mcp-servers.test.ts +++ b/packages/control-plane/test/integration/mcp-servers.test.ts @@ -13,9 +13,25 @@ interface McpServerMetadata { hasEnv: boolean; hasHeaders: boolean; repoScopes: string[] | null; + toolAllowlist: string[] | null; enabled: boolean; } +async function createRemote(body: Record = {}): Promise { + const response = await serviceFetch("https://test.local/mcp-servers", { + method: "POST", + body: JSON.stringify({ + name: "docs", + type: "remote", + url: "https://mcp.integration.test/mcp", + headers: { Authorization: "Bearer integration-mcp-token" }, + ...body, + }), + }); + expect(response.status).toBe(201); + return response.json(); +} + describe("MCP Servers API", () => { beforeEach(cleanD1Tables); @@ -487,4 +503,102 @@ describe("MCP Servers API", () => { expect(response.status).toBe(404); }); }); + + describe("tool allowlists", () => { + it("exposes every tool by default and keeps an empty allowlist distinct", async () => { + expect((await createRemote()).toolAllowlist).toBeNull(); + const none = await createRemote({ name: "none", toolAllowlist: [] }); + expect(none.toolAllowlist).toEqual([]); + + const response = await serviceFetch(`https://test.local/mcp-servers/${none.id}`); + expect((await response.json()).toolAllowlist).toEqual([]); + }); + + it("normalizes, keeps and clears an allowlist across updates", async () => { + const server = await createRemote(); + const set = await serviceFetch(`https://test.local/mcp-servers/${server.id}`, { + method: "PUT", + body: JSON.stringify({ toolAllowlist: ["search", " fetch_page ", "search"], revision: 1 }), + }); + expect(set.status).toBe(200); + expect((await set.json()).toolAllowlist).toEqual(["fetch_page", "search"]); + + const renamed = await serviceFetch(`https://test.local/mcp-servers/${server.id}`, { + method: "PUT", + body: JSON.stringify({ name: "docs-renamed", revision: 2 }), + }); + expect((await renamed.json()).toolAllowlist).toEqual([ + "fetch_page", + "search", + ]); + + const cleared = await serviceFetch(`https://test.local/mcp-servers/${server.id}`, { + method: "PUT", + body: JSON.stringify({ toolAllowlist: null, revision: 3 }), + }); + expect((await cleared.json()).toolAllowlist).toBeNull(); + }); + + it("rejects an allowlist that is not a list of tool names", async () => { + const server = await createRemote(); + const response = await serviceFetch(`https://test.local/mcp-servers/${server.id}`, { + method: "PUT", + body: JSON.stringify({ toolAllowlist: "search", revision: 1 }), + }); + expect(response.status).toBe(400); + }); + }); + + describe("POST /mcp-servers/:id/tools", () => { + it("lists a remote server's tools using its stored headers", async () => { + const server = await createRemote(); + const response = await serviceFetch(`https://test.local/mcp-servers/${server.id}/tools`, { + method: "POST", + }); + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ + tools: [{ name: "fetch_page" }, { name: "search", description: "Search the docs" }], + }); + }); + + it("does not repeat the server's response when it does not speak MCP", async () => { + const server = await createRemote({ url: "https://mcp.integration.test/not-mcp" }); + const response = await serviceFetch(`https://test.local/mcp-servers/${server.id}/tools`, { + method: "POST", + }); + expect(response.status).toBe(502); + await expect(response.json<{ error: string }>()).resolves.toEqual({ + error: "Could not load tools from the MCP server", + }); + }); + + it("reports a server that rejects the stored credentials as a bad gateway", async () => { + const server = await createRemote({ url: "https://mcp.integration.test/unauthorized" }); + const response = await serviceFetch(`https://test.local/mcp-servers/${server.id}/tools`, { + method: "POST", + }); + expect(response.status).toBe(502); + await expect(response.json<{ error: string }>()).resolves.toEqual({ + error: + "Could not load tools from the MCP server: The server returned HTTP 401; check its headers", + }); + }); + + it("refuses local servers and unknown ids", async () => { + const local = await serviceFetch("https://test.local/mcp-servers", { + method: "POST", + body: JSON.stringify({ name: "local", type: "local", command: ["npx", "server"] }), + }); + const { id } = await local.json(); + const localTools = await serviceFetch(`https://test.local/mcp-servers/${id}/tools`, { + method: "POST", + }); + expect(localTools.status).toBe(400); + + const missing = await serviceFetch("https://test.local/mcp-servers/missing/tools", { + method: "POST", + }); + expect(missing.status).toBe(404); + }); + }); }); diff --git a/packages/control-plane/vitest.integration.config.ts b/packages/control-plane/vitest.integration.config.ts index a087266419..b1de0dfc7f 100644 --- a/packages/control-plane/vitest.integration.config.ts +++ b/packages/control-plane/vitest.integration.config.ts @@ -16,6 +16,59 @@ const migrationsPath = path.resolve(__dirname, "../../terraform/d1/migrations"); // which interops correctly. Test-only — production bundles via esbuild/wrangler. const luxonCjsEntry = createRequire(__filename).resolve("luxon"); +interface McpMessage { + id?: number; + method: string; + params?: { protocolVersion?: string }; +} + +/** The result the fake MCP server returns for a request, or undefined for a notification. */ +function fakeMcpResult(message: McpMessage): unknown { + if (message.method === "initialize") { + return { + protocolVersion: message.params?.protocolVersion, + capabilities: { tools: {} }, + serverInfo: { name: "integration-mcp", version: "1.0.0" }, + }; + } + if (message.method === "tools/list") { + return { + tools: [ + { name: "search", description: "Search the docs", inputSchema: { type: "object" } }, + { + name: "fetch_page", + inputSchema: { type: "object" }, + outputSchema: { type: "object", properties: { text: { type: "string" } } }, + }, + ], + }; + } + return undefined; +} + +/** + * A Streamable HTTP MCP server for tool-discovery tests at mcp.integration.test. + * Every path requires the bearer token and advertises the same tools, one with + * an output schema; `/unauthorized` rejects every request and `/not-mcp` + * answers with a web page. + */ +async function fakeMcpServer(request: Request): Promise { + const url = new URL(request.url); + if (url.pathname === "/unauthorized") return new Response("Unauthorized", { status: 401 }); + if (request.headers.get("authorization") !== "Bearer integration-mcp-token") { + return new Response("Unauthorized", { status: 401 }); + } + if (url.pathname === "/not-mcp") { + return new Response("Docs", { headers: { "content-type": "text/html" } }); + } + + if (request.method !== "POST") return new Response(null, { status: 405 }); + const message = (await request.json()) as McpMessage; + const result = fakeMcpResult(message); + if (result === undefined) return new Response(null, { status: 202 }); + return Response.json({ jsonrpc: "2.0", id: message.id, result }); +} + /** Generate a random base64-encoded 32-byte AES key for tests. */ function generateTestEncryptionKey(): string { const key = webcrypto.getRandomValues(new Uint8Array(32)); @@ -51,6 +104,7 @@ export default defineConfig({ compatibilityFlags: ["nodejs_compat"], async outboundService(request: Request) { const url = new URL(request.url); + if (url.hostname === "mcp.integration.test") return fakeMcpServer(request); if (url.hostname.endsWith(".modal.run")) { return new Response("Modal is unavailable in integration tests", { status: 404 }); } diff --git a/packages/docs/content/docs/configure/mcp-servers.mdx b/packages/docs/content/docs/configure/mcp-servers.mdx index 660ca8ec4c..9ea2e29d4f 100644 --- a/packages/docs/content/docs/configure/mcp-servers.mdx +++ b/packages/docs/content/docs/configure/mcp-servers.mdx @@ -1,14 +1,17 @@ --- title: MCP servers -description: Give agents extra tools by registering local or remote MCP servers, scoping them to repositories, and keeping local servers fast to start. +description: Give agents extra tools by registering local or remote MCP servers, scoping them to repositories, choosing which of their tools agents see, and keeping local servers fast to start. audience: team-owner owner: platform status: published -lastReviewed: "2026-09-23" +lastReviewed: "2026-10-09" relatedCode: - packages/shared/src/types/integrations.ts - packages/web/src/components/settings/mcp-servers-settings.tsx + - packages/web/src/components/settings/mcp-tool-access.tsx - packages/control-plane/src/routes/mcp-servers.ts + - packages/control-plane/src/mcp/tool-discovery.ts + - packages/sandbox-runtime/src/sandbox_runtime/mcp_tool_access.py - docs/HOW_IT_WORKS.md - docs/CLAUDE_AGENT.md --- @@ -48,7 +51,7 @@ For a remote server, enter the URL, such as `https://mcp.example.com/sse`. Optio ### Availability and state -Choose "All repositories" (available in every agent session) or "Selected repositories only" and tick the repositories. Leave Enabled on, then click Add Server. +Choose "All repositories" (available in every agent session) or "Selected repositories only" and tick the repositories. Leave Enabled on, then click Add Server. To limit the server's tools, see [Limit the tools an agent sees](#limit-the-tools-an-agent-sees). @@ -72,12 +75,36 @@ Credentials you enter as environment variables or headers are stored encrypted o Edit a server from the list to change any field and click Save Changes. Deleting a server cannot be undone. +## Limit the tools an agent sees + +Every tool a server offers adds its name, description, and input schema to the agent's context. A server with many tools can take up a large part of the context window in every request, even when a session only needs a few of them. Under Tools, choose: + +| Option | Effect | +| ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| All tools | The default. The agent sees every tool the server offers, including tools the server adds later. | +| Selected tools only | The agent can use only the tools you choose. Tools the server adds later stay off until you select them. With no tool selected, the server is left out of sessions. | + +To choose tools for a remote server, save it first, then open it and click Load tools. OpenInspect connects to the server with its saved URL and headers and lists the tools it offers; tick the ones the agent should use. Loading tools requires the `mcp_servers.manage` permission. If you edit the URL or headers, save before loading so the list comes from the new connection. + +For a local server, or when a remote server cannot be reached from OpenInspect, type each tool name and click Add. Use the names the server advertises, such as `search_issues`. After you load tools, a selected tool that the server no longer offers is listed under "Selected but not offered by the server" so you can remove it. + +The list shows the number of selected tools next to each restricted server. + +How each harness applies the selection: + +- OpenCode: tools outside the selection are left out of every request, so they take no context. +- Claude Agent: only the selected tools are allowed, and calls to any other tool of that server are refused, even when Claude settings in the repository allow them. Those tools can still appear in Claude Agent's tool list. + +Both harnesses build a tool's name from the server and tool names, so they cannot always tell apart servers whose names overlap. While a server such as `docs` is limited, a server whose name extends it (`docs_extra` in OpenCode, `docs__extra` in either harness) gets no tools rather than expose the limited server's other tools. Rename one of them to avoid this. + +A selection takes effect the next time a session's sandbox starts; a running session keeps the tools it started with. + ## How servers reach the agent - OpenCode: enabled servers in scope are written into the session's OpenCode configuration when the agent starts. -- Claude Agent: the same servers are passed through unchanged to the harness. OpenInspect's own tools reach Claude Agent through a built-in server named `oi`, separate from the servers you register. +- Claude Agent: the same servers are passed to the harness. OpenInspect's own tools reach Claude Agent through a built-in server named `oi`, separate from the servers you register. -Server commands, arguments, and credentials are passed through as configured. Changes apply to new sessions; a running session keeps the server set it started with. +Server commands, arguments, and credentials are passed through as configured, and each server's tool selection is applied as described in [Limit the tools an agent sees](#limit-the-tools-an-agent-sees). Changes apply to new sessions; a running session keeps the server set it started with. ## Startup cost of local servers @@ -122,6 +149,21 @@ See [lifecycle scripts](/configure/lifecycle-scripts) and [prebuilt images](/con image has not been rebuilt since the change. Fix: use the exact same `package@version` in both places and rebuild the prebuilt image. + + + Cause: the server is unreachable from OpenInspect, rejects the saved headers, or does not + answer within 15 seconds. The message includes the HTTP status when the server returned one; + 401 or 403 means it rejected the headers. Fix: check the URL and headers, save, and try again. + You can also type tool names by hand. + + + + Cause: the server is set to "Selected tools only" and the tool is not selected, or its name + does not match the name the server advertises. A server whose name extends a limited + server's name, such as `docs_extra` next to `docs`, gets no tools. Fix: open + the server, click Load tools and tick the tool (for a local server, type its exact name), then + start a new session. For a name clash, rename one of the servers. + Cause: the name is empty, a remote server has no URL, a local server has no command, or diff --git a/packages/docs/content/docs/models/agent-harnesses.mdx b/packages/docs/content/docs/models/agent-harnesses.mdx index 3a09c9f5c8..8d36535536 100644 --- a/packages/docs/content/docs/models/agent-harnesses.mdx +++ b/packages/docs/content/docs/models/agent-harnesses.mdx @@ -4,7 +4,7 @@ description: Compare the two agent harnesses, choose one when you create a sessi audience: team-owner owner: platform status: published -lastReviewed: "2026-10-08" +lastReviewed: "2026-10-09" relatedCode: - packages/shared/src/harnesses.ts - docs/CLAUDE_AGENT.md @@ -89,7 +89,7 @@ OpenInspect's own tools are served to Claude Agent in-process as the `oi` MCP se | `create-pull-request` | When the session has a repository | | `slack-notify` | When agent notifications are enabled for the repository | -Session MCP servers configured under Settings › MCP Servers are passed through unchanged. +Session MCP servers configured under Settings › MCP Servers are passed through with their tool selection applied: Claude Agent allows only a restricted server's selected tools, and a server with none selected is left out. See [MCP servers](/configure/mcp-servers#limit-the-tools-an-agent-sees). ### Sub-agents diff --git a/packages/modal-infra/tests/test_web_api_create_sandbox.py b/packages/modal-infra/tests/test_web_api_create_sandbox.py index 393c5a96cf..d2bf197ef4 100644 --- a/packages/modal-infra/tests/test_web_api_create_sandbox.py +++ b/packages/modal-infra/tests/test_web_api_create_sandbox.py @@ -995,3 +995,26 @@ async def create_sandbox(self, config): assert exc_info.value.status_code == status assert exc_info.value.detail == detail + + +def test_session_config_helper_keeps_mcp_tool_allowlists(): + """Rebuilding SessionConfig must not drop a server's tool allowlist.""" + servers = [ + {"id": "a", "name": "docs", "type": "remote", "url": "https://mcp.docs"}, + { + "id": "b", + "name": "linear", + "type": "remote", + "url": "https://mcp.linear", + "toolAllowlist": ["list_issues"], + }, + {"id": "c", "name": "noisy", "type": "local", "command": ["npx"], "toolAllowlist": []}, + ] + config = web_api._session_config_from_create_request( + {"session_id": "s1", "mcp_servers": servers}, + repo_owner="acme", + repo_name="app", + ) + + dumped = config.model_dump(mode="json")["mcp_servers"] + assert [server.get("toolAllowlist") for server in dumped] == [None, ["list_issues"], []] diff --git a/packages/sandbox-runtime/src/sandbox_runtime/harness/claude.py b/packages/sandbox-runtime/src/sandbox_runtime/harness/claude.py index 44c780ae89..6b91db3504 100644 --- a/packages/sandbox-runtime/src/sandbox_runtime/harness/claude.py +++ b/packages/sandbox-runtime/src/sandbox_runtime/harness/claude.py @@ -17,7 +17,7 @@ from dataclasses import dataclass from typing import TYPE_CHECKING, Any, Final, Protocol -from claude_agent_sdk import ClaudeAgentOptions, ClaudeSDKClient +from claude_agent_sdk import ClaudeAgentOptions, ClaudeSDKClient, HookMatcher from ..attachment_processor import ( MAX_SESSION_ATTACHMENTS_PER_MESSAGE, @@ -28,6 +28,12 @@ RuntimeCredentialDenied, RuntimeCredentialUnavailable, ) +from ..mcp_tool_access import ( + claude_tool_guard, + claude_tool_rules, + mcp_name_segment, + namespace_allowlists, +) from .base import ( EventSink, HarnessId, @@ -315,13 +321,23 @@ def build_options(self, model: str, reasoning_effort: str | None) -> Any: raise RuntimeError("Claude harness is not open") mcp_servers: dict[str, Any] = mcp_server_options(self.config.mcp_servers) allowed_tools = [*ALLOWED_TOOLS] - allowed_tools.extend(f"mcp__{name}__*" for name in mcp_servers) + disallowed_tools = [*DISALLOWED_TOOLS] + allowlists = namespace_allowlists( + server for server in self.config.mcp_servers if str(server.get("name")) in mcp_servers + ) + for name in mcp_servers: + allowed, disallowed = claude_tool_rules(name, allowlists[mcp_name_segment(name)]) + allowed_tools.extend(allowed) + disallowed_tools.extend(disallowed) if self._tool_client is not None: if self._tool_server is None: factory = self._tool_server_factory or _default_tool_server self._tool_server = factory(self._tool_client) mcp_servers[OI_TOOL_SERVER_NAME] = self._tool_server allowed_tools.append(f"mcp__{OI_TOOL_SERVER_NAME}__*") + # The built-in server replaces a configured one of the same name. + allowlists.pop(OI_TOOL_SERVER_NAME, None) + tool_guard = claude_tool_guard(allowlists) system_prompt: dict[str, Any] = {"type": "preset", "preset": "claude_code"} if self.config.system_prompt_append: system_prompt["append"] = self.config.system_prompt_append @@ -332,7 +348,7 @@ def build_options(self, model: str, reasoning_effort: str | None) -> Any: "model": model, "mcp_servers": mcp_servers, "allowed_tools": allowed_tools, - "disallowed_tools": [*DISALLOWED_TOOLS], + "disallowed_tools": disallowed_tools, "permission_mode": "dontAsk", "system_prompt": system_prompt, "settings": CLAUDE_POLICY_SETTINGS, @@ -343,6 +359,8 @@ def build_options(self, model: str, reasoning_effort: str | None) -> Any: "stderr": self._trajectory.stderr, **reasoning_options(model, reasoning_effort), } + if tool_guard is not None: + kwargs["hooks"] = {"PreToolUse": [HookMatcher(hooks=[tool_guard])]} if self._resume_on_connect: kwargs["resume"] = self.session_id else: diff --git a/packages/sandbox-runtime/src/sandbox_runtime/mcp_tool_access.py b/packages/sandbox-runtime/src/sandbox_runtime/mcp_tool_access.py new file mode 100644 index 0000000000..0ae3db1130 --- /dev/null +++ b/packages/sandbox-runtime/src/sandbox_runtime/mcp_tool_access.py @@ -0,0 +1,189 @@ +"""Per-server MCP tool allowlists, rendered for each agent harness. + +A server config's ``toolAllowlist`` names the server's own tools the agent may +use. Absent or ``None`` allows every tool; a list allows only those names, so +an empty list allows none. +""" + +from __future__ import annotations + +import fnmatch +import re +from typing import TYPE_CHECKING, Any + +if TYPE_CHECKING: + from collections.abc import Iterable, Mapping + + from claude_agent_sdk.types import HookCallback, HookContext, HookInput, SyncHookJSONOutput + + Allowlists = dict[str, tuple[str, ...] | None] + +# OpenCode and Claude Code both build an MCP tool's name from the server and tool +# names with every character outside this set replaced by "_". +_UNSAFE_NAME_CHARS = re.compile(r"[^a-zA-Z0-9_-]") + +# OpenCode's built-in tools (``GET /experimental/tool/ids``) at the version pinned +# in packages/sandbox-images/toolchain.json. OpenCode names an MCP tool +# ``_`` with no namespace marker, so a server's deny rule can also +# match a built-in such as ``apply_patch``; these are re-allowed after the MCP +# rules. Re-run the wire contract test in test_opencode_reasoning_contract.py +# when bumping OpenCode. +OPENCODE_BUILTIN_TOOL_IDS = ( + "apply_patch", + "bash", + "edit", + "glob", + "grep", + "read", + "skill", + "task", + "todowrite", + "webfetch", + "websearch", + "write", +) + + +def mcp_name_segment(value: str) -> str: + """One server or tool name, as both harnesses spell it inside a tool name.""" + # Both harnesses run JavaScript, whose regex replaces UTF-16 code units: a + # character outside the Basic Multilingual Plane becomes two underscores. + return _UNSAFE_NAME_CHARS.sub(lambda match: "_" * (2 if ord(match[0]) > 0xFFFF else 1), value) + + +def tool_allowlist(server: Mapping[str, Any]) -> tuple[str, ...] | None: + """The server's allowlist; a malformed value allows no tools rather than all.""" + raw = server.get("toolAllowlist") + if raw is None: + return None + if not isinstance(raw, (list, tuple)): + return () + return tuple(tool for tool in raw if isinstance(tool, str) and tool) + + +def namespace_allowlists(servers: Iterable[Mapping[str, Any]]) -> Allowlists: + """Each tool-name namespace's allowlist, keyed by sanitized server name. + + Servers whose names differ only in sanitized characters (``my.docs`` and + ``my_docs``) produce the same tool names, so they share one entry. It is + restricted if any of them is, to the tools every restricted one allows. + """ + allowlists: Allowlists = {} + for server in servers: + if not server.get("name"): + continue + namespace = mcp_name_segment(str(server["name"])) + allowlist = tool_allowlist(server) + current = allowlists.get(namespace) + if current is None: + allowlists[namespace] = allowlist + elif allowlist is not None: + allowlists[namespace] = tuple(tool for tool in current if tool in allowlist) + return allowlists + + +def opencode_shadowed_namespaces(allowlists: Allowlists) -> list[str]: + """Unrestricted namespaces that extend a restricted one (``docs_extra`` next + to ``docs``). OpenCode names tools ``_``, so ``docs_extra_*`` + would also match ``docs``' own ``extra_*`` tools; these servers get none of + their own tools rather than widen the restricted server's allowlist.""" + restricted = [namespace for namespace, allowlist in allowlists.items() if allowlist is not None] + return sorted( + namespace + for namespace, allowlist in allowlists.items() + if allowlist is None and any(namespace.startswith(f"{other}_") for other in restricted) + ) + + +def _set_last(rules: dict[str, str], key: str, action: str) -> None: + # OpenCode applies the last matching rule, so a re-asserted key must move + # to the end rather than keep its first position. + rules.pop(key, None) + rules[key] = action + + +def opencode_permission_rules( + servers: Iterable[Mapping[str, Any]], other_tool_ids: Iterable[str] +) -> dict[str, str]: + """OpenCode ``permission`` rules that hide every tool outside an allowlist. + + Denied tools are left out of the model request, not only refused when + called. Rules follow the global allow, ordered so that a restricted + namespace extending another (``foo_bar`` after ``foo``) keeps its own rules, + and ``other_tool_ids`` (built-in and custom tools) caught by a deny + pattern are re-allowed last. Returns no rules when nothing is restricted, + leaving the global allow as the only rule. + """ + allowlists = namespace_allowlists(servers) + if all(allowlist is None for allowlist in allowlists.values()): + return {} + + shadowed = set(opencode_shadowed_namespaces(allowlists)) + rules: dict[str, str] = {} + for namespace in sorted(allowlists, key=len): + allowlist = allowlists[namespace] + if allowlist is None: + if namespace not in shadowed: + _set_last(rules, f"{namespace}_*", "allow") + continue + _set_last(rules, f"{namespace}_*", "deny") + for tool in allowlist: + _set_last(rules, f"{namespace}_{mcp_name_segment(tool)}", "allow") + + denied = [pattern for pattern, action in rules.items() if action == "deny"] + for tool_id in other_tool_ids: + if any(fnmatch.fnmatchcase(tool_id, pattern) for pattern in denied): + _set_last(rules, tool_id, "allow") + return rules + + +def claude_tool_rules(name: str, allowlist: tuple[str, ...] | None) -> tuple[list[str], list[str]]: + """``(allowed_tools, disallowed_tools)`` entries for one Claude MCP server. + + Under ``dontAsk`` a listed tool runs and any other is refused, unless another + approval covers it; ``claude_tool_guard`` refuses those. Claude Code removes + a tool from context only when a deny rule names it, and the runtime does not + know a server's other tool names, so they stay visible but are refused; an + empty allowlist denies the whole server with a bare server rule. + """ + prefix = f"mcp__{mcp_name_segment(name)}" + if allowlist is None: + return [f"{prefix}__*"], [] + if not allowlist: + return [], [prefix] + return [f"{prefix}__{mcp_name_segment(tool)}" for tool in allowlist], [] + + +def claude_tool_guard(allowlists: Allowlists) -> HookCallback | None: + """A ``PreToolUse`` hook refusing every unselected tool of a restricted server. + + Approvals add up across sources: a ``mcp__docs__*`` rule in the session's + Claude settings, or the wildcard of a server whose name overlaps + (``mcp__docs__*`` also matches ``docs__extra``'s tools, since Claude Code + splits names on ``__``), would otherwise approve them. A hook's deny wins. + """ + restricted = {ns: allowlist for ns, allowlist in allowlists.items() if allowlist is not None} + if not restricted: + return None + prefixes = tuple(f"mcp__{namespace}__" for namespace in restricted) + allowed = frozenset( + f"mcp__{namespace}__{mcp_name_segment(tool)}" + for namespace, allowlist in restricted.items() + for tool in allowlist + ) + + async def guard( + hook_input: HookInput, _tool_use_id: str | None, _context: HookContext + ) -> SyncHookJSONOutput: + tool_name = str(hook_input.get("tool_name", "")) + if not tool_name.startswith(prefixes) or tool_name in allowed: + return {} + return { + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": "This MCP tool is not enabled for this server.", + } + } + + return guard diff --git a/packages/sandbox-runtime/src/sandbox_runtime/opencode_server.py b/packages/sandbox-runtime/src/sandbox_runtime/opencode_server.py index ef3a5c874e..7742c1f08b 100644 --- a/packages/sandbox-runtime/src/sandbox_runtime/opencode_server.py +++ b/packages/sandbox-runtime/src/sandbox_runtime/opencode_server.py @@ -15,6 +15,12 @@ from .constants import OPENCODE_PORT from .git_excludes import install_runtime_git_excludes from .mcp_packages import McpPackageInstaller +from .mcp_tool_access import ( + OPENCODE_BUILTIN_TOOL_IDS, + namespace_allowlists, + opencode_permission_rules, + opencode_shadowed_namespaces, +) from .memories import memory_path from .process_output import iter_process_lines from .sandbox_bin import install_bin_scripts @@ -30,6 +36,21 @@ AGENT_TOOLS_REQUIRING_REPOSITORY: set[str] = set() +def _custom_tool_ids(workdir: Path) -> list[str]: + """Custom tools OpenCode loads from the project and global config, both + Open-Inspect's and the repository's own, named after their files.""" + config_dirs = (workdir / ".opencode", resolve_opencode_global_config_dir()) + return sorted( + { + path.stem + for config_dir in config_dirs + for subdirectory in ("tool", "tools") + for path in (config_dir / subdirectory).glob("*") + if path.suffix in {".js", ".ts"} + } + ) + + def resolve_opencode_global_config_dir() -> Path: """Resolve OpenCode's global config directory using its xdg-basedir rules.""" override = os.environ.get("OPENCODE_CONFIG_DIR") @@ -431,6 +452,15 @@ async def start(self, repositories: Sequence[RepoEntry], workdir: Path) -> None: # Working directory: the repo for single-repo sessions, /workspace # for multi-repo (every member visible) and repo-less sessions. installed_runtime_paths = self._prepare_opencode_filesystem(workdir, repositories) + tool_rules = opencode_permission_rules( + mcp_servers, (*OPENCODE_BUILTIN_TOOL_IDS, *_custom_tool_ids(workdir)) + ) + if tool_rules: + opencode_config["permission"].update(tool_rules) + self.log.info("mcp.tool_allowlists_applied", rule_count=len(tool_rules)) + shadowed = opencode_shadowed_namespaces(namespace_allowlists(mcp_servers)) + if shadowed: + self.log.warn("mcp.tool_allowlist_shadowed_servers", namespaces=shadowed) # Deploy auth proxy plugins for control-plane-managed subscriptions. opencode_dir = workdir / ".opencode" managed_plugins = ( diff --git a/packages/sandbox-runtime/src/sandbox_runtime/runtime_manifest.json b/packages/sandbox-runtime/src/sandbox_runtime/runtime_manifest.json index 1d4edd80e8..0c56d5e079 100644 --- a/packages/sandbox-runtime/src/sandbox_runtime/runtime_manifest.json +++ b/packages/sandbox-runtime/src/sandbox_runtime/runtime_manifest.json @@ -1,11 +1,11 @@ { - "runtimeVersion": "v74-persistent-memory", - "generation": 74, + "runtimeVersion": "v75-mcp-tool-allowlist", + "generation": 75, "minimumCompatibleGeneration": 62, "minimumPreservationGeneration": 71, - "minimumRebuildGeneration": 74, + "minimumRebuildGeneration": 75, "harnessMinimumGeneration": { - "claude": 74, - "opencode": 74 + "claude": 75, + "opencode": 75 } } diff --git a/packages/sandbox-runtime/src/sandbox_runtime/types.py b/packages/sandbox-runtime/src/sandbox_runtime/types.py index ba17e3e961..3898e8c3fd 100644 --- a/packages/sandbox-runtime/src/sandbox_runtime/types.py +++ b/packages/sandbox-runtime/src/sandbox_runtime/types.py @@ -38,6 +38,7 @@ class McpServerConfig(TypedDict, total=False): env: dict[str, str] headers: dict[str, str] repoScopes: list[str] | None + toolAllowlist: list[str] | None enabled: bool diff --git a/packages/sandbox-runtime/tests/fixtures/mcp_stdio_server.py b/packages/sandbox-runtime/tests/fixtures/mcp_stdio_server.py new file mode 100644 index 0000000000..f90badd73a --- /dev/null +++ b/packages/sandbox-runtime/tests/fixtures/mcp_stdio_server.py @@ -0,0 +1,43 @@ +"""Minimal stdio MCP server for wire tests: advertises the tools named in argv.""" + +import json +import sys + +TOOLS = sys.argv[1:] + + +def reply(message_id, result): + sys.stdout.write(json.dumps({"jsonrpc": "2.0", "id": message_id, "result": result}) + "\n") + sys.stdout.flush() + + +for line in sys.stdin: + message = json.loads(line) + method = message.get("method") + if "id" not in message: + continue + if method == "initialize": + reply( + message["id"], + { + "protocolVersion": message["params"]["protocolVersion"], + "capabilities": {"tools": {}}, + "serverInfo": {"name": "wire-test", "version": "1.0.0"}, + }, + ) + elif method == "tools/list": + reply( + message["id"], + { + "tools": [ + { + "name": name, + "description": f"Wire test tool {name}", + "inputSchema": {"type": "object", "properties": {}}, + } + for name in TOOLS + ] + }, + ) + else: + reply(message["id"], {}) diff --git a/packages/sandbox-runtime/tests/test_claude_harness.py b/packages/sandbox-runtime/tests/test_claude_harness.py index 84a50c81cb..19cc90b387 100644 --- a/packages/sandbox-runtime/tests/test_claude_harness.py +++ b/packages/sandbox-runtime/tests/test_claude_harness.py @@ -209,6 +209,86 @@ async def test_options_follow_the_design_mapping(self, tmp_path: Path) -> None: assert "mcp__local__*" in options["allowed_tools"] assert "Bash" in options["allowed_tools"] + @pytest.mark.asyncio + async def test_tool_allowlists_limit_each_servers_tools(self, tmp_path: Path) -> None: + h = Harness( + tmp_path, + turns=[[_result(0.1)]], + mcp_servers=( + {"name": "linear", "type": "remote", "url": "https://mcp.linear"}, + { + "name": "docs", + "type": "remote", + "url": "https://mcp.docs", + "toolAllowlist": ["search", "fetch/page"], + }, + { + "name": "noisy", + "type": "local", + "command": ["npx", "noisy"], + "toolAllowlist": [], + }, + ), + ) + await h.harness.open() + await h.harness.create_session() + await _run( + h.harness, + HarnessPrompt(message_id="m1", text="hi", model="m"), + ) + options = h.client.options + mcp_rules = [tool for tool in options["allowed_tools"] if tool.startswith("mcp__")] + assert mcp_rules[:3] == [ + "mcp__linear__*", + "mcp__docs__search", + "mcp__docs__fetch_page", + ] + assert "mcp__docs__*" not in options["allowed_tools"] + assert not any(tool.startswith("mcp__noisy") for tool in options["allowed_tools"]) + assert options["disallowed_tools"] == ["AskUserQuestion", "mcp__noisy"] + assert set(options["mcp_servers"]) >= {"linear", "docs", "noisy"} + # Approvals from other sources add up, so a hook refuses the unselected tools. + [matcher] = options["hooks"]["PreToolUse"] + [guard] = matcher.hooks + denied = await guard({"tool_name": "mcp__docs__delete"}, None, None) + assert denied["hookSpecificOutput"]["permissionDecision"] == "deny" + assert await guard({"tool_name": "mcp__docs__fetch_page"}, None, None) == {} + assert await guard({"tool_name": "mcp__linear__anything"}, None, None) == {} + + async def test_no_tool_guard_without_allowlists(self, tmp_path: Path) -> None: + h = Harness( + tmp_path, + turns=[[_result(0.1)]], + mcp_servers=({"name": "linear", "type": "remote", "url": "https://mcp.linear"},), + ) + await h.harness.open() + await h.harness.create_session() + await _run(h.harness, HarnessPrompt(message_id="m1", text="hi", model="m")) + assert "hooks" not in h.client.options + + async def test_servers_sharing_tool_names_share_the_strictest_allowlist( + self, tmp_path: Path + ) -> None: + h = Harness( + tmp_path, + turns=[[_result(0.1)]], + mcp_servers=( + {"name": "my.docs", "type": "remote", "url": "https://mcp.docs"}, + { + "name": "my_docs", + "type": "remote", + "url": "https://mcp.docs2", + "toolAllowlist": ["search"], + }, + ), + ) + await h.harness.open() + await h.harness.create_session() + await _run(h.harness, HarnessPrompt(message_id="m1", text="hi", model="m")) + allowed = h.client.options["allowed_tools"] + assert "mcp__my_docs__*" not in allowed + assert allowed.count("mcp__my_docs__search") == 2 + async def test_stdout_ceiling_clears_the_whole_attachment_budget(self, tmp_path: Path) -> None: """One NDJSON line carries every attachment the runtime accepts. diff --git a/packages/sandbox-runtime/tests/test_mcp.py b/packages/sandbox-runtime/tests/test_mcp.py index dba31608df..755a8dc683 100644 --- a/packages/sandbox-runtime/tests/test_mcp.py +++ b/packages/sandbox-runtime/tests/test_mcp.py @@ -249,3 +249,72 @@ def test_accepts_valid_packages(self, regex, pkg): ) def test_rejects_dangerous_inputs(self, regex, pkg): assert not regex.match(pkg), f"Expected {pkg} to NOT match" + + +# ─── Tool allowlists in the OpenCode config ────────────────────────────────── + + +async def _start_and_capture_config(sup, installed_paths=frozenset()): + with ( + patch.object(sup, "_setup_managed_oauth"), + patch.object(sup, "_install_mcp_packages", new_callable=AsyncMock), + patch.object(sup, "_prepare_opencode_filesystem", return_value=set(installed_paths)), + patch.object(sup, "_wait_for_health", new_callable=AsyncMock), + patch( + "sandbox_runtime.opencode_server.asyncio.create_subprocess_exec", + new_callable=AsyncMock, + return_value=MagicMock(stdout=None), + ) as spawn, + patch( + "sandbox_runtime.opencode_server.asyncio.create_task", + side_effect=lambda coro: coro.close(), + ), + ): + await sup.start((), sup.workspace_path) + return json.loads(spawn.call_args.kwargs["env"]["OPENCODE_CONFIG_CONTENT"]) + + +class TestToolAllowlistConfig: + @pytest.mark.asyncio + async def test_unrestricted_servers_keep_the_blanket_permission(self, tmp_path): + sup = _make_supervisor( + {"mcp_servers": [{"name": "docs", "type": "remote", "url": "https://mcp.docs"}]} + ) + sup.workspace_path = tmp_path + config = await _start_and_capture_config(sup) + assert config["permission"] == {"*": {"*": "allow"}} + + @pytest.mark.asyncio + async def test_restricted_server_rules_follow_the_global_allow(self, tmp_path): + sup = _make_supervisor( + { + "mcp_servers": [ + { + "name": "memory", + "type": "remote", + "url": "https://mcp.memory", + "toolAllowlist": ["recall"], + } + ] + } + ) + sup.workspace_path = tmp_path / "repo" + # Open-Inspect's tools and the repository's own share the project tool directory. + project_tools = sup.workspace_path / ".opencode" / "tool" + project_tools.mkdir(parents=True) + for name in ("memory_read.js", "spawn-child.js", "memory_local.ts", "memory_notes.md"): + (project_tools / name).write_text("") + global_tools = tmp_path / "global" / "tools" + global_tools.mkdir(parents=True) + (global_tools / "memory_global.js").write_text("") + with patch.dict(os.environ, {"OPENCODE_CONFIG_DIR": str(tmp_path / "global")}): + config = await _start_and_capture_config(sup) + assert list(config["permission"].items()) == [ + ("*", {"*": "allow"}), + ("memory_*", "deny"), + ("memory_recall", "allow"), + ("memory_global", "allow"), + ("memory_local", "allow"), + ("memory_read", "allow"), + ] + assert config["mcp"]["memory"]["url"] == "https://mcp.memory" diff --git a/packages/sandbox-runtime/tests/test_mcp_tool_access.py b/packages/sandbox-runtime/tests/test_mcp_tool_access.py new file mode 100644 index 0000000000..7b5e7e6d41 --- /dev/null +++ b/packages/sandbox-runtime/tests/test_mcp_tool_access.py @@ -0,0 +1,203 @@ +"""Tests for per-server MCP tool allowlists in both harnesses.""" + +import pytest + +from sandbox_runtime.mcp_tool_access import ( + OPENCODE_BUILTIN_TOOL_IDS, + claude_tool_guard, + claude_tool_rules, + mcp_name_segment, + namespace_allowlists, + opencode_permission_rules, + opencode_shadowed_namespaces, + tool_allowlist, +) + + +def server(name, allowlist=None, **extra): + config = {"name": name, "type": "remote", "url": "https://mcp.example.com", **extra} + if allowlist is not None: + config["toolAllowlist"] = allowlist + return config + + +class TestToolAllowlist: + def test_absent_or_null_allows_every_tool(self): + assert tool_allowlist(server("docs")) is None + assert tool_allowlist({**server("docs"), "toolAllowlist": None}) is None + + def test_keeps_named_tools(self): + assert tool_allowlist(server("docs", ["search", "fetch"])) == ("search", "fetch") + + @pytest.mark.parametrize("raw", ["search", {"tools": ["search"]}, 1]) + def test_malformed_value_allows_no_tools(self, raw): + assert tool_allowlist({**server("docs"), "toolAllowlist": raw}) == () + + def test_drops_non_string_and_blank_entries(self): + assert tool_allowlist(server("docs", ["search", "", 3, None])) == ("search",) + + +class TestMcpNameSegment: + @pytest.mark.parametrize( + ("raw", "expected"), + [ + ("linear", "linear"), + ("my.server", "my_server"), + ("find/issues", "find_issues"), + ("café", "caf_"), + # One underscore per UTF-16 code unit, as JavaScript's regex replaces them. + ("my\U0001f600docs", "my__docs"), + ], + ) + def test_matches_harness_tool_name_sanitization(self, raw, expected): + assert mcp_name_segment(raw) == expected + + +class TestNamespaceAllowlists: + def test_keys_allowlists_by_sanitized_server_name(self): + assert namespace_allowlists([server("my.docs", ["search"]), server("linear")]) == { + "my_docs": ("search",), + "linear": None, + } + + def test_colliding_names_keep_only_tools_every_restricted_server_allows(self): + # my.docs and my_docs produce the same tool names, so neither may widen the other. + servers = [server("my.docs", ["search", "fetch"]), server("my_docs", ["fetch"])] + assert namespace_allowlists(servers) == {"my_docs": ("fetch",)} + + def test_a_restricted_server_restricts_a_colliding_unrestricted_one(self): + servers = [server("my.docs"), server("my_docs", ["search"])] + assert namespace_allowlists(servers) == {"my_docs": ("search",)} + assert namespace_allowlists(list(reversed(servers))) == {"my_docs": ("search",)} + + def test_skips_servers_without_a_name(self): + assert namespace_allowlists([{"toolAllowlist": []}]) == {} + + +class TestOpencodePermissionRules: + def test_no_rules_when_nothing_is_restricted(self): + assert opencode_permission_rules([server("docs"), server("linear")], ["memory_read"]) == {} + + def test_denies_the_server_namespace_then_allows_listed_tools(self): + rules = opencode_permission_rules([server("docs", ["search", "fetch"])], []) + assert list(rules.items()) == [ + ("docs_*", "deny"), + ("docs_search", "allow"), + ("docs_fetch", "allow"), + ] + + def test_empty_allowlist_denies_the_whole_server(self): + assert opencode_permission_rules([server("docs", [])], []) == {"docs_*": "deny"} + + def test_sanitizes_server_and_tool_names(self): + rules = opencode_permission_rules([server("my.server", ["find/issues"])], []) + assert list(rules) == ["my_server_*", "my_server_find_issues"] + + def test_unrestricted_namespace_extending_a_restricted_one_gets_no_tools(self): + # OpenCode names tools _, so foo_bar's "foo_bar_*" would also + # allow foo's own bar_* tools; it is left under foo's deny instead. + servers = [server("foo_bar"), server("foo", ["query"])] + rules = opencode_permission_rules(servers, []) + assert list(rules.items()) == [("foo_*", "deny"), ("foo_query", "allow")] + assert opencode_shadowed_namespaces(namespace_allowlists(servers)) == ["foo_bar"] + + def test_restricted_longer_namespace_keeps_its_rules_after_a_shorter_restricted_one(self): + rules = opencode_permission_rules([server("foo_bar", ["x"]), server("foo", ["query"])], []) + assert list(rules.items()) == [ + ("foo_*", "deny"), + ("foo_query", "allow"), + ("foo_bar_*", "deny"), + ("foo_bar_x", "allow"), + ] + + def test_namespaces_that_only_share_a_prefix_are_not_shadowed(self): + servers = [server("foobar"), server("foo", ["query"])] + assert opencode_shadowed_namespaces(namespace_allowlists(servers)) == [] + assert opencode_permission_rules(servers, [])["foobar_*"] == "allow" + + def test_restricted_longer_namespace_overrides_unrestricted_shorter_one(self): + rules = opencode_permission_rules([server("foo"), server("foo_bar", ["x"])], []) + assert list(rules.items()) == [ + ("foo_*", "allow"), + ("foo_bar_*", "deny"), + ("foo_bar_x", "allow"), + ] + + def test_reallows_other_tools_a_deny_pattern_would_hide(self): + rules = opencode_permission_rules( + [server("memory", ["recall"]), server("apply", [])], + ["memory_read", "memory_search", "apply_patch", "bash", "spawn-child"], + ) + assert list(rules.items()) == [ + ("apply_*", "deny"), + ("memory_*", "deny"), + ("memory_recall", "allow"), + ("memory_read", "allow"), + ("memory_search", "allow"), + ("apply_patch", "allow"), + ] + + def test_skips_servers_without_a_name(self): + assert opencode_permission_rules([{"toolAllowlist": []}], []) == {} + + def test_builtin_tool_ids_are_unique_and_sorted(self): + assert list(OPENCODE_BUILTIN_TOOL_IDS) == sorted(set(OPENCODE_BUILTIN_TOOL_IDS)) + + +class TestClaudeToolRules: + def test_unrestricted_server_allows_every_tool(self): + assert claude_tool_rules("linear", None) == (["mcp__linear__*"], []) + + def test_restricted_server_allows_only_listed_tools(self): + assert claude_tool_rules("linear", ("list_issues", "get/issue")) == ( + ["mcp__linear__list_issues", "mcp__linear__get_issue"], + [], + ) + + def test_empty_allowlist_denies_the_whole_server(self): + assert claude_tool_rules("linear", ()) == ([], ["mcp__linear"]) + + def test_sanitizes_the_server_name(self): + assert claude_tool_rules("my server", None) == (["mcp__my_server__*"], []) + + +class TestClaudeToolGuard: + @staticmethod + async def decision(guard, tool_name): + result = await guard({"tool_name": tool_name}, None, None) + return result.get("hookSpecificOutput", {}).get("permissionDecision", "pass") + + def test_no_guard_when_nothing_is_restricted(self): + assert claude_tool_guard({"linear": None}) is None + + @pytest.mark.parametrize( + ("tool_name", "expected"), + [ + ("mcp__linear__get_issue", "pass"), + ("mcp__linear__delete_issue", "deny"), + ("mcp__docs__anything", "pass"), + ("mcp__noisy__shout", "deny"), + ("Bash", "pass"), + ], + ) + async def test_refuses_only_restricted_servers_unselected_tools(self, tool_name, expected): + guard = claude_tool_guard({"linear": ("get/issue",), "docs": None, "noisy": ()}) + assert guard is not None + assert await self.decision(guard, tool_name) == expected + + @pytest.mark.parametrize( + ("tool_name", "expected"), + [ + # Claude Code splits names on "__", so docs' approval covers these too. + ("mcp__docs__extra__search", "pass"), + ("mcp__docs__extra__delete", "deny"), + ("mcp__docs__fetch_page", "pass"), + ], + ) + async def test_refuses_unselected_tools_an_overlapping_server_would_approve( + self, tool_name, expected + ): + allowlists = namespace_allowlists([server("docs"), server("docs__extra", ["search"])]) + guard = claude_tool_guard(allowlists) + assert guard is not None + assert await self.decision(guard, tool_name) == expected diff --git a/packages/sandbox-runtime/tests/test_opencode_reasoning_contract.py b/packages/sandbox-runtime/tests/test_opencode_reasoning_contract.py index 8650a407d8..cf0d825f5d 100644 --- a/packages/sandbox-runtime/tests/test_opencode_reasoning_contract.py +++ b/packages/sandbox-runtime/tests/test_opencode_reasoning_contract.py @@ -23,6 +23,7 @@ import shutil import socket import subprocess +import sys import threading import time import urllib.request @@ -31,6 +32,7 @@ import pytest +from sandbox_runtime.mcp_tool_access import OPENCODE_BUILTIN_TOOL_IDS, opencode_permission_rules from tests.test_prompt_stream import make_stream pytest_plugins = ["tests.test_reasoning_config"] @@ -38,6 +40,7 @@ BINARY = os.environ.get("OPENCODE_TEST_BINARY") pytestmark = pytest.mark.skipif(not BINARY, reason="set OPENCODE_TEST_BINARY for wire tests") CATALOG = Path(__file__).parent / "fixtures/reasoning-models.json" +MCP_FIXTURE_SERVER = Path(__file__).parent / "fixtures/mcp_stdio_server.py" def openai_events(model): @@ -167,6 +170,39 @@ def do_POST(self): "_bridge-client.js", ): shutil.copy(source / name, destination / name) + if getattr(request, "param", None) == "mcp": + # "docs" allows one of its tools. "docs_extra" extends its name, so + # OpenCode cannot tell its tools from docs' own extra_* tools and it + # gets none. "my.\U0001f600notes" checks name sanitization, including a + # character JavaScript sees as two UTF-16 code units. + fixture_tools = { + "docs": ["search", "fetch_page", "extra_delete_everything"], + "docs_extra": ["ping"], + "my.\U0001f600notes": ["read/page", "write"], + } + config["mcp"] = { + name: { + "type": "local", + "command": [sys.executable, str(MCP_FIXTURE_SERVER), *tools], + } + for name, tools in fixture_tools.items() + } + servers = [ + {"name": "docs", "toolAllowlist": ["search"]}, + {"name": "docs_extra"}, + {"name": "my.\U0001f600notes", "toolAllowlist": ["read/page"]}, + ] + # A repository's own tool whose name starts like a restricted server's. + custom_tools = tmp_path / ".opencode/tool" + custom_tools.mkdir(parents=True, exist_ok=True) + (custom_tools / "docs_local.js").write_text( + 'import { tool } from "@opencode-ai/plugin";\n' + 'export default tool({ description: "Local docs", args: {}, ' + 'async execute() { return "ok"; } });\n' + ) + config["permission"].update( + opencode_permission_rules(servers, (*OPENCODE_BUILTIN_TOOL_IDS, "docs_local")) + ) config["agent"] = {"build": {"options": {"reasoningEffort": "high"}}} for provider in ("openai", "anthropic"): config["provider"].setdefault(provider, {})["options"] = { @@ -265,6 +301,25 @@ async def test_memory_text_and_tool_contract_reach_real_opencode_provider_reques assert "environmentId" not in tools["memory_write"]["input_schema"]["properties"] +@pytest.mark.parametrize("wire_server", ["mcp"], indirect=True) +async def test_mcp_tool_allowlist_reaches_real_opencode_provider_request(wire_server): + call, captured = wire_server + builtin_ids = set(call("/experimental/tool/ids")) - {"invalid", "docs_local"} + assert builtin_ids == set(OPENCODE_BUILTIN_TOOL_IDS) + + sent, _ = submit(call, captured, "anthropic/claude-sonnet-4-6", None) + names = {tool["name"] for tool in sent["tools"]} + assert {"docs_search", "my___notes_read_page", "docs_local"} <= names + hidden = { + "docs_fetch_page", + "docs_extra_delete_everything", + "docs_extra_ping", + "my___notes_write", + } + assert not hidden & names + assert {"bash", "read", "edit", "write", "glob", "grep"} <= names + + async def test_all_fixture_efforts_reach_provider(wire_server): call, captured = wire_server catalog = json.loads(CATALOG.read_text()) diff --git a/packages/shared/src/types/integrations.test.ts b/packages/shared/src/types/integrations.test.ts index 329198f391..fe6acfac97 100644 --- a/packages/shared/src/types/integrations.test.ts +++ b/packages/shared/src/types/integrations.test.ts @@ -12,9 +12,13 @@ import { isValidSandboxTimeoutMs, findSandboxPortConflict, matchRoutingRules, + MAX_MCP_SERVER_TOOLS, + createMcpServerInputSchema, mcpServerCommandSchema, mcpServerCredentialMapSchema, mcpServerTypeSchema, + mcpToolAllowlistSchema, + updateMcpServerInputSchema, normalizeRoutingRules, omitUnsupportedSandboxSettings, resolveBuildTimeoutSeconds, @@ -382,6 +386,36 @@ describe("MCP server schemas", () => { expect(mcpServerCommandSchema.safeParse(["npx", 1]).success).toBe(false); expect(mcpServerCredentialMapSchema.safeParse({ DEBUG: 1 }).success).toBe(false); }); + + it("normalizes a tool allowlist to sorted, unique, trimmed names", () => { + expect(mcpToolAllowlistSchema.parse([" search ", "fetch", "search"])).toEqual([ + "fetch", + "search", + ]); + expect(mcpToolAllowlistSchema.parse([])).toEqual([]); + }); + + it("rejects blank tool names and allowlists over the size limit", () => { + expect(mcpToolAllowlistSchema.safeParse([" "]).success).toBe(false); + expect(mcpToolAllowlistSchema.safeParse([1]).success).toBe(false); + const tooMany = Array.from({ length: MAX_MCP_SERVER_TOOLS + 1 }, (_, i) => `tool_${i}`); + expect(mcpToolAllowlistSchema.safeParse(tooMany).success).toBe(false); + }); + + it("keeps an empty allowlist distinct from no allowlist on create and update", () => { + const remote = { name: "docs", type: "remote", url: "https://mcp.example.com/mcp" }; + expect(createMcpServerInputSchema.parse(remote).toolAllowlist).toBeUndefined(); + expect(createMcpServerInputSchema.parse({ ...remote, toolAllowlist: null }).toolAllowlist).toBe( + null + ); + expect( + createMcpServerInputSchema.parse({ ...remote, toolAllowlist: [] }).toolAllowlist + ).toEqual([]); + expect(updateMcpServerInputSchema.parse({ revision: 2, toolAllowlist: ["b", "a"] })).toEqual({ + revision: 2, + toolAllowlist: ["a", "b"], + }); + }); }); describe("normalizeRoutingRules", () => { diff --git a/packages/shared/src/types/integrations.ts b/packages/shared/src/types/integrations.ts index 5eaf20451b..211a6b04ae 100644 --- a/packages/shared/src/types/integrations.ts +++ b/packages/shared/src/types/integrations.ts @@ -701,10 +701,19 @@ export interface McpServerConfig { env?: Record; headers?: Record; repoScopes?: string[] | null; + /** Server tool names the agent may use. Null or absent exposes every tool. */ + toolAllowlist?: string[] | null; enabled: boolean; } export const DEFAULT_MCP_SERVER_ENABLED = true; +/** Most tools an allowlist may name; discovery fails for a server that advertises more. */ +export const MAX_MCP_SERVER_TOOLS = 1000; +/** Allowlisted tool names: trimmed, non-empty, deduplicated and sorted. */ +export const mcpToolAllowlistSchema = z + .array(z.string().trim().min(1).max(256)) + .max(MAX_MCP_SERVER_TOOLS) + .transform((tools) => [...new Set(tools)].sort()); export const mcpServerTypeSchema = z.enum(["local", "remote"]); export const mcpServerCommandSchema = z.array(z.string()).min(1); export const mcpServerCredentialMapSchema = z.record(z.string(), z.string()); @@ -712,6 +721,7 @@ export const mcpServerCredentialMapSchema = z.record(z.string(), z.string()); const mcpServerCommonFields = { name: z.string().trim().min(1), repoScopes: z.array(z.string()).nullable().optional(), + toolAllowlist: mcpToolAllowlistSchema.nullable().optional(), enabled: z.boolean().optional(), }; @@ -771,9 +781,20 @@ export interface McpServerMetadata { hasEnv: boolean; hasHeaders: boolean; repoScopes?: string[] | null; + toolAllowlist?: string[] | null; enabled: boolean; } +/** One tool a remote MCP server advertises, as returned by tool discovery. */ +export interface McpToolMetadata { + name: string; + description?: string; +} + +export interface DiscoverMcpToolsResponse { + tools: McpToolMetadata[]; +} + export const INTEGRATION_DEFINITIONS: { id: IntegrationId; name: string; diff --git a/packages/web/src/app/api/mcp-servers/[id]/tools/route.ts b/packages/web/src/app/api/mcp-servers/[id]/tools/route.ts new file mode 100644 index 0000000000..8adbe4c0fd --- /dev/null +++ b/packages/web/src/app/api/mcp-servers/[id]/tools/route.ts @@ -0,0 +1,23 @@ +import type { NextRequest } from "next/server"; +import { NextResponse } from "next/server"; +import { getServerAuthSession } from "@/lib/server-auth-session"; +import { controlPlaneUserFetch } from "@/lib/control-plane"; + +export async function POST(_request: NextRequest, { params }: { params: Promise<{ id: string }> }) { + const session = await getServerAuthSession(); + if (!session?.user) { + return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); + } + + const { id } = await params; + try { + const response = await controlPlaneUserFetch(`/mcp-servers/${encodeURIComponent(id)}/tools`, { + method: "POST", + }); + const data = await response.json(); + return NextResponse.json(data, { status: response.status }); + } catch (error) { + console.error("Failed to load MCP server tools:", error); + return NextResponse.json({ error: "Failed to load MCP server tools" }, { status: 500 }); + } +} diff --git a/packages/web/src/components/settings/mcp-servers-settings.test.tsx b/packages/web/src/components/settings/mcp-servers-settings.test.tsx index 0df7e49f77..59d893722d 100644 --- a/packages/web/src/components/settings/mcp-servers-settings.test.tsx +++ b/packages/web/src/components/settings/mcp-servers-settings.test.tsx @@ -2,7 +2,7 @@ /// import { afterEach, describe, expect, it, vi } from "vitest"; -import { cleanup, render, screen, waitFor } from "@testing-library/react"; +import { cleanup, render, screen, waitFor, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import * as matchers from "@testing-library/jest-dom/matchers"; import type { McpServerMetadata } from "@open-inspect/shared/types/integrations"; @@ -12,7 +12,9 @@ expect.extend(matchers); const mocks = vi.hoisted(() => ({ mutate: vi.fn(), + createMcpServer: vi.fn(), updateMcpServer: vi.fn(), + discoverMcpTools: vi.fn(), allowedPermissions: null as Set | null, })); @@ -22,9 +24,10 @@ vi.mock("@/hooks/use-repos", () => ({ })); vi.mock("@/hooks/use-mcp-servers", () => ({ useMcpServers: () => ({ servers, loading: false, mutate: mocks.mutate }), - createMcpServer: vi.fn(), + createMcpServer: mocks.createMcpServer, updateMcpServer: mocks.updateMcpServer, deleteMcpServer: vi.fn(), + discoverMcpTools: mocks.discoverMcpTools, })); vi.mock("@/hooks/use-current-user-authorization", () => ({ useCurrentUserAuthorization: () => ({ @@ -54,10 +57,28 @@ const servers: McpServerMetadata[] = [ hasEnv: false, hasHeaders: false, repoScopes: null, + toolAllowlist: ["search", "retired_tool"], + enabled: true, + }, + { + id: "server-c", + revision: 1, + name: "Server C", + type: "local", + command: ["npx", "server-c"], + hasEnv: false, + hasHeaders: false, + repoScopes: null, enabled: true, }, ]; +const catalog = [ + { name: "fetch_page", description: "Fetch a documentation page" }, + { name: "search", description: "Search the documentation" }, + { name: "delete_index" }, +]; + afterEach(() => { cleanup(); vi.clearAllMocks(); @@ -121,9 +142,211 @@ describe("McpServersSettings", () => { name: "Server A", enabled: true, repoScopes: null, + toolAllowlist: null, type: "local", command: ["npx", "tool"], revision: 3, }); }); + + describe("tool access", () => { + it("summarizes restricted servers in the list", () => { + render(); + + expect(screen.getByRole("button", { name: /Server B/ })).toHaveTextContent("2 tools"); + expect(screen.getByRole("button", { name: /Server A/ })).not.toHaveTextContent("tools"); + }); + + it("loads a saved remote server's tools and saves the selected ones", async () => { + mocks.discoverMcpTools.mockResolvedValue(catalog); + mocks.updateMcpServer.mockResolvedValue({ ...servers[0], revision: 4 }); + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /Server A/ })); + await user.click(screen.getByRole("radio", { name: /Selected tools only/ })); + expect(screen.getByText(/No tools selected/)).toBeInTheDocument(); + await user.click(screen.getByRole("button", { name: "Load tools" })); + + expect(mocks.discoverMcpTools).toHaveBeenCalledWith("server-a"); + await user.click(await screen.findByRole("checkbox", { name: /^search/ })); + expect(screen.getByText("1 of 3 selected")).toBeInTheDocument(); + await user.click(screen.getByRole("button", { name: "Save Changes" })); + + await waitFor(() => + expect(mocks.updateMcpServer).toHaveBeenCalledWith( + "server-a", + expect.objectContaining({ toolAllowlist: ["search"], revision: 3 }) + ) + ); + }); + + it("keeps a stored selection and lets tools the server no longer offers be removed", async () => { + mocks.discoverMcpTools.mockResolvedValue(catalog); + mocks.updateMcpServer.mockResolvedValue({ ...servers[1], revision: 8 }); + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /Server B/ })); + expect(screen.getByRole("radio", { name: /Selected tools only/ })).toBeChecked(); + await user.click(screen.getByRole("button", { name: "Load tools" })); + + expect(await screen.findByRole("checkbox", { name: /^search/ })).toBeChecked(); + expect(screen.getByText(/not offered by the server/)).toBeInTheDocument(); + await user.click(screen.getByRole("button", { name: "Remove retired_tool" })); + await user.click(screen.getByRole("button", { name: "Save Changes" })); + + await waitFor(() => + expect(mocks.updateMcpServer).toHaveBeenCalledWith( + "server-b", + expect.objectContaining({ toolAllowlist: ["search"] }) + ) + ); + }); + + it("selects and clears only the tools matching the search", async () => { + mocks.discoverMcpTools.mockResolvedValue(catalog); + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /Server A/ })); + await user.click(screen.getByRole("radio", { name: /Selected tools only/ })); + await user.click(screen.getByRole("button", { name: "Load tools" })); + await user.type(await screen.findByRole("textbox", { name: "Search tools" }), "page"); + + expect(screen.queryByRole("checkbox", { name: /^search/ })).not.toBeInTheDocument(); + await user.click(screen.getByRole("button", { name: "Select all" })); + await user.clear(screen.getByRole("textbox", { name: "Search tools" })); + + expect(screen.getByRole("checkbox", { name: /^fetch_page/ })).toBeChecked(); + expect(screen.getByRole("checkbox", { name: /^search/ })).not.toBeChecked(); + }); + + it("shows why loading failed", async () => { + mocks.discoverMcpTools.mockRejectedValue( + new Error("Could not load tools from the MCP server") + ); + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /Server A/ })); + await user.click(screen.getByRole("radio", { name: /Selected tools only/ })); + await user.click(screen.getByRole("button", { name: "Load tools" })); + + expect(await screen.findByRole("alert")).toHaveTextContent( + "Could not load tools from the MCP server" + ); + }); + + it("loads tools only with the saved connection details", async () => { + mocks.discoverMcpTools.mockResolvedValue([{ name: "search" }]); + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /Server A/ })); + await user.click(screen.getByRole("radio", { name: /Selected tools only/ })); + await user.click(screen.getByRole("button", { name: "Load tools" })); + expect(await screen.findByRole("checkbox", { name: /search/ })).toBeInTheDocument(); + await user.type(screen.getByDisplayValue("https://a.example.com"), "/v2"); + + expect( + screen.queryByRole("button", { name: /Load tools|Reload tools/ }) + ).not.toBeInTheDocument(); + expect(screen.queryByRole("checkbox", { name: /search/ })).not.toBeInTheDocument(); + expect(screen.getByText(/Save the connection changes/)).toBeInTheDocument(); + }); + + it("waits for typed headers to be saved before loading tools", async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /Server A/ })); + await user.click(screen.getByRole("radio", { name: /Selected tools only/ })); + expect(screen.getByRole("button", { name: "Load tools" })).toBeInTheDocument(); + await user.type(screen.getByPlaceholderText("Header-Name"), "Authorization"); + await user.type(screen.getByPlaceholderText("value"), "Bearer new"); + + expect(screen.queryByRole("button", { name: "Load tools" })).not.toBeInTheDocument(); + expect(screen.getByText(/Save the connection changes/)).toBeInTheDocument(); + }); + + it("sends the selection when creating a server", async () => { + mocks.createMcpServer.mockResolvedValue({ ...servers[2], id: "server-d", revision: 1 }); + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: "Add Server" })); + await user.type(screen.getByPlaceholderText("e.g. playwright, context7"), "playwright"); + await user.type(screen.getByPlaceholderText("npx -y @playwright/mcp"), "npx server-d"); + await user.click(screen.getByRole("radio", { name: /Selected tools only/ })); + await user.type(screen.getByRole("textbox", { name: "Tool name" }), "navigate{Enter}"); + // The header button opens the form; the second "Add Server" submits it. + const [, submit] = screen.getAllByRole("button", { name: "Add Server" }); + await user.click(submit); + + await waitFor(() => + expect(mocks.createMcpServer).toHaveBeenCalledWith( + expect.objectContaining({ name: "playwright", toolAllowlist: ["navigate"] }) + ) + ); + }); + + it("asks to save a new remote server before loading its tools", async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: "Add Server" })); + await user.click(screen.getByRole("button", { name: "Remote" })); + await user.click(screen.getByRole("radio", { name: /Selected tools only/ })); + + expect(screen.queryByRole("button", { name: "Load tools" })).not.toBeInTheDocument(); + expect(screen.getByText("Save the server to load its tools.")).toBeInTheDocument(); + }); + + it("adds tool names by hand for local servers", async () => { + mocks.updateMcpServer.mockResolvedValue({ ...servers[2], revision: 2 }); + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /Server C/ })); + await user.click(screen.getByRole("radio", { name: /Selected tools only/ })); + expect(screen.queryByRole("button", { name: "Load tools" })).not.toBeInTheDocument(); + expect(screen.getByText(/Local servers start inside the sandbox/)).toBeInTheDocument(); + await user.type(screen.getByRole("textbox", { name: "Tool name" }), " navigate {Enter}"); + await user.type(screen.getByRole("textbox", { name: "Tool name" }), "click"); + await user.click(screen.getByRole("button", { name: "Add" })); + + const selection = screen.getByRole("list", { name: "Selected tools" }); + expect( + within(selection) + .getAllByRole("listitem") + .map((item) => item.textContent) + ).toEqual(["navigate", "click"]); + await user.click(screen.getByRole("button", { name: "Save Changes" })); + + await waitFor(() => + expect(mocks.updateMcpServer).toHaveBeenCalledWith( + "server-c", + expect.objectContaining({ toolAllowlist: ["click", "navigate"] }) + ) + ); + }); + + it("restores every tool when switched back to All tools", async () => { + mocks.updateMcpServer.mockResolvedValue({ ...servers[1], revision: 8 }); + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: /Server B/ })); + await user.click(screen.getByRole("radio", { name: /All tools/ })); + await user.click(screen.getByRole("button", { name: "Save Changes" })); + + await waitFor(() => + expect(mocks.updateMcpServer).toHaveBeenCalledWith( + "server-b", + expect.objectContaining({ toolAllowlist: null }) + ) + ); + }); + }); }); diff --git a/packages/web/src/components/settings/mcp-servers-settings.tsx b/packages/web/src/components/settings/mcp-servers-settings.tsx index 5940423695..5c9f2f7d6e 100644 --- a/packages/web/src/components/settings/mcp-servers-settings.tsx +++ b/packages/web/src/components/settings/mcp-servers-settings.tsx @@ -33,6 +33,12 @@ import { AlertDialogTitle, } from "@/components/ui/alert-dialog"; import { useCurrentUserAuthorization } from "@/hooks/use-current-user-authorization"; +import { + McpToolAccess, + toolAccessFromAllowlist, + toolAccessToAllowlist, + type ToolAccess, +} from "./mcp-tool-access"; type ScopeMode = "global" | "selected"; type Editor = @@ -62,6 +68,7 @@ type FormState = { envRows: EnvRow[]; repoScopes: string[]; scopeMode: ScopeMode; + toolAccess: ToolAccess; enabled: boolean; }; @@ -73,6 +80,7 @@ const emptyForm: FormState = { envRows: [createEnvRow()], repoScopes: [], scopeMode: "global", + toolAccess: toolAccessFromAllowlist(null), enabled: DEFAULT_MCP_SERVER_ENABLED, }; @@ -90,6 +98,7 @@ function metadataToForm(metadata: McpServerMetadata): FormState { envRows: [createEnvRow()], repoScopes: metadata.repoScopes ?? [], scopeMode: metadata.repoScopes?.length ? "selected" : "global", + toolAccess: toolAccessFromAllowlist(metadata.toolAllowlist), enabled: metadata.enabled, }; } @@ -265,6 +274,31 @@ interface McpServerFormProps { loadingRepos: boolean; radioPrefix: string; hasExistingCredentials?: boolean; + /** The saved server whose tools can be loaded, unless its connection was edited. */ + savedServer?: McpServerMetadata; +} + +function toolDiscovery(form: FormState, savedServer: McpServerMetadata | undefined) { + if (form.type === "local") { + return { + kind: "unavailable", + reason: "Local servers start inside the sandbox, so add their tool names below.", + } as const; + } + if (!savedServer) { + return { kind: "unavailable", reason: "Save the server to load its tools." } as const; + } + const connectionEdited = + savedServer.type !== "remote" || + form.url.trim() !== savedServer.url || + Object.keys(envRowsToRecord(form.envRows)).length > 0; + if (connectionEdited) { + return { + kind: "unavailable", + reason: "Save the connection changes to load tools with them.", + } as const; + } + return { kind: "available", serverId: savedServer.id, revision: savedServer.revision } as const; } function McpServerForm({ @@ -274,6 +308,7 @@ function McpServerForm({ loadingRepos, radioPrefix, hasExistingCredentials, + savedServer, }: McpServerFormProps) { const selectedRepoScopes = new Set(form.repoScopes); @@ -424,6 +459,13 @@ function McpServerForm({ )} + setForm({ ...form, toolAccess })} + radioPrefix={radioPrefix} + discovery={toolDiscovery(form, savedServer)} + /> +