diff --git a/docs/mcp-contracts.md b/docs/mcp-contracts.md index cdc58704b3..da2d20c70d 100644 --- a/docs/mcp-contracts.md +++ b/docs/mcp-contracts.md @@ -453,7 +453,7 @@ actually bound: any `127.0.0.0/8` address, `::1`, an IPv4-mapped loopback and 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. A reply sent before a request body was read (a refused `Host` or `Origin`, an unknown route, the startup `503`) closes the connection after the reply, so an unread upload cannot keep a socket open. Scoped +passes); a non-loopback bind relaxes both checks and logs the relaxation. A reply sent before a request body was read (a refused `Host` or `Origin`, an unknown route, the startup `503`) closes the connection after the reply, so an unread upload cannot keep a socket open. Request targets are taken verbatim as origin-form paths (`//host/path` is a path, not another origin); an absolute-form target is refused with `400`, and a method Fetch does not support with `405`. Only a lone `initialize` can replace an open session; a batch that carries one is left to the transport's refusal. 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 diff --git a/docs/specs/agent-environments/http-transport/verification.md b/docs/specs/agent-environments/http-transport/verification.md index 77c8947954..dabef5e7b8 100644 --- a/docs/specs/agent-environments/http-transport/verification.md +++ b/docs/specs/agent-environments/http-transport/verification.md @@ -265,3 +265,13 @@ comes from the incoming message's own body declaration, so a `GET` or `HEAD` that carries a body also closes, and the regression uses a keep-alive agent and checks that the socket actually closes after the early reply while a bodiless `GET` keeps its socket. + +A tenth round (Codex, three findings): the takeover guard now requires a lone +`initialize`, mirroring the transport (test: a batch of `initialize` plus +`notifications/initialized` is refused while the connected client keeps +echoing); the shutdown hook records completion only after it returns, so a +hook that threw stays eligible for the next close (test: a hook that throws +once runs twice across two closes and then no more); and the bridge joins the +request target onto the listener's origin verbatim, refusing absolute-form +targets (test: `//attacker.example/card` is a 404 for that path, a plain route +sees the bound host only, and `http://attacker.example/card` gets `400`). diff --git a/src/server/http/nodeHttpBridge.ts b/src/server/http/nodeHttpBridge.ts index e8bf78b589..86813543c1 100644 --- a/src/server/http/nodeHttpBridge.ts +++ b/src/server/http/nodeHttpBridge.ts @@ -5,6 +5,20 @@ import { pipeline } from "node:stream/promises"; /** Fetch rejects a body on these methods, so their request streams are not forwarded. */ const BODYLESS_METHODS: ReadonlySet = new Set(["GET", "HEAD"]); +/** The request target was not an origin-form path; the bridge serves an origin server only. */ +export class UnsupportedRequestTargetError extends Error { + override readonly name = "UnsupportedRequestTargetError"; + constructor(target: string) { + super(`Unsupported request target ${target}`); + } +} + +/** Join an origin-form target onto the listener's origin verbatim, so `//host/path` stays a path. */ +const requestUrl = (target: string, origin: string): URL => { + if (!target.startsWith("/")) throw new UnsupportedRequestTargetError(target); + return new URL(`${origin}${target}`); +}; + const toHeaders = (incoming: IncomingMessage["headers"]): Headers => { const headers = new Headers(); for (const [name, value] of Object.entries(incoming)) { @@ -27,7 +41,7 @@ export const toWebRequest = ( if (!response.writableFinished) controller.abort(); }); const method = request.method ?? "GET"; - return new Request(new URL(request.url ?? "/", origin), { + return new Request(requestUrl(request.url ?? "/", origin), { method, headers: toHeaders(request.headers), signal: controller.signal, diff --git a/src/server/http/serveStreamableHttp.test.ts b/src/server/http/serveStreamableHttp.test.ts index 725d4877c3..d262f186d3 100644 --- a/src/server/http/serveStreamableHttp.test.ts +++ b/src/server/http/serveStreamableHttp.test.ts @@ -609,3 +609,100 @@ it("answers a method Fetch refuses instead of letting it escape the request guar ]); await client.close(); }); + +/** One request with a caller-controlled request target, which URL-based clients normalize away. */ +const rawTargetRequest = ( + served: URL, + target: string, +): Promise<{ status: number; body: string }> => + new Promise((resolve, reject) => { + const request = httpRequest( + { + host: served.hostname, + port: Number(served.port), + path: target, + method: "GET", + agent: false, + headers: { host: `127.0.0.1:${served.port}` }, + }, + (response) => { + let body = ""; + response.setEncoding("utf8"); + response.on("data", (chunk: string) => { + body += chunk; + }); + response.on("end", () => + resolve({ status: response.statusCode ?? 0, body }), + ); + }, + ); + request.on("error", reject); + request.end(); + }); + +it("keeps a network-path request target as a path and refuses absolute-form targets", async () => { + const seen: string[] = []; + const { served } = await start({ + routes: [ + { + method: "GET", + path: "/card", + handle: (request) => { + seen.push(new URL(request.url).host); + return Response.json({ ok: true }); + }, + }, + ], + }); + const url = new URL(served.url); + const networkPath = await rawTargetRequest(url, "//attacker.example/card"); + expect(networkPath.status).toBe(404); + expect(networkPath.body).toContain( + "No route for GET //attacker.example/card", + ); + const absolute = await rawTargetRequest(url, "http://attacker.example/card"); + expect(absolute.status).toBe(400); + const plain = await rawTargetRequest(url, "/card"); + expect(plain.status).toBe(200); + expect(seen).toEqual([`127.0.0.1:${url.port}`]); +}); + +it("lets only a lone initialize replace a session, not a batch that carries one", async () => { + const { served } = await start(); + const { client } = await connect(served.url); + const url = new URL(served.url); + const single = JSON.parse(await initializeRequest().text()) as unknown; + const batch = await rawHttpRequest(url, { + method: "POST", + headers: { + host: `127.0.0.1:${url.port}`, + "content-type": "application/json", + accept: "application/json, text/event-stream", + }, + body: JSON.stringify([ + single, + { jsonrpc: "2.0", method: "notifications/initialized" }, + ]), + }); + expect(batch.status).toBeGreaterThanOrEqual(400); + expect(batch.status).toBeLessThan(500); + expect(await echo(client, "not evicted")).toEqual([ + { type: "text", text: "not evicted" }, + ]); + await client.close(); +}); + +it("retries a shutdown hook that threw instead of marking it complete", async () => { + let attempts = 0; + const slot = new McpSessionSlot( + createEchoServer(() => { + attempts += 1; + if (attempts === 1) throw new Error("hook failed once"); + }), + undefined, + ); + await expect(slot.close()).rejects.toThrow("hook failed once"); + await expect(slot.close()).resolves.toBeUndefined(); + await slot.close(); + expect(attempts).toBe(2); +}); diff --git a/src/server/http/serveStreamableHttp.ts b/src/server/http/serveStreamableHttp.ts index 8abe6c4aa8..444cc6271d 100644 --- a/src/server/http/serveStreamableHttp.ts +++ b/src/server/http/serveStreamableHttp.ts @@ -19,7 +19,11 @@ import { import { isIPv4, isIPv6 } from "node:net"; import type { Logger } from "pino"; -import { toWebRequest, writeWebResponse } from "./nodeHttpBridge.js"; +import { + toWebRequest, + UnsupportedRequestTargetError, + writeWebResponse, +} from "./nodeHttpBridge.js"; import { StreamableHttpListenError } from "./streamableHttpListenError.js"; /** MCP endpoint path shared by `rea mcp --http` and agent environment servers. */ @@ -136,8 +140,9 @@ const isInitializePost = async (request: Request): Promise => { const read = await readRequestBody(probe); if (read.tooLarge) return false; const body: unknown = JSON.parse(read.text); + // The transport accepts only a lone initialize, so only that may evict a session. return Array.isArray(body) - ? body.some((message) => isInitializeRequest(message)) + ? body.length === 1 && isInitializeRequest(body[0]) : isInitializeRequest(body); } catch { return false; @@ -168,10 +173,17 @@ export class McpSessionSlot { ) { const protocol = protocolOf(instance); const captured = protocol.onclose ?? NO_SHUTDOWN_HOOK; + let running = false; + // Completion is recorded only after the hook returns, so a hook that threw stays eligible for a retry. this.#shutdownHook = () => { - if (this.#hookRan) return; - this.#hookRan = true; - captured(); + if (this.#hookRan || running) return; + running = true; + try { + captured(); + this.#hookRan = true; + } finally { + running = false; + } }; protocol.onclose = this.#shutdownHook; } @@ -398,10 +410,18 @@ const serveNodeRequest = async ( try { try { webRequest = toWebRequest(request, response, origin()); - } catch { - // Fetch refuses methods such as TRACE that node:http accepts; answer instead of throwing. + } catch (cause: unknown) { + // Fetch refuses methods such as TRACE that node:http accepts, and the bridge refuses + // targets that are not origin-form paths; both are answered instead of thrown. + const target = cause instanceof UnsupportedRequestTargetError; await writeWebResponse( - jsonError(405, -32600, `Unsupported method ${request.method ?? ""}`), + jsonError( + target ? 400 : 405, + -32600, + target + ? "Unsupported request target" + : `Unsupported method ${request.method ?? ""}`, + ), response, { closeConnection: true }, );