fix(mcp): guard request conversion and close after any unread declared body

Addresses the ninth automated review round: a method Fetch refuses, such as
TRACE, is answered with 405 instead of escaping the request guard; the
unread-body decision now comes from the incoming message's own body
declaration, so a GET or HEAD carrying a body closes too; and the regression
uses a keep-alive client and checks that the socket actually closes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

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.

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<unknown> => {
   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<Record<string, string>>;
+    readonly body?: string;
+  },
+): Promise<{
+  status: number;
+  headers: Record<string, string | string[] | undefined>;
+  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();
 });

src/server/http/serveStreamableHttp.ts

@@ -386,12 +386,27 @@ const serveNodeRequest = async (
   response: ServerResponse,
 ): Promise<void> => {
   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(),