diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 05c92029..5100083b 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -17,12 +17,18 @@ afterEach(() => { }); describe("withOAuthRequestTimeout", () => { - it("passes a prompt response straight through", async () => { - const ok = new Response("{}", { status: 200 }); - const inner = vi.fn().mockResolvedValue(ok); + 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. + const inner = vi + .fn() + .mockResolvedValue(new Response('{"ok":true}', { status: 200 })); const wrapped = withOAuthRequestTimeout(inner, 1000); - await expect(wrapped(URL_UNDER_TEST)).resolves.toBe(ok); + const response = await wrapped(URL_UNDER_TEST); + + expect(response.status).toBe(200); + await expect(response.json()).resolves.toEqual({ ok: true }); expect(inner).toHaveBeenCalledTimes(1); }); @@ -103,7 +109,7 @@ describe("withOAuthRequestTimeout", () => { expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); }); - it("preserves a caller-supplied signal, which still wins on its own", async () => { + 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", () => @@ -119,6 +125,139 @@ describe("withOAuthRequestTimeout", () => { 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 on the browser's `createRemoteFetch` path, + // which drops 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 immediately when the caller's signal is already aborted", async () => { + const wrapped = withOAuthRequestTimeout(neverSettles, 60_000); + const reason = new Error("already gone"); + + await expect( + wrapped(URL_UNDER_TEST, { signal: AbortSignal.abort(reason) }), + ).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("returns a readable response whose metadata survives the rebuild", 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); + + expect(response.status).toBe(201); + expect(response.statusText).toBe("Created"); + expect(response.ok).toBe(true); + expect(response.headers.get("x-probe")).toBe("kept"); + expect(response.url).toBe(original.url); + expect(response.redirected).toBe(original.redirected); + expect(response.type).toBe(original.type); + await expect(response.json()).resolves.toEqual({ + issuer: "https://as.example.com", + }); + }); + + it("rebuilds a null-body status without handing it a body", async () => { + // `new Response(body, { status: 204 })` throws; the rebuild has to pass null. + const wrapped = withOAuthRequestTimeout( + vi + .fn() + .mockResolvedValue(new Response(null, { status: 204 })), + 1000, + ); + + const response = await wrapped(URL_UNDER_TEST); + + 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); @@ -173,7 +312,7 @@ describe("withOAuthRequestTimeout", () => { vi.useFakeTimers(); const clearSpy = vi.spyOn(globalThis, "clearTimeout"); const wrapped = withOAuthRequestTimeout( - vi.fn().mockResolvedValue(new Response(null)), + vi.fn().mockResolvedValue(new Response("{}")), 1000, ); diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index e89aa261..8fae3dcb 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -22,6 +22,13 @@ * 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 the body is buffered under the same race + * and the response rebuilt around it. + * * 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 @@ -77,6 +84,63 @@ function requestUrlOf(input: RequestInfo | URL): string { 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). + * + * `init.signal` still wins whenever the key is *present*, per the fetch spec: + * an explicit `null` there means "no signal" even when `input` is a `Request` + * that has one. + */ +function callerSignalOf( + input: RequestInfo | URL, + init: RequestInit | undefined, +): AbortSignal | undefined { + if (init && "signal" in init) return init.signal ?? undefined; + if (typeof input !== "string" && !(input instanceof URL)) return input.signal; + return undefined; +} + +/** + * Statuses the Fetch Standard defines as null-body. Handing the `Response` + * constructor a body with one of these throws a `TypeError`, so the rebuilt + * response below must pass `null` instead. + */ +const NULL_BODY_STATUSES = new Set([101, 103, 204, 205, 304]); + +/** + * Rebuild a `Response` around an already-buffered body. + * + * `url`, `redirected` and `type` are getters with no constructor option, and + * callers do read them — the RFC 8414/OIDC compat wrapper stamps a header + * naming the URL a body came from, and the SDK's discovery reports the + * responding URL in its errors. So they are copied across explicitly rather + * than silently reset to `""` / `false` / `"default"`. + */ +function rebuildResponse(response: Response, body: ArrayBuffer): Response { + const rebuilt = new Response( + NULL_BODY_STATUSES.has(response.status) ? null : body, + { + status: response.status, + statusText: response.statusText, + headers: response.headers, + }, + ); + for (const key of ["url", "redirected", "type"] as const) { + Object.defineProperty(rebuilt, key, { + value: response[key], + enumerable: true, + configurable: true, + }); + } + return rebuilt; +} + /** * Wrap a `fetch` so every call through it is bounded by `timeoutMs`. * @@ -87,9 +151,14 @@ function requestUrlOf(input: RequestInfo | URL): string { * `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. + * 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 buffered under the same deadline, because `fetch` + * resolves on headers and a stalled body would otherwise be unbounded. */ export function withOAuthRequestTimeout( fetchFn: typeof fetch, @@ -106,16 +175,18 @@ export function withOAuthRequestTimeout( 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 callerSignal = callerSignalOf(input, init); const signal = callerSignal ? AbortSignal.any([controller.signal, callerSignal]) : controller.signal; let timer: ReturnType | undefined; let timedOut = false; - const deadline = new Promise((_resolve, reject) => { + 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 so a direct fetch is actually cancelled rather than left @@ -124,20 +195,48 @@ export function withOAuthRequestTimeout( 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 on a direct fetch, but on the browser's `createRemoteFetch` + // path — the one this wrapper exists for — the signal is dropped in + // flight, so an abort 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. + if (callerSignal) { + if (callerSignal.aborted) { + reject(callerSignal.reason); + } else { + onCallerAbort = () => reject(callerSignal.reason); + callerSignal.addEventListener("abort", onCallerAbort, { once: true }); + } + } }); try { - return await Promise.race([ + const response = await Promise.race([ fetchFn(input, { ...init, signal }), - deadline, + abandoned, ]); + // `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. This is the other reason the wrapper must + // never be applied to the transport fetch, whose bodies are streams that + // are supposed to stay open. + const body = await Promise.race([response.arrayBuffer(), abandoned]); + return rebuildResponse(response, body); } 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. + // 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 { @@ -145,6 +244,12 @@ export function withOAuthRequestTimeout( 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); + } } }; }