fix: earn the CIMD marker, rather than writing it on AS support alone

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>

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);

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

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 (!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 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 }),
+    },
+  );
 }

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<OAuthClientRegistrationKind | undefined> {
+    return await this.storage.getClientRegistrationKind(this.serverUrl, issuer);
+  }
+
   /** @see OAuthStorage.saveCimdClientMetadataUrl */
   async saveCimdClientMetadataUrl(
     issuer: string,