diff --git a/clients/web/src/lib/remoteOAuthStorage.ts b/clients/web/src/lib/remoteOAuthStorage.ts index f1066cf0..d0497c37 100644 --- a/clients/web/src/lib/remoteOAuthStorage.ts +++ b/clients/web/src/lib/remoteOAuthStorage.ts @@ -30,7 +30,18 @@ export function getWebOAuthBaseUrl(): string { export const webOAuthFetch: typeof fetch = (...args) => globalThis.fetch(...args); -/** Memoized `RemoteOAuthStorage` for the given backend + auth token. */ +/** + * Memoized `RemoteOAuthStorage` for the given backend + auth token. + * + * The cache key is intentionally `{baseUrl, authToken}` only — `fetchFn` is NOT + * part of it. Whichever call constructs an instance first wins, so a later + * caller's `fetchFn` is ignored. This is safe today because every call site + * either omits `fetchFn` (defaulting to {@link webOAuthFetch}) or passes an + * `environmentFactory` wrapper that is functionally identical (`globalThis.fetch`), + * so the shared-instance goal (one whole-blob writer per backend) matters more + * than which wrapper is used. A future caller needing a genuinely different + * transport must key on it (or bypass the cache). + */ export function getRemoteOAuthStorage( baseUrl: string, authToken: string | undefined, diff --git a/clients/web/src/test/core/auth/oauth-storage.test.ts b/clients/web/src/test/core/auth/oauth-storage.test.ts index d622c122..5caf2c90 100644 --- a/clients/web/src/test/core/auth/oauth-storage.test.ts +++ b/clients/web/src/test/core/auth/oauth-storage.test.ts @@ -141,6 +141,48 @@ describe("OAuthStorageBase — async hydration", () => { expect(await storage.getCodeVerifier(SERVER)).toBe("new"); }); + it("clear() before hydration is not resurrected by the late merge", async () => { + // A clear issued while hydration is still gated must survive the pending + // rehydrate() that would otherwise merge the persisted (un-cleared) blob + // back on top and silently restore the credential. + const { storage, release } = makeAsyncBackedStorage({ + [SERVER]: { tokens: { access_token: "t", token_type: "Bearer" } }, + }); + storage.clear(SERVER); + release(); + await storage.ready(); + // Give the deferred re-apply (scheduled off `hydrated`) a microtask to run. + await Promise.resolve(); + expect(await storage.getTokens(SERVER)).toBeUndefined(); + }); + + it("clearTokens before hydration clears only tokens and survives the merge", async () => { + const { storage, release } = makeAsyncBackedStorage({ + [SERVER]: { + tokens: { access_token: "t", token_type: "Bearer" }, + codeVerifier: "keep-me", + }, + }); + storage.clearTokens(SERVER); + release(); + await storage.ready(); + await Promise.resolve(); + // Tokens cleared, but the sibling field from the persisted blob remains. + expect(await storage.getTokens(SERVER)).toBeUndefined(); + expect(await storage.getCodeVerifier(SERVER)).toBe("keep-me"); + }); + + it("clear() after hydration does not schedule a deferred re-apply", async () => { + const { storage, release } = makeAsyncBackedStorage({ + [SERVER]: { tokens: { access_token: "t", token_type: "Bearer" } }, + }); + release(); + await storage.ready(); + // Hydration already settled → synchronous clear, immediately observable. + storage.clear(SERVER); + expect(await storage.getTokens(SERVER)).toBeUndefined(); + }); + it("normalizeServerUrl canonicalizes host case, bare-origin trailing slash, default port, and whitespace", () => { expect(normalizeServerUrl("https://Example.COM/mcp")).toBe( "https://example.com/mcp", @@ -175,6 +217,47 @@ describe("OAuthStorageBase — async hydration", () => { }); }); + it("a partial write migrates a pre-normalization raw-key blob onto the canonical key (no orphaned fields)", async () => { + // Persisted under the bare-origin raw key (no trailing slash), the form a + // pre-normalization writer used; canonical form is `https://api.partner.dev/`. + // The caller consistently addresses the endpoint with that same raw string + // (matching getServerState's raw-key fallback). + const RAW = "https://api.partner.dev"; + const { storage, release } = makeAsyncBackedStorage({ + [RAW]: { tokens: { access_token: "existing", token_type: "Bearer" } }, + }); + release(); + await storage.ready(); + // A partial write must MERGE onto the existing blob, not shadow it with a + // fresh canonical entry — the pre-existing token must remain reachable. + await storage.saveCodeVerifier(RAW, "v"); + expect(await storage.getCodeVerifier(RAW)).toBe("v"); + expect(await storage.getTokens(RAW)).toEqual({ + access_token: "existing", + token_type: "Bearer", + }); + // The blob now lives under the canonical key, so the canonical form finds it + // too and the raw-key orphan no longer shadows anything. + expect(await storage.getTokens("https://api.partner.dev/")).toEqual({ + access_token: "existing", + token_type: "Bearer", + }); + }); + + it("clear() removes a pre-normalization raw-key blob, not just the canonical key", async () => { + const RAW = "https://api.partner.dev"; + const { storage, release } = makeAsyncBackedStorage({ + [RAW]: { tokens: { access_token: "existing", token_type: "Bearer" } }, + }); + release(); + await storage.ready(); + storage.clear(RAW); + // Both the canonical and raw keys must be gone — no orphan resurfacing via + // getServerState's raw-key fallback. + expect(await storage.getTokens(RAW)).toBeUndefined(); + expect(await storage.getTokens("https://api.partner.dev/")).toBeUndefined(); + }); + it("getServerState falls back to a pre-normalization persisted key", async () => { // A blob persisted before normalization existed may carry a raw key that // the canonical form differs from (here: bare-origin without slash). diff --git a/core/auth/oauth-storage.ts b/core/auth/oauth-storage.ts index e5df6b65..29707b1c 100644 --- a/core/auth/oauth-storage.ts +++ b/core/auth/oauth-storage.ts @@ -96,6 +96,34 @@ export class OAuthStorageBase implements OAuthStorage { return this.hydrated; } + /** + * Run a `clear*` mutation against a hydrated store: synchronously if + * hydration has already landed, otherwise deferred until it does. + * + * `clear*` are declared synchronous in {@link OAuthStorage} (some feed sync + * SDK getters), so unlike the `save*` family they cannot `await this.hydrated` + * up front. But a clear applied to the still-empty pre-hydration store is + * doubly wrong: (a) the pending `rehydrate()` would merge the persisted + * (un-cleared) blob back over it, resurrecting the credential; and worse, + * (b) applying it triggers a persist write of the near-empty store, which for + * a whole-blob adapter (file / remote HTTP) can clobber the on-disk blob — + * every other server's tokens included — before hydration ever reads it. + * Deferring the mutation until after hydration avoids both: it merges onto + * the real persisted state. The common case (hydration already settled) stays + * synchronous and immediately observable. A failed hydration still resolves + * {@link hydrated} (see constructor), so a deferred clear runs harmlessly + * against the empty store. The effect of a pre-hydration clear is therefore + * asynchronous, which is safe: the only callers are fire-and-forget + * invalidations that never synchronously read the value straight back. + */ + private clearAfterHydration(mutate: () => void): void { + if (this.store.persist.hasHydrated()) { + mutate(); + } else { + void this.hydrated.then(mutate); + } + } + async getClientInformation( serverUrl: string, isPreregistered?: boolean, @@ -179,7 +207,9 @@ export class OAuthStorageBase implements OAuthStorage { } clearTokens(serverUrl: string): void { - this.store.getState().setServerState(serverUrl, { tokens: undefined }); + this.clearAfterHydration(() => + this.store.getState().setServerState(serverUrl, { tokens: undefined }), + ); } async getCodeVerifier(serverUrl: string): Promise { @@ -197,18 +227,28 @@ export class OAuthStorageBase implements OAuthStorage { } clearCodeVerifier(serverUrl: string): void { - this.store - .getState() - .setServerState(serverUrl, { codeVerifier: undefined }); + this.clearAfterHydration(() => + this.store + .getState() + .setServerState(serverUrl, { codeVerifier: undefined }), + ); } /** * Intentionally synchronous. The only caller is * {@link BaseOAuthClientProvider.scope}, a sync getter the SDK requires - * (it feeds the sync `clientMetadata` getter). Scope is always set via - * {@link saveScope} in the same session before it's read (during the - * pre-redirect half of the flow), so the in-memory store has the value - * regardless of async hydration. + * (it feeds the sync `clientMetadata` getter). It is safe without awaiting + * hydration because of an ordering invariant the callers uphold: some + * awaited read always precedes it, flushing hydration first. + * - Pre-redirect half of the flow: `saveScope` (which awaits hydration) + * writes the value in the same session, so the in-memory store has it. + * - Post-redirect callback path: the in-memory store is reset, so the + * value comes from hydration — but `buildOAuthConnectionState` + * (`connection-state.ts`) awaits `getClientInformation`/ + * `getServerMetadata` (both of which await hydration) before it reads + * `getScope`, so hydration has already landed by then. + * A future refactor that reads `getScope` without a preceding awaited + * storage read would break this invariant. */ getScope(serverUrl: string): string | undefined { const state = this.store.getState().getServerState(serverUrl); @@ -221,7 +261,9 @@ export class OAuthStorageBase implements OAuthStorage { } clearScope(serverUrl: string): void { - this.store.getState().setServerState(serverUrl, { scope: undefined }); + this.clearAfterHydration(() => + this.store.getState().setServerState(serverUrl, { scope: undefined }), + ); } async getServerMetadata(serverUrl: string): Promise { @@ -241,13 +283,17 @@ export class OAuthStorageBase implements OAuthStorage { } clearServerMetadata(serverUrl: string): void { - this.store - .getState() - .setServerState(serverUrl, { serverMetadata: undefined }); + this.clearAfterHydration(() => + this.store + .getState() + .setServerState(serverUrl, { serverMetadata: undefined }), + ); } clear(serverUrl: string): void { - this.store.getState().clearServerState(serverUrl); + this.clearAfterHydration(() => + this.store.getState().clearServerState(serverUrl), + ); } async getIdpSession(issuer: string): Promise { @@ -272,10 +318,14 @@ export class OAuthStorageBase implements OAuthStorage { } clearIdpSession(issuer: string): void { - this.store.getState().clearIdpSession(issuer); + this.clearAfterHydration(() => + this.store.getState().clearIdpSession(issuer), + ); } clearEnterpriseManagedResourceServers(): void { - this.store.getState().clearEnterpriseManagedResourceServers(); + this.clearAfterHydration(() => + this.store.getState().clearEnterpriseManagedResourceServers(), + ); } } diff --git a/core/auth/store.ts b/core/auth/store.ts index b32360ed..eed06ed4 100644 --- a/core/auth/store.ts +++ b/core/auth/store.ts @@ -10,7 +10,10 @@ import type { OAuthTokens, OAuthMetadata, } from "@modelcontextprotocol/sdk/shared/auth.js"; -import type { IdpSessionState, OAuthClientRegistrationKind } from "./storage.js"; +import type { + IdpSessionState, + OAuthClientRegistrationKind, +} from "./storage.js"; /** * OAuth state for a single server @@ -92,21 +95,36 @@ export function createOAuthStore( updates: Partial, ) => { const key = normalizeServerUrl(serverUrl); - set((state) => ({ - servers: { - ...state.servers, - [key]: { - ...state.servers[key], - ...updates, - }, - }, - })); + set((state) => { + const servers = { ...state.servers }; + // Mirror getServerState's canonical→raw fallback for writes: if no + // canonical entry exists yet but a pre-normalization blob lives + // under the raw key, migrate it onto the canonical key so this + // partial write MERGES onto the existing credential instead of + // shadowing it with a fresh canonical entry (which would orphan the + // raw-key blob's other fields — e.g. a token — behind the new + // canonical one that getServerState now prefers). + let base = servers[key]; + if (base === undefined && servers[serverUrl] !== undefined) { + // Reaching here implies `serverUrl !== key`: if they were equal, + // `servers[serverUrl]` would equal the `servers[key]` we just + // found undefined, so the guard above could not have passed. + base = servers[serverUrl]; + delete servers[serverUrl]; + } + servers[key] = { ...base, ...updates }; + return { servers }; + }); }, clearServerState: (serverUrl: string) => { const key = normalizeServerUrl(serverUrl); set((state) => { const rest = { ...state.servers }; delete rest[key]; + // Also drop a pre-normalization blob under the raw key so a clear + // fully removes the credential rather than leaving an orphan that + // getServerState's raw-key fallback would keep surfacing. + if (serverUrl !== key) delete rest[serverUrl]; return { servers: rest }; }); }, diff --git a/core/storage/adapters/remote-storage.ts b/core/storage/adapters/remote-storage.ts index 686506ab..c216c98a 100644 --- a/core/storage/adapters/remote-storage.ts +++ b/core/storage/adapters/remote-storage.ts @@ -72,6 +72,15 @@ export function createRemoteStorageAdapter( // full-page navigation (the OAuth authorize redirect): without it the // browser may abort the request mid-flight and the just-saved // codeVerifier/clientInformation never reaches disk. + // + // Caveat: the fetch spec caps the *combined* body size of all in-flight + // keepalive requests at 64 KB. `value` here is the whole store blob + // (every server's tokens + metadata), so a user with a very large + // number of authorized servers could in theory exceed that and have the + // browser reject this write. That ceiling is well above realistic + // inspector usage; if it ever bites, the fix is per-server stores rather + // than dropping `keepalive` (which would reintroduce the redirect-abort + // bug this guards against). The catch below surfaces such a rejection. const res = await fetchFn(url, { method: "POST", headers,