diff --git a/clients/web/src/test/core/auth/storage-browser.test.ts b/clients/web/src/test/core/auth/storage-browser.test.ts index 1d6a2869..8fb99cb1 100644 --- a/clients/web/src/test/core/auth/storage-browser.test.ts +++ b/clients/web/src/test/core/auth/storage-browser.test.ts @@ -533,6 +533,63 @@ describe("BrowserOAuthStorage", () => { }); }); + it("does not stamp a legacy fallback credential when the issuer slot lacks it", async () => { + // Legacy unkeyed client info survives, but a byIssuer slot exists for + // issuerA holding only tokens (no clientInformation). Reading client info + // for issuerA must fall back to the legacy value WITHOUT stamping it with + // issuerA — the legacy credential may have been minted by a different AS. + await storage.saveClientInformation( + testServerUrl, + { client_id: "legacy-client" }, + { registrationKind: "dcr" }, + ); + await storage.saveTokens( + testServerUrl, + { access_token: "tok-a", token_type: "Bearer" }, + { issuer: issuerA }, + ); + + const clientInfo = await storage.getClientInformation( + testServerUrl, + false, + issuerA, + ); + // No issuer stamp — so the SDK's discardIfIssuerMismatch stays engaged. + expect(clientInfo).toEqual({ client_id: "legacy-client" }); + + // Symmetric case: legacy tokens survive, slot has only clientInformation. + await storage.clear(testServerUrl); + await storage.saveTokens(testServerUrl, { + access_token: "legacy-tok", + token_type: "Bearer", + }); + await storage.saveClientInformation( + testServerUrl, + { client_id: "client-a" }, + { registrationKind: "dcr", issuer: issuerA }, + ); + const tokens = await storage.getTokens(testServerUrl, issuerA); + expect(tokens).toEqual({ + access_token: "legacy-tok", + token_type: "Bearer", + }); + }); + + it("does not promote a cleared issuer to the active (ctx-less) slot", async () => { + await storage.saveTokens( + testServerUrl, + { access_token: "tok-a", token_type: "Bearer" }, + { issuer: issuerA }, + ); + // Clearing a *different* issuer must not make it the active issuer. + await storage.clearTokens(testServerUrl, issuerB); + + // The ctx-less (per-request bearer) read still resolves issuerA's token. + expect(await storage.getTokens(testServerUrl)).toMatchObject({ + access_token: "tok-a", + }); + }); + it("clears every issuer's tokens/registration when no issuer is given", async () => { await storage.saveTokens( testServerUrl, diff --git a/clients/web/src/utils/oauthNetworkPhase.test.ts b/clients/web/src/utils/oauthNetworkPhase.test.ts index ba5abd57..fa50206c 100644 --- a/clients/web/src/utils/oauthNetworkPhase.test.ts +++ b/clients/web/src/utils/oauthNetworkPhase.test.ts @@ -45,6 +45,20 @@ describe("oauthNetworkPhase", () => { expect(oauthNetworkPhase("https://mcp.example.com/mcp")).toBeUndefined(); }); + it("matches the endpoint as the final path segment, not a nested one", () => { + // A nested segment must not classify as the OAuth endpoint... + expect( + oauthNetworkPhase("https://as.example.com/oauth/token/refresh"), + ).toBeUndefined(); + expect( + oauthNetworkPhase("https://gw.example.com/api/register/foo"), + ).toBeUndefined(); + // ...but a trailing slash on the real endpoint is tolerated. + expect(oauthNetworkPhase("https://as.example.com/oauth/token/")).toBe( + "token", + ); + }); + it("tolerates a non-absolute URL by stripping the query", () => { expect(oauthNetworkPhase("/register?foo=bar")).toBe("registration"); expect(oauthNetworkPhase("not a url")).toBeUndefined(); diff --git a/clients/web/src/utils/oauthNetworkPhase.ts b/clients/web/src/utils/oauthNetworkPhase.ts index 7d8772ca..5fd1a121 100644 --- a/clients/web/src/utils/oauthNetworkPhase.ts +++ b/clients/web/src/utils/oauthNetworkPhase.ts @@ -41,13 +41,16 @@ export function oauthNetworkPhase( ) { return "discovery"; } - if (path.endsWith("/register") || path.includes("/register/")) { + // Match the endpoint as the final path segment (trailing slash tolerated) so a + // nested path like `/token/refresh` or `/api/register/foo` is not misclassified. + const endpoint = path.replace(/\/+$/, ""); + if (endpoint.endsWith("/register")) { return "registration"; } - if (path.endsWith("/token") || path.includes("/token/")) { + if (endpoint.endsWith("/token")) { return "token"; } - if (path.endsWith("/authorize") || path.includes("/authorize/")) { + if (endpoint.endsWith("/authorize")) { return "authorize"; } return undefined; diff --git a/core/auth/oauth-storage.ts b/core/auth/oauth-storage.ts index d0225fd9..f68baa64 100644 --- a/core/auth/oauth-storage.ts +++ b/core/auth/oauth-storage.ts @@ -112,6 +112,10 @@ export class OAuthStorageBase implements OAuthStorage { issuer: string, updates: Partial, clearLegacy: Partial, + // Saves promote the issuer to `activeIssuer` (it answers ctx-less reads); + // clears must not — clearing one AS's credentials shouldn't make it the + // active one. + setActive = true, ): void { const state = this.memory.getState().getServerState(serverUrl); const byIssuer = { @@ -120,7 +124,7 @@ export class OAuthStorageBase implements OAuthStorage { }; this.memory.getState().setServerState(serverUrl, { byIssuer, - activeIssuer: issuer, + ...(setActive && { activeIssuer: issuer }), ...clearLegacy, }); } @@ -154,14 +158,19 @@ export class OAuthStorageBase implements OAuthStorage { // Per-issuer registration (SEP-2352), falling back to the legacy unkeyed slot. const slot = this.issuerSlot(state, issuer); - const clientInfo = slot?.clientInformation ?? state.clientInformation; + const fromSlot = slot?.clientInformation; + const clientInfo = fromSlot ?? state.clientInformation; if (!clientInfo) { return undefined; } const parsed = await OAuthClientInformationSchema.parseAsync(clientInfo); + // Stamp only when the value actually came from the issuer-keyed slot. Gating + // on `fromSlot` (not merely on the slot existing) keeps a legacy unkeyed + // credential — potentially minted by a *different* AS — from being stamped + // with this issuer, which would defeat the SDK's discardIfIssuerMismatch. return withIssuer( parsed, - slot ? this.resolveReadIssuer(state, issuer) : undefined, + fromSlot ? this.resolveReadIssuer(state, issuer) : undefined, ); } @@ -237,6 +246,7 @@ export class OAuthStorageBase implements OAuthStorage { issuer, { clientInformation: undefined, clientRegistrationKind: undefined }, {}, + false, ); } else { // Clear every issuer's registration plus the legacy unkeyed entry. @@ -261,14 +271,17 @@ export class OAuthStorageBase implements OAuthStorage { await this.ensureLoaded(); const state = this.memory.getState().getServerState(serverUrl); const slot = this.issuerSlot(state, issuer); - const tokens = slot?.tokens ?? state.tokens; + const fromSlot = slot?.tokens; + const tokens = fromSlot ?? state.tokens; if (!tokens) { return undefined; } const parsed = await OAuthTokensSchema.parseAsync(tokens); + // Stamp only when the value came from the issuer-keyed slot (see + // getClientInformation) — never stamp a legacy unkeyed token with this issuer. return withIssuer( parsed, - slot ? this.resolveReadIssuer(state, issuer) : undefined, + fromSlot ? this.resolveReadIssuer(state, issuer) : undefined, ); } @@ -302,7 +315,13 @@ export class OAuthStorageBase implements OAuthStorage { async clearTokens(serverUrl: string, issuer?: string): Promise { await this.ensureLoaded(); if (issuer !== undefined) { - this.updateIssuerSlot(serverUrl, issuer, { tokens: undefined }, {}); + this.updateIssuerSlot( + serverUrl, + issuer, + { tokens: undefined }, + {}, + false, + ); } else { const state = this.memory.getState().getServerState(serverUrl); const byIssuer = this.mapIssuerSlots(state, () => ({