From 030632454df3352cec528def9af27f134b770352 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 31 Jul 2026 04:51:09 -0700 Subject: [PATCH] fix(mcp): read the initialize handshake before the transport consumes the body The #10175 handshake telemetry called `c.req.raw.clone()` after createMcpHandler had already read the request body. The Fetch spec forbids cloning a request whose body is disturbed, so it threw `TypeError: unusable`; handleMcpRequest's catch rethrew, and a correct 2xx MCP response was discarded in favour of an unhandled 500 -- on every `initialize`, the first call of every MCP session. The telemetry key gate did not contain it: the handshake read is an argument to recordMcpInitialize, evaluated before that function's own no-op check, so the throw happened whether or not POSTHOG_API_KEY was set. Parse the JSON-RPC envelope once, before the handler runs, and derive both the usage metadata and the clientInfo handshake from it. No clone survives past the handler. Closes #10190 --- src/mcp/server.ts | 38 ++++++++++++++++----- test/unit/mcp-server-telemetry.test.ts | 47 ++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 9 deletions(-) diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 2d956990b5..2e04af022f 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -665,7 +665,12 @@ export async function handleMcpRequest(c: AppContext): Promise { if (!identity) return c.json({ error: "unauthorized" }, 401); const telemetry = buildMcpClientTelemetry(c.req.raw.headers, { defaultClientName: "mcp" })!; - const usageMetadata = await describeMcpUsageRequest(c.req.raw, telemetry.metadata); + // ONE clone-and-parse of the JSON-RPC body, here, BEFORE createMcpHandler below consumes it (#10190). + // A second `request.clone()` after that point throws `TypeError: unusable` -- the Fetch spec forbids + // cloning a request whose body is already disturbed -- which is why the post-response handshake read this + // replaces turned every `initialize` into an unhandled 500. + const envelope = await readMcpRequestEnvelope(c.req.raw); + const usageMetadata = describeMcpUsageRequest(envelope, c.req.raw.method, telemetry.metadata); const startedAt = Date.now(); const executionCtx = getExecutionContext(c); // #9525: the dispatch chokepoint's sink is built per request so its deferred work rides this @@ -693,7 +698,7 @@ export async function handleMcpRequest(c: AppContext): Promise { // a rejected handshake never inflates the session/client counts. if (response.status < 400) { if (usageMetadata.rpcMethod === "initialize") { - recordMcpInitialize(c.env, defer, await readInitializeHandshake(c.req.raw), analyticsContext); + recordMcpInitialize(c.env, defer, readInitializeHandshake(envelope), analyticsContext); } else if (usageMetadata.rpcMethod === "tools/list") { // Names come from this server's own registration chokepoint, not the cross-server contract // registry, so the event reports what was actually advertised to THIS client. @@ -765,20 +770,35 @@ function trimmedHeader(value: string | null): string | undefined { * client. Returns an empty handshake for a malformed or absent body: the fields are optional by * contract, and a session that connected is still worth counting. */ -async function readInitializeHandshake(request: Request): Promise { - const body = await request.clone().json().catch(() => null); - if (!body || typeof body !== "object") return {}; - const clientInfo = (body as { params?: { clientInfo?: { name?: unknown; version?: unknown } } }).params?.clientInfo; +function readInitializeHandshake(envelope: McpRequestEnvelope | null): McpInitializeTelemetry { + const clientInfo = envelope?.params?.clientInfo; return { clientName: typeof clientInfo?.name === "string" ? clientInfo.name : undefined, clientVersion: typeof clientInfo?.version === "string" ? clientInfo.version : undefined, }; } -async function describeMcpUsageRequest(request: Request, telemetryMetadata: Record | undefined): Promise> { +/** The JSON-RPC envelope fields the telemetry paths read. Deliberately structural and permissive: this is an + * unvalidated client body, and every consumer below re-checks the type of the field it uses. */ +type McpRequestEnvelope = { + method?: unknown; + params?: { name?: unknown; clientInfo?: { name?: unknown; version?: unknown } }; +}; + +/** Clone-and-parse the request body exactly once, at the top of {@link handleMcpRequest} (#10190). Returns + * null for an absent or malformed body -- the telemetry fields are all optional by contract, and a request + * that is still worth counting must never be failed over its own instrumentation. */ +async function readMcpRequestEnvelope(request: Request): Promise { const body = await request.clone().json().catch(() => null); - if (!body || typeof body !== "object") return { transport: "http", method: request.method, ...telemetryMetadata }; - const envelope = body as { method?: unknown; params?: { name?: unknown } }; + return body && typeof body === "object" ? (body as McpRequestEnvelope) : null; +} + +function describeMcpUsageRequest( + envelope: McpRequestEnvelope | null, + method: string, + telemetryMetadata: Record | undefined, +): Record { + if (!envelope) return { transport: "http", method, ...telemetryMetadata }; const rpcMethod = typeof envelope.method === "string" ? envelope.method : undefined; const toolName = envelope.params && typeof envelope.params.name === "string" ? envelope.params.name : undefined; return { diff --git a/test/unit/mcp-server-telemetry.test.ts b/test/unit/mcp-server-telemetry.test.ts index 449ad65cc0..e01833d88e 100644 --- a/test/unit/mcp-server-telemetry.test.ts +++ b/test/unit/mcp-server-telemetry.test.ts @@ -273,4 +273,51 @@ describe("MCP server telemetry", () => { expect(response.status).toBe(200); await expect(response.clone().json()).resolves.toEqual({ ok: true, result: "unchanged" }); }); + + // REGRESSION (#10190): the #10175 handshake read called `c.req.raw.clone()` AFTER createMcpHandler had + // consumed the body. The Fetch spec forbids cloning a request whose body is already disturbed, so it threw + // `TypeError: unusable`, handleMcpRequest's catch rethrew, and a correct 2xx MCP response was replaced by an + // unhandled 500 -- on every `initialize`, i.e. the first call of every MCP session. The telemetry key gate + // did not save it: the handshake read is an ARGUMENT to recordMcpInitialize, evaluated before its no-op check. + const runInitialize = async (params: unknown): Promise => { + vi.resetModules(); + // A handler that CONSUMES the request body, exactly as the real MCP transport does -- the whole point of + // the regression. A handler that ignored the body would pass even with the bug reintroduced. + vi.doMock("agents/mcp", () => ({ + createMcpHandler: () => async (request: Request) => { + await request.text(); + return Response.json({ jsonrpc: "2.0", id: "init", result: { protocolVersion: "2024-11-05" } }); + }, + })); + const { handleMcpRequest } = await import("../../src/mcp/server"); + const env = createTestEnv({ PRODUCT_USAGE_HASH_SALT: "mcp-initialize-clone-salt" }); + const request = new Request("https://api.test/mcp", { + method: "POST", + headers: { authorization: `Bearer ${env.LOOPOVER_MCP_TOKEN}`, "content-type": "application/json" }, + body: JSON.stringify({ jsonrpc: "2.0", id: "init", method: "initialize", ...(params === undefined ? {} : { params }) }), + }); + return handleMcpRequest({ + env, + executionCtx: { waitUntil() {}, passThroughOnException() {} }, + req: { method: "POST", raw: request, header: (name: string) => request.headers.get(name) ?? undefined }, + json: (body: unknown, status?: number) => Response.json(body, status === undefined ? undefined : { status }), + } as never); + }; + + it("REGRESSION (#10190): an initialize whose body the handler consumed still returns the handler's response, not a 500", async () => { + const response = await runInitialize({ + protocolVersion: "2024-11-05", + capabilities: {}, + clientInfo: { name: "claude-code", version: "2.1.0" }, + }); + expect(response.status).toBe(200); + await expect(response.clone().json()).resolves.toMatchObject({ result: { protocolVersion: "2024-11-05" } }); + }); + + it("REGRESSION (#10190): an initialize with no params, and one whose clientInfo fields are not strings, still succeed", async () => { + // The handshake fields are optional by contract -- a client that omits them must not be failed over + // LoopOver's own instrumentation. + await expect(runInitialize(undefined)).resolves.toMatchObject({ status: 200 }); + await expect(runInitialize({ clientInfo: { name: 42, version: null } })).resolves.toMatchObject({ status: 200 }); + }); });