diff --git a/clients/web/src/test/core/auth/cimd.test.ts b/clients/web/src/test/core/auth/cimd.test.ts index 63bc64a59..fee1b1432 100644 --- a/clients/web/src/test/core/auth/cimd.test.ts +++ b/clients/web/src/test/core/auth/cimd.test.ts @@ -24,6 +24,7 @@ describe("ensureCimdClientRegistration", () => { storage = { getClientInformation: vi.fn(async () => undefined), saveClientInformation: vi.fn(async () => {}), + getDiscoveryState: vi.fn(async () => undefined), getScope: vi.fn().mockResolvedValue(undefined), getTokens: vi.fn(async () => undefined), saveTokens: vi.fn(async () => {}), @@ -62,12 +63,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 +158,104 @@ 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", + // #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(); + }); + + 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" }, + ); + storage.getClientRegistrationKind = vi.fn( + async (): Promise<"dcr"> => "dcr", + ); + + 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/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/clients/web/src/test/core/auth/providers.test.ts b/clients/web/src/test/core/auth/providers.test.ts index fa2d81a22..1992e7fcf 100644 --- a/clients/web/src/test/core/auth/providers.test.ts +++ b/clients/web/src/test/core/auth/providers.test.ts @@ -7,6 +7,10 @@ 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 { ensureCimdClientRegistration } from "@inspector/core/auth/cimd.js"; import { BrowserNavigation, BrowserOAuthClientProvider, @@ -216,6 +220,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 +243,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 +632,448 @@ 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"; + + /** 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.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 back-stamping a registration recorded as cimd", async () => { + const storage = makeCimdStorage(); + 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 }, + ); + }); + + // RFC 7591 §3.2 leaves a dynamically issued `client_id` opaque, so an + // existing DCR may carry the configured metadata URL. Back-stamping it + // must not relabel it (Copilot). + it("keeps dcr when back-stamping a DCR that uses the metadata URL", async () => { + const storage = makeStorage(); + vi.mocked(storage.getClientInformation).mockImplementation( + async (_url: string, preregistered?: boolean) => + preregistered ? undefined : { client_id: METADATA_URL }, + ); + vi.mocked(storage.getClientRegistrationKind).mockResolvedValue("dcr"); + // Even with an AS that does advertise CIMD. + seedDiscovery(storage, ISSUER, true); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: METADATA_URL, + }); + + await provider.saveClientInformation( + { client_id: METADATA_URL }, + { issuer: ISSUER }, + ); + + expect(storage.saveClientInformation).toHaveBeenCalledWith( + SERVER, + { client_id: METADATA_URL }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + }); + + // Nothing stored for this issuer, so the SDK is creating the + // registration. It reaches its URL-based-client-ID branch exactly when + // the AS advertises CIMD — read back from the discovery state it + // persisted moments earlier. + it("records cimd for a new registration when the AS advertises CIMD", async () => { + const storage = makeStorage(); + seedDiscovery(storage, ISSUER, true); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: METADATA_URL, + }); + + await provider.saveClientInformation( + { client_id: METADATA_URL }, + { issuer: ISSUER }, + ); + + expect(storage.saveClientInformation).toHaveBeenCalledWith( + SERVER, + { client_id: METADATA_URL }, + { registrationKind: "cimd", issuer: ISSUER }, + ); + }); + + it("records dcr for a new registration when the AS does not advertise CIMD", async () => { + const storage = makeStorage(); + seedDiscovery(storage, ISSUER, false); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: METADATA_URL, + }); + + await provider.saveClientInformation( + { client_id: METADATA_URL }, + { issuer: ISSUER }, + ); + + expect(storage.saveClientInformation).toHaveBeenCalledWith( + SERVER, + { client_id: METADATA_URL }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + }); + + it("records dcr when the discovery state describes a different issuer", async () => { + const storage = makeStorage(); + seedDiscovery(storage, "https://as-other.example.com", true); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: METADATA_URL, + }); + + await provider.saveClientInformation( + { client_id: METADATA_URL }, + { issuer: ISSUER }, + ); + + expect(storage.saveClientInformation).toHaveBeenCalledWith( + SERVER, + { client_id: METADATA_URL }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + }); + + it("records dcr for a server-minted client_id while CIMD is configured", async () => { + const storage = makeCimdStorage(); + seedDiscovery(storage, ISSUER, true); + const provider = makeProvider(storage, vi.fn(), { + clientMetadataUrl: METADATA_URL, + }); + + 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 }, + ); + + expect(storage.saveClientInformation).toHaveBeenCalledWith( + SERVER, + { client_id: METADATA_URL }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + }); + + // SEP-2352 keys registrations per authorization server, so a second AS + // behind one resource is a separate determination. Driven against a real + // `OAuthStorageBase` and the real `ensureCimdClientRegistration`, because + // 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"; + + function makeRealStorage(): OAuthStorage { + const backend: OAuthPersistBackend = { + read: async () => null, + write: async () => {}, + }; + return new OAuthStorageBase(new OAuthMemoryStore(), backend); + } + + /** + * 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): 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, + 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}`); + }; + } + + /** 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, + }); + 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 a second CIMD-supporting issuer takes over", async () => { + const storage = makeRealStorage(); + 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 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 }, + ); + + expect( + await storage.getClientRegistrationKind(SERVER, ISSUER_B), + ).toBe("cimd"); + expect( + 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); + + // Issuer B does *not* advertise CIMD, so nothing is recorded for B... + await ensureCimdClientRegistration({ + serverUrl: SERVER, + 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( + { client_id: METADATA_URL }, + { issuer: ISSUER_B }, + ); + + expect( + await storage.getClientRegistrationKind(SERVER, ISSUER_B), + ).toBe("dcr"); + // Issuer A's own provenance is untouched. + expect( + await storage.getClientRegistrationKind(SERVER, ISSUER), + ).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), + }); + // 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"); + }); + + // 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); + // 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 recorded kind are both gone... + expect( + await storage.getClientInformation(SERVER, false, ISSUER), + ).toBeUndefined(); + expect( + await storage.getClientRegistrationKind(SERVER, ISSUER), + ).toBeUndefined(); + + // ...so 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); + + 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(), { + 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..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 @@ -370,6 +370,112 @@ 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. + // + // ⚠️ 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, + registrationKind: "cimd", + }); }); it("should retry original request after OAuth completion with CIMD", async () => { diff --git a/core/auth/cimd.ts b/core/auth/cimd.ts index 31614d1b3..d17ce7202 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"; @@ -27,39 +26,69 @@ 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; + // 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; + } - 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; + } } - // 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, - ); + const issuer = metadata?.issuer; if (!metadata?.client_id_metadata_document_supported) return; - const clientInformation: OAuthClientInformation = { - client_id: clientMetadataUrl, - }; - await params.provider.saveClientInformation(clientInformation, { - registrationKind: "cimd", - }); + // ⚠️ 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 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) return; + + 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/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 { diff --git a/core/auth/providers.ts b/core/auth/providers.ts index 0447b7291..92512c0a2 100644 --- a/core/auth/providers.ts +++ b/core/auth/providers.ts @@ -293,16 +293,19 @@ 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 defaults registration kind to DCR; 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; 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 +316,87 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { ); } + /** + * Resolve the registration kind for a save that carries no explicit one — that + * 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). + * + * Two cases reach here, and they are told apart by whether a registration + * already exists for this issuer: + * + * - **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. + * + * 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). + * + * Consequences worth stating, since each was a defect on the way here: + * + * - 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, + issuer: string | undefined, + ): Promise { + const clientMetadataUrl = this.clientMetadataUrl?.trim(); + if ( + !clientMetadataUrl || + clientInformation.client_id !== clientMetadataUrl + ) { + return "dcr"; + } + + // 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, + ); + 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 { await this.storage.saveScope(this.serverUrl, scope); this.cachedScope = scope;