From aca522de6a166d5bf5788ea8245ee4bbf1d6554f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 02:38:13 -0400 Subject: [PATCH] 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 ff66653e..6079a36c 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 fc36ea10..07c5c0c7 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 {