diff --git a/README.md b/README.md index f09bbd9..0f6f6a6 100644 --- a/README.md +++ b/README.md @@ -70,7 +70,7 @@ Every client call takes `{ fetch, timeoutMs, signal, session }` as its last argu ### Grants -Every call is checked as resource `tool:`. The most specific match wins, and `deny` beats `ask` beats `allow`. `tool:.` grants one remote tool and `tool:.*` covers a whole server. +Every call is checked as resource `tool:`. The most specific match wins, and `deny` beats `ask` beats `allow`. `tool:.` grants one remote tool and `tool:.*` covers a whole server. A handle or server name may not contain `.`, and a catalog may not list a tool name twice, so no tool falls under another server's grant. List `"."` in `allowWithoutAsk` to let an `allow` grant run that tool without approval. The list is ignored for tools flagged `destructiveHint: true`. @@ -112,7 +112,7 @@ mountMcpDiscovery(mcpRoutes, { Mount `mcpRoutes` on the hub app under `/api/tenants/:tenantId`, behind the hub's auth and tenant middleware. -`POST /api/tenants/:tenantId/mcp/discover` with `{ url, credentialId? }` returns `{ data: { serverInfo, tools } }`. `url` must be https (http only on loopback). `credentialId` names a tenant credential whose secret is sent as a bearer; a credential holding `MCP_NO_TOKEN_SENTINEL` from `@corbits/credential-http` sends no `authorization` header. Errors: 400 for a bad body or URL, 404 for an unknown credential or one that is not active or has expired, 422 when the server fails discovery, the secret is not a valid header value, or the request would leave the credential's origin. A 422 carries only this package's own messages; any other failure reads as a generic handshake error, so no response or `onError` text quotes the secret. `requireGrant` is the host's own grant middleware for this route. +`POST /api/tenants/:tenantId/mcp/discover` with `{ url, credentialId? }` returns `{ data: { serverInfo, tools } }`. `url` must be https; plain-http loopback is refused unless the host sets `allowLoopback: true` for local development. `credentialId` names a tenant credential whose secret is sent as a bearer; a credential holding `MCP_NO_TOKEN_SENTINEL` from `@corbits/credential-http` sends no `authorization` header. Errors: 400 for a bad body or URL, 404 for an unknown credential or one that is not active or has expired, 422 when the server fails discovery, the secret is not a valid header value, or the request would leave the credential's origin. A 422 carries only this package's own messages and never the upstream HTTP status; any other failure reads as a generic handshake error, so no response or `onError` text quotes the secret. `requireGrant` is the host's own grant middleware for this route. When a secret is sent, the fetch is pinned to the origin of the credential's provider `apiBaseUrl`; with no credential or a keyless one, to the URL's origin. Redirects are always refused, so the secret never leaves that origin. Each discovery request times out after 30 seconds. @@ -175,6 +175,7 @@ Grant the agent's principal `tool:deepwiki.*` for the whole server, or `tool:dee - Discovery with a credential pins to the credential's provider `apiBaseUrl` origin, not to the requested URL's origin. A credential with a secret whose provider has no `apiBaseUrl` is refused, and a URL on another origin needs `extraOrigins`. No origin is allowed by default. - `readCredentialSecret` is now `readCredential`, which returns `{ secret, origin? }`. `discoverMcpServer` takes `credential: { secret, origin? }` instead of `secret`. - `mcpInitialize` returns an `McpSession` (`protocolVersion`, `sessionId?`, `serverInfo?`) and sends `notifications/initialized`. Pass it as `{ session }` to `mcpListTools` and `mcpCallTool`; a stateful server needs it. +- The discovery route refuses plain-http loopback URLs unless `allowLoopback` is set. Handles and server names containing `.`, and catalogs repeating a tool name, are refused. `mcpTools` refuses an empty or repeated server name, and a non-https URL, before any request. - `@intx/agent` and `@intx/harness` peers are `^0.4.0`. - Discovery rejects authorization-server metadata whose `issuer` differs from the one the protected resource names. diff --git a/src/client.test.ts b/src/client.test.ts index b1015bd..ce32b03 100644 --- a/src/client.test.ts +++ b/src/client.test.ts @@ -352,3 +352,29 @@ describe("initialize hardening", () => { expect((failure as Error).message).toContain("within 50ms"); }); }); + +describe("tools/list", () => { + test("a catalog with duplicate tool names is refused", async () => { + const server = Bun.serve({ + port: 0, + async fetch(req) { + const parsed = RequestIdOnly(await req.json()); + if (parsed instanceof type.errors) + return new Response("", { status: 400 }); + const tool = { name: "dup", inputSchema: {} }; + return Response.json({ + jsonrpc: "2.0", + id: parsed.id, + result: { tools: [tool, tool] }, + }); + }, + }); + try { + await expect(mcpListTools(server.url.toString())).rejects.toThrow( + /duplicate tool names/, + ); + } finally { + void server.stop(true); + } + }); +}); diff --git a/src/client.ts b/src/client.ts index a6c88a5..682a4d9 100644 --- a/src/client.ts +++ b/src/client.ts @@ -51,9 +51,13 @@ const JsonRpcResponse = type({ }); export class McpError extends Error { - constructor(message: string) { + /** The HTTP status, when the server answered with a non-2xx one. */ + readonly status: number | undefined; + + constructor(message: string, status?: number) { super(message); this.name = "McpError"; + this.status = status; } } @@ -123,6 +127,7 @@ async function sendRequest( await response.body?.cancel(); throw new McpError( `MCP server ${url} responded ${response.status} to ${method}`, + response.status, ); } @@ -188,6 +193,7 @@ async function sendNotification( if (!response.ok) { throw new McpError( `MCP server ${url} responded ${response.status} to ${method}`, + response.status, ); } } @@ -351,6 +357,12 @@ export async function mcpListTools( `MCP server ${url} sent a malformed tools/list result: ${parsed.summary}`, ); } + const names = new Set(parsed.tools.map((tool) => tool.name)); + if (names.size !== parsed.tools.length) { + throw new McpError( + `MCP server ${url} sent a tools/list result with duplicate tool names`, + ); + } return parsed.tools; } diff --git a/src/hub/discover.test.ts b/src/hub/discover.test.ts index c0f8b63..fc1312e 100644 --- a/src/hub/discover.test.ts +++ b/src/hub/discover.test.ts @@ -29,6 +29,7 @@ function appWith( readonly status?: string; readonly expiresAt?: Date | null; readonly onError?: (error: unknown) => void; + readonly allowLoopback?: boolean; } = {}, ): Hono { const app = new Hono(); @@ -74,6 +75,7 @@ function appWith( }, extraOrigins: opts.extraOrigins, onError: opts.onError, + allowLoopback: opts.allowLoopback ?? true, } as unknown as MountMcpDiscoveryOpts; mountMcpDiscovery(app, mountOpts); return app; @@ -337,4 +339,31 @@ describe("POST /mcp/discover", () => { expect(status).toBe(404); expect(handle.requestsSeen).toHaveLength(0); }); + + test("a plain-http loopback target is refused unless the host allows it", async () => { + handle = startTestMcpServer(); + const { status, json } = await post(appWith({}, { allowLoopback: false }), { + url: handle.url, + }); + expect(status).toBe(400); + expect(String(json["error"])).toContain("https"); + expect(handle.requestsSeen).toHaveLength(0); + }); + + test("an upstream status is not echoed", async () => { + const server = Bun.serve({ + port: 0, + fetch: () => new Response("admin", { status: 403 }), + }); + try { + const { status, json } = await post(appWith({}), { + url: new URL("/admin", server.url).href, + }); + expect(status).toBe(422); + expect(String(json["error"])).toEndWith("the server refused the request"); + expect(String(json["error"])).not.toContain("403"); + } finally { + await server.stop(true); + } + }); }); diff --git a/src/hub/discover.ts b/src/hub/discover.ts index 510cc01..2f357c9 100644 --- a/src/hub/discover.ts +++ b/src/hub/discover.ts @@ -42,6 +42,11 @@ export type MountMcpDiscoveryOpts = { * origin. Passed through to `@corbits/credential-http`. Empty by default. */ readonly extraOrigins?: Readonly>; + /** + * Allow plain-http loopback targets, for local development only. Off by + * default so the route cannot be pointed at the hub's own ports. + */ + readonly allowLoopback?: boolean; /** Reported when a discovery attempt fails; the caller only sees a message. */ readonly onError?: (error: unknown, context: { url: string }) => void; }; @@ -205,6 +210,9 @@ export function mountMcpDiscovery( 400, ); } + if (target.protocol === "http:" && opts.allowLoopback !== true) { + return c.json({ error: "the MCP server URL must be https" }, 400); + } try { const found = @@ -229,11 +237,16 @@ export function mountMcpDiscovery( return c.json({ data }); } catch (cause) { // Only this package's own messages are passed on: anything else, such - // as a fetch error, may quote the material the request carried. + // as a fetch error, may quote the material the request carried. An + // upstream status is not echoed, so the route is no port-probing oracle. const error = - cause instanceof McpError + cause instanceof McpError && cause.status === undefined ? cause - : new McpError("the handshake failed"); + : new McpError( + cause instanceof McpError + ? "the server refused the request" + : "the handshake failed", + ); opts.onError?.(error, { url: body.url }); return c.json( { diff --git a/src/naming.ts b/src/naming.ts index c2ced26..ca8a2b8 100644 --- a/src/naming.ts +++ b/src/naming.ts @@ -3,12 +3,55 @@ // onto identical tool names and identical approval marks. import type { McpTool } from "./client.js"; +import { parseMcpEndpoint } from "./url.js"; /** `.`, the grantable unit: `tool:.*` covers a server. */ export function qualifiedName(server: string, tool: string): string { return `${server}.${tool}`; } +/** + * Refuse a catalog whose names could reach another server's grants: a server + * name with a `.` could make `.` fall under another server's + * `tool:.*`, and a repeated tool name would make two tools one name. + */ +export function assertCatalogNames( + server: string, + tools: readonly McpTool[], +): void { + if (server.includes(".")) { + throw new Error( + `MCP server name "${server}" must not contain "."; it would overlap another server's grants`, + ); + } + const seen = new Set(); + for (const tool of tools) { + if (seen.has(tool.name)) { + throw new Error( + `MCP server "${server}" lists the tool "${tool.name}" more than once`, + ); + } + seen.add(tool.name); + } +} + +/** + * Refuse a server list before any request: every name non-empty and unique, + * so no two servers share a grant prefix, and every URL a permitted endpoint. + */ +export function assertServers( + servers: readonly { name: string; url: string }[], + label: string, +): void { + const seen = new Set(); + for (const { name, url } of servers) { + if (name.length === 0) throw new Error(`empty ${label}`); + if (seen.has(name)) throw new Error(`duplicate ${label} "${name}"`); + seen.add(name); + parseMcpEndpoint(url); + } +} + export function toolDescription(tool: McpTool, server: string): string { const hints = Object.entries(tool.annotations ?? {}) .filter(([, v]) => v === true) diff --git a/src/sidecar-bundle.test.ts b/src/sidecar-bundle.test.ts index 8084fcf..968a885 100644 --- a/src/sidecar-bundle.test.ts +++ b/src/sidecar-bundle.test.ts @@ -313,3 +313,40 @@ describe("mcpServers round trip through a mediated handle", () => { expect(result.content).toContain("was cancelled"); }); }); + +describe("catalog names", () => { + const tool = (name: string): McpTool => ({ name, inputSchema: {} }); + + test("a catalog listing a tool twice is refused", () => { + expect(() => + mcpServers({ + servers: [ + { + handle: "srv", + url: "https://x.example.test/mcp", + tools: [tool("dup"), tool("dup")], + }, + ], + }), + ).toThrow(/more than once/); + }); + + test("a handle that would fall under another server's grant is refused", () => { + expect(() => + mcpServers({ + servers: [ + { + handle: "srv", + url: "https://x.example.test/mcp", + tools: [tool("read")], + }, + { + handle: "srv.read", + url: "https://y.example.test/mcp", + tools: [tool("x")], + }, + ], + }), + ).toThrow(/must not contain "."/); + }); +}); diff --git a/src/sidecar-bundle.ts b/src/sidecar-bundle.ts index 158e721..0c39744 100644 --- a/src/sidecar-bundle.ts +++ b/src/sidecar-bundle.ts @@ -26,8 +26,13 @@ import { McpToolSchema, type McpTool, } from "./client.js"; -import { isAskExempt, qualifiedName, toolDescription } from "./naming.js"; -import { parseMcpEndpoint } from "./url.js"; +import { + assertCatalogNames, + assertServers, + isAskExempt, + qualifiedName, + toolDescription, +} from "./naming.js"; /** The consumer key a host matches a credential binding against. */ export const SIDECAR_BUNDLE_ID = "@corbits/mcp/sidecar-bundle"; @@ -94,21 +99,18 @@ function assertConfig(config: McpServersConfig): void { if (parsed instanceof type.errors) { throw new Error(`invalid @corbits/mcp server config: ${parsed.summary}`); } - const seen = new Set(); - for (const server of config.servers) { - if (seen.has(server.handle)) { - throw new Error( - `invalid @corbits/mcp server config: duplicate credential handle "${server.handle}"`, - ); - } - seen.add(server.handle); - try { - parseMcpEndpoint(server.url); - } catch (cause) { - throw new Error( - `invalid @corbits/mcp server config: ${cause instanceof Error ? cause.message : String(cause)}`, - ); + try { + assertServers( + config.servers.map(({ handle, url }) => ({ name: handle, url })), + "credential handle", + ); + for (const server of config.servers) { + assertCatalogNames(server.handle, server.tools); } + } catch (cause) { + throw new Error( + `invalid @corbits/mcp server config: ${cause instanceof Error ? cause.message : String(cause)}`, + ); } } diff --git a/src/tool.test.ts b/src/tool.test.ts index 3127c20..6efc104 100644 --- a/src/tool.test.ts +++ b/src/tool.test.ts @@ -98,6 +98,30 @@ describe("mcpTools discovers a live server and floors every tool at ask", () => }); }); +describe("mcpTools refuses a bad server list before any request", () => { + test("empty, duplicate and non-https entries are refused", async () => { + handle = startTestMcpServer(); + const url = handle.url; + await expect(mcpTools({ servers: [{ name: "", url }] })).rejects.toThrow( + "empty MCP server name", + ); + await expect( + mcpTools({ + servers: [ + { name: "srv", url }, + { name: "srv", url }, + ], + }), + ).rejects.toThrow('duplicate MCP server name "srv"'); + await expect( + mcpTools({ + servers: [{ name: "srv", url: "http://mcp.example.test/mcp" }], + }), + ).rejects.toThrow("must be https"); + expect(handle.requestsSeen).toHaveLength(0); + }); +}); + describe("mcpTools bounds a stalling server", () => { const env = { sources: [], diff --git a/src/tool.ts b/src/tool.ts index 80bdf4b..a267cfe 100644 --- a/src/tool.ts +++ b/src/tool.ts @@ -20,7 +20,13 @@ import { type McpClientOptions, type McpTool, } from "./client.js"; -import { isAskExempt, qualifiedName, toolDescription } from "./naming.js"; +import { + assertCatalogNames, + assertServers, + isAskExempt, + qualifiedName, + toolDescription, +} from "./naming.js"; export interface McpServerConfig { /** Namespace prefix for this server's tools: `.`. */ @@ -87,6 +93,7 @@ export async function mcpTools( ): Promise> { const allowWithoutAsk = options.allowWithoutAsk ?? []; const timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS; + assertServers(options.servers, "MCP server name"); const discovered: DiscoveredTool[] = []; for (const server of options.servers) { @@ -97,6 +104,7 @@ export async function mcpTools( }; const session = await mcpInitialize(server.url, clientOpts); const tools = await mcpListTools(server.url, { ...clientOpts, session }); + assertCatalogNames(server.name, tools); for (const tool of tools) { discovered.push({ server,