diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 190a2640..df8bbb74 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -4,6 +4,7 @@ import { OAuthRequestTimeoutError, 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"; @@ -329,6 +330,78 @@ describe("withOAuthRequestTimeout", () => { }); }); + 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, each leg is bounded on its own and the probe is abandoned", async () => { + vi.useFakeTimers(); + const inner = stallingProbeFetch(); + const wrapped = withRfc8414OidcCompat( + withOAuthRequestTimeout(inner, 1000), + ); + + const settled = wrapped(RFC8414).then( + (response) => response, + (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 result = await settled; + // The compat wrapper is deliberately inert when a probe *throws* — it + // hands the original response back and lets discovery run its normal + // course — so a timed-out probe surfaces as the original 404 rather than + // as an error, and the second candidate is not tried. What innermost buys + // is that the probe is bounded at all, and that a stall in it can never be + // attributed to the RFC 8414 request the caller actually made. + expect(result).toBeInstanceOf(Response); + expect((result as Response).status).toBe(404); + expect(inner).toHaveBeenCalledTimes(2); + expect(String(inner.mock.calls[1][0])).toBe(PROBES[0]); + }); + }); + it("clears its timer once the request settles", async () => { vi.useFakeTimers(); const clearSpy = vi.spyOn(globalThis, "clearTimeout"); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 180243ad..7ccbefa3 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -811,8 +811,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 @@ -823,8 +824,18 @@ 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(); + this.fetchFn = withRfc8414OidcCompat(withOverrides); + // #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`. The transport keeps `this.fetchFn`, deliberately + // unbounded: its bodies are streams that are supposed to stay open. + this.effectiveAuthFetch = this.buildEffectiveAuthFetch( + withRfc8414OidcCompat(withOAuthRequestTimeout(withOverrides)), + ); this.sessionId = options.sessionId; @@ -991,16 +1002,15 @@ export class InspectorClient extends InspectorClientEventTarget { ); } - private buildEffectiveAuthFetch(): typeof 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); + /** + * @param base - the composed OAuth-path fetch, deadline innermost (#2319). + * Passed in rather than read off `this.fetchFn`, which is the *transport* + * chain and must stay unbounded. The cost of keeping the two apart is that + * the discovery the SDK runs from inside the transport (the 401/refresh path) + * is not covered by the deadline; that leg sits inside an SDK request and is + * bounded by its per-request timeout. + */ + 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