diff --git a/docs/specs/agent-environments/http-transport/verification.md b/docs/specs/agent-environments/http-transport/verification.md index 3f23150f62..77c8947954 100644 --- a/docs/specs/agent-environments/http-transport/verification.md +++ b/docs/specs/agent-environments/http-transport/verification.md @@ -256,3 +256,12 @@ hold the socket. The bridge now closes the connection after any reply whose request body was never read (test: an unknown-route POST and a foreign-Host POST are answered with `Connection: close`, while a keep-alive GET without a body keeps its connection). + +A ninth round (Greptile and Codex): request conversion ran outside the guard, +so a method Fetch refuses (`TRACE`) could crash the process; the conversion is +now guarded and answered with `405` and a closed connection (test: `TRACE` +gets `405` and the next client still echoes). The unread-body decision now +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. diff --git a/src/server/http/serveStreamableHttp.test.ts b/src/server/http/serveStreamableHttp.test.ts index 4037bfc433..725d4877c3 100644 --- a/src/server/http/serveStreamableHttp.test.ts +++ b/src/server/http/serveStreamableHttp.test.ts @@ -6,6 +6,7 @@ import { McpServer, WebStandardStreamableHTTPServerTransport, } from "@modelcontextprotocol/server"; +import { Agent, request as httpRequest } from "node:http"; import pino from "pino"; import { afterEach, expect, it, vi } from "vitest"; import { z } from "zod"; @@ -83,6 +84,50 @@ const echo = async (client: Client, text: string): Promise => { return result.content; }; +/** One keep-alive request; reports whether the server closed the socket after the reply. */ +const keepAliveRequest = ( + agent: Agent, + url: URL, + options: { + readonly method: string; + readonly headers?: Readonly>; + readonly body?: string; + }, +): Promise<{ + status: number; + headers: Record; + socketClosed: boolean; +}> => + new Promise((resolve, reject) => { + const request = httpRequest( + url, + { method: options.method, headers: { ...options.headers }, agent }, + (response) => { + const socket = response.socket; + let closed = false; + socket?.once("close", () => { + closed = true; + }); + response.resume(); + response.once("end", () => { + // Give a closing server one turn to end the socket after the reply. + setTimeout( + () => + resolve({ + status: response.statusCode ?? 0, + headers: response.headers, + socketClosed: closed || (socket?.destroyed ?? false), + }), + 50, + ); + }); + response.once("error", reject); + }, + ); + request.once("error", reject); + request.end(options.body); + }); + /** A new client's initialize as the bridge hands it to the slot: no session id yet. */ const initializeRequest = (): Request => new Request("http://127.0.0.1/mcp", { @@ -516,27 +561,51 @@ it("answers an oversized sessionless POST promptly instead of stalling on the pr await client.close(); }, 20_000); -it("closes the connection after a reply sent before the request body was read", async () => { +it("closes the connection after a reply sent before the request body was read, on a keep-alive client", async () => { const { served } = await start(); const url = new URL(served.url); - const host = `127.0.0.1:${url.port}`; - const unread = await rawHttpRequest(new URL("/nowhere", url), { - method: "POST", - headers: { host, "content-type": "application/json" }, - body: '{"never":"read"}', - }); - expect(unread.status).toBe(404); - expect(unread.headers.connection).toBe("close"); - const foreign = await rawHttpRequest(url, { - method: "POST", - headers: { host: "attacker.example", "content-type": "application/json" }, - body: '{"never":"read"}', - }); - expect(foreign.status).toBe(403); - expect(foreign.headers.connection).toBe("close"); - // A keep-alive client shows the server does not close after a bodiless reply. - const bodiless = await fetch(new URL("/nowhere", url)); - expect(bodiless.status).toBe(404); - expect(bodiless.headers.get("connection")).not.toBe("close"); - await bodiless.body?.cancel(); + const agent = new Agent({ keepAlive: true, maxSockets: 1 }); + try { + const unread = await keepAliveRequest(agent, new URL("/nowhere", url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: '{"never":"read"}', + }); + expect(unread.status).toBe(404); + expect(unread.headers.connection).toBe("close"); + expect(unread.socketClosed).toBe(true); + // Node's parser refuses a chunked GET body itself; a declared Content-Length reaches the bridge. + const bodyOnGet = await keepAliveRequest(agent, new URL("/nowhere", url), { + method: "GET", + headers: { "content-type": "text/plain", "content-length": "15" }, + body: "a body on a GET", + }); + expect(bodyOnGet.status).toBe(404); + expect(bodyOnGet.headers.connection).toBe("close"); + expect(bodyOnGet.socketClosed).toBe(true); + const bodiless = await keepAliveRequest(agent, new URL("/nowhere", url), { + method: "GET", + }); + expect(bodiless.status).toBe(404); + expect(bodiless.headers.connection).not.toBe("close"); + expect(bodiless.socketClosed).toBe(false); + } finally { + agent.destroy(); + } +}); + +it("answers a method Fetch refuses instead of letting it escape the request guard", async () => { + const { served } = await start(); + const url = new URL(served.url); + const trace = await rawHttpRequest(url, { + method: "TRACE", + headers: { host: `127.0.0.1:${url.port}` }, + }); + expect(trace.status).toBe(405); + expect(trace.headers.connection).toBe("close"); + const { client } = await connect(served.url); + expect(await echo(client, "still alive")).toEqual([ + { type: "text", text: "still alive" }, + ]); + await client.close(); }); diff --git a/src/server/http/serveStreamableHttp.ts b/src/server/http/serveStreamableHttp.ts index 7a12eff5d5..8abe6c4aa8 100644 --- a/src/server/http/serveStreamableHttp.ts +++ b/src/server/http/serveStreamableHttp.ts @@ -386,12 +386,27 @@ const serveNodeRequest = async ( response: ServerResponse, ): Promise => { const { route, origin, logger } = target; - const webRequest = toWebRequest(request, response, origin()); - // A reply sent before the body was read leaves that body unread on the socket, which the - // bridged stream keeps paused; closing the connection after the reply releases it. + // A reply sent before an incoming body was read leaves that body on the socket, which the + // bridged stream keeps paused; closing the connection after the reply releases it. The + // incoming message decides, so a GET or HEAD that carried a body closes too. + const declaresBody = + request.headers["content-length"] !== undefined || + request.headers["transfer-encoding"] !== undefined; + let webRequest: Request | undefined; const unreadBody = (): boolean => - webRequest.body !== null && !webRequest.bodyUsed; + declaresBody && !(webRequest?.bodyUsed ?? false); try { + try { + webRequest = toWebRequest(request, response, origin()); + } catch { + // Fetch refuses methods such as TRACE that node:http accepts; answer instead of throwing. + await writeWebResponse( + jsonError(405, -32600, `Unsupported method ${request.method ?? ""}`), + response, + { closeConnection: true }, + ); + return; + } const webResponse = await route(webRequest); await writeWebResponse(webResponse, response, { closeConnection: unreadBody(),