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/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts new file mode 100644 index 00000000..05c92029 --- /dev/null +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -0,0 +1,186 @@ +import { describe, it, expect, vi, afterEach } from "vitest"; +import { + DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + OAuthRequestTimeoutError, + withOAuthRequestTimeout, +} from "@inspector/core/auth/requestTimeout.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 straight through", async () => { + const ok = new Response("{}", { status: 200 }); + const inner = vi.fn().mockResolvedValue(ok); + const wrapped = withOAuthRequestTimeout(inner, 1000); + + await expect(wrapped(URL_UNDER_TEST)).resolves.toBe(ok); + 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("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 signal is dropped in flight", async () => { + vi.useFakeTimers(); + // The browser's `createRemoteFetch` re-issues the call to `/api/fetch` and + // forwards no signal, so aborting locally cancels nothing. The race is what + // has to bound it. + 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("preserves a caller-supplied signal, which still wins on its own", 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("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, because setTimeout takes an integer", async () => { + vi.useFakeTimers(); + // A budget derived from `performance.now()` arrives fractional; a fractional + // delay throws ERR_OUT_OF_RANGE before the request is ever made. + 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("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, + }); + }); + + 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(null)), + 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/core/auth/index.ts b/core/auth/index.ts index c74702ce..6dd49b3b 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -51,6 +51,13 @@ export { withRfc8414OidcCompat, } from "./oidcDiscoveryCompat.js"; +// OAuth-path request deadline (#2319) +export { + DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + OAuthRequestTimeoutError, + withOAuthRequestTimeout, +} from "./requestTimeout.js"; + // Storage export type { OAuthStorage, diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts new file mode 100644 index 00000000..e89aa261 --- /dev/null +++ b/core/auth/requestTimeout.ts @@ -0,0 +1,150 @@ +/** + * 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. + * + * 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. + * + * ## Why a race, and not just the signal + * + * The signal alone cannot enforce this. In the browser the OAuth 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 on exactly the path this bound exists for — a browser session + * against a wedged authorization server — aborting the local promise would + * change nothing. 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. Same reasoning, same shape, as + * `revokeToken`. + */ + +/** + * 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 { + readonly url: string; + readonly timeoutMs: number; + + constructor(url: string, timeoutMs: number) { + super(`OAuth request to ${url} timed out after ${timeoutMs}ms`); + this.name = "OAuthRequestTimeoutError"; + this.url = url; + this.timeoutMs = timeoutMs; + } +} + +/** 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; +} + +/** + * Wrap a `fetch` so every call through it is bounded by `timeoutMs`. + * + * Apply this to an **OAuth-path** fetch only. It must never wrap the transport + * fetch: a Streamable HTTP or SSE response is a long-lived stream that is + * *supposed* to stay open, and a deadline there would sever every connection + * after the budget. `InspectorClient` keeps the two apart by wrapping inside + * `buildEffectiveAuthFetch` rather than wrapping `this.fetchFn`, which the + * transport is handed directly. + * + * A caller-supplied `init.signal` is preserved — the two are composed with + * `AbortSignal.any`, so an outer cancellation still wins and this wrapper only + * ever *adds* a reason to give up. + */ +export function withOAuthRequestTimeout( + fetchFn: typeof fetch, + timeoutMs: number = DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, +): typeof fetch { + // Whole milliseconds, because `setTimeout` and `AbortSignal.timeout` both + // take an integer: Node throws `ERR_OUT_OF_RANGE` on a fractional delay — + // before the fetch, so the request is never sent and the caller sees a failed + // operation rather than a timeout. A budget derived from `performance.now()` + // is fractional, so this is reachable. `revocation.ts:253` carries the same + // note for the same reason. + const budget = Math.max(0, Math.round(timeoutMs)); + + return async (input, init) => { + const url = requestUrlOf(input); + const controller = new AbortController(); + // `init.signal` is `AbortSignal | null | undefined`; only an actual signal + // is worth composing. + const callerSignal = init?.signal ?? undefined; + const signal = callerSignal + ? AbortSignal.any([controller.signal, callerSignal]) + : controller.signal; + + let timer: ReturnType | undefined; + let timedOut = false; + const deadline = new Promise((_resolve, reject) => { + timer = setTimeout(() => { + timedOut = true; + // Abort first so a direct fetch is actually cancelled rather than left + // running detached, then reject so the proxied fetch — which never saw + // the signal — is abandoned on schedule too. + controller.abort(new OAuthRequestTimeoutError(url, budget)); + reject(new OAuthRequestTimeoutError(url, budget)); + }, budget); + }); + + try { + return await Promise.race([ + fetchFn(input, { ...init, signal }), + deadline, + ]); + } catch (err) { + // `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. + 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); + } + }; +} diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index cec72b49..180243ad 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -256,6 +256,7 @@ import { } from "../auth/challenge.js"; import { withOAuthEndpointOverrides } from "../auth/endpointOverrides.js"; import { withRfc8414OidcCompat } from "../auth/oidcDiscoveryCompat.js"; +import { 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"; @@ -991,7 +992,15 @@ export class InspectorClient extends InspectorClientEventTarget { } private buildEffectiveAuthFetch(): typeof fetch { - const base = this.fetchFn ?? fetch; + // #2319: bound every OAuth-path request — discovery, dynamic client + // registration, the token exchange, the refresh. Applied here rather than + // to `this.fetchFn` because the transport is handed `this.fetchFn` + // directly, and a deadline on a Streamable HTTP / SSE response would sever + // the very stream it is supposed to hold open. The cost of that placement + // is that the discovery the SDK runs from *inside* the transport (the + // 401/refresh path) is not covered here; that leg sits inside an SDK + // request and is bounded by its per-request timeout. + const base = withOAuthRequestTimeout(this.fetchFn ?? 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