diff --git a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts index 511f307b..14096711 100644 --- a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts +++ b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts @@ -543,6 +543,41 @@ describe("probe cancellation (#2319)", () => { 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("still falls back to the original response on an ordinary probe failure", async () => { const inner = vi.fn((input) => { if (String(input) === RFC8414) { diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 832a147b..1875d878 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -7,10 +7,11 @@ * - **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 rebuilt response.** Buffering means the caller gets a different - * `Response` object, so `url` / `redirected` / `type` are asserted to survive - * and `content-encoding` / `content-length` to be dropped — the buffer holds - * the decoded body, which makes both of those headers lies. + * - **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. @@ -44,8 +45,9 @@ afterEach(() => { describe("withOAuthRequestTimeout", () => { it("passes a prompt response through", async () => { - // Not the same object: the body is buffered under the deadline (see the - // stalled-body case below) and the response rebuilt around it. + // 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 })); diff --git a/core/auth/oidcDiscoveryCompat.ts b/core/auth/oidcDiscoveryCompat.ts index 760b3540..d6ca43d3 100644 --- a/core/auth/oidcDiscoveryCompat.ts +++ b/core/auth/oidcDiscoveryCompat.ts @@ -271,7 +271,18 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof 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`. - if (callerSignal?.aborted) throw callerSignal.reason; + // + // 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) { + await releaseBody(response); + throw callerSignal.reason; + } let probe: Response; try { @@ -287,13 +298,19 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // 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) throw err; + if (err instanceof OAuthRequestTimeoutError) { + await releaseBody(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) throw callerSignal.reason; + if (callerSignal?.aborted) { + await releaseBody(response); + throw callerSignal.reason; + } return response; } if (!probe.ok) { @@ -307,7 +324,10 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // 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) throw callerSignal.reason; + if (callerSignal?.aborted) { + await releaseBody(response); + throw callerSignal.reason; + } if (continuesDiscovery(probe.status)) continue; return response; } @@ -331,7 +351,10 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // 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) throw callerSignal.reason; + if (callerSignal?.aborted) { + await releaseBody(response); + throw callerSignal.reason; + } return response; } if (!isRfc8414OnlyMetadata(parsed)) {