mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-26 18:31:34 +01:00
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
This commit is contained in:
1 parent
c281bd1705
commit
b2bf4599c3
2 files changed
+76
-2
No files matched your search
@@ -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,
|
||||
|
||||
@@ -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<Response> {
|
||||
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)) {
|
||||
|
||||
Reference in new issue
Block a user