diff --git a/clients/web/src/test/core/auth/cimd.test.ts b/clients/web/src/test/core/auth/cimd.test.ts index 5b5db72a..fee1b143 100644 --- a/clients/web/src/test/core/auth/cimd.test.ts +++ b/clients/web/src/test/core/auth/cimd.test.ts @@ -17,28 +17,6 @@ function createProvider(storage: OAuthStorage): BaseOAuthClientProvider { }); } -/** Discovery that advertises CIMD support for the default AS location. */ -function cimdDiscoveryFetch(): typeof fetch { - return 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}`); - }; -} - describe("ensureCimdClientRegistration", () => { let storage: OAuthStorage; @@ -47,8 +25,6 @@ describe("ensureCimdClientRegistration", () => { getClientInformation: vi.fn(async () => undefined), saveClientInformation: vi.fn(async () => {}), getDiscoveryState: vi.fn(async () => undefined), - getCimdClientMetadataUrl: vi.fn(async () => undefined), - saveCimdClientMetadataUrl: vi.fn(async () => {}), getScope: vi.fn().mockResolvedValue(undefined), getTokens: vi.fn(async () => undefined), saveTokens: vi.fn(async () => {}), @@ -97,12 +73,6 @@ describe("ensureCimdClientRegistration", () => { }, { registrationKind: "cimd", issuer: "http://127.0.0.1:9999" }, ); - // The provenance marker for this AS, which outlives the credential. - expect(storage.saveCimdClientMetadataUrl).toHaveBeenCalledWith( - SERVER_URL, - "http://127.0.0.1:9999", - METADATA_URL, - ); }); it("does not register when the AS metadata omits CIMD support", async () => { @@ -133,13 +103,6 @@ describe("ensureCimdClientRegistration", () => { }); expect(storage.saveClientInformation).not.toHaveBeenCalled(); - // The marker is actively withdrawn, not merely left unwritten, so an AS that - // stops advertising CIMD stops being treated as one. - expect(storage.saveCimdClientMetadataUrl).toHaveBeenCalledWith( - SERVER_URL, - "http://127.0.0.1:9999", - undefined, - ); }); it("discovers protected-resource metadata at the challenge-advertised URL (#2071)", async () => { @@ -246,60 +209,6 @@ describe("ensureCimdClientRegistration", () => { ).resolves.toBeUndefined(); expect(storage.saveClientInformation).not.toHaveBeenCalled(); - // No marker is invented either — nothing was learned about the AS. - expect(storage.saveCimdClientMetadataUrl).not.toHaveBeenCalled(); - }); - - // #2242 (Copilot): an AS advertising CIMD is not on its own evidence that the - // registration standing for it is a CIMD one. RFC 7591 §3.2 leaves a - // dynamically issued `client_id` opaque, so a real DCR may carry this very - // URL — marking it would relabel it. - it("withdraws the marker when an existing DCR happens to use the metadata URL as its client_id", async () => { - storage.getClientInformation = vi.fn( - async (_url: string, preregistered?: boolean) => - preregistered ? undefined : { client_id: METADATA_URL }, - ); - storage.getClientRegistrationKind = vi.fn( - async (): Promise<"dcr"> => "dcr", - ); - const fetchFn = cimdDiscoveryFetch(); - - await ensureCimdClientRegistration({ - serverUrl: SERVER_URL, - provider: createProvider(storage), - fetchFn, - }); - - expect(storage.saveClientInformation).not.toHaveBeenCalled(); - expect(storage.saveCimdClientMetadataUrl).toHaveBeenCalledWith( - SERVER_URL, - "http://127.0.0.1:9999", - undefined, - ); - }); - - it("reaffirms the marker for an existing registration already recorded as cimd", async () => { - storage.getClientInformation = vi.fn( - async (_url: string, preregistered?: boolean) => - preregistered ? undefined : { client_id: METADATA_URL }, - ); - storage.getClientRegistrationKind = vi.fn( - async (): Promise<"cimd"> => "cimd", - ); - const fetchFn = cimdDiscoveryFetch(); - - await ensureCimdClientRegistration({ - serverUrl: SERVER_URL, - provider: createProvider(storage), - fetchFn, - }); - - expect(storage.saveClientInformation).not.toHaveBeenCalled(); - expect(storage.saveCimdClientMetadataUrl).toHaveBeenCalledWith( - SERVER_URL, - "http://127.0.0.1:9999", - METADATA_URL, - ); }); it("no-ops when client information is already stored for the discovered issuer", async () => { diff --git a/clients/web/src/test/core/auth/connection-state.test.ts b/clients/web/src/test/core/auth/connection-state.test.ts index 44a07110..d328b01b 100644 --- a/clients/web/src/test/core/auth/connection-state.test.ts +++ b/clients/web/src/test/core/auth/connection-state.test.ts @@ -61,8 +61,6 @@ function createStorage( getCodeVerifier: vi.fn(), getDiscoveryState: vi.fn().mockResolvedValue(undefined), saveDiscoveryState: vi.fn(), - getCimdClientMetadataUrl: vi.fn(async () => undefined), - saveCimdClientMetadataUrl: vi.fn(async () => undefined), clearDiscoveryState: vi.fn(), }; } diff --git a/clients/web/src/test/core/auth/providers.test.ts b/clients/web/src/test/core/auth/providers.test.ts index 87dee09f..1992e7fc 100644 --- a/clients/web/src/test/core/auth/providers.test.ts +++ b/clients/web/src/test/core/auth/providers.test.ts @@ -221,8 +221,6 @@ describe("OAuthNavigation", () => { getScope: vi.fn().mockResolvedValue(undefined), getClientInformation: vi.fn(async () => undefined), getClientRegistrationKind: vi.fn(async () => undefined), - getCimdClientMetadataUrl: vi.fn(async () => undefined), - saveCimdClientMetadataUrl: vi.fn(async () => undefined), saveClientInformation: vi.fn(async () => undefined), savePreregisteredClientInformation: vi.fn(async () => undefined), saveScope: vi.fn(async () => undefined), @@ -643,16 +641,38 @@ describe("OAuthNavigation", () => { const ISSUER = "https://as.example.com"; const METADATA_URL = "https://app.example.com/client-metadata.json"; - /** Storage holding this AS's CIMD marker for METADATA_URL. */ + /** Discovery state as SDK `auth()` persists it before saving client info. */ + function seedDiscovery( + storage: OAuthStorage, + issuer: string, + cimd: boolean, + ) { + vi.mocked(storage.getDiscoveryState).mockResolvedValue({ + authorizationServerUrl: issuer, + authorizationServerMetadata: { + issuer, + authorization_endpoint: `${issuer}/authorize`, + token_endpoint: `${issuer}/token`, + response_types_supported: ["code"], + ...(cimd && { client_id_metadata_document_supported: true }), + }, + }); + } + + /** Storage whose issuer slot already holds a CIMD registration. */ function makeCimdStorage(): OAuthStorage { const storage = makeStorage(); - vi.mocked(storage.getCimdClientMetadataUrl).mockResolvedValue( - METADATA_URL, + vi.mocked(storage.getClientInformation).mockImplementation( + async (_url: string, preregistered?: boolean) => + preregistered ? undefined : { client_id: METADATA_URL }, + ); + vi.mocked(storage.getClientRegistrationKind).mockResolvedValue( + "cimd", ); return storage; } - it("keeps cimd when CIMD is configured and this issuer carries the marker", async () => { + it("keeps cimd when back-stamping a registration recorded as cimd", async () => { const storage = makeCimdStorage(); const provider = makeProvider(storage, vi.fn(), { clientMetadataUrl: METADATA_URL, @@ -663,10 +683,6 @@ describe("OAuthNavigation", () => { { issuer: ISSUER }, ); - expect(storage.getCimdClientMetadataUrl).toHaveBeenCalledWith( - SERVER, - ISSUER, - ); expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER, { client_id: METADATA_URL }, @@ -674,24 +690,50 @@ describe("OAuthNavigation", () => { ); }); - // SDK v2 `auth()` answers `invalid_client` / `unauthorized_client` with - // `invalidateCredentials("client")` and an immediate retry. That clears - // the stored registration *and* its kind, so provenance read off the - // credential would be gone by the time the retry's CIMD save lands - // (Copilot). The marker is not a credential and survives. - it("keeps cimd through invalid-client recovery, which clears the credential", async () => { - const storage = makeCimdStorage(); + // RFC 7591 §3.2 leaves a dynamically issued `client_id` opaque, so an + // existing DCR may carry the configured metadata URL. Back-stamping it + // must not relabel it (Copilot). + it("keeps dcr when back-stamping a DCR that uses the metadata URL", async () => { + const storage = makeStorage(); + vi.mocked(storage.getClientInformation).mockImplementation( + async (_url: string, preregistered?: boolean) => + preregistered ? undefined : { client_id: METADATA_URL }, + ); + vi.mocked(storage.getClientRegistrationKind).mockResolvedValue("dcr"); + // Even with an AS that does advertise CIMD. + seedDiscovery(storage, ISSUER, true); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: METADATA_URL, + }); + + await provider.saveClientInformation( + { client_id: METADATA_URL }, + { issuer: ISSUER }, + ); + + expect(storage.saveClientInformation).toHaveBeenCalledWith( + SERVER, + { client_id: METADATA_URL }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + }); + + // Nothing stored for this issuer, so the SDK is creating the + // registration. It reaches its URL-based-client-ID branch exactly when + // the AS advertises CIMD — read back from the discovery state it + // persisted moments earlier. + it("records cimd for a new registration when the AS advertises CIMD", async () => { + const storage = makeStorage(); + seedDiscovery(storage, ISSUER, true); const provider = makeProvider(storage, vi.fn(), { clientMetadataUrl: METADATA_URL, }); - await provider.invalidateCredentials("client"); await provider.saveClientInformation( { client_id: METADATA_URL }, { issuer: ISSUER }, ); - expect(storage.clearClientInformation).toHaveBeenCalledWith(SERVER); expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER, { client_id: METADATA_URL }, @@ -699,8 +741,47 @@ describe("OAuthNavigation", () => { ); }); + it("records dcr for a new registration when the AS does not advertise CIMD", async () => { + const storage = makeStorage(); + seedDiscovery(storage, ISSUER, false); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: METADATA_URL, + }); + + await provider.saveClientInformation( + { client_id: METADATA_URL }, + { issuer: ISSUER }, + ); + + expect(storage.saveClientInformation).toHaveBeenCalledWith( + SERVER, + { client_id: METADATA_URL }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + }); + + it("records dcr when the discovery state describes a different issuer", async () => { + const storage = makeStorage(); + seedDiscovery(storage, "https://as-other.example.com", true); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: METADATA_URL, + }); + + await provider.saveClientInformation( + { client_id: METADATA_URL }, + { issuer: ISSUER }, + ); + + expect(storage.saveClientInformation).toHaveBeenCalledWith( + SERVER, + { client_id: METADATA_URL }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + }); + it("records dcr for a server-minted client_id while CIMD is configured", async () => { const storage = makeCimdStorage(); + seedDiscovery(storage, ISSUER, true); const provider = makeProvider(storage, vi.fn(), { clientMetadataUrl: METADATA_URL, }); @@ -710,8 +791,9 @@ describe("OAuthNavigation", () => { { issuer: ISSUER }, ); - // The id is not the metadata URL, so the marker is never consulted. - expect(storage.getCimdClientMetadataUrl).not.toHaveBeenCalled(); + // The id is not the metadata URL, so nothing is read at all. + expect(storage.getClientInformation).not.toHaveBeenCalled(); + expect(storage.getDiscoveryState).not.toHaveBeenCalled(); expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER, { client_id: "dcr-minted-id" }, @@ -719,8 +801,9 @@ describe("OAuthNavigation", () => { ); }); - it("records dcr when CIMD is not configured, even if the marker is set", async () => { + it("records dcr when CIMD is not configured for this connection", async () => { const storage = makeCimdStorage(); + seedDiscovery(storage, ISSUER, true); const provider = makeProvider(storage); await provider.saveClientInformation( @@ -735,48 +818,6 @@ describe("OAuthNavigation", () => { ); }); - it("records dcr when the marker names a different metadata URL", async () => { - const storage = makeStorage(); - vi.mocked(storage.getCimdClientMetadataUrl).mockResolvedValue( - "https://other.example.com/client-metadata.json", - ); - const provider = makeProvider(storage, vi.fn(), { - clientMetadataUrl: METADATA_URL, - }); - - await provider.saveClientInformation( - { client_id: METADATA_URL }, - { issuer: ISSUER }, - ); - - expect(storage.saveClientInformation).toHaveBeenCalledWith( - SERVER, - { client_id: METADATA_URL }, - { registrationKind: "dcr", issuer: ISSUER }, - ); - }); - - it("records dcr when this issuer carries no marker", async () => { - // The AS returns the configured metadata URL from a real registration - // (RFC 7591 §3.2 leaves the id opaque). With no marker for this AS, - // the save is still DCR. - const storage = makeStorage(); - const provider = makeProvider(storage, vi.fn(), { - clientMetadataUrl: METADATA_URL, - }); - - await provider.saveClientInformation( - { client_id: METADATA_URL }, - { issuer: ISSUER }, - ); - - expect(storage.saveClientInformation).toHaveBeenCalledWith( - SERVER, - { client_id: METADATA_URL }, - { registrationKind: "dcr", issuer: ISSUER }, - ); - }); - // 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 @@ -859,6 +900,16 @@ describe("OAuthNavigation", () => { provider, fetchFn: discoveryFetch(ISSUER_B, true), }); + await storage.saveDiscoveryState(SERVER, { + authorizationServerUrl: ISSUER_B, + authorizationServerMetadata: { + issuer: ISSUER_B, + authorization_endpoint: `${ISSUER_B}/authorize`, + token_endpoint: `${ISSUER_B}/token`, + response_types_supported: ["code"], + client_id_metadata_document_supported: true, + }, + }); await provider.saveClientInformation( { client_id: METADATA_URL }, { issuer: ISSUER_B }, @@ -882,6 +933,20 @@ describe("OAuthNavigation", () => { provider, fetchFn: discoveryFetch(ISSUER_B, false), }); + // Discovery state as SDK `auth()` persists it for issuer B — which + // is what tells the save apart from a CIMD one. Seeded explicitly so + // the assertion rests on B's advertised capabilities rather than on + // discovery state merely being absent. + await storage.saveDiscoveryState(SERVER, { + authorizationServerUrl: ISSUER_B, + authorizationServerMetadata: { + issuer: ISSUER_B, + authorization_endpoint: `${ISSUER_B}/authorize`, + token_endpoint: `${ISSUER_B}/token`, + response_types_supported: ["code"], + }, + }); + // ...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( @@ -920,10 +985,6 @@ describe("OAuthNavigation", () => { provider, fetchFn: discoveryFetch(ISSUER, true), }); - expect( - await storage.getCimdClientMetadataUrl(SERVER, ISSUER), - ).toBeUndefined(); - // The SDK's issuer back-stamp of that same registration. await provider.saveClientInformation( { client_id: METADATA_URL }, @@ -935,33 +996,38 @@ describe("OAuthNavigation", () => { ).toBe("dcr"); }); - // The provenance marker's whole reason for existing: SDK v2 `auth()` - // answers `invalid_client` with `invalidateCredentials("client")` and - // an immediate retry, and that clear removes the credential *and* its - // registration kind. Asserted against real storage, since the point is - // what `clearClientInformation` does and does not touch (Copilot). - it("keeps the CIMD marker through invalid-client credential invalidation", async () => { + // SDK v2 `auth()` answers `invalid_client` with + // `invalidateCredentials("client")` and an immediate retry. That clear + // removes the registration *and* its kind, so the retry takes the + // new-registration path and must be answered from the discovery state + // — which the clear does not touch (Copilot). Asserted against real + // storage, since the point is what `clearClientInformation` does. + it("keeps cimd through invalid-client recovery, which clears the credential", async () => { const storage = makeRealStorage(); const provider = await bindIssuerA(storage); - expect(await storage.getCimdClientMetadataUrl(SERVER, ISSUER)).toBe( - METADATA_URL, - ); + // Discovery state as SDK `auth()` persisted it for issuer A. + await storage.saveDiscoveryState(SERVER, { + authorizationServerUrl: ISSUER, + authorizationServerMetadata: { + issuer: ISSUER, + authorization_endpoint: `${ISSUER}/authorize`, + token_endpoint: `${ISSUER}/token`, + response_types_supported: ["code"], + client_id_metadata_document_supported: true, + }, + }); await provider.invalidateCredentials("client"); - // The credential and its kind are gone... + // The credential and its recorded kind are both gone... expect( await storage.getClientInformation(SERVER, false, ISSUER), ).toBeUndefined(); expect( await storage.getClientRegistrationKind(SERVER, ISSUER), ).toBeUndefined(); - // ...but the marker is not a credential, so it survives. - expect(await storage.getCimdClientMetadataUrl(SERVER, ISSUER)).toBe( - METADATA_URL, - ); - // The SDK's retry re-runs its URL-based client-ID branch. + // ...so the SDK's retry re-runs its URL-based client-ID branch. await provider.saveClientInformation( { client_id: METADATA_URL }, { issuer: ISSUER }, diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index 1da62370..96d1df80 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -70,8 +70,6 @@ function createMockParams( takeRevocationSnapshot: vi.fn().mockResolvedValue({ byIssuer: {} }), getDiscoveryState: vi.fn().mockResolvedValue(undefined), saveDiscoveryState: vi.fn().mockResolvedValue(undefined), - getCimdClientMetadataUrl: vi.fn(async () => undefined), - saveCimdClientMetadataUrl: vi.fn(async () => undefined), clearDiscoveryState: vi.fn().mockResolvedValue(undefined), }; diff --git a/core/auth/cimd.ts b/core/auth/cimd.ts index edf87438..d17ce720 100644 --- a/core/auth/cimd.ts +++ b/core/auth/cimd.ts @@ -68,50 +68,19 @@ export async function ensureCimdClientRegistration(params: { } const issuer = metadata?.issuer; - const supportsCimd = metadata?.client_id_metadata_document_supported === true; - - /** - * The marker records that *this* AS accepts this URL as a `client_id` **and** - * that the registration standing for it got there through CIMD. It is written - * only where both are established, and actively withdrawn otherwise, so it - * cannot go stale: discovery runs on every connect. - */ - const setMarker = async (url: string | undefined) => { - if (issuer) await params.provider.saveCimdClientMetadataUrl(issuer, url); - }; - - if (!supportsCimd) { - // Withdrawn, not merely left alone — an AS that stops advertising CIMD - // stops being treated as one. - await setMarker(undefined); - return; - } + if (!metadata?.client_id_metadata_document_supported) return; // ⚠️ Keyed by the issuer just resolved, not read ctx-less. A ctx-less read // resolves through the *active* issuer, so it early-returns for every - // subsequent issuer and leaves them with no CIMD record at all. It answers the - // static case first, since `clientInformation` checks the preregistered slot - // before any issuer slot. + // subsequent issuer and leaves them with no CIMD registration at all. It + // answers the static case first, since `clientInformation` checks the + // preregistered slot before any issuer slot — and a static client, like any + // existing registration, means there is nothing to pre-register. const existing = await params.provider.clientInformation( issuer ? { issuer } : undefined, ); - if (existing?.client_id) { - // Something is already registered for this AS, so this call establishes - // nothing — and AS support for CIMD is not on its own evidence that *that* - // registration is a CIMD one. RFC 7591 §3.2 leaves a dynamically issued - // `client_id` opaque, so an existing DCR may carry this very URL; marking it - // would relabel a real dynamic registration (Copilot). Reaffirm the marker - // only for a registration already recorded as `cimd` under this exact URL, - // and withdraw it otherwise — which also covers a static client. - const existingKind = await params.provider.clientRegistrationKind(issuer); - const isCimdRegistration = - existingKind === "cimd" && existing.client_id === clientMetadataUrl; - await setMarker(isCimdRegistration ? clientMetadataUrl : undefined); - return; - } + if (existing?.client_id) return; - // From here this call *is* the CIMD registration, so the marker is earned. - await setMarker(clientMetadataUrl); await params.provider.saveClientInformation( { client_id: clientMetadataUrl }, { diff --git a/core/auth/oauth-storage.ts b/core/auth/oauth-storage.ts index 26f76f39..68b7810c 100644 --- a/core/auth/oauth-storage.ts +++ b/core/auth/oauth-storage.ts @@ -186,34 +186,6 @@ export class OAuthStorageBase implements OAuthStorage { ); } - async getCimdClientMetadataUrl( - serverUrl: string, - issuer?: string, - ): Promise { - await this.ensureLoaded(); - const state = this.memory.getState().getServerState(serverUrl); - return this.issuerSlot(state, issuer)?.cimdClientMetadataUrl; - } - - async saveCimdClientMetadataUrl( - serverUrl: string, - issuer: string, - clientMetadataUrl: string | undefined, - ): Promise { - await this.ensureLoaded(); - // Not a save of credentials, so it must not promote this issuer to - // `activeIssuer` — the marker is written during discovery, before anything - // has been authorized against this AS. - this.updateIssuerSlot( - serverUrl, - issuer, - { cimdClientMetadataUrl: clientMetadataUrl }, - {}, - false, - ); - await this.persist(); - } - async saveClientInformation( serverUrl: string, clientInformation: OAuthClientInformation, diff --git a/core/auth/providers.ts b/core/auth/providers.ts index f64f7a92..92512c0a 100644 --- a/core/auth/providers.ts +++ b/core/auth/providers.ts @@ -9,11 +9,7 @@ import type { OAuthMetadata, OAuthDiscoveryState, } from "@modelcontextprotocol/client"; -import type { - OAuthStorage, - SaveClientInformationOptions, - OAuthClientRegistrationKind, -} from "./storage.js"; +import type { OAuthStorage, SaveClientInformationOptions } from "./storage.js"; import { generateOAuthState } from "./utils.js"; import { applyAuthorizationParams } from "./authorizationParams.js"; import { scopeForDeclinedRefreshGrant } from "./scopes.js"; @@ -327,36 +323,40 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { * save as DCR is what relabeled a CIMD registration `Dynamic (DCR)` in * Connection Info the moment the SDK bound it to an issuer (#2242). * - * The claim is deliberately narrow — three conditions must all hold, and the - * decisive one is a fact *we recorded about this authorization server*, not an - * inference about what it returned: + * Two cases reach here, and they are told apart by whether a registration + * already exists for this issuer: * - * 1. CIMD is configured for this connection right now, and - * 2. the incoming `client_id` is exactly that metadata-document URL, and - * 3. `ensureCimdClientRegistration` recorded that same URL as the CIMD marker - * **for this issuer**, having read `client_id_metadata_document_supported` - * from *that* AS's own metadata. + * - **A back-stamp.** A registration is already stored for this issuer under + * this `client_id`, and the SDK is only adding the `issuer` to it. Its + * recorded kind is the answer — kind and credential are written and cleared + * together, so a stored registration always has one. + * - **A new registration.** Nothing is stored for this issuer, so this save + * creates it. SDK v2 `auth()` reaches its URL-based-client-ID branch — rather + * than `registerClient` — exactly when the AS advertises + * `client_id_metadata_document_supported` and a `clientMetadataUrl` is + * configured, and it persists the AS metadata via `saveDiscoveryState` + * *before* it reads or writes client information. So the branch it took is + * not inferred here, it is read back from the state it just wrote. * - * 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* be an AS that - * currently advertises CIMD and dynamically registered anyway. Anything else — - * a fresh DCR, a different id, CIMD switched off, an AS that does not advertise - * CIMD — falls through to `"dcr"`. + * This is why the check is not "the `client_id` looks like our metadata URL". + * RFC 7591 §3.2 makes a dynamically issued `client_id` opaque, so an AS may + * mint that very URL from `POST /register`; the URL comparison only decides + * whether CIMD is *in play* for this connection, and the two cases above decide + * what actually happened (#2242, Copilot). * - * ⚠️ (3) reads the **marker**, not the stored registration kind, and the two - * differ in exactly one place that matters: `invalidateCredentials("client")` - * clears the credential and its kind, and SDK v2 `auth()` calls it on an - * `invalid_client` / `unauthorized_client` response before retrying. The - * retry's URL-based client-ID save would then find no kind and be recorded as - * DCR. The marker describes the AS rather than the credential, so it survives - * that clear (#2242, Copilot). + * Consequences worth stating, since each was a defect on the way here: * - * ⚠️ (3) is issuer-scoped with no fallback to the server's active issuer. A - * second AS behind one resource is a separate determination: it may not support - * CIMD and may register dynamically, and RFC 7591 permits it to mint the very - * URL the first AS uses as its CIMD `client_id`. + * - An existing DCR whose `client_id` happens to be the metadata URL stays + * `dcr` — it takes the back-stamp path and its recorded kind says so. + * - `invalidateCredentials("client")`, which SDK `auth()` calls on + * `invalid_client` before retrying, clears the registration and its kind. The + * retry therefore takes the new-registration path and is answered from + * discovery state, which that clear does not touch. + * - A second AS behind one resource gets its own answer, since discovery state + * describes the issuer the SDK actually resolved. One that does not advertise + * CIMD is `dcr` even when it mints the metadata URL as its `client_id`. + * - A transient failure in our own CIMD preflight costs nothing: the SDK's own + * discovery is what this reads. */ private async resolveSdkRegistrationKind( clientInformation: OAuthClientInformation, @@ -369,35 +369,32 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { ) { return "dcr"; } - const marker = await this.storage.getCimdClientMetadataUrl( + + // Issuer-keyed, with `getClientInformation`'s own fallback to the unkeyed + // slot covering a registration written before an issuer was known. + const stored = await this.storage.getClientInformation( this.serverUrl, + false, issuer, ); - return marker === clientMetadataUrl ? "cimd" : "dcr"; - } + if (stored?.client_id === clientMetadataUrl) { + const storedKind = await this.storage.getClientRegistrationKind( + this.serverUrl, + issuer, + ); + // `"static"` lives in the preregistered slot, never this one. + return storedKind === "cimd" ? "cimd" : "dcr"; + } - /** @see OAuthStorage.getCimdClientMetadataUrl */ - async cimdClientMetadataUrl(issuer?: string): Promise { - return await this.storage.getCimdClientMetadataUrl(this.serverUrl, issuer); - } - - /** @see OAuthStorage.getClientRegistrationKind */ - async clientRegistrationKind( - issuer?: string, - ): Promise { - return await this.storage.getClientRegistrationKind(this.serverUrl, issuer); - } - - /** @see OAuthStorage.saveCimdClientMetadataUrl */ - async saveCimdClientMetadataUrl( - issuer: string, - clientMetadataUrl: string | undefined, - ): Promise { - await this.storage.saveCimdClientMetadataUrl( - this.serverUrl, - issuer, - clientMetadataUrl, - ); + // A new registration: read back the branch the SDK took. + const discovery = await this.storage.getDiscoveryState(this.serverUrl); + const metadata = discovery?.authorizationServerMetadata; + // Require the metadata to describe *this* issuer, so a state left over from + // a previously resolved AS cannot answer for a different one. + if (issuer !== undefined && metadata?.issuer !== issuer) return "dcr"; + return metadata?.client_id_metadata_document_supported === true + ? "cimd" + : "dcr"; } async saveScope(scope: string | undefined): Promise { diff --git a/core/auth/storage.ts b/core/auth/storage.ts index 559f118d..edc6451e 100644 --- a/core/auth/storage.ts +++ b/core/auth/storage.ts @@ -81,23 +81,6 @@ export interface OAuthStorage { issuer?: string, ): Promise; - /** - * The CIMD client-metadata URL this authorization server was confirmed to - * accept as a `client_id`. Survives {@link clearClientInformation}, because it - * records a property of the AS rather than a credential (#2242). - */ - getCimdClientMetadataUrl( - serverUrl: string, - issuer?: string, - ): Promise; - - /** Write (or, with `undefined`, clear) the marker above for one issuer. */ - saveCimdClientMetadataUrl( - serverUrl: string, - issuer: string, - clientMetadataUrl: string | undefined, - ): Promise; - /** * Save client information (dynamically registered) */ diff --git a/core/auth/store.ts b/core/auth/store.ts index 6243600c..d5e13511 100644 --- a/core/auth/store.ts +++ b/core/auth/store.ts @@ -30,21 +30,6 @@ export interface IssuerBoundOAuthState { /** Set when {@link clientInformation} is saved — DCR vs CIMD. */ clientRegistrationKind?: OAuthClientRegistrationKind; tokens?: OAuthTokens; - /** - * The CIMD client-metadata URL this AS was confirmed to accept as a `client_id` - * — written by `ensureCimdClientRegistration` after reading - * `client_id_metadata_document_supported` from *this* issuer's metadata, and - * refreshed (or cleared) on every connect because that check now runs each time. - * - * Deliberately **not** a credential, and so deliberately **not** cleared by - * {@link OAuthStorage.clearClientInformation}. It records a property of the - * authorization server and our own configuration, which an `invalid_client` - * response says nothing about: SDK v2 `auth()` answers that error by calling - * `invalidateCredentials("client")` and retrying, and the retry's URL-based - * client-ID save would otherwise land with no provenance and be recorded as - * DCR (#2242, Copilot). - */ - cimdClientMetadataUrl?: string; } /**