diff --git a/server/src/__tests__/remote-http-rebinding.test.ts b/server/src/__tests__/remote-http-rebinding.test.ts index c4e5bf5e4c3..fc67679f621 100644 --- a/server/src/__tests__/remote-http-rebinding.test.ts +++ b/server/src/__tests__/remote-http-rebinding.test.ts @@ -1,5 +1,5 @@ import { createServer, type IncomingMessage, type Server, type ServerResponse } from "node:http"; -import { connect as netConnect, type AddressInfo, type Socket } from "node:net"; +import { connect as netConnect, createServer as netCreateServer, type AddressInfo, type Socket } from "node:net"; import { gzipSync } from "node:zlib"; import { afterEach, describe, expect, it } from "vitest"; @@ -79,18 +79,26 @@ function guardError(message: string, code: string) { function routingSocketFactory(routes: Record): { factory: RemoteHttpSocketFactory; dialled: string[]; + sockets: Socket[]; } { const dialled: string[] = []; + const sockets: Socket[] = []; const factory: RemoteHttpSocketFactory = (target) => { dialled.push(target.address); const port = routes[target.address]; if (port === undefined) throw new Error(`test network has no route to ${target.address}`); const socket = netConnect({ host: "127.0.0.1", port }); openSockets.push(socket); + sockets.push(socket); Object.defineProperty(socket, "remoteAddress", { get: () => target.address, configurable: true }); return socket; }; - return { factory, dialled }; + return { factory, dialled, sockets }; +} + +/** Waits for the socket bookkeeping the transport does after it rejects. */ +async function flush(): Promise { + await new Promise((resolve) => setTimeout(resolve, 50)); } /** A name server that answers public first and loopback afterwards. */ @@ -309,9 +317,45 @@ describe("guarded remote HTTP fetch (PAP-17098 DNS rebinding)", () => { socketFactory: network.factory, responseTimeoutMs: 150, error: guardError, - })).rejects.toThrow(/did not send response headers/); + })).rejects.toMatchObject({ code: "remote_http_response_timeout" }); + // The deadline has to hand back the socket, not just the request handler: + // a bounded call that still leaks a descriptor per silent peer is the same + // exhaustion bug wearing a timer. expect(upstream.requests).toHaveLength(1); + await flush(); + expect(network.sockets.map((socket) => socket.destroyed)).toEqual([true]); + }); + + it("gives up on a peer that dribbles response headers forever", async () => { + // This is why the headers deadline is a hard timer rather than + // `req.setTimeout`: an idle timeout never fires against this peer, because + // every trickled header line resets it. + const raw = netCreateServer((socket) => { + socket.write("HTTP/1.1 200 OK\r\n"); + const beat = setInterval(() => socket.write("x-pad: keepalive\r\n"), 25); + const stop = () => clearInterval(beat); + socket.on("close", stop); + socket.on("error", stop); + }); + await new Promise((resolve) => raw.listen(0, "127.0.0.1", resolve)); + const port = (raw.address() as AddressInfo).port; + const network = routingSocketFactory({ [PUBLIC_ADDRESS]: port }); + + try { + await expect(guardedRemoteHttpFetch(`http://${REBIND_HOST}/mcp`, {}, { + allowPrivateNetwork: false, + lookup: async () => [{ address: PUBLIC_ADDRESS, family: 4 }], + socketFactory: network.factory, + responseTimeoutMs: 150, + error: guardError, + })).rejects.toMatchObject({ code: "remote_http_response_timeout" }); + + await flush(); + expect(network.sockets.map((socket) => socket.destroyed)).toEqual([true]); + } finally { + await new Promise((resolve) => raw.close(() => resolve())); + } }); it("gives up on a response body that stalls midway", async () => { @@ -331,7 +375,64 @@ describe("guarded remote HTTP fetch (PAP-17098 DNS rebinding)", () => { }); expect(response.status).toBe(200); - await expect(response.text()).rejects.toThrow(); + await expect(response.text()).rejects.toMatchObject({ code: "remote_http_response_timeout" }); + await flush(); + expect(network.sockets.map((socket) => socket.destroyed)).toEqual([true]); + }); + + it("falls over to the next approved address when the first is unreachable", async () => { + // Pinning must not cost the failover `fetch` gave a multi-homed host: one + // dead A record should not take the connection down with it. + const upstream = await startServer(); + const dead = await startServer(); + // Close the listener so the address is routable but refuses connections. + await new Promise((resolve) => dead.server.close(() => resolve())); + const network = routingSocketFactory({ + "93.184.216.35": dead.port, + [PUBLIC_ADDRESS]: upstream.port, + }); + + const response = await guardedRemoteHttpFetch(`http://${REBIND_HOST}/mcp`, {}, { + allowPrivateNetwork: false, + lookup: async () => [ + { address: "93.184.216.35", family: 4 }, + { address: PUBLIC_ADDRESS, family: 4 }, + ], + socketFactory: network.factory, + error: guardError, + }); + + await expect(response.json()).resolves.toEqual({ ok: true }); + expect(network.dialled).toEqual(["93.184.216.35", PUBLIC_ADDRESS]); + }); + + it("fails closed instead of falling over when a peer fails the address check", async () => { + // Failover covers unreachable addresses only. A peer that answers from an + // unapproved address is the rebinding defence firing, and must not be + // retried past. + const internal = await startServer(); + const upstream = await startServer(); + const dialled: string[] = []; + const factory: RemoteHttpSocketFactory = (target) => { + dialled.push(target.address); + const socket = netConnect({ host: "127.0.0.1", port: internal.port }); + openSockets.push(socket); + return socket; + }; + + await expect(guardedRemoteHttpFetch(`http://${REBIND_HOST}/mcp`, {}, { + allowPrivateNetwork: false, + lookup: async () => [ + { address: "93.184.216.35", family: 4 }, + { address: PUBLIC_ADDRESS, family: 4 }, + ], + socketFactory: factory, + error: guardError, + })).rejects.toMatchObject({ code: "remote_http_private_endpoint" }); + + expect(dialled).toEqual(["93.184.216.35"]); + expect(internal.requests).toHaveLength(0); + expect(upstream.requests).toHaveLength(0); }); it("still refuses a private IP literal before any transport runs", async () => { diff --git a/server/src/services/remote-http-fetch.ts b/server/src/services/remote-http-fetch.ts index 68405359bb2..87bfae484c4 100644 --- a/server/src/services/remote-http-fetch.ts +++ b/server/src/services/remote-http-fetch.ts @@ -19,12 +19,19 @@ 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. + * Stands in for 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 (PAP-17110). + * + * Deliberately far tighter than undici's 300 s: everything that reaches this + * transport is metadata discovery, a token exchange, a DCR call or an MCP + * JSON-RPC round trip, none of which has any business taking minutes. Callers + * that own a longer budget — `tools/call`, which an operator can raise to 60 s — + * pass `responseTimeoutMs` so this default never truncates it. */ -const DEFAULT_RESPONSE_TIMEOUT_MS = 300_000; +const DEFAULT_RESPONSE_TIMEOUT_MS = 30_000; /** How a verified socket is opened. Overridable so tests can simulate a rebind. */ export type RemoteHttpSocketFactory = (target: { @@ -115,15 +122,7 @@ async function pinnedRequest( signal?.throwIfAborted?.(); - const socket = await openVerifiedSocket({ - address: approved[0]!, - approvedSet, - port, - hostname, - useTls, - signal, - options, - }); + const socket = await dialApprovedAddress({ approved, approvedSet, port, hostname, useTls, signal, options }); try { return await sendRequest({ @@ -135,6 +134,7 @@ async function pinnedRequest( init, signal, responseTimeoutMs: options.responseTimeoutMs ?? DEFAULT_RESPONSE_TIMEOUT_MS, + error: options.error, }); } catch (error) { socket.destroy(); @@ -142,6 +142,45 @@ async function pinnedRequest( } } +/** An approved address that could not be reached, so the next one may be tried. */ +class UnreachableAddressError extends Error { + constructor(readonly reason: unknown) { + super("Remote MCP approved address was unreachable"); + } +} + +/** + * Try the approved addresses in resolution order. + * + * `fetch` walks every A/AAAA record before giving up, so pinning to `approved[0]` + * alone would break a multi-homed host whose first record happens to be dead. A + * peer that fails the address check is a different matter — that is the rebinding + * defence firing, not a reachability problem — so it fails closed immediately + * rather than moving down the list. + */ +async function dialApprovedAddress(input: { + approved: string[]; + approvedSet: Set; + port: number; + hostname: string; + useTls: boolean; + signal: AbortSignal | null; + options: GuardedRemoteHttpFetchOptions; +}): Promise { + let lastReason: unknown; + for (const address of input.approved) { + input.signal?.throwIfAborted?.(); + try { + return await openVerifiedSocket({ ...input, address }); + } catch (error) { + if (!(error instanceof UnreachableAddressError)) throw error; + lastReason = error.reason; + } + } + throw lastReason + ?? input.options.error("Remote MCP endpoint could not be reached", "remote_http_connect_failed"); +} + /** * Connect to `address`, confirm the peer really is that approved address, and * only then complete the TLS handshake. Verifying before the handshake keeps @@ -163,7 +202,15 @@ async function openVerifiedSocket(input: { const raw = factory({ address, port, hostname, useTls }); - await once(raw, "connect", { signal, timeoutMs: connectTimeoutMs, what: "connect to" }); + try { + await once(raw, "connect", { signal, timeoutMs: connectTimeoutMs, what: "connect to" }); + } catch (error) { + // A connect timeout used to leave the half-open socket behind, because + // nothing above this point owns it yet. + raw.destroy(); + if (signal?.aborted) throw error; + throw new UnreachableAddressError(error); + } const peer = raw.remoteAddress ? normalizeIpAddress(raw.remoteAddress) : null; if (!peer || isPrivateOrReservedIp(peer) || !approvedSet.has(peer)) { @@ -188,7 +235,8 @@ async function openVerifiedSocket(input: { } catch (error) { secure.destroy(); raw.destroy(); - throw error; + if (signal?.aborted) throw error; + throw new UnreachableAddressError(error); } return secure; } @@ -202,8 +250,9 @@ async function sendRequest(input: { init: RequestInit; signal: AbortSignal | null; responseTimeoutMs: number; + error: RemoteHttpEndpointErrorFactory; }): Promise { - const { endpoint, hostname, port, useTls, socket, init, signal, responseTimeoutMs } = input; + const { endpoint, hostname, port, useTls, socket, init, signal, responseTimeoutMs, error } = input; const headers = new Headers(init.headers); const body = readRequestBody(init.body); const method = (init.method ?? "GET").toUpperCase(); @@ -237,7 +286,9 @@ async function sendRequest(input: { // 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")); + // Destroying the request tears the socket down too, so a silent peer costs + // neither a pending handler nor a leaked descriptor. + req.destroy(error("Remote MCP endpoint did not respond in time", "remote_http_response_timeout")); }, responseTimeoutMs); headersTimer.unref?.(); req.on("response", () => clearTimeout(headersTimer)); @@ -249,8 +300,13 @@ async function sendRequest(input: { // 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")); + message.destroy(error("Remote MCP endpoint stalled mid-response", "remote_http_response_timeout")); }); + // A finished response must not leave an armed socket timer behind, or a later + // reader of the same socket inherits a deadline it never asked for. + const disarm = () => message.setTimeout(0); + message.once("end", disarm); + message.once("close", disarm); const responseHeaders = new Headers(); for (const [key, value] of Object.entries(message.headers)) { diff --git a/server/src/services/tool-gateway.ts b/server/src/services/tool-gateway.ts index cd6577a4569..7a22e8c07f5 100644 --- a/server/src/services/tool-gateway.ts +++ b/server/src/services/tool-gateway.ts @@ -3107,7 +3107,13 @@ export function createToolGatewayService( arguments: parameters ?? {}, }, }), - }, remoteHttpFetchOptions()); + }, { + ...remoteHttpFetchOptions(), + // This call site owns a caller-set budget that can exceed the + // transport's default response deadline, so hand it down rather than + // letting the tighter default cut a legitimately slow tool short. + responseTimeoutMs: ms, + }); const body = await readBoundedRemoteResponse(response); execution.response = { httpStatus: response.status,