fix: enforce the OAuth deadline across the proxy hop (#2319 round 5)
Two findings, both real. The race bounded only the caller's promise on the browser path. `createRemoteFetch` dropped the signal, so an abort — including the deadline's own — left the POST to /api/fetch in flight; the backend never saw its request cancelled and its outbound fetch ran on with neither signal nor timeout, holding a handler and an authorization-server socket per timed-out attempt. `createRemoteFetch` now forwards the caller's signal (init.signal when present and not undefined, else a Request's own), and the /api/fetch route composes `c.req.raw.signal` with a server-side deadline matching the client budget, cleared in `finally` so a streaming response is not severed. A probe's own timeout was swallowed by `withRfc8414OidcCompat`, which returned the preceding 404 and discarded the endpoint name the deadline exists to provide. A deadline the Inspector imposed is its own error, not the server's answer, so it escapes now; every other probe failure still falls back as documented. Nothing regresses: every candidate the loop probes is one the SDK's own buildDiscoveryUrls emits, so a stall there is one the SDK would have hit itself, unbounded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
42139c2d23
commit
d848300077
2 changed files with 142 additions and 16 deletions
|
|
@ -373,32 +373,25 @@ describe("withOAuthRequestTimeout", () => {
|
|||
expect(err.url).toBe(RFC8414);
|
||||
});
|
||||
|
||||
it("innermost, each leg is bounded on its own and the probe is abandoned", async () => {
|
||||
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 settled = wrapped(RFC8414).then(
|
||||
(response) => response,
|
||||
(err: unknown) => err,
|
||||
);
|
||||
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 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);
|
||||
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);
|
||||
expect(String(inner.mock.calls[1][0])).toBe(PROBES[0]);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -168,4 +168,77 @@ 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<typeof fetch>()
|
||||
.mockResolvedValue(ok(standardRemoteBody));
|
||||
const remoteFetch = createRemoteFetch({
|
||||
baseUrl: "http://remote.example",
|
||||
fetchFn: fetchFn as unknown as typeof fetch,
|
||||
});
|
||||
return { fetchFn, remoteFetch };
|
||||
}
|
||||
|
||||
function signalOf(fetchFn: ReturnType<typeof vi.fn<typeof fetch>>) {
|
||||
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();
|
||||
|
||||
await remoteFetch(
|
||||
new Request("http://upstream.example/", { signal: caller.signal }),
|
||||
);
|
||||
|
||||
expect(signalOf(fetchFn)).toBe(caller.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();
|
||||
|
||||
await remoteFetch(
|
||||
new Request("http://upstream.example/", { signal: caller.signal }),
|
||||
{ signal: undefined },
|
||||
);
|
||||
|
||||
expect(signalOf(fetchFn)).toBe(caller.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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue