diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 2903d606f..41b88f95e 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -34,8 +34,10 @@ import { StderrLogState } from "@inspector/core/mcp/state/stderrLogState.js"; import type { RedirectUrlProvider } from "@inspector/core/auth/index.js"; import { parseOAuthCallbackParams, + parseOAuthState, generateOAuthErrorDescription, } from "@inspector/core/auth/index.js"; +import { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js"; import { useInspectorClient } from "@inspector/core/react/useInspectorClient.js"; import { useServers } from "@inspector/core/react/useServers.js"; import { useSettingsDraft } from "@inspector/core/react/useSettingsDraft.js"; @@ -450,11 +452,68 @@ function App() { [messages], ); + // Backend-backed session storage used to carry the fetch (Network) log + // across the OAuth full-page redirect. The auth handshake's first half — + // protected-resource + auth-server discovery and Dynamic Client + // Registration — happens on the pre-redirect page; without persisting it + // those `auth` entries would vanish when the browser navigates to the + // authorization server. `FetchRequestLogState` saves to this on the + // client's `saveSession` event (fired in `onBeforeOAuthRedirect`) keyed by + // the OAuth authId, and restores from it when rebuilt on `/oauth/callback`. + // Created once; `getAuthToken()` is stable for the page's lifetime. + const sessionStorageAdapter = useMemo( + () => + new RemoteInspectorClientStorage({ + baseUrl: + typeof window !== "undefined" + ? window.location.origin + : "http://localhost", + authToken: getAuthToken(), + }), + [], + ); + + // Always points at the live `FetchRequestLogState` so the synchronous + // pre-redirect hook below can read the current Network log without being + // rebound every time the active server (and its log state) changes. + const fetchLogRef = useRef(null); + + // Flush the pre-redirect Network log to backend storage, keyed by the OAuth + // authId carried in the authorization URL's `state`. Runs synchronously from + // `BrowserNavigation` right before `window.location.href`, so the keepalive + // POST it kicks off outlives the unloading page. The `/oauth/callback` + // rebuild restores these entries via `FetchRequestLogState`'s `sessionId`. + // Stable identity: it reads mutable refs, so it never needs to be rebuilt. + const onBeforeOAuthRedirect = useCallback( + (authorizationUrl: URL) => { + const stateParam = authorizationUrl.searchParams.get("state"); + const authId = stateParam + ? (parseOAuthState(stateParam)?.authId ?? undefined) + : undefined; + if (!authId) return; + const fetchRequests = fetchLogRef.current?.getFetchRequests() ?? []; + if (fetchRequests.length === 0) return; + const now = Date.now(); + // Fire-and-forget: the keepalive request inside `saveSession` is + // dispatched synchronously here, before navigation commits. + void sessionStorageAdapter + .saveSession(authId, { + fetchRequests, + createdAt: now, + updatedAt: now, + }) + .catch(() => { + // Best-effort; losing the pre-redirect log is non-fatal. + }); + }, + [sessionStorageAdapter], + ); + // Wire up + tear down per active server. Called by `onToggleConnection` // when the user switches targets. Returns the new client so the toggle // can call `connect()` against it before React re-renders. const setupClientForServer = useCallback( - (server: ServerEntry): InspectorClient => { + (server: ServerEntry, sessionId?: string): InspectorClient => { // Tear down the previous session's managers — each destroy() // unsubscribes from the old client's events. Skipped on the first // call (initial values are null). @@ -471,6 +530,7 @@ function App() { const { environment } = createWebEnvironment( getAuthToken(), redirectUrlProvider, + onBeforeOAuthRedirect, ); // The settings node persisted in mcp.json for this server — distinct // from the InspectorClient options we're about to derive from it. @@ -519,6 +579,10 @@ function App() { }), ...(oauth && { oauth }), ...(savedSettings && { serverSettings: savedSettings }), + // Set on the `/oauth/callback` rebuild so the client's `saveSession` + // events (and any later persistence) key off the same OAuth authId + // the pre-redirect page saved under. + ...(sessionId && { sessionId }), }); setInspectorClient(client); @@ -538,7 +602,18 @@ function App() { new ResourceSubscriptionsState(client, nextResourcesState), ); setMessageLogState(new MessageLogState(client)); - setFetchRequestLogState(new FetchRequestLogState(client)); + // Wire session storage so the fetch log survives the OAuth redirect. + // When `sessionId` is supplied (the `/oauth/callback` rebuild) the prior + // page's `auth` entries are restored on construction; the actual save is + // driven synchronously from `onBeforeOAuthRedirect` above (keyed by the + // same authId). Keep `fetchLogRef` pointed at this instance so that hook + // reads the current log. + const nextFetchLog = new FetchRequestLogState(client, { + sessionStorage: sessionStorageAdapter, + ...(sessionId && { sessionId }), + }); + fetchLogRef.current = nextFetchLog; + setFetchRequestLogState(nextFetchLog); setStderrLogState(new StderrLogState(client)); return client; @@ -553,6 +628,8 @@ function App() { messageLogState, fetchRequestLogState, stderrLogState, + sessionStorageAdapter, + onBeforeOAuthRedirect, ], ); @@ -574,6 +651,14 @@ function App() { oauthCallbackHandledRef.current = true; const params = parseOAuthCallbackParams(window.location.search); + // The OAuth `state` round-trips `{mode}:{authId}`; the authId is the + // session key the pre-redirect page saved the fetch log under, so the + // rebuilt client can restore those `auth` entries. Read it before the + // URL is cleared below. + const stateParam = new URLSearchParams(window.location.search).get("state"); + const sessionId = stateParam + ? (parseOAuthState(stateParam)?.authId ?? undefined) + : undefined; const pendingId = window.sessionStorage.getItem(OAUTH_PENDING_SERVER_KEY) ?? undefined; window.sessionStorage.removeItem(OAUTH_PENDING_SERVER_KEY); @@ -609,7 +694,7 @@ function App() { } void (async () => { - const client = setupClientForServer(server); + const client = setupClientForServer(server, sessionId); setActiveServerId(server.id); // Two distinct failure modes get distinct toasts. A token-exchange // failure means OAuth did NOT complete — and since the single-use code diff --git a/clients/web/src/lib/environmentFactory.ts b/clients/web/src/lib/environmentFactory.ts index 350a16f00..bba83fc32 100644 --- a/clients/web/src/lib/environmentFactory.ts +++ b/clients/web/src/lib/environmentFactory.ts @@ -30,10 +30,16 @@ export interface WebEnvironmentResult { * `authToken` is read from a higher level (currently unused in this app since * v2 has no auth-token UI yet, but kept in the signature so the wiring is * ready when token plumbing lands). + * + * `onBeforeOAuthRedirect` runs synchronously immediately before the OAuth + * full-page redirect (see `BrowserNavigation`). The app uses it to flush the + * pre-redirect Network log to backend storage so the auth handshake's first + * half (discovery + Dynamic Client Registration) survives the navigation. */ export function createWebEnvironment( authToken: string | undefined, redirectUrlProvider: RedirectUrlProvider, + onBeforeOAuthRedirect?: (authorizationUrl: URL) => void, ): WebEnvironmentResult { const baseUrl = `${window.location.protocol}//${window.location.host}`; @@ -62,7 +68,7 @@ export function createWebEnvironment( logger, oauth: { storage: new BrowserOAuthStorage(), - navigation: new BrowserNavigation(), + navigation: new BrowserNavigation(undefined, onBeforeOAuthRedirect), redirectUrlProvider, }, }; diff --git a/clients/web/src/test/core/auth/providers.test.ts b/clients/web/src/test/core/auth/providers.test.ts index 77734cf8e..1aa53a869 100644 --- a/clients/web/src/test/core/auth/providers.test.ts +++ b/clients/web/src/test/core/auth/providers.test.ts @@ -79,6 +79,39 @@ describe("OAuthNavigation", () => { "BrowserNavigation requires browser environment", ); }); + + it("runs beforeNavigate synchronously BEFORE assigning location.href", () => { + // The pre-redirect persistence relies on this ordering: the hook must + // observe the still-current document (location.href not yet reassigned) + // so a keepalive request it fires outlives the navigation. + const order: string[] = []; + const authUrl = new URL("http://example.com/authorize?state=normal:abc"); + const navigation = new BrowserNavigation(undefined, (url) => { + order.push("before"); + // At hook time the redirect has not happened yet. + expect((global as GlobalWithWindow).window!.location.href).toBe( + "http://localhost:5173", + ); + expect(url.toString()).toBe(authUrl.toString()); + }); + + navigation.navigateToAuthorization(authUrl); + order.push("after"); + + expect(order).toEqual(["before", "after"]); + expect((global as GlobalWithWindow).window!.location.href).toBe( + authUrl.toString(), + ); + }); + + it("still navigates when no beforeNavigate hook is provided", () => { + const navigation = new BrowserNavigation(); + const authUrl = new URL("http://example.com/authorize"); + navigation.navigateToAuthorization(authUrl); + expect((global as GlobalWithWindow).window!.location.href).toBe( + authUrl.toString(), + ); + }); }); describe("BrowserOAuthClientProvider", () => { diff --git a/clients/web/src/test/core/mcp/remote/sessionStorage.test.ts b/clients/web/src/test/core/mcp/remote/sessionStorage.test.ts index e091c30d5..0437f9047 100644 --- a/clients/web/src/test/core/mcp/remote/sessionStorage.test.ts +++ b/clients/web/src/test/core/mcp/remote/sessionStorage.test.ts @@ -160,4 +160,28 @@ describe("RemoteInspectorClientStorage", () => { ], ).toBeUndefined(); }); + + it("defaults to a wrapper around globalThis.fetch (not the bare reference)", async () => { + // Regression guard: the default must call `globalThis.fetch` with the + // global as receiver. Assigning the bare reference and invoking it as + // `this.fetchFn(...)` throws "Illegal invocation" — which callers swallow + // via `.catch(() => {})`, so it would silently break all save/load. + const spy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response(null, { status: 204 })); + try { + const storage = new RemoteInspectorClientStorage({ + baseUrl: "http://remote.example/", + }); + await expect( + storage.saveSession("sid", sampleState()), + ).resolves.toBeUndefined(); + expect(spy).toHaveBeenCalledTimes(1); + expect(spy.mock.calls[0]?.[0]).toBe( + "http://remote.example/api/storage/inspector-session-sid", + ); + } finally { + spy.mockRestore(); + } + }); }); diff --git a/clients/web/src/test/core/mcp/state/fetchRequestLogState.test.ts b/clients/web/src/test/core/mcp/state/fetchRequestLogState.test.ts index e2cec0366..6af743e86 100644 --- a/clients/web/src/test/core/mcp/state/fetchRequestLogState.test.ts +++ b/clients/web/src/test/core/mcp/state/fetchRequestLogState.test.ts @@ -202,6 +202,73 @@ describe("FetchRequestLogState", () => { expect(hydrated.getFetchRequests().map((e) => e.id)).toEqual(["r2", "r3"]); }); + it("merges restored entries ahead of live ones, deduping by id, when load resolves after a live append", async () => { + // Mirrors the `/oauth/callback` race: the resuming connect appends live + // entries while the persisted pre-redirect log is still loading. The + // restored (older) entries must land in front without clobbering the live + // ones, and an id present in both must not duplicate. + let resolveLoad: + | ((s: InspectorClientSessionState | undefined) => void) + | undefined; + const storage: InspectorClientStorage = { + async saveSession() {}, + loadSession: () => + new Promise((resolve) => { + resolveLoad = resolve; + }), + async deleteSession() {}, + }; + const hydrated = new FetchRequestLogState(client, { + sessionStorage: storage, + sessionId: "sess-merge", + }); + // Live entries arrive before the load resolves. + client.dispatchTypedEvent("fetchRequest", entry("token")); + client.dispatchTypedEvent("fetchRequest", entry("transport")); + // Restored set includes the two pre-redirect entries plus a duplicate of + // a live one ("token"). + resolveLoad?.({ + fetchRequests: [entry("discovery"), entry("register"), entry("token")], + createdAt: 0, + updatedAt: 0, + }); + await Promise.resolve(); + await Promise.resolve(); + expect(hydrated.getFetchRequests().map((e) => e.id)).toEqual([ + "discovery", + "register", + "token", + "transport", + ]); + hydrated.destroy(); + }); + + it("hydrate is a no-op when restored entries are all already present", async () => { + const storage = makeStorage({ + "sess-dup": { + fetchRequests: [entry("a")], + createdAt: 0, + updatedAt: 0, + }, + }); + const hydrated = new FetchRequestLogState(client, { + sessionStorage: storage, + sessionId: "sess-dup", + }); + const changes: FetchRequestEntry[][] = []; + hydrated.addEventListener("fetchRequestsChange", (e) => + changes.push(e.detail), + ); + client.dispatchTypedEvent("fetchRequest", entry("a")); + changes.length = 0; // ignore the live-append dispatch + await Promise.resolve(); + await Promise.resolve(); + // The only restored entry ("a") is already present → no merge, no event. + expect(hydrated.getFetchRequests().map((e) => e.id)).toEqual(["a"]); + expect(changes).toEqual([]); + hydrated.destroy(); + }); + it("ignores hydration when destroy() happens first", async () => { let resolveLoad: | ((s: InspectorClientSessionState | undefined) => void) diff --git a/core/auth/browser/providers.ts b/core/auth/browser/providers.ts index bafc633b1..7f30b2cf4 100644 --- a/core/auth/browser/providers.ts +++ b/core/auth/browser/providers.ts @@ -9,15 +9,29 @@ export type { OAuthNavigationCallback } from "../providers.js"; /** * Browser navigation handler - * Redirects the browser window to the authorization URL, optionally invokes an - * extra callback. + * Redirects the browser window to the authorization URL. + * + * `beforeNavigate` runs **synchronously, immediately before** the + * `window.location.href` assignment. It is the last point at which code can + * run on the soon-to-unload document, so consumers that need to flush state + * across the OAuth redirect (e.g. persisting the Network log via a `keepalive` + * request) must do it here — a request dispatched synchronously before + * navigation survives the unload, whereas one fired from a later microtask + * (after `auth()` returns) is dropped when the document tears down. + * + * `callback` runs after navigation is requested; kept for backwards + * compatibility with callers that only need a post-redirect notification. */ export class BrowserNavigation extends CallbackNavigation { - constructor(callback?: OAuthNavigationCallback) { + constructor( + callback?: OAuthNavigationCallback, + beforeNavigate?: (authorizationUrl: URL) => void, + ) { super((url) => { if (typeof window === "undefined") { throw new Error("BrowserNavigation requires browser environment"); } + beforeNavigate?.(url); window.location.href = url.href; return callback?.(url); }); diff --git a/core/mcp/remote/sessionStorage.ts b/core/mcp/remote/sessionStorage.ts index 593baf417..9c24ee59c 100644 --- a/core/mcp/remote/sessionStorage.ts +++ b/core/mcp/remote/sessionStorage.ts @@ -31,7 +31,12 @@ export class RemoteInspectorClientStorage implements InspectorClientStorage { constructor(options: RemoteInspectorClientStorageOptions) { this.baseUrl = options.baseUrl.replace(/\/$/, ""); this.authToken = options.authToken; - this.fetchFn = options.fetchFn ?? globalThis.fetch; + // Default to a wrapper rather than `globalThis.fetch` directly: invoking + // the bare reference as `this.fetchFn(...)` re-binds `this` to this + // instance, which native `fetch` rejects with "Illegal invocation". The + // wrapper preserves the global receiver. (Same gotcha handled in + // `environmentFactory.ts` for the transport/auth fetchers.) + this.fetchFn = options.fetchFn ?? ((...args) => globalThis.fetch(...args)); } private getStoreId(sessionId: string): string { @@ -69,6 +74,17 @@ export class RemoteInspectorClientStorage implements InspectorClientStorage { method: "POST", headers, body: JSON.stringify(serializedState), + // `saveSession` runs as the OAuth full-page redirect is being + // scheduled, so a normal fetch would be cancelled mid-flight and the + // pre-redirect network log (discovery + DCR) would never reach disk. + // `keepalive` lets the request outlive the unloading document. + // Caveat: keepalive caps the body at 64KB. The pre-redirect-log payload + // is small, but this method is also reachable from + // `FetchRequestLogState`'s `saveSession` listener with the full session + // log — a long session could exceed the cap and be dropped silently. + // Acceptable here: the persisted log is a best-effort convenience, not + // load-bearing state. + keepalive: true, }); if (!res.ok) { diff --git a/core/mcp/state/fetchRequestLogState.ts b/core/mcp/state/fetchRequestLogState.ts index cfa99936e..35f8f4e6b 100644 --- a/core/mcp/state/fetchRequestLogState.ts +++ b/core/mcp/state/fetchRequestLogState.ts @@ -103,6 +103,15 @@ export class FetchRequestLogState extends TypedEventTarget, ): void => { @@ -156,12 +165,22 @@ export class FetchRequestLogState extends TypedEventTarget e.id)); + const restored = entries.filter((e) => !existingIds.has(e.id)); + if (restored.length === 0) return; + const merged = [...restored, ...this.fetchRequests]; + this.fetchRequests = this.maxFetchRequests > 0 - ? entries.slice(-this.maxFetchRequests) - : entries; - this.fetchRequests = trimmed; + ? merged.slice(-this.maxFetchRequests) + : merged; this.dispatchTypedEvent("fetchRequestsChange", this.getFetchRequests()); }