fix: make the proxy deadline per-request, not unconditional (#2319 round 13)
The server-side timer added in round 9 applied to every /api/fetch request. But that route carries MCP traffic as well as OAuth work, so a Streamable HTTP tool call that legitimately withholds its response headers past the budget was aborted there — reintroducing on the backend exactly the regression exemptMcpEndpoint prevents on the client, and reporting it as an OAuth timeout besides. The caller now decides. `withOAuthRequestTimeout` stamps its budget on the RequestInit it hands down, `createRemoteFetch` reads it back and puts it in the /api/fetch envelope, and the route applies a timer only when one is present — so an exempt MCP request, which never gets an init of its own, is bounded by nothing. The carrier is a WeakMap keyed on the init rather than a header: serializeRequest copies request headers verbatim into the payload and the route re-sends them to the authorization server, so a marker header would leak to a third party. It needs no cast onto RequestInit and is collected with the object. The `fetchTimeoutMs` option from round 9 is gone — the deadline now lives in exactly one place, the client wrapper, which is also what makes the 504 marker honest: only a request the OAuth wrapper bounded can produce one. Ten tests: the stamping and its rounding, nothing stamped on an exempt call or an init that never went through the wrapper, the envelope carrying the budget (and not a header), its omission on both exempt and direct calls, and at the route the 504 marker, no deadline without one, and a non-numeric deadline ignored. The no-deadline case is verified by mutation: an unconditional timer makes it fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
234d0a9ac4
commit
de6960ec5a
4 changed files with 259 additions and 50 deletions
|
|
@ -56,6 +56,7 @@ export {
|
|||
DEFAULT_OAUTH_REQUEST_TIMEOUT_MS,
|
||||
OAUTH_TIMEOUT_WIRE_CODE,
|
||||
OAuthRequestTimeoutError,
|
||||
deadlineForRequestInit,
|
||||
exemptMcpEndpoint,
|
||||
isOAuthRequestTimeoutWire,
|
||||
withOAuthRequestTimeout,
|
||||
|
|
|
|||
|
|
@ -148,6 +148,39 @@ export function exemptMcpEndpoint(
|
|||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* The deadline a wrapped call is running under, keyed by the exact `RequestInit`
|
||||
* the wrapper handed down (#2319).
|
||||
*
|
||||
* This is how the budget crosses the proxy hop without travelling *upstream*.
|
||||
* `/api/fetch` must not time every request it serves — `createRemoteFetch` also
|
||||
* carries MCP traffic, and a Streamable HTTP tool call can legitimately withhold
|
||||
* its response headers for minutes — so the route needs to know which requests
|
||||
* are bounded and which are not (Copilot). A header would be the obvious
|
||||
* carrier and is the wrong one: `serializeRequest` copies request headers
|
||||
* verbatim into the payload and the route re-sends them to the authorization
|
||||
* server, so a marker header would leak to a third party.
|
||||
*
|
||||
* A `WeakMap` keyed on the init object has neither problem. It is invisible to
|
||||
* serialization, it needs no cast onto `RequestInit`, and it is collected with
|
||||
* the object — the wrapper builds a fresh init per call, so there is one entry
|
||||
* per in-flight request and nothing accumulates.
|
||||
*/
|
||||
const REQUEST_DEADLINES = new WeakMap<object, number>();
|
||||
|
||||
/**
|
||||
* The deadline `withOAuthRequestTimeout` stamped on this init, if any.
|
||||
*
|
||||
* `undefined` means the request is not bounded by this wrapper and must not be
|
||||
* bounded by anything downstream either — an exempt MCP request reads as
|
||||
* `undefined` because the exempt path never builds an init of its own.
|
||||
*/
|
||||
export function deadlineForRequestInit(
|
||||
init: RequestInit | undefined,
|
||||
): number | undefined {
|
||||
return init ? REQUEST_DEADLINES.get(init) : undefined;
|
||||
}
|
||||
|
||||
/** The request URL, whatever form `fetch`'s first argument took. */
|
||||
function requestUrlOf(input: RequestInfo | URL): string {
|
||||
if (typeof input === "string") return input;
|
||||
|
|
@ -342,9 +375,14 @@ export function withOAuthRequestTimeout(
|
|||
}
|
||||
});
|
||||
|
||||
// Built once and stamped, so `createRemoteFetch` can read the budget off it
|
||||
// and tell `/api/fetch` this particular request is bounded.
|
||||
const nextInit: RequestInit = { ...init, signal };
|
||||
REQUEST_DEADLINES.set(nextInit, budget);
|
||||
|
||||
try {
|
||||
const response = await Promise.race([
|
||||
fetchFn(input, { ...init, signal }),
|
||||
fetchFn(input, nextInit),
|
||||
abandoned,
|
||||
]);
|
||||
// `fetch` resolves once the response *headers* arrive, so stopping here
|
||||
|
|
|
|||
|
|
@ -22,6 +22,7 @@
|
|||
*/
|
||||
|
||||
import {
|
||||
deadlineForRequestInit,
|
||||
isOAuthRequestTimeoutWire,
|
||||
OAuthRequestTimeoutError,
|
||||
} from "../../auth/requestTimeout.js";
|
||||
|
|
@ -154,10 +155,26 @@ export function createRemoteFetch(options: RemoteFetchOptions): typeof fetch {
|
|||
? input.signal
|
||||
: undefined;
|
||||
|
||||
// #2319: tell the route whether *this* request is bounded, and by how much.
|
||||
// The route serves MCP traffic as well as OAuth work, and a Streamable HTTP
|
||||
// tool call can legitimately withhold its response headers for minutes — so
|
||||
// a timer there must be per-request rather than unconditional (Copilot). An
|
||||
// exempt request carries no deadline and the route applies none.
|
||||
//
|
||||
// In the JSON envelope rather than a header: `headers` is re-sent verbatim
|
||||
// to the upstream server, so a marker header would leak to a third party.
|
||||
const timeoutMs = deadlineForRequestInit(init);
|
||||
|
||||
const res = await fetchFn(`${baseUrl}/api/fetch`, {
|
||||
method: "POST",
|
||||
headers: reqHeaders,
|
||||
body: JSON.stringify({ url, method, headers, body }),
|
||||
body: JSON.stringify({
|
||||
url,
|
||||
method,
|
||||
headers,
|
||||
body,
|
||||
...(timeoutMs !== undefined && { timeoutMs }),
|
||||
}),
|
||||
signal,
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -28,10 +28,7 @@ import { bodyLimit } from "hono/body-limit";
|
|||
import { watch as chokidarWatch, type FSWatcher } from "chokidar";
|
||||
import { createTransportNode } from "../../node/transport.js";
|
||||
import { createProxyFetch } from "../../node/proxyFetch.js";
|
||||
import {
|
||||
DEFAULT_OAUTH_REQUEST_TIMEOUT_MS,
|
||||
OAUTH_TIMEOUT_WIRE_CODE,
|
||||
} from "../../../auth/requestTimeout.js";
|
||||
import { OAUTH_TIMEOUT_WIRE_CODE } from "../../../auth/requestTimeout.js";
|
||||
import type {
|
||||
RemoteConnectRequest,
|
||||
RemoteSendRequest,
|
||||
|
|
@ -238,21 +235,6 @@ export interface RemoteServerOptions {
|
|||
/** Optional path for the user's server list file (/api/servers). Default: ~/.mcp-inspector/mcp.json */
|
||||
mcpConfigPath?: string;
|
||||
|
||||
/**
|
||||
* Deadline for the outbound request `/api/fetch` makes, in milliseconds
|
||||
* (#2319). Defaults to {@link DEFAULT_OAUTH_REQUEST_TIMEOUT_MS}, the same
|
||||
* budget the browser-side `withOAuthRequestTimeout` uses, so the two agree on
|
||||
* when an attempt is hopeless.
|
||||
*
|
||||
* This is the backstop for a client that vanishes without aborting; the
|
||||
* primary mechanism is the caller's own signal, which `createRemoteFetch`
|
||||
* forwards onto this hop. Exposed rather than hardcoded so a test can prove
|
||||
* the backstop actually fires without spending thirty seconds of wall clock
|
||||
* on it — and because a very slow authorization server is a legitimate reason
|
||||
* to want a longer one.
|
||||
*/
|
||||
fetchTimeoutMs?: number;
|
||||
|
||||
/**
|
||||
* When false, the server list is read-only for this session: all
|
||||
* `/api/servers` mutations (POST/PUT/DELETE/order) are rejected with 403,
|
||||
|
|
@ -588,10 +570,6 @@ export function createRemoteApp(
|
|||
options: RemoteServerOptions,
|
||||
): CreateRemoteAppResult {
|
||||
const dangerouslyOmitAuth = !!options.dangerouslyOmitAuth;
|
||||
const fetchTimeoutMs = Math.max(
|
||||
0,
|
||||
Math.round(options.fetchTimeoutMs ?? DEFAULT_OAUTH_REQUEST_TIMEOUT_MS),
|
||||
);
|
||||
|
||||
// Determine auth token when auth is enabled: options > env var > generate
|
||||
const authToken = dangerouslyOmitAuth
|
||||
|
|
@ -1227,6 +1205,8 @@ export function createRemoteApp(
|
|||
method?: string;
|
||||
headers?: Record<string, string>;
|
||||
body?: string;
|
||||
/** Per-request deadline, set only by a bounded OAuth call (#2319). */
|
||||
timeoutMs?: number;
|
||||
};
|
||||
try {
|
||||
body = (await c.req.json()) as typeof body;
|
||||
|
|
@ -1234,7 +1214,13 @@ export function createRemoteApp(
|
|||
return c.json({ error: "Invalid JSON body" }, 400);
|
||||
}
|
||||
|
||||
const { url, method = "GET", headers = {}, body: reqBody } = body;
|
||||
const {
|
||||
url,
|
||||
method = "GET",
|
||||
headers = {},
|
||||
body: reqBody,
|
||||
timeoutMs: requestedTimeoutMs,
|
||||
} = body;
|
||||
if (!url) {
|
||||
return c.json({ error: "Missing url" }, 400);
|
||||
}
|
||||
|
|
@ -1262,14 +1248,30 @@ export function createRemoteApp(
|
|||
// the body has been either read or explicitly cancelled — this route never
|
||||
// hands a live stream to its caller, so there is nothing left for the timer
|
||||
// to protect and nothing it could sever.
|
||||
const timer = setTimeout(() => {
|
||||
deadlineFired = true;
|
||||
controller.abort(
|
||||
new Error(
|
||||
`proxied request to ${url} timed out after ${fetchTimeoutMs}ms`,
|
||||
),
|
||||
);
|
||||
}, fetchTimeoutMs);
|
||||
// ⚠️ Only the caller decides whether this request is bounded, and by how
|
||||
// much. This route is the browser's way out to the network for MCP traffic
|
||||
// as well as OAuth work, so an unconditional timer here would abort a
|
||||
// Streamable HTTP tool call that legitimately withholds its response
|
||||
// headers for longer than the budget — reintroducing on the backend exactly
|
||||
// the regression `exemptMcpEndpoint` exists to prevent on the client, and
|
||||
// reporting it as an OAuth timeout besides (Copilot). No deadline in the
|
||||
// envelope means no timer at all.
|
||||
const deadlineMs =
|
||||
typeof requestedTimeoutMs === "number" &&
|
||||
Number.isFinite(requestedTimeoutMs)
|
||||
? Math.max(0, Math.round(requestedTimeoutMs))
|
||||
: undefined;
|
||||
const timer =
|
||||
deadlineMs === undefined
|
||||
? undefined
|
||||
: setTimeout(() => {
|
||||
deadlineFired = true;
|
||||
controller.abort(
|
||||
new Error(
|
||||
`proxied request to ${url} timed out after ${deadlineMs}ms`,
|
||||
),
|
||||
);
|
||||
}, deadlineMs);
|
||||
|
||||
try {
|
||||
// Proxy-aware, not the bare global. This route is the browser's ONLY way
|
||||
|
|
@ -1332,14 +1334,14 @@ export function createRemoteApp(
|
|||
error: msg,
|
||||
code: OAUTH_TIMEOUT_WIRE_CODE,
|
||||
url,
|
||||
timeoutMs: fetchTimeoutMs,
|
||||
timeoutMs: deadlineMs,
|
||||
},
|
||||
504,
|
||||
);
|
||||
}
|
||||
return c.json({ error: msg }, 500);
|
||||
} finally {
|
||||
clearTimeout(timer);
|
||||
if (timer !== undefined) clearTimeout(timer);
|
||||
}
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue