diff --git a/clients/web/src/test/core/auth/cimd.test.ts b/clients/web/src/test/core/auth/cimd.test.ts index 2eac1395..5b5db72a 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 90dbc853..87dee09f 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 c50365ee..edf87438 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 (!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 }), + }, + ); } diff --git a/core/auth/providers.ts b/core/auth/providers.ts index 18af9ed0..f64f7a92 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,