Skip to content

Commit 89d6af4

Browse files
cliffhallclaude
andcommitted
fix(auth): mirror the SDK's candidate policy in the compat probe loop
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall <cliff@futurescale.com>
1 parent 28c7a9f commit 89d6af4

2 files changed

Lines changed: 58 additions & 32 deletions

File tree

‎clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts‎

Lines changed: 36 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -203,17 +203,17 @@ describe("withRfc8414OidcCompat", () => {
203203
expect(base).toHaveBeenCalledTimes(2);
204204
});
205205

206-
it("skips a candidate that is not JSON and keeps looking", async () => {
206+
it("accepts a 2xx candidate whatever media type it carries", async () => {
207+
// The SDK parses a 2xx discovery body regardless of `content-type`, so
208+
// gating on it here could skip a document the SDK would have used (Copilot).
207209
vi.spyOn(console, "warn").mockImplementation(() => {});
208210
const base = vi.fn<typeof fetch>(async (input) => {
209-
const url = String(input);
210-
if (url === OIDC_SUFFIXED) {
211-
return new Response("<html>login</html>", {
211+
if (String(input) === OIDC_SUFFIXED) {
212+
return new Response(JSON.stringify(RFC8414_DOC), {
212213
status: 200,
213-
headers: { "content-type": "text/html" },
214+
headers: { "content-type": "text/plain" },
214215
});
215216
}
216-
if (url === OIDC_APPENDED) return json(RFC8414_DOC);
217217
return notFound();
218218
});
219219
const wrapped = withRfc8414OidcCompat(base);
@@ -223,10 +223,12 @@ describe("withRfc8414OidcCompat", () => {
223223
);
224224
});
225225

226-
it("skips a candidate whose JSON body will not parse", async () => {
226+
it("stops at a candidate whose body will not parse", async () => {
227+
// Terminal for the SDK, so a later candidate must not be promoted over it
228+
// — that would turn a failure into a success (Copilot).
227229
const original = notFound();
228230
const base = vi.fn<typeof fetch>(async (input) => {
229-
if (String(input).includes("openid-configuration")) {
231+
if (String(input) === OIDC_SUFFIXED) {
230232
return new Response("{ not json", {
231233
status: 200,
232234
headers: { "content-type": "application/json" },
@@ -237,10 +239,12 @@ describe("withRfc8414OidcCompat", () => {
237239
const wrapped = withRfc8414OidcCompat(base);
238240

239241
await expect(wrapped(RFC8414_URL)).resolves.toBe(original);
240-
expect(base).toHaveBeenCalledTimes(3);
242+
// The failed original and the first candidate only — the second is not
243+
// reached.
244+
expect(base).toHaveBeenCalledTimes(2);
241245
});
242246

243-
it("skips a candidate that itself fails", async () => {
247+
it("walks past a candidate that 404s, as the SDK does", async () => {
244248
const original = notFound();
245249
const base = vi.fn<typeof fetch>().mockResolvedValue(original);
246250
const wrapped = withRfc8414OidcCompat(base);
@@ -249,7 +253,27 @@ describe("withRfc8414OidcCompat", () => {
249253
expect(base).toHaveBeenCalledTimes(3);
250254
});
251255

252-
it("treats a probe that throws as a candidate to skip", async () => {
256+
it("stops at a candidate returning a status the SDK would not walk past", async () => {
257+
// A 500 on the first OIDC candidate is an outage the SDK surfaces; hiding
258+
// it behind a second candidate's document would be worse than the bug
259+
// being worked around (Copilot).
260+
const original = notFound();
261+
const base = vi.fn<typeof fetch>(async (input) => {
262+
const url = String(input);
263+
if (url === OIDC_SUFFIXED) return new Response("boom", { status: 500 });
264+
if (url === OIDC_APPENDED) return json(RFC8414_DOC);
265+
return original;
266+
});
267+
const wrapped = withRfc8414OidcCompat(base);
268+
269+
await expect(wrapped(RFC8414_URL)).resolves.toBe(original);
270+
expect(base).toHaveBeenCalledTimes(2);
271+
});
272+
273+
it("stops when a probe throws", async () => {
274+
// The SDK swallows only the browser's CORS `TypeError`; elsewhere a network
275+
// error propagates. Leave it to the SDK to make the same request and reach
276+
// its own verdict (Copilot).
253277
const original = notFound();
254278
const base = vi.fn<typeof fetch>(async (input) => {
255279
if (String(input).includes("openid-configuration")) {
@@ -260,7 +284,7 @@ describe("withRfc8414OidcCompat", () => {
260284
const wrapped = withRfc8414OidcCompat(base);
261285

262286
await expect(wrapped(RFC8414_URL)).resolves.toBe(original);
263-
expect(base).toHaveBeenCalledTimes(3);
287+
expect(base).toHaveBeenCalledTimes(2);
264288
});
265289

266290
it("probes on a 502, which the SDK also walks past", async () => {

‎core/auth/oidcDiscoveryCompat.ts‎

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -160,19 +160,6 @@ function continuesDiscovery(status: number): boolean {
160160
return (status >= 400 && status < 500) || status === 502;
161161
}
162162

163-
/**
164-
* Whether a `content-type` names a whole JSON document.
165-
*
166-
* An exact media-type test rather than a substring search for "json", for the
167-
* same reason `endpointOverrides.ts` uses one: a streaming media type would be
168-
* read to completion and never resolve.
169-
*/
170-
function isJsonDocumentResponse(contentType: string | null): boolean {
171-
if (!contentType) return false;
172-
const mediaType = contentType.split(";")[0].trim().toLowerCase();
173-
return mediaType === "application/json" || mediaType.endsWith("+json");
174-
}
175-
176163
/**
177164
* Whether a parsed body is RFC 8414 authorization-server metadata that is *not*
178165
* a valid OpenID provider document — the exact shape the upstream schema
@@ -252,26 +239,41 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch {
252239
if (candidates.length === 0) return response;
253240

254241
const headers = discoveryHeaders(input, init);
242+
// The loop advances to the next candidate only where the SDK's own loop
243+
// would. Anywhere else it hands the original response back and lets
244+
// discovery run its normal course, because *promoting a later candidate
245+
// over an earlier one the SDK would have stopped at* would turn a failure
246+
// into a success — a much worse defect than the one being worked around
247+
// (Copilot). Concretely: a 500 on the first OIDC candidate is an outage the
248+
// SDK surfaces, a malformed JSON body there is terminal for it, and a
249+
// network error propagates outside the browser's CORS case. In each of
250+
// those, returning `response` leaves the SDK to make the same request and
251+
// reach the same verdict it always would.
255252
for (const candidate of candidates) {
256253
let probe: Response;
257254
try {
258255
probe = await fetchFn(candidate, { headers });
259256
} catch {
260-
// A network or CORS failure on a URL the SDK had not asked for yet is
261-
// not this wrapper's to report — the SDK will make the same request and
262-
// handle it. Try the next candidate.
263-
continue;
257+
return response;
258+
}
259+
if (!probe.ok) {
260+
if (continuesDiscovery(probe.status)) continue;
261+
return response;
264262
}
265-
if (!probe.ok) continue;
266-
if (!isJsonDocumentResponse(probe.headers.get("content-type"))) continue;
267263

264+
// Deliberately not gated on `content-type`: the SDK parses a 2xx
265+
// discovery body whatever media type it carries, so gating here could
266+
// skip a genuine OIDC document served with an odd one and substitute a
267+
// later RFC-8414-only document in its place. A body that will not parse
268+
// falls through to `return response` below, where the SDK re-fetches it
269+
// and raises its own parse error.
268270
let parsed: unknown;
269271
let body: string;
270272
try {
271273
body = await probe.text();
272274
parsed = JSON.parse(body);
273275
} catch {
274-
continue;
276+
return response;
275277
}
276278
if (!isRfc8414OnlyMetadata(parsed)) {
277279
// Either not metadata at all, or a genuine OpenID provider document the

0 commit comments

Comments
 (0)