From 4789cb954e2c5b46da285a335277c3b75eb65cdb Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 00:04:40 -0400 Subject: [PATCH] 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 fa2d81a2..3998f118 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 a2778b87..9e5431b7 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 0447b729..5469e96b 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;