From b2bf4599c3890ea2cd480d8837e80b4d218ae3c7 Mon Sep 17 00:00:00 2001 From: Dotta Date: Tue, 11 Aug 2026 20:32:50 +0000 Subject: [PATCH] fix(server): give the pinned remote MCP transport a response deadline The pinned transport replaced platform fetch, and with it undici's headersTimeout/bodyTimeout. Only the gateway tools/call path passes an AbortSignal; the OAuth discovery, registration and token-exchange callers pass none, so a remote server that accepted the connection and then stayed silent would hold the request open indefinitely. That is a hang the code this replaced did not have. Adds a 300s default matching undici, applied as a hard headers deadline (not req.setTimeout, which is socket-idle based and a dribbling server could reset forever) plus an idle deadline between body chunks so a half-sent body rejects in response.text() instead of hanging the caller. Both new tests hang to the vitest timeout when the deadlines are removed. Refs PAP-17098 --- .../__tests__/remote-http-rebinding.test.ts | 40 +++++++++++++++++++ server/src/services/remote-http-fetch.ts | 38 +++++++++++++++++- 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/remote-http-rebinding.test.ts b/server/src/__tests__/remote-http-rebinding.test.ts index 7910e622919..c4e5bf5e4c3 100644 --- a/server/src/__tests__/remote-http-rebinding.test.ts +++ b/server/src/__tests__/remote-http-rebinding.test.ts @@ -294,6 +294,46 @@ describe("guarded remote HTTP fetch (PAP-17098 DNS rebinding)", () => { expect(response.headers.get("content-encoding")).toBeNull(); }); + it("gives up on a server that accepts the connection and never answers", async () => { + // Platform `fetch` applied undici's headersTimeout for free. The OAuth + // callers pass no AbortSignal, so the pinned transport has to own the + // deadline or a silent server would hold the request open forever. + const upstream = await startServer(() => { + /* accept the request and never respond */ + }); + const network = routingSocketFactory({ [PUBLIC_ADDRESS]: upstream.port }); + + await expect(guardedRemoteHttpFetch(`http://${REBIND_HOST}/mcp`, {}, { + allowPrivateNetwork: false, + lookup: async () => [{ address: PUBLIC_ADDRESS, family: 4 }], + socketFactory: network.factory, + responseTimeoutMs: 150, + error: guardError, + })).rejects.toThrow(/did not send response headers/); + + expect(upstream.requests).toHaveLength(1); + }); + + it("gives up on a response body that stalls midway", async () => { + const upstream = await startServer((_req, res) => { + res.writeHead(200, { "content-type": "application/json" }); + res.write('{"partial":'); + // never finishes the body + }); + const network = routingSocketFactory({ [PUBLIC_ADDRESS]: upstream.port }); + + const response = await guardedRemoteHttpFetch(`http://${REBIND_HOST}/mcp`, {}, { + allowPrivateNetwork: false, + lookup: async () => [{ address: PUBLIC_ADDRESS, family: 4 }], + socketFactory: network.factory, + responseTimeoutMs: 150, + error: guardError, + }); + + expect(response.status).toBe(200); + await expect(response.text()).rejects.toThrow(); + }); + it("still refuses a private IP literal before any transport runs", async () => { await expect(guardedRemoteHttpFetch("http://169.254.169.254/latest/meta-data/", {}, { allowPrivateNetwork: false, diff --git a/server/src/services/remote-http-fetch.ts b/server/src/services/remote-http-fetch.ts index 18883ad8cf6..68405359bb2 100644 --- a/server/src/services/remote-http-fetch.ts +++ b/server/src/services/remote-http-fetch.ts @@ -18,6 +18,14 @@ const NULL_BODY_STATUSES = new Set([101, 103, 204, 205, 304]); const DEFAULT_CONNECT_TIMEOUT_MS = 10_000; +/** + * Matches undici's `headersTimeout`/`bodyTimeout`, which the platform `fetch` + * applied for free. Without it a remote server that accepts the connection and + * then stays silent would hold the request open indefinitely — the OAuth callers + * pass no `AbortSignal`, so nothing else would ever cut it loose. + */ +const DEFAULT_RESPONSE_TIMEOUT_MS = 300_000; + /** How a verified socket is opened. Overridable so tests can simulate a rebind. */ export type RemoteHttpSocketFactory = (target: { address: string; @@ -37,6 +45,8 @@ export type GuardedRemoteHttpFetchOptions = RemoteHttpEndpointGuardOptions & { */ socketFactory?: RemoteHttpSocketFactory; connectTimeoutMs?: number; + /** Deadline for response headers, and idle deadline between body chunks. */ + responseTimeoutMs?: number; /** * Platform `fetch`, used only when the deployment allows private endpoints and * there is therefore no egress boundary to pin against. @@ -116,7 +126,16 @@ async function pinnedRequest( }); try { - return await sendRequest({ endpoint, hostname, port, useTls, socket, init, signal }); + return await sendRequest({ + endpoint, + hostname, + port, + useTls, + socket, + init, + signal, + responseTimeoutMs: options.responseTimeoutMs ?? DEFAULT_RESPONSE_TIMEOUT_MS, + }); } catch (error) { socket.destroy(); throw error; @@ -182,8 +201,9 @@ async function sendRequest(input: { socket: Socket; init: RequestInit; signal: AbortSignal | null; + responseTimeoutMs: number; }): Promise { - const { endpoint, hostname, port, useTls, socket, init, signal } = input; + const { endpoint, hostname, port, useTls, socket, init, signal, responseTimeoutMs } = input; const headers = new Headers(init.headers); const body = readRequestBody(init.body); const method = (init.method ?? "GET").toUpperCase(); @@ -214,10 +234,24 @@ async function sendRequest(input: { ...(signal ? { signal } : {}), }, resolve); req.on("error", reject); + // Headers deadline. `req.setTimeout` is socket-idle based, which a server + // that dribbles bytes could reset forever, so hold a hard timer instead. + const headersTimer = setTimeout(() => { + req.destroy(new Error("Remote MCP endpoint did not send response headers in time")); + }, responseTimeoutMs); + headersTimer.unref?.(); + req.on("response", () => clearTimeout(headersTimer)); + req.on("error", () => clearTimeout(headersTimer)); if (body !== undefined) req.write(body); req.end(); }); + // Body idle deadline, mirroring undici's `bodyTimeout`: a stalled stream is + // destroyed so `response.text()` rejects instead of hanging the caller. + message.setTimeout(responseTimeoutMs, () => { + message.destroy(new Error("Remote MCP endpoint stalled while sending its response body")); + }); + const responseHeaders = new Headers(); for (const [key, value] of Object.entries(message.headers)) { if (Array.isArray(value)) {