From 89d6af4436f168c3bee7c99ef53c1f2a45dc1c12 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 28 Aug 2026 01:59:04 -0400 Subject: [PATCH] fix(auth): mirror the SDK's candidate policy in the compat probe loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The probe loop advanced to the next OIDC candidate on any failure, which meant it could promote a later candidate over an earlier one the SDK would have stopped at — turning a failure into a success, a worse defect than the one being worked around. Four distinct cases: a 500 (an outage the SDK surfaces), a 2xx body that will not parse (terminal for the SDK), a network error (which the SDK propagates outside the browser's CORS case), and a non-JSON media type. It now continues only where the SDK's own loop does — a 4xx or 502 — and otherwise returns the original response, leaving the SDK to make the same request and reach its own verdict. The `content-type` gate is gone entirely, because the SDK parses a 2xx discovery body whatever media type it carries, so gating could skip a genuine OpenID provider document and substitute an RFC-8414-only one from a lower-priority candidate in its place. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- .../core/auth/oidcDiscoveryCompat.test.ts | 48 ++++++++++++++----- core/auth/oidcDiscoveryCompat.ts | 42 ++++++++-------- 2 files changed, 58 insertions(+), 32 deletions(-) diff --git a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts index 6e855629..47aaac15 100644 --- a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts +++ b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts @@ -203,17 +203,17 @@ describe("withRfc8414OidcCompat", () => { expect(base).toHaveBeenCalledTimes(2); }); - it("skips a candidate that is not JSON and keeps looking", async () => { + it("accepts a 2xx candidate whatever media type it carries", async () => { + // The SDK parses a 2xx discovery body regardless of `content-type`, so + // gating on it here could skip a document the SDK would have used (Copilot). vi.spyOn(console, "warn").mockImplementation(() => {}); const base = vi.fn(async (input) => { - const url = String(input); - if (url === OIDC_SUFFIXED) { - return new Response("login", { + if (String(input) === OIDC_SUFFIXED) { + return new Response(JSON.stringify(RFC8414_DOC), { status: 200, - headers: { "content-type": "text/html" }, + headers: { "content-type": "text/plain" }, }); } - if (url === OIDC_APPENDED) return json(RFC8414_DOC); return notFound(); }); const wrapped = withRfc8414OidcCompat(base); @@ -223,10 +223,12 @@ describe("withRfc8414OidcCompat", () => { ); }); - it("skips a candidate whose JSON body will not parse", async () => { + it("stops at a candidate whose body will not parse", async () => { + // Terminal for the SDK, so a later candidate must not be promoted over it + // — that would turn a failure into a success (Copilot). const original = notFound(); const base = vi.fn(async (input) => { - if (String(input).includes("openid-configuration")) { + if (String(input) === OIDC_SUFFIXED) { return new Response("{ not json", { status: 200, headers: { "content-type": "application/json" }, @@ -237,10 +239,12 @@ describe("withRfc8414OidcCompat", () => { const wrapped = withRfc8414OidcCompat(base); await expect(wrapped(RFC8414_URL)).resolves.toBe(original); - expect(base).toHaveBeenCalledTimes(3); + // The failed original and the first candidate only — the second is not + // reached. + expect(base).toHaveBeenCalledTimes(2); }); - it("skips a candidate that itself fails", async () => { + it("walks past a candidate that 404s, as the SDK does", async () => { const original = notFound(); const base = vi.fn().mockResolvedValue(original); const wrapped = withRfc8414OidcCompat(base); @@ -249,7 +253,27 @@ describe("withRfc8414OidcCompat", () => { expect(base).toHaveBeenCalledTimes(3); }); - it("treats a probe that throws as a candidate to skip", async () => { + it("stops at a candidate returning a status the SDK would not walk past", async () => { + // A 500 on the first OIDC candidate is an outage the SDK surfaces; hiding + // it behind a second candidate's document would be worse than the bug + // being worked around (Copilot). + const original = notFound(); + const base = vi.fn(async (input) => { + const url = String(input); + if (url === OIDC_SUFFIXED) return new Response("boom", { status: 500 }); + if (url === OIDC_APPENDED) return json(RFC8414_DOC); + return original; + }); + const wrapped = withRfc8414OidcCompat(base); + + await expect(wrapped(RFC8414_URL)).resolves.toBe(original); + expect(base).toHaveBeenCalledTimes(2); + }); + + it("stops when a probe throws", async () => { + // The SDK swallows only the browser's CORS `TypeError`; elsewhere a network + // error propagates. Leave it to the SDK to make the same request and reach + // its own verdict (Copilot). const original = notFound(); const base = vi.fn(async (input) => { if (String(input).includes("openid-configuration")) { @@ -260,7 +284,7 @@ describe("withRfc8414OidcCompat", () => { const wrapped = withRfc8414OidcCompat(base); await expect(wrapped(RFC8414_URL)).resolves.toBe(original); - expect(base).toHaveBeenCalledTimes(3); + expect(base).toHaveBeenCalledTimes(2); }); it("probes on a 502, which the SDK also walks past", async () => { diff --git a/core/auth/oidcDiscoveryCompat.ts b/core/auth/oidcDiscoveryCompat.ts index 5f1fa93f..19b80141 100644 --- a/core/auth/oidcDiscoveryCompat.ts +++ b/core/auth/oidcDiscoveryCompat.ts @@ -160,19 +160,6 @@ function continuesDiscovery(status: number): boolean { return (status >= 400 && status < 500) || status === 502; } -/** - * Whether a `content-type` names a whole JSON document. - * - * An exact media-type test rather than a substring search for "json", for the - * same reason `endpointOverrides.ts` uses one: a streaming media type would be - * read to completion and never resolve. - */ -function isJsonDocumentResponse(contentType: string | null): boolean { - if (!contentType) return false; - const mediaType = contentType.split(";")[0].trim().toLowerCase(); - return mediaType === "application/json" || mediaType.endsWith("+json"); -} - /** * Whether a parsed body is RFC 8414 authorization-server metadata that is *not* * a valid OpenID provider document — the exact shape the upstream schema @@ -252,26 +239,41 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { if (candidates.length === 0) return response; const headers = discoveryHeaders(input, init); + // The loop advances to the next candidate only where the SDK's own loop + // would. Anywhere else it hands the original response back and lets + // discovery run its normal course, because *promoting a later candidate + // over an earlier one the SDK would have stopped at* would turn a failure + // into a success — a much worse defect than the one being worked around + // (Copilot). Concretely: a 500 on the first OIDC candidate is an outage the + // SDK surfaces, a malformed JSON body there is terminal for it, and a + // network error propagates outside the browser's CORS case. In each of + // those, returning `response` leaves the SDK to make the same request and + // reach the same verdict it always would. for (const candidate of candidates) { let probe: Response; try { probe = await fetchFn(candidate, { headers }); } catch { - // A network or CORS failure on a URL the SDK had not asked for yet is - // not this wrapper's to report — the SDK will make the same request and - // handle it. Try the next candidate. - continue; + return response; + } + if (!probe.ok) { + if (continuesDiscovery(probe.status)) continue; + return response; } - if (!probe.ok) continue; - if (!isJsonDocumentResponse(probe.headers.get("content-type"))) continue; + // Deliberately not gated on `content-type`: the SDK parses a 2xx + // discovery body whatever media type it carries, so gating here could + // skip a genuine OIDC document served with an odd one and substitute a + // later RFC-8414-only document in its place. A body that will not parse + // falls through to `return response` below, where the SDK re-fetches it + // and raises its own parse error. let parsed: unknown; let body: string; try { body = await probe.text(); parsed = JSON.parse(body); } catch { - continue; + return response; } if (!isRfc8414OnlyMetadata(parsed)) { // Either not metadata at all, or a genuine OpenID provider document the