diff --git a/CHANGELOG.md b/CHANGELOG.md index 496d937b48..0f5f69e611 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,18 @@ 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. A session resumed from a snapshot taken before this release starts +without servers set to **Selected tools only**, with a warning, since its runtime cannot enforce the +selection. 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..ae78f21687 100644 --- a/docs/HOW_IT_WORKS.md +++ b/docs/HOW_IT_WORKS.md @@ -404,6 +404,54 @@ 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`). + +Runtimes older than the manifest's `minimumMcpToolAllowlistGeneration` ignore the allowlist. A +sandbox restored from a snapshot runs the snapshot's runtime, and a prebuilt image records the +runtime it was built with, so the control plane withholds restricted servers from a launch on an +older runtime. It logs `mcp.restricted_servers_withheld` and, on restore, adds a warning to the +session. A launch from a provider's base image is assumed to run the control plane's own runtime +version. + +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. The rules must be in OpenCode's config before it loads those files, so the runtime + lists each file's exports with Bun's parser (`Bun.Transpiler.scan`, which reads a module without + running it) and names them as OpenCode does: `` for the default export and `_` + for each named one. A CommonJS file, or one the parser rejects, falls back to its file name. A + tool re-exported with `export * from` cannot be listed and stays hidden if a deny pattern matches + it. If the scan fails, the runtime falls back to file names and logs + `opencode.custom_tool_scan_failed`. 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. An unrestricted server + named `docs__extra` next to a restricted `docs` therefore gets no tools. +- 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, Open-Inspect or repository tool name (an + `apply` server's `patch` tool versus OpenCode's `apply_patch`, or a `docs` server's `search` tool + versus a `search` export in `.opencode/tool/docs.ts`, tool or not), or as another server's + selected tool. That name stays allowed, so the MCP tool does too. + ### 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..8762d0c9bf --- /dev/null +++ b/packages/control-plane/src/mcp/tool-discovery.test.ts @@ -0,0 +1,313 @@ +import type { FetchLike } from "@modelcontextprotocol/client"; +import { MAX_MCP_SERVER_TOOLS } from "@open-inspect/shared/types/integrations"; +import { describe, expect, it, vi } from "vitest"; +import { + McpToolDiscoveryError, + authenticatedFetch, + discoverRemoteMcpTools, + type RemoteMcpServer, +} from "./tool-discovery"; + +const remoteServer: RemoteMcpServer = { + url: "https://mcp.example.com/mcp", + headers: { Authorization: "Bearer secret", "X-Tenant": "acme" }, +}; + +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 the Streamable HTTP status when SSE fails without one", async () => { + const fetchMock = vi.fn(async (_input: RequestInfo | URL, init?: RequestInit) => { + if (init?.method === "POST") return new Response("internal detail", { status: 404 }); + throw new TypeError("fetch failed"); + }); + + 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 404"); + }); + + it("reports a timeout during the SSE fallback", async () => { + const fetchMock = vi.fn((_input: RequestInfo | URL, init?: RequestInit) => + init?.method === "POST" + ? Promise.resolve(new Response(null, { status: 405 })) + : 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.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("refuses a non-HTTP URL without connecting", async () => { + const fetchMock = vi.fn() as unknown as FetchLike; + await expect( + discoverRemoteMcpTools( + { ...remoteServer, url: "ftp://mcp.example.com" }, + { 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..25597bb4ed --- /dev/null +++ b/packages/control-plane/src/mcp/tool-discovery.ts @@ -0,0 +1,158 @@ +import { + Client, + SdkHttpError, + SseError, + SSEClientTransport, + StreamableHTTPClientTransport, + type FetchLike, + type Transport, +} from "@modelcontextprotocol/client"; +import { + MAX_MCP_SERVER_TOOLS, + 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. +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; +} + +/** A remote MCP server's connection settings. */ +export interface RemoteMcpServer { + url: string; + headers: Record; +} + +type ListToolsAttempt = + { tools: McpToolMetadata[] } | { error: unknown; status: number | undefined }; + +/** + * Lists the tools a remote MCP server advertises, connecting with the given + * 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: RemoteMcpServer, + { fetch: baseFetch = fetch, timeoutMs = DISCOVERY_TIMEOUT_MS }: DiscoveryOptions = {} +): Promise { + 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 request = authenticatedFetch(server.headers, timeout, baseFetch); + // Throws discovery errors and timeouts as McpToolDiscoveryError; returns any + // other failure, with its HTTP status, for the caller to judge. + const attempt = async (transport: Transport): Promise => { + try { + return { tools: await listTools(transport, timeout) }; + } catch (err) { + if (err instanceof McpToolDiscoveryError) throw err; + if (timeout.aborted) { + throw new McpToolDiscoveryError( + `The server did not respond within ${timeoutMs / 1000} seconds`, + { cause: err } + ); + } + return { error: err, status: httpStatus(err) }; + } + }; + + const streamable = await attempt(new StreamableHTTPClientTransport(url, { fetch: request })); + if ("tools" in streamable) return streamable.tools; + // Only a 4xx can mean a legacy SSE server; any other failure is the real one. + if (streamable.status === undefined) throw streamable.error; + if (streamable.status < 400 || streamable.status >= 500) { + throw statusError(streamable.status, streamable.error); + } + + const sse = await attempt(new SSEClientTransport(url, { fetch: request })); + if ("tools" in sse) return sse.tools; + // Those statuses only say the URL does not speak Streamable HTTP, so the SSE + // status explains more; any other 4xx, such as a 401, is the real answer. + const status = NOT_STREAMABLE_HTTP_STATUSES.has(streamable.status) + ? (sse.status ?? streamable.status) + : streamable.status; + throw statusError(status, new AggregateError([streamable.error, sse.error])); +} 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..a7c0101891 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 }); } +/** 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; +} + +/** Connects to a saved remote server with its stored headers and lists its tools. */ +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" || !server.url) { + return error("Only remote MCP servers can list their tools", 400); + } + + try { + const tools = await discoverRemoteMcpTools({ url: server.url, headers: server.headers ?? {} }); + 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/src/sandbox/lifecycle/launch-context.test.ts b/packages/control-plane/src/sandbox/lifecycle/launch-context.test.ts new file mode 100644 index 0000000000..6d4d5f4e2d --- /dev/null +++ b/packages/control-plane/src/sandbox/lifecycle/launch-context.test.ts @@ -0,0 +1,62 @@ +import { describe, expect, it, vi } from "vitest"; +import { MIN_MCP_TOOL_ALLOWLIST_RUNTIME_GENERATION } from "../runtime-manifest"; +import { SandboxLaunchContext } from "./launch-context"; + +describe("SandboxLaunchContext.loadMcpServers", () => { + const server = { + type: "remote" as const, + url: "https://mcp.example.com", + repoScopes: null, + enabled: true, + }; + + function launchContext(toolAllowlists: Array) { + return new SandboxLaunchContext({ + sessionContext: { getSessionRepositories: () => [], getUserEnvVars: async () => undefined }, + provider: { name: "test", capabilities: { supportsSandboxTimeout: true } }, + config: { + model: "anthropic/claude-haiku-5-5", + mcpServerLookup: { + getDecryptedForSession: async () => + toolAllowlists.map((toolAllowlist, i) => ({ + ...server, + id: `mcp-${i}`, + name: toolAllowlist ? `limited-${i}` : `open-${i}`, + toolAllowlist, + })), + }, + }, + getLogger: () => ({ info: vi.fn(), warn: vi.fn() }), + }); + } + + it.each([ + [ + "an older", + `v${MIN_MCP_TOOL_ALLOWLIST_RUNTIME_GENERATION - 1}-old`, + ["open-1"], + ["limited-0"], + ], + ["an unparseable", "unversioned", ["open-1"], ["limited-0"]], + [ + "a current", + `v${MIN_MCP_TOOL_ALLOWLIST_RUNTIME_GENERATION}-current`, + ["limited-0", "open-1"], + [], + ], + ])( + "withholds restricted servers only from %s runtime", + async (_case, version, sent, withheld) => { + const loaded = await launchContext([["search"], null]).loadMcpServers([], version); + + expect(loaded.servers?.map((s) => s.name)).toEqual(sent); + expect(loaded.withheld).toEqual(withheld); + } + ); + + it("sends no servers when every one is withheld", async () => { + const loaded = await launchContext([["search"]]).loadMcpServers([], "v1-old"); + + expect(loaded).toEqual({ servers: undefined, withheld: ["limited-0"] }); + }); +}); diff --git a/packages/control-plane/src/sandbox/lifecycle/launch-context.ts b/packages/control-plane/src/sandbox/lifecycle/launch-context.ts index 62fe7e6a24..ec037dd174 100644 --- a/packages/control-plane/src/sandbox/lifecycle/launch-context.ts +++ b/packages/control-plane/src/sandbox/lifecycle/launch-context.ts @@ -6,9 +6,14 @@ import { type McpServerConfig, type SandboxSettings, } from "@open-inspect/shared/types/integrations"; -import { repoImageBuildScope, type ImageBuildScope } from "../../image-builds/model"; +import { + parseRuntimeVersionNumber, + repoImageBuildScope, + type ImageBuildScope, +} from "../../image-builds/model"; import type { Logger } from "../../logger"; import { sessionHasRepository, type SessionRow } from "../../session/types"; +import { MIN_MCP_TOOL_ALLOWLIST_RUNTIME_GENERATION } from "../runtime-manifest"; import { SandboxProviderError, type CreateSandboxConfig, @@ -59,6 +64,12 @@ export interface SandboxLaunchContextDependencies { getLogger: () => Pick; } +export interface LoadedMcpServers { + servers: McpServerConfig[] | undefined; + /** Names of restricted servers left out because the runtime cannot enforce their allowlists. */ + withheld: string[]; +} + export type AgentLaunchFields = Pick; export interface RepositoryLaunchInputs { @@ -84,6 +95,11 @@ export function resolveImageBuildScope( : null; } +function supportsMcpToolAllowlists(runtimeVersion: string): boolean { + const generation = parseRuntimeVersionNumber(runtimeVersion); + return generation !== null && generation >= MIN_MCP_TOOL_ALLOWLIST_RUNTIME_GENERATION; +} + /** * Stateless launch-input mechanics, with no reservation, admission, or provider I/O authority. * The manager calls each operation at its existing await point: fresh and restore intentionally @@ -210,26 +226,47 @@ export class SandboxLaunchContext { } } + /** + * `runtimeVersion` is the runtime that will read the servers: the selected + * prebuilt image's (the current runtime's when none is selected), or the + * snapshot's on restore. Restricted servers are withheld from a runtime that + * predates tool allowlists or whose version cannot be parsed, since it would + * give them every tool. + */ async loadMcpServers( - repositories: SessionRepositoryInfo[] - ): Promise { + repositories: SessionRepositoryInfo[], + runtimeVersion: string + ): Promise { try { - if (!this.config.mcpServerLookup) return undefined; - const servers = await this.config.mcpServerLookup.getDecryptedForSession( + if (!this.config.mcpServerLookup) return { servers: undefined, withheld: [] }; + const loaded = await this.config.mcpServerLookup.getDecryptedForSession( repositories.map(({ repoOwner, repoName }) => ({ repoOwner, repoName })) ); + const withheld = supportsMcpToolAllowlists(runtimeVersion) + ? [] + : loaded.filter((server) => server.toolAllowlist != null).map((server) => server.name); + const servers = withheld.length + ? loaded.filter((server) => server.toolAllowlist == null) + : loaded; + if (withheld.length) { + this.getLogger().warn("MCP servers with a tool allowlist withheld from an older runtime", { + event: "mcp.restricted_servers_withheld", + runtime_version: runtimeVersion, + names: withheld, + }); + } this.getLogger().info("MCP servers loaded", { event: "mcp.loaded", - count: servers?.length ?? 0, - names: servers?.map((s) => s.name) ?? [], + count: servers.length, + names: servers.map((s) => s.name), }); - return servers?.length ? servers : undefined; + return { servers: servers.length ? servers : undefined, withheld }; } catch (err) { this.getLogger().warn("Failed to load MCP servers", { event: "mcp.load_failed", error: String(err), }); - return undefined; + return { servers: undefined, withheld: [] }; } } diff --git a/packages/control-plane/src/sandbox/lifecycle/manager.test.ts b/packages/control-plane/src/sandbox/lifecycle/manager.test.ts index 7ee1204f98..28f3b908aa 100644 --- a/packages/control-plane/src/sandbox/lifecycle/manager.test.ts +++ b/packages/control-plane/src/sandbox/lifecycle/manager.test.ts @@ -8,6 +8,7 @@ import { afterEach, describe, it, expect, vi } from "vitest"; import { DEFAULT_LIFECYCLE_CONFIG, type SandboxShutdownLifecycle } from "./manager"; import { TERMINAL_TOKEN_TTL_SECONDS } from "./sandbox-access"; import { COMPATIBLE_RUNTIME_VERSION } from "../../image-builds/test-helpers"; +import { MIN_MCP_TOOL_ALLOWLIST_RUNTIME_GENERATION } from "../runtime-manifest"; import { SandboxProviderError, SandboxLaunchRejectedError, @@ -142,6 +143,90 @@ describe("SandboxLifecycleManager", () => { clock.mockRestore(); } }); + + describe("MCP servers limited to selected tools", () => { + const server = { + type: "remote" as const, + url: "https://mcp.example.com", + repoScopes: null, + enabled: true, + }; + const mcpServerLookup = { + getDecryptedForSession: vi.fn(async () => [ + { ...server, id: "mcp-1", name: "limited", toolAllowlist: ["search"] }, + { ...server, id: "mcp-2", name: "open", toolAllowlist: null }, + ]), + }; + + function launch(sandbox: ReturnType) { + const storage = createMockStorage(createMockSession(), sandbox); + const provider = createMockProvider({ + restoreFromSnapshot: vi.fn(async (config) => ({ + success: true as const, + sandboxId: config.sandboxId, + providerObjectId: "restored-source", + lifetime: noLifetime(), + })), + }); + const recordWarning = vi.fn(); + const manager = createTestLifecycleManager( + provider, + storage, + storage, + createMockBroadcaster(), + createMockWebSocketManager(false), + createMockAlarmScheduler(), + createMockIdGenerator(), + createUnmanagedShutdown(), + { ...createTestConfig(), mcpServerLookup, recordWarning } + ); + return { manager, provider, recordWarning }; + } + + it("withholds them from a restore on an older runtime and tells the user", async () => { + const { manager, provider, recordWarning } = launch( + createMockSandbox({ + status: "stopped", + snapshot_image_id: "saved-image", + snapshot_runtime_version: `v${MIN_MCP_TOOL_ALLOWLIST_RUNTIME_GENERATION - 1}-old`, + }) + ); + + await manager.spawnSandbox(); + + const restore = vi.mocked(provider.restoreFromSnapshot!).mock.calls[0][0]; + expect(restore.mcpServers?.map((s) => s.name)).toEqual(["open"]); + expect(recordWarning).toHaveBeenCalledExactlyOnceWith( + expect.stringContaining("(limited)"), + "mcp-servers-withheld:saved-image" + ); + }); + + it("keeps them for a restore on a runtime that enforces them", async () => { + const { manager, provider, recordWarning } = launch( + createMockSandbox({ + status: "stopped", + snapshot_image_id: "saved-image", + snapshot_runtime_version: `v${MIN_MCP_TOOL_ALLOWLIST_RUNTIME_GENERATION}-current`, + }) + ); + + await manager.spawnSandbox(); + + const restore = vi.mocked(provider.restoreFromSnapshot!).mock.calls[0][0]; + expect(restore.mcpServers?.map((s) => s.name)).toEqual(["limited", "open"]); + expect(recordWarning).not.toHaveBeenCalled(); + }); + + it("keeps them for a fresh sandbox on the current runtime", async () => { + const { manager, provider } = launch(createMockSandbox({ status: "pending" })); + + await manager.spawnSandbox(); + + const create = vi.mocked(provider.createSandbox).mock.calls[0][0]; + expect(create.mcpServers?.map((s) => s.name)).toEqual(["limited", "open"]); + }); + }); }); }); diff --git a/packages/control-plane/src/sandbox/lifecycle/manager.ts b/packages/control-plane/src/sandbox/lifecycle/manager.ts index a5f2e785d9..d8da86c392 100644 --- a/packages/control-plane/src/sandbox/lifecycle/manager.ts +++ b/packages/control-plane/src/sandbox/lifecycle/manager.ts @@ -66,6 +66,7 @@ import { } from "./launch-context"; import type { AlarmScheduler, BackgroundTasks, SessionWebSocket } from "../../platform-ports"; import { DEFAULT_SANDBOX_STATUS } from "../sandbox-status"; +import { SANDBOX_RUNTIME_VERSION } from "../runtime-manifest"; import type { SandboxGeneration, SandboxReadiness, @@ -808,7 +809,10 @@ export class SandboxLifecycleManager const prebuiltImageId: string | null = selectedImage?.providerImageId ?? null; const prebuiltImageSha: string | null = selectedImage?.primaryBaseSha ?? null; - const mcpServers = await this.launchContext.loadMcpServers(repositories); + const { servers: mcpServers } = await this.launchContext.loadMcpServers( + repositories, + selectedImage?.runtimeVersion ?? SANDBOX_RUNTIME_VERSION + ); const codeServerEnabled = session.code_server_enabled === 1; const vncEnabled = session.vnc_enabled === 1; @@ -1097,6 +1101,21 @@ export class SandboxLifecycleManager /** * Restore a sandbox from a filesystem snapshot. */ + /** Tells the user which restricted MCP servers a restored, older runtime was not given. */ + private recordWithheldMcpServers(names: string[], snapshotImageId: string): void { + try { + this.config.recordWarning?.( + `MCP servers limited to selected tools (${names.join(", ")}) are unavailable in this session, because its saved sandbox runs a runtime that cannot apply the selection. Start a new session to use them.`, + `mcp-servers-withheld:${snapshotImageId}` + ); + } catch (error) { + this.log.warn("Could not record withheld MCP servers warning", { + event: "mcp.restricted_servers_withheld_notice_failed", + error: error instanceof Error ? error.message : String(error), + }); + } + } + private async restoreFromSnapshot( snapshotImageId: string, snapshotRuntimeVersion: string @@ -1151,7 +1170,8 @@ export class SandboxLifecycleManager const vncEnabled = session.vnc_enabled === 1; const agentSlackNotifyEnabled = await this.launchContext.resolveAgentSlackNotifyEnabled(session); - const mcpServers = await this.launchContext.loadMcpServers(repositories); + const { servers: mcpServers, withheld: withheldMcpServers } = + await this.launchContext.loadMcpServers(repositories, snapshotRuntimeVersion); const { sandboxSettings, timeoutSeconds } = this.launchContext.resolveSandboxSettings(session); const restoreConfig = { @@ -1215,6 +1235,9 @@ export class SandboxLifecycleManager type: "sandbox_restored", message: "Session restored from snapshot", }); + if (withheldMcpServers.length) { + this.recordWithheldMcpServers(withheldMcpServers, snapshotImageId); + } this.log.info("Sandbox restore completed", { event: "sandbox.restore", diff --git a/packages/control-plane/src/sandbox/runtime-manifest.ts b/packages/control-plane/src/sandbox/runtime-manifest.ts index 1078a03fd4..955df02fac 100644 --- a/packages/control-plane/src/sandbox/runtime-manifest.ts +++ b/packages/control-plane/src/sandbox/runtime-manifest.ts @@ -11,6 +11,8 @@ export const MIN_COMPATIBLE_RUNTIME_GENERATION = runtimeManifest.minimumCompatib export const MIN_SHUTDOWN_PROTOCOL_RUNTIME_GENERATION = runtimeManifest.minimumPreservationGeneration; export const MIN_REBUILD_RUNTIME_GENERATION = runtimeManifest.minimumRebuildGeneration; +export const MIN_MCP_TOOL_ALLOWLIST_RUNTIME_GENERATION = + runtimeManifest.minimumMcpToolAllowlistGeneration; /** Per-harness image floors; see minCompatibleRuntimeVersionFor in image-builds/model.ts. */ export const HARNESS_MIN_RUNTIME_GENERATION: Readonly>> = runtimeManifest.harnessMinimumGeneration; 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/fake-mcp-server.ts b/packages/control-plane/test/integration/fake-mcp-server.ts new file mode 100644 index 0000000000..d1d1662370 --- /dev/null +++ b/packages/control-plane/test/integration/fake-mcp-server.ts @@ -0,0 +1,52 @@ +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. + */ +export 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 }); +} 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..9386bbc28d 100644 --- a/packages/control-plane/vitest.integration.config.ts +++ b/packages/control-plane/vitest.integration.config.ts @@ -3,6 +3,7 @@ import { defineConfig } from "vitest/config"; import path from "path"; import { webcrypto } from "node:crypto"; import { createRequire } from "node:module"; +import { fakeMcpServer } from "./test/integration/fake-mcp-server"; const migrationsPath = path.resolve(__dirname, "../../terraform/d1/migrations"); @@ -51,6 +52,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..c08213050a 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-11" 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,23 @@ 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` (OpenCode) or `docs__extra` next to `docs`, gets no + tools. A session that started before your deployment supported tool selection resumes + without its limited servers and shows a warning. 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_runtime_manifest.py b/packages/modal-infra/tests/test_runtime_manifest.py index 8fe475b41a..84dfa5dfbd 100644 --- a/packages/modal-infra/tests/test_runtime_manifest.py +++ b/packages/modal-infra/tests/test_runtime_manifest.py @@ -7,3 +7,4 @@ def test_runtime_manifest_generation_matches_version() -> None: assert CACHE_BUSTER == RUNTIME_VERSION assert RUNTIME_MANIFEST["minimumCompatibleGeneration"] <= RUNTIME_MANIFEST["generation"] assert RUNTIME_MANIFEST["minimumRebuildGeneration"] <= RUNTIME_MANIFEST["generation"] + assert RUNTIME_MANIFEST["minimumMcpToolAllowlistGeneration"] <= RUNTIME_MANIFEST["generation"] 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..70c76a645b 100644 --- a/packages/sandbox-runtime/src/sandbox_runtime/harness/claude.py +++ b/packages/sandbox-runtime/src/sandbox_runtime/harness/claude.py @@ -28,6 +28,7 @@ RuntimeCredentialDenied, RuntimeCredentialUnavailable, ) +from ..mcp_tool_access import claude_tool_policy from .base import ( EventSink, HarnessId, @@ -314,14 +315,16 @@ def build_options(self, model: str, reasoning_effort: str | None) -> Any: if self.credential is None or self.wrapper_path is None or self.session_id is None: 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) 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) + # The built-in server replaces a configured one of the same name. mcp_servers[OI_TOOL_SERVER_NAME] = self._tool_server - allowed_tools.append(f"mcp__{OI_TOOL_SERVER_NAME}__*") + tool_policy = claude_tool_policy( + self.config.mcp_servers, + reserved=(OI_TOOL_SERVER_NAME,) if self._tool_client is not None else (), + ) system_prompt: dict[str, Any] = {"type": "preset", "preset": "claude_code"} if self.config.system_prompt_append: system_prompt["append"] = self.config.system_prompt_append @@ -331,8 +334,9 @@ def build_options(self, model: str, reasoning_effort: str | None) -> Any: "env": harness_env(self.config.config_dir, self.credential), "model": model, "mcp_servers": mcp_servers, - "allowed_tools": allowed_tools, - "disallowed_tools": [*DISALLOWED_TOOLS], + "allowed_tools": [*ALLOWED_TOOLS, *tool_policy.allowed_tools], + "disallowed_tools": [*DISALLOWED_TOOLS, *tool_policy.disallowed_tools], + "hooks": tool_policy.hooks, "permission_mode": "dontAsk", "system_prompt": system_prompt, "settings": CLAUDE_POLICY_SETTINGS, 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..170cc87087 --- /dev/null +++ b/packages/sandbox-runtime/src/sandbox_runtime/mcp_tool_access.py @@ -0,0 +1,237 @@ +"""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 dataclasses import dataclass, field +from typing import TYPE_CHECKING, Any + +from claude_agent_sdk import HookMatcher + +if TYPE_CHECKING: + from collections.abc import Collection, Iterable, Mapping + + from claude_agent_sdk.types import ( + HookCallback, + HookContext, + HookEvent, + 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 OpenCode +# OPENCODE_BUILTIN_TOOL_IDS_VERSION. OpenCode names an MCP tool +# ``_`` with no namespace marker, so a server's deny pattern could +# also match a built-in id; these are re-allowed after the MCP rules. When +# bumping OpenCode in packages/sandbox-images/toolchain.json, re-run the wire +# contract test in test_opencode_reasoning_contract.py against the new binary +# (updating the version it asserts), then update this tuple and its version. +OPENCODE_BUILTIN_TOOL_IDS_VERSION = "1.18.29" +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 + + +@dataclass(frozen=True) +class OpenCodeToolPolicy: + """OpenCode ``permission`` rules for a session's MCP servers. + + ``rules`` is empty when no server is restricted. ``shadowed`` lists the + unrestricted namespaces ``opencode_tool_policy`` leaves without an allow rule. + """ + + rules: dict[str, str] = field(default_factory=dict) + shadowed: list[str] = field(default_factory=list) + + +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_tool_policy( + servers: Iterable[Mapping[str, Any]], other_tool_ids: Iterable[str] +) -> OpenCodeToolPolicy: + """Rules that hide every MCP tool outside its server's 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. + + OpenCode names tools ``_``, so an unrestricted ``docs_extra``'s + ``docs_extra_*`` would also match restricted ``docs``' own ``extra_*`` + tools. Such a server gets no allow rule rather than widening the allowlist. + """ + allowlists = _namespace_allowlists(servers) + restricted = [namespace for namespace, allowlist in allowlists.items() if allowlist is not None] + if not restricted: + return OpenCodeToolPolicy() + + shadowed = sorted( + namespace + for namespace, allowlist in allowlists.items() + if allowlist is None and any(namespace.startswith(f"{other}_") for other in restricted) + ) + 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 OpenCodeToolPolicy(rules, shadowed) + + +@dataclass(frozen=True) +class ClaudeToolPolicy: + """Claude Agent SDK options for a session's MCP servers; ``hooks`` is empty + when no server is restricted.""" + + allowed_tools: list[str] + disallowed_tools: list[str] + hooks: dict[HookEvent, list[HookMatcher]] = field(default_factory=dict) + + +def claude_tool_policy( + servers: Iterable[Mapping[str, Any]], *, reserved: Collection[str] = () +) -> ClaudeToolPolicy: + """Allow and deny rules for each server's tools, plus a guard hook. + + ``reserved`` names servers the harness provides itself in place of any + configured server of the same name: their configured allowlists are ignored + and all their tools are allowed. + + Under ``dontAsk`` a listed tool runs and any other is refused, unless another + approval covers it. 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 approve unselected tools, so a + ``PreToolUse`` hook refuses them; a hook's deny wins. That also leaves an + unrestricted ``docs__extra`` next to a restricted ``docs`` without tools, + since their tool names cannot be told apart. 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. + """ + allowlists = { + namespace: allowlist + for namespace, allowlist in _namespace_allowlists(servers).items() + if namespace not in reserved + } + allowed: list[str] = [] + disallowed: list[str] = [] + for namespace, allowlist in allowlists.items(): + prefix = f"mcp__{namespace}" + if allowlist is None: + allowed.append(f"{prefix}__*") + elif not allowlist: + disallowed.append(prefix) + else: + allowed.extend(f"{prefix}__{mcp_name_segment(tool)}" for tool in allowlist) + allowed.extend(f"mcp__{name}__*" for name in reserved) + + restricted = {ns: allowlist for ns, allowlist in allowlists.items() if allowlist is not None} + if not restricted: + return ClaudeToolPolicy(allowed, disallowed) + guard = HookMatcher(hooks=[_claude_tool_guard(restricted)]) + return ClaudeToolPolicy(allowed, disallowed, {"PreToolUse": [guard]}) + + +def _claude_tool_guard(restricted: dict[str, tuple[str, ...]]) -> HookCallback: + """A ``PreToolUse`` hook refusing every ``mcp____*`` tool outside + its allowlist.""" + 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..64bd72b1db 100644 --- a/packages/sandbox-runtime/src/sandbox_runtime/opencode_server.py +++ b/packages/sandbox-runtime/src/sandbox_runtime/opencode_server.py @@ -15,6 +15,7 @@ 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, opencode_tool_policy, tool_allowlist from .memories import memory_path from .process_output import iter_process_lines from .sandbox_bin import install_bin_scripts @@ -30,6 +31,86 @@ AGENT_TOOLS_REQUIRING_REPOSITORY: set[str] = set() +_TOOL_EXPORT_SCAN_TIMEOUT_SECONDS = 15.0 +# Lists each file's export names with Bun's parser, which reads a module without +# running it: OpenCode, a Bun program, loads the same syntax. A file the parser +# rejects maps to null. +_TOOL_EXPORT_SCAN_SCRIPT = """ +const fs = require("fs"); +const exports = {}; +for (const file of process.argv.slice(1)) { + try { + const loader = file.endsWith(".ts") ? "ts" : "js"; + exports[file] = new Bun.Transpiler({ loader }).scan(fs.readFileSync(file, "utf8")).exports; + } catch { + exports[file] = null; + } +} +console.log(JSON.stringify(exports)); +""" + + +async def _read_tool_exports(paths: Sequence[Path]) -> dict[str, list[str] | None]: + """Each tool file's export names, from the image's pinned ``bun``.""" + process = await asyncio.create_subprocess_exec( + "bun", + "-e", + _TOOL_EXPORT_SCAN_SCRIPT, + *map(str, paths), + # Away from the repository, so its bunfig.toml does not apply. + cwd="/", + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + try: + stdout, stderr = await asyncio.wait_for( + process.communicate(), _TOOL_EXPORT_SCAN_TIMEOUT_SECONDS + ) + except TimeoutError: + process.kill() + await process.wait() + raise + if process.returncode: + raise RuntimeError(stderr.decode(errors="replace").strip()[-500:]) + exports: dict[str, list[str] | None] = json.loads(stdout) + return exports + + +async def _custom_tool_ids(workdir: Path, log: Any) -> list[str]: + """Ids of the custom tools in the project's and global config's tool + directories, Open-Inspect's and the repository's own. + + OpenCode names a file's default export ```` and each named export + ``_``. The rules must be in its config before it loads the + files, so their exports are read with Bun's parser. A file without ES module + exports (CommonJS) or one the parser cannot read falls back to its file + name, as if its only tool were the default export. + """ + config_dirs = (workdir / ".opencode", resolve_opencode_global_config_dir()) + paths = [ + path + for config_dir in config_dirs + for subdirectory in ("tool", "tools") + for path in sorted((config_dir / subdirectory).glob("*")) + if path.suffix in {".js", ".ts"} and path.is_file() + ] + if not paths: + return [] + try: + exports = await _read_tool_exports(paths) + except Exception as exc: + log.warn("opencode.custom_tool_scan_failed", exc=exc) + exports = {} + tool_ids: set[str] = set() + for path in paths: + names = exports.get(str(path)) + if not names: + tool_ids.add(path.stem) + continue + tool_ids.update(path.stem if name == "default" else f"{path.stem}_{name}" for name in names) + return sorted(tool_ids) + + 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 +512,17 @@ 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) + # Only a restricted server's deny pattern can hide a built-in or custom tool. + restricted = any(tool_allowlist(server) is not None for server in mcp_servers) + custom_tool_ids = await _custom_tool_ids(workdir, self.log) if restricted else [] + tool_policy = opencode_tool_policy( + mcp_servers, (*OPENCODE_BUILTIN_TOOL_IDS, *custom_tool_ids) + ) + if tool_policy.rules: + opencode_config["permission"].update(tool_policy.rules) + self.log.info("mcp.tool_allowlists_applied", rules=tool_policy.rules) + if tool_policy.shadowed: + self.log.warn("mcp.tool_allowlist_shadowed_servers", namespaces=tool_policy.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..1471099e49 100644 --- a/packages/sandbox-runtime/src/sandbox_runtime/runtime_manifest.json +++ b/packages/sandbox-runtime/src/sandbox_runtime/runtime_manifest.json @@ -1,11 +1,12 @@ { - "runtimeVersion": "v74-persistent-memory", - "generation": 74, + "runtimeVersion": "v75-mcp-tool-allowlist", + "generation": 75, "minimumCompatibleGeneration": 62, "minimumPreservationGeneration": 71, - "minimumRebuildGeneration": 74, + "minimumRebuildGeneration": 75, + "minimumMcpToolAllowlistGeneration": 75, "harnessMinimumGeneration": { - "claude": 74, - "opencode": 74 + "claude": 75, + "opencode": 75 } } diff --git a/packages/sandbox-runtime/src/sandbox_runtime/runtime_manifest.py b/packages/sandbox-runtime/src/sandbox_runtime/runtime_manifest.py index c965181c14..c21dface59 100644 --- a/packages/sandbox-runtime/src/sandbox_runtime/runtime_manifest.py +++ b/packages/sandbox-runtime/src/sandbox_runtime/runtime_manifest.py @@ -12,6 +12,10 @@ class RuntimeManifest(TypedDict): minimumCompatibleGeneration: int minimumPreservationGeneration: int minimumRebuildGeneration: int + # The first runtime that enforces MCP server tool allowlists; older ones + # would give a restricted server every tool, so the control plane leaves + # restricted servers out of their launches. + minimumMcpToolAllowlistGeneration: int # Per-harness floors for prebuilt-image selection: a harness whose runtime # support arrived later than the global floor names its own generation. harnessMinimumGeneration: dict[str, int] 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/claude_fakes.py b/packages/sandbox-runtime/tests/claude_fakes.py index e22686426e..a1a9e95d84 100644 --- a/packages/sandbox-runtime/tests/claude_fakes.py +++ b/packages/sandbox-runtime/tests/claude_fakes.py @@ -137,7 +137,7 @@ def __init__(self, tmp_path: Path, *, turns: list[list[Any]] | None = None, **ov default_model="claude-sonnet-4-6", oauth_managed=oauth_managed, system_prompt_append=overrides.pop("system_prompt_append", None), - tools=None, + tools=overrides.pop("tools", None), ) binary = tmp_path / "claude-bin" binary.write_text("#!/bin/sh\n") @@ -155,6 +155,7 @@ def client_factory(options: Any) -> FakeSdkClient: environ=environ, client_factory=client_factory, options_factory=lambda **kwargs: kwargs, + tool_server_factory=overrides.pop("tool_server_factory", None), transcript_exists=transcript_exists, binary=binary, ) 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..9f2daa8972 100644 --- a/packages/sandbox-runtime/tests/test_claude_harness.py +++ b/packages/sandbox-runtime/tests/test_claude_harness.py @@ -42,6 +42,7 @@ reasoning_options, ) from sandbox_runtime.harness.claude_env import ClaudeAuthMode +from sandbox_runtime.harness.claude_tools import OI_TOOL_SERVER_NAME, ToolServerConfig from sandbox_runtime.harness.claude_translate import AUTHENTICATION_FAILED_MESSAGE from tests.claude_fakes import ( FakeCredentialClient, @@ -209,6 +210,118 @@ 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 h.client.options["hooks"] == {} + + async def test_built_in_tool_server_replaces_a_configured_one(self, tmp_path: Path) -> None: + h = Harness( + tmp_path, + turns=[[_result(0.1)]], + mcp_servers=( + { + "name": OI_TOOL_SERVER_NAME, + "type": "remote", + "url": "https://mcp.oi", + "toolAllowlist": ["search"], + }, + ), + tools=ToolServerConfig( + control_plane_url="https://control-plane.test", + session_id="s1", + auth_token="token", + repo_manifest_path=tmp_path / "manifest.json", + has_repository=False, + slack_notify_enabled=False, + ), + tool_server_factory=lambda _client: "built-in", + ) + 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 + assert options["mcp_servers"][OI_TOOL_SERVER_NAME] == "built-in" + assert f"mcp__{OI_TOOL_SERVER_NAME}__*" in options["allowed_tools"] + assert f"mcp__{OI_TOOL_SERVER_NAME}__search" not in options["allowed_tools"] + assert options["hooks"] == {} + await h.harness.close() + + 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") == 1 + 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..7b5a8cd971 100644 --- a/packages/sandbox-runtime/tests/test_mcp.py +++ b/packages/sandbox-runtime/tests/test_mcp.py @@ -2,11 +2,13 @@ import json import os +import shutil from unittest.mock import AsyncMock, MagicMock, patch import pytest from sandbox_runtime.mcp_packages import NPM_PACKAGE_RE, McpPackageInstaller +from sandbox_runtime.opencode_server import _custom_tool_ids from tests.runtime_helpers import make_opencode_server @@ -249,3 +251,154 @@ 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, source in ( + ("memory_read.js", "export default tool({})"), + ("spawn-child.js", "export default tool({})"), + ("memory_local.ts", "export const notes = tool({})"), + ("memory_notes.md", "export default tool({})"), + ): + (project_tools / name).write_text(source) + global_tools = tmp_path / "global" / "tools" + global_tools.mkdir(parents=True) + (global_tools / "memory_global.js").write_text("export default tool({})") + scanned = { + "memory_read.js": ["default"], + "spawn-child.js": ["default"], + "memory_local.ts": ["notes"], + "memory_global.js": ["default"], + } + + async def read_tool_exports(paths): + return {str(path): scanned[path.name] for path in paths} + + with ( + patch.dict(os.environ, {"OPENCODE_CONFIG_DIR": str(tmp_path / "global")}), + patch("sandbox_runtime.opencode_server._read_tool_exports", read_tool_exports), + ): + config = await _start_and_capture_config(sup) + assert list(config["permission"].items()) == [ + ("*", {"*": "allow"}), + ("memory_*", "deny"), + ("memory_recall", "allow"), + ("memory_global", "allow"), + ("memory_local_notes", "allow"), + ("memory_read", "allow"), + ] + assert config["mcp"]["memory"]["url"] == "https://mcp.memory" + + @pytest.mark.asyncio + async def test_custom_tools_are_not_scanned_without_a_restricted_server(self, tmp_path): + sup = _make_supervisor( + {"mcp_servers": [{"name": "docs", "type": "remote", "url": "https://mcp.docs"}]} + ) + sup.workspace_path = tmp_path + (tmp_path / ".opencode" / "tool").mkdir(parents=True) + (tmp_path / ".opencode" / "tool" / "docs_local.js").write_text("export default 1") + with patch("sandbox_runtime.opencode_server._read_tool_exports") as read_tool_exports: + config = await _start_and_capture_config(sup) + read_tool_exports.assert_not_called() + assert config["permission"] == {"*": {"*": "allow"}} + + +class TestCustomToolIds: + @pytest.fixture + def tool_dir(self, tmp_path, monkeypatch): + monkeypatch.setenv("OPENCODE_CONFIG_DIR", str(tmp_path / "global")) + tool_dir = tmp_path / ".opencode" / "tool" + tool_dir.mkdir(parents=True) + return tool_dir + + @pytest.mark.asyncio + @pytest.mark.skipif(shutil.which("bun") is None, reason="needs bun, as in the sandbox image") + async def test_names_tools_as_opencode_does_from_bun_parsed_exports(self, tmp_path, tool_dir): + sources = { + # A comment or string that looks like an export is not one. + "docs.ts": '/* export const fetch_page = 1 */ const s = "export const x = 1";\n' + "export const local_index = tool({}), lookup = tool({});", + "docs_local.js": 'import { tool } from "@opencode-ai/plugin"; export default tool({});', + "docs_notes.js": "const a = tool({}); export { a, a as find };", + "broken.js": "export const = ;", + "legacy.js": 'const { tool } = require("@opencode-ai/plugin"); module.exports = tool({});', + "readme.md": "export default 1", + } + for name, source in sources.items(): + (tool_dir / name).write_text(source) + (tool_dir / "folder.ts").mkdir() + (tool_dir / "gone.js").symlink_to(tmp_path / "missing.js") + log = MagicMock() + + assert await _custom_tool_ids(tmp_path, log) == [ + # A file Bun cannot parse, or a CommonJS one, falls back to its name. + "broken", + "docs_local", + "docs_local_index", + "docs_lookup", + "docs_notes_a", + "docs_notes_find", + "legacy", + ] + log.warn.assert_not_called() + + @pytest.mark.asyncio + async def test_falls_back_to_file_names_when_the_scan_fails(self, tmp_path, tool_dir): + (tool_dir / "docs_notes.js").write_text("export const lookup = tool({})") + log = MagicMock() + failing = AsyncMock(side_effect=FileNotFoundError("bun")) + + with patch("sandbox_runtime.opencode_server._read_tool_exports", failing): + assert await _custom_tool_ids(tmp_path, log) == ["docs_notes"] + log.warn.assert_called_once() + assert log.warn.call_args.args[0] == "opencode.custom_tool_scan_failed" 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..a22e1d8e7e --- /dev/null +++ b/packages/sandbox-runtime/tests/test_mcp_tool_access.py @@ -0,0 +1,218 @@ +"""Tests for per-server MCP tool allowlists in both harnesses.""" + +import json +from pathlib import Path + +import pytest + +from sandbox_runtime.mcp_tool_access import ( + OPENCODE_BUILTIN_TOOL_IDS, + OPENCODE_BUILTIN_TOOL_IDS_VERSION, + claude_tool_policy, + mcp_name_segment, + opencode_tool_policy, + tool_allowlist, +) + +TOOLCHAIN = Path(__file__).parents[2] / "sandbox-images/toolchain.json" + + +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 TestOpencodeToolPolicy: + def test_no_rules_when_nothing_is_restricted(self): + policy = opencode_tool_policy([server("docs"), server("linear")], ["memory_read"]) + assert policy.rules == {} + assert policy.shadowed == [] + + 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 opencode_tool_policy(servers, []).rules == { + "my_docs_*": "deny", + "my_docs_fetch": "allow", + } + + def test_a_restricted_server_restricts_a_colliding_unrestricted_one(self): + servers = [server("my.docs"), server("my_docs", ["search"])] + expected = {"my_docs_*": "deny", "my_docs_search": "allow"} + assert opencode_tool_policy(servers, []).rules == expected + assert opencode_tool_policy(list(reversed(servers)), []).rules == expected + + def test_denies_the_server_namespace_then_allows_listed_tools(self): + rules = opencode_tool_policy([server("docs", ["search", "fetch"])], []).rules + assert list(rules.items()) == [ + ("docs_*", "deny"), + ("docs_search", "allow"), + ("docs_fetch", "allow"), + ] + + def test_empty_allowlist_denies_the_whole_server(self): + assert opencode_tool_policy([server("docs", [])], []).rules == {"docs_*": "deny"} + + def test_sanitizes_server_and_tool_names(self): + rules = opencode_tool_policy([server("my.server", ["find/issues"])], []).rules + 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. + policy = opencode_tool_policy([server("foo_bar"), server("foo", ["query"])], []) + assert list(policy.rules.items()) == [("foo_*", "deny"), ("foo_query", "allow")] + assert policy.shadowed == ["foo_bar"] + + def test_restricted_longer_namespace_keeps_its_rules_after_a_shorter_restricted_one(self): + rules = opencode_tool_policy([server("foo_bar", ["x"]), server("foo", ["query"])], []).rules + 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): + policy = opencode_tool_policy([server("foobar"), server("foo", ["query"])], []) + assert policy.shadowed == [] + assert policy.rules["foobar_*"] == "allow" + + def test_restricted_longer_namespace_overrides_unrestricted_shorter_one(self): + rules = opencode_tool_policy([server("foo"), server("foo_bar", ["x"])], []).rules + 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_tool_policy( + [server("memory", ["recall"]), server("apply", [])], + ["memory_read", "memory_search", "apply_patch", "bash", "spawn-child"], + ).rules + 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_tool_policy([{"toolAllowlist": []}], []).rules == {} + + def test_builtin_tool_ids_are_unique_and_sorted(self): + assert list(OPENCODE_BUILTIN_TOOL_IDS) == sorted(set(OPENCODE_BUILTIN_TOOL_IDS)) + + def test_builtin_tool_ids_were_taken_from_the_pinned_opencode(self): + # After bumping OpenCode, re-run the MCP wire test in + # test_opencode_reasoning_contract.py against the new binary, then + # update OPENCODE_BUILTIN_TOOL_IDS and its version. + pinned = json.loads(TOOLCHAIN.read_text())["opencode"] + assert pinned == OPENCODE_BUILTIN_TOOL_IDS_VERSION + + +class TestClaudeToolPolicy: + @staticmethod + async def decision(policy, tool_name): + [matcher] = policy.hooks["PreToolUse"] + [guard] = matcher.hooks + result = await guard({"tool_name": tool_name}, None, None) + return result.get("hookSpecificOutput", {}).get("permissionDecision", "pass") + + def test_unrestricted_server_allows_every_tool_without_a_guard(self): + policy = claude_tool_policy([server("linear")]) + assert policy.allowed_tools == ["mcp__linear__*"] + assert policy.disallowed_tools == [] + assert policy.hooks == {} + + def test_restricted_server_allows_only_listed_tools(self): + policy = claude_tool_policy([server("linear", ["list_issues", "get/issue"])]) + assert policy.allowed_tools == ["mcp__linear__list_issues", "mcp__linear__get_issue"] + assert policy.disallowed_tools == [] + + def test_empty_allowlist_denies_the_whole_server(self): + policy = claude_tool_policy([server("linear", [])]) + assert policy.allowed_tools == [] + assert policy.disallowed_tools == ["mcp__linear"] + + def test_sanitizes_the_server_name(self): + assert claude_tool_policy([server("my server")]).allowed_tools == ["mcp__my_server__*"] + + def test_servers_sharing_tool_names_share_the_strictest_allowlist(self): + policy = claude_tool_policy([server("my.docs"), server("my_docs", ["search"])]) + assert policy.allowed_tools == ["mcp__my_docs__search"] + + def test_reserved_server_replaces_a_configured_one(self): + policy = claude_tool_policy([server("oi", ["search"]), server("linear")], reserved=("oi",)) + assert policy.allowed_tools == ["mcp__linear__*", "mcp__oi__*"] + assert policy.hooks == {} + + @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_guard_refuses_only_restricted_servers_unselected_tools( + self, tool_name, expected + ): + policy = claude_tool_policy( + [server("linear", ["get/issue"]), server("docs"), server("noisy", [])] + ) + assert await self.decision(policy, 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_guard_refuses_unselected_tools_an_overlapping_server_would_approve( + self, tool_name, expected + ): + policy = claude_tool_policy([server("docs"), server("docs__extra", ["search"])]) + assert await self.decision(policy, 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..f1055b32a0 100644 --- a/packages/sandbox-runtime/tests/test_opencode_reasoning_contract.py +++ b/packages/sandbox-runtime/tests/test_opencode_reasoning_contract.py @@ -23,14 +23,18 @@ import shutil import socket import subprocess +import sys import threading import time import urllib.request from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from pathlib import Path +from unittest.mock import MagicMock import pytest +from sandbox_runtime.mcp_tool_access import OPENCODE_BUILTIN_TOOL_IDS, opencode_tool_policy +from sandbox_runtime.opencode_server import _custom_tool_ids from tests.test_prompt_stream import make_stream pytest_plugins = ["tests.test_reasoning_config"] @@ -38,6 +42,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): @@ -106,7 +111,7 @@ def anthropic_events(model): @pytest.fixture -async def wire_server(tmp_path, reasoning_config, request): +async def wire_server(tmp_path, reasoning_config, request, monkeypatch): assert subprocess.check_output([BINARY, "--version"], text=True).strip() == "1.18.29" captured = [] @@ -167,6 +172,53 @@ 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 tools whose names start like a restricted server's: + # OpenCode names a default export after its file and a named export + # _, even in a file named exactly like the server. Each + # file is one line with a commented-out export of docs' unselected + # fetch_page, which must not be mistaken for a tool. + custom_tools = tmp_path / ".opencode/tool" + custom_tools.mkdir(parents=True, exist_ok=True) + definition = ( + 'tool({ description: "Local", args: {}, async execute() { return "ok"; } })' + ) + for file_name, export in ( + ("docs_local.js", "default"), + ("docs_notes.js", "const lookup ="), + ("docs.js", "const local_index ="), + ): + (custom_tools / file_name).write_text( + 'import { tool } from "@opencode-ai/plugin"; ' + f"/* export const fetch_page = 1; */ export {export} {definition};\n" + ) + monkeypatch.setenv("OPENCODE_CONFIG_DIR", str(tmp_path / "config/opencode")) + log = MagicMock() + custom_tool_ids = await _custom_tool_ids(tmp_path, log) + log.warn.assert_not_called() + policy = opencode_tool_policy(servers, (*OPENCODE_BUILTIN_TOOL_IDS, *custom_tool_ids)) + config["permission"].update(policy.rules) config["agent"] = {"build": {"options": {"reasoningEffort": "high"}}} for provider in ("openai", "anthropic"): config["provider"].setdefault(provider, {})["options"] = { @@ -265,6 +317,29 @@ 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 + custom_ids = {"docs_local", "docs_notes_lookup", "docs_local_index"} + assert set(call("/experimental/tool/ids")) == { + *OPENCODE_BUILTIN_TOOL_IDS, + *custom_ids, + "invalid", + } + + 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", *custom_ids} <= 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.test.ts b/packages/web/src/app/api/mcp-servers/[id]/tools/route.test.ts new file mode 100644 index 0000000000..867d8fcfd4 --- /dev/null +++ b/packages/web/src/app/api/mcp-servers/[id]/tools/route.test.ts @@ -0,0 +1,52 @@ +import { NextRequest } from "next/server"; +import { beforeEach, expect, it, vi } from "vitest"; +import { getServerAuthSession } from "@/lib/server-auth-session"; +import { controlPlaneUserFetch } from "@/lib/control-plane"; +import { POST } from "./route"; + +vi.mock("@/lib/server-auth-session", () => ({ getServerAuthSession: vi.fn() })); +vi.mock("@/lib/control-plane", () => ({ controlPlaneUserFetch: vi.fn() })); + +const loadTools = () => + POST(new NextRequest("http://localhost/api/mcp-servers/mcp%2F1/tools", { method: "POST" }), { + params: Promise.resolve({ id: "mcp/1" }), + }); + +beforeEach(() => { + vi.resetAllMocks(); + vi.mocked(getServerAuthSession).mockResolvedValue({ user: { id: "user-1" } }); +}); + +it("relays the server's tools from the control plane", async () => { + vi.mocked(controlPlaneUserFetch).mockResolvedValue( + Response.json({ tools: [{ name: "search" }] }) + ); + + const response = await loadTools(); + + expect(controlPlaneUserFetch).toHaveBeenCalledWith("/mcp-servers/mcp%2F1/tools", { + method: "POST", + }); + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ tools: [{ name: "search" }] }); +}); + +it("keeps the status of an error response whose body is not JSON", async () => { + vi.mocked(controlPlaneUserFetch).mockResolvedValue( + new Response("Bad gateway", { status: 502 }) + ); + + const response = await loadTools(); + + expect(response.status).toBe(502); + expect(await response.json()).toEqual({ error: "Unexpected response from control plane" }); +}); + +it("refuses a signed-out request without calling the control plane", async () => { + vi.mocked(getServerAuthSession).mockResolvedValue(null); + + const response = await loadTools(); + + expect(response.status).toBe(401); + expect(controlPlaneUserFetch).not.toHaveBeenCalled(); +}); 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..e5a5e9ee2b --- /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"; +import { relayJsonResponse } from "@/lib/control-plane-json-proxy"; + +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", + }); + return relayJsonResponse(response); + } 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..0b8d9aa6ea 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 { act, 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,252 @@ 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("drops tools that finish loading after the connection was edited", async () => { + let resolveTools: (tools: typeof catalog) => void = () => {}; + mocks.discoverMcpTools.mockReturnValue(new Promise((resolve) => (resolveTools = resolve))); + 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" })); + const url = screen.getByDisplayValue("https://a.example.com"); + await user.type(url, "/v2"); + await act(async () => resolveTools(catalog)); + await user.clear(url); + await user.type(url, "https://a.example.com"); + + expect(screen.getByRole("button", { name: "Load tools" })).toBeInTheDocument(); + expect(screen.queryByRole("checkbox", { name: /search/ })).not.toBeInTheDocument(); + }); + + it("drops loaded tools when the server is saved elsewhere", async () => { + mocks.discoverMcpTools.mockResolvedValue(catalog); + const user = userEvent.setup(); + const { rerender } = 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(); + // Another admin replaced the headers, so the loaded tools may not apply. + const original = servers[0]; + servers[0] = { ...original, revision: original.revision + 1, hasHeaders: true }; + try { + rerender(); + } finally { + servers[0] = original; + } + + expect(screen.getByRole("button", { name: "Load tools" })).toBeInTheDocument(); + expect(screen.queryByRole("checkbox", { name: /^search/ })).not.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: ["navigate", "click"] }) + ) + ); + }); + + 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..4baed146fd 100644 --- a/packages/web/src/components/settings/mcp-servers-settings.tsx +++ b/packages/web/src/components/settings/mcp-servers-settings.tsx @@ -33,6 +33,13 @@ import { AlertDialogTitle, } from "@/components/ui/alert-dialog"; import { useCurrentUserAuthorization } from "@/hooks/use-current-user-authorization"; +import { + McpToolAccess, + toolAccessFromAllowlist, + toolAccessToAllowlist, + type McpToolDiscovery, + type ToolAccess, +} from "./mcp-tool-access"; type ScopeMode = "global" | "selected"; type Editor = @@ -62,6 +69,7 @@ type FormState = { envRows: EnvRow[]; repoScopes: string[]; scopeMode: ScopeMode; + toolAccess: ToolAccess; enabled: boolean; }; @@ -73,6 +81,7 @@ const emptyForm: FormState = { envRows: [createEnvRow()], repoScopes: [], scopeMode: "global", + toolAccess: toolAccessFromAllowlist(null), enabled: DEFAULT_MCP_SERVER_ENABLED, }; @@ -90,6 +99,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 +275,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 +): McpToolDiscovery { + if (form.type === "local") { + return { + kind: "unavailable", + reason: "Local servers start inside the sandbox, so add their tool names below.", + }; + } + if (!savedServer) { + return { kind: "unavailable", reason: "Save the server to load its tools." }; + } + 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." }; + } + return { kind: "available", serverId: savedServer.id, revision: savedServer.revision }; } function McpServerForm({ @@ -274,6 +309,7 @@ function McpServerForm({ loadingRepos, radioPrefix, hasExistingCredentials, + savedServer, }: McpServerFormProps) { const selectedRepoScopes = new Set(form.repoScopes); @@ -424,6 +460,13 @@ function McpServerForm({ )} + setForm({ ...form, toolAccess })} + radioPrefix={radioPrefix} + discovery={toolDiscovery(form, savedServer)} + /> +