Skip to content

Commit aca522d

Browse files
cliffhallclaude
andcommitted
fix: delegate issuer context and discovery state through the EMA wrapper
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA Signed-off-by: cliffhall <cliff@futurescale.com>
1 parent be08f6d commit aca522d

2 files changed

Lines changed: 75 additions & 6 deletions

File tree

clients/web/src/test/core/auth/ema/transportProvider.test.ts

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ interface FakeInner {
6565
clearCapturedAuthUrl: ReturnType<typeof vi.fn>;
6666
saveCodeVerifier: ReturnType<typeof vi.fn>;
6767
codeVerifier: ReturnType<typeof vi.fn>;
68+
saveDiscoveryState: ReturnType<typeof vi.fn>;
69+
discoveryState: ReturnType<typeof vi.fn>;
6870
}
6971

7072
function createInner(): FakeInner {
@@ -82,6 +84,8 @@ function createInner(): FakeInner {
8284
clearCapturedAuthUrl: vi.fn(),
8385
saveCodeVerifier: vi.fn(),
8486
codeVerifier: vi.fn(() => "verifier-xyz"),
87+
saveDiscoveryState: vi.fn(),
88+
discoveryState: vi.fn(),
8589
};
8690
}
8791

@@ -118,14 +122,54 @@ describe("EmaTransportOAuthProvider", () => {
118122
expect(await provider.codeVerifier()).toBe("verifier-xyz");
119123

120124
await provider.saveClientInformation({ client_id: "new" } as never);
121-
expect(inner.saveClientInformation).toHaveBeenCalledWith({
122-
client_id: "new",
123-
});
125+
expect(inner.saveClientInformation).toHaveBeenCalledWith(
126+
{ client_id: "new" },
127+
undefined,
128+
);
124129

125130
await provider.saveCodeVerifier("cv");
126131
expect(inner.saveCodeVerifier).toHaveBeenCalledWith("cv");
127132
});
128133

134+
// SEP-2352: the wrapper used to drop the SDK's `ctx`, so every EMA read and
135+
// write landed on the unkeyed slot — and, since #2242, the registration-kind
136+
// resolver had no issuer to check and recorded a CIMD registration made over
137+
// an EMA connection as DCR (Copilot).
138+
it("forwards the SDK issuer context on client-information reads and writes", async () => {
139+
const ctx = { issuer: "https://as.example.com" };
140+
141+
await provider.clientInformation(ctx);
142+
expect(inner.clientInformation).toHaveBeenCalledWith(ctx);
143+
144+
await provider.saveClientInformation({ client_id: "new" } as never, ctx);
145+
expect(inner.saveClientInformation).toHaveBeenCalledWith(
146+
{ client_id: "new" },
147+
ctx,
148+
);
149+
});
150+
151+
// Without these the SDK persists no discovery state for an EMA connection, so
152+
// it re-discovers every call, cannot run its callback-leg AS binding check,
153+
// and leaves the registration-kind resolver nothing to read back.
154+
it("delegates discovery state to the inner provider", async () => {
155+
const state = {
156+
authorizationServerUrl: "https://as.example.com",
157+
authorizationServerMetadata: {
158+
issuer: "https://as.example.com",
159+
authorization_endpoint: "https://as.example.com/authorize",
160+
token_endpoint: "https://as.example.com/token",
161+
response_types_supported: ["code"],
162+
},
163+
};
164+
165+
await provider.saveDiscoveryState(state);
166+
expect(inner.saveDiscoveryState).toHaveBeenCalledWith(state);
167+
168+
inner.discoveryState.mockReturnValue(state);
169+
expect(await provider.discoveryState()).toEqual(state);
170+
expect(inner.discoveryState).toHaveBeenCalled();
171+
});
172+
129173
it("tokens() returns stored tokens when the access token is still usable", async () => {
130174
const stored: OAuthTokens = {
131175
access_token: VALID_ACCESS_TOKEN,

core/auth/ema/transportProvider.ts

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import type { OAuthClientProvider } from "@modelcontextprotocol/client";
22
import type {
3+
OAuthClientInformationContext,
34
OAuthClientInformationMixed,
45
OAuthClientMetadata,
6+
OAuthDiscoveryState,
57
OAuthTokens,
68
} from "@modelcontextprotocol/client";
79
import type { BaseOAuthClientProvider } from "../providers.js";
@@ -50,17 +52,40 @@ export class EmaTransportOAuthProvider implements OAuthClientProvider {
5052
return this.inner.state();
5153
}
5254

53-
clientInformation():
55+
// SEP-2352: `ctx` carries the authorization-server `issuer` the SDK resolved,
56+
// and the inner provider keys registrations by it. Dropping it here made every
57+
// EMA read and write land on the unkeyed slot — and, since #2242, left
58+
// `resolveSdkRegistrationKind` with no issuer to check, so a CIMD registration
59+
// made over an EMA connection was recorded as DCR (Copilot).
60+
clientInformation(
61+
ctx?: OAuthClientInformationContext,
62+
):
5463
| OAuthClientInformationMixed
5564
| undefined
5665
| Promise<OAuthClientInformationMixed | undefined> {
57-
return this.inner.clientInformation();
66+
return this.inner.clientInformation(ctx);
5867
}
5968

6069
saveClientInformation(
6170
clientInformation: OAuthClientInformationMixed,
71+
ctx?: OAuthClientInformationContext,
6272
): void | Promise<void> {
63-
return this.inner.saveClientInformation(clientInformation);
73+
return this.inner.saveClientInformation(clientInformation, ctx);
74+
}
75+
76+
// Without these the SDK persists no discovery state for an EMA connection, so
77+
// it re-discovers on every call, cannot perform its SEP-2352 callback-leg
78+
// authorization-server binding check (it warns as much), and — since #2242 —
79+
// leaves the registration-kind resolver nothing to read back.
80+
saveDiscoveryState(state: OAuthDiscoveryState): void | Promise<void> {
81+
return this.inner.saveDiscoveryState(state);
82+
}
83+
84+
discoveryState():
85+
| OAuthDiscoveryState
86+
| undefined
87+
| Promise<OAuthDiscoveryState | undefined> {
88+
return this.inner.discoveryState();
6489
}
6590

6691
async tokens(): Promise<OAuthTokens | undefined> {

0 commit comments

Comments
 (0)