From f3f68c731b63822090cfde7da0899b109a0dfcc6 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 00:44:49 -0400 Subject: [PATCH] fix: keep CIMD provenance when a resource resolves to a second issuer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review (#2287): scoping the stored-CIMD check to the incoming issuer alone mislabels the SDK's own CIMD write for a second authorization server. SEP-2352 keys registrations per AS, so the first binding promotes the unkeyed CIMD entry into issuer A's slot and clears the fallback; `ensureCimdClientRegistration` then early-returns on its ctx-less read, and the save under issuer B finds nothing recorded for B. Check the issuer slot and then the server's active registration, so a second issuer stays CIMD while a `client_id` the AS minted itself still falls through to `dcr`. Covered by two tests driving a real `OAuthStorageBase` through the A → B sequence, since the behaviour under test is how storage promotes and clears slots rather than anything a mock would express. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA Signed-off-by: cliffhall --- .../web/src/test/core/auth/providers.test.ts | 77 +++++++++++++++++++ core/auth/providers.ts | 50 +++++++----- 2 files changed, 109 insertions(+), 18 deletions(-) diff --git a/clients/web/src/test/core/auth/providers.test.ts b/clients/web/src/test/core/auth/providers.test.ts index cef8985a..928c8f8e 100644 --- a/clients/web/src/test/core/auth/providers.test.ts +++ b/clients/web/src/test/core/auth/providers.test.ts @@ -7,6 +7,9 @@ import { type OAuthProviderConfig, } from "@inspector/core/auth/providers.js"; 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 { BrowserNavigation, BrowserOAuthClientProvider, @@ -775,6 +778,80 @@ 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). + describe("across two authorization servers", () => { + const ISSUER_B = "https://as-b.example.com"; + + function makeRealStorage(): OAuthStorage { + const backend: OAuthPersistBackend = { + read: async () => null, + write: async () => {}, + }; + return new OAuthStorageBase(new OAuthMemoryStore(), backend); + } + + async function bindFirstIssuer(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 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(); + + // The SDK's own CIMD branch, saving under the second issuer. + await provider.saveClientInformation( + { client_id: METADATA_URL }, + { issuer: ISSUER_B }, + ); + + expect( + await storage.getClientRegistrationKind(SERVER, ISSUER_B), + ).toBe("cimd"); + expect( + await storage.getClientRegistrationKind(SERVER, ISSUER), + ).toBe("cimd"); + }); + + it("still records dcr for a second issuer that mints its own client_id", async () => { + const storage = makeRealStorage(); + const provider = await bindFirstIssuer(storage); + + await provider.saveClientInformation( + { client_id: "b-registered-id" }, + { issuer: ISSUER_B }, + ); + + expect( + await storage.getClientRegistrationKind(SERVER, ISSUER_B), + ).toBe("dcr"); + }); + }); + it("an explicit registrationKind wins and consults no storage reads", async () => { const storage = makeCimdStorage(); const provider = makeProvider(storage, vi.fn(), { diff --git a/core/auth/providers.ts b/core/auth/providers.ts index ed7b3556..a43ada0e 100644 --- a/core/auth/providers.ts +++ b/core/auth/providers.ts @@ -326,9 +326,9 @@ 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. the registration already stored under that same `client_id` is recorded - * as `cimd` — written by `ensureCimdClientRegistration`, which reaches that - * line only after confirming the AS advertises + * 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 * `client_id_metadata_document_supported`. * * RFC 7591 §3.2 makes a dynamically issued `client_id` opaque, so a client may @@ -338,6 +338,15 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { * to already hold a CIMD registration recorded under it. Anything else — a * fresh DCR, a different id, CIMD switched off, no prior CIMD registration — * 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). */ private async resolveSdkRegistrationKind( clientInformation: OAuthClientInformation, @@ -350,21 +359,26 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { ) { return "dcr"; } - // Reads through to the unkeyed slot our own pre-registration wrote, since - // the issuer slot does not exist yet on the save that creates it. - 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"; + // `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"; } async saveScope(scope: string | undefined): Promise {