fix: put the OAuth deadline inside the compat wrapper (#2319 round 3)
`buildEffectiveAuthFetch` took `this.fetchFn` as its base, which already carried `withRfc8414OidcCompat` — so the deadline sat outside it. A stalled OIDC probe was then timed on whatever budget the preceding RFC 8414 request had left, and reported under that request's URL rather than the probe's, defeating the endpoint-level diagnostic this change exists to add. The auth chain is now composed separately, deadline innermost, matching the CLI's storedAuthFetch: withRfc8414OidcCompat(withOAuthRequestTimeout(withOverrides)) `this.fetchFn` keeps the transport chain, unbounded, so an SSE or Streamable HTTP body is still free to stay open. Two tests pin the ordering by contrast: outside, a stalled probe is misreported under the preceding URL; innermost, it is bounded on its own budget and cannot be attributed to the RFC 8414 request. 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
2710c98681
commit
42139c2d23
2 changed files with 97 additions and 14 deletions
|
|
@ -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<typeof fetch>((input) => {
|
||||
if (String(input) === RFC8414) {
|
||||
return Promise.resolve(new Response(null, { status: 404 }));
|
||||
}
|
||||
return new Promise<Response>(() => {});
|
||||
});
|
||||
}
|
||||
|
||||
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");
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in a new issue