diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 28ccd0dc..990267d0 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -53,6 +53,7 @@ import { getAuthorizationServerUrlCandidates, } from "@inspector/core/auth/discovery.js"; import { withRfc8414OidcCompat } from "@inspector/core/auth/oidcDiscoveryCompat.js"; +import { withOAuthRequestTimeout } from "@inspector/core/auth/requestTimeout.js"; import { writeStoreFile } from "@inspector/core/storage/store-io.js"; import { refreshAuthorization, @@ -353,7 +354,14 @@ export async function refreshStoredAuthToken( // else puts a proxy under it, and a server reachable only through // `HTTPS_PROXY` would otherwise be probed directly. The same fetch is handed // to the token request below, so neither leg bypasses the proxy (Copilot). - const storedAuthFetch = withRfc8414OidcCompat(createProxyFetch() ?? fetch); + // #2319: this path runs outside `InspectorClient`, so nothing else bounds it + // — the discovery and the token request below would otherwise hang forever + // against an authorization server that accepts the connection and never + // answers. Innermost, so the compat wrapper's own probe requests inherit the + // deadline too. + const storedAuthFetch = withRfc8414OidcCompat( + withOAuthRequestTimeout(createProxyFetch() ?? fetch), + ); const discover: typeof discoverAuthorizationServerMetadata = deps.discover ?? ((authorizationServerUrl, options) => diff --git a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts index e3bf9d4d..c9ea94b9 100644 --- a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts +++ b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts @@ -439,3 +439,186 @@ describe("withRfc8414OidcCompat", () => { expect(base).toHaveBeenCalledTimes(2); }); }); + +describe("probe cancellation (#2319)", () => { + const RFC8414 = + "https://as.example.com/.well-known/oauth-authorization-server/tenant"; + + /** 404 on the RFC 8414 leg, then never answers the OIDC probe. */ + function stallingProbeFetch() { + return vi.fn((input, init) => { + if (String(input) === RFC8414) { + return Promise.resolve(new Response(null, { status: 404 })); + } + return new Promise((_resolve, reject) => { + // A real `fetch` rejects immediately on a signal that is already + // aborted, so the double is only useful if it does the same. + if (init?.signal?.aborted) { + reject(init.signal.reason as Error); + return; + } + init?.signal?.addEventListener("abort", () => + reject(init.signal?.reason as Error), + ); + }); + }); + } + + it("carries the caller's signal into the probe", async () => { + const inner = stallingProbeFetch(); + const wrapped = withRfc8414OidcCompat(inner); + const caller = new AbortController(); + + const settled = wrapped(RFC8414, { signal: caller.signal }).then( + (r) => r, + (e: unknown) => e, + ); + // Wait until the probe is actually in flight, so the abort exercises + // cancellation rather than the pre-aborted short-circuit. + await vi.waitFor(() => { + expect(inner).toHaveBeenCalledTimes(2); + }); + const reason = new Error("caller gave up"); + caller.abort(reason); + + // Not the preceding 404: substituting it would report a discovery failure + // for a request the caller deliberately cancelled. + expect(await settled).toBe(reason); + expect((inner.mock.calls[1][1] as RequestInit).signal).toBe(caller.signal); + }); + + it("does not issue a probe the caller already cancelled", async () => { + const inner = stallingProbeFetch(); + const wrapped = withRfc8414OidcCompat(inner); + const reason = new Error("already gone"); + + const settled = wrapped(RFC8414, { + signal: AbortSignal.abort(reason), + }).then( + (r) => r, + (e: unknown) => e, + ); + + expect(await settled).toBe(reason); + // Only the RFC 8414 leg, which the caller did make. + expect(inner).toHaveBeenCalledTimes(1); + }); + + it("rethrows an abort that lands while the probe body is pending", async () => { + // `fetch` resolves on headers, so a late abort rejects `probe.text()` + // rather than the fetch — a separate catch, which used to answer with the + // preceding 404. + const caller = new AbortController(); + const inner = vi.fn((input, init) => { + if (String(input) === RFC8414) { + return Promise.resolve(new Response(null, { status: 404 })); + } + // Headers now, body never. + return Promise.resolve( + new Response( + new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode('{"issuer":')); + init?.signal?.addEventListener("abort", () => + ctrl.error(init.signal?.reason as Error), + ); + }, + }), + { headers: { "content-type": "application/json" } }, + ), + ); + }); + const wrapped = withRfc8414OidcCompat(inner); + + const settled = wrapped(RFC8414, { signal: caller.signal }).then( + (r) => r, + (e: unknown) => e, + ); + await vi.waitFor(() => { + expect(inner).toHaveBeenCalledTimes(2); + }); + const reason = new Error("caller gave up mid-body"); + caller.abort(reason); + + expect(await settled).toBe(reason); + }); + + it("releases the discarded discovery response on an exceptional exit", async () => { + // Nobody receives `response` when the loop throws, and an unread body holds + // its connection open on Node/undici — the same reason the successful + // substitution path releases it. + const cancels = vi.fn<() => Promise>().mockResolvedValue(undefined); + const discovery = new Response( + new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode("not found")); + }, + }), + { status: 404 }, + ); + // Spy on the body's cancel, which is what `releaseBody` calls. + const body = discovery.body; + if (body) vi.spyOn(body, "cancel").mockImplementation(cancels); + + const inner = vi.fn((input) => + String(input) === RFC8414 + ? Promise.resolve(discovery) + : new Promise(() => {}), + ); + const wrapped = withRfc8414OidcCompat(inner); + + const settled = wrapped(RFC8414, { + signal: AbortSignal.abort(new Error("gone")), + }).then( + () => "resolved", + () => "threw", + ); + + expect(await settled).toBe("threw"); + expect(cancels).toHaveBeenCalled(); + }); + + it("does not wait on a cancel that never settles", async () => { + // ⚠️ `ReadableStream.cancel()` adopts the underlying source's cancel + // promise, which is permitted never to settle — so awaiting the release on + // a path that is propagating a cancellation would hang the very thing it + // was meant to end. + const discovery = new Response( + new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode("not found")); + }, + cancel() { + return new Promise(() => {}); + }, + }), + { status: 404 }, + ); + const inner = vi.fn((input) => + String(input) === RFC8414 + ? Promise.resolve(discovery) + : new Promise(() => {}), + ); + const wrapped = withRfc8414OidcCompat(inner); + const reason = new Error("gone"); + + // Would hang if the release were awaited; the assertion is that it settles. + await expect( + wrapped(RFC8414, { signal: AbortSignal.abort(reason) }), + ).rejects.toBe(reason); + }); + + it("still falls back to the original response on an ordinary probe failure", async () => { + const inner = vi.fn((input) => { + if (String(input) === RFC8414) { + return Promise.resolve(new Response(null, { status: 404 })); + } + return Promise.reject(new TypeError("network error")); + }); + const wrapped = withRfc8414OidcCompat(inner); + + const response = await wrapped(RFC8414); + + expect(response.status).toBe(404); + }); +}); diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts new file mode 100644 index 00000000..5f5b0465 --- /dev/null +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -0,0 +1,895 @@ +/** + * Unit coverage for `withOAuthRequestTimeout` (#2319) — the deadline every + * OAuth-path request runs under. + * + * What is pinned here, and why each case exists rather than being obvious: + * + * - **The bound itself**, including the *body*: `fetch` resolves on headers, so + * a server that sends them and then stalls has to be caught by the buffering + * race, not by the fetch promise. + * - **The response's identity.** The bound drains a *clone* and hands back the + * original, so the cases assert identity, that the body is still readable + * after the drain, and that a caller's own `clone()` keeps its metadata — + * the property a rebuilt response could not have, since `url` / `redirected` + * / `type` are internal slots. + * - **Cancellation semantics.** The caller's signal is forwarded *and* raced, + * so the tests distinguish the two: one uses a fetch that honours the signal, + * another uses one that ignores it entirely. + * - **Composition with `withRfc8414OidcCompat`**, written as a contrast between + * the two orderings. It is the ordering that is under test, not one + * arrangement's behaviour, so the wrong one is asserted to misattribute. + * + * Fake timers throughout: every budget here is exercised by advancing the clock + * rather than by waiting, so the suite costs milliseconds. + */ +import { describe, it, expect, vi, afterEach } from "vitest"; +import { + DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + OAuthRequestTimeoutError, + deadlineForRequestInit, + exemptMcpEndpoint, + withOAuthRequestTimeout, +} from "@inspector/core/auth/requestTimeout.js"; +import { withRfc8414OidcCompat } from "@inspector/core/auth/oidcDiscoveryCompat.js"; + +const URL_UNDER_TEST = + "https://as.example.com/.well-known/oauth-authorization-server"; + +/** A fetch that never settles — the wedged authorization server of #2319. */ +const neverSettles: typeof fetch = () => new Promise(() => {}); + +afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); +}); + +describe("withOAuthRequestTimeout", () => { + it("passes a prompt response through", async () => { + // The same object: the body is drained through a clone under the deadline + // (see the stalled-body case below), so nothing about the response the + // caller receives changes. + const inner = vi + .fn() + .mockResolvedValue(new Response('{"ok":true}', { status: 200 })); + const wrapped = withOAuthRequestTimeout(inner, 1000); + + const response = await wrapped(URL_UNDER_TEST); + + expect(response.status).toBe(200); + await expect(response.json()).resolves.toEqual({ ok: true }); + expect(inner).toHaveBeenCalledTimes(1); + }); + + it("rejects a stalled request with an error naming the endpoint", async () => { + vi.useFakeTimers(); + const wrapped = withOAuthRequestTimeout(neverSettles, 1000); + + const pending = wrapped(URL_UNDER_TEST); + const assertion = expect(pending).rejects.toThrow( + `OAuth request to ${URL_UNDER_TEST} timed out after 1000ms`, + ); + await vi.advanceTimersByTimeAsync(1000); + await assertion; + }); + + it("reports the timeout as OAuthRequestTimeoutError carrying url and budget", async () => { + vi.useFakeTimers(); + const wrapped = withOAuthRequestTimeout(neverSettles, 1000); + + const pending = wrapped(new URL(URL_UNDER_TEST)); + const assertion = pending.catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + const err = await assertion; + + expect(err).toBeInstanceOf(OAuthRequestTimeoutError); + const timeout = err as OAuthRequestTimeoutError; + expect(timeout.name).toBe("OAuthRequestTimeoutError"); + expect(timeout.url).toBe(URL_UNDER_TEST); + expect(timeout.timeoutMs).toBe(1000); + }); + + it("redacts sensitive query values from the endpoint it names", async () => { + vi.useFakeTimers(); + // This message is recorded verbatim by `createFetchTracker` into the + // Network log and the persisted session, and an OAuth endpoint's query + // string can carry a `code`, an `access_token` or a `client_secret` — + // which `fetchTracking` already redacts everywhere else it records a URL. + const sensitive = + "https://as.example.com/token?code=abc123&client_secret=shhh&state=keepme"; + const wrapped = withOAuthRequestTimeout(neverSettles, 1000); + + const assertion = wrapped(sensitive).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + const err = (await assertion) as OAuthRequestTimeoutError; + + for (const secret of ["abc123", "shhh"]) { + expect(err.message).not.toContain(secret); + expect(err.url).not.toContain(secret); + } + // Still identifiable, which was the point of naming the endpoint: the path + // and every non-sensitive parameter survive. Read through `URL`, since + // `URLSearchParams.toString()` percent-encodes the redaction marker. + const redacted = new URL(err.url); + expect(redacted.origin + redacted.pathname).toBe( + "https://as.example.com/token", + ); + expect(redacted.searchParams.get("state")).toBe("keepme"); + expect(redacted.searchParams.get("code")).toBe("[REDACTED]"); + expect(redacted.searchParams.get("client_secret")).toBe("[REDACTED]"); + }); + + it("takes the URL off a Request object too", async () => { + vi.useFakeTimers(); + const wrapped = withOAuthRequestTimeout(neverSettles, 1000); + + const pending = wrapped(new Request(URL_UNDER_TEST, { method: "POST" })); + const assertion = pending.catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({ + url: URL_UNDER_TEST, + }); + }); + + it("aborts the underlying fetch rather than leaving it running detached", async () => { + vi.useFakeTimers(); + let seen: AbortSignal | undefined; + const inner: typeof fetch = (_input, init) => { + seen = init?.signal ?? undefined; + return new Promise((_resolve, reject) => { + init?.signal?.addEventListener("abort", () => + reject(new Error("aborted by signal")), + ); + }); + }; + const wrapped = withOAuthRequestTimeout(inner, 1000); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + expect(seen?.aborted).toBe(true); + // The underlying rejection can win the race against our own; either way the + // caller must be told it was a timeout, and which endpoint stalled. + expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); + }); + + it("still enforces the deadline when the fetch ignores the signal", async () => { + vi.useFakeTimers(); + // The signal now reaches all the way out — `createRemoteFetch` forwards it + // onto the proxy hop and `/api/fetch` composes it into its outbound call — + // but a `fetchFn` that ignores `AbortSignal` cancels nothing, so the race + // is what bounds this case. + const signalIgnoring: typeof fetch = () => new Promise(() => {}); + const wrapped = withOAuthRequestTimeout(signalIgnoring, 1000); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); + }); + + it("forwards a caller-supplied signal to the inner fetch", async () => { + const inner: typeof fetch = (_input, init) => + new Promise((_resolve, reject) => { + init?.signal?.addEventListener("abort", () => + reject(new Error("caller cancelled")), + ); + }); + const wrapped = withOAuthRequestTimeout(inner, 60_000); + + const caller = new AbortController(); + const pending = wrapped(URL_UNDER_TEST, { signal: caller.signal }); + caller.abort(); + + await expect(pending).rejects.toThrow("caller cancelled"); + }); + + it("races caller cancellation too, so it wins on a signal-ignoring fetch", async () => { + // Forwarding alone is not enough against a `fetchFn` that ignores the + // signal: without the race the caller's abort would sit pending for the + // whole budget instead of winning. + const wrapped = withOAuthRequestTimeout(neverSettles, 60_000); + + const caller = new AbortController(); + const pending = wrapped(URL_UNDER_TEST, { signal: caller.signal }); + const reason = new Error("caller gave up"); + caller.abort(reason); + + await expect(pending).rejects.toBe(reason); + }); + + it("rejects an already-aborted signal without sending the request", async () => { + const inner = vi.fn(neverSettles); + const wrapped = withOAuthRequestTimeout(inner, 60_000); + const reason = new Error("already gone"); + + await expect( + wrapped(URL_UNDER_TEST, { signal: AbortSignal.abort(reason) }), + ).rejects.toBe(reason); + // Racing it would still have evaluated the inner fetch, and a fetch that + // does not check an already-aborted signal would send the request. + expect(inner).not.toHaveBeenCalled(); + }); + + it("treats an undefined init.signal as absent, keeping a Request's own", async () => { + // `RequestInit` is a WebIDL dictionary: a member present as `undefined` is + // converted as absent. `{ ...base, signal: undefined }` is what a spread + // over an options object with no signal produces. + const wrapped = withOAuthRequestTimeout(neverSettles, 60_000); + + const caller = new AbortController(); + const pending = wrapped( + new Request(URL_UNDER_TEST, { signal: caller.signal }), + { signal: undefined }, + ); + const reason = new Error("request cancelled"); + caller.abort(reason); + + await expect(pending).rejects.toBe(reason); + }); + + it("honours the signal embedded in a Request input", async () => { + // `init.signal` is absent here, so the Request's own signal is the caller's + // — reading `init` alone would silently override it. + const wrapped = withOAuthRequestTimeout(neverSettles, 60_000); + + const caller = new AbortController(); + const pending = wrapped( + new Request(URL_UNDER_TEST, { signal: caller.signal }), + ); + const reason = new Error("request cancelled"); + caller.abort(reason); + + await expect(pending).rejects.toBe(reason); + }); + + it("lets an explicit null init.signal override a Request's own", async () => { + vi.useFakeTimers(); + // Per the fetch spec a present `signal` key wins, `null` included. + const wrapped = withOAuthRequestTimeout(neverSettles, 1000); + + const caller = new AbortController(); + const pending = wrapped( + new Request(URL_UNDER_TEST, { signal: caller.signal }), + { signal: null }, + ); + const assertion = pending.catch((err: unknown) => err); + caller.abort(new Error("ignored")); + await vi.advanceTimersByTimeAsync(1000); + + expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); + }); + + it("removes its abort listener once the request settles", async () => { + const caller = new AbortController(); + const removeSpy = vi.spyOn(caller.signal, "removeEventListener"); + const wrapped = withOAuthRequestTimeout( + vi.fn().mockResolvedValue(new Response("{}")), + 60_000, + ); + + await wrapped(URL_UNDER_TEST, { signal: caller.signal }); + + // A caller signal outlives one request — one connect attempt makes several + // — so a listener left behind per request would accumulate on it. + expect(removeSpy).toHaveBeenCalledWith("abort", expect.any(Function)); + }); + + it("bounds a stalled response body, not just the headers", async () => { + vi.useFakeTimers(); + // `fetch` resolves on headers. A server that sends them and then stalls + // would leave the caller's `response.json()` hanging unwatched. + const stalledBody: typeof fetch = () => + Promise.resolve( + new Response( + new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode('{"issuer":')); + }, + }), + { headers: { "content-type": "application/json" } }, + ), + ); + const wrapped = withOAuthRequestTimeout(stalledBody, 1000); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); + }); + + it("releases both tee branches when the deadline wins", async () => { + vi.useFakeTimers(); + // Losing the race rejects the caller; on its own it does not stop the read. + // A drain still running would keep pulling bytes *and* keep buffering them + // for the unread original branch — unbounded memory on exactly the + // signal-ignoring body the race exists to contain. Observed through the + // stream's own `cancel`, which is what tearing the branches down calls. + let cancelled = false; + const stalled = new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode('{"issuer":')); + }, + // Never resolves: headers arrived, the body never finishes. + pull() { + return new Promise(() => {}); + }, + cancel() { + cancelled = true; + }, + }); + const wrapped = withOAuthRequestTimeout( + vi + .fn() + .mockResolvedValue( + new Response(stalled, { headers: { "content-type": "text/plain" } }), + ), + 1000, + ); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); + // The cancels are deliberately not awaited — cancelling one branch of a tee + // can wait on the other — so let them settle before observing. + for (let i = 0; i < 20; i++) await Promise.resolve(); + expect(cancelled).toBe(true); + }); + + it("still reports the timeout when tearing the branches down fails", async () => { + vi.useFakeTimers(); + // Cancelling a stream that is already errored, locked elsewhere, or whose + // source rejects is not a failure here — the cleanup runs on a path that is + // already rejecting with something the caller cares about far more. + const hostile = new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode("x")); + }, + pull() { + return new Promise(() => {}); + }, + cancel() { + throw new Error("cancel blew up"); + }, + }); + const wrapped = withOAuthRequestTimeout( + vi + .fn() + .mockResolvedValue( + new Response(hostile, { headers: { "content-type": "text/plain" } }), + ), + 1000, + ); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); + // And no unhandled rejection escapes from the cleanup. + for (let i = 0; i < 20; i++) await Promise.resolve(); + }); + + it("hands back the original response, not a copy", async () => { + const original = new Response('{"issuer":"https://as.example.com"}', { + status: 201, + statusText: "Created", + headers: { "content-type": "application/json", "x-probe": "kept" }, + }); + const wrapped = withOAuthRequestTimeout( + vi.fn().mockResolvedValue(original), + 1000, + ); + + const response = await wrapped(URL_UNDER_TEST); + + // Identity, which is the strongest form of "nothing was changed": every + // native slot, the headers guard and `clone()` behave as they would if this + // wrapper were not in the chain at all. + expect(response).toBe(original); + await expect(response.json()).resolves.toEqual({ + issuer: "https://as.example.com", + }); + }); + + it("leaves the body readable after draining the clone", async () => { + // The bound works by draining a clone; teeing buffers the other branch, so + // the caller must still be able to read the original without a second trip. + const wrapped = withOAuthRequestTimeout( + vi.fn().mockResolvedValue(new Response("hello")), + 1000, + ); + + const response = await wrapped(URL_UNDER_TEST); + + expect(response.bodyUsed).toBe(false); + await expect(response.text()).resolves.toBe("hello"); + }); + + it("leaves the caller free to clone it again, with metadata intact", async () => { + // The case a rebuilt response could never satisfy: `url` / `redirected` / + // `type` are internal slots, so a shadowed own property survives a direct + // read and is lost the moment anyone calls `clone()`. + const original = new Response("{}", { status: 200 }); + Object.defineProperty(original, "url", { + value: "https://as.example.com/redirected", + configurable: true, + }); + Object.defineProperty(original, "redirected", { + value: true, + configurable: true, + }); + const wrapped = withOAuthRequestTimeout( + vi.fn().mockResolvedValue(original), + 1000, + ); + + const response = await wrapped(URL_UNDER_TEST); + const copy = response.clone(); + + expect(copy.url).toBe("https://as.example.com/redirected"); + expect(copy.redirected).toBe(true); + await expect(copy.json()).resolves.toEqual({}); + }); + + it("keeps content-encoding and content-length, which are the server's own", async () => { + // Nothing is rebuilt, so nothing invalidates them: the caller gets the + // response `fetch` produced and decodes it the way `fetch` intends. + const original = new Response("{}", { + headers: { + "content-type": "application/json", + "content-encoding": "gzip", + }, + }); + const wrapped = withOAuthRequestTimeout( + vi.fn().mockResolvedValue(original), + 1000, + ); + + const response = await wrapped(URL_UNDER_TEST); + + expect(response.headers.get("content-encoding")).toBe("gzip"); + }); + + it("passes a null-body status through untouched", async () => { + const original = new Response(null, { status: 204 }); + const wrapped = withOAuthRequestTimeout( + vi.fn().mockResolvedValue(original), + 1000, + ); + + const response = await wrapped(URL_UNDER_TEST); + + expect(response).toBe(original); + expect(response.status).toBe(204); + expect(response.body).toBeNull(); + }); + + it("propagates a non-timeout failure unchanged", async () => { + const boom = new TypeError("network error"); + const inner = vi.fn().mockRejectedValue(boom); + const wrapped = withOAuthRequestTimeout(inner, 1000); + + await expect(wrapped(URL_UNDER_TEST)).rejects.toBe(boom); + }); + + it("rounds a fractional budget, which is reported to the caller", async () => { + vi.useFakeTimers(); + // `setTimeout` would accept the fraction and truncate it. The rounding is + // for the budget's *reported* form: a `performance.now()` subtraction would + // otherwise name "1000.4000000953674ms" in the message and expose a + // non-integer `timeoutMs`. + const wrapped = withOAuthRequestTimeout(neverSettles, 1000.4); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({ + timeoutMs: 1000, + }); + }); + + it("falls back to the default on a non-finite budget", async () => { + vi.useFakeTimers(); + // `NaN` survives `Math.max(0, Math.round(NaN))` and `setTimeout(fn, NaN)` + // fires immediately, so without this every OAuth request under a budget + // that came out of bad arithmetic would fail at once with "timed out after + // NaNms". + for (const bad of [NaN, Infinity, -Infinity]) { + const wrapped = withOAuthRequestTimeout(neverSettles, bad); + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + + await vi.advanceTimersByTimeAsync(DEFAULT_OAUTH_REQUEST_TIMEOUT_MS - 1); + expect(await Promise.race([assertion, Promise.resolve("pending")])).toBe( + "pending", + ); + + await vi.advanceTimersByTimeAsync(1); + expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({ + timeoutMs: DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + }); + } + }); + + it("clamps an over-large budget to what setTimeout can schedule", async () => { + vi.useFakeTimers(); + // Past 2**31-1 the delay overflows a 32-bit signed int and Node falls back + // to 1ms — an immediate timeout, the opposite of what was asked for. + const wrapped = withOAuthRequestTimeout(neverSettles, 2 ** 40); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + expect(await Promise.race([assertion, Promise.resolve("pending")])).toBe( + "pending", + ); + + await vi.advanceTimersByTimeAsync(2_147_483_647); + expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({ + timeoutMs: 2_147_483_647, + }); + }); + + it("clamps a negative budget to zero rather than throwing", async () => { + vi.useFakeTimers(); + const wrapped = withOAuthRequestTimeout(neverSettles, -1); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(0); + + expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({ + timeoutMs: 0, + }); + }); + + it("defaults to a generous budget a slow authorization server can meet", async () => { + vi.useFakeTimers(); + const wrapped = withOAuthRequestTimeout(neverSettles); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(DEFAULT_OAUTH_REQUEST_TIMEOUT_MS - 1); + expect(await Promise.race([assertion, Promise.resolve("pending")])).toBe( + "pending", + ); + + await vi.advanceTimersByTimeAsync(1); + expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({ + timeoutMs: DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + }); + }); + + 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"; + + /** Most cases are plain JSON-RPC traffic, which carries no form media type. */ + const JSON_HEADERS = new Headers({ "content-type": "application/json" }); + /** The token family: RFC 6749 §4.1.3 and RFC 7009 §2.1 are form-encoded. */ + const FORM_HEADERS = new Headers({ + "content-type": "application/x-www-form-urlencoded;charset=UTF-8", + }); + + /** Streamable HTTP: every MCP request goes to the configured URL. */ + const streamable = (serverUrl: string) => + exemptMcpEndpoint( + () => serverUrl, + () => false, + ); + /** Legacy SSE: the message endpoint arrives inside the stream. */ + const sse = (serverUrl: string) => + exemptMcpEndpoint( + () => serverUrl, + () => true, + ); + + it("exempts the MCP endpoint itself, whatever method or query", () => { + const isExempt = exemptMcpEndpoint(() => SERVER); + + expect(isExempt(SERVER, JSON_HEADERS)).toBe(true); + expect(isExempt(`${SERVER}?sessionId=abc`, JSON_HEADERS)).toBe(true); + expect(isExempt(`${SERVER}#frag`, JSON_HEADERS)).toBe(true); + }); + + it("never exempts a form-encoded request, whatever URL it uses", () => { + // `oauthTokenUrl` takes any absolute URL, including the MCP endpoint's + // own — at which point a URL rule would exempt the transport-internal + // refresh, the one request this change exists to bound. The token family + // is form-encoded and MCP never is, so the media type settles it first. + for (const isExempt of [ + streamable(SERVER), + sse(SERVER), + exemptMcpEndpoint(() => SERVER), + ]) { + expect(isExempt(SERVER, FORM_HEADERS)).toBe(false); + } + // And a missing server URL no longer fails open past it either. + expect(exemptMcpEndpoint(() => undefined)(SERVER, FORM_HEADERS)).toBe( + false, + ); + }); + + it("bounds same-origin OAuth work on a Streamable HTTP connection", () => { + // The narrow rule, available because the endpoint is the configured URL. + const isExempt = streamable(SERVER); + + expect(isExempt(SERVER, JSON_HEADERS)).toBe(true); + expect( + isExempt( + "https://srv.example.com/.well-known/oauth-protected-resource/mcp", + JSON_HEADERS, + ), + ).toBe(false); + expect(isExempt("https://srv.example.com/token", JSON_HEADERS)).toBe( + false, + ); + }); + + it("widens to the origin on legacy SSE, whose message path is unknowable", () => { + // The cost of the wider rule, asserted rather than implied: same-origin + // OAuth is exempt here, and falls back to the SDK's per-request timeout. + const isExempt = sse("https://srv.example.com/sse"); + + expect( + isExempt( + "https://srv.example.com/.well-known/openid-configuration", + JSON_HEADERS, + ), + ).toBe(true); + expect(isExempt("https://as.example.com/token", JSON_HEADERS)).toBe( + false, + ); + }); + + it("falls back to the origin rule when the transport is unknown", () => { + // Fails open on this uncertainty like every other: a path rule applied to + // a connection that turns out to be SSE severs real MCP traffic. + const noHint = exemptMcpEndpoint(() => SERVER); + const throws = exemptMcpEndpoint( + () => SERVER, + () => { + throw new Error("not yet connected"); + }, + ); + + expect(noHint("https://srv.example.com/token", JSON_HEADERS)).toBe(true); + expect(throws("https://srv.example.com/token", JSON_HEADERS)).toBe(true); + }); + + it("exempts legacy SSE's separate message endpoint", () => { + // The case a path rule gets wrong: `SSEClientTransport` opens the + // configured URL and is handed a *different* pathname to POST every + // JSON-RPC message to. Bounding those would sever a slow tool call over + // SSE and report it as an OAuth timeout. The SDK enforces that the + // message endpoint shares the stream URL's origin, which is what makes + // an origin rule exact rather than approximate. + const isExempt = exemptMcpEndpoint(() => "https://srv.example.com/sse"); + + expect( + isExempt( + "https://srv.example.com/messages?sessionId=abc", + JSON_HEADERS, + ), + ).toBe(true); + expect(isExempt("https://srv.example.com/", JSON_HEADERS)).toBe(true); + }); + + it("bounds OAuth work on a different origin", () => { + const isExempt = exemptMcpEndpoint(() => SERVER); + + expect(isExempt("https://as.example.com/token", JSON_HEADERS)).toBe( + false, + ); + expect( + isExempt( + "https://as.example.com/.well-known/openid-configuration", + JSON_HEADERS, + ), + ).toBe(false); + // Origin is scheme + host + port, so any of the three differing bounds it. + expect(isExempt("http://srv.example.com/token", JSON_HEADERS)).toBe( + false, + ); + expect(isExempt("https://srv.example.com:8443/token", JSON_HEADERS)).toBe( + false, + ); + }); + + it("fails open when the server URL is unknown or unparseable", () => { + // The two errors are not symmetric: bounding what should not be bounded + // severs a long-running tool call, while failing to bound leaves the + // SDK's own per-request timeout as the backstop it already was. + expect( + exemptMcpEndpoint(() => undefined)( + "https://as.example.com/token", + JSON_HEADERS, + ), + ).toBe(true); + expect( + exemptMcpEndpoint(() => "")( + "https://as.example.com/token", + JSON_HEADERS, + ), + ).toBe(true); + expect(exemptMcpEndpoint(() => "not a url")(SERVER, JSON_HEADERS)).toBe( + true, + ); + expect(exemptMcpEndpoint(() => SERVER)("not a url", JSON_HEADERS)).toBe( + true, + ); + }); + + it("fails open when the server-URL getter throws", () => { + // `InspectorClient.getServerUrl()` throws for a non-HTTP configuration, + // and this wrapped fetch can still be handed to a transport or a custom + // factory that calls it — propagating a configuration error out of a + // fetch would turn the documented fail-open into a hard failure. + const isExempt = exemptMcpEndpoint(() => { + throw new Error("Server URL is only available for HTTP transports"); + }); + + expect(isExempt("https://as.example.com/token", JSON_HEADERS)).toBe(true); + // Still not past the form-encoded check, which runs first. + expect(isExempt("https://as.example.com/token", FORM_HEADERS)).toBe( + false, + ); + }); + + it("reads the server URL per call, since it changes between connects", () => { + let current = SERVER; + const isExempt = exemptMcpEndpoint(() => current); + + expect(isExempt("https://other.example.com/mcp", JSON_HEADERS)).toBe( + false, + ); + current = "https://other.example.com/mcp"; + expect(isExempt("https://other.example.com/mcp", JSON_HEADERS)).toBe( + true, + ); + expect(isExempt(SERVER, JSON_HEADERS)).toBe(false); + }); + }); + + it("passes an exempt request through with no deadline and no buffering", async () => { + vi.useFakeTimers(); + // Untouched means untouched: no signal of ours, and the body is left as a + // live stream rather than buffered — which is what an SSE response needs. + const body = new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode("data: hi\n\n")); + }, + }); + const original = new Response(body, { + headers: { "content-type": "text/event-stream" }, + }); + const inner = vi.fn().mockResolvedValue(original); + const wrapped = withOAuthRequestTimeout(inner, 1000, () => true); + + const response = await wrapped(URL_UNDER_TEST); + + expect(response).toBe(original); + expect( + (inner.mock.calls[0][1] as RequestInit | undefined)?.signal, + ).toBeUndefined(); + // Nothing is armed, so advancing past the budget cannot sever it. + await vi.advanceTimersByTimeAsync(5000); + expect(response.bodyUsed).toBe(false); + }); + + describe("composed under withRfc8414OidcCompat (#2319 ordering)", () => { + // `withRfc8414OidcCompat` re-fetches an OIDC candidate after a failed RFC + // 8414 discovery, so where the deadline sits relative to it decides which + // request is being timed. `InspectorClient` and the CLI both put it + // innermost; these two cases are why. + const RFC8414 = + "https://as.example.com/.well-known/oauth-authorization-server/tenant"; + // A path-suffixed RFC 8414 candidate has two OIDC siblings, and the compat + // wrapper tries both. + const PROBES = [ + "https://as.example.com/.well-known/openid-configuration/tenant", + "https://as.example.com/tenant/.well-known/openid-configuration", + ]; + + /** Answers the RFC 8414 leg with a prompt 404, then stalls on the probe. */ + function stallingProbeFetch() { + return vi.fn((input) => { + if (String(input) === RFC8414) { + return Promise.resolve(new Response(null, { status: 404 })); + } + return new Promise(() => {}); + }); + } + + it("outside, a stalled probe is misreported under the preceding URL", async () => { + vi.useFakeTimers(); + const inner = stallingProbeFetch(); + const wrapped = withOAuthRequestTimeout( + withRfc8414OidcCompat(inner), + 1000, + ); + + const assertion = wrapped(RFC8414).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + const err = (await assertion) as OAuthRequestTimeoutError; + expect(err).toBeInstanceOf(OAuthRequestTimeoutError); + // The endpoint that stalled was PROBE. One budget covers both legs, and + // the error names the request the caller made rather than the one that + // hung — exactly the diagnostic this change exists to provide. + expect(err.url).toBe(RFC8414); + }); + + it("innermost, the probe's own timeout escapes and names the probe", async () => { + vi.useFakeTimers(); + const inner = stallingProbeFetch(); + const wrapped = withRfc8414OidcCompat( + withOAuthRequestTimeout(inner, 1000), + ); + + const assertion = wrapped(RFC8414).catch((err: unknown) => err); + // The probe is timed from when it starts, on a budget of its own — not on + // whatever an outer one had left after the RFC 8414 leg. + await vi.advanceTimersByTimeAsync(1000); + + const err = (await assertion) as OAuthRequestTimeoutError; + // The compat wrapper is inert on an ordinary probe failure, but a deadline + // the Inspector imposed escapes it, so the caller is told which endpoint + // stalled rather than being handed the preceding 404. + expect(err).toBeInstanceOf(OAuthRequestTimeoutError); + expect(err.url).toBe(PROBES[0]); + expect(inner).toHaveBeenCalledTimes(2); + }); + }); + + it("clears its timer once the request settles", async () => { + vi.useFakeTimers(); + const clearSpy = vi.spyOn(globalThis, "clearTimeout"); + const wrapped = withOAuthRequestTimeout( + vi.fn().mockResolvedValue(new Response("{}")), + 1000, + ); + + await wrapped(URL_UNDER_TEST); + + expect(clearSpy).toHaveBeenCalled(); + // Nothing is left to fire, so no unhandled rejection can surface later. + await vi.advanceTimersByTimeAsync(5000); + }); +}); diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index e50fa1e9..c108b530 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -393,11 +393,11 @@ describe("revokeToken", () => { }); }); - // The web fetch is `createRemoteFetch`, which re-issues the call as a POST to - // `/api/fetch` and drops `init.signal`; the backend's outbound fetch gets no - // signal either. A signal-only bound is therefore inert on exactly the path - // the timeout exists for, so the deadline has to hold against a fetch that - // ignores the signal entirely. + // The signal reaches every path since #2319 — `createRemoteFetch` forwards it + // onto the `/api/fetch` hop and that route composes it into its outbound + // fetch — but a signal-only bound is still only as good as the fetch beneath + // it. A `fetchFn` that ignores `AbortSignal`, or a wedged backend, would + // leave a signal-only deadline inert, so it has to hold without one. it("gives up on a fetch that never settles and ignores the signal", async () => { const outcome = await revokeToken({ endpoint: REVOKE_URL, 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 7ae9edbf..27a42d61 100644 --- a/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts +++ b/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts @@ -1,5 +1,10 @@ import { describe, it, expect, vi } from "vitest"; import { createRemoteFetch } from "@inspector/core/mcp/remote/createRemoteFetch.js"; +import { + OAUTH_TIMEOUT_WIRE_CODE, + OAuthRequestTimeoutError, + withOAuthRequestTimeout, +} from "@inspector/core/auth/requestTimeout.js"; function ok(body: object): Response { return new Response(JSON.stringify(body), { @@ -168,4 +173,225 @@ describe("createRemoteFetch", () => { expect(res.headers.get("x-test")).toBe("yes"); expect(await res.text()).toBe("hello"); }); + + describe("cancellation forwarding (#2319)", () => { + // Without this the caller's abort settled only its own promise: the POST + // stayed in flight, the backend never saw its request cancelled, and its + // outbound fetch to the authorization server ran on detached. + function capture() { + const fetchFn = vi + .fn() + .mockResolvedValue(ok(standardRemoteBody)); + const remoteFetch = createRemoteFetch({ + baseUrl: "http://remote.example", + // A contextually typed forwarding function rather than a double cast: + // the mock stays inspectable and nothing bypasses the type system. + fetchFn: (input, init) => fetchFn(input, init), + }); + return { fetchFn, remoteFetch }; + } + + function signalOf(fetchFn: ReturnType>) { + return (fetchFn.mock.calls[0][1] as RequestInit).signal; + } + + it("forwards init.signal onto the proxy hop", async () => { + const { fetchFn, remoteFetch } = capture(); + const caller = new AbortController(); + + await remoteFetch("http://upstream.example/", { signal: caller.signal }); + + expect(signalOf(fetchFn)).toBe(caller.signal); + }); + + it("forwards a Request's own signal when init has none", async () => { + const { fetchFn, remoteFetch } = capture(); + const caller = new AbortController(); + // Compared against `request.signal`, not `caller.signal`: the Fetch + // standard gives `new Request(url, { signal })` a *dependent* signal, so + // the two are distinct objects in a spec-compliant runtime. Asserting + // against the controller's would encode non-standard identity and could + // pass or fail on the host rather than on the behaviour (Copilot). + const request = new Request("http://upstream.example/", { + signal: caller.signal, + }); + + await remoteFetch(request); + + expect(signalOf(fetchFn)).toBe(request.signal); + }); + + it("treats an undefined init.signal as absent, keeping the Request's", async () => { + // WebIDL dictionary conversion: a member present as `undefined` is absent. + const { fetchFn, remoteFetch } = capture(); + const caller = new AbortController(); + const request = new Request("http://upstream.example/", { + signal: caller.signal, + }); + + await remoteFetch(request, { signal: undefined }); + + // Against `request.signal` for the dependent-signal reason above. + expect(signalOf(fetchFn)).toBe(request.signal); + }); + + it("honours an explicit null init.signal as no signal", async () => { + const { fetchFn, remoteFetch } = capture(); + const caller = new AbortController(); + + await remoteFetch( + new Request("http://upstream.example/", { signal: caller.signal }), + { signal: null }, + ); + + expect(signalOf(fetchFn)).toBeUndefined(); + }); + + it("sends no signal when the caller supplied none", async () => { + const { fetchFn, remoteFetch } = capture(); + + await remoteFetch("http://upstream.example/"); + + expect(signalOf(fetchFn)).toBeUndefined(); + }); + }); + + describe("proxy-side deadline (#2319)", () => { + function remoteReturning(status: number, body: string) { + const fetchFn = vi.fn().mockResolvedValue( + new Response(body, { + status, + headers: { "content-type": "application/json" }, + }), + ); + return createRemoteFetch({ + baseUrl: "http://remote.example", + // Forwarding function, not a double cast — see `capture` above. + fetchFn: (input, init) => fetchFn(input, init), + }); + } + + it("rebuilds an OAuthRequestTimeoutError from the route's marker", async () => { + // The proxy boundary in isolation: whichever end's deadline fires, a + // timeout that comes back as an ordinary error response must still + // reconstruct as a typed one. Both ends run the same budget in + // production, so this path is the backend winning the race — a + // backgrounded tab throttling `setTimeout` is the plausible case — and + // which one fired must not decide whether the error names its endpoint. + const remoteFetch = remoteReturning( + 504, + JSON.stringify({ + error: "proxied request to https://as.example.com/x timed out", + code: OAUTH_TIMEOUT_WIRE_CODE, + url: "https://as.example.com/x", + timeoutMs: 30000, + }), + ); + + const err = await remoteFetch("https://as.example.com/x").catch( + (e: unknown) => e, + ); + + expect(err).toBeInstanceOf(OAuthRequestTimeoutError); + const timeout = err as OAuthRequestTimeoutError; + expect(timeout.url).toBe("https://as.example.com/x"); + expect(timeout.timeoutMs).toBe(30000); + }); + + it("leaves an ordinary error response as a plain Error", async () => { + const remoteFetch = remoteReturning( + 500, + JSON.stringify({ error: "connect ECONNREFUSED" }), + ); + + const err = await remoteFetch("https://as.example.com/x").catch( + (e: unknown) => e, + ); + + expect(err).not.toBeInstanceOf(OAuthRequestTimeoutError); + expect((err as Error).message).toMatch(/Remote fetch failed \(500\)/); + }); + + it("ignores a non-JSON error body rather than throwing on the parse", async () => { + const remoteFetch = remoteReturning(502, "bad gateway"); + + const err = await remoteFetch("https://as.example.com/x").catch( + (e: unknown) => e, + ); + + expect((err as Error).message).toMatch(/Remote fetch failed \(502\)/); + }); + + it("ignores a marker whose fields are the wrong shape", async () => { + // An upstream could serve JSON of its own through a failing proxy; the + // guard checks the field types, not just the code. + const remoteFetch = remoteReturning( + 504, + JSON.stringify({ code: OAUTH_TIMEOUT_WIRE_CODE, url: 5 }), + ); + + const err = await remoteFetch("https://as.example.com/x").catch( + (e: unknown) => e, + ); + + 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 aabda5f2..7314d514 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 @@ -300,6 +300,12 @@ describe("server.ts supplemental coverage", () => { let h: Harness; let target: ServerType; let targetUrl: string; + /** Resolves when an upstream `/stall` request is closed by its client. */ + let stallClosed: Promise; + /** Paths the upstream fixture has actually received. */ + let upstreamHits: string[]; + /** Resolves when the never-ending `/stream-open` response is closed. */ + let openStreamClosed: Promise; // Isolate the WHOLE suite from the developer's (or CI's) proxy environment. // Setting only uppercase HTTP_PROXY in the one proxy test would not be @@ -336,7 +342,43 @@ describe("server.ts supplemental coverage", () => { h = await start(); // A tiny upstream HTTP server we can point /api/fetch at. const { createServer } = await import("node:http"); + let markClosed: () => void = () => {}; + stallClosed = new Promise((resolve) => { + markClosed = resolve; + }); + upstreamHits = []; + let markStreamClosed: () => void = () => {}; + openStreamClosed = new Promise((resolve) => { + markStreamClosed = resolve; + }); const srv = createServer((req, res) => { + upstreamHits.push(req.url ?? ""); + if (req.url === "/stream-hostile") { + // Headers, one frame, and a connection whose teardown the route must + // not wait on: `ReadableStream.cancel()` adopts the source's cancel + // promise, which may never settle. + res.writeHead(200, { "Content-Type": "text/event-stream" }); + res.write("data: hi\n\n"); + return; + } + if (req.url === "/stream-open") { + // Event-stream headers, one frame, and then nothing — the shape that + // used to leave a socket open for good, because the route classifies + // it as a stream, returns JSON without the body, and clears its + // deadline on the way out. + res.writeHead(200, { "Content-Type": "text/event-stream" }); + res.write("data: hi\n\n"); + res.on("close", () => markStreamClosed()); + return; + } + if (req.url === "/stall") { + // Accept the request, answer nothing, and report when the client + // gives up — the #2319 wedged-authorization-server shape. `close` on + // the response fires whether the socket was torn down or the handler + // simply ended, and nothing here ever ends it. + res.on("close", () => markClosed()); + return; + } if (req.url === "/stream") { res.writeHead(200, { "Content-Type": "text/event-stream" }); res.write("data: hi\n\n"); @@ -381,6 +423,175 @@ describe("server.ts supplemental coverage", () => { await stop(h); }); + it("releases the upstream request when the browser cancels (#2319)", async () => { + // The point of forwarding the signal through `createRemoteFetch`: without + // it the browser's abort settled only its own promise, and this handler + // plus the upstream socket stayed pending for as long as the server cared + // to stall — once per timed-out attempt. Asserted at the route rather + // than on a mock, because the mock cannot show the socket being released. + 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 rejected = pending.catch((err: unknown) => err); + + // Wait until the upstream has actually received the request, so the abort + // cannot race ahead of it and pass for the wrong reason. + await vi.waitFor(() => { + expect(upstreamHits).toContain("/stall"); + }); + caller.abort(); + + expect(await rejected).toBeInstanceOf(Error); + // The upstream sees its connection go away. Without the signal composed + // into the route's outbound fetch this never resolves and the test times + // out. + await stallClosed; + }); + + it("cancels a discarded stream that sends headers and never ends (#2319)", async () => { + // The route answers with JSON and no body, so nobody downstream owns the + // upstream stream and nothing will ever read it; the route cancels it + // explicitly rather than relying on that being reclaimed for it. + // + // ⚠️ This pins the OUTCOME, not the mechanism: measured, it still passes + // with the explicit `cancel()` removed, because undici reclaims an unread + // response body here on its own. So the cancel is defensive — it makes + // the release explicit and independent of that behaviour — and this test + // will catch the route starting to hold the stream open, not the cancel + // being deleted. Said plainly rather than left to imply a stronger claim. + const res = await fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: `${targetUrl}/stream-open` }), + }); + + expect(res.status).toBe(200); + const payload = (await res.json()) as { status: number; body?: string }; + expect(payload.status).toBe(200); + expect(payload.body).toBeUndefined(); + // The observable contract: the route does not leave the upstream stream + // running after it has answered. (Per the qualification above, this does + // not isolate the explicit `cancel()` as the cause.) + await openStreamClosed; + }); + + 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. What it pins is typed-marker + // preservation when the *backend* wins the client-side race — both ends + // run the same budget, and a backgrounded tab throttling `setTimeout` is + // the plausible way the server's fires first. The error has to survive + // the hop as something an `instanceof OAuthRequestTimeoutError` check can + // still recognize, whichever end produced it. + const res = await fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: `${targetUrl}/stall`, timeoutMs: 120 }), + }); + + expect(res.status).toBe(504); + const payload = (await res.json()) as { + error: string; + code: string; + url: string; + timeoutMs: number; + }; + expect(payload.code).toBe("oauth_request_timeout"); + expect(payload.url).toBe(`${targetUrl}/stall`); + expect(payload.timeoutMs).toBe(120); + expect(payload.error).toContain("timed out after 120ms"); + // And the upstream is released rather than left running. + 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. + // + // ⚠️ What it does NOT catch, stated so nobody reads more into it: the + // wait below is 400ms, so an unconditional timer restored at the + // production 30s budget would still leave the request pending here and + // the test would pass (Copilot). Catching that needs either a ~31s wait + // on every CI run or a route-level deadline knob existing only for the + // test — and a second source of truth for the deadline is the defect + // round 13 removed. Measured, this fails against an unconditional timer + // at any budget shorter than the wait. + 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("answers a discarded stream without waiting on its cancellation", async () => { + // ⚠️ The route starts the cancel and returns. Awaiting it would keep the + // handler pending on a source that never settles its cancel promise — + // and on an unbounded request there is no timer to release it either. + const res = await fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: `${targetUrl}/stream-hostile` }), + }); + + expect(res.status).toBe(200); + const payload = (await res.json()) as { status: number; body?: string }; + expect(payload.status).toBe(200); + expect(payload.body).toBeUndefined(); + }); + 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 c74702ce..918eaac3 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -51,6 +51,18 @@ export { withRfc8414OidcCompat, } from "./oidcDiscoveryCompat.js"; +// OAuth-path request deadline (#2319) +export { + DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + OAUTH_TIMEOUT_WIRE_CODE, + OAuthRequestTimeoutError, + deadlineForRequestInit, + exemptMcpEndpoint, + isOAuthRequestTimeoutWire, + withOAuthRequestTimeout, +} from "./requestTimeout.js"; +export type { OAuthRequestTimeoutWire } from "./requestTimeout.js"; + // Storage export type { OAuthStorage, diff --git a/core/auth/oidcDiscoveryCompat.ts b/core/auth/oidcDiscoveryCompat.ts index bb30e722..ce0ace87 100644 --- a/core/auth/oidcDiscoveryCompat.ts +++ b/core/auth/oidcDiscoveryCompat.ts @@ -90,6 +90,7 @@ * wiring in `core/mcp/inspectorClient.ts`. */ +import { callerSignalOf, OAuthRequestTimeoutError } from "./requestTimeout.js"; import { OAuthMetadataSchema, OpenIdProviderDiscoveryMetadataSchema, @@ -169,6 +170,22 @@ async function releaseBody(response: Response): Promise { await response.body?.cancel().catch(() => {}); } +/** + * The same release, started but not waited on. + * + * ⚠️ `ReadableStream.cancel()` adopts the underlying source's cancel promise, + * which is permitted never to settle — so awaiting it on a path that is + * *propagating a cancellation* can hang the very thing that was meant to end + * (Copilot). Every exceptional exit below uses this: the release is a courtesy + * to the connection pool, and the caller's abort or timeout must reach it + * regardless. The `void` is the documented case where the callee owns its + * failures — `releaseBody` swallows its own — and the caller genuinely cannot + * await. + */ +function releaseBodyDetached(response: Response): void { + void releaseBody(response); +} + /** * Whether a parsed body is RFC 8414 authorization-server metadata that is *not* * a valid OpenID provider document — the exact shape the upstream schema @@ -248,6 +265,13 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { if (candidates.length === 0) return response; const headers = discoveryHeaders(input, init); + // The probe is a request the caller never made, issued on its behalf — so + // it has to be cancellable by the caller too. Without the signal, aborting + // after the initial 404 left the probe running to its own deadline and the + // caller was handed a timeout in place of its own abort (Copilot). Same + // precedence rule as everywhere else, hence the shared helper rather than a + // second copy of it. + const callerSignal = callerSignalOf(input, init); // The loop advances to the next candidate only where the SDK's own loop // would. Anywhere else it hands the original response back and lets // discovery run its normal course, because *promoting a later candidate @@ -259,10 +283,50 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // those, returning `response` leaves the SDK to make the same request and // reach the same verdict it always would. for (const candidate of candidates) { + // Don't issue a probe the caller has already given up on. A real `fetch` + // rejects immediately on a pre-aborted signal, so this is mostly about + // not sending the request at all — the same reasoning as the + // short-circuit in `withOAuthRequestTimeout`. + // + // Every exceptional exit below releases `response` first. On a normal + // return it is the value the caller gets and reads; on a throw nobody + // will ever read it, and an unread body holds its connection open on + // Node/undici — the same reason the successful-substitution path + // releases it (Copilot). Matters most for a standalone user of this + // exported wrapper, where no timeout wrapper is underneath to have + // already drained it. + if (callerSignal?.aborted) { + releaseBodyDetached(response); + throw callerSignal.reason; + } + let probe: Response; try { - probe = await fetchFn(candidate, { headers }); - } catch { + probe = await fetchFn(candidate, { headers, signal: callerSignal }); + } catch (err) { + // A deadline the Inspector imposed is *our* error, not the server's + // answer, so it escapes rather than being folded back into the original + // response (#2319, Copilot). Swallowing it would hand the caller the + // preceding 404 and discard the one thing the deadline adds — the name + // of the endpoint that stalled — leaving discovery to fail later under + // a message that points at the wrong URL. There is no flow this can + // break that was not already broken: every candidate this loop probes + // is one the SDK's own `buildDiscoveryUrls` emits, so a stall here is a + // stall the SDK would have hit itself, unbounded. Everything else — a + // CORS rejection, DNS, a reset — still falls back as documented above. + if (err instanceof OAuthRequestTimeoutError) { + releaseBodyDetached(response); + throw err; + } + // Likewise a caller that gave up: substituting the preceding 404 for + // its own abort would report a discovery failure for a request it + // deliberately cancelled. The caller's own `reason` is what surfaces, + // here and at the two sites below, rather than whatever shape the + // underlying layer happened to reject with. + if (callerSignal?.aborted) { + releaseBodyDetached(response); + throw callerSignal.reason; + } return response; } if (!probe.ok) { @@ -272,6 +336,14 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // candidate exhaust the origin's pool (Copilot). Same discipline as // `core/mcp/node/authChallengeFetch.ts`. await releaseBody(probe); + // Rechecked after the release, which awaits: an abort that lands while + // the body is being discarded must not be answered with the preceding + // response either, and `continue` would otherwise carry on probing for + // a caller that has stopped waiting. + if (callerSignal?.aborted) { + releaseBodyDetached(response); + throw callerSignal.reason; + } if (continuesDiscovery(probe.status)) continue; return response; } @@ -288,6 +360,17 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { body = await probe.text(); parsed = JSON.parse(body); } catch { + // `fetch` resolves on headers, so the abort can land here rather than + // in the catch above: the probe's headers arrived, the caller gave up, + // and `probe.text()` rejects on the partial body. Substituting the + // preceding 404 would hand a standalone user of this wrapper a + // discovery failure in place of its own abort reason (Copilot). A body + // that merely will not parse still falls through, which is the case + // this catch exists for. + if (callerSignal?.aborted) { + releaseBodyDetached(response); + throw callerSignal.reason; + } return response; } if (!isRfc8414OnlyMetadata(parsed)) { diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts new file mode 100644 index 00000000..7b771919 --- /dev/null +++ b/core/auth/requestTimeout.ts @@ -0,0 +1,577 @@ +/** + * A bound on every network call the Inspector makes on the OAuth path (#2319). + * + * ## Why this exists + * + * Protected-resource-metadata discovery, authorization-server metadata + * discovery, dynamic client registration, the token exchange and the refresh + * all went out with no `AbortSignal` at all. Token revocation was the single + * exception — `revocation.ts` has carried its own deadline since #2144, and it + * is the pattern this module generalizes. + * + * An unbounded fetch matters more here than it usually would, because of where + * this work runs and what the UI does while it runs: + * + * - It runs **server-side**, in the Inspector's own Node process, proxied + * through the backend. A stall is therefore invisible in browser devtools — + * #2188's reporter correctly observed that no request left the browser at + * all while a Node-side socket sat established and idle. + * - Connect-time auth errors deliberately hold the connection status at + * `"connecting"` rather than moving it to `"error"` + * (`isConnectAuthRecoveryError`, `challenge.ts`), on the theory that a + * redirect is about to end the attempt. A stalled fetch inside that window is + * a silent, unbounded spinner rather than a surfaced failure. + * + * The deadline covers the **whole exchange**, not just the headers: `fetch` + * resolves as soon as response headers arrive, so a server that sends headers + * — or half a JSON document — and then stalls would leave the caller's + * `response.json()` hanging with nothing watching it. Every response on this + * path is a small finite document, so a *clone* of it is drained under the same + * race and the original — untouched, with every native slot intact — is what + * the caller gets. + * + * Today the common case is bounded, but only incidentally: a discovery stall + * that happens to sit inside an SDK `initialize` rides that request's own + * timeout and surfaces after 60s as `Request timed out`. This module makes the + * bound deliberate, applies it to the stalls that sit *outside* an SDK request + * too, and — the point of naming the URL in the error — says *which* endpoint + * stalled instead of blaming the handshake. + * + * ## The signal, the proxy hop, and why the race is still here + * + * The signal is the primary mechanism and it now reaches all the way out. On a + * direct fetch (CLI, TUI, backend) it cancels the request outright. In the + * browser the OAuth fetch is `createRemoteFetch`, which re-issues the call as a + * POST to `/api/fetch` — and as of #2319 it forwards the signal onto that hop, + * where the route composes `c.req.raw.signal` into its own outbound fetch. So + * an abort here tears down the backend's request to the authorization server + * rather than leaving it running detached, which is what it used to do. + * + * The race is kept anyway, and it is not redundant. It bounds what the signal + * cannot: a `fetchFn` that ignores `AbortSignal` altogether — an injected or + * test double, or any future transport that drops it the way the proxy used to + * — and a backend that is itself wedged, where the outbound request is + * cancelled but the hop back never completes. A deadline that depends on every + * layer beneath it honouring a signal is not a deadline. Same reasoning, same + * shape, as `revokeToken`. + */ + +import { redactUrlQuery } from "../mcp/fetchTracking.js"; + +/** + * 30 seconds. Deliberately generous: discovery against a slow or cold-starting + * authorization server is legitimate, and a bound that fires on a server that + * was going to answer is worse than the unbounded wait it replaced. It still + * halves the 60s the SDK handshake incidentally provides, and it is the only + * bound at all on the legs that sit outside an SDK request. + */ +export const DEFAULT_OAUTH_REQUEST_TIMEOUT_MS = 30_000; + +/** + * Raised when an OAuth-path request outlives its budget. + * + * Carries the URL because "discovery timed out" without one is only marginally + * better than a spinner: the OAuth path makes several requests to several + * hosts, and which of them stalled is the whole diagnostic. + */ +export class OAuthRequestTimeoutError extends Error { + /** + * The endpoint that stalled, with sensitive query values redacted. + * + * Redacted in the constructor rather than at each display site, so every + * consumer is covered by construction: this error's message is recorded + * verbatim by `createFetchTracker` into the Network log and the persisted + * session, and an OAuth endpoint can carry a `code`, an `access_token` or a + * `client_secret` in its query string — which `fetchTracking` already + * deliberately redacts everywhere else it records a URL (Copilot). The path + * and every non-sensitive parameter survive, so the endpoint stays + * identifiable, which was the point of naming it. + */ + readonly url: string; + readonly timeoutMs: number; + + constructor(url: string, timeoutMs: number) { + const safeUrl = redactUrlQuery(url); + super(`OAuth request to ${safeUrl} timed out after ${timeoutMs}ms`); + this.name = "OAuthRequestTimeoutError"; + this.url = safeUrl; + this.timeoutMs = timeoutMs; + } +} + +/** + * The largest delay `setTimeout` can actually schedule. Past `2 ** 31 - 1` the + * delay overflows a 32-bit signed integer and Node falls back to **1ms**, so an + * `Infinity` or an over-large budget produces an *immediate* timeout — the exact + * opposite of what the caller asked for (Copilot). + */ +const MAX_TIMER_DELAY_MS = 2_147_483_647; + +/** + * Coerce a caller's budget into something `setTimeout` can honour and the error + * can honestly report. + * + * - Non-finite (`NaN`, `±Infinity`) falls back to the default. `NaN` survives + * `Math.max(0, Math.round(...))` and `setTimeout(fn, NaN)` fires immediately, + * so a caller whose arithmetic produced a `NaN` would otherwise see every + * OAuth request fail at once with "timed out after NaNms". + * - Above `MAX_TIMER_DELAY_MS` it clamps rather than overflowing to 1ms. + * - Negative clamps to 0, which is a real budget (fire on the next tick), not + * an error. + * + * Rounded because the budget is *reported* — see the note at the call site. + */ +function normalizeBudget(timeoutMs: number): number { + if (!Number.isFinite(timeoutMs)) return DEFAULT_OAUTH_REQUEST_TIMEOUT_MS; + return Math.min(MAX_TIMER_DELAY_MS, Math.max(0, Math.round(timeoutMs))); +} + +/** + * How a proxy-side deadline is carried back to the browser (#2319). + * + * `/api/fetch` serializes its failures as a JSON error response, and + * `createRemoteFetch` turns any non-OK answer into a plain `Error` — so a + * timeout enforced by the *backend* would arrive as an untyped error and the + * `instanceof OAuthRequestTimeoutError` checks downstream would all be false + * (Copilot). Both ends run the same budget, so the client's race usually + * settles first; this matters when the backend's timer wins anyway — most + * plausibly a backgrounded tab, where the browser throttles `setTimeout` while + * the server's fires on schedule. Which of two equal deadlines happened to fire + * must not decide whether the error names its endpoint. So the route stamps + * this marker and `createRemoteFetch` reconstructs the typed error from it. + */ +export const OAUTH_TIMEOUT_WIRE_CODE = "oauth_request_timeout"; + +/** The body `/api/fetch` returns when its own deadline fired. */ +export interface OAuthRequestTimeoutWire { + code: typeof OAUTH_TIMEOUT_WIRE_CODE; + url: string; + timeoutMs: number; +} + +/** Whether a parsed `/api/fetch` error body is a proxy-side deadline. */ +export function isOAuthRequestTimeoutWire( + value: unknown, +): value is OAuthRequestTimeoutWire { + if (typeof value !== "object" || value === null) return false; + const candidate = value as Record; + return ( + candidate.code === OAUTH_TIMEOUT_WIRE_CODE && + typeof candidate.url === "string" && + typeof candidate.timeoutMs === "number" + ); +} + +/** + * Build the `isExempt` predicate for a chain that carries MCP traffic as well as + * OAuth work: it returns true for the MCP endpoint itself. + * + * ⚠️ **Fails open, deliberately.** The two errors are not symmetric. Bounding a + * request that should not be bounded severs a long-running tool call or an SSE + * stream — a severe, user-visible regression. Failing to bound one leaves the + * SDK's own per-request timeout as the backstop it has always been. So anything + * uncertain — no server URL yet, a URL that will not parse, either side — is + * treated as the MCP endpoint and exempted. + * + * The comparison is **as narrow as the transport allows**, which is not the same + * rule for both of them: + * + * - **Streamable HTTP** sends every MCP request to the configured URL, so + * `origin + pathname` is exact. Same-origin OAuth work — a protected-resource + * document, or an authorization server deployed beside the resource — is a + * different path and stays bounded, which is what #2319 asks for (Copilot). + * - **Legacy SSE** opens the configured URL (`/sse`), is handed a *separate* + * message endpoint by the server, and POSTs every JSON-RPC message to that + * second pathname. We never see that URL — it arrives inside the stream, in + * the SDK's own parser — so a path rule there bounds and body-buffers real + * MCP traffic, severing a slow tool call at 30s and reporting it as an + * `OAuthRequestTimeoutError`. Origin is the narrowest rule available, and it + * is exact rather than approximate: the SDK *enforces* that the message + * endpoint shares the stream URL's origin and rejects one that does not. + * + * So the residual gap is confined to legacy SSE against a server that also + * hosts its own OAuth endpoints, where transport-internal OAuth falls back to + * the SDK's per-request timeout — where it was before #2319. The Inspector's + * own discovery and token requests to those URLs are bounded regardless, on the + * OAuth chain. + * + * ⚠️ **Fails open** on every uncertainty, and the uncertainties differ by rule: + * no server URL, an unparseable URL on either side, or *not knowing which + * transport this is* all resolve to exempt — the last one by falling back to + * the wider origin rule. + */ +/** + * Read the transport hint, treating "absent" and "threw" alike as the wide + * case. A path rule applied to a connection that turns out to be legacy SSE + * severs real MCP traffic, so every uncertainty resolves toward the origin rule. + */ +function readNegotiatesEndpoint(hint: (() => boolean) | undefined): boolean { + if (!hint) return true; + try { + return hint(); + } catch { + return true; + } +} + +/** + * The media type every OAuth token-family request uses — the token exchange, + * the refresh, and revocation are all `application/x-www-form-urlencoded` per + * RFC 6749 §4.1.3 and RFC 7009 §2.1. MCP is JSON-RPC over `application/json` + * and never uses it, which is what makes this a safe discriminator rather than + * a heuristic. + */ +const OAUTH_FORM_MEDIA_TYPE = "application/x-www-form-urlencoded"; + +export function exemptMcpEndpoint( + getServerUrl: () => string | undefined, + /** + * Whether the message endpoint is negotiated rather than configured — true + * for legacy SSE. Read per call, since the transport can change between + * connects. Absent, or throwing, is treated as `true`. + */ + negotiatesEndpoint?: () => boolean, +): (url: string, headers: Headers) => boolean { + return (url, headers) => { + // A URL match is not by itself proof of MCP traffic. `oauthTokenUrl` takes + // any absolute URL, so a user may point it at the MCP endpoint's own URL, + // and the transport-internal refresh then travels this chain to a path the + // rules below would exempt — leaving the one request this whole change + // exists to bound running unbounded (Copilot). The token family is + // form-encoded and MCP never is, so the media type settles it before any + // URL comparison happens. + if ( + (headers.get("content-type") ?? "").split(";")[0].trim().toLowerCase() === + OAUTH_FORM_MEDIA_TYPE + ) { + return false; + } + + // `getServerUrl()` throws for a non-HTTP configuration, and this wrapped + // fetch can be handed to a transport or a custom factory that calls it + // anyway — propagating a configuration error out of a fetch would turn the + // documented fail-open into a hard failure (Copilot). Caught here for the + // same reason `readNegotiatesEndpoint` catches. + let serverUrl: string | undefined; + try { + serverUrl = getServerUrl(); + } catch { + return true; + } + if (!serverUrl) return true; + const negotiated = readNegotiatesEndpoint(negotiatesEndpoint); + try { + const a = new URL(url); + const b = new URL(serverUrl); + if (a.origin !== b.origin) return false; + return negotiated || a.pathname === b.pathname; + } catch { + return true; + } + }; +} + +/** + * 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's headers, whatever form `fetch`'s arguments took. `init` wins + * over a `Request`'s own, matching `fetch`. + */ +function requestHeadersOf( + input: RequestInfo | URL, + init: RequestInit | undefined, +): Headers { + if (init?.headers) return new Headers(init.headers); + if (typeof input !== "string" && !(input instanceof URL)) { + return new Headers(input.headers); + } + return new Headers(); +} + +/** + * Pull a reader to completion, discarding what it yields. An absent reader — a + * null-body response — is already complete. + * + * Read through an explicit reader rather than `clone().arrayBuffer()` so the + * losing side of the race has something it can cancel: `arrayBuffer()` locks + * the stream to an internal reader nothing else can reach. + */ +async function drainToEnd( + reader: ReadableStreamDefaultReader | undefined, +): Promise { + if (!reader) return; + for (;;) { + const { done } = await reader.read(); + if (done) return; + } +} + +/** + * Tear down both branches of the tee. + * + * Deliberately **not awaited**, and the `void`s say which of the documented + * cases this is: the callee owns its failures (each carries its own `catch`), + * and the caller genuinely cannot await — cancelling one branch of a tee can + * itself wait on the other, which would hold the timeout open for exactly as + * long as the stall it is meant to end. Best-effort on each besides: a stream + * already cancelled, errored or locked elsewhere is not a failure here, and + * this runs on a path that is already rejecting with something the caller cares + * about more. + */ +function cancelBoth( + reader: ReadableStreamDefaultReader | undefined, + response: Response | undefined, +): void { + void reader?.cancel().catch(() => {}); + void response?.body?.cancel().catch(() => {}); +} + +/** The request URL, whatever form `fetch`'s first argument took. */ +function requestUrlOf(input: RequestInfo | URL): string { + if (typeof input === "string") return input; + if (input instanceof URL) return input.href; + return input.url; +} + +/** + * The caller's own `AbortSignal`, if it supplied one. + * + * A `Request` carries its signal on the *input*, not in `init`, and this + * wrapper always passes a `signal` of its own in `init` — so reading `init` + * alone would silently override the embedded one and change ordinary `fetch` + * cancellation semantics for `withOAuthRequestTimeout(new Request(url, { signal }))` + * (Copilot). + * + * An `init.signal` still wins, but only when it is not `undefined`. `RequestInit` + * is a WebIDL dictionary, where a member present with the value `undefined` is + * converted as *absent* — so `{ ...base, signal: undefined }`, which is what a + * spread over an options object that had no signal produces, must inherit the + * `Request`'s signal rather than clear it (Copilot). An explicit `null` is the + * genuine "no signal" override and is honoured as one. + */ +export function callerSignalOf( + input: RequestInfo | URL, + init: RequestInit | undefined, +): AbortSignal | undefined { + const explicit = init?.signal; + if (explicit !== undefined) return explicit ?? undefined; + if (typeof input !== "string" && !(input instanceof URL)) return input.signal; + return undefined; +} + +/** + * Wrap a `fetch` so every call through it is bounded by `timeoutMs`. + * + * Every request that reaches it unexempted is bounded — so on a chain that + * carries anything other than OAuth work, `isExempt` is not optional. A + * Streamable HTTP or SSE response is a long-lived stream that is *supposed* to + * stay open, and a Streamable HTTP POST for a long-running tool call + * legitimately withholds its response headers for minutes; a deadline over + * either severs a working connection. `InspectorClient` uses the wrapper on + * both of its chains and the two differ only in this: the OAuth chain passes no + * predicate because every request on it is OAuth work, while the transport + * chain passes `exemptMcpEndpoint`. + * + * `isExempt` receives the request's URL **and headers**, because a URL alone + * cannot always tell the two kinds of traffic apart — see `exemptMcpEndpoint`. + * + * `isExempt` lets one wrapped fetch serve a mixed chain. It is how the + * *transport* fetch can be bounded at all: that chain carries both the SDK's + * OAuth work and the MCP traffic itself, and the MCP traffic must not be + * timed — a Streamable HTTP POST for a long-running tool call legitimately + * withholds its response headers for minutes, and an SSE body stays open + * indefinitely. So `InspectorClient` exempts the MCP endpoint by URL and bounds + * everything else on that chain, and **fails open**: if the server URL cannot + * be determined or parsed, everything is exempt. Bounding a request that should + * not be bounded breaks a working tool call; failing to bound one leaves the + * SDK's own per-request timeout as the backstop it already was. + * + * A caller's own signal is preserved — whether it arrived in `init` or embedded + * in a `Request` — and is both forwarded to the inner fetch (composed with + * `AbortSignal.any`) and raced here, so an outer cancellation still wins even on + * a fetch that drops the signal. This wrapper only ever *adds* a reason to give + * up; it never removes the caller's. + * + * The response **body** is drained through a clone under the same deadline, + * because `fetch` resolves on headers and a stalled body would otherwise be + * unbounded. The caller receives the original response, not a copy. + */ +export function withOAuthRequestTimeout( + fetchFn: typeof fetch, + timeoutMs: number = DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + isExempt?: (url: string, headers: Headers) => boolean, +): typeof fetch { + // Whole milliseconds. Not for the reason `revocation.ts:253` gives — that one + // calls `AbortSignal.timeout`, which really does throw `ERR_OUT_OF_RANGE` on + // a fractional delay (verified on Node 26) and so would fail before the + // request was sent. This wrapper drives its deadline with `setTimeout` and an + // `AbortController`, and `setTimeout` accepts a fractional delay and + // truncates it, so nothing here would throw (Copilot). + // + // The real reason is that the budget is *reported*: it is interpolated into + // the timeout message and exposed as `OAuthRequestTimeoutError.timeoutMs`. A + // caller whose budget came from a `performance.now()` subtraction would + // otherwise produce "timed out after 1000.4000000953674ms" and a non-integer + // public field. Rounding rather than flooring so a caller's own + // whole-millisecond timeout survives the trip through that clock and is still + // the number the message names. + const budget = normalizeBudget(timeoutMs); + + return async (input, init) => { + const url = requestUrlOf(input); + // An exempt request is passed through completely untouched — no deadline, + // no signal of ours, and crucially no body buffering, since the responses + // this exists for are streams that must stay open (see `isExempt` above). + if (isExempt?.(url, requestHeadersOf(input, init))) { + return fetchFn(input, init); + } + + const controller = new AbortController(); + const callerSignal = callerSignalOf(input, init); + // Before anything is constructed or sent. Racing an already-aborted signal + // would still evaluate `fetchFn(...)`, and a `fetchFn` that does not check + // an already-aborted signal — the proxy hop was one until #2319 — would + // send the OAuth request even though the caller cancelled before the call + // (Copilot). + if (callerSignal?.aborted) throw callerSignal.reason; + + const signal = callerSignal + ? AbortSignal.any([controller.signal, callerSignal]) + : controller.signal; + + let timer: ReturnType | undefined; + let timedOut = false; + let onCallerAbort: (() => void) | undefined; + + // The losing side of every race below: it rejects when the budget runs out, + // and when the caller cancels. + const abandoned = new Promise((_resolve, reject) => { + timer = setTimeout(() => { + timedOut = true; + // Abort first, which is what actually cancels the request — through + // the proxy hop too, since #2319 — then reject, so a `fetchFn` that + // ignores the signal is still abandoned on schedule. + controller.abort(new OAuthRequestTimeoutError(url, budget)); + reject(new OAuthRequestTimeoutError(url, budget)); + }, budget); + + // The caller's cancellation is raced too, not merely forwarded. Forwarding + // it is enough wherever the signal is honoured, but a `fetchFn` that + // ignores it would otherwise leave this promise pending for the whole + // budget instead of the outer cancellation winning as documented + // (Copilot). The caller's own `reason` is preserved, so it still sees its + // abort rather than a substituted error. + // Not aborted — the short-circuit above returned for that case. + if (callerSignal) { + onCallerAbort = () => reject(callerSignal.reason); + callerSignal.addEventListener("abort", onCallerAbort, { once: true }); + } + }); + + // 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); + + // Held so the losing path can tear both tee branches down — see below. + let settled: Response | undefined; + let drainReader: ReadableStreamDefaultReader | undefined; + + try { + const response = await Promise.race([ + fetchFn(input, nextInit), + abandoned, + ]); + settled = response; + // `fetch` resolves once the response *headers* arrive, so stopping here + // would leave the body unbounded: an authorization server can send + // headers, or half a JSON document, and then stall, and the caller's + // `response.json()` would hang with nothing watching it (Copilot). Every + // response on this path is a small, finite document — metadata, + // a registration, a token — so buffering it under the same deadline + // bounds the whole exchange. It is also the second reason `isExempt` is + // not optional on a mixed chain: an exempt request skips this buffering + // entirely, which is what lets an SSE body stay open rather than being + // drained into memory here. + // Read a *clone* under the deadline and hand back the original. Cloning + // tees the body, so draining one branch buffers the other: by the time + // this resolves the original is fully in memory and the caller reads it + // without touching the network again — the bound is the same, and the + // response the caller gets is the one `fetch` produced. + // + // The alternative, rebuilding a `Response` around the buffered bytes, + // could never be faithful (Copilot). `url`, `redirected` and `type` are + // internal slots; shadowing them as own properties satisfies a direct + // read and is lost again the moment a caller calls `clone()`, which + // returns a fresh native `Response`. The headers guard differs too, and + // the decoded bytes made the inherited `content-encoding` and + // `content-length` wrong, so those had to be stripped — a rebuilt + // response was observably not the original in at least four ways. Not + // rebuilding removes the whole class. + const copy = response.clone(); + drainReader = copy.body?.getReader(); + await Promise.race([drainToEnd(drainReader), abandoned]); + return response; + } catch (err) { + // ⚠️ Losing the race rejects the caller; on its own it does not stop the + // read. `clone()` tees the body, so a drain still running would keep + // pulling bytes *and* keep buffering them for the unread original branch + // — unbounded memory on exactly the signal-ignoring body the race exists + // to contain (Copilot). Both branches are cancelled here, which is safe + // precisely because this path throws: the caller never receives the + // response, so nothing is left to read it. + cancelBoth(drainReader, settled); + // `controller.abort()` above can reject the underlying fetch *before* + // `reject` runs, in which case the race settles with undici's + // `AbortError` instead of ours. Which of the two wins is an ordering + // detail of the fetch implementation, so normalize on the flag rather + // than on the error that surfaced: a timeout must always be reported as + // one, naming the endpoint. A caller-driven abort leaves the flag false + // and so passes through with its own reason intact. + if (timedOut) throw new OAuthRequestTimeoutError(url, budget); + throw err; + } finally { + /* v8 ignore next -- the Promise executor runs synchronously, so `timer` + is always assigned by the time this runs; the guard exists only because + TypeScript cannot see that. */ + if (timer !== undefined) clearTimeout(timer); + // Drop the listener even though it is `once`: a caller signal can outlive + // this request (one `AbortController` per connect attempt, several + // requests through it), and a listener per request would accumulate on it. + if (onCallerAbort && callerSignal) { + callerSignal.removeEventListener("abort", onCallerAbort); + } + } + }; +} diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index f2d16c0f..1ffc8e6b 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -268,14 +268,15 @@ export async function revokeToken( // outcome — and every caller has already cleared its local state by now, so // a rejection would break the documented best-effort guarantee. const { url, init } = buildRevocationRequest(params); - // The signal alone is not enough to bound this. In the browser the fetch is - // `createRemoteFetch`, which re-issues the call as a POST to `/api/fetch` - // and does not forward `init.signal`; the backend's outbound fetch gets no - // signal either. So a wedged authorization server would hold the teardown - // open indefinitely on exactly the path the timeout exists for. The race is - // what actually enforces the deadline; the signal is kept because it does - // cancel the direct-fetch paths (CLI, TUI, backend) rather than merely - // abandoning them. + // The signal cancels; the race is what guarantees the deadline. Since #2319 + // the signal reaches every path — `createRemoteFetch` forwards it onto the + // `/api/fetch` hop and that route composes it into its outbound fetch, so a + // browser-side abort now tears down the backend's request to the + // authorization server as well as the direct-fetch paths (CLI, TUI, + // backend) it always did. The race is still not redundant: it bounds a + // `fetchFn` that ignores `AbortSignal` altogether, and a backend that is + // itself wedged. Same reasoning, and the same shape, as + // `withOAuthRequestTimeout`. const response = await withDeadline( params.fetchFn(url, { ...init, signal: AbortSignal.timeout(timeoutMs) }), timeoutMs, @@ -307,11 +308,12 @@ export async function revokeToken( /** * Reject with a timeout error if `promise` has not settled within `timeoutMs`. * - * The underlying request is abandoned rather than cancelled — nothing here can - * cancel a fetch the proxy already stripped the signal from. That is the right - * trade for this caller: the point is that the *teardown* proceeds, and the - * response, if it ever arrives, is a revocation we no longer need to wait for. - * The timer is cleared on the settled path so a caller is never held awake by it. + * This abandons the promise; cancelling the request is the caller's signal's job + * (see `revokeToken`, where the two are used together). Abandonment alone is an + * acceptable floor for this caller even where the signal is ignored: the point + * is that the *teardown* proceeds, and the response, if it ever arrives, is a + * revocation we no longer need to wait for. The timer is cleared on the settled + * path so a caller is never held awake by it. */ async function withDeadline( promise: Promise, diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index cec72b49..29456ff4 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -256,6 +256,11 @@ import { } from "../auth/challenge.js"; import { withOAuthEndpointOverrides } from "../auth/endpointOverrides.js"; import { withRfc8414OidcCompat } from "../auth/oidcDiscoveryCompat.js"; +import { + DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + exemptMcpEndpoint, + withOAuthRequestTimeout, +} from "../auth/requestTimeout.js"; import type { TokenRevocationOutcome } from "../auth/revocation.js"; import type { OAuthTokens } from "@modelcontextprotocol/client"; import { silentLogger, type InspectorLogger } from "../logging/logger.js"; @@ -810,8 +815,9 @@ export class InspectorClient extends InspectorClientEventTarget { // `this.fetchFn` directly. The overrides are read lazily — `oauthManager` is // created a few lines below, and `setOAuthConfig` can change them later — so // the wrapper is inert until a server actually configures one. - this.fetchFn = withOAuthEndpointOverrides(this.fetchFn ?? fetch, () => - this.oauthManager?.getEndpointOverrides(), + const withOverrides = withOAuthEndpointOverrides( + this.fetchFn ?? fetch, + () => this.oauthManager?.getEndpointOverrides(), ); // #2172: recover discovery when a plain OAuth 2.0 authorization server // publishes RFC 8414 metadata at `/.well-known/openid-configuration`, which @@ -822,8 +828,40 @@ export class InspectorClient extends InspectorClientEventTarget { // still hit the upstream failure. The substituted response is stamped with // `COMPAT_SOURCE_HEADER` so a captured entry names the URL its body came // from rather than appearing to be a 200 from the RFC 8414 path. - this.fetchFn = withRfc8414OidcCompat(this.fetchFn); - this.effectiveAuthFetch = this.buildEffectiveAuthFetch(); + // The transport chain is bounded too, but only for the requests on it that + // are *not* MCP traffic — the OAuth work the SDK runs from inside the + // transport on the 401/refresh path, which otherwise waits out the SDK's + // incidental per-request timeout and reports a bare `Request timed out` + // naming no endpoint (Copilot). The MCP endpoint itself is exempt, and the + // predicate fails open: a long-running tool call withholding its response + // headers for minutes, or an SSE body that stays open, must never be timed + // here. Deadline innermost, for the same reason as the auth chain below. + this.fetchFn = withRfc8414OidcCompat( + withOAuthRequestTimeout( + withOverrides, + DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + exemptMcpEndpoint( + () => this.getServerUrl(), + // Legacy SSE negotiates its message endpoint inside the stream, so + // its path is unknowable here and the exemption has to widen to the + // origin. Streamable HTTP sends everything to the configured URL, so + // the path rule is exact there and same-origin OAuth stays bounded. + () => this.transportConfig?.type === "sse", + ), + ), + ); + // #2319: the auth chain is composed separately so the deadline sits + // *inside* the compat wrapper. Reusing `this.fetchFn` as the base would put + // it outside, and then a stalled OIDC probe would be reported under the + // URL of the RFC 8414 request that preceded it — defeating the whole point + // of naming the endpoint — while every probe in the loop shared that one + // request's budget (Copilot). Innermost here matches the CLI's + // `storedAuthFetch`. It also passes no `isExempt`, unlike the transport + // chain above: every request on this chain is OAuth work, so there is + // nothing here to exempt. + this.effectiveAuthFetch = this.buildEffectiveAuthFetch( + withRfc8414OidcCompat(withOAuthRequestTimeout(withOverrides)), + ); this.sessionId = options.sessionId; @@ -990,8 +1028,22 @@ export class InspectorClient extends InspectorClientEventTarget { ); } - private buildEffectiveAuthFetch(): typeof fetch { - const base = this.fetchFn ?? fetch; + /** + * @param base - the composed OAuth-path fetch, deadline innermost (#2319). + * + * Passed in rather than read off `this.fetchFn` because the two chains are + * bounded on different terms, not because one of them is unbounded. Both + * carry a deadline; the transport chain additionally exempts the MCP endpoint + * (`exemptMcpEndpoint`), since its bodies are streams meant to stay open and + * a long-running tool call may withhold its headers for minutes. Reusing it + * here would extend that exemption to the OAuth manager's own requests — and + * an authorization server published at the MCP endpoint's path would then go + * unbounded on the one chain that exists to bound it. + * + * The SDK's transport-internal discovery (the 401/refresh path) is covered by + * the transport chain's own deadline, not by this one. + */ + private buildEffectiveAuthFetch(base: typeof fetch): typeof fetch { // Capture auth response bodies (OAuth discovery, DCR, token exchange) so // they're inspectable in the Network tab. Token-exchange responses carry // `access_token` / `refresh_token`; the Network UI masks those (and other diff --git a/core/mcp/remote/createRemoteFetch.ts b/core/mcp/remote/createRemoteFetch.ts index 4dcbf689..d0757956 100644 --- a/core/mcp/remote/createRemoteFetch.ts +++ b/core/mcp/remote/createRemoteFetch.ts @@ -21,6 +21,12 @@ * the web client against a strict modern server, same as from the CLI/TUI. */ +import { + deadlineForRequestInit, + isOAuthRequestTimeoutWire, + OAuthRequestTimeoutError, +} from "../../auth/requestTimeout.js"; + export interface RemoteFetchOptions { /** Base URL of the remote server (e.g. http://localhost:3000) */ baseUrl: string; @@ -133,14 +139,68 @@ export function createRemoteFetch(options: RemoteFetchOptions): typeof fetch { reqHeaders["x-mcp-remote-auth"] = `Bearer ${options.authToken}`; } + // #2319: forward the caller's cancellation onto the proxy hop. Without it + // an abort — including the OAuth deadline's own — settled only the caller's + // promise: the POST stayed in flight, so the backend never saw its request + // cancelled and its outbound fetch to the authorization server was left + // running detached, holding a handler and a socket for as long as the + // server cared to stall (Copilot). `init.signal` wins when present and not + // `undefined` (a WebIDL dictionary member present as `undefined` converts + // as absent), otherwise a `Request` carries its own. + const explicitSignal = init?.signal; + const signal = + explicitSignal !== undefined + ? (explicitSignal ?? undefined) + : input instanceof Request + ? 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, }); if (!res.ok) { const text = await res.text(); + // A deadline the backend enforced arrives as an ordinary error response; + // rebuild the typed error from its marker so the `instanceof` checks + // downstream — most importantly the one that lets a stalled probe escape + // `withRfc8414OidcCompat` — still see a timeout for what it is (#2319, + // Copilot). + // + // Both ends run the same budget, so the client's race usually settles + // first and this path is not taken. It matters when the backend's timer + // wins anyway — most plausibly a backgrounded tab, where the browser + // throttles `setTimeout` while the server's fires on schedule. Without + // this, which of two equal deadlines happened to fire would decide + // whether the error carried its endpoint. + let parsed: unknown; + try { + parsed = JSON.parse(text); + } catch { + parsed = undefined; + } + if (isOAuthRequestTimeoutWire(parsed)) { + throw new OAuthRequestTimeoutError(parsed.url, parsed.timeoutMs); + } throw new Error(`Remote fetch failed (${res.status}): ${text}`); } diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 69439f6d..15179eb1 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -28,6 +28,8 @@ 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 { OAUTH_TIMEOUT_WIRE_CODE } from "../../../auth/requestTimeout.js"; +import { redactUrlQuery } from "../../fetchTracking.js"; import type { RemoteConnectRequest, RemoteSendRequest, @@ -1204,6 +1206,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; @@ -1211,11 +1215,75 @@ 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); } + // #2319: bound the outbound call, and release it when the browser gives up. + // This hop is where a client-side deadline used to stop being enforceable: + // the browser abandons its promise, but the request it made is served here, + // and an outbound fetch with neither a signal nor a timeout holds a handler + // and an authorization-server socket open indefinitely — once per timed-out + // retry (Copilot). `c.req.raw.signal` is the propagated cancellation, now + // that `createRemoteFetch` forwards it; the timer is the backstop for a + // client that vanishes without aborting, and it matches the client-side + // budget so the two agree on when this is hopeless. + const controller = new AbortController(); + // Set by the timer below and read in the `catch`. The fetch rejects with + // the signal's abort reason, but which of several aborts won is not worth + // inferring from the error — the flag says it directly, the same + // normalization `withOAuthRequestTimeout` uses. + let deadlineFired = false; + const clientSignal: AbortSignal | undefined = c.req.raw.signal; + const signal = clientSignal + ? AbortSignal.any([controller.signal, clientSignal]) + : controller.signal; + // Cleared in `finally`, before the handler returns. Safe because by then + // 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. + // ⚠️ 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. + // Clamped to what `setTimeout` can schedule: past 2**31-1 the delay + // overflows and Node falls back to 1ms, turning an over-large budget into + // an immediate timeout. Non-finite is ignored outright rather than + // defaulted — an envelope carrying `NaN` is a malformed request, and the + // safe reading of a malformed deadline is "no deadline". + const deadlineMs = + typeof requestedTimeoutMs === "number" && + Number.isFinite(requestedTimeoutMs) + ? Math.min(2_147_483_647, Math.max(0, Math.round(requestedTimeoutMs))) + : undefined; + // Redacted for the same reason `OAuthRequestTimeoutError` redacts: this + // message and this URL are handed back to the browser, recorded in the + // Network log and persisted, and an OAuth endpoint's query string can carry + // a `code`, an `access_token` or a `client_secret` (Copilot). + const safeUrl = redactUrlQuery(url); + const timer = + deadlineMs === undefined + ? undefined + : setTimeout(() => { + deadlineFired = true; + controller.abort( + new Error( + `proxied request to ${safeUrl} timed out after ${deadlineMs}ms`, + ), + ); + }, deadlineMs); + try { // Proxy-aware, not the bare global. This route is the browser's ONLY way // out to the network: the web client's `environment.fetch` is @@ -1228,6 +1296,7 @@ export function createRemoteApp( method, headers: new Headers(headers), body: reqBody, + signal, }); const resHeaders: Record = {}; @@ -1242,6 +1311,26 @@ export function createRemoteApp( let resBody: string | undefined; if (!isStream && res.body) { resBody = await res.text(); + } else if (isStream) { + // This route does not return the stream — it answers with JSON and no + // body — so nobody downstream owns it and nothing will ever read it. + // Left un-cancelled, an OAuth endpoint that sends event-stream or + // NDJSON headers and never closes would hold the upstream socket open + // for good, since the `finally` below clears the only deadline once + // this handler returns (Copilot). Best-effort, like `releaseBody` in + // `oidcDiscoveryCompat`: a body already consumed, locked or absent is + // not an error here. Measured, undici does reclaim an unread body on + // its own in this configuration — so this is belt and braces rather + // than a demonstrated leak, and it is kept because relying on that is + // an implementation detail of the fetch beneath us, not a contract. + // + // ⚠️ Started, not awaited. `ReadableStream.cancel()` adopts the + // underlying source's cancel promise, which is permitted never to + // settle — awaiting it would keep this handler pending forever, and on + // an unbounded request there is no timer to release it either + // (Copilot). The `void` is the documented case where the callee owns + // its failures, via the `catch` below, and the caller cannot await. + void res.body?.cancel().catch(() => {}); } return c.json({ @@ -1253,7 +1342,24 @@ export function createRemoteApp( }); } catch (err) { const msg = err instanceof Error ? err.message : String(err); + if (deadlineFired) { + // Stamped so the browser can rebuild an `OAuthRequestTimeoutError` + // rather than receiving a plain `Error` that no `instanceof` check + // downstream can recognize (Copilot). 504 because that is what this + // is — the gateway gave up on the upstream, not a fault in the route. + return c.json( + { + error: msg, + code: OAUTH_TIMEOUT_WIRE_CODE, + url: safeUrl, + timeoutMs: deadlineMs, + }, + 504, + ); + } return c.json({ error: msg }, 500); + } finally { + if (timer !== undefined) clearTimeout(timer); } });