From e5141cdc6b2b09fe5318bd8a0f2bf7a51d10df9e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 18:27:18 +0000 Subject: [PATCH] fix(mcp): require the auth choice startMcpHttpServer serves under MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `token` was optional with no default, so a host that omitted it published an unauthenticated endpoint that executes every registered task — and nothing logged or threw, because `authorizeBearer(header, undefined)` allows the request without reading the header, which is also the right answer for a deliberate opt-out. A wildcard bind made it worse: `resolveAllowedHosts` returns no `Host` allow-list for one, so `{ port, host: "0.0.0.0", createServer }` type-checked and served task execution to the network with neither a token nor a rebinding guard. `token` is now required and `string | null`: omitting it is a type error, an untyped caller is refused at run time, and `null` — the written opt-out — is refused outright together with a wildcard bind, where nothing else is left to decide who may run a task. BREAKING CHANGE: `StartMcpHttpServerArgs.token` is required. Pass the token, or `null` to serve unauthenticated on a named interface. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01T8DU24G9GWuRUJncc5TJFZ --- .claude/CLAUDE.md | 5 +- examples/cli/src/commands/mcpServe.test.ts | 2 +- examples/cli/src/commands/mcpServe.ts | 8 ++-- packages/mcp/README.md | 3 +- packages/mcp/src/server/McpHttpServer.ts | 46 ++++++++++++++++--- .../server/__tests__/McpHttpServer.test.ts | 44 ++++++++++++++---- .../src/server/__tests__/elicitation.test.ts | 2 +- 7 files changed, 88 insertions(+), 22 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 9b41ce9d2..49a169d59 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -413,7 +413,10 @@ stream eagerly and so cannot tell the two apart. builder and embarc want the same server without the CLI around it: `createTaskMcpServer` (the tool surface, over any transport), `startMcpHttpServer` (`node:http`), `McpSessionRouter` (the Streamable HTTP session map, for a host that already has a web -framework) and `authorizeBearer`. It is built on the SDK's low-level `Server` rather than +framework) and `authorizeBearer`. The token property above is the CLI's own only in how it +generates one: `startMcpHttpServer` requires `token`, and a host serving unauthenticated +has to write `null` — refused outright on a wildcard bind, where nothing else decides who +may run a task. It is built on the SDK's low-level `Server` rather than `McpServer` because tasks describe themselves in JSON Schema and `registerTool` takes only Zod — going through it would mean converting a schema to Zod and back to publish it. diff --git a/examples/cli/src/commands/mcpServe.test.ts b/examples/cli/src/commands/mcpServe.test.ts index 58507acf6..cf4000655 100644 --- a/examples/cli/src/commands/mcpServe.test.ts +++ b/examples/cli/src/commands/mcpServe.test.ts @@ -78,6 +78,6 @@ describe("resolveServeToken", () => { }); it("serves without a token only when --no-auth said so", () => { - expect(resolveServeToken({ auth: false }, { [MCP_TOKEN_ENV]: "from-env" })).toBeUndefined(); + expect(resolveServeToken({ auth: false }, { [MCP_TOKEN_ENV]: "from-env" })).toBeNull(); }); }); diff --git a/examples/cli/src/commands/mcpServe.ts b/examples/cli/src/commands/mcpServe.ts index 910eb0594..323909fed 100644 --- a/examples/cli/src/commands/mcpServe.ts +++ b/examples/cli/src/commands/mcpServe.ts @@ -49,11 +49,11 @@ interface McpServeOptions { } /** - * The bearer token this server will require, or `undefined` for none. + * The bearer token this server will require, or `null` for none. * * A pinned token wins over a generated one because a client config has to hold * the same value across restarts, and the environment wins over nothing at all - * — but only `--no-auth` reaches `undefined`. Falling through to an + * — but only `--no-auth` reaches `null`. Falling through to an * unauthenticated server because no token was supplied is exactly the accident * this generates one to prevent — so an empty `--token` or an empty variable * falls through to a generated token rather than to the empty string, which is @@ -62,8 +62,8 @@ interface McpServeOptions { export function resolveServeToken( opts: { readonly auth: boolean; readonly token?: string }, env: Readonly> = process.env -): string | undefined { - if (!opts.auth) return undefined; +): string | null { + if (!opts.auth) return null; return opts.token || env[MCP_TOKEN_ENV] || generateBearerToken(); } diff --git a/packages/mcp/README.md b/packages/mcp/README.md index 9997450a9..46d3aa4d8 100644 --- a/packages/mcp/README.md +++ b/packages/mcp/README.md @@ -49,7 +49,8 @@ import { createTaskMcpServer, generateBearerToken, startMcpHttpServer } from "@w const handle = await startMcpHttpServer({ port: 8788, host: "127.0.0.1", - // `undefined` serves unauthenticated, which is a decision, never a default. + // Required. `null` serves unauthenticated, which has to be said out loud — + // and is refused outright on a wildcard bind. token: generateBearerToken(), createServer: () => createTaskMcpServer({ name: "my-app", version: "1.0.0" }), }); diff --git a/packages/mcp/src/server/McpHttpServer.ts b/packages/mcp/src/server/McpHttpServer.ts index 8be43b732..68a892e71 100644 --- a/packages/mcp/src/server/McpHttpServer.ts +++ b/packages/mcp/src/server/McpHttpServer.ts @@ -35,12 +35,16 @@ export interface StartMcpHttpServerArgs { /** Path the MCP endpoint answers on. Defaults to {@link DEFAULT_MCP_PATH}. */ readonly path?: string; /** - * Bearer token every request must present. + * Bearer token every request must present, or `null` to serve without + * authentication. * - * `undefined` serves without authentication — a deliberate choice for a host - * that has its own gate in front, never a default worth falling into. + * Required, and `null` rather than an omission, because every tool this + * server offers executes a task. A host that never states the choice would + * otherwise publish task execution to whatever reaches the port, and nothing + * in the request path can tell that apart from a host that meant to. + * `null` is refused for a wildcard bind — see {@link startMcpHttpServer}. */ - readonly token?: string | undefined; + readonly token: string | null; /** One MCP server instance per client session. */ readonly createServer: () => McpServerInstance; /** @@ -75,6 +79,34 @@ function isWildcardHost(host: string): boolean { return host === "" || host === "0.0.0.0" || host === "::" || host === "[::]"; } +/** + * Refuses a server that would run tasks for whoever reaches the port. + * + * `undefined` cannot be spelled through the types and is checked anyway: an + * untyped caller still arrives here with the field omitted, and that used to + * mean "serve unauthenticated". + * + * A wildcard bind additionally refuses `null` outright. It is reachable under + * every name and address the machine answers to, and {@link resolveAllowedHosts} + * can derive no `Host` allow-list from it either, so nothing at all would be + * left deciding who gets in. Naming the interface to bind is how an + * unauthenticated server on a reachable address is asked for. + */ +function assertAuthChoice(token: string | null | undefined, host: string): void { + if (token === undefined) { + throw new Error( + "startMcpHttpServer requires `token`: a bearer token every request must present, " + + "or `null` to serve task execution without authentication." + ); + } + if (token === null && isWildcardHost(host.toLowerCase())) { + throw new Error( + `startMcpHttpServer refuses to serve unauthenticated on the wildcard bind "${host}": ` + + "every tool it offers runs a task. Pass a token, or bind the one interface it should answer on." + ); + } +} + /** * The `Host` values to answer to, or `undefined` for "do not check". * @@ -260,6 +292,8 @@ async function serve( export async function startMcpHttpServer( args: StartMcpHttpServerArgs ): Promise { + assertAuthChoice(args.token, args.host); + const token = args.token ?? undefined; const path = args.path ?? DEFAULT_MCP_PATH; const router = new McpSessionRouter({ createTransport: (hooks) => new StreamableHTTPServerTransport(hooks), @@ -267,7 +301,7 @@ export async function startMcpHttpServer( }); const ctx: RequestContext = { path, - token: args.token, + token, allowedHosts: resolveAllowedHosts(args.host, args.allowedHosts), maxBodyBytes: args.maxBodyBytes ?? DEFAULT_MAX_BODY_BYTES, router, @@ -302,7 +336,7 @@ export async function startMcpHttpServer( return { server, url: `http://${displayHost}:${port}${path}`, - token: args.token, + token, sessionCount: () => router.size, close: async () => { await router.closeAll(); diff --git a/packages/mcp/src/server/__tests__/McpHttpServer.test.ts b/packages/mcp/src/server/__tests__/McpHttpServer.test.ts index 160f42917..07654aa9d 100644 --- a/packages/mcp/src/server/__tests__/McpHttpServer.test.ts +++ b/packages/mcp/src/server/__tests__/McpHttpServer.test.ts @@ -68,10 +68,7 @@ class StepTask extends Task, { done: boolean }> { const TASKS = [GreetTask, StepTask] as unknown as AnyTaskConstructor[]; -const open = async ( - token: string | undefined, - maxBodyBytes?: number -): Promise => +const open = async (token: string | null, maxBodyBytes?: number): Promise => startMcpHttpServer({ port: 0, host: "127.0.0.1", @@ -333,13 +330,17 @@ describe("startMcpHttpServer, bound to a wildcard", () => { await startMcpHttpServer({ port: 0, host: "0.0.0.0", - token: undefined, + token: TOKEN, createServer: () => createTaskMcpServer({ name: "test", version: "1.0.0", tasks: TASKS }), }) ); const response = await rawPost( `http://127.0.0.1:${new URL(handle.url).port}${new URL(handle.url).pathname}`, - { Host: "192.168.1.10", "content-type": "application/json" }, + { + Host: "192.168.1.10", + Authorization: `Bearer ${TOKEN}`, + "content-type": "application/json", + }, "{}" ); expect(response.status).not.toBe(403); @@ -350,7 +351,7 @@ describe("startMcpHttpServer, bound to a wildcard", () => { await startMcpHttpServer({ port: 0, host: "0.0.0.0", - token: undefined, + token: TOKEN, allowedHosts: ["mcp.internal"], createServer: () => createTaskMcpServer({ name: "test", version: "1.0.0", tasks: TASKS }), }) @@ -366,11 +367,38 @@ describe("startMcpHttpServer, bound to a wildcard", () => { describe("startMcpHttpServer, unauthenticated", () => { it("serves without a token when the host turned authentication off", async () => { - const handle = track(await open(undefined)); + const handle = track(await open(null)); expect(handle.token).toBeUndefined(); const client = track(await connect(handle, undefined)); const { tools } = await client.listTools(); expect(tools.map((tool) => tool.name)).toEqual(["GreetTask", "StepTask"]); }); + + it("refuses to start when the caller never stated an auth choice", async () => { + // Every tool here executes a task, so an omitted `token` must not be the + // same thing as an explicit opt-out. The types say so; an untyped caller + // (this package is published, and JS consumers exist) arrives with the + // field missing and has to be turned away at run time too. + const args = { + port: 0, + host: "127.0.0.1", + createServer: () => createTaskMcpServer({ name: "test", version: "1.0.0", tasks: TASKS }), + } as unknown as Parameters[0]; + await expect(startMcpHttpServer(args)).rejects.toThrow(/requires `token`/); + }); + + it("refuses an unauthenticated wildcard bind", async () => { + // A wildcard answers on every address the machine has, and + // `resolveAllowedHosts` derives no Host allow-list from it — so with no + // token there is nothing left deciding who may run tasks here. + await expect( + startMcpHttpServer({ + port: 0, + host: "0.0.0.0", + token: null, + createServer: () => createTaskMcpServer({ name: "test", version: "1.0.0", tasks: TASKS }), + }) + ).rejects.toThrow(/unauthenticated/); + }); }); diff --git a/packages/mcp/src/server/__tests__/elicitation.test.ts b/packages/mcp/src/server/__tests__/elicitation.test.ts index 27a6f0a8e..bd5925aa0 100644 --- a/packages/mcp/src/server/__tests__/elicitation.test.ts +++ b/packages/mcp/src/server/__tests__/elicitation.test.ts @@ -105,7 +105,7 @@ const open = async (elicitation?: boolean): Promise => startMcpHttpServer({ port: 0, host: "127.0.0.1", - token: undefined, + token: null, createServer: () => createTaskMcpServer({ name: "test", version: "1.0.0", tasks: TASKS, elicitation }), });