From 4789cb954e2c5b46da285a335277c3b75eb65cdb Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 00:04:40 -0400 Subject: [PATCH 01/10] fix: preserve CIMD registration provenance across SDK issuer binding (#2242) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `BaseOAuthClientProvider.saveClientInformation` treated every save without an explicit `registrationKind` as DCR. The SDK reaches it from three places in `auth()` and only one is a dynamic registration, so the issuer-binding write overwrote the `cimd` provenance our own pre-registration had just stored — and Connection Info reported `Dynamic (DCR)` for a connection that never issued a `POST /oauth/register`. Recover the kind instead: a `client_id` equal to the configured `clientMetadataUrl` is CIMD (which also covers the SDK's own CIMD write), and otherwise a stored registration with the same `client_id` carries its recorded kind forward. Matching on `client_id` keeps stale CIMD provenance from leaking onto a later DCR registration for the same server. 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 | 144 ++++++++++++++++++ .../mcp/inspectorClient-oauth-e2e.test.ts | 9 ++ core/auth/providers.ts | 55 ++++++- 3 files changed, 204 insertions(+), 4 deletions(-) diff --git a/clients/web/src/test/core/auth/providers.test.ts b/clients/web/src/test/core/auth/providers.test.ts index fa2d81a22..3998f118b 100644 --- a/clients/web/src/test/core/auth/providers.test.ts +++ b/clients/web/src/test/core/auth/providers.test.ts @@ -216,6 +216,7 @@ describe("OAuthNavigation", () => { load: vi.fn().mockResolvedValue(undefined), getScope: vi.fn().mockResolvedValue(undefined), getClientInformation: vi.fn(async () => undefined), + getClientRegistrationKind: vi.fn(async () => undefined), saveClientInformation: vi.fn(async () => undefined), savePreregisteredClientInformation: vi.fn(async () => undefined), saveScope: vi.fn(async () => undefined), @@ -238,11 +239,13 @@ describe("OAuthNavigation", () => { function makeProvider( storage: OAuthStorage, navCallback = vi.fn(), + extraConfig: Partial = {}, ): BaseOAuthClientProvider { const config: OAuthProviderConfig = { storage, redirectUrlProvider: new MutableRedirectUrlProvider(), navigation: new CallbackNavigation(navCallback), + ...extraConfig, }; return new BaseOAuthClientProvider(SERVER, config); } @@ -625,6 +628,147 @@ describe("OAuthNavigation", () => { ); }); + // #2242: the SDK binds an existing registration to its issuer by calling + // `saveClientInformation(info, { issuer })` with no registration kind. + // Treating every such save as DCR relabeled a CIMD registration + // "Dynamic (DCR)" in Connection Info, even though no `POST /register` + // ever happened. + describe("registration kind on an unstamped (SDK) save", () => { + const ISSUER = "https://as.example.com"; + const METADATA_URL = "https://app.example.com/client-metadata.json"; + + it("keeps cimd when the client_id is the configured metadata document URL", async () => { + 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: "cimd", issuer: ISSUER }, + ); + }); + + it("still records dcr for a server-minted client_id while CIMD is configured", async () => { + const storage = makeStorage(); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: METADATA_URL, + }); + + await provider.saveClientInformation( + { client_id: "dcr-minted-id" }, + { issuer: ISSUER }, + ); + + expect(storage.saveClientInformation).toHaveBeenCalledWith( + SERVER, + { client_id: "dcr-minted-id" }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + }); + + it("preserves the stored kind when the stored client_id matches", 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( + "cimd", + ); + // No `clientMetadataUrl` on the provider — the provenance comes from + // storage alone, so a config cleared since the registration was made + // does not silently demote it. + const provider = makeProvider(storage); + + await provider.saveClientInformation( + { client_id: METADATA_URL }, + { issuer: ISSUER }, + ); + + expect(storage.getClientInformation).toHaveBeenCalledWith( + SERVER, + false, + ISSUER, + ); + expect(storage.saveClientInformation).toHaveBeenCalledWith( + SERVER, + { client_id: METADATA_URL }, + { registrationKind: "cimd", issuer: ISSUER }, + ); + }); + + it("does not leak a stored cimd kind onto a different client_id", 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( + "cimd", + ); + const provider = makeProvider(storage); + + await provider.saveClientInformation( + { client_id: "freshly-registered" }, + { issuer: ISSUER }, + ); + + expect(storage.getClientRegistrationKind).not.toHaveBeenCalled(); + expect(storage.saveClientInformation).toHaveBeenCalledWith( + SERVER, + { client_id: "freshly-registered" }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + }); + + it("falls back to dcr when storage has a matching id but no recorded kind", async () => { + const storage = makeStorage(); + vi.mocked(storage.getClientInformation).mockImplementation( + async (_url: string, preregistered?: boolean) => + preregistered ? undefined : { client_id: "legacy-id" }, + ); + const provider = makeProvider(storage); + + await provider.saveClientInformation( + { client_id: "legacy-id" }, + { issuer: ISSUER }, + ); + + expect(storage.saveClientInformation).toHaveBeenCalledWith( + SERVER, + { client_id: "legacy-id" }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + }); + + it("an explicit registrationKind wins and consults no storage reads", async () => { + const storage = makeStorage(); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: METADATA_URL, + }); + + await provider.saveClientInformation( + { client_id: METADATA_URL }, + { registrationKind: "cimd" }, + ); + + expect(storage.getClientInformation).not.toHaveBeenCalled(); + expect(storage.getClientRegistrationKind).not.toHaveBeenCalled(); + expect(storage.saveClientInformation).toHaveBeenCalledWith( + SERVER, + { client_id: METADATA_URL }, + { registrationKind: "cimd", issuer: undefined }, + ); + }); + }); + it("round-trips discovery state to storage", async () => { const storage = makeStorage(); const provider = makeProvider(storage); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts index a2778b87f..9e5431b7d 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts @@ -370,6 +370,15 @@ describe("InspectorClient OAuth E2E", () => { // Connection should now be successful expect(client.getStatus()).toBe("connected"); + + // #2242: the metadata-document URL is the client_id, and the stored + // provenance still says CIMD after the SDK bound the registration to + // the issuer — no `POST /register` ever happened. + const oauthState = await client.getOAuthState(); + expect(oauthState?.client).toMatchObject({ + clientId: metadataUrl, + registrationKind: "cimd", + }); }); it("should retry original request after OAuth completion with CIMD", async () => { diff --git a/core/auth/providers.ts b/core/auth/providers.ts index 0447b7291..5469e96b5 100644 --- a/core/auth/providers.ts +++ b/core/auth/providers.ts @@ -294,15 +294,15 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { // `OAuthClientInformationContext` ({ issuer }); our own DCR/CIMD callers // pass `SaveClientInformationOptions` ({ registrationKind }). Accept either // and read whichever keys are present: the SDK supplies `issuer` (SEP-2352 - // per-AS keying) and defaults registration kind to DCR; our callers supply - // the registration kind and no issuer yet. + // per-AS keying) and no kind — `resolveSdkRegistrationKind` recovers it — + // while our callers supply the registration kind and no issuer yet. options?: SaveClientInformationOptions | OAuthClientInformationContext, ): Promise { + const issuer = options && "issuer" in options ? options.issuer : undefined; const registrationKind = options && "registrationKind" in options ? options.registrationKind - : "dcr"; - const issuer = options && "issuer" in options ? options.issuer : undefined; + : await this.resolveSdkRegistrationKind(clientInformation, issuer); await this.storage.saveClientInformation( this.serverUrl, clientInformation, @@ -313,6 +313,53 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { ); } + /** + * Resolve the registration kind for a save that carries no explicit one — that + * is, one the SDK made. Three SDK call sites reach here: back-stamping an + * existing registration with its `issuer`, the SDK's own CIMD write (whose + * `client_id` *is* the metadata-document URL), and a real dynamic + * registration. Only the last is `"dcr"`, so defaulting every unstamped save + * to it relabels a CIMD registration `Dynamic (DCR)` in Connection Info the + * moment the SDK binds it to an issuer — reported as #2242, where no + * `POST /register` was ever made. + * + * `client_id` is what tells the cases apart, so match on it rather than on the + * stored kind alone: a DCR `client_id` is minted by the authorization server, + * so a later DCR registration for the same server cannot inherit the earlier + * CIMD provenance. + */ + private async resolveSdkRegistrationKind( + clientInformation: OAuthClientInformation, + issuer: string | undefined, + ): Promise { + const clientMetadataUrl = this.clientMetadataUrl?.trim(); + if ( + clientMetadataUrl && + clientInformation.client_id === clientMetadataUrl + ) { + return "cimd"; + } + // Falls back to the unkeyed slot our own pre-registration wrote, since the + // issuer slot does not exist yet on the save that creates it. Reading it + // covers a CIMD registration whose `clientMetadataUrl` config has since + // been cleared, so the provenance is not silently demoted. + const stored = await this.storage.getClientInformation( + this.serverUrl, + false, + issuer, + ); + if (!stored || stored.client_id !== clientInformation.client_id) { + 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 { await this.storage.saveScope(this.serverUrl, scope); this.cachedScope = scope; From c8c050b51e154b9d3211600e19006f529c7d6078 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 00:25:49 -0400 Subject: [PATCH 02/10] fix: require a recorded CIMD registration before preserving the kind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review (#2287): RFC 7591 §3.2 leaves a dynamically issued `client_id` opaque, so `client_id === clientMetadataUrl` is not on its own proof that CIMD ran — an authorization server could in principle mint that value from `POST /register`. Narrow the claim to a conjunction, whose decisive term is a registration we recorded as CIMD ourselves rather than an inference about what the AS returned: CIMD must be configured for this connection, the incoming `client_id` must be exactly that metadata-document URL, and the registration already stored under that id must be recorded as `cimd`. Everything else falls through to `dcr`. 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 | 101 +++++++++++------- core/auth/providers.ts | 49 +++++---- 2 files changed, 92 insertions(+), 58 deletions(-) diff --git a/clients/web/src/test/core/auth/providers.test.ts b/clients/web/src/test/core/auth/providers.test.ts index 3998f118b..cef8985ad 100644 --- a/clients/web/src/test/core/auth/providers.test.ts +++ b/clients/web/src/test/core/auth/providers.test.ts @@ -637,8 +637,21 @@ describe("OAuthNavigation", () => { const ISSUER = "https://as.example.com"; const METADATA_URL = "https://app.example.com/client-metadata.json"; - it("keeps cimd when the client_id is the configured metadata document URL", async () => { + /** Storage already holding the CIMD pre-registration for METADATA_URL. */ + function makeCimdStorage(): OAuthStorage { const storage = makeStorage(); + 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 the stored registration matches", async () => { + const storage = makeCimdStorage(); const provider = makeProvider(storage, vi.fn(), { clientMetadataUrl: METADATA_URL, }); @@ -648,6 +661,13 @@ describe("OAuthNavigation", () => { { issuer: ISSUER }, ); + // Reads the dynamic slot for this issuer, which falls back to the + // unkeyed slot the pre-registration wrote. + expect(storage.getClientInformation).toHaveBeenCalledWith( + SERVER, + false, + ISSUER, + ); expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER, { client_id: METADATA_URL }, @@ -655,8 +675,8 @@ describe("OAuthNavigation", () => { ); }); - it("still records dcr for a server-minted client_id while CIMD is configured", async () => { - const storage = makeStorage(); + it("records dcr for a server-minted client_id while CIMD is configured", async () => { + const storage = makeCimdStorage(); const provider = makeProvider(storage, vi.fn(), { clientMetadataUrl: METADATA_URL, }); @@ -666,6 +686,10 @@ describe("OAuthNavigation", () => { { issuer: ISSUER }, ); + // The id is not the metadata URL, so nothing is read and nothing is + // carried forward. + expect(storage.getClientInformation).not.toHaveBeenCalled(); + expect(storage.getClientRegistrationKind).not.toHaveBeenCalled(); expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER, { client_id: "dcr-minted-id" }, @@ -673,18 +697,8 @@ describe("OAuthNavigation", () => { ); }); - it("preserves the stored kind when the stored client_id matches", 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( - "cimd", - ); - // No `clientMetadataUrl` on the provider — the provenance comes from - // storage alone, so a config cleared since the registration was made - // does not silently demote it. + it("records dcr when CIMD is not configured, even if storage says cimd", async () => { + const storage = makeCimdStorage(); const provider = makeProvider(storage); await provider.saveClientInformation( @@ -692,64 +706,77 @@ describe("OAuthNavigation", () => { { issuer: ISSUER }, ); - expect(storage.getClientInformation).toHaveBeenCalledWith( + expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER, - false, - ISSUER, + { client_id: METADATA_URL }, + { registrationKind: "dcr", issuer: ISSUER }, ); + }); + + it("records dcr when the metadata URL differs from the configured one", async () => { + const storage = makeCimdStorage(); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: "https://other.example.com/client-metadata.json", + }); + + await provider.saveClientInformation( + { client_id: METADATA_URL }, + { issuer: ISSUER }, + ); + expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER, { client_id: METADATA_URL }, - { registrationKind: "cimd", issuer: ISSUER }, + { registrationKind: "dcr", issuer: ISSUER }, ); }); - it("does not leak a stored cimd kind onto a different client_id", async () => { + it("records dcr when no CIMD registration was ever stored", async () => { + // The AS returns the configured metadata URL from a real registration + // (RFC 7591 §3.2 leaves the id opaque). With nothing recorded as CIMD + // under that id, the save is still DCR. const storage = makeStorage(); - vi.mocked(storage.getClientInformation).mockImplementation( - async (_url: string, preregistered?: boolean) => - preregistered ? undefined : { client_id: METADATA_URL }, - ); - vi.mocked(storage.getClientRegistrationKind).mockResolvedValue( - "cimd", - ); - const provider = makeProvider(storage); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: METADATA_URL, + }); await provider.saveClientInformation( - { client_id: "freshly-registered" }, + { client_id: METADATA_URL }, { issuer: ISSUER }, ); - expect(storage.getClientRegistrationKind).not.toHaveBeenCalled(); expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER, - { client_id: "freshly-registered" }, + { client_id: METADATA_URL }, { registrationKind: "dcr", issuer: ISSUER }, ); }); - it("falls back to dcr when storage has a matching id but no recorded kind", async () => { + it("records dcr when the stored kind under that id is not cimd", async () => { const storage = makeStorage(); vi.mocked(storage.getClientInformation).mockImplementation( async (_url: string, preregistered?: boolean) => - preregistered ? undefined : { client_id: "legacy-id" }, + preregistered ? undefined : { client_id: METADATA_URL }, ); - const provider = makeProvider(storage); + vi.mocked(storage.getClientRegistrationKind).mockResolvedValue("dcr"); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: METADATA_URL, + }); await provider.saveClientInformation( - { client_id: "legacy-id" }, + { client_id: METADATA_URL }, { issuer: ISSUER }, ); expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER, - { client_id: "legacy-id" }, + { client_id: METADATA_URL }, { registrationKind: "dcr", issuer: ISSUER }, ); }); it("an explicit registrationKind wins and consults no storage reads", async () => { - const storage = makeStorage(); + const storage = makeCimdStorage(); const provider = makeProvider(storage, vi.fn(), { clientMetadataUrl: METADATA_URL, }); diff --git a/core/auth/providers.ts b/core/auth/providers.ts index 5469e96b5..ed7b3556c 100644 --- a/core/auth/providers.ts +++ b/core/auth/providers.ts @@ -315,18 +315,29 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { /** * Resolve the registration kind for a save that carries no explicit one — that - * is, one the SDK made. Three SDK call sites reach here: back-stamping an - * existing registration with its `issuer`, the SDK's own CIMD write (whose - * `client_id` *is* the metadata-document URL), and a real dynamic - * registration. Only the last is `"dcr"`, so defaulting every unstamped save - * to it relabels a CIMD registration `Dynamic (DCR)` in Connection Info the - * moment the SDK binds it to an issuer — reported as #2242, where no - * `POST /register` was ever made. + * is, one the SDK made. SDK v2's `saveClientInformation` contract passes only + * `{ issuer }`, so the mechanism cannot be handed to us; treating every such + * save as DCR is what relabeled a CIMD registration `Dynamic (DCR)` in + * Connection Info the moment the SDK bound it to an issuer (#2242). * - * `client_id` is what tells the cases apart, so match on it rather than on the - * stored kind alone: a DCR `client_id` is minted by the authorization server, - * so a later DCR registration for the same server cannot inherit the earlier - * CIMD provenance. + * The claim is deliberately narrow — three conditions must all hold, and the + * decisive one is a registration *we ourselves recorded* as CIMD, not an + * inference about what the authorization server returned: + * + * 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 + * `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 — + * falls through to `"dcr"`. */ private async resolveSdkRegistrationKind( clientInformation: OAuthClientInformation, @@ -334,23 +345,19 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { ): Promise { const clientMetadataUrl = this.clientMetadataUrl?.trim(); if ( - clientMetadataUrl && - clientInformation.client_id === clientMetadataUrl + !clientMetadataUrl || + clientInformation.client_id !== clientMetadataUrl ) { - return "cimd"; + return "dcr"; } - // Falls back to the unkeyed slot our own pre-registration wrote, since the - // issuer slot does not exist yet on the save that creates it. Reading it - // covers a CIMD registration whose `clientMetadataUrl` config has since - // been cleared, so the provenance is not silently demoted. + // 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 || stored.client_id !== clientInformation.client_id) { - return "dcr"; - } + if (stored?.client_id !== clientMetadataUrl) return "dcr"; const storedKind = await this.storage.getClientRegistrationKind( this.serverUrl, issuer, From f3f68c731b63822090cfde7da0899b109a0dfcc6 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 00:44:49 -0400 Subject: [PATCH 03/10] 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 cef8985ad..928c8f8ee 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 ed7b3556c..a43ada0e1 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 { From 4c6a9fb376f115cce764fc97f0b446787d144a18 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 01:16:30 -0400 Subject: [PATCH 04/10] 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 | 107 ++++++++++++++---- core/auth/cimd.ts | 30 ++++- core/auth/providers.ts | 67 ++++++----- 4 files changed, 185 insertions(+), 66 deletions(-) diff --git a/clients/web/src/test/core/auth/cimd.test.ts b/clients/web/src/test/core/auth/cimd.test.ts index 63bc64a59..15aebd406 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 928c8f8ee..c560b5cc0 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,96 @@ 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 }, ); + expect( + await storage.getClientRegistrationKind(SERVER, ISSUER), + ).toBe("cimd"); return provider; } - it("keeps cimd when the resource resolves to a second issuer", async () => { + it("keeps cimd when a second CIMD-supporting issuer takes over", async () => { const storage = makeRealStorage(); - const provider = await bindFirstIssuer(storage); + 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 }, + ); expect( - await storage.getClientRegistrationKind(SERVER, ISSUER), + await storage.getClientRegistrationKind(SERVER, ISSUER_B), ).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(); + await storage.getClientRegistrationKind(SERVER, ISSUER), + ).toBe("cimd"); + }); + + it("records dcr when a second issuer without CIMD mints the same URL as its client_id", async () => { + const storage = makeRealStorage(); + const provider = await bindIssuerA(storage); - // The SDK's own CIMD branch, saving under the second issuer. + // 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 }, @@ -831,15 +893,16 @@ describe("OAuthNavigation", () => { expect( await storage.getClientRegistrationKind(SERVER, ISSUER_B), - ).toBe("cimd"); + ).toBe("dcr"); + // Issuer A's own provenance is untouched. expect( await storage.getClientRegistrationKind(SERVER, ISSUER), ).toBe("cimd"); }); - it("still records dcr for a second issuer that mints its own client_id", async () => { + it("records dcr for a second issuer that mints its own client_id", async () => { const storage = makeRealStorage(); - const provider = await bindFirstIssuer(storage); + 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 31614d1b3..5074f37c5 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 a43ada0e1..73ec1acce 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 { From 4297ef844948cee246cd3f905fce93f282af2f39 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 01:34:03 -0400 Subject: [PATCH 05/10] fix: record CIMD provenance as an issuer-keyed marker, not on the credential MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review (#2287), three findings: 1. **invalid-client recovery lost the provenance.** SDK v2 `auth()` answers `invalid_client` / `unauthorized_client` with `invalidateCredentials("client")` and an immediate retry; our clear drops the registration *and* its kind, so the retry's URL-based client-ID save landed with nothing recorded and was stored as `dcr`. Provenance now lives in `cimdClientMetadataUrl`, an issuer-keyed marker on the issuer slot that records a property of the AS rather than a credential — `clearClientInformation` deliberately leaves it alone. `ensureCimdClientRegistration` writes it, and withdraws it when the AS stops advertising CIMD, on every connect. 2. **Discovery became a hard network dependency.** Moving the existing-client check after discovery meant a well-known outage could fail a reconnect the SDK would have served from its persisted discovery state. Reuse `provider.discoveryState()` first, and treat a discovery failure as "skip pre-registration" rather than an error — this helper is an optimization over what `auth()` does for itself. 3. **Dropped an unjustified double cast** in the test fetch helper; the async signature is directly assignable to `typeof fetch`. 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 | 67 ++++++++++ .../test/core/auth/connection-state.test.ts | 2 + .../web/src/test/core/auth/providers.test.ts | 117 ++++++++++++------ .../src/test/core/mcp/oauthManager.test.ts | 2 + core/auth/cimd.ts | 93 ++++++++------ core/auth/oauth-storage.ts | 28 +++++ core/auth/providers.ts | 66 +++++----- core/auth/storage.ts | 17 +++ core/auth/store.ts | 15 +++ 9 files changed, 299 insertions(+), 108 deletions(-) diff --git a/clients/web/src/test/core/auth/cimd.test.ts b/clients/web/src/test/core/auth/cimd.test.ts index 15aebd406..2eac13950 100644 --- a/clients/web/src/test/core/auth/cimd.test.ts +++ b/clients/web/src/test/core/auth/cimd.test.ts @@ -24,6 +24,9 @@ describe("ensureCimdClientRegistration", () => { storage = { 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 () => {}), @@ -72,6 +75,12 @@ 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 () => { @@ -102,6 +111,13 @@ 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 () => { @@ -161,6 +177,57 @@ describe("ensureCimdClientRegistration", () => { ); }); + // #2242 (Copilot): the existing-client check moved after discovery, so this + // helper must not turn a well-known outage into a failed reconnect. It reuses + // the discovery state SDK `auth()` persists, and treats a discovery failure as + // "skip pre-registration" rather than an error. + it("reuses persisted discovery state instead of re-fetching", async () => { + storage.getDiscoveryState = vi.fn(async () => ({ + authorizationServerUrl: "http://127.0.0.1:9999", + authorizationServerMetadata: { + 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, + }, + })); + const fetchFn = vi.fn(async () => { + throw new Error("discovery must not run when state is cached"); + }); + + await ensureCimdClientRegistration({ + serverUrl: SERVER_URL, + provider: createProvider(storage), + fetchFn, + }); + + expect(fetchFn).not.toHaveBeenCalled(); + expect(storage.saveClientInformation).toHaveBeenCalledWith( + SERVER_URL, + { client_id: METADATA_URL }, + { registrationKind: "cimd", issuer: "http://127.0.0.1:9999" }, + ); + }); + + it("skips pre-registration when discovery fails, rather than throwing", async () => { + const fetchFn = vi.fn(async () => { + throw new Error("well-known endpoint is down"); + }); + + await expect( + ensureCimdClientRegistration({ + serverUrl: SERVER_URL, + provider: createProvider(storage), + fetchFn, + }), + ).resolves.toBeUndefined(); + + expect(storage.saveClientInformation).not.toHaveBeenCalled(); + // No marker is invented either — nothing was learned about the AS. + expect(storage.saveCimdClientMetadataUrl).not.toHaveBeenCalled(); + }); + 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. 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 d328b01b6..44a07110b 100644 --- a/clients/web/src/test/core/auth/connection-state.test.ts +++ b/clients/web/src/test/core/auth/connection-state.test.ts @@ -61,6 +61,8 @@ 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 c560b5cc0..90dbc8532 100644 --- a/clients/web/src/test/core/auth/providers.test.ts +++ b/clients/web/src/test/core/auth/providers.test.ts @@ -221,6 +221,8 @@ 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), @@ -641,20 +643,16 @@ describe("OAuthNavigation", () => { const ISSUER = "https://as.example.com"; const METADATA_URL = "https://app.example.com/client-metadata.json"; - /** Storage already holding the CIMD pre-registration for METADATA_URL. */ + /** Storage holding this AS's CIMD marker for METADATA_URL. */ function makeCimdStorage(): OAuthStorage { const storage = makeStorage(); - vi.mocked(storage.getClientInformation).mockImplementation( - async (_url: string, preregistered?: boolean) => - preregistered ? undefined : { client_id: METADATA_URL }, - ); - vi.mocked(storage.getClientRegistrationKind).mockResolvedValue( - "cimd", + vi.mocked(storage.getCimdClientMetadataUrl).mockResolvedValue( + METADATA_URL, ); return storage; } - it("keeps cimd when CIMD is configured and the stored registration matches", async () => { + it("keeps cimd when CIMD is configured and this issuer carries the marker", async () => { const storage = makeCimdStorage(); const provider = makeProvider(storage, vi.fn(), { clientMetadataUrl: METADATA_URL, @@ -665,11 +663,8 @@ describe("OAuthNavigation", () => { { issuer: ISSUER }, ); - // Reads the dynamic slot for this issuer, which falls back to the - // unkeyed slot the pre-registration wrote. - expect(storage.getClientInformation).toHaveBeenCalledWith( + expect(storage.getCimdClientMetadataUrl).toHaveBeenCalledWith( SERVER, - false, ISSUER, ); expect(storage.saveClientInformation).toHaveBeenCalledWith( @@ -679,49 +674,54 @@ describe("OAuthNavigation", () => { ); }); - it("records dcr for a server-minted client_id while CIMD is configured", async () => { + // 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(); const provider = makeProvider(storage, vi.fn(), { clientMetadataUrl: METADATA_URL, }); + await provider.invalidateCredentials("client"); await provider.saveClientInformation( - { client_id: "dcr-minted-id" }, + { client_id: METADATA_URL }, { issuer: ISSUER }, ); - // The id is not the metadata URL, so nothing is read and nothing is - // carried forward. - expect(storage.getClientInformation).not.toHaveBeenCalled(); - expect(storage.getClientRegistrationKind).not.toHaveBeenCalled(); + expect(storage.clearClientInformation).toHaveBeenCalledWith(SERVER); expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER, - { client_id: "dcr-minted-id" }, - { registrationKind: "dcr", issuer: ISSUER }, + { client_id: METADATA_URL }, + { registrationKind: "cimd", issuer: ISSUER }, ); }); - it("records dcr when CIMD is not configured, even if storage says cimd", async () => { + it("records dcr for a server-minted client_id while CIMD is configured", async () => { const storage = makeCimdStorage(); - const provider = makeProvider(storage); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: METADATA_URL, + }); await provider.saveClientInformation( - { client_id: METADATA_URL }, + { client_id: "dcr-minted-id" }, { issuer: ISSUER }, ); + // The id is not the metadata URL, so the marker is never consulted. + expect(storage.getCimdClientMetadataUrl).not.toHaveBeenCalled(); expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER, - { client_id: METADATA_URL }, + { client_id: "dcr-minted-id" }, { registrationKind: "dcr", issuer: ISSUER }, ); }); - it("records dcr when the metadata URL differs from the configured one", async () => { + it("records dcr when CIMD is not configured, even if the marker is set", async () => { const storage = makeCimdStorage(); - const provider = makeProvider(storage, vi.fn(), { - clientMetadataUrl: "https://other.example.com/client-metadata.json", - }); + const provider = makeProvider(storage); await provider.saveClientInformation( { client_id: METADATA_URL }, @@ -735,11 +735,11 @@ describe("OAuthNavigation", () => { ); }); - it("records dcr when no CIMD registration was ever stored", async () => { - // The AS returns the configured metadata URL from a real registration - // (RFC 7591 §3.2 leaves the id opaque). With nothing recorded as CIMD - // under that id, the save is still DCR. + 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, }); @@ -756,13 +756,11 @@ describe("OAuthNavigation", () => { ); }); - it("records dcr when the stored kind under that id is not cimd", async () => { + 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(); - vi.mocked(storage.getClientInformation).mockImplementation( - async (_url: string, preregistered?: boolean) => - preregistered ? undefined : { client_id: METADATA_URL }, - ); - vi.mocked(storage.getClientRegistrationKind).mockResolvedValue("dcr"); const provider = makeProvider(storage, vi.fn(), { clientMetadataUrl: METADATA_URL, }); @@ -802,8 +800,8 @@ describe("OAuthNavigation", () => { * 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) => { + function discoveryFetch(issuer: string, cimd: boolean): typeof fetch { + return async (input: RequestInfo | URL) => { const url = String(input); if (url.includes("/.well-known/oauth-protected-resource")) { return new Response( @@ -827,7 +825,7 @@ describe("OAuthNavigation", () => { ); } throw new Error(`unexpected fetch: ${url}`); - }) as unknown as typeof fetch; + }; } /** Issuer A pre-registers via CIMD, then the SDK binds it. */ @@ -900,6 +898,43 @@ describe("OAuthNavigation", () => { ).toBe("cimd"); }); + // 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 () => { + const storage = makeRealStorage(); + const provider = await bindIssuerA(storage); + expect(await storage.getCimdClientMetadataUrl(SERVER, ISSUER)).toBe( + METADATA_URL, + ); + + await provider.invalidateCredentials("client"); + + // The credential and its kind are 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. + await provider.saveClientInformation( + { client_id: METADATA_URL }, + { issuer: ISSUER }, + ); + + 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); diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index 96d1df80c..1da62370b 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -70,6 +70,8 @@ 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 5074f37c5..c50365ee8 100644 --- a/core/auth/cimd.ts +++ b/core/auth/cimd.ts @@ -27,50 +27,69 @@ export async function ensureCimdClientRegistration(params: { const clientMetadataUrl = params.provider.clientMetadataUrl?.trim(); if (!clientMetadataUrl) return; - let resourceMetadata; - try { - resourceMetadata = await discoverOAuthProtectedResourceMetadata( - params.serverUrl, - { resourceMetadataUrl: params.resourceMetadataUrl }, - // The same fetch the AS-metadata leg below uses. On web that is - // `createRemoteFetch`, which proxies through the backend to sidestep - // CORS — on the global `fetch` this leg would fail in the browser, be - // swallowed by the catch, and leave CIMD probing the wrong - // authorization server (Copilot). - params.fetchFn, + // Prefer the discovery state SDK `auth()` itself persists and reuses. Without + // this, moving the existing-client check after discovery would turn a + // temporary well-known outage into a failed reconnect, even where the SDK + // could have proceeded from cache (Copilot). + let metadata = (await params.provider.discoveryState()) + ?.authorizationServerMetadata; + + if (!metadata) { + let resourceMetadata; + try { + resourceMetadata = await discoverOAuthProtectedResourceMetadata( + params.serverUrl, + { resourceMetadataUrl: params.resourceMetadataUrl }, + // The same fetch the AS-metadata leg below uses. On web that is + // `createRemoteFetch`, which proxies through the backend to sidestep + // CORS — on the global `fetch` this leg would fail in the browser, be + // swallowed by the catch, and leave CIMD probing the wrong + // authorization server (Copilot). + params.fetchFn, + ); + } catch { + resourceMetadata = undefined; + } + + try { + // Walks the path-scoped authorization-server URL before the bare origin, so + // a server hosted under a path is probed where it actually publishes its + // metadata rather than only at the domain root (#2110). + metadata = await discoverAuthorizationServerMetadataForServer( + params.serverUrl, + resourceMetadata, + params.fetchFn, + ); + } catch { + // Pre-registration is an optimization over what SDK `auth()` does for + // itself, so a discovery failure here must never fail the connection: bail + // out and let `auth()` run its own discovery and error handling. + return; + } + } + + const issuer = metadata?.issuer; + + // Record — or withdraw — this AS's CIMD marker before anything else, so it + // stays current rather than only ever being written once. It is keyed by + // issuer and is not a credential, so `invalidateCredentials("client")` leaves + // it alone; see `IssuerBoundOAuthState.cimdClientMetadataUrl`. + if (issuer) { + await params.provider.saveCimdClientMetadataUrl( + issuer, + metadata?.client_id_metadata_document_supported + ? clientMetadataUrl + : undefined, ); - } catch { - resourceMetadata = undefined; } - // Walks the path-scoped authorization-server URL before the bare origin, so a - // server hosted under a path is probed where it actually publishes its - // metadata rather than only at the domain root (#2110). - const metadata = await discoverAuthorizationServerMetadataForServer( - params.serverUrl, - resourceMetadata, - params.fetchFn, - ); 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 + // ⚠️ 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 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, ); diff --git a/core/auth/oauth-storage.ts b/core/auth/oauth-storage.ts index 68b7810c2..26f76f391 100644 --- a/core/auth/oauth-storage.ts +++ b/core/auth/oauth-storage.ts @@ -186,6 +186,34 @@ 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 73ec1acce..d62256438 100644 --- a/core/auth/providers.ts +++ b/core/auth/providers.ts @@ -321,38 +321,35 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { * 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 registration *we ourselves recorded* as CIMD, not an - * inference about what the authorization server returned: + * decisive one is a fact *we recorded about this authorization server*, not an + * inference about what it returned: * * 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 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`. + * 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. * * 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 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"`. + * 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"`. * - * ⚠️ (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. + * ⚠️ (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). * - * 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. + * ⚠️ (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`. */ private async resolveSdkRegistrationKind( clientInformation: OAuthClientInformation, @@ -365,19 +362,28 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { ) { return "dcr"; } - const stored = await this.storage.getClientInformation( + const marker = await this.storage.getCimdClientMetadataUrl( this.serverUrl, - false, issuer, ); - if (stored?.client_id !== clientMetadataUrl) return "dcr"; - const storedKind = await this.storage.getClientRegistrationKind( + return marker === clientMetadataUrl ? "cimd" : "dcr"; + } + + /** @see OAuthStorage.getCimdClientMetadataUrl */ + async cimdClientMetadataUrl(issuer?: string): Promise { + return await this.storage.getCimdClientMetadataUrl(this.serverUrl, issuer); + } + + /** @see OAuthStorage.saveCimdClientMetadataUrl */ + async saveCimdClientMetadataUrl( + issuer: string, + clientMetadataUrl: string | undefined, + ): Promise { + await this.storage.saveCimdClientMetadataUrl( this.serverUrl, issuer, + clientMetadataUrl, ); - // `"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 { diff --git a/core/auth/storage.ts b/core/auth/storage.ts index edc6451ec..559f118d5 100644 --- a/core/auth/storage.ts +++ b/core/auth/storage.ts @@ -81,6 +81,23 @@ 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 d5e135118..6243600c8 100644 --- a/core/auth/store.ts +++ b/core/auth/store.ts @@ -30,6 +30,21 @@ 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; } /** From 4350731f366a4e00fad8a18e8e19205f8dfc68d8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 01:35:03 -0400 Subject: [PATCH 06/10] docs: correct the saveClientInformation contract comment Copilot review (#2287, suppressed comment): the comment still said internal callers supply no issuer, which stopped being true when ensureCimdClientRegistration started binding its save to the discovered issuer. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA Signed-off-by: cliffhall --- core/auth/providers.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/core/auth/providers.ts b/core/auth/providers.ts index d62256438..18af9ed0b 100644 --- a/core/auth/providers.ts +++ b/core/auth/providers.ts @@ -293,9 +293,12 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { // SDK v2's `OAuthClientProvider.saveClientInformation` passes an // `OAuthClientInformationContext` ({ issuer }); our own DCR/CIMD callers // pass `SaveClientInformationOptions` ({ registrationKind }). Accept either - // and read whichever keys are present: the SDK supplies `issuer` (SEP-2352 - // per-AS keying) and no kind — `resolveSdkRegistrationKind` recovers it — - // while our callers supply the registration kind and no issuer yet. + // and read whichever keys are present. The SDK supplies `issuer` (SEP-2352 + // per-AS keying) and never a kind, so `resolveSdkRegistrationKind` recovers + // one. Our own callers always supply the kind, and supply the `issuer` too + // when they know it — `ensureCimdClientRegistration` does, having just + // discovered it; the unkeyed slot is only for the case where AS metadata + // carried no `issuer` at all. options?: SaveClientInformationOptions | OAuthClientInformationContext, ): Promise { const issuer = options && "issuer" in options ? options.issuer : undefined; From abb6a47f7d7a814b23667b5ba97c330af390efa5 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 01:46:59 -0400 Subject: [PATCH 07/10] fix: earn the CIMD marker, rather than writing it on AS support alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review (#2287): the marker was written as soon as the AS advertised `client_id_metadata_document_supported`, before checking what registration already existed. RFC 7591 §3.2 leaves a dynamically issued `client_id` opaque, so an existing DCR may carry the configured metadata URL — and marking the issuer then relabels that real dynamic registration as CIMD on the SDK's next issuer stamp. The marker now records both facts it is read for: that this AS accepts the URL as a `client_id`, *and* that the registration standing for it got there through CIMD. It is written where this helper establishes the registration itself, reaffirmed for one already recorded as `cimd` under that exact URL, and withdrawn otherwise — which also covers a static client and an AS that has stopped advertising CIMD. Covered at both levels: a mocked case asserting the marker is withdrawn for an existing DCR on the same URL, and an end-to-end real-storage case asserting such a registration is still `dcr` after the SDK issuer-stamps it. 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 | 77 +++++++++++++++++++ .../web/src/test/core/auth/providers.test.ts | 37 +++++++++ core/auth/cimd.ts | 73 +++++++++++------- core/auth/providers.ts | 13 +++- 4 files changed, 171 insertions(+), 29 deletions(-) diff --git a/clients/web/src/test/core/auth/cimd.test.ts b/clients/web/src/test/core/auth/cimd.test.ts index 2eac13950..5b5db72ae 100644 --- a/clients/web/src/test/core/auth/cimd.test.ts +++ b/clients/web/src/test/core/auth/cimd.test.ts @@ -17,6 +17,28 @@ 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; @@ -228,6 +250,58 @@ describe("ensureCimdClientRegistration", () => { 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 () => { // Dynamic slot only — a preregistered hit would short-circuit // `clientInformation()` before it ever reaches the issuer-keyed read. @@ -235,6 +309,9 @@ describe("ensureCimdClientRegistration", () => { async (_url: string, preregistered?: boolean) => preregistered ? undefined : { client_id: "existing-client" }, ); + storage.getClientRegistrationKind = vi.fn( + async (): Promise<"dcr"> => "dcr", + ); const fetchFn = vi.fn(async (input: RequestInfo | URL) => { const url = String(input); diff --git a/clients/web/src/test/core/auth/providers.test.ts b/clients/web/src/test/core/auth/providers.test.ts index 90dbc8532..87dee09f4 100644 --- a/clients/web/src/test/core/auth/providers.test.ts +++ b/clients/web/src/test/core/auth/providers.test.ts @@ -898,6 +898,43 @@ describe("OAuthNavigation", () => { ).toBe("cimd"); }); + // Copilot: an AS advertising CIMD does not make an *existing* dynamic + // registration a CIMD one. RFC 7591 §3.2 leaves the id opaque, so a + // real DCR may carry the metadata URL; end to end, it must stay `dcr`. + it("does not relabel an existing DCR whose client_id is the metadata URL", async () => { + const storage = makeRealStorage(); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: METADATA_URL, + }); + + // A real dynamic registration that happens to use the same URL. + await provider.saveClientInformation( + { client_id: METADATA_URL }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + + // The AS does advertise CIMD, so the pre-registration runs and finds + // that registration already in place. + await ensureCimdClientRegistration({ + serverUrl: SERVER, + 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 }, + { issuer: ISSUER }, + ); + + expect( + await storage.getClientRegistrationKind(SERVER, ISSUER), + ).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 diff --git a/core/auth/cimd.ts b/core/auth/cimd.ts index c50365ee8..edf874389 100644 --- a/core/auth/cimd.ts +++ b/core/auth/cimd.ts @@ -1,5 +1,4 @@ import { discoverOAuthProtectedResourceMetadata } from "@modelcontextprotocol/client"; -import type { OAuthClientInformation } from "@modelcontextprotocol/client"; import { discoverAuthorizationServerMetadataForServer } from "./discovery.js"; import type { BaseOAuthClientProvider } from "./providers.js"; @@ -69,40 +68,58 @@ export async function ensureCimdClientRegistration(params: { } const issuer = metadata?.issuer; + const supportsCimd = metadata?.client_id_metadata_document_supported === true; - // Record — or withdraw — this AS's CIMD marker before anything else, so it - // stays current rather than only ever being written once. It is keyed by - // issuer and is not a credential, so `invalidateCredentials("client")` leaves - // it alone; see `IssuerBoundOAuthState.cimdClientMetadataUrl`. - if (issuer) { - await params.provider.saveCimdClientMetadataUrl( - issuer, - metadata?.client_id_metadata_document_supported - ? clientMetadataUrl - : undefined, - ); - } + /** + * 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 (!metadata?.client_id_metadata_document_supported) return; + if (!supportsCimd) { + // Withdrawn, not merely left alone — an AS that stops advertising CIMD + // stops being treated as one. + await setMarker(undefined); + 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 still - // answers the static case first, since `clientInformation` checks the - // preregistered slot before any issuer slot. + // 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. const existing = await params.provider.clientInformation( issuer ? { issuer } : undefined, ); - if (existing?.client_id) return; + 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; + } - 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 }), - }); + // From here this call *is* the CIMD registration, so the marker is earned. + await setMarker(clientMetadataUrl); + await params.provider.saveClientInformation( + { client_id: clientMetadataUrl }, + { + 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 18af9ed0b..f64f7a922 100644 --- a/core/auth/providers.ts +++ b/core/auth/providers.ts @@ -9,7 +9,11 @@ import type { OAuthMetadata, OAuthDiscoveryState, } from "@modelcontextprotocol/client"; -import type { OAuthStorage, SaveClientInformationOptions } from "./storage.js"; +import type { + OAuthStorage, + SaveClientInformationOptions, + OAuthClientRegistrationKind, +} from "./storage.js"; import { generateOAuthState } from "./utils.js"; import { applyAuthorizationParams } from "./authorizationParams.js"; import { scopeForDeclinedRefreshGrant } from "./scopes.js"; @@ -377,6 +381,13 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { 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, From 62737900772f8f5953168c6b3ef870a39c1ea585 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 02:03:12 -0400 Subject: [PATCH 08/10] fix: read the SDK's own discovery state instead of a marker of our own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review (#2287), two findings — and taken together they said the marker was the wrong mechanism, so this removes it rather than patching it again: - A transient failure in our CIMD preflight recorded no marker, so a *successful* SDK fallback discovery that advertised CIMD was still stored as `dcr`. - `existing` and `existingKind` could describe different registrations, because `clientInformation()` answers preregistered-first while `clientRegistrationKind()` prefers the issuer's dynamic slot. SDK v2 `auth()` persists the authorization-server metadata via `saveDiscoveryState` *before* it reads or writes client information, and it takes its URL-based-client-ID branch — rather than `registerClient` — exactly when that metadata advertises `client_id_metadata_document_supported` and a `clientMetadataUrl` is configured. So the branch is not inferred at all now: it is read back from the state the SDK itself just wrote. `resolveSdkRegistrationKind` becomes two cases. A save for an issuer that already has a registration under this `client_id` is a back-stamp, answered by that registration's recorded kind — so an existing DCR on the metadata URL stays `dcr`. A save for an issuer with none is a new registration, answered by the discovery state, guarded on the metadata describing that same issuer. This drops `cimdClientMetadataUrl` and its storage accessors entirely, and restores `ensureCimdClientRegistration` to a plain issuer-bound pre-registration. Invalid-client recovery, second-issuer CIMD, second-issuer DCR minting the metadata URL, and preflight failure are all consequences of the two cases rather than separate mechanisms. 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 | 91 --------- .../test/core/auth/connection-state.test.ts | 2 - .../web/src/test/core/auth/providers.test.ts | 182 ++++++++++++------ .../src/test/core/mcp/oauthManager.test.ts | 2 - core/auth/cimd.ts | 43 +---- core/auth/oauth-storage.ts | 28 --- core/auth/providers.ts | 109 +++++------ core/auth/storage.ts | 17 -- core/auth/store.ts | 15 -- 9 files changed, 183 insertions(+), 306 deletions(-) diff --git a/clients/web/src/test/core/auth/cimd.test.ts b/clients/web/src/test/core/auth/cimd.test.ts index 5b5db72ae..fee1b1432 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 44a07110b..d328b01b6 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 87dee09f4..1992e7fcf 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,54 +690,63 @@ 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.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 }, - { registrationKind: "cimd", issuer: ISSUER }, + { registrationKind: "dcr", issuer: ISSUER }, ); }); - it("records dcr for a server-minted client_id while CIMD is configured", async () => { - const storage = makeCimdStorage(); + // 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.saveClientInformation( - { client_id: "dcr-minted-id" }, + { client_id: METADATA_URL }, { issuer: ISSUER }, ); - // The id is not the metadata URL, so the marker is never consulted. - expect(storage.getCimdClientMetadataUrl).not.toHaveBeenCalled(); expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER, - { client_id: "dcr-minted-id" }, - { registrationKind: "dcr", issuer: ISSUER }, + { client_id: METADATA_URL }, + { registrationKind: "cimd", issuer: ISSUER }, ); }); - it("records dcr when CIMD is not configured, even if the marker is set", async () => { - const storage = makeCimdStorage(); - const provider = makeProvider(storage); + 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 }, @@ -735,11 +760,9 @@ describe("OAuthNavigation", () => { ); }); - it("records dcr when the marker names a different metadata URL", async () => { + it("records dcr when the discovery state describes a different issuer", async () => { const storage = makeStorage(); - vi.mocked(storage.getCimdClientMetadataUrl).mockResolvedValue( - "https://other.example.com/client-metadata.json", - ); + seedDiscovery(storage, "https://as-other.example.com", true); const provider = makeProvider(storage, vi.fn(), { clientMetadataUrl: METADATA_URL, }); @@ -756,15 +779,33 @@ describe("OAuthNavigation", () => { ); }); - 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(); + 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, }); + await provider.saveClientInformation( + { client_id: "dcr-minted-id" }, + { issuer: ISSUER }, + ); + + // 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" }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + }); + + 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( { client_id: METADATA_URL }, { issuer: ISSUER }, @@ -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 1da62370b..96d1df80c 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 edf874389..d17ce7202 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 26f76f391..68b7810c2 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 f64f7a922..92512c0a2 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( - this.serverUrl, - issuer, - ); - return marker === clientMetadataUrl ? "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( + // 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, - clientMetadataUrl, ); + 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"; + } + + // 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 559f118d5..edc6451ec 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 6243600c8..d5e135118 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; } /** From be08f6d37d0e9fdc4873c9ca2b176db2b9db72ff Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 02:28:07 -0400 Subject: [PATCH 09/10] test: restore end-to-end coverage of resolveSdkRegistrationKind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review (#2287): the CIMD E2E assertion had gone vacuous. Once the pre-registration started writing the issuer-keyed slot itself, the SDK found an already-stamped credential and never called `saveClientInformation` at all — so the assertion held even with the resolver gutted. Verified: reverting the dispatch to `: "dcr"` left all 34 tests passing. Add a case that seeds the legacy *unkeyed* CIMD registration — what every pre-SEP-2352 install has on disk, and the exact shape #2242 was reported against. The SDK back-stamps it with the issuer, which is the save that used to relabel it `Dynamic (DCR)`, so the resolver is genuinely on the path. That case now fails on both SSE and Streamable HTTP when the dispatch is reverted, which is the reported bug reproduced across the integration boundary. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA Signed-off-by: cliffhall --- .../mcp/inspectorClient-oauth-e2e.test.ts | 97 +++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts index 9e5431b7d..dfbedba9d 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts @@ -374,6 +374,103 @@ describe("InspectorClient OAuth E2E", () => { // #2242: the metadata-document URL is the client_id, and the stored // provenance still says CIMD after the SDK bound the registration to // the issuer — no `POST /register` ever happened. + // + // ⚠️ This assertion alone does not exercise `resolveSdkRegistrationKind`: + // the CIMD pre-registration now writes the issuer-keyed slot itself, so + // the SDK finds an already-stamped credential and never calls + // `saveClientInformation`. The test below covers the resolver across the + // integration boundary (Copilot). + const oauthState = await client.getOAuthState(); + expect(oauthState?.client).toMatchObject({ + clientId: metadataUrl, + registrationKind: "cimd", + }); + }); + + // The reported #2242 shape: an unkeyed CIMD registration — what every + // pre-SEP-2352 install has on disk, and what the pre-registration wrote + // before it knew the issuer. The SDK back-stamps it, which is the save + // that used to relabel it `Dynamic (DCR)`. This is the case that puts + // `resolveSdkRegistrationKind` on the path end to end. + it("keeps CIMD provenance when the SDK issuer-stamps an unkeyed registration", async () => { + const testRedirectUrl = "http://localhost:3001/oauth/callback"; + + const clientMetadata: ClientMetadataDocument = { + redirect_uris: [testRedirectUrl], + token_endpoint_auth_method: "none", + grant_types: ["authorization_code", "refresh_token"], + response_types: ["code"], + client_name: "MCP Inspector Test Client", + client_uri: "https://github.com/modelcontextprotocol/inspector", + scope: "mcp", + }; + + metadataServer = await createClientMetadataServer(clientMetadata); + const metadataUrl = metadataServer.url; + + const serverConfig = { + ...getDefaultServerConfig(), + serverType: transport.serverType, + ...createOAuthTestServerConfig({ + requireAuth: true, + supportCIMD: true, + }), + }; + + server = new TestServerHttp(serverConfig); + const port = await server.start(); + const serverUrl = `http://localhost:${port}`; + await waitForOAuthWellKnown(serverUrl); + + const oauthConfig = createTestOAuthConfig({ + mode: "cimd", + clientMetadataUrl: metadataUrl, + redirectUrl: testRedirectUrl, + }); + + const mcpUrl = `${serverUrl}${transport.endpoint}`; + // Seed the legacy unkeyed slot: a CIMD registration with no issuer. + await oauthConfig.storage.saveClientInformation( + mcpUrl, + { client_id: metadataUrl }, + { registrationKind: "cimd" }, + ); + + const clientConfig: InspectorClientOptions = { + environment: { + transport: createTransportNode, + oauth: { + storage: oauthConfig.storage, + navigation: oauthConfig.navigation, + redirectUrlProvider: oauthConfig.redirectUrlProvider, + }, + }, + oauth: { + clientId: oauthConfig.clientId, + clientSecret: oauthConfig.clientSecret, + clientMetadataUrl: oauthConfig.clientMetadataUrl, + scope: oauthConfig.scope, + }, + }; + + client = new InspectorClient( + { + type: transport.clientType, + url: mcpUrl, + } as MCPServerConfig, + clientConfig, + ); + + const authUrl = await client.authenticate(); + if (!authUrl) throw new Error("Expected authorization URL"); + + const { code: authCode, iss: authCodeIss } = + await completeOAuthAuthorization(authUrl); + await client.completeOAuthFlow(authCode, authCodeIss); + await client.connect(); + + expect(client.getStatus()).toBe("connected"); + const oauthState = await client.getOAuthState(); expect(oauthState?.client).toMatchObject({ clientId: metadataUrl, From aca522de6a166d5bf5788ea8245ee4bbf1d6554f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 02:38:13 -0400 Subject: [PATCH 10/10] fix: delegate issuer context and discovery state through the EMA wrapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review (#2287): `EmaTransportOAuthProvider` forwarded `clientInformation` / `saveClientInformation` without the SDK's `ctx`, and implemented neither `discoveryState` nor `saveDiscoveryState`. Since the wrapper does expose `clientMetadataUrl`, an EMA connection can still take SDK `auth()`'s URL-based-client-ID branch — and the inner provider then saw `issuer === undefined` with no discovery state to read back, so the CIMD write was recorded as DCR. Forward `ctx` on both, and delegate the two discovery-state methods to the inner provider. Both were pre-existing SEP-2352 gaps in their own right: dropping the issuer put every EMA read and write on the unkeyed slot, and the missing discovery state meant the SDK re-discovered on every call and warned that it could not run its callback-leg authorization-server binding check. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA Signed-off-by: cliffhall --- .../core/auth/ema/transportProvider.test.ts | 50 +++++++++++++++++-- core/auth/ema/transportProvider.ts | 31 ++++++++++-- 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/clients/web/src/test/core/auth/ema/transportProvider.test.ts b/clients/web/src/test/core/auth/ema/transportProvider.test.ts index ff66653e4..6079a36c4 100644 --- a/clients/web/src/test/core/auth/ema/transportProvider.test.ts +++ b/clients/web/src/test/core/auth/ema/transportProvider.test.ts @@ -65,6 +65,8 @@ interface FakeInner { clearCapturedAuthUrl: ReturnType; saveCodeVerifier: ReturnType; codeVerifier: ReturnType; + saveDiscoveryState: ReturnType; + discoveryState: ReturnType; } function createInner(): FakeInner { @@ -82,6 +84,8 @@ function createInner(): FakeInner { clearCapturedAuthUrl: vi.fn(), saveCodeVerifier: vi.fn(), codeVerifier: vi.fn(() => "verifier-xyz"), + saveDiscoveryState: vi.fn(), + discoveryState: vi.fn(), }; } @@ -118,14 +122,54 @@ describe("EmaTransportOAuthProvider", () => { expect(await provider.codeVerifier()).toBe("verifier-xyz"); await provider.saveClientInformation({ client_id: "new" } as never); - expect(inner.saveClientInformation).toHaveBeenCalledWith({ - client_id: "new", - }); + expect(inner.saveClientInformation).toHaveBeenCalledWith( + { client_id: "new" }, + undefined, + ); await provider.saveCodeVerifier("cv"); expect(inner.saveCodeVerifier).toHaveBeenCalledWith("cv"); }); + // SEP-2352: the wrapper used to drop the SDK's `ctx`, so every EMA read and + // write landed on the unkeyed slot — and, since #2242, the registration-kind + // resolver had no issuer to check and recorded a CIMD registration made over + // an EMA connection as DCR (Copilot). + it("forwards the SDK issuer context on client-information reads and writes", async () => { + const ctx = { issuer: "https://as.example.com" }; + + await provider.clientInformation(ctx); + expect(inner.clientInformation).toHaveBeenCalledWith(ctx); + + await provider.saveClientInformation({ client_id: "new" } as never, ctx); + expect(inner.saveClientInformation).toHaveBeenCalledWith( + { client_id: "new" }, + ctx, + ); + }); + + // Without these the SDK persists no discovery state for an EMA connection, so + // it re-discovers every call, cannot run its callback-leg AS binding check, + // and leaves the registration-kind resolver nothing to read back. + it("delegates discovery state to the inner provider", async () => { + const state = { + authorizationServerUrl: "https://as.example.com", + authorizationServerMetadata: { + issuer: "https://as.example.com", + authorization_endpoint: "https://as.example.com/authorize", + token_endpoint: "https://as.example.com/token", + response_types_supported: ["code"], + }, + }; + + await provider.saveDiscoveryState(state); + expect(inner.saveDiscoveryState).toHaveBeenCalledWith(state); + + inner.discoveryState.mockReturnValue(state); + expect(await provider.discoveryState()).toEqual(state); + expect(inner.discoveryState).toHaveBeenCalled(); + }); + it("tokens() returns stored tokens when the access token is still usable", async () => { const stored: OAuthTokens = { access_token: VALID_ACCESS_TOKEN, diff --git a/core/auth/ema/transportProvider.ts b/core/auth/ema/transportProvider.ts index fc36ea107..07c5c0c79 100644 --- a/core/auth/ema/transportProvider.ts +++ b/core/auth/ema/transportProvider.ts @@ -1,7 +1,9 @@ import type { OAuthClientProvider } from "@modelcontextprotocol/client"; import type { + OAuthClientInformationContext, OAuthClientInformationMixed, OAuthClientMetadata, + OAuthDiscoveryState, OAuthTokens, } from "@modelcontextprotocol/client"; import type { BaseOAuthClientProvider } from "../providers.js"; @@ -50,17 +52,40 @@ export class EmaTransportOAuthProvider implements OAuthClientProvider { return this.inner.state(); } - clientInformation(): + // SEP-2352: `ctx` carries the authorization-server `issuer` the SDK resolved, + // and the inner provider keys registrations by it. Dropping it here made every + // EMA read and write land on the unkeyed slot — and, since #2242, left + // `resolveSdkRegistrationKind` with no issuer to check, so a CIMD registration + // made over an EMA connection was recorded as DCR (Copilot). + clientInformation( + ctx?: OAuthClientInformationContext, + ): | OAuthClientInformationMixed | undefined | Promise { - return this.inner.clientInformation(); + return this.inner.clientInformation(ctx); } saveClientInformation( clientInformation: OAuthClientInformationMixed, + ctx?: OAuthClientInformationContext, ): void | Promise { - return this.inner.saveClientInformation(clientInformation); + return this.inner.saveClientInformation(clientInformation, ctx); + } + + // Without these the SDK persists no discovery state for an EMA connection, so + // it re-discovers on every call, cannot perform its SEP-2352 callback-leg + // authorization-server binding check (it warns as much), and — since #2242 — + // leaves the registration-kind resolver nothing to read back. + saveDiscoveryState(state: OAuthDiscoveryState): void | Promise { + return this.inner.saveDiscoveryState(state); + } + + discoveryState(): + | OAuthDiscoveryState + | undefined + | Promise { + return this.inner.discoveryState(); } async tokens(): Promise {