diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index a08dedbc..c4d98292 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -83,7 +83,10 @@ import { useServers } from "@inspector/core/react/useServers.js"; import { useSettingsDraft } from "@inspector/core/react/useSettingsDraft.js"; import { useClientSettingsDraft } from "@inspector/core/react/useClientSettingsDraft.js"; import { useEmaIdpLoginState } from "@inspector/core/react/useEmaIdpLoginState.js"; -import { getBrowserOAuthStorage } from "@inspector/core/auth/browser/index.js"; +import { + getRemoteOAuthStorage, + getWebOAuthBaseUrl, +} from "./lib/remoteOAuthStorage"; import { useManagedTools } from "@inspector/core/react/useManagedTools.js"; import { useManagedPrompts } from "@inspector/core/react/useManagedPrompts.js"; import { useManagedResources } from "@inspector/core/react/useManagedResources.js"; @@ -1455,9 +1458,10 @@ function App() { // `onToggleConnection` unloaded the previous one), so all React state is // reset and we recover the initiating server from sessionStorage. We wait for // `servers` to hydrate before acting; the ref guard keeps the exchange to a - // single run. The persisted PKCE verifier + DCR client info live in - // `BrowserOAuthStorage` and survive the redirect, so `completeOAuthFlow` - // exchanges the code without needing the original in-memory state machine. + // single run. The persisted PKCE verifier + DCR client info live in the + // backend-backed `RemoteOAuthStorage` (`~/.mcp-inspector/storage/oauth.json`) + // and survive the redirect, so `completeOAuthFlow` exchanges the code without + // needing the original in-memory state machine. useEffect(() => { if (typeof window === "undefined") return; if (window.location.pathname !== OAUTH_CALLBACK_PATH) return; @@ -2384,7 +2388,10 @@ function App() { const clientSettingsModalValue = clientSettingsDraft ?? EMPTY_CLIENT_SETTINGS; - const emaOAuthStorage = useMemo(() => getBrowserOAuthStorage(), []); + const emaOAuthStorage = useMemo( + () => getRemoteOAuthStorage(getWebOAuthBaseUrl(), getAuthToken()), + [], + ); const { loginState: emaIdpLoginState, logout: logoutEmaIdp } = useEmaIdpLoginState( emaOAuthStorage, @@ -2415,6 +2422,10 @@ function App() { config: server.config, inspectorClient: isActive ? inspectorClient : null, isActiveConnection: isActive, + oauthStorage: getRemoteOAuthStorage( + getWebOAuthBaseUrl(), + getAuthToken(), + ), }); if (!cleared) return; diff --git a/clients/web/src/lib/environmentFactory.ts b/clients/web/src/lib/environmentFactory.ts index e64cadef..7c9497a0 100644 --- a/clients/web/src/lib/environmentFactory.ts +++ b/clients/web/src/lib/environmentFactory.ts @@ -4,11 +4,9 @@ import { createRemoteFetch, createRemoteLogger, } from "@inspector/core/mcp/remote/index.js"; -import { - getBrowserOAuthStorage, - BrowserNavigation, -} from "@inspector/core/auth/browser/index.js"; +import { BrowserNavigation } from "@inspector/core/auth/browser/index.js"; import type { RedirectUrlProvider } from "@inspector/core/auth/index.js"; +import { getRemoteOAuthStorage } from "./remoteOAuthStorage"; export interface WebEnvironmentResult { environment: InspectorClientEnvironment; @@ -20,8 +18,11 @@ export interface WebEnvironmentResult { * - transport / fetch / logger all routed through the in-process Hono * backend at `window.location.origin` (the `clients/web/server` * dev-backend wires this in `/api/*`). - * - OAuth storage + navigation use the `BrowserOAuthStorage` (sessionStorage) - * and `BrowserNavigation` (full-page redirect) adapters. + * - OAuth storage uses `RemoteOAuthStorage` (POSTs to the same backend's + * `/api/storage/oauth`, which writes `~/.mcp-inspector/storage/oauth.json` + * mode 0600), so a token obtained in this browser session is also + * available to the CLI/TUI on the same host. Navigation still uses + * `BrowserNavigation` (full-page redirect). * * Returns both the assembled environment and the logger so callers can share * the same pino instance for any direct logging they need to do, instead of @@ -67,7 +68,7 @@ export function createWebEnvironment( }), logger, oauth: { - storage: getBrowserOAuthStorage(), + storage: getRemoteOAuthStorage(baseUrl, authToken, fetchFn), navigation: new BrowserNavigation(undefined, onBeforeOAuthRedirect), redirectUrlProvider, }, diff --git a/clients/web/src/lib/remoteOAuthStorage.test.ts b/clients/web/src/lib/remoteOAuthStorage.test.ts new file mode 100644 index 00000000..c1d57dbe --- /dev/null +++ b/clients/web/src/lib/remoteOAuthStorage.test.ts @@ -0,0 +1,64 @@ +import { describe, it, expect, vi } from "vitest"; +import { RemoteOAuthStorage } from "@inspector/core/auth/remote/index.js"; +import { + getRemoteOAuthStorage, + getWebOAuthBaseUrl, + webOAuthFetch, +} from "./remoteOAuthStorage"; + +const NOOP_FETCH = vi.fn( + async () => + new Response(JSON.stringify({ state: { servers: {} }, version: 0 }), { + status: 200, + }), +) as unknown as typeof fetch; + +describe("remoteOAuthStorage", () => { + it("getWebOAuthBaseUrl derives the backend origin from window.location", () => { + expect(getWebOAuthBaseUrl()).toBe( + `${window.location.protocol}//${window.location.host}`, + ); + }); + + it("returns a RemoteOAuthStorage instance", () => { + const storage = getRemoteOAuthStorage( + "http://a.example", + undefined, + NOOP_FETCH, + ); + expect(storage).toBeInstanceOf(RemoteOAuthStorage); + }); + + it("memoizes one instance per {baseUrl, authToken}", () => { + const a1 = getRemoteOAuthStorage("http://memo.example", "tok", NOOP_FETCH); + const a2 = getRemoteOAuthStorage("http://memo.example", "tok", NOOP_FETCH); + expect(a1).toBe(a2); + + // A different auth token is a distinct key → distinct instance. + const b = getRemoteOAuthStorage("http://memo.example", "other", NOOP_FETCH); + expect(b).not.toBe(a1); + + // A different base URL is a distinct key → distinct instance. + const c = getRemoteOAuthStorage("http://memo2.example", "tok", NOOP_FETCH); + expect(c).not.toBe(a1); + }); + + it("treats an undefined authToken as its own stable key", () => { + const a = getRemoteOAuthStorage("http://undef.example", undefined); + const b = getRemoteOAuthStorage("http://undef.example", undefined); + expect(a).toBe(b); + }); + + it("webOAuthFetch delegates to globalThis.fetch preserving the receiver", async () => { + const spy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response("ok", { status: 200 })); + try { + const res = await webOAuthFetch("http://x.example"); + expect(spy).toHaveBeenCalledWith("http://x.example"); + expect(res.status).toBe(200); + } finally { + spy.mockRestore(); + } + }); +}); diff --git a/clients/web/src/lib/remoteOAuthStorage.ts b/clients/web/src/lib/remoteOAuthStorage.ts new file mode 100644 index 00000000..f1066cf0 --- /dev/null +++ b/clients/web/src/lib/remoteOAuthStorage.ts @@ -0,0 +1,45 @@ +import { RemoteOAuthStorage } from "@inspector/core/auth/remote/index.js"; + +/** + * Shared `RemoteOAuthStorage` accessor for the web client. + * + * The browser's OAuth state is persisted through the in-process Hono backend + * (`POST /api/storage/oauth` → `~/.mcp-inspector/storage/oauth.json`, mode + * 0600) rather than `sessionStorage`, so a credential obtained in the browser + * is the same blob the TUI/CLI read on the same host (web ⇄ TUI ⇄ CLI parity). + * + * One instance per `{baseUrl, authToken}`. The environment factory is called + * per-connect (a fresh `InspectorClient` is built for each server switch), but + * the OAuth blob on disk is a single shared resource: a stale instance's + * whole-blob POST could clobber a write made by a newer one. Memoizing also + * avoids re-hydrating the same file on every connect, and lets the connection + * path, the EMA IdP hook, and the per-server "clear OAuth" action all share + * one in-memory view of the store. + */ +const oauthStorageCache = new Map(); + +/** Backend origin the web client talks to (same origin that serves the SPA). */ +export function getWebOAuthBaseUrl(): string { + return `${window.location.protocol}//${window.location.host}`; +} + +/** + * `window.fetch` loses its `this` binding when extracted, raising "Illegal + * invocation"; wrap so the call site preserves the global receiver. + */ +export const webOAuthFetch: typeof fetch = (...args) => + globalThis.fetch(...args); + +/** Memoized `RemoteOAuthStorage` for the given backend + auth token. */ +export function getRemoteOAuthStorage( + baseUrl: string, + authToken: string | undefined, + fetchFn: typeof fetch = webOAuthFetch, +): RemoteOAuthStorage { + const key = `${baseUrl} ${authToken ?? ""}`; + const cached = oauthStorageCache.get(key); + if (cached) return cached; + const storage = new RemoteOAuthStorage({ baseUrl, authToken, fetchFn }); + oauthStorageCache.set(key, storage); + return storage; +} diff --git a/clients/web/src/test/core/auth/connection-state.test.ts b/clients/web/src/test/core/auth/connection-state.test.ts index 62716e47..a65c8383 100644 --- a/clients/web/src/test/core/auth/connection-state.test.ts +++ b/clients/web/src/test/core/auth/connection-state.test.ts @@ -16,18 +16,19 @@ function createStorage( dynamic: Awaited>; registrationKind: ReturnType; scope: string | undefined; - serverMetadata: ReturnType; + serverMetadata: Awaited>; idpSession: Awaited>; - idpMetadata: ReturnType; + idpMetadata: Awaited>; }> = {}, ): OAuthStorage { return { + ready: vi.fn().mockResolvedValue(undefined), getTokens: vi.fn().mockResolvedValue(overrides.tokens), getClientInformation: vi.fn(async (_url, isPreregistered) => isPreregistered ? overrides.preregistered : overrides.dynamic, ), getScope: vi.fn().mockReturnValue(overrides.scope), - getServerMetadata: vi.fn((url: string) => { + getServerMetadata: vi.fn(async (url: string) => { if (url.startsWith("ema-idp:")) { return overrides.idpMetadata ?? null; } diff --git a/clients/web/src/test/core/auth/ema/idpOidc.test.ts b/clients/web/src/test/core/auth/ema/idpOidc.test.ts index 0831b4db..9966b88b 100644 --- a/clients/web/src/test/core/auth/ema/idpOidc.test.ts +++ b/clients/web/src/test/core/auth/ema/idpOidc.test.ts @@ -470,7 +470,7 @@ describe("completeIdpOidcAuthorization", () => { }); it("throws when the PKCE verifier is missing", async () => { - storage = buildStorage({ getCodeVerifier: vi.fn(() => undefined) }); + storage = buildStorage({ getCodeVerifier: vi.fn(async () => undefined) }); await expect( completeIdpOidcAuthorization({ idp, diff --git a/clients/web/src/test/core/auth/oauth-storage.test.ts b/clients/web/src/test/core/auth/oauth-storage.test.ts new file mode 100644 index 00000000..d622c122 --- /dev/null +++ b/clients/web/src/test/core/auth/oauth-storage.test.ts @@ -0,0 +1,224 @@ +import { describe, it, expect, vi } from "vitest"; +import { createJSONStorage } from "zustand/middleware"; +import { OAuthStorageBase } from "@inspector/core/auth/oauth-storage.js"; +import { + createOAuthStore, + normalizeServerUrl, +} from "@inspector/core/auth/store.js"; + +/** + * Builds an OAuthStorageBase backed by an *async* in-memory storage adapter + * pre-seeded with the given persisted blob. Hydration of the store completes + * only after the returned `release()` is called, so tests can assert that + * getters wait for it. + */ +function makeAsyncBackedStorage( + persistedServers: Record = {}, +) { + let mem: string | null = JSON.stringify({ + state: { servers: persistedServers }, + version: 0, + }); + let releaseGetItem!: () => void; + const gate = new Promise((resolve) => { + releaseGetItem = resolve; + }); + const adapter = createJSONStorage(() => ({ + getItem: async () => { + await gate; + return mem; + }, + setItem: async (name: string, value: string) => { + void name; + mem = value; + }, + removeItem: async () => { + mem = null; + }, + })); + const store = createOAuthStore(adapter); + return { storage: new OAuthStorageBase(store), release: releaseGetItem }; +} + +const SERVER = "https://example.com/mcp"; +const METADATA = { + issuer: SERVER, + authorization_endpoint: `${SERVER}/authorize`, + token_endpoint: `${SERVER}/token`, + response_types_supported: ["code"], +}; + +describe("OAuthStorageBase — async hydration", () => { + it("getCodeVerifier awaits hydration before reading state", async () => { + const { storage, release } = makeAsyncBackedStorage({ + [SERVER]: { codeVerifier: "persisted-verifier" }, + }); + // The store is empty until release() lets the async getItem resolve. + let resolvedWith: string | undefined | "pending" = "pending"; + const p = storage.getCodeVerifier(SERVER).then((v) => { + resolvedWith = v; + }); + // Give the microtask queue a chance — the getter should NOT have resolved + // because hydration is still gated. + await Promise.resolve(); + expect(resolvedWith).toBe("pending"); + release(); + await p; + expect(resolvedWith).toBe("persisted-verifier"); + }); + + it("getServerMetadata awaits hydration before reading state", async () => { + const { storage, release } = makeAsyncBackedStorage({ + [SERVER]: { serverMetadata: METADATA }, + }); + const p = storage.getServerMetadata(SERVER); + release(); + expect(await p).toEqual(METADATA); + }); + + it("getTokens and getClientInformation await hydration", async () => { + const { storage, release } = makeAsyncBackedStorage({ + [SERVER]: { + tokens: { access_token: "t", token_type: "Bearer" }, + clientInformation: { client_id: "abc" }, + }, + }); + const tokens = storage.getTokens(SERVER); + const client = storage.getClientInformation(SERVER); + release(); + expect(await tokens).toEqual({ access_token: "t", token_type: "Bearer" }); + expect(await client).toEqual({ client_id: "abc" }); + }); + + it("ready() resolves once hydration completes", async () => { + const { storage, release } = makeAsyncBackedStorage(); + let ready = false; + void storage.ready().then(() => { + ready = true; + }); + await Promise.resolve(); + expect(ready).toBe(false); + release(); + await storage.ready(); + expect(ready).toBe(true); + }); + + it("ready() resolves promptly when the adapter is synchronous", async () => { + let mem: string | null = null; + const syncAdapter = createJSONStorage(() => ({ + getItem: () => mem, + setItem: (name: string, value: string) => { + void name; + mem = value; + }, + removeItem: () => { + mem = null; + }, + })); + const storage = new OAuthStorageBase(createOAuthStore(syncAdapter)); + // With a sync adapter the constructor's rehydrate() resolves on the next + // microtask, so ready() and the getters settle without any release step. + await storage.ready(); + expect(await storage.getCodeVerifier(SERVER)).toBeUndefined(); + }); + + it("save* awaits hydration so a late merge cannot clobber the write", async () => { + const { storage, release } = makeAsyncBackedStorage({ + [SERVER]: { codeVerifier: "persisted-old" }, + }); + // Kick off a save while hydration is still gated. It must not write until + // hydration completes, otherwise the persist merge would overwrite "new" + // with "persisted-old". + const save = storage.saveCodeVerifier(SERVER, "new"); + let saved = false; + void save.then(() => { + saved = true; + }); + await Promise.resolve(); + expect(saved).toBe(false); + release(); + await save; + expect(await storage.getCodeVerifier(SERVER)).toBe("new"); + }); + + it("normalizeServerUrl canonicalizes host case, bare-origin trailing slash, default port, and whitespace", () => { + expect(normalizeServerUrl("https://Example.COM/mcp")).toBe( + "https://example.com/mcp", + ); + expect(normalizeServerUrl("https://example.com")).toBe( + "https://example.com/", + ); + expect(normalizeServerUrl("https://example.com:443/mcp")).toBe( + "https://example.com/mcp", + ); + expect(normalizeServerUrl(" https://example.com/mcp ")).toBe( + "https://example.com/mcp", + ); + // Non-URL strings (e.g. a stdio server name) are passed through trimmed. + expect(normalizeServerUrl(" my-stdio-server ")).toBe("my-stdio-server"); + }); + + it("store keys are normalized so save and get tolerate cosmetic URL differences", async () => { + const { storage, release } = makeAsyncBackedStorage(); + release(); + await storage.saveCodeVerifier("https://Example.COM/mcp", "v"); + expect(await storage.getCodeVerifier("https://example.com/mcp")).toBe("v"); + // Bare-origin: the web inspector stores `new URL().href` (trailing slash); + // a CLI caller passing the slash-less form must still hit it. + await storage.saveTokens("https://api.partner.dev/", { + access_token: "t", + token_type: "Bearer", + }); + expect(await storage.getTokens("https://api.partner.dev")).toEqual({ + access_token: "t", + token_type: "Bearer", + }); + }); + + 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). + const { storage, release } = makeAsyncBackedStorage({ + "https://legacy.example": { codeVerifier: "legacy" }, + }); + release(); + expect(await storage.getCodeVerifier("https://legacy.example")).toBe( + "legacy", + ); + }); + + it("ready()/getters resolve (empty) when the adapter throws instead of hanging", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + const throwingAdapter = createJSONStorage(() => ({ + getItem: async () => { + throw new Error("Failed to read store: 500"); + }, + setItem: async () => {}, + removeItem: async () => {}, + })); + const storage = new OAuthStorageBase(createOAuthStore(throwingAdapter)); + // Race ready() against a short timeout — if the catch path doesn't + // resolve `hydrated`, this would hit "TIMEOUT" instead of "ok". + const result = await Promise.race([ + storage.ready().then(() => "ok"), + new Promise((r) => setTimeout(() => r("TIMEOUT"), 200)), + ]); + expect(result).toBe("ok"); + expect(await storage.getTokens(SERVER)).toBeUndefined(); + expect(warn).toHaveBeenCalled(); + // The failure is recorded so callers can distinguish "no token" from + // "store unreadable". + expect(storage.getHydrationError()).toBeInstanceOf(Error); + } finally { + warn.mockRestore(); + } + }); + + it("getHydrationError() is undefined after a successful hydration", async () => { + const { storage, release } = makeAsyncBackedStorage(); + release(); + await storage.ready(); + expect(storage.getHydrationError()).toBeUndefined(); + }); +}); diff --git a/clients/web/src/test/core/auth/providers.test.ts b/clients/web/src/test/core/auth/providers.test.ts index f8bcea48..f533f7fe 100644 --- a/clients/web/src/test/core/auth/providers.test.ts +++ b/clients/web/src/test/core/auth/providers.test.ts @@ -221,9 +221,9 @@ describe("OAuthNavigation", () => { getTokens: vi.fn(async () => undefined), saveTokens: vi.fn(async () => undefined), saveCodeVerifier: vi.fn(async () => undefined), - getCodeVerifier: vi.fn(() => undefined), + getCodeVerifier: vi.fn(async () => undefined), clear: vi.fn(), - getServerMetadata: vi.fn(() => null), + getServerMetadata: vi.fn(async () => null), saveServerMetadata: vi.fn(async () => undefined), } as unknown as OAuthStorage; } @@ -271,21 +271,21 @@ describe("OAuthNavigation", () => { expect(await provider.clientInformation()).toEqual({ client_id: "dyn" }); }); - it("codeVerifier() throws when none is saved", () => { + it("codeVerifier() rejects when none is saved", async () => { const storage = makeStorage(); const provider = makeProvider(storage); - expect(() => provider.codeVerifier()).toThrow( + await expect(provider.codeVerifier()).rejects.toThrow( /No code verifier saved for session/, ); }); - it("codeVerifier() returns the saved verifier", () => { + it("codeVerifier() returns the saved verifier", async () => { const storage = makeStorage(); - vi.mocked(storage.getCodeVerifier).mockReturnValue("cv-1"); + vi.mocked(storage.getCodeVerifier).mockResolvedValue("cv-1"); const provider = makeProvider(storage); - expect(provider.codeVerifier()).toBe("cv-1"); + expect(await provider.codeVerifier()).toBe("cv-1"); }); it("clientMetadata reflects the stored scope when present", () => { @@ -362,7 +362,7 @@ describe("OAuthNavigation", () => { expect(storage.saveCodeVerifier).toHaveBeenCalledWith(SERVER, "cv"); expect(storage.saveServerMetadata).toHaveBeenCalled(); expect(await provider.tokens()).toBeUndefined(); - expect(provider.getServerMetadata()).toBeNull(); + expect(await provider.getServerMetadata()).toBeNull(); const state = await provider.state(); expect(typeof state).toBe("string"); expect(state.length).toBeGreaterThan(0); 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 f009b4bd..cb97b460 100644 --- a/clients/web/src/test/core/auth/storage-browser.test.ts +++ b/clients/web/src/test/core/auth/storage-browser.test.ts @@ -1,5 +1,8 @@ import { describe, it, expect, beforeEach, afterEach } from "vitest"; -import { BrowserOAuthStorage } from "@inspector/core/auth/browser/storage.js"; +import { + BrowserOAuthStorage, + getBrowserOAuthStorage, +} from "@inspector/core/auth/browser/storage.js"; import type { OAuthClientInformation, OAuthTokens, @@ -41,6 +44,16 @@ const mockSessionStorage = new MockSessionStorage(); (global as typeof globalThis & { sessionStorage?: Storage }).sessionStorage = mockSessionStorage; +describe("getBrowserOAuthStorage", () => { + it("returns a memoized singleton across calls", () => { + const first = getBrowserOAuthStorage(); + const second = getBrowserOAuthStorage(); + expect(first).toBeInstanceOf(BrowserOAuthStorage); + // Second call hits the cached-instance branch rather than constructing anew. + expect(second).toBe(first); + }); +}); + describe("BrowserOAuthStorage", () => { let storage: BrowserOAuthStorage; const testServerUrl = "http://localhost:3000"; @@ -226,8 +239,10 @@ describe("BrowserOAuthStorage", () => { it("should return stored scope", async () => { const scope = "read write"; - storage.saveScope(testServerUrl, scope); - const result = await storage.getScope(testServerUrl); + // getScope is intentionally synchronous; await the save (which waits for + // hydration) so the value has landed before the sync read. + await storage.saveScope(testServerUrl, scope); + const result = storage.getScope(testServerUrl); expect(result).toBe(scope); }); @@ -237,8 +252,8 @@ describe("BrowserOAuthStorage", () => { it("should save scope", async () => { const scope = "read write"; - storage.saveScope(testServerUrl, scope); - const result = await storage.getScope(testServerUrl); + await storage.saveScope(testServerUrl, scope); + const result = storage.getScope(testServerUrl); expect(result).toBe(scope); }); @@ -322,13 +337,13 @@ describe("BrowserOAuthStorage", () => { it("clearCodeVerifier removes only the PKCE verifier", async () => { storage.saveCodeVerifier(testServerUrl, "verifier"); - expect(storage.getCodeVerifier(testServerUrl)).toBe("verifier"); + expect(await storage.getCodeVerifier(testServerUrl)).toBe("verifier"); storage.clearCodeVerifier(testServerUrl); - expect(storage.getCodeVerifier(testServerUrl)).toBeUndefined(); + expect(await storage.getCodeVerifier(testServerUrl)).toBeUndefined(); }); it("clearScope removes only the scope", async () => { - storage.saveScope(testServerUrl, "read"); + await storage.saveScope(testServerUrl, "read"); expect(storage.getScope(testServerUrl)).toBe("read"); storage.clearScope(testServerUrl); expect(storage.getScope(testServerUrl)).toBeUndefined(); @@ -342,9 +357,9 @@ describe("BrowserOAuthStorage", () => { response_types_supported: ["code"], }; storage.saveServerMetadata(testServerUrl, metadata); - expect(storage.getServerMetadata(testServerUrl)).toEqual(metadata); + expect(await storage.getServerMetadata(testServerUrl)).toEqual(metadata); storage.clearServerMetadata(testServerUrl); - expect(storage.getServerMetadata(testServerUrl)).toBeNull(); + expect(await storage.getServerMetadata(testServerUrl)).toBeNull(); }); }); @@ -358,10 +373,10 @@ describe("BrowserOAuthStorage", () => { token_type: "Bearer", }; - storage.saveClientInformation(testServerUrl, clientInfo, { + await storage.saveClientInformation(testServerUrl, clientInfo, { registrationKind: "dcr", }); - storage.saveTokens(testServerUrl, tokens); + await storage.saveTokens(testServerUrl, tokens); storage.clear(testServerUrl); @@ -375,10 +390,10 @@ describe("BrowserOAuthStorage", () => { client_id: "test-client-id", }; - storage.saveClientInformation(testServerUrl, clientInfo, { + await storage.saveClientInformation(testServerUrl, clientInfo, { registrationKind: "dcr", }); - storage.saveClientInformation(otherServerUrl, clientInfo, { + await storage.saveClientInformation(otherServerUrl, clientInfo, { registrationKind: "dcr", }); diff --git a/clients/web/src/test/core/auth/storage-remote.test.ts b/clients/web/src/test/core/auth/storage-remote.test.ts index c20617ef..8cd787d4 100644 --- a/clients/web/src/test/core/auth/storage-remote.test.ts +++ b/clients/web/src/test/core/auth/storage-remote.test.ts @@ -72,9 +72,9 @@ describe("RemoteOAuthStorage (unit, mocked fetch)", () => { it("codeVerifier round-trip and clearCodeVerifier", async () => { await storage.saveCodeVerifier(serverUrl, "verifier"); - expect(storage.getCodeVerifier(serverUrl)).toBe("verifier"); + expect(await storage.getCodeVerifier(serverUrl)).toBe("verifier"); storage.clearCodeVerifier(serverUrl); - expect(storage.getCodeVerifier(serverUrl)).toBeUndefined(); + expect(await storage.getCodeVerifier(serverUrl)).toBeUndefined(); }); it("scope round-trip and clearScope", async () => { @@ -92,9 +92,9 @@ describe("RemoteOAuthStorage (unit, mocked fetch)", () => { response_types_supported: ["code"], }; await storage.saveServerMetadata(serverUrl, md); - expect(storage.getServerMetadata(serverUrl)).toEqual(md); + expect(await storage.getServerMetadata(serverUrl)).toEqual(md); storage.clearServerMetadata(serverUrl); - expect(storage.getServerMetadata(serverUrl)).toBeNull(); + expect(await storage.getServerMetadata(serverUrl)).toBeNull(); }); it("clear() wipes all state for a server", async () => { @@ -120,4 +120,65 @@ describe("RemoteOAuthStorage (unit, mocked fetch)", () => { // No public accessor; constructing without throwing covers the default-branch. expect(s).toBeInstanceOf(RemoteOAuthStorage); }); + + it("getCodeVerifier waits for the async hydration GET before reading", async () => { + // Unlike the other tests in this file (which write-then-read in the same + // session), this asserts that a value PERSISTED on the backend before the + // store was constructed is still returned — i.e. the getter awaits the + // remote storage adapter's hydration GET. + let releaseGet!: () => void; + const getGate = new Promise((resolve) => { + releaseGet = resolve; + }); + const fetchFn = vi.fn( + async (url: RequestInfo | URL, init?: RequestInit) => { + if ((init?.method ?? "GET") === "GET") { + await getGate; + return new Response( + JSON.stringify({ + state: { + servers: { [serverUrl]: { codeVerifier: "from-disk" } }, + }, + version: 0, + }), + { status: 200 }, + ); + } + void url; + return new Response("{}", { status: 200 }); + }, + ); + const s = new RemoteOAuthStorage({ + baseUrl: "http://remote.example", + storeId: "hydration-test", + fetchFn: fetchFn as unknown as typeof fetch, + }); + const p = s.getCodeVerifier(serverUrl); + releaseGet(); + expect(await p).toBe("from-disk"); + }); + + it("persist POST is sent with keepalive so it survives an immediate redirect", async () => { + const fetchFn = vi.fn( + async (url: RequestInfo | URL, init?: RequestInit) => { + void url; + void init; + return new Response("{}", { status: 200 }); + }, + ); + const s = new RemoteOAuthStorage({ + baseUrl: "http://remote.example", + storeId: "keepalive-test", + fetchFn: fetchFn as unknown as typeof fetch, + }); + await s.ready(); + await s.saveCodeVerifier(serverUrl, "v"); + // Zustand persist fires setItem on the next microtask after set(); flush + // a couple of ticks so the POST has been issued. + await Promise.resolve(); + await Promise.resolve(); + const post = fetchFn.mock.calls.find((c) => c[1]?.method === "POST"); + expect(post).toBeDefined(); + expect(post?.[1]?.keepalive).toBe(true); + }); }); diff --git a/clients/web/src/test/core/auth/utils.test.ts b/clients/web/src/test/core/auth/utils.test.ts index 712b95ee..8d857e26 100644 --- a/clients/web/src/test/core/auth/utils.test.ts +++ b/clients/web/src/test/core/auth/utils.test.ts @@ -61,6 +61,26 @@ describe("generateOAuthState", () => { const s2 = generateOAuthState(); expect(s1).not.toBe(s2); }); + + it("throws instead of silently degrading when crypto.getRandomValues is unavailable", () => { + const original = globalThis.crypto; + // Simulate a runtime without Web Crypto — the CSRF token must not fall back + // to a predictable Math.random source. + Object.defineProperty(globalThis, "crypto", { + value: undefined, + configurable: true, + }); + try { + expect(() => generateOAuthState()).toThrow( + /crypto\.getRandomValues is not available/, + ); + } finally { + Object.defineProperty(globalThis, "crypto", { + value: original, + configurable: true, + }); + } + }); }); describe("parseOAuthState", () => { diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index 23eeb92d..be436ac7 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -38,6 +38,7 @@ function createMockParams( const dispatchOAuthError = vi.fn(); const storage = { + ready: vi.fn().mockResolvedValue(undefined), getScope: vi.fn().mockReturnValue(undefined), getClientInformation: vi.fn().mockResolvedValue(undefined), getClientRegistrationKind: vi.fn().mockReturnValue(undefined), diff --git a/clients/web/src/test/integration/auth/node/storage.test.ts b/clients/web/src/test/integration/auth/node/storage.test.ts index 2c72f8aa..3956a53a 100644 --- a/clients/web/src/test/integration/auth/node/storage.test.ts +++ b/clients/web/src/test/integration/auth/node/storage.test.ts @@ -4,6 +4,7 @@ import { getOAuthStore, clearAllOAuthClientState, } from "@inspector/core/auth/node/storage-node.js"; +import { normalizeServerUrl } from "@inspector/core/auth/store.js"; import type { OAuthClientInformation, OAuthTokens, @@ -341,9 +342,9 @@ describe("NodeOAuthStorage", () => { it("clearCodeVerifier removes only the PKCE verifier", async () => { await storage.saveCodeVerifier(testServerUrl, "verifier"); - expect(storage.getCodeVerifier(testServerUrl)).toBe("verifier"); + expect(await storage.getCodeVerifier(testServerUrl)).toBe("verifier"); storage.clearCodeVerifier(testServerUrl); - expect(storage.getCodeVerifier(testServerUrl)).toBeUndefined(); + expect(await storage.getCodeVerifier(testServerUrl)).toBeUndefined(); }); it("clearScope removes only the scope", async () => { @@ -361,9 +362,9 @@ describe("NodeOAuthStorage", () => { response_types_supported: ["code"], }; await storage.saveServerMetadata(testServerUrl, metadata); - expect(storage.getServerMetadata(testServerUrl)).toEqual(metadata); + expect(await storage.getServerMetadata(testServerUrl)).toEqual(metadata); storage.clearServerMetadata(testServerUrl); - expect(storage.getServerMetadata(testServerUrl)).toBeNull(); + expect(await storage.getServerMetadata(testServerUrl)).toBeNull(); }); }); @@ -505,9 +506,9 @@ describe("OAuth Store (Zustand)", () => { const parsed = JSON.parse( await fs.readFile(persistTestPath, "utf-8"), ) as StateShape; - expect(parsed.state.servers[serverUrl]?.clientInformation).toEqual( - clientInfo, - ); + expect( + parsed.state.servers[normalizeServerUrl(serverUrl)]?.clientInformation, + ).toEqual(clientInfo); } finally { try { await fs.unlink(persistTestPath); @@ -582,9 +583,10 @@ describe("NodeOAuthStorage with custom storagePath", () => { await fs.readFile(customPath, "utf-8"), ) as StateShape; - expect(parsed.state.servers[testServerUrl]?.tokens?.access_token).toBe( - tokens.access_token, - ); + expect( + parsed.state.servers[normalizeServerUrl(testServerUrl)]?.tokens + ?.access_token, + ).toBe(tokens.access_token); const stored = await storage.getTokens(testServerUrl); expect(stored?.access_token).toBe(tokens.access_token); @@ -635,7 +637,13 @@ describe("NodeOAuthStorage with custom storagePath", () => { ); try { + // The store is created with skipHydration: true; OAuthStorageBase + // normally drives rehydrate(), but this test writes via the raw store + // so hydrate it explicitly first (otherwise the later `new + // NodeOAuthStorage()` wrapper would drive the first rehydrate and clobber + // these unpersisted in-memory writes). const defaultStore = getOAuthStore(); + await defaultStore.persist.rehydrate(); defaultStore.getState().setServerState(testServerUrl, { tokens: { access_token: "default-token", diff --git a/clients/web/src/test/integration/storage/adapters.test.ts b/clients/web/src/test/integration/storage/adapters.test.ts index 8c248a3f..2c63047d 100644 --- a/clients/web/src/test/integration/storage/adapters.test.ts +++ b/clients/web/src/test/integration/storage/adapters.test.ts @@ -11,7 +11,10 @@ import { serve } from "@hono/node-server"; import type { ServerType } from "@hono/node-server"; import { createFileStorageAdapter } from "@inspector/core/storage/adapters/file-storage.js"; import { createRemoteStorageAdapter } from "@inspector/core/storage/adapters/remote-storage.js"; -import { createOAuthStore } from "@inspector/core/auth/store.js"; +import { + createOAuthStore, + normalizeServerUrl, +} from "@inspector/core/auth/store.js"; import { createRemoteApp } from "@inspector/core/mcp/remote/node/server.js"; import { writeStoreFile, @@ -90,13 +93,46 @@ describe("Storage adapters", () => { }); it("setItem throws when the POST fails", async () => { + // The adapter logs the swallowed-persist failure via console.error before + // rethrowing; suppress the expected noise. + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + const fetchFn = vi + .fn() + .mockResolvedValueOnce(new Response("nope", { status: 500 })); + const adapter = makeAdapter(fetchFn as typeof fetch); + await expect( + adapter.setItem("name", { state: { a: 1 }, version: 0 }), + ).rejects.toThrow(/500/); + expect(errorSpy).toHaveBeenCalled(); + } finally { + errorSpy.mockRestore(); + } + }); + + it("setItem sends keepalive so the POST survives an immediate redirect", async () => { const fetchFn = vi .fn() - .mockResolvedValueOnce(new Response("nope", { status: 500 })); + .mockResolvedValueOnce(new Response("{}")); const adapter = makeAdapter(fetchFn as typeof fetch); - await expect( - adapter.setItem("name", { state: { a: 1 }, version: 0 }), - ).rejects.toThrow(/500/); + await adapter.setItem("name", { state: { a: 1 }, version: 0 }); + expect(fetchFn.mock.calls[0]?.[1]?.keepalive).toBe(true); + }); + + it("setItem rethrows a network-level fetch rejection after logging it", async () => { + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + const fetchFn = vi + .fn() + .mockRejectedValueOnce(new Error("network down")); + const adapter = makeAdapter(fetchFn as typeof fetch); + await expect( + adapter.setItem("name", { state: { a: 1 }, version: 0 }), + ).rejects.toThrow(/network down/); + expect(errorSpy).toHaveBeenCalled(); + } finally { + errorSpy.mockRestore(); + } }); it("removeItem tolerates 404 but rethrows on other failures", async () => { @@ -155,7 +191,9 @@ describe("Storage adapters", () => { await flushStoreFileWrites(filePath); const fileContent = readFileSync(filePath, "utf-8"); const parsed = JSON.parse(fileContent); - expect(parsed.state.servers["https://example.com"].tokens).toEqual({ + expect( + parsed.state.servers[normalizeServerUrl("https://example.com")].tokens, + ).toEqual({ access_token: "test-token", token_type: "Bearer", }); @@ -173,10 +211,12 @@ describe("Storage adapters", () => { }); await flushStoreFileWrites(filePath); - // Create new store instance (should load persisted state) + // Create new store instance and load persisted state. The store is + // created with skipHydration: true, so drive the single rehydrate + // explicitly (OAuthStorageBase does this in production). const storage2 = createFileStorageAdapter({ filePath }); const store2 = createOAuthStore(storage2); - await new Promise((resolve) => setTimeout(resolve, 100)); + await store2.persist.rehydrate(); const state = store2.getState().getServerState("https://example.com"); expect(state.tokens).toEqual({ @@ -332,6 +372,7 @@ describe("Storage adapters", () => { tokens: { access_token: "test-token", token_type: "Bearer" }, }); + const exampleKey = normalizeServerUrl("https://example.com"); await waitForRemoteStore(baseUrl, "test-store", authToken, (body) => { const d = body as { state?: { @@ -339,8 +380,7 @@ describe("Storage adapters", () => { }; }; return ( - d?.state?.servers?.["https://example.com"]?.tokens?.access_token === - "test-token" + d?.state?.servers?.[exampleKey]?.tokens?.access_token === "test-token" ); }); @@ -353,7 +393,7 @@ describe("Storage adapters", () => { }); expect(res.status).toBe(200); const storeData = await res.json(); - expect(storeData.state.servers["https://example.com"].tokens).toEqual({ + expect(storeData.state.servers[exampleKey].tokens).toEqual({ access_token: "test-token", token_type: "Bearer", }); @@ -376,6 +416,7 @@ describe("Storage adapters", () => { store1.getState().setServerState("https://example.com", { tokens: { access_token: "initial-token", token_type: "Bearer" }, }); + const exampleKey = normalizeServerUrl("https://example.com"); await waitForRemoteStore(baseUrl, "test-store", authToken, (body) => { const d = body as { state?: { @@ -383,26 +424,21 @@ describe("Storage adapters", () => { }; }; return ( - d?.state?.servers?.["https://example.com"]?.tokens?.access_token === + d?.state?.servers?.[exampleKey]?.tokens?.access_token === "initial-token" ); }); - // Create new store instance (should load persisted state) + // Create new store instance and load persisted state. The store is + // created with skipHydration: true, so drive the single rehydrate + // explicitly (OAuthStorageBase does this in production). const storage2 = createRemoteStorageAdapter({ baseUrl, storeId: "test-store", authToken, }); const store2 = createOAuthStore(storage2); - await vi.waitFor( - () => { - const state = store2.getState().getServerState("https://example.com"); - if (!state.tokens) throw new Error("Store not yet hydrated"); - return state; - }, - { timeout: 2000, interval: 50 }, - ); + await store2.persist.rehydrate(); const state = store2.getState().getServerState("https://example.com"); expect(state.tokens).toEqual({ diff --git a/clients/web/src/utils/clearServerOAuthState.test.ts b/clients/web/src/utils/clearServerOAuthState.test.ts index 6cdbfeec..713f7b97 100644 --- a/clients/web/src/utils/clearServerOAuthState.test.ts +++ b/clients/web/src/utils/clearServerOAuthState.test.ts @@ -1,50 +1,49 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; -import { getBrowserOAuthStorage } from "@inspector/core/auth/browser/index.js"; +import { describe, it, expect, vi } from "vitest"; import { clearServerOAuthState } from "./clearServerOAuthState"; describe("clearServerOAuthState", () => { - beforeEach(() => { - getBrowserOAuthStorage().clear("https://mcp.example.com/mcp"); - }); - - it("clears storage by server URL when not the active connection", async () => { - const storage = getBrowserOAuthStorage(); - await storage.saveTokens("https://mcp.example.com/mcp", { - access_token: "tok", - token_type: "Bearer", - }); + it("clears storage by server URL when not the active connection", () => { + const oauthStorage = { clear: vi.fn() }; const cleared = clearServerOAuthState({ config: { type: "streamable-http", url: "https://mcp.example.com/mcp" }, isActiveConnection: false, + oauthStorage, }); expect(cleared).toBe(true); - expect( - await storage.getTokens("https://mcp.example.com/mcp"), - ).toBeUndefined(); + expect(oauthStorage.clear).toHaveBeenCalledWith( + "https://mcp.example.com/mcp", + ); }); it("uses the live client when clearing the active connection", () => { const inspectorClient = { clearOAuthTokens: vi.fn(), }; + const oauthStorage = { clear: vi.fn() }; const cleared = clearServerOAuthState({ config: { type: "streamable-http", url: "https://mcp.example.com/mcp" }, inspectorClient: inspectorClient as never, isActiveConnection: true, + oauthStorage, }); expect(cleared).toBe(true); expect(inspectorClient.clearOAuthTokens).toHaveBeenCalledTimes(1); + // The backend-backed store is not touched directly when a live client is + // present — the client owns resetting both persisted and in-memory state. + expect(oauthStorage.clear).not.toHaveBeenCalled(); }); it("returns false for stdio servers", () => { + const oauthStorage = { clear: vi.fn() }; expect( clearServerOAuthState({ config: { type: "stdio", command: "node", args: [] }, isActiveConnection: false, + oauthStorage, }), ).toBe(false); }); diff --git a/clients/web/src/utils/clearServerOAuthState.ts b/clients/web/src/utils/clearServerOAuthState.ts index a3fd9d7c..975308d0 100644 --- a/clients/web/src/utils/clearServerOAuthState.ts +++ b/clients/web/src/utils/clearServerOAuthState.ts @@ -1,4 +1,4 @@ -import { getBrowserOAuthStorage } from "@inspector/core/auth/browser/index.js"; +import type { OAuthStorage } from "@inspector/core/auth/storage.js"; import { getOAuthServerUrl } from "@inspector/core/mcp/config.js"; import type { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; @@ -8,6 +8,13 @@ export interface ClearServerOAuthStateParams { /** When set and this server is the active connection, clear via the live client. */ inspectorClient?: InspectorClient | null; isActiveConnection: boolean; + /** + * Backend-backed OAuth storage (`RemoteOAuthStorage`) used to clear a + * non-active server's persisted state. Must be the same shared instance the + * connection path writes through so the clear targets `oauth.json`, not a + * divergent sessionStorage copy. + */ + oauthStorage: Pick; } /** @@ -26,7 +33,7 @@ export function clearServerOAuthState( if (params.isActiveConnection && params.inspectorClient) { params.inspectorClient.clearOAuthTokens(); } else { - getBrowserOAuthStorage().clear(serverUrl); + params.oauthStorage.clear(serverUrl); } return true; } diff --git a/core/auth/connection-state.ts b/core/auth/connection-state.ts index 3ae4466b..263d5da8 100644 --- a/core/auth/connection-state.ts +++ b/core/auth/connection-state.ts @@ -86,7 +86,7 @@ export async function buildOAuthConnectionState( const preregistered = await storage.getClientInformation(serverUrl, true); const dynamic = await storage.getClientInformation(serverUrl); const storageScope = storage.getScope(serverUrl); - const serverMetadata = storage.getServerMetadata(serverUrl); + const serverMetadata = await storage.getServerMetadata(serverUrl); const authorized = isAccessTokenUsable(tokens); const grantedScope = resolveGrantedScope(tokens, storageScope); @@ -120,7 +120,9 @@ export async function buildOAuthConnectionState( const idp = params.enterpriseManagedAuth.idp; const issuer = normalizeIdpIssuer(idp.issuer); const idpSession = await getEmaIdpLoginState(storage, issuer); - const idpMetadata = storage.getServerMetadata(idpOAuthStorageKey(issuer)); + const idpMetadata = await storage.getServerMetadata( + idpOAuthStorageKey(issuer), + ); state.ema = { idpIssuer: issuer, idpClientId: idp.clientId, diff --git a/core/auth/ema/idpOidc.ts b/core/auth/ema/idpOidc.ts index 8dff2ae6..a5199315 100644 --- a/core/auth/ema/idpOidc.ts +++ b/core/auth/ema/idpOidc.ts @@ -35,7 +35,7 @@ async function resolveIdpMetadata( fetchFn?: typeof fetch, ): Promise { const storageKey = idpOAuthStorageKey(issuer); - const cached = storage.getServerMetadata(storageKey); + const cached = await storage.getServerMetadata(storageKey); if (cached?.token_endpoint) { return cached; } @@ -101,7 +101,7 @@ export async function completeIdpOidcAuthorization(params: { }> { const issuer = normalizeIdpIssuer(params.idp.issuer); const storageKey = idpOAuthStorageKey(issuer); - const metadata = params.storage.getServerMetadata(storageKey); + const metadata = await params.storage.getServerMetadata(storageKey); if (!metadata) { throw new Error("IdP OAuth metadata not found — restart EMA IdP login"); } @@ -111,7 +111,7 @@ export async function completeIdpOidcAuthorization(params: { if (!clientInformation) { throw new Error("IdP client information not found — restart EMA IdP login"); } - const codeVerifier = params.storage.getCodeVerifier(storageKey); + const codeVerifier = await params.storage.getCodeVerifier(storageKey); if (!codeVerifier) { throw new Error("IdP PKCE verifier not found — restart EMA IdP login"); } diff --git a/core/auth/node/storage-node.ts b/core/auth/node/storage-node.ts index 952cb13b..8e66ffe6 100644 --- a/core/auth/node/storage-node.ts +++ b/core/auth/node/storage-node.ts @@ -10,11 +10,17 @@ import { const DEFAULT_STATE_PATH = getStoreFilePath(getDefaultStorageDir(), "oauth"); /** - * Get path to OAuth state file. - * @param customPath - Optional custom path (full path to state file). Default: ~/.mcp-inspector/storage/oauth.json + * Get path to OAuth state file. Resolution order: explicit `customPath`, then + * the `MCP_INSPECTOR_OAUTH_STATE_PATH` environment variable (so tests and + * scripted runs can point at an isolated fixture without touching + * `~/.mcp-inspector`), then the default `~/.mcp-inspector/storage/oauth.json`. */ export function getStateFilePath(customPath?: string): string { - return customPath ?? DEFAULT_STATE_PATH; + return ( + customPath ?? + process.env.MCP_INSPECTOR_OAUTH_STATE_PATH ?? + DEFAULT_STATE_PATH + ); } const storeCache = new Map>(); diff --git a/core/auth/oauth-storage.ts b/core/auth/oauth-storage.ts index f2246112..e5df6b65 100644 --- a/core/auth/oauth-storage.ts +++ b/core/auth/oauth-storage.ts @@ -20,18 +20,87 @@ import type { * Concrete OAuthStorage implementation parameterized on a Zustand store. * The store carries the storage adapter (sessionStorage, file, remote HTTP, …), * so the same body works for browser, Node, and remote environments. + * + * With an async storage adapter (file, remote HTTP) the store is empty until + * hydration completes. Every read used on the post-OAuth-redirect callback path + * (`getCodeVerifier`, `getServerMetadata`, `getClientInformation`, `getTokens`) + * — and every save — therefore awaits {@link ready} first; saves wait so a + * late hydration merge cannot clobber a value written before it landed. The + * underlying store is created with `skipHydration: true` + * ({@link createOAuthStore}), so the constructor's `rehydrate()` is the single + * hydration and there is no auto-hydration to race it. With a synchronous + * adapter (sessionStorage) hydration resolves on the next microtask and the + * behaviour is effectively unchanged. */ export class OAuthStorageBase implements OAuthStorage { private readonly store: ReturnType; + private readonly hydrated: Promise; + private hydrationError?: Error; constructor(store: ReturnType) { this.store = store; + // The store is created with `skipHydration: true` so this is the SOLE + // hydration. Driving it explicitly (rather than relying on persist's + // auto-hydration + onFinishHydration) lets us catch a rejecting adapter — + // onFinishHydration only fires on success, so a throwing getItem would + // otherwise leave `hydrated` pending forever and hang every getter. + // A failed hydration resolves to "empty store", which is the correct + // fallback (reads return undefined; writes proceed) — but the failure is + // recorded so callers can distinguish "no token stored" from "could not + // read the store" (e.g. corrupt file, EACCES, backend 500). + const recordFailure = (err: unknown) => { + this.hydrationError = + err instanceof Error + ? err + : new Error( + typeof err === "string" + ? err + : "OAuth storage hydration failed (adapter getItem threw or returned invalid data)", + ); + console.warn( + "[OAuthStorage] hydration failed; continuing with empty state:", + this.hydrationError, + ); + }; + this.hydrated = this.store.persist.hasHydrated() + ? Promise.resolve() + : Promise.resolve(this.store.persist.rehydrate()).then( + () => { + // Zustand swallows getItem errors internally (routing them to + // onRehydrateStorage) and resolves rehydrate() regardless; + // hasHydrated() only flips true on success, so use it to detect a + // silently-failed hydration. The underlying error is lost in this + // path — record a generic one so getHydrationError() is non-null. + if (!this.store.persist.hasHydrated()) recordFailure(undefined); + }, + (err: unknown) => recordFailure(err), + ); + } + + /** + * If hydration failed (adapter threw, file unreadable/corrupt, backend + * non-2xx), the captured error. Undefined when hydration succeeded or has + * not yet completed — call after `await ready()`. Lets callers report + * "oauth.json is unreadable" instead of a misleading "no token stored". + */ + getHydrationError(): Error | undefined { + return this.hydrationError; + } + + /** + * Resolves once the underlying persist adapter has hydrated the store. + * Callers that need to read state outside the typed getters can await this + * directly (e.g. before reading the store via `getState()` for diagnostics). + */ + ready(): Promise { + return this.hydrated; } async getClientInformation( serverUrl: string, isPreregistered?: boolean, ): Promise { + await this.hydrated; const state = this.store.getState().getServerState(serverUrl); const clientInfo = isPreregistered ? state.preregisteredClientInformation @@ -56,6 +125,7 @@ export class OAuthStorageBase implements OAuthStorage { clientInformation: OAuthClientInformation, options: SaveClientInformationOptions, ): Promise { + await this.hydrated; this.store.getState().setServerState(serverUrl, { clientInformation, clientRegistrationKind: options.registrationKind, @@ -66,6 +136,7 @@ export class OAuthStorageBase implements OAuthStorage { serverUrl: string, clientInformation: OAuthClientInformation, ): Promise { + await this.hydrated; this.store.getState().setServerState(serverUrl, { preregisteredClientInformation: clientInformation, clientRegistrationKind: "static", @@ -86,6 +157,7 @@ export class OAuthStorageBase implements OAuthStorage { } async getTokens(serverUrl: string): Promise { + await this.hydrated; const state = this.store.getState().getServerState(serverUrl); if (!state.tokens) { return undefined; @@ -99,6 +171,7 @@ export class OAuthStorageBase implements OAuthStorage { tokens: OAuthTokens, options?: SaveTokensOptions, ): Promise { + await this.hydrated; this.store.getState().setServerState(serverUrl, { tokens, ...(options?.enterpriseManaged === true && { enterpriseManaged: true }), @@ -109,7 +182,8 @@ export class OAuthStorageBase implements OAuthStorage { this.store.getState().setServerState(serverUrl, { tokens: undefined }); } - getCodeVerifier(serverUrl: string): string | undefined { + async getCodeVerifier(serverUrl: string): Promise { + await this.hydrated; const state = this.store.getState().getServerState(serverUrl); return state.codeVerifier; } @@ -118,6 +192,7 @@ export class OAuthStorageBase implements OAuthStorage { serverUrl: string, codeVerifier: string, ): Promise { + await this.hydrated; this.store.getState().setServerState(serverUrl, { codeVerifier }); } @@ -127,12 +202,21 @@ export class OAuthStorageBase implements OAuthStorage { .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. + */ getScope(serverUrl: string): string | undefined { const state = this.store.getState().getServerState(serverUrl); return state.scope; } async saveScope(serverUrl: string, scope: string | undefined): Promise { + await this.hydrated; this.store.getState().setServerState(serverUrl, { scope }); } @@ -140,7 +224,8 @@ export class OAuthStorageBase implements OAuthStorage { this.store.getState().setServerState(serverUrl, { scope: undefined }); } - getServerMetadata(serverUrl: string): OAuthMetadata | null { + async getServerMetadata(serverUrl: string): Promise { + await this.hydrated; const state = this.store.getState().getServerState(serverUrl); return state.serverMetadata || null; } @@ -149,6 +234,7 @@ export class OAuthStorageBase implements OAuthStorage { serverUrl: string, metadata: OAuthMetadata, ): Promise { + await this.hydrated; this.store .getState() .setServerState(serverUrl, { serverMetadata: metadata }); @@ -165,6 +251,7 @@ export class OAuthStorageBase implements OAuthStorage { } async getIdpSession(issuer: string): Promise { + await this.hydrated; const session = this.store.getState().getIdpSession(issuer); if ( !session.idToken && @@ -180,6 +267,7 @@ export class OAuthStorageBase implements OAuthStorage { issuer: string, session: Partial, ): Promise { + await this.hydrated; this.store.getState().setIdpSession(issuer, session); } diff --git a/core/auth/providers.ts b/core/auth/providers.ts index b01c9fb7..f4b9faf0 100644 --- a/core/auth/providers.ts +++ b/core/auth/providers.ts @@ -234,8 +234,8 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { await this.storage.saveCodeVerifier(this.serverUrl, codeVerifier); } - codeVerifier(): string { - const verifier = this.storage.getCodeVerifier(this.serverUrl); + async codeVerifier(): Promise { + const verifier = await this.storage.getCodeVerifier(this.serverUrl); if (!verifier) { throw new Error("No code verifier saved for session"); } @@ -246,8 +246,8 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { this.storage.clear(this.serverUrl); } - getServerMetadata(): OAuthMetadata | null { - return this.storage.getServerMetadata(this.serverUrl); + async getServerMetadata(): Promise { + return await this.storage.getServerMetadata(this.serverUrl); } async saveServerMetadata(metadata: OAuthMetadata): Promise { diff --git a/core/auth/storage.ts b/core/auth/storage.ts index f8b46874..c95ff084 100644 --- a/core/auth/storage.ts +++ b/core/auth/storage.ts @@ -21,6 +21,12 @@ export interface SaveClientInformationOptions { } export interface OAuthStorage { + /** + * Resolves once any async hydration of the underlying store has completed. + * With a synchronous storage adapter this resolves immediately. + */ + ready(): Promise; + /** * Get client information (preregistered or dynamically registered) */ @@ -78,9 +84,13 @@ export interface OAuthStorage { clearTokens(serverUrl: string): void; /** - * Get code verifier (for PKCE) + * Get code verifier (for PKCE). + * + * Async because the backend-backed store may still be hydrating when this is + * read on the post-redirect OAuth callback path; the implementation awaits + * hydration before reading. */ - getCodeVerifier(serverUrl: string): string | undefined; + getCodeVerifier(serverUrl: string): Promise; /** * Save code verifier (for PKCE) @@ -108,9 +118,13 @@ export interface OAuthStorage { clearScope(serverUrl: string): void; /** - * Get server metadata discovered during OAuth + * Get server metadata discovered during OAuth. + * + * Async because the backend-backed store may still be hydrating when this is + * read on the post-redirect OAuth callback path; the implementation awaits + * hydration before reading. */ - getServerMetadata(serverUrl: string): OAuthMetadata | null; + getServerMetadata(serverUrl: string): Promise; /** * Save server metadata discovered during OAuth diff --git a/core/auth/store.ts b/core/auth/store.ts index a54bfb30..b32360ed 100644 --- a/core/auth/store.ts +++ b/core/auth/store.ts @@ -28,6 +28,26 @@ export interface ServerOAuthState { enterpriseManaged?: boolean; } +/** + * Normalize a server URL to its canonical form for use as a store key. + * + * Different callers (web deep-link parser, CLI `--server-url`, the SDK's own + * client constructor) can present the same endpoint with cosmetic differences + * — host case, a trailing slash on a bare-origin URL, default-port omission — + * that `new URL().href` collapses. Keying by the canonical form means a token + * the web inspector saved under `https://Example.com/mcp` is found when the + * CLI later asks for `https://example.com/mcp/`. Non-URL strings (e.g. a + * stdio server name) are returned trimmed so the store still works for them. + */ +export function normalizeServerUrl(serverUrl: string): string { + const trimmed = serverUrl.trim(); + try { + return new URL(trimmed).href; + } catch { + return trimmed; + } +} + /** * Zustand store state (all servers) */ @@ -59,26 +79,34 @@ export function createOAuthStore( servers: {}, idpSessions: {}, getServerState: (serverUrl: string) => { - return get().servers[serverUrl] || {}; + const servers = get().servers; + // Look up by canonical key, falling back to the raw key so an + // already-persisted blob written before normalization existed (or by + // another writer) is still found. + return ( + servers[normalizeServerUrl(serverUrl)] ?? servers[serverUrl] ?? {} + ); }, setServerState: ( serverUrl: string, updates: Partial, ) => { + const key = normalizeServerUrl(serverUrl); set((state) => ({ servers: { ...state.servers, - [serverUrl]: { - ...state.servers[serverUrl], + [key]: { + ...state.servers[key], ...updates, }, }, })); }, clearServerState: (serverUrl: string) => { + const key = normalizeServerUrl(serverUrl); set((state) => { const rest = { ...state.servers }; - delete rest[serverUrl]; + delete rest[key]; return { servers: rest }; }); }, @@ -118,6 +146,11 @@ export function createOAuthStore( { name: "mcp-inspector-oauth", storage, + // OAuthStorageBase drives `persist.rehydrate()` itself so it can await + // (and catch errors from) the single hydration. Auto-hydration here + // would race the explicit one and could clobber a save that landed + // between them. + skipHydration: true, }, ), ); diff --git a/core/auth/utils.ts b/core/auth/utils.ts index 638a4080..123274f8 100644 --- a/core/auth/utils.ts +++ b/core/auth/utils.ts @@ -50,20 +50,17 @@ export const parseOAuthCallbackParams = (location: string): CallbackParams => { * @returns A random state for the OAuth 2.0 flow. */ export const generateOAuthState = (): string => { - // Generate a random state - const array = new Uint8Array(32); - - // Use crypto.getRandomValues (available in both browser and Node.js) - if (typeof crypto !== "undefined" && crypto.getRandomValues) { - crypto.getRandomValues(array); - } else { - // Fallback for environments without crypto.getRandomValues - // This should not happen in modern environments - for (let i = 0; i < array.length; i++) { - array[i] = Math.floor(Math.random() * 256); - } + // OAuth state is a CSRF token — it MUST be unpredictable. crypto.getRandomValues + // is available in every supported runtime (browsers, Node ≥15); if it's somehow + // missing, fail loudly rather than silently degrading to Math.random (whose + // output is predictable from a small amount of observed state). + if (typeof crypto === "undefined" || !crypto.getRandomValues) { + throw new Error( + "crypto.getRandomValues is not available; refusing to generate an OAuth state with a non-cryptographic RNG.", + ); } - + const array = new Uint8Array(32); + crypto.getRandomValues(array); return Array.from(array, (byte) => byte.toString(16).padStart(2, "0")).join( "", ); diff --git a/core/storage/adapters/remote-storage.ts b/core/storage/adapters/remote-storage.ts index d143584a..686506ab 100644 --- a/core/storage/adapters/remote-storage.ts +++ b/core/storage/adapters/remote-storage.ts @@ -42,7 +42,9 @@ export function createRemoteStorageAdapter( if (res.status === 404) { return null; } - throw new Error(`Failed to read store: ${res.status}`); + throw new Error( + `Failed to read store '${options.storeId}' from ${baseUrl}/api/storage/${options.storeId}: ${res.status}`, + ); } const store = await res.json(); @@ -62,16 +64,32 @@ export function createRemoteStorageAdapter( headers["x-mcp-remote-auth"] = `Bearer ${options.authToken}`; } - // Zustand gives us the full persisted format as a string - // Store it as-is (the API treats it as an opaque blob) - const res = await fetchFn(`${baseUrl}/api/storage/${options.storeId}`, { - method: "POST", - headers, - body: value, // Already a JSON string from Zustand - }); + const url = `${baseUrl}/api/storage/${options.storeId}`; + try { + // Zustand gives us the full persisted format as a string + // Store it as-is (the API treats it as an opaque blob). + // `keepalive` lets this POST outlive an immediately-following + // 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. + const res = await fetchFn(url, { + method: "POST", + headers, + body: value, // Already a JSON string from Zustand + keepalive: true, + }); - if (!res.ok) { - throw new Error(`Failed to write store: ${res.status}`); + if (!res.ok) { + throw new Error( + `Failed to write store '${options.storeId}' to ${url}: ${res.status}`, + ); + } + } catch (err) { + // Zustand's persist middleware swallows setItem rejections — without + // this the user sees a green "Connected" while the token never landed + // on disk. Surface the failure so it's at least observable. + console.error("[remote-storage] persist write failed:", err); + throw err; } }, removeItem: async (_name: string) => {