diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 0738add7..dbd89bcc 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -25,6 +25,7 @@ import { describe, it, expect, vi, afterEach } from "vitest"; import { DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, OAuthRequestTimeoutError, + deadlineForRequestInit, exemptMcpEndpoint, withOAuthRequestTimeout, } from "@inspector/core/auth/requestTimeout.js"; @@ -414,6 +415,46 @@ describe("withOAuthRequestTimeout", () => { }); }); + describe("the deadline stamped on the init (#2319 proxy hop)", () => { + // How the budget reaches `/api/fetch` without travelling upstream: a header + // would be copied verbatim into the payload and re-sent to the + // authorization server, so the carrier is a WeakMap keyed on the init. + it("stamps the budget on the init it hands down", async () => { + const inner = vi.fn().mockResolvedValue(new Response("{}")); + const wrapped = withOAuthRequestTimeout(inner, 1234); + + await wrapped(URL_UNDER_TEST); + + expect(deadlineForRequestInit(inner.mock.calls[0][1])).toBe(1234); + }); + + it("stamps the rounded budget, matching what is reported", async () => { + const inner = vi.fn().mockResolvedValue(new Response("{}")); + const wrapped = withOAuthRequestTimeout(inner, 1000.4); + + await wrapped(URL_UNDER_TEST); + + expect(deadlineForRequestInit(inner.mock.calls[0][1])).toBe(1000); + }); + + it("stamps nothing on an exempt request", async () => { + // The route must apply no deadline to MCP traffic, and it decides that + // from the absence of a budget in the envelope. + const inner = vi.fn().mockResolvedValue(new Response("{}")); + const wrapped = withOAuthRequestTimeout(inner, 1234, () => true); + + await wrapped(URL_UNDER_TEST, { method: "POST" }); + + expect(deadlineForRequestInit(inner.mock.calls[0][1])).toBeUndefined(); + }); + + it("reads nothing off an init that never went through the wrapper", () => { + expect(deadlineForRequestInit(undefined)).toBeUndefined(); + expect(deadlineForRequestInit({})).toBeUndefined(); + expect(deadlineForRequestInit({ method: "GET" })).toBeUndefined(); + }); + }); + describe("exemptMcpEndpoint (the transport chain's mixed traffic)", () => { const SERVER = "https://srv.example.com/mcp"; diff --git a/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts b/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts index 0eaa397b..37e71d9d 100644 --- a/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts +++ b/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts @@ -3,6 +3,7 @@ import { createRemoteFetch } from "@inspector/core/mcp/remote/createRemoteFetch. import { OAUTH_TIMEOUT_WIRE_CODE, OAuthRequestTimeoutError, + withOAuthRequestTimeout, } from "@inspector/core/auth/requestTimeout.js"; function ok(body: object): Response { @@ -334,4 +335,61 @@ describe("createRemoteFetch", () => { expect(err).not.toBeInstanceOf(OAuthRequestTimeoutError); }); }); + + describe("carrying the caller's deadline to the route (#2319)", () => { + function envelopeOf(fetchFn: ReturnType>) { + const init = fetchFn.mock.calls[0][1] as RequestInit; + return JSON.parse(init.body as string) as Record; + } + + it("puts a bounded call's budget in the envelope, not in the headers", async () => { + const fetchFn = vi + .fn() + .mockResolvedValue(ok(standardRemoteBody)); + const remoteFetch = createRemoteFetch({ + baseUrl: "http://remote.example", + fetchFn: (input, init) => fetchFn(input, init), + }); + // Composed the way `InspectorClient` composes it: the wrapper outside, + // the remote fetch beneath. + const bounded = withOAuthRequestTimeout(remoteFetch, 7000); + + await bounded("http://upstream.example/token"); + + const envelope = envelopeOf(fetchFn); + expect(envelope.timeoutMs).toBe(7000); + // Not a header: `headers` is re-sent verbatim to the upstream server, so + // a marker there would leak the Inspector's internals to a third party. + expect(JSON.stringify(envelope.headers)).not.toContain("7000"); + }); + + it("omits the budget for an exempt call, so the route bounds nothing", async () => { + const fetchFn = vi + .fn() + .mockResolvedValue(ok(standardRemoteBody)); + const remoteFetch = createRemoteFetch({ + baseUrl: "http://remote.example", + fetchFn: (input, init) => fetchFn(input, init), + }); + const bounded = withOAuthRequestTimeout(remoteFetch, 7000, () => true); + + await bounded("http://upstream.example/mcp"); + + expect(envelopeOf(fetchFn)).not.toHaveProperty("timeoutMs"); + }); + + it("omits the budget for a direct call that no wrapper bounded", async () => { + const fetchFn = vi + .fn() + .mockResolvedValue(ok(standardRemoteBody)); + const remoteFetch = createRemoteFetch({ + baseUrl: "http://remote.example", + fetchFn: (input, init) => fetchFn(input, init), + }); + + await remoteFetch("http://upstream.example/mcp"); + + expect(envelopeOf(fetchFn)).not.toHaveProperty("timeoutMs"); + }); + }); }); diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index 9613d2af..09b756e8 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -51,7 +51,6 @@ interface StartOpts { secretStore?: SecretStore; logger?: pinoType.Logger; seedConfig?: string; - fetchTimeoutMs?: number; } async function start(opts: StartOpts = {}): Promise { @@ -66,9 +65,6 @@ async function start(opts: StartOpts = {}): Promise { initialConfig: { defaultEnvironment: {} }, secretStore: opts.secretStore ?? new InMemorySecretStore(), logger: opts.logger, - ...(opts.fetchTimeoutMs !== undefined && { - fetchTimeoutMs: opts.fetchTimeoutMs, - }), }); const { baseUrl, server } = await new Promise<{ baseUrl: string; @@ -475,19 +471,16 @@ describe("server.ts supplemental coverage", () => { await openStreamClosed; }); - it("answers 504 with a typed marker when its own deadline fires (#2319)", async () => { - // The backstop, on its own short budget so this costs milliseconds rather - // than the production thirty seconds. It matters because this is the only - // deadline on the transport-internal discovery path, which has no - // client-side wrapper — so its error has to survive the hop as something - // an `instanceof OAuthRequestTimeoutError` check can still recognize. - await stop(h); - h = await start({ fetchTimeoutMs: 120 }); - + it("answers 504 with a typed marker when the request's deadline fires (#2319)", async () => { + // The caller's budget travels in the envelope, so this costs milliseconds + // rather than the production thirty seconds. The marker matters because + // a backend-enforced deadline is the only one on the transport-internal + // discovery path, and its error has to survive the hop as something an + // `instanceof OAuthRequestTimeoutError` check can still recognize. const res = await fetch(`${h.baseUrl}/api/fetch`, { method: "POST", headers: { "content-type": "application/json" }, - body: JSON.stringify({ url: `${targetUrl}/stall` }), + body: JSON.stringify({ url: `${targetUrl}/stall`, timeoutMs: 120 }), }); expect(res.status).toBe(504); @@ -505,6 +498,65 @@ describe("server.ts supplemental coverage", () => { await stallClosed; }); + it("applies no deadline to a request that carries none (#2319)", async () => { + // ⚠️ The regression this guards: an unconditional timer here would abort + // a Streamable HTTP tool call that legitimately withholds its response + // headers — reintroducing on the backend exactly what `exemptMcpEndpoint` + // prevents on the client, and reporting it as an OAuth timeout besides. + const caller = new AbortController(); + const pending = fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: `${targetUrl}/stall` }), + signal: caller.signal, + }); + const settled = pending.then( + (r) => `responded ${r.status}`, + () => "aborted by us", + ); + + await vi.waitFor(() => { + expect(upstreamHits).toContain("/stall"); + }); + // Well past any budget the route might have applied on its own. Nothing + // should have answered: an unbounded request is still in flight. + await new Promise((r) => setTimeout(r, 400)); + expect( + await Promise.race([settled, Promise.resolve("still pending")]), + ).toBe("still pending"); + + caller.abort(); + expect(await settled).toBe("aborted by us"); + }); + + it("ignores a non-numeric deadline rather than trusting it (#2319)", async () => { + const caller = new AbortController(); + const pending = fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + url: `${targetUrl}/stall`, + timeoutMs: "soon", + }), + signal: caller.signal, + }); + const settled = pending.then( + (r) => `responded ${r.status}`, + () => "aborted by us", + ); + + await vi.waitFor(() => { + expect(upstreamHits).toContain("/stall"); + }); + await new Promise((r) => setTimeout(r, 300)); + expect( + await Promise.race([settled, Promise.resolve("still pending")]), + ).toBe("still pending"); + + caller.abort(); + expect(await settled).toBe("aborted by us"); + }); + it("routes the outbound request through HTTP_PROXY (#2067)", async () => { // /api/fetch is the browser's ONLY way out to the network — the web // client's `environment.fetch` is `createRemoteFetch()`, which forwards diff --git a/core/auth/index.ts b/core/auth/index.ts index e38812eb..918eaac3 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -56,6 +56,7 @@ export { DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, OAUTH_TIMEOUT_WIRE_CODE, OAuthRequestTimeoutError, + deadlineForRequestInit, exemptMcpEndpoint, isOAuthRequestTimeoutWire, withOAuthRequestTimeout, diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 1f1df46c..d74bff90 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -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(); + +/** + * 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 diff --git a/core/mcp/remote/createRemoteFetch.ts b/core/mcp/remote/createRemoteFetch.ts index bd54db8c..144f6681 100644 --- a/core/mcp/remote/createRemoteFetch.ts +++ b/core/mcp/remote/createRemoteFetch.ts @@ -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, }); diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 3e8a121b..b584bb1c 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -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; 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); } });