From b76f354d76384abe56b079b0b37e1367d1908efd Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Sun, 18 Oct 2026 09:18:08 -0700 Subject: [PATCH] fix(mcp): classify loopback from the bound address and keep startup failures out of onerror Addresses the third automated review round: loopback is classified from the address the listener bound with the configured name added to the Host and Origin allowlist; scoped IPv6 literals are refused before binding and a URL that cannot be formed closes the listener; listener errors reach onerror only after a successful bind; the MCP runtime keeps its factory owner as the single owner of instance cleanup through `closeInstance: false`. Co-Authored-By: Claude Fable 5.1 --- docs/mcp-contracts.md | 16 ++- .../agent-environments/http-transport/spec.md | 11 +- .../http-transport/verification.md | 12 ++ src/main.ts | 2 + src/server/http/serveStreamableHttp.test.ts | 62 ++++++++++ src/server/http/serveStreamableHttp.ts | 109 ++++++++++++------ 6 files changed, 169 insertions(+), 43 deletions(-) diff --git a/docs/mcp-contracts.md b/docs/mcp-contracts.md index d9b2508ffb..4d0680d0ac 100644 --- a/docs/mcp-contracts.md +++ b/docs/mcp-contracts.md @@ -447,12 +447,16 @@ application/json` and an `Accept` naming both `application/json` and session; any other sessionless POST goes to the live transport, which refuses it without disturbing the session. Requests that carry no valid session id are refused with `400` or `404` as the SDK transport defines. A bind is loopback by -address class, not spelling: any `127.0.0.0/8` address, `::1`, an IPv4-mapped -loopback and `localhost` names. On such a bind, requests whose `Host` header is -outside the SDK loopback allowlist plus the bind's own hostname are refused with -`403`, and a browser `Origin` outside that allowlist is refused the same way (a -missing `Origin` passes); a non-loopback bind relaxes both checks and logs the -relaxation. Request bodies are bounded by the SDK's +address class, not spelling, and is classified from the address the listener +actually bound: any `127.0.0.0/8` address, `::1`, an IPv4-mapped loopback and +`localhost` names, including a host alias that resolves to one of them. On such +a bind, requests whose `Host` header is outside the SDK loopback allowlist plus +the configured name and the bound address are refused with `403`, and a browser +`Origin` outside that allowlist is refused the same way (a missing `Origin` +passes); a non-loopback bind relaxes both checks and logs the relaxation. Scoped +IPv6 literals (`fe80::1%eth0`) are refused before binding, because no URL can +name them. Listener errors reach the runtime's connection-loss handler only +after a successful bind; a bind failure prints the listen-failure message alone. Request bodies are bounded by the SDK's 4 MiB limit, and the response budget keeps its stdio-derived floor. The listener ends on SIGINT or SIGTERM only, not on stdin EOF, and exits `130` or `143` after cleanup settles. diff --git a/docs/specs/agent-environments/http-transport/spec.md b/docs/specs/agent-environments/http-transport/spec.md index 47cc8ee2fc..930b4380a4 100644 --- a/docs/specs/agent-environments/http-transport/spec.md +++ b/docs/specs/agent-environments/http-transport/spec.md @@ -30,9 +30,14 @@ client registration keep stdio. response chunks as they are produced and turning a client disconnect into a request abort and body cancellation. - Classify a loopback bind by address class (`127.0.0.0/8`, `::1`, IPv4-mapped - loopback, `localhost` names), validate the `Host` and `Origin` headers with - the SDK loopback allowlist plus the bind's own hostname on such binds, and - relax both for other binds and log that relaxation. + loopback, `localhost` names) from the address the listener bound, validate + the `Host` and `Origin` headers with the SDK loopback allowlist plus the + configured name and the bound address on such binds, and relax both for + other binds and log that relaxation. Refuse scoped IPv6 literals before + binding, close the listener if its URL cannot be formed, and forward + listener errors to `onerror` only after a successful bind. +- Let the caller own instance cleanup (`closeInstance: false`) when its factory + owner already closes every instance, as the MCP runtime does. - Let only an `initialize` that carries the streamable headers and passes those checks replace an open session; refuse requests once the slot is closed, join an arming session during shutdown, and keep close stages retryable. diff --git a/docs/specs/agent-environments/http-transport/verification.md b/docs/specs/agent-environments/http-transport/verification.md index 2ecebdfec9..f00accc1fa 100644 --- a/docs/specs/agent-environments/http-transport/verification.md +++ b/docs/specs/agent-environments/http-transport/verification.md @@ -204,3 +204,15 @@ initialize is refused while the connected client keeps echoing), and the server instance is created only after the listener has bound, so a bind failure has nothing to clean up and a factory failure releases the listener and rethrows (test: a throwing factory rejects without announcing a URL). + +A third round (Greptile and Codex, five findings) was addressed next: the +listener's `error` forwarding to `onerror` is registered after the bind, so +`EADDRINUSE` prints only the listen-failure message (test: an occupied port +rejects without calling `onerror`); scoped IPv6 literals are refused before +binding and a URL that cannot be formed from the bound address closes the +listener (test: `fe80::1%lo0` rejects before the factory runs); loopback is +classified from the bound address with the configured name added to the +allowlist (test: `ip6-localhost` bound to `::1`, `dev.local` bound to +`127.0.0.1`); and the MCP runtime passes `closeInstance: false` so the factory +owner in `startMcpTransport` remains the single owner of instance cleanup +(test: `close` leaves the instance open until its owner closes it). diff --git a/src/main.ts b/src/main.ts index bbc36c2452..0a7c42337a 100644 --- a/src/main.ts +++ b/src/main.ts @@ -51,9 +51,11 @@ const httpRuntimeDependencies = ( // Loaded here so the default stdio entry never loads the HTTP listener. const { serveStreamableHttp } = await import("./server/http/serveStreamableHttp.js"); + // The transport's factory owner closes every instance it created; the listener must not race it. return serveStreamableHttp({ ...endpoint, createMcpServer: factory, + closeInstance: false, ...options, }); }, diff --git a/src/server/http/serveStreamableHttp.test.ts b/src/server/http/serveStreamableHttp.test.ts index e5afd1b579..7a9a34ed4d 100644 --- a/src/server/http/serveStreamableHttp.test.ts +++ b/src/server/http/serveStreamableHttp.test.ts @@ -359,3 +359,65 @@ it("releases the listener when the server factory fails after the port is bound" for (const line of logs) seen.push((JSON.parse(line) as { msg: string }).msg); expect(seen).not.toContain("MCP transport listening"); }); + +it("classifies loopback from the bound address and lists the configured name beside it", () => { + const sdk = ["localhost", "127.0.0.1", "[::1]"]; + expect(hostHeaderAllowlist("ip6-localhost", "::1")).toEqual([ + ...sdk, + "ip6-localhost", + ]); + expect(hostHeaderAllowlist("dev.local", "127.0.0.1")).toEqual([ + ...sdk, + "dev.local", + ]); + expect(hostHeaderAllowlist("0.0.0.0", "0.0.0.0")).toBeUndefined(); + expect(hostHeaderAllowlist("dev.local", "192.0.2.10")).toBeUndefined(); +}); + +it("refuses a scoped IPv6 bind before creating anything and keeps bind failures away from onerror", async () => { + let factoryCalls = 0; + const onerror = (): void => { + throw new Error("onerror must not run during startup"); + }; + await expect( + serveStreamableHttp({ + host: "fe80::1%lo0", + port: 0, + onerror, + createMcpServer: () => { + factoryCalls += 1; + return createEchoServer(() => undefined); + }, + }), + ).rejects.toBeInstanceOf(StreamableHttpListenError); + expect(factoryCalls).toBe(0); + const { served } = await start(); + await expect( + serveStreamableHttp({ + host: "127.0.0.1", + port: Number(new URL(served.url).port), + onerror, + createMcpServer: () => createEchoServer(() => undefined), + }), + ).rejects.toThrow(/EADDRINUSE/u); +}); + +it("leaves the instance to its owner when closeInstance is false", async () => { + let instanceCloses = 0; + const server = createEchoServer(() => undefined); + const original = server.close.bind(server); + server.close = async () => { + instanceCloses += 1; + await original(); + }; + const served = await serveStreamableHttp({ + host: "127.0.0.1", + port: 0, + closeInstance: false, + createMcpServer: () => server, + }); + await served.close(); + expect(instanceCloses).toBe(0); + await server.close(); + expect(instanceCloses).toBe(1); +}); diff --git a/src/server/http/serveStreamableHttp.ts b/src/server/http/serveStreamableHttp.ts index 40fde0364e..beb1be5a23 100644 --- a/src/server/http/serveStreamableHttp.ts +++ b/src/server/http/serveStreamableHttp.ts @@ -41,6 +41,8 @@ export interface ServeStreamableHttpOptions { readonly logger?: Logger; /** Reports listener failures after binding; request failures are logged instead. */ readonly onerror?: (error: Error) => void; + /** Close the server instance as the last stage of `close`; false when the factory's owner closes it. */ + readonly closeInstance?: boolean; } export interface StreamableHttpServer { @@ -78,11 +80,15 @@ const isLoopbackHostname = (hostname: string): boolean => { const hex = /^([0-9a-f]{1,4}):[0-9a-f]{1,4}$/u.exec(mapped); return hex !== null && Number.parseInt(hex[1] ?? "0", 16) >> 8 === 0x7f; }; -/** Host and Origin allowlist for a loopback bind, including the bind's own hostname; undefined relaxes validation. */ -export const hostHeaderAllowlist = (host: string): string[] | undefined => { - const hostname = normalizedHostname(host); - if (hostname === undefined || !isLoopbackHostname(hostname)) return undefined; - return [...new Set([...localhostAllowedHostnames(), hostname])]; +/** Host and Origin allowlist when the configured host or the address it bound is loopback, including both names; undefined relaxes validation. */ +export const hostHeaderAllowlist = ( + host: string, + boundAddress = host, +): string[] | undefined => { + const names = [normalizedHostname(host), normalizedHostname(boundAddress)]; + const known = names.filter((name): name is string => name !== undefined); + if (!known.some((name) => isLoopbackHostname(name))) return undefined; + return [...new Set([...localhostAllowedHostnames(), ...known])]; }; const jsonError = (status: number, code: number, message: string): Response => @@ -240,9 +246,15 @@ const endpointUrl = ( export const serveStreamableHttp = async ( options: ServeStreamableHttpOptions, ): Promise => { - const { host, port, logger, routes = [] } = options; - const allowedHosts = hostHeaderAllowlist(host); - // The instance is created after the listener binds, so a bind failure leaves nothing to clean up. + const { host, port, logger, routes = [], closeInstance = true } = options; + // Scoped IPv6 literals bind but cannot be named in a URL; refuse them before anything exists. + if (host.includes("%")) + throw new StreamableHttpListenError( + new Error(`scoped IPv6 bind address ${host} is not supported`), + ); + // Loopback is classified from the address the listener binds, after it binds; the instance + // is created after that, so a bind failure leaves nothing to clean up. + let allowedHosts: string[] | undefined; let slot: McpSessionSlot | undefined; let origin = ""; let stopped = false; @@ -269,31 +281,15 @@ export const serveStreamableHttp = async ( : match.handle(request); }; const httpServer = createHttpServer(); - const handleRequest = async ( + const handleRequest = ( request: IncomingMessage, response: ServerResponse, - ): Promise => { - try { - const webResponse = await route(toWebRequest(request, response, origin)); - await writeWebResponse(webResponse, response); - } catch (cause: unknown) { - // A client that left mid-stream is routine; a failure before headers is not. - logger?.[response.headersSent ? "debug" : "warn"]( - { error: describe(cause) }, - "MCP HTTP request did not complete", - ); - if (response.headersSent) response.destroy(); - else - await writeWebResponse( - jsonError(500, -32603, "Internal server error"), - response, - ).catch(() => undefined); - } - }; - httpServer.on("request", (request, response) => { - void handleRequest(request, response); - }); - httpServer.on("error", (error) => options.onerror?.(error)); + ): Promise => + serveNodeRequest( + { route, origin: () => origin, logger }, + request, + response, + ); const closeListener = async (): Promise => { stopped = true; httpServer.closeAllConnections(); @@ -309,8 +305,24 @@ export const serveStreamableHttp = async ( } catch (cause: unknown) { throw new StreamableHttpListenError(cause); } - const url = endpointUrl(httpServer.address()); - origin = new URL(url).origin; + const bound = httpServer.address(); + let url: string; + try { + url = endpointUrl(bound); + origin = new URL(url).origin; + } catch (cause: unknown) { + await closeListener().catch(() => undefined); + throw new StreamableHttpListenError(cause); + } + allowedHosts = hostHeaderAllowlist( + host, + bound !== null && typeof bound === "object" ? bound.address : host, + ); + httpServer.on("request", (request, response) => { + void handleRequest(request, response); + }); + // Bind failures already rejected the listening wait above; only later listener errors are reported. + httpServer.on("error", (error) => options.onerror?.(error)); let instance: McpServer | Server; try { instance = await options.createMcpServer({ era: "legacy" }); @@ -335,11 +347,40 @@ export const serveStreamableHttp = async ( const close = stagedClose([ closeListener, () => live.close(), - () => instance.close(), + ...(closeInstance ? [() => instance.close()] : []), ]); return { url, close }; }; +/** Translate one node:http exchange through `route`, logging a failure and answering it when headers are still open. */ +const serveNodeRequest = async ( + target: { + readonly route: (request: Request) => Promise; + readonly origin: () => string; + readonly logger: Logger | undefined; + }, + request: IncomingMessage, + response: ServerResponse, +): Promise => { + const { route, origin, logger } = target; + try { + const webResponse = await route(toWebRequest(request, response, origin())); + await writeWebResponse(webResponse, response); + } catch (cause: unknown) { + // A client that left mid-stream is routine; a failure before headers is not. + logger?.[response.headersSent ? "debug" : "warn"]( + { error: describe(cause) }, + "MCP HTTP request did not complete", + ); + if (response.headersSent) response.destroy(); + else + await writeWebResponse( + jsonError(500, -32603, "Internal server error"), + response, + ).catch(() => undefined); + } +}; + /** Run close stages in order once each; a failed stage is retried by the next call instead of replaying its rejection. */ const stagedClose = ( stages: readonly (() => Promise)[],