From 4c6a9fb376f115cce764fc97f0b446787d144a18 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 01:16:30 -0400 Subject: [PATCH] fix: bind CIMD provenance to the issuer it was discovered for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review (#2287): the active-issuer fallback added last round could copy issuer A's CIMD provenance onto a real dynamic registration for issuer B, since RFC 7591 §3.2 permits B to mint the same opaque URL as its `client_id`. Fix it at the source rather than in the resolver. `ensureCimdClientRegistration` now runs discovery first and records the registration against the issuer it just discovered, having confirmed *that* AS advertises `client_id_metadata_document_supported`. Its "already registered?" check moves after discovery and is keyed by that issuer — read ctx-less it resolved through the active issuer and early-returned for every later one, which is what forced the cross-issuer fallback in the first place. `resolveSdkRegistrationKind` is therefore issuer-scoped again, with no fallback. A second AS behind one resource now gets its own determination: CIMD when it advertises CIMD, `dcr` otherwise — including when it mints the metadata URL as its own `client_id`. The cost is a discovery round trip per connect attempt rather than only the first; noted at the call site. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA Signed-off-by: cliffhall --- clients/web/src/test/core/auth/cimd.test.ts | 47 +++++-- .../web/src/test/core/auth/providers.test.ts | 115 ++++++++++++++---- core/auth/cimd.ts | 30 ++++- core/auth/providers.ts | 67 +++++----- 4 files changed, 189 insertions(+), 70 deletions(-) diff --git a/clients/web/src/test/core/auth/cimd.test.ts b/clients/web/src/test/core/auth/cimd.test.ts index 63bc64a5..15aebd40 100644 --- a/clients/web/src/test/core/auth/cimd.test.ts +++ b/clients/web/src/test/core/auth/cimd.test.ts @@ -62,12 +62,15 @@ describe("ensureCimdClientRegistration", () => { fetchFn, }); + // #2242: the record is bound to the issuer just discovered, so a second AS + // behind the same resource gets its own CIMD determination rather than + // inheriting this one. expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER_URL, { client_id: METADATA_URL, }, - { registrationKind: "cimd" }, + { registrationKind: "cimd", issuer: "http://127.0.0.1:9999" }, ); }); @@ -154,22 +157,52 @@ describe("ensureCimdClientRegistration", () => { expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER_URL, { client_id: METADATA_URL }, - { registrationKind: "cimd" }, + { registrationKind: "cimd", issuer: "http://127.0.0.1:9999" }, ); }); - it("no-ops when client information is already stored", async () => { - storage.getClientInformation = vi.fn(async () => ({ - client_id: "existing-client", - })); + it("no-ops when client information is already stored for the discovered issuer", async () => { + // Dynamic slot only — a preregistered hit would short-circuit + // `clientInformation()` before it ever reaches the issuer-keyed read. + storage.getClientInformation = vi.fn( + async (_url: string, preregistered?: boolean) => + preregistered ? undefined : { client_id: "existing-client" }, + ); + + const fetchFn = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("/.well-known/oauth-protected-resource")) { + return new Response(JSON.stringify({ resource: SERVER_URL })); + } + if (url.includes("/.well-known/oauth-authorization-server")) { + return new Response( + JSON.stringify({ + issuer: "http://127.0.0.1:9999", + authorization_endpoint: "http://127.0.0.1:9999/oauth/authorize", + token_endpoint: "http://127.0.0.1:9999/oauth/token", + response_types_supported: ["code"], + client_id_metadata_document_supported: true, + }), + ); + } + throw new Error(`unexpected fetch: ${url}`); + }); const provider = createProvider(storage); await ensureCimdClientRegistration({ serverUrl: SERVER_URL, provider, - fetchFn: vi.fn(), + fetchFn, }); expect(storage.saveClientInformation).not.toHaveBeenCalled(); + // #2242: the existing-client check is keyed by the issuer discovery just + // resolved, not read ctx-less — a ctx-less read resolves through the + // *active* issuer and would early-return for every later issuer. + expect(storage.getClientInformation).toHaveBeenCalledWith( + SERVER_URL, + false, + "http://127.0.0.1:9999", + ); }); }); diff --git a/clients/web/src/test/core/auth/providers.test.ts b/clients/web/src/test/core/auth/providers.test.ts index 928c8f8e..c560b5cc 100644 --- a/clients/web/src/test/core/auth/providers.test.ts +++ b/clients/web/src/test/core/auth/providers.test.ts @@ -10,6 +10,7 @@ import type { OAuthStorage } from "@inspector/core/auth/storage.js"; import { OAuthStorageBase } from "@inspector/core/auth/oauth-storage.js"; import { OAuthMemoryStore } from "@inspector/core/auth/store.js"; import type { OAuthPersistBackend } from "@inspector/core/auth/oauth-persist.js"; +import { ensureCimdClientRegistration } from "@inspector/core/auth/cimd.js"; import { BrowserNavigation, BrowserOAuthClientProvider, @@ -778,9 +779,12 @@ describe("OAuthNavigation", () => { ); }); - // SEP-2352 keys registrations per authorization server. Driven against a - // real `OAuthStorageBase` rather than mocks, because the bug is in how - // the *storage* promotes and clears slots across issuers (Copilot). + // SEP-2352 keys registrations per authorization server, so a second AS + // behind one resource is a separate determination. Driven against a real + // `OAuthStorageBase` and the real `ensureCimdClientRegistration`, because + // the behaviour under test is how the pre-registration binds provenance to + // a discovered issuer and how storage promotes and clears slots — neither + // of which a mock would express (Copilot). describe("across two authorization servers", () => { const ISSUER_B = "https://as-b.example.com"; @@ -792,38 +796,71 @@ describe("OAuthNavigation", () => { return new OAuthStorageBase(new OAuthMemoryStore(), backend); } - async function bindFirstIssuer(storage: OAuthStorage) { + /** + * Discovery that points the resource at `issuer` as its authorization + * server and declares CIMD support per `cimd`. The RFC 9728 document + * has to name the AS, so that the RFC 8414 §3.3 issuer echo the SDK + * enforces resolves against the AS URL rather than the resource's. + */ + function discoveryFetch(issuer: string, cimd: boolean) { + return (async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("/.well-known/oauth-protected-resource")) { + return new Response( + JSON.stringify({ + resource: SERVER, + authorization_servers: [issuer], + }), + ); + } + if (url.startsWith(issuer)) { + return new Response( + JSON.stringify({ + issuer, + authorization_endpoint: `${issuer}/authorize`, + token_endpoint: `${issuer}/token`, + response_types_supported: ["code"], + ...(cimd && { + client_id_metadata_document_supported: true, + }), + }), + ); + } + throw new Error(`unexpected fetch: ${url}`); + }) as unknown as typeof fetch; + } + + /** Issuer A pre-registers via CIMD, then the SDK binds it. */ + async function bindIssuerA(storage: OAuthStorage) { const provider = makeProvider(storage, vi.fn(), { clientMetadataUrl: METADATA_URL, }); - // Our own pre-registration writes the unkeyed slot... - await provider.saveClientInformation( - { client_id: METADATA_URL }, - { registrationKind: "cimd" }, - ); - // ...which the SDK's first issuer-stamped save promotes into - // issuer A's slot, clearing the unkeyed fallback. + await ensureCimdClientRegistration({ + serverUrl: SERVER, + provider, + fetchFn: discoveryFetch(ISSUER, true), + }); await provider.saveClientInformation( { client_id: METADATA_URL }, { issuer: ISSUER }, ); - return provider; - } - - it("keeps cimd when the resource resolves to a second issuer", async () => { - const storage = makeRealStorage(); - const provider = await bindFirstIssuer(storage); - expect( await storage.getClientRegistrationKind(SERVER, ISSUER), ).toBe("cimd"); - // The precondition that made this go wrong: nothing is stored for - // issuer B, and the unkeyed fallback is gone. - expect( - await storage.getClientInformation(SERVER, false, ISSUER_B), - ).toBeUndefined(); + return provider; + } - // The SDK's own CIMD branch, saving under the second issuer. + it("keeps cimd when a second CIMD-supporting issuer takes over", async () => { + const storage = makeRealStorage(); + const provider = await bindIssuerA(storage); + + // Issuer B also advertises CIMD, so the pre-registration records it + // for B too — it must not early-return on issuer A's client. + await ensureCimdClientRegistration({ + serverUrl: SERVER, + provider, + fetchFn: discoveryFetch(ISSUER_B, true), + }); await provider.saveClientInformation( { client_id: METADATA_URL }, { issuer: ISSUER_B }, @@ -837,9 +874,35 @@ describe("OAuthNavigation", () => { ).toBe("cimd"); }); - it("still records dcr for a second issuer that mints its own client_id", async () => { + it("records dcr when a second issuer without CIMD mints the same URL as its client_id", async () => { const storage = makeRealStorage(); - const provider = await bindFirstIssuer(storage); + const provider = await bindIssuerA(storage); + + // Issuer B does *not* advertise CIMD, so nothing is recorded for B... + await ensureCimdClientRegistration({ + serverUrl: SERVER, + provider, + fetchFn: discoveryFetch(ISSUER_B, false), + }); + // ...and RFC 7591 §3.2 lets it mint an opaque id that happens to be + // the very URL issuer A uses as its CIMD client_id. + await provider.saveClientInformation( + { client_id: METADATA_URL }, + { issuer: ISSUER_B }, + ); + + expect( + await storage.getClientRegistrationKind(SERVER, ISSUER_B), + ).toBe("dcr"); + // Issuer A's own provenance is untouched. + expect( + await storage.getClientRegistrationKind(SERVER, ISSUER), + ).toBe("cimd"); + }); + + it("records dcr for a second issuer that mints its own client_id", async () => { + const storage = makeRealStorage(); + const provider = await bindIssuerA(storage); await provider.saveClientInformation( { client_id: "b-registered-id" }, diff --git a/core/auth/cimd.ts b/core/auth/cimd.ts index 31614d1b..5074f37c 100644 --- a/core/auth/cimd.ts +++ b/core/auth/cimd.ts @@ -27,9 +27,6 @@ export async function ensureCimdClientRegistration(params: { const clientMetadataUrl = params.provider.clientMetadataUrl?.trim(); if (!clientMetadataUrl) return; - const existing = await params.provider.clientInformation(); - if (existing?.client_id) return; - let resourceMetadata; try { resourceMetadata = await discoverOAuthProtectedResourceMetadata( @@ -56,10 +53,37 @@ export async function ensureCimdClientRegistration(params: { ); if (!metadata?.client_id_metadata_document_supported) return; + // SEP-2352 keys a registration to the authorization server that issued it, so + // the record this writes is bound to the issuer we just discovered rather than + // to the server as a whole. That binding is what makes the provenance + // trustworthy later: `BaseOAuthClientProvider.saveClientInformation` preserves + // `cimd` only for an issuer this function recorded it for, having first + // confirmed *that* AS advertises `client_id_metadata_document_supported` + // (#2242, Copilot). A second AS behind the same resource therefore gets its own + // determination — pre-registered here when it too supports CIMD, and left to + // dynamic registration when it does not. + // + // ⚠️ This is why the "do we already have a client?" check below sits *after* + // discovery rather than short-circuiting it, at the cost of a discovery round + // trip on each connect attempt rather than only the first. Read ctx-less — as + // it was — it resolves through the *active* issuer and so early-returns for + // every subsequent issuer, leaving them with no CIMD record at all. It still + // answers the static case first, since `clientInformation` checks the + // preregistered slot before any issuer slot. + const issuer = metadata.issuer; + const existing = await params.provider.clientInformation( + issuer ? { issuer } : undefined, + ); + if (existing?.client_id) return; + const clientInformation: OAuthClientInformation = { client_id: clientMetadataUrl, }; await params.provider.saveClientInformation(clientInformation, { registrationKind: "cimd", + // An AS metadata document without an `issuer` is malformed (RFC 8414 §2), + // but the type allows it; fall back to the unkeyed slot rather than + // inventing a key. + ...(issuer && { issuer }), }); } diff --git a/core/auth/providers.ts b/core/auth/providers.ts index a43ada0e..73ec1acc 100644 --- a/core/auth/providers.ts +++ b/core/auth/providers.ts @@ -326,27 +326,33 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { * * 1. CIMD is configured for this connection right now, and * 2. the incoming `client_id` is exactly that metadata-document URL, and - * 3. a registration stored for this server under that same `client_id` is - * recorded as `cimd` — written by `ensureCimdClientRegistration`, which - * reaches that line only after confirming the AS advertises + * 3. the registration stored **for this issuer** under that same `client_id` + * is recorded as `cimd` — written by `ensureCimdClientRegistration`, which + * reaches that line only after confirming *that* AS advertises * `client_id_metadata_document_supported`. * * RFC 7591 §3.2 makes a dynamically issued `client_id` opaque, so a client may * not assume its format — which is why (2) is not load-bearing on its own. For * a `registerClient` result to be mislabeled here, the AS would have to mint an - * identifier byte-identical to the HTTPS URL we configured *and* we would have - * to already hold a CIMD registration recorded under it. Anything else — a - * fresh DCR, a different id, CIMD switched off, no prior CIMD registration — + * identifier byte-identical to the HTTPS URL we configured *and* be an AS we + * had already recorded a CIMD registration for — that is, one that advertises + * CIMD and then dynamically registers anyway. Anything else — a fresh DCR, a + * different id, CIMD switched off, no prior CIMD registration for this issuer — * falls through to `"dcr"`. * - * ⚠️ (3) is deliberately **not** scoped to the incoming issuer alone. SEP-2352 - * keys registrations per authorization server, so a resource that resolves to a - * second issuer legitimately has no record under it yet: the first binding - * promotes the unkeyed CIMD entry into issuer A's slot and clears the fallback, - * `ensureCimdClientRegistration` then early-returns on the ctx-less read, and - * the SDK's own CIMD branch saves under issuer B with nothing stored for B. - * Checking the issuer slot and then the server's active registration keeps that - * second issuer labeled CIMD (Copilot). + * ⚠️ (3) is scoped to the issuer on purpose, and the lookup deliberately does + * **not** fall back to the server's active issuer. SEP-2352 keys registrations + * per AS, so a second AS behind the same resource is a separate determination: + * it may well not support CIMD and register dynamically, and RFC 7591 permits + * it to mint the very URL the first AS uses as a CIMD `client_id` (Copilot). + * `ensureCimdClientRegistration` binds the record to the issuer it discovered, + * which is what lets this stay issuer-scoped without losing a genuine + * second-issuer CIMD registration. + * + * The read is still issuer-*keyed* rather than issuer-*only*: `getClientInformation` + * falls back to the unkeyed slot when no `byIssuer` entry exists, which is how a + * pre-registration written before an issuer was known is still found on the save + * that first binds one. */ private async resolveSdkRegistrationKind( clientInformation: OAuthClientInformation, @@ -359,26 +365,19 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { ) { return "dcr"; } - // `undefined` resolves to the server's active issuer, falling back to the - // unkeyed slot our own pre-registration wrote — which is where the record - // still lives on the save that first binds an issuer. - const lookupKeys = issuer === undefined ? [undefined] : [issuer, undefined]; - for (const key of lookupKeys) { - const stored = await this.storage.getClientInformation( - this.serverUrl, - false, - key, - ); - if (stored?.client_id !== clientMetadataUrl) continue; - const storedKind = await this.storage.getClientRegistrationKind( - this.serverUrl, - key, - ); - // `"static"` lives in the preregistered slot, never this one, so `"cimd"` - // is the only kind worth carrying forward. - if (storedKind === "cimd") return "cimd"; - } - return "dcr"; + const stored = await this.storage.getClientInformation( + this.serverUrl, + false, + issuer, + ); + if (stored?.client_id !== clientMetadataUrl) return "dcr"; + const storedKind = await this.storage.getClientRegistrationKind( + this.serverUrl, + issuer, + ); + // `"static"` lives in the preregistered slot, never this one, so `"cimd"` + // is the only kind worth carrying forward. + return storedKind === "cimd" ? "cimd" : "dcr"; } async saveScope(scope: string | undefined): Promise {