From ba48e06ffd9779e74b9125bbfe02ae1fc1ac5ccd Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 8 Jul 2026 10:59:40 -0400 Subject: [PATCH] fix(auth): guard OAuth-storage async paths against unhandled rejections Review follow-ups on the RemoteOAuthStorage web migration: - useEmaIdpLoginState.logout(): add a .catch to the now-async clearEmaIdpSession call so a storage failure no longer surfaces as an unhandled rejection; leave loginState unchanged on failure (the session was not actually cleared). Add a test asserting no unhandled rejection. - createOAuthStore: attach a no-op catch to the eager persistLoadPromise so a hydration read that fails before any load() awaits it does not become an unhandled rejection; real awaiters still observe the rejection via the shared reference. - storage-remote.test: replace `as unknown as typeof fetch` double-casts with the typed vi.fn() pattern the same PR established. - remoteOAuthStorage: correct the doc comment to describe the cache-of-one accurately instead of implying a keyed map. Co-Authored-By: Claude Opus 4.8 (1M context) --- clients/web/src/lib/remoteOAuthStorage.ts | 8 ++- .../src/test/core/auth/storage-remote.test.ts | 8 +-- .../core/react/useEmaIdpLoginState.test.tsx | 52 +++++++++++++++++++ core/auth/store.ts | 7 +++ core/react/useEmaIdpLoginState.ts | 14 +++-- 5 files changed, 80 insertions(+), 9 deletions(-) diff --git a/clients/web/src/lib/remoteOAuthStorage.ts b/clients/web/src/lib/remoteOAuthStorage.ts index 0d95806b..12b13e94 100644 --- a/clients/web/src/lib/remoteOAuthStorage.ts +++ b/clients/web/src/lib/remoteOAuthStorage.ts @@ -17,8 +17,12 @@ const defaultFetch: typeof fetch = (...args) => globalThis.fetch(...args); * Shared web OAuth store: `RemoteOAuthStorage` → `GET/POST /api/storage/oauth` * → `~/.mcp-inspector/storage/oauth.json` (same file as CLI/TUI). * - * Memoized by `{ baseUrl, authToken }` so connect, EMA IdP session, and - * per-server clear all mutate the same in-memory view. + * Caches a single instance keyed by `{ baseUrl, authToken }`: repeat calls with + * the same key return that instance, so connect, EMA IdP session, and per-server + * clear all mutate the same in-memory view. A call with a different key replaces + * the cached instance (cache of one). That is sufficient because the web app + * uses a stable origin + page-lifetime API token, so the key never changes + * within a session. */ export function getRemoteOAuthStorage( options: WebRemoteOAuthStorageOptions, 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 be8536a5..fa1b02fe 100644 --- a/clients/web/src/test/core/auth/storage-remote.test.ts +++ b/clients/web/src/test/core/auth/storage-remote.test.ts @@ -142,12 +142,12 @@ describe("RemoteOAuthStorage (unit, mocked fetch)", () => { version: 0, }; let resolveFetch!: (value: Response) => void; - const delayedFetch = vi.fn( + const delayedFetch = vi.fn( () => new Promise((resolve) => { resolveFetch = resolve; }), - ) as unknown as typeof fetch; + ); const delayedStorage = new RemoteOAuthStorage({ baseUrl: "http://remote.example", @@ -167,9 +167,9 @@ describe("RemoteOAuthStorage (unit, mocked fetch)", () => { }); it("load() rejects when remote GET fails instead of hanging", async () => { - const failingFetch = vi.fn( + const failingFetch = vi.fn( async () => new Response("server error", { status: 500 }), - ) as unknown as typeof fetch; + ); const failingStorage = new RemoteOAuthStorage({ baseUrl: "http://remote.example", diff --git a/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx b/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx index bc7f1d6b..0b141571 100644 --- a/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx +++ b/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx @@ -67,6 +67,58 @@ describe("useEmaIdpLoginState", () => { }); }); + it("logout swallows a clear failure without an unhandled rejection and keeps state", async () => { + const exp = Math.floor(Date.now() / 1000) + 3600; + const payload = btoa(JSON.stringify({ exp })) + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=+$/, ""); + vi.mocked(storage.getIdpSession).mockResolvedValue({ + idToken: `h.${payload}.s`, + }); + vi.mocked(storage.clearIdpSession).mockRejectedValue( + new Error("storage backend unreachable"), + ); + const unhandled = vi.fn(); + const onUnhandled = (event: PromiseRejectionEvent) => { + event.preventDefault(); + unhandled(); + }; + window.addEventListener("unhandledrejection", onUnhandled); + + try { + const { result } = renderHook(() => + useEmaIdpLoginState(storage, "https://idp.test", true), + ); + + await waitFor(() => { + expect(result.current.loginState).toBe("logged_in"); + }); + + act(() => { + result.current.logout(); + }); + + await waitFor(() => { + expect(storage.clearIdpSession).toHaveBeenCalledWith( + "https://idp.test", + ); + }); + // Give the rejected promise a turn to settle so any unhandled rejection + // would have fired. + await act(async () => { + await Promise.resolve(); + }); + + // Clear failed, so the session is still present: state stays "logged_in" + // and no unhandled rejection escaped. + expect(result.current.loginState).toBe("logged_in"); + expect(unhandled).not.toHaveBeenCalled(); + } finally { + window.removeEventListener("unhandledrejection", onUnhandled); + } + }); + it("reports 'expired' for an expired token with no refresh token", async () => { const exp = Math.floor(Date.now() / 1000) - 3600; const payload = btoa(JSON.stringify({ exp })) diff --git a/core/auth/store.ts b/core/auth/store.ts index 52d966c3..1be06be6 100644 --- a/core/auth/store.ts +++ b/core/auth/store.ts @@ -78,6 +78,13 @@ export function createOAuthStore( resolvePersistLoad = resolve; rejectPersistLoad = reject; }); + // Zustand kicks off async hydration as soon as the store is created, before + // any consumer calls `OAuthStorageBase.load()`. If that initial read fails + // and no one is awaiting yet, `persistLoadPromise` would reject with no + // handler and surface as an unhandled rejection. Attach a no-op handler so + // the promise is always "handled"; real awaiters still observe the rejection + // through the reference returned by `waitForOAuthStorePersistLoad`. + void persistLoadPromise.catch(() => {}); const settlePersistLoad = (error?: unknown) => { if (persistLoadSettled) { diff --git a/core/react/useEmaIdpLoginState.ts b/core/react/useEmaIdpLoginState.ts index 19f69827..85932561 100644 --- a/core/react/useEmaIdpLoginState.ts +++ b/core/react/useEmaIdpLoginState.ts @@ -40,9 +40,17 @@ export function useEmaIdpLoginState( const logout = useCallback(() => { if (!normalizedIssuer) return; - void clearEmaIdpSession(storage, normalizedIssuer).then(() => { - setLoginState("none"); - }); + void clearEmaIdpSession(storage, normalizedIssuer) + .then(() => { + setLoginState("none"); + }) + .catch(() => { + // Clearing the persisted IdP session failed (e.g. the storage backend + // is unreachable). Swallow the rejection so it does not surface as an + // unhandled promise, and leave loginState unchanged so the UI keeps + // reflecting the still-present session rather than falsely showing + // signed-out. + }); }, [storage, normalizedIssuer]); return { loginState, refresh, logout };