From 4d893c16ba4b475d84c1ab49ee70454e1e9e844d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 30 May 2026 17:15:34 -0400 Subject: [PATCH 1/6] feat(auth): inject MCP_INSPECTOR_API_TOKEN into served index.html (#1378) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reloading the web client at the bare URL (no `?MCP_INSPECTOR_API_TOKEN=…` query string) with empty sessionStorage made every `/api/*` request 401 — the browser had no way to recover the backend's auth token. Embed the token into `index.html` on every page load so the browser no longer depends on the query string surviving navigation: - New shared helper `clients/web/server/inject-auth-token.ts` embeds `` (escaped against `` injection; no-op when auth is dangerously omitted). - Dev: the Vite plugin injects via `transformIndexHtml`. - Prod: the Hono server injects on the `/` route. - `App.tsx` `getAuthToken()` now reads the injected global first, then the query string, then sessionStorage (both fallbacks preserved). - Shared global name lives in `INSPECTOR_API_TOKEN_GLOBAL` (`core/mcp/remote/constants.ts`). Tests: helper unit coverage + an integration test exercising the real prod server's `/` → `/api/*` flow (injected token authenticates; missing token 401s). AGENTS.md documents the token-recovery order. Co-Authored-By: Claude Opus 4.8 (1M context) --- AGENTS.md | 11 ++ clients/web/server/inject-auth-token.ts | 51 +++++++++ clients/web/server/server.ts | 7 +- clients/web/server/vite-hono-plugin.ts | 17 +++ clients/web/src/App.tsx | 30 +++-- .../server/inject-auth-token.test.ts | 67 ++++++++++++ .../server/server-token-injection.test.ts | 103 ++++++++++++++++++ core/mcp/remote/constants.ts | 11 ++ 8 files changed, 289 insertions(+), 8 deletions(-) create mode 100644 clients/web/server/inject-auth-token.ts create mode 100644 clients/web/src/test/integration/server/inject-auth-token.test.ts create mode 100644 clients/web/src/test/integration/server/server-token-injection.test.ts diff --git a/AGENTS.md b/AGENTS.md index 1c05b22d3..ac743b3e5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,6 +15,7 @@ inspector/ │ │ │ # start-vite-dev-server.ts (in-process Vite starter for the launcher), │ │ │ # web-server-config.ts (env parsing + initial-config payload + banner), │ │ │ # sandbox-controller.ts (MCP Apps sandbox HTTP server), +│ │ │ # inject-auth-token.ts (embeds the API token into served index.html), │ │ │ # vite-base-config.ts (shared optimizeDeps exclusions) │ │ └── static/ # sandbox_proxy.html (served by sandbox-controller for MCP Apps tab) │ ├── cli/ # CLI client @@ -66,6 +67,16 @@ inspector/ * The InspectorClient from v1.5/main will be merged into v2/main, and wired up to the new web Inspector. The TUI and CLI will follow. Eventually when everything works on v2/main we will replace main with v2/main, eliminating the legacy implementations. +## Web backend auth token + +The dev/prod web backend protects every `/api/*` route with `x-mcp-remote-auth: Bearer `. The browser recovers that token from three sources, in priority order (see `App.tsx` `getAuthToken()`): + +1. `window.__INSPECTOR_API_TOKEN__` — injected into `index.html` on every page load by the backend (the dev Vite plugin via `transformIndexHtml`, the prod Hono server on the `/` route), both routed through `clients/web/server/inject-auth-token.ts`. This is what makes a bare-URL reload, a bookmark, or a cleared `sessionStorage` keep working. +2. `?MCP_INSPECTOR_API_TOKEN=…` query string — the URL the launcher banner prints; kept as a fallback for pasted full URLs. +3. `sessionStorage` — backstop for navigations that land without either of the above. + +Injection is a no-op when auth is disabled (`DANGEROUSLY_OMIT_AUTH`), and the global name is the shared `INSPECTOR_API_TOKEN_GLOBAL` constant in `core/mcp/remote/constants.ts`. + ## Maintenance Rules ### Keep documentation files up to date diff --git a/clients/web/server/inject-auth-token.ts b/clients/web/server/inject-auth-token.ts new file mode 100644 index 000000000..c066c0454 --- /dev/null +++ b/clients/web/server/inject-auth-token.ts @@ -0,0 +1,51 @@ +/** + * Embed the remote API auth token into the served `index.html` so the browser + * doesn't depend on the `?MCP_INSPECTOR_API_TOKEN=…` query string surviving + * navigation, bookmarks, or a hand-typed reload at the bare URL. The dev Vite + * plugin (`vite-hono-plugin.ts`) and the prod Hono server (`server.ts`) both + * funnel through this single helper so the injected shape stays identical + * across both backends. `App.tsx`'s `getAuthToken()` reads the embedded global + * ahead of the URL / sessionStorage fallbacks. + */ + +import { INSPECTOR_API_TOKEN_GLOBAL } from "../../../core/mcp/remote/constants.ts"; + +/** + * Serialize the token as a JS string literal safe to drop inside an inline + * `` would otherwise close + * the tag early. The token is normally a hex string, but it can be a + * user-supplied value (`MCP_INSPECTOR_API_TOKEN` env / `--auth-token`), so we + * don't assume it's benign. + */ +function serializeTokenForScript(token: string): string { + return JSON.stringify(token).replace(/window.__INSPECTOR_API_TOKEN__ = "…"` + * tag injected. The script is placed just before `` when present, else + * just before ``, else prepended — in every case it runs before the app + * bundle (which lives further down the document) so the global is set by the + * time `getAuthToken()` reads it. + * + * An empty `token` (auth disabled via `DANGEROUSLY_OMIT_AUTH`) is a no-op: the + * page is returned untouched and no global is defined, matching the banner's + * "no token in the URL" behavior. + */ +export function injectAuthToken(html: string, token: string): string { + if (!token) return html; + const script = ``; + const headClose = html.indexOf(""); + if (headClose !== -1) { + return html.slice(0, headClose) + script + html.slice(headClose); + } + const bodyClose = html.indexOf(""); + if (bodyClose !== -1) { + return html.slice(0, bodyClose) + script + html.slice(bodyClose); + } + return script + html; +} diff --git a/clients/web/server/server.ts b/clients/web/server/server.ts index b0b0e5d5d..0f47a59fb 100644 --- a/clients/web/server/server.ts +++ b/clients/web/server/server.ts @@ -13,6 +13,7 @@ import { serveStatic } from "@hono/node-server/serve-static"; import { Hono } from "hono"; import { createRemoteApp } from "../../../core/mcp/remote/node/server.ts"; import { createSandboxController } from "./sandbox-controller.js"; +import { injectAuthToken } from "./inject-auth-token.js"; import type { WebServerConfig } from "./web-server-config.js"; import { webServerConfigToInitialPayload, @@ -64,7 +65,11 @@ export async function startHonoServer( try { const indexPath = join(rootPath, "index.html"); const html = readFileSync(indexPath, "utf-8"); - return c.html(html); + // Embed the API token so a reload at the bare URL (no + // `?MCP_INSPECTOR_API_TOKEN=…`) still authenticates against /api/*. + // No-op when auth is dangerously omitted (empty token). The dev Vite + // plugin applies the same injection via `transformIndexHtml`. + return c.html(injectAuthToken(html, resolvedAuthToken)); } catch (error) { console.error("Error serving index.html:", error); return c.notFound(); diff --git a/clients/web/server/vite-hono-plugin.ts b/clients/web/server/vite-hono-plugin.ts index 3c4225d47..4fa411ccb 100644 --- a/clients/web/server/vite-hono-plugin.ts +++ b/clients/web/server/vite-hono-plugin.ts @@ -17,6 +17,7 @@ import open from "open"; // spurious "could not resolve" warnings during build. import { createRemoteApp } from "../../../core/mcp/remote/node/server.ts"; import { createSandboxController } from "./sandbox-controller.js"; +import { injectAuthToken } from "./inject-auth-token.js"; import type { WebServerConfig } from "./web-server-config.js"; import { webServerConfigToInitialPayload, @@ -24,8 +25,20 @@ import { } from "./web-server-config.js"; export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { + // Resolved once `configureServer` runs (createRemoteApp generates a token + // when none is supplied). Captured here so the `transformIndexHtml` hook — + // which fires per index.html request, after `configureServer` — can embed it + // into the served page. Stays "" when auth is dangerously omitted or under + // Vitest (where `configureServer` returns early), making injection a no-op. + let resolvedAuthToken = ""; return { name: "hono-api-middleware", + // Embed the API token into the dev-served index.html so a reload at the + // bare URL (no `?MCP_INSPECTOR_API_TOKEN=…`) still authenticates. The + // prod server applies the same injection in `server.ts`. + transformIndexHtml(html) { + return injectAuthToken(html, resolvedAuthToken); + }, // `apply: 'serve'` keeps the plugin out of `vite build`, but Vitest still // instantiates a Vite server in middleware mode (no HTTP server) for // transforms and invokes `configureServer` regardless. Returning early @@ -67,6 +80,10 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { initialConfig: webServerConfigToInitialPayload(config), }); + // Expose the resolved token to `transformIndexHtml`. Left empty when + // auth is dangerously omitted so the page carries no token global. + resolvedAuthToken = config.dangerouslyOmitAuth ? "" : resolvedToken; + // Chain the API close (mcp.json watcher) and the sandbox into the // Vite server's close so dev-server restarts release both resources. const originalClose = server.close.bind(server); diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 9b07497cb..13067ba06 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -17,7 +17,10 @@ import type { ServerEntry, ServerType, } from "@inspector/core/mcp/types.js"; -import { API_SERVER_ENV_VARS } from "@inspector/core/mcp/remote/constants.js"; +import { + API_SERVER_ENV_VARS, + INSPECTOR_API_TOKEN_GLOBAL, +} from "@inspector/core/mcp/remote/constants.js"; import { ManagedToolsState } from "@inspector/core/mcp/state/managedToolsState.js"; import { ManagedPromptsState } from "@inspector/core/mcp/state/managedPromptsState.js"; import { ManagedResourcesState } from "@inspector/core/mcp/state/managedResourcesState.js"; @@ -65,15 +68,28 @@ const redirectUrlProvider: RedirectUrlProvider = { getRedirectUrl: () => `${window.location.origin}/oauth/callback`, }; -// Pull the dev-backend's auth token off the URL the launcher banner prints. -// `npm run dev` opens `http://localhost:6274?MCP_INSPECTOR_API_TOKEN=…`; -// every browser request to /api/* needs the same token in the -// `x-mcp-remote-auth: Bearer …` header or the Hono backend returns 401. -// Persist to sessionStorage so SPA navigations / OAuth round-trips don't -// drop the token from the URL bar. +// Recover the backend's auth token. Every browser request to /api/* needs it +// in the `x-mcp-remote-auth: Bearer …` header or the Hono backend returns 401. +// Three sources, in priority order: +// 1. `window.__INSPECTOR_API_TOKEN__` — injected into index.html by the +// backend on every page load (dev Vite plugin + prod Hono server). This +// is the robust path: it survives a bare-URL reload, a bookmark, or a +// cleared sessionStorage, none of which carry the query string. +// 2. `?MCP_INSPECTOR_API_TOKEN=…` — the URL the launcher banner prints. Kept +// as a fallback for pasted full URLs and older integrations. +// 3. sessionStorage — backstop for SPA navigations / OAuth round-trips that +// land without either of the above. +// The URL value is persisted to sessionStorage so a later navigation that +// drops it from the bar still authenticates. function getAuthToken(): string | undefined { if (typeof window === "undefined") return undefined; const STORAGE_KEY = API_SERVER_ENV_VARS.AUTH_TOKEN; + const fromGlobal = (window as unknown as Record)[ + INSPECTOR_API_TOKEN_GLOBAL + ]; + if (typeof fromGlobal === "string" && fromGlobal) { + return fromGlobal; + } const params = new URLSearchParams(window.location.search); const fromUrl = params.get(API_SERVER_ENV_VARS.AUTH_TOKEN); if (fromUrl) { diff --git a/clients/web/src/test/integration/server/inject-auth-token.test.ts b/clients/web/src/test/integration/server/inject-auth-token.test.ts new file mode 100644 index 000000000..ec2265e68 --- /dev/null +++ b/clients/web/src/test/integration/server/inject-auth-token.test.ts @@ -0,0 +1,67 @@ +import { describe, it, expect } from "vitest"; +import { injectAuthToken } from "../../../../server/inject-auth-token.js"; +import { INSPECTOR_API_TOKEN_GLOBAL } from "../../../../../../core/mcp/remote/constants.js"; + +const TOKEN = "deadbeefcafef00d"; +const scriptFor = (token: string) => + `window.${INSPECTOR_API_TOKEN_GLOBAL} = ${JSON.stringify(token)};`; + +describe("injectAuthToken", () => { + it("injects the token global just before ", () => { + const html = "X"; + const out = injectAuthToken(html, TOKEN); + expect(out).toContain(scriptFor(TOKEN)); + // The script must land inside , ahead of the closing tag. + const scriptIdx = out.indexOf(scriptFor(TOKEN)); + const headCloseIdx = out.indexOf(""); + expect(scriptIdx).toBeLessThan(headCloseIdx); + expect(scriptIdx).toBeGreaterThan(out.indexOf("")); + }); + + it("falls back to before when there is no ", () => { + const html = "
"; + const out = injectAuthToken(html, TOKEN); + const scriptIdx = out.indexOf(scriptFor(TOKEN)); + expect(scriptIdx).toBeGreaterThan(-1); + expect(scriptIdx).toBeLessThan(out.indexOf("")); + }); + + it("prepends when there is neither nor ", () => { + const html = "
"; + const out = injectAuthToken(html, TOKEN); + expect(out.startsWith(``)).toBe(true); + expect(out.endsWith(html)).toBe(true); + }); + + it("returns the html untouched for an empty token (auth disabled)", () => { + const html = ""; + expect(injectAuthToken(html, "")).toBe(html); + }); + + it("escapes a token containing so the tag can't close early", () => { + const evil = "abc"; + const out = injectAuthToken("", evil); + // The raw, unescaped sequence must not survive into the output. + expect(out).not.toContain("`), + ); + expect(match).not.toBeNull(); + const parsed = JSON.parse( + (match as RegExpMatchArray)[1].replace(/\\u003c/g, "<"), + ); + expect(parsed).toBe(TOKEN); + }); +}); diff --git a/clients/web/src/test/integration/server/server-token-injection.test.ts b/clients/web/src/test/integration/server/server-token-injection.test.ts new file mode 100644 index 000000000..c2e4f9b20 --- /dev/null +++ b/clients/web/src/test/integration/server/server-token-injection.test.ts @@ -0,0 +1,103 @@ +import { describe, it, expect, beforeAll, afterAll } from "vitest"; +import { createServer } from "node:net"; +import { mkdtemp, writeFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { startHonoServer } from "../../../../server/server.js"; +import type { WebServerConfig } from "../../../../server/web-server-config.js"; +import type { WebServerHandle } from "../../../../server/types.js"; +import { INSPECTOR_API_TOKEN_GLOBAL } from "../../../../../../core/mcp/remote/constants.js"; + +// Ask the OS for an ephemeral port, then release it for the server to claim. +// There's a vanishingly small reuse window between close and re-bind, but it's +// the standard pattern for "start a real server on a free port" in tests and +// far less flaky than hard-coding one. +async function findFreePort(): Promise { + return new Promise((resolve, reject) => { + const srv = createServer(); + srv.unref(); + srv.on("error", reject); + srv.listen(0, "127.0.0.1", () => { + const addr = srv.address(); + if (addr && typeof addr === "object") { + const { port } = addr; + srv.close(() => resolve(port)); + } else { + srv.close(() => reject(new Error("Could not resolve a free port"))); + } + }); + }); +} + +// Pull the injected token back out of the served index.html the same way the +// browser would — read `window.__INSPECTOR_API_TOKEN__ = "…"`. +function tokenFromHtml(html: string): string | undefined { + const match = html.match( + new RegExp(`window\\.${INSPECTOR_API_TOKEN_GLOBAL} = (.+?);`), + ); + if (!match) return undefined; + return JSON.parse(match[1].replace(/\\u003c/g, "<")) as string; +} + +const TOKEN = "test-injected-token-1234567890"; +const INDEX_HTML = + "Inspector" + + '
'; + +describe("startHonoServer index.html token injection (/ -> /api/*)", () => { + let handle: WebServerHandle; + let baseUrl: string; + let staticRoot: string; + + beforeAll(async () => { + staticRoot = await mkdtemp(join(tmpdir(), "inspector-inject-")); + await writeFile(join(staticRoot, "index.html"), INDEX_HTML, "utf-8"); + + const port = await findFreePort(); + baseUrl = `http://127.0.0.1:${port}`; + const config: WebServerConfig = { + port, + hostname: "127.0.0.1", + authToken: TOKEN, + dangerouslyOmitAuth: false, + initialMcpConfig: null, + storageDir: undefined, + // Allow the same-origin requests the test issues below. + allowedOrigins: [baseUrl], + sandboxPort: 0, + sandboxHost: "127.0.0.1", + logger: undefined, + autoOpen: false, + staticRoot, + }; + handle = await startHonoServer(config); + }); + + afterAll(async () => { + await handle?.close(); + if (staticRoot) await rm(staticRoot, { recursive: true, force: true }); + }); + + it("embeds the auth token in the page served at /", async () => { + const res = await fetch(`${baseUrl}/`); + expect(res.status).toBe(200); + const html = await res.text(); + expect(tokenFromHtml(html)).toBe(TOKEN); + }); + + it("the embedded token authenticates a subsequent /api/* request", async () => { + const indexRes = await fetch(`${baseUrl}/`); + const token = tokenFromHtml(await indexRes.text()); + expect(token).toBe(TOKEN); + + const apiRes = await fetch(`${baseUrl}/api/config`, { + headers: { "x-mcp-remote-auth": `Bearer ${token}` }, + }); + expect(apiRes.status).toBe(200); + }); + + it("rejects an /api/* request that omits the token (proving injection is what unblocks the flow)", async () => { + const apiRes = await fetch(`${baseUrl}/api/config`); + expect(apiRes.status).toBe(401); + }); +}); diff --git a/core/mcp/remote/constants.ts b/core/mcp/remote/constants.ts index 05dbb0137..2cae9cd88 100644 --- a/core/mcp/remote/constants.ts +++ b/core/mcp/remote/constants.ts @@ -12,3 +12,14 @@ export const API_SERVER_ENV_VARS = { */ AUTH_TOKEN: "MCP_INSPECTOR_API_TOKEN", } as const; + +/** + * Name of the global property the web backend injects into `index.html` so the + * browser can recover the API token without depending on the + * `?MCP_INSPECTOR_API_TOKEN=…` query string surviving navigation (or a manual + * reload at the bare URL). The dev Vite plugin and the prod Hono server both + * embed `` on the served + * page; `App.tsx`'s `getAuthToken()` reads it ahead of the URL / sessionStorage + * fallbacks. See `clients/web/server/inject-auth-token.ts`. + */ +export const INSPECTOR_API_TOKEN_GLOBAL = "__INSPECTOR_API_TOKEN__"; From 718aa59b293281358c382f3a0bbf27fe75e8f0f0 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 30 May 2026 17:38:54 -0400 Subject: [PATCH 2/6] feat(auth): wire OAuth authorization-code flow into App.tsx (#1379) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OAuth-protected MCP servers could not be connected to from the v2 web client: the core OAuth pipeline exists, but App.tsx never invoked it, so a connect attempt 401'd and surfaced "Remote send failed (401): … Missing Authorization header" as a toast. Wire the two missing entry points (all core primitives already in place): - Auto-trigger on 401: in onToggleConnection's catch, detect an upstream 401 (isUnauthorizedError) and call client.authenticate(), which runs discovery + DCR (backend-proxied) and redirects the page to the auth server via BrowserNavigation. The initiating server id is persisted to sessionStorage first, since the OAuth `state` carries only mode+authId and the full-page redirect wipes React state. - /oauth/callback handler: a mount effect that, once `servers` hydrate, parses the callback params, recovers the pending server, rebuilds its InspectorClient, runs completeOAuthFlow(code) (PKCE verifier + DCR client info survive in BrowserOAuthStorage), replaceState("/") so a reload can't replay the single-use code, then connect(). An `error=` callback toasts instead of retrying. connect() already attaches the OAuth provider to the transport (inspectorClient.ts), so once tokens land in BrowserOAuthStorage the outbound request carries the bearer token. Extracted the pure pieces (constants + isUnauthorizedError) to src/utils/oauthFlow.ts with unit tests. Verified end-to-end in a real browser against the MCP SDK demo OAuth server: Connect -> redirect -> auto-approve -> callback -> Connected, with the access token shown in the Connection Info modal (#1377). Co-Authored-By: Claude Opus 4.8 (1M context) --- clients/web/src/App.tsx | 115 +++++++++++++++++++++++- clients/web/src/utils/oauthFlow.test.ts | 61 +++++++++++++ clients/web/src/utils/oauthFlow.ts | 41 +++++++++ 3 files changed, 213 insertions(+), 4 deletions(-) create mode 100644 clients/web/src/utils/oauthFlow.test.ts create mode 100644 clients/web/src/utils/oauthFlow.ts diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 13067ba06..94eed112a 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -32,6 +32,10 @@ import { MessageLogState } from "@inspector/core/mcp/state/messageLogState.js"; import { FetchRequestLogState } from "@inspector/core/mcp/state/fetchRequestLogState.js"; import { StderrLogState } from "@inspector/core/mcp/state/stderrLogState.js"; import type { RedirectUrlProvider } from "@inspector/core/auth/index.js"; +import { + parseOAuthCallbackParams, + generateOAuthErrorDescription, +} from "@inspector/core/auth/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"; @@ -59,13 +63,18 @@ import type { OAuthDetails } from "./components/groups/ConnectionInfoContent/Con import { ServerRemoveConfirmModal } from "./components/groups/ServerRemoveConfirmModal/ServerRemoveConfirmModal"; import { buildExportFilename, downloadJsonFile } from "./lib/downloadFile"; import { createWebEnvironment } from "./lib/environmentFactory"; +import { + OAUTH_CALLBACK_PATH, + OAUTH_PENDING_SERVER_KEY, + isUnauthorizedError, +} from "./utils/oauthFlow"; // OAuth redirect URL provider — points at the dev backend's `/oauth/callback` // handler. The InspectorClient only consults this when the active server // requires OAuth; for stdio MCP servers it's never used. Created once and // reused so `BrowserOAuthClientProvider` doesn't re-instantiate per render. const redirectUrlProvider: RedirectUrlProvider = { - getRedirectUrl: () => `${window.location.origin}/oauth/callback`, + getRedirectUrl: () => `${window.location.origin}${OAUTH_CALLBACK_PATH}`, }; // Recover the backend's auth token. Every browser request to /api/* needs it @@ -253,6 +262,11 @@ function App() { const connectStartRef = useRef(undefined); const [latencyMs, setLatencyMs] = useState(undefined); + // One-shot guard for the `/oauth/callback` handler below. The effect waits + // for the async `servers` list to hydrate, so it can run on more than one + // render; this ref ensures the token exchange fires exactly once per load. + const oauthCallbackHandledRef = useRef(false); + // Hook layer. Each hook subscribes to its respective event source and // re-renders the App on change. When `inspectorClient` / state managers // are null, the hooks degrade to empty results. @@ -536,6 +550,73 @@ function App() { ], ); + // Finish the OAuth authorization-code flow when the auth server redirects + // back to `/oauth/callback`. This runs on a fresh page load (the redirect in + // `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. + useEffect(() => { + if (typeof window === "undefined") return; + if (window.location.pathname !== OAUTH_CALLBACK_PATH) return; + if (oauthCallbackHandledRef.current) return; + // `useServers` returns [] until the first fetch resolves; defer until the + // list is populated so `find` can resolve the pending server. + if (servers.length === 0) return; + oauthCallbackHandledRef.current = true; + + const params = parseOAuthCallbackParams(window.location.search); + const pendingId = + window.sessionStorage.getItem(OAUTH_PENDING_SERVER_KEY) ?? undefined; + window.sessionStorage.removeItem(OAUTH_PENDING_SERVER_KEY); + + // Strip the code/state off the URL immediately so a reload can't replay + // the (now single-use) authorization code through the exchange again. + window.history.replaceState({}, "", "/"); + + if (!params.successful) { + notifications.show({ + title: "OAuth authorization failed", + message: generateOAuthErrorDescription(params), + color: "red", + }); + return; + } + + const server = pendingId + ? servers.find((s) => s.id === pendingId) + : undefined; + if (!server) { + notifications.show({ + title: "OAuth callback could not be matched", + message: + "Could not determine which server started the OAuth flow. Please try connecting again.", + color: "red", + }); + return; + } + + void (async () => { + const client = setupClientForServer(server); + setActiveServerId(server.id); + try { + await client.completeOAuthFlow(params.code); + connectStartRef.current = Date.now(); + await client.connect(); + } catch (err) { + connectStartRef.current = undefined; + const message = err instanceof Error ? err.message : String(err); + notifications.show({ + title: `Failed to complete OAuth for "${server.name}"`, + message, + color: "red", + }); + } + })(); + }, [servers, setupClientForServer]); + const onToggleConnection = useCallback( async (id: string) => { // Same server, already connected → disconnect. @@ -571,11 +652,37 @@ function App() { } catch (err) { // Handshake-only. A mid-session transport failure does not throw, // so a future error event from InspectorClient is the right hook - // for surfacing those (TODO(#1323)). For now: toast on the - // handshake error so the user actually sees what went wrong + // for surfacing those (TODO(#1323)). + connectStartRef.current = undefined; + + // A 401 from an OAuth-protected server means we have no (valid) token + // yet. Kick off the authorization-code flow: `authenticate()` runs + // discovery + DCR (proxied through the backend), then redirects the + // whole page to the auth server via `BrowserNavigation`. Persist the + // initiating server id first so the `/oauth/callback` load can resume + // against the right client. The redirect unloads this page, so there's + // nothing to do after the await on the success path. + if (isUnauthorizedError(err)) { + try { + window.sessionStorage.setItem(OAUTH_PENDING_SERVER_KEY, id); + await client.authenticate(); + return; + } catch (authErr) { + window.sessionStorage.removeItem(OAUTH_PENDING_SERVER_KEY); + const message = + authErr instanceof Error ? authErr.message : String(authErr); + notifications.show({ + title: `OAuth authorization failed for "${target.name}"`, + message, + color: "red", + }); + return; + } + } + + // Non-auth handshake error: toast so the user sees what went wrong // instead of the ConnectionToggle silently reverting to // "disconnected". - connectStartRef.current = undefined; const message = err instanceof Error ? err.message : String(err); notifications.show({ title: `Failed to connect to "${target.name}"`, diff --git a/clients/web/src/utils/oauthFlow.test.ts b/clients/web/src/utils/oauthFlow.test.ts new file mode 100644 index 000000000..2c081b262 --- /dev/null +++ b/clients/web/src/utils/oauthFlow.test.ts @@ -0,0 +1,61 @@ +import { describe, it, expect } from "vitest"; +import { + OAUTH_CALLBACK_PATH, + OAUTH_PENDING_SERVER_KEY, + isUnauthorizedError, +} from "./oauthFlow"; + +describe("oauthFlow constants", () => { + it("uses the redirect path the providers expect", () => { + expect(OAUTH_CALLBACK_PATH).toBe("/oauth/callback"); + }); + + it("namespaces the pending-server key", () => { + expect(OAUTH_PENDING_SERVER_KEY).toBe( + "mcp-inspector:oauth-pending-server-id", + ); + }); +}); + +describe("isUnauthorizedError", () => { + it("detects a 401 carried on error.status", () => { + const err = Object.assign(new Error("Remote send failed"), { status: 401 }); + expect(isUnauthorizedError(err)).toBe(true); + }); + + it("detects a 401 carried on error.code", () => { + const err = Object.assign(new Error("boom"), { code: 401 }); + expect(isUnauthorizedError(err)).toBe(true); + }); + + it("detects a 401 formatted into the message when status is lost", () => { + const err = new Error( + 'Remote send failed (401): {"error":"invalid_token","error_description":"Missing Authorization header"}', + ); + expect(isUnauthorizedError(err)).toBe(true); + }); + + it("matches a 401 in a plain (non-Error) thrown value", () => { + expect(isUnauthorizedError("Remote connect failed (401): nope")).toBe(true); + }); + + it("does not treat other status codes as unauthorized", () => { + const err = Object.assign(new Error("Remote send failed (500): boom"), { + status: 500, + }); + expect(isUnauthorizedError(err)).toBe(false); + }); + + it("does not match a bare 401 substring that isn't the formatted status", () => { + // The transport formats the status as "(401)"; a 401 appearing elsewhere + // (e.g. inside an id or token) must not be mistaken for an auth failure. + const err = new Error("tool returned value 40123 for request"); + expect(isUnauthorizedError(err)).toBe(false); + }); + + it("returns false for null / undefined / non-401 primitives", () => { + expect(isUnauthorizedError(null)).toBe(false); + expect(isUnauthorizedError(undefined)).toBe(false); + expect(isUnauthorizedError(42)).toBe(false); + }); +}); diff --git a/clients/web/src/utils/oauthFlow.ts b/clients/web/src/utils/oauthFlow.ts new file mode 100644 index 000000000..5ea9a076a --- /dev/null +++ b/clients/web/src/utils/oauthFlow.ts @@ -0,0 +1,41 @@ +/** + * Shared constants and predicates for the browser-side OAuth authorization-code + * flow wired into `App.tsx`. Extracted here so the pure logic is unit-testable + * independently of the React component that orchestrates it. + */ + +/** + * The pathname the auth server redirects back to after the user authorizes. + * `App.tsx`'s `redirectUrlProvider` points OAuth flows at + * `${origin}${OAUTH_CALLBACK_PATH}`; the mount effect detects this pathname and + * finishes the token exchange. + */ +export const OAUTH_CALLBACK_PATH = "/oauth/callback"; + +/** + * sessionStorage key holding the id of the server whose OAuth flow is in + * flight. The OAuth `state` parameter only carries `{mode}:{authId}`, not which + * configured server initiated the flow, and the full-page redirect to the auth + * server wipes all in-memory React state. The id is stashed here right before + * redirecting so the post-callback page load can rebuild the right + * `InspectorClient` and resume the connection. + */ +export const OAUTH_PENDING_SERVER_KEY = "mcp-inspector:oauth-pending-server-id"; + +/** + * True when a thrown connect error represents an upstream 401. The remote + * transport preserves the status on the error object + * (`remoteClientTransport` sets `error.status`); we also fall back to the + * formatted message (`"… (401) …"`) in case the status is lost crossing an SDK + * boundary. A 401 on connect to an HTTP/SSE server is the signal to start the + * OAuth authorization flow. + */ +export function isUnauthorizedError(err: unknown): boolean { + if (typeof err === "object" && err !== null) { + const status = (err as { status?: number; code?: number }).status; + const code = (err as { code?: number }).code; + if (status === 401 || code === 401) return true; + } + const message = err instanceof Error ? err.message : String(err); + return /\(401\)/.test(message); +} From cc294bae39800c60d7d0faa8b4814b9defc167bf Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 30 May 2026 18:49:12 -0400 Subject: [PATCH 3/6] fix(auth): persist OAuth pre-redirect Network log across the redirect (#1384) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After #1379, the Network tab showed only the post-redirect auth HTTP (discovery re-run + POST /token); the pre-redirect discovery and the DCR POST /register that run during authenticate() were lost when the page navigated to the auth server. Root causes: 1. Ordering — BrowserNavigation set `window.location.href` before the client's `saveSession` event fired (OAuthManager calls onBeforeOAuthRedirect *after* auth() already navigated), so the save raced the unload and was dropped. Fix: BrowserNavigation now runs a synchronous `beforeNavigate` hook immediately before assigning location.href; App wires it through createWebEnvironment to flush the active fetch log to RemoteInspectorClient Storage (keyed by the authId parsed from the auth URL) via a keepalive POST that outlives the unload. 2. Illegal invocation — RemoteInspectorClientStorage defaulted to `this.fetchFn = globalThis.fetch` and called `this.fetchFn(...)`, which re-binds `this` and makes native fetch throw "Illegal invocation" (swallowed by the catch). This silently broke *all* session save/load. Fix: default to a wrapper that preserves the global receiver. 3. Restore race — hydrateFetchRequests replaced the list, so a load that resolved after the resuming connect appended live entries would clobber them. Fix: merge restored (older) entries ahead of live ones, dedupe by id. saveSession also now uses keepalive: true. Verified end-to-end against the MCP SDK demo OAuth server: the connected page's Network tab shows the full handshake — pre-redirect discovery + DCR /register plus post-redirect discovery + /token as `auth`, alongside `transport`. Added unit tests for the beforeNavigate ordering and the hydrate merge/dedupe. Co-Authored-By: Claude Opus 4.8 (1M context) --- clients/web/src/App.tsx | 91 ++++++++++++++++++- clients/web/src/lib/environmentFactory.ts | 8 +- .../web/src/test/core/auth/providers.test.ts | 33 +++++++ .../mcp/state/fetchRequestLogState.test.ts | 67 ++++++++++++++ core/auth/browser/providers.ts | 20 +++- core/mcp/remote/sessionStorage.ts | 16 +++- core/mcp/state/fetchRequestLogState.ts | 18 +++- 7 files changed, 241 insertions(+), 12 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 94eed112a..5baa5714c 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"; @@ -444,11 +446,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). @@ -465,6 +524,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. @@ -513,6 +573,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); @@ -532,7 +596,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; @@ -547,6 +622,8 @@ function App() { messageLogState, fetchRequestLogState, stderrLogState, + sessionStorageAdapter, + onBeforeOAuthRedirect, ], ); @@ -568,6 +645,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); @@ -599,7 +684,7 @@ function App() { } void (async () => { - const client = setupClientForServer(server); + const client = setupClientForServer(server, sessionId); setActiveServerId(server.id); try { await client.completeOAuthFlow(params.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/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..af101d699 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,15 @@ export class RemoteInspectorClientStorage implements InspectorClientStorage { method: "POST", headers, body: JSON.stringify(serializedState), + // `saveSession` is only invoked right before the OAuth full-page + // redirect (via the client's `saveSession` event in + // `onBeforeOAuthRedirect`). Navigation is already scheduled by that + // point, 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. The + // payload is the pre-redirect log only (a few small auth entries, no + // captured bodies), comfortably under keepalive's 64KB cap. + keepalive: true, }); if (!res.ok) { diff --git a/core/mcp/state/fetchRequestLogState.ts b/core/mcp/state/fetchRequestLogState.ts index cfa99936e..032e18f15 100644 --- a/core/mcp/state/fetchRequestLogState.ts +++ b/core/mcp/state/fetchRequestLogState.ts @@ -156,12 +156,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()); } From 025f06af19e6b1d0152c0a2f9f0b2bf5fb328ddb Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 30 May 2026 20:01:28 -0400 Subject: [PATCH 4/6] fix(auth): inject token into prod SPA fallback + prime sessionStorage + no-store (#1378) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses code-review feedback on the token-injection PR: - Prod `server.ts` SPA deep-link fallback (e.g. `/oauth/callback`) previously served the raw index.html off disk via serveStatic, bypassing injection — so a bookmark/reload at a non-`/` route with empty sessionStorage would 401. Route the SPA fallback through the same `serveIndexHtml` (inject) helper; real static assets (paths with a dot) still serve verbatim. Dev already injected on every HTML serve via Vite `transformIndexHtml`. - `getAuthToken()` now persists the injected `window.__INSPECTOR_API_TOKEN__` to sessionStorage (not just the URL-param branch), priming the backstop for any later navigation that loses the global. - Injected HTML responses now send `Cache-Control: no-store`, so a page carrying a token isn't cached and served stale after a restart regenerates the token. Integration tests added: SPA fallback (`/oauth/callback`) carries the token, `Cache-Control: no-store` on injected HTML, real assets served verbatim, and unknown `/api` routes 404 rather than falling through to the HTML shell. Co-Authored-By: Claude Opus 4.8 (1M context) --- clients/web/server/server.ts | 53 ++++++++++++------- clients/web/src/App.tsx | 24 +++++---- .../server/server-token-injection.test.ts | 41 ++++++++++++++ 3 files changed, 90 insertions(+), 28 deletions(-) diff --git a/clients/web/server/server.ts b/clients/web/server/server.ts index 0f47a59fb..4d6aac4c5 100644 --- a/clients/web/server/server.ts +++ b/clients/web/server/server.ts @@ -11,6 +11,7 @@ import open from "open"; import { serve } from "@hono/node-server"; import { serveStatic } from "@hono/node-server/serve-static"; import { Hono } from "hono"; +import type { Context } from "hono"; import { createRemoteApp } from "../../../core/mcp/remote/node/server.ts"; import { createSandboxController } from "./sandbox-controller.js"; import { injectAuthToken } from "./inject-auth-token.js"; @@ -61,33 +62,47 @@ export async function startHonoServer( return apiApp.fetch(c.req.raw); }); + // Serve index.html with the API token injected so a reload at any bare URL + // (no `?MCP_INSPECTOR_API_TOKEN=…`) still authenticates against /api/*. + // No-op when auth is dangerously omitted (empty token). The dev Vite plugin + // applies the same injection via `transformIndexHtml`. `Cache-Control: + // no-store` keeps a browser/proxy from serving a page that carries a stale + // token after a server restart regenerates it (randomBytes per start). + const serveIndexHtml = (c: Context) => { + const indexPath = join(rootPath, "index.html"); + const html = readFileSync(indexPath, "utf-8"); + c.header("Cache-Control", "no-store"); + return c.html(injectAuthToken(html, resolvedAuthToken)); + }; + app.get("/", async (c) => { try { - const indexPath = join(rootPath, "index.html"); - const html = readFileSync(indexPath, "utf-8"); - // Embed the API token so a reload at the bare URL (no - // `?MCP_INSPECTOR_API_TOKEN=…`) still authenticates against /api/*. - // No-op when auth is dangerously omitted (empty token). The dev Vite - // plugin applies the same injection via `transformIndexHtml`. - return c.html(injectAuthToken(html, resolvedAuthToken)); + return serveIndexHtml(c); } catch (error) { console.error("Error serving index.html:", error); return c.notFound(); } }); - app.use( - "/*", - serveStatic({ - root: rootPath, - rewriteRequestPath: (path) => { - if (!path.includes(".") && !path.startsWith("/api")) { - return "/index.html"; - } - return path; - }, - }), - ); + // Real static assets (paths with a file extension). Missing files fall + // through to the SPA fallback below via `next()`. + app.use("/*", serveStatic({ root: rootPath })); + + // SPA deep-link fallback: any non-/api route that didn't resolve to a static + // asset (e.g. `/oauth/callback`, the OAuth landing URL) serves the *injected* + // index.html — not the raw file — so bookmarks and hand-typed reloads at + // those paths get the token global too, matching the `/` route. + app.get("*", async (c) => { + if (c.req.path.startsWith("/api")) { + return c.notFound(); + } + try { + return serveIndexHtml(c); + } catch (error) { + console.error("Error serving index.html:", error); + return c.notFound(); + } + }); const httpServer = serve( { diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 13067ba06..044e2e961 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -79,27 +79,33 @@ const redirectUrlProvider: RedirectUrlProvider = { // as a fallback for pasted full URLs and older integrations. // 3. sessionStorage — backstop for SPA navigations / OAuth round-trips that // land without either of the above. -// The URL value is persisted to sessionStorage so a later navigation that -// drops it from the bar still authenticates. +// Both the injected global and the URL value are persisted to sessionStorage +// so a later navigation that drops them (e.g. a deep-link load that wasn't +// injected, or an iframe) still authenticates from the backstop. function getAuthToken(): string | undefined { if (typeof window === "undefined") return undefined; const STORAGE_KEY = API_SERVER_ENV_VARS.AUTH_TOKEN; + // Best-effort persistence — sessionStorage may be unavailable (privacy + // mode, iframe sandboxing, etc.); the resolved value still works for the + // current page load regardless. + const persist = (token: string): void => { + try { + window.sessionStorage.setItem(STORAGE_KEY, token); + } catch { + // ignore — see note above + } + }; const fromGlobal = (window as unknown as Record)[ INSPECTOR_API_TOKEN_GLOBAL ]; if (typeof fromGlobal === "string" && fromGlobal) { + persist(fromGlobal); return fromGlobal; } const params = new URLSearchParams(window.location.search); const fromUrl = params.get(API_SERVER_ENV_VARS.AUTH_TOKEN); if (fromUrl) { - try { - window.sessionStorage.setItem(STORAGE_KEY, fromUrl); - } catch { - // Best-effort persistence — sessionStorage may be unavailable - // (privacy mode, iframe sandboxing, etc.); the URL value still - // works for the current page load. - } + persist(fromUrl); return fromUrl; } try { diff --git a/clients/web/src/test/integration/server/server-token-injection.test.ts b/clients/web/src/test/integration/server/server-token-injection.test.ts index c2e4f9b20..7e7992f30 100644 --- a/clients/web/src/test/integration/server/server-token-injection.test.ts +++ b/clients/web/src/test/integration/server/server-token-injection.test.ts @@ -44,6 +44,8 @@ const INDEX_HTML = "Inspector" + '
'; +const ASSET_JS = 'console.log("static asset");\n'; + describe("startHonoServer index.html token injection (/ -> /api/*)", () => { let handle: WebServerHandle; let baseUrl: string; @@ -52,6 +54,9 @@ describe("startHonoServer index.html token injection (/ -> /api/*)", () => { beforeAll(async () => { staticRoot = await mkdtemp(join(tmpdir(), "inspector-inject-")); await writeFile(join(staticRoot, "index.html"), INDEX_HTML, "utf-8"); + // A real static asset (path has a file extension) to prove serveStatic + // still serves files verbatim rather than routing them through injection. + await writeFile(join(staticRoot, "asset.js"), ASSET_JS, "utf-8"); const port = await findFreePort(); baseUrl = `http://127.0.0.1:${port}`; @@ -100,4 +105,40 @@ describe("startHonoServer index.html token injection (/ -> /api/*)", () => { const apiRes = await fetch(`${baseUrl}/api/config`); expect(apiRes.status).toBe(401); }); + + it("injects the token into the SPA deep-link fallback (e.g. /oauth/callback)", async () => { + // A non-/api path with no file extension resolves to the SPA fallback, + // which must serve the *injected* index.html — not a raw file — so a + // bookmark or reload at the OAuth callback URL still authenticates. + const res = await fetch(`${baseUrl}/oauth/callback?code=abc&state=xyz`); + expect(res.status).toBe(200); + const html = await res.text(); + expect(tokenFromHtml(html)).toBe(TOKEN); + }); + + it("sets Cache-Control: no-store on injected HTML so a restart's new token isn't served stale", async () => { + const root = await fetch(`${baseUrl}/`); + expect(root.headers.get("cache-control")).toBe("no-store"); + const fallback = await fetch(`${baseUrl}/oauth/callback`); + expect(fallback.headers.get("cache-control")).toBe("no-store"); + }); + + it("serves real static assets verbatim (no token injection)", async () => { + const res = await fetch(`${baseUrl}/asset.js`); + expect(res.status).toBe(200); + const body = await res.text(); + expect(body).toBe(ASSET_JS); + expect(body).not.toContain(INSPECTOR_API_TOKEN_GLOBAL); + }); + + it("does not route /api paths through the SPA fallback", async () => { + // An unknown /api route must 404 as JSON-less notFound, never the HTML + // shell (which would mask real API errors behind a 200 page). + const res = await fetch(`${baseUrl}/api/does-not-exist`, { + headers: { "x-mcp-remote-auth": `Bearer ${TOKEN}` }, + }); + expect(res.status).toBe(404); + const body = await res.text(); + expect(body).not.toContain(INSPECTOR_API_TOKEN_GLOBAL); + }); }); From b5de744cf2c4e4279915b4b0bea5568b19955e38 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 30 May 2026 20:14:49 -0400 Subject: [PATCH 5/6] =?UTF-8?q?fix(auth):=20address=20#1383=20review=20?= =?UTF-8?q?=E2=80=94=20split=20OAuth/connect=20toasts,=20tighten=20401=20m?= =?UTF-8?q?atch=20(#1379)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-review feedback on the OAuth-wiring PR: - Callback effect: split completeOAuthFlow vs connect() into separate try/catch blocks. A token-exchange failure now reads "OAuth token exchange failed … Please try connecting again." (the single-use code is spent and the URL was cleared, so a reload can't retry); a post-OAuth connect failure reads "Failed to connect" since OAuth actually succeeded and re-clicking Connect reuses the persisted tokens. - isUnauthorizedError: anchor the message fallback on the transport's `failed …(401)` wording instead of a bare `(401)`, so an unrelated `(401)` spliced into an error message can't trip the OAuth flow. Added a test. - Documented that clearing the pending id + URL before the server lookup is intentional (deleted/renamed server mid-flow → require a fresh Connect). Also merges the squash-merged #1382 base from v2/main. Co-Authored-By: Claude Opus 4.8 (1M context) --- clients/web/src/App.tsx | 24 +++++++++++++++++++++++- clients/web/src/utils/oauthFlow.test.ts | 7 +++++++ clients/web/src/utils/oauthFlow.ts | 13 ++++++++----- 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 392ddcc39..2903d606f 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -591,6 +591,10 @@ function App() { return; } + // By design, the pending id and URL are cleared above before this lookup: + // if the server was deleted/renamed (e.g. in another tab) mid-flow, there's + // nothing to resume against, so we surface the error and require a fresh + // Connect rather than leaving stale callback state lying around. const server = pendingId ? servers.find((s) => s.id === pendingId) : undefined; @@ -607,15 +611,33 @@ function App() { void (async () => { const client = setupClientForServer(server); 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 + // is spent and the URL was already cleared, a reload can't retry, so we + // tell the user to start over. A failure in the subsequent connect() + // means OAuth DID complete (tokens are persisted): it's a transport + // problem, and re-clicking Connect reuses the saved tokens (no second + // authorization). Conflating them would mislead the user into + // re-authorizing when they don't need to. try { await client.completeOAuthFlow(params.code); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + notifications.show({ + title: `OAuth token exchange failed for "${server.name}"`, + message: `${message}\n\nPlease try connecting again.`, + color: "red", + }); + return; + } + try { connectStartRef.current = Date.now(); await client.connect(); } catch (err) { connectStartRef.current = undefined; const message = err instanceof Error ? err.message : String(err); notifications.show({ - title: `Failed to complete OAuth for "${server.name}"`, + title: `Failed to connect to "${server.name}"`, message, color: "red", }); diff --git a/clients/web/src/utils/oauthFlow.test.ts b/clients/web/src/utils/oauthFlow.test.ts index 2c081b262..9896edd7b 100644 --- a/clients/web/src/utils/oauthFlow.test.ts +++ b/clients/web/src/utils/oauthFlow.test.ts @@ -53,6 +53,13 @@ describe("isUnauthorizedError", () => { expect(isUnauthorizedError(err)).toBe(false); }); + it("does not match a stray '(401)' that isn't the transport's 'failed' wording", () => { + // A tool result or unrelated message embedding `(401)` without the + // transport's `failed …(401)` shape must not trip the OAuth flow. + const err = new Error("tool output: response code (401) seen upstream"); + expect(isUnauthorizedError(err)).toBe(false); + }); + it("returns false for null / undefined / non-401 primitives", () => { expect(isUnauthorizedError(null)).toBe(false); expect(isUnauthorizedError(undefined)).toBe(false); diff --git a/clients/web/src/utils/oauthFlow.ts b/clients/web/src/utils/oauthFlow.ts index 5ea9a076a..928ccd250 100644 --- a/clients/web/src/utils/oauthFlow.ts +++ b/clients/web/src/utils/oauthFlow.ts @@ -25,10 +25,13 @@ export const OAUTH_PENDING_SERVER_KEY = "mcp-inspector:oauth-pending-server-id"; /** * True when a thrown connect error represents an upstream 401. The remote * transport preserves the status on the error object - * (`remoteClientTransport` sets `error.status`); we also fall back to the - * formatted message (`"… (401) …"`) in case the status is lost crossing an SDK - * boundary. A 401 on connect to an HTTP/SSE server is the signal to start the - * OAuth authorization flow. + * (`remoteClientTransport` sets `error.status`); this structured check is the + * primary path. As a fallback for cases where the status is lost crossing an + * SDK boundary, we match the transport's own formatted wording — + * `"Remote (connect|send|events stream) failed (401): …"` — anchored on + * `failed …(401)` rather than a bare `(401)`, so an unrelated `(401)` spliced + * into an error message (e.g. from a tool result) can't trip the OAuth flow. + * A 401 on connect to an HTTP/SSE server is the signal to start OAuth. */ export function isUnauthorizedError(err: unknown): boolean { if (typeof err === "object" && err !== null) { @@ -37,5 +40,5 @@ export function isUnauthorizedError(err: unknown): boolean { if (status === 401 || code === 401) return true; } const message = err instanceof Error ? err.message : String(err); - return /\(401\)/.test(message); + return /\bfailed\b[^\n]*\(401\)/i.test(message); } From 89c06ecbf54fd37b2db5e5c6f636fcd621424e9e Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 30 May 2026 20:30:41 -0400 Subject: [PATCH 6/6] =?UTF-8?q?docs(auth):=20address=20#1385=20review=20?= =?UTF-8?q?=E2=80=94=20clarify=20double-save=20+=20keepalive=20cap=20+=20t?= =?UTF-8?q?est=20fetch=20default=20(#1384)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-review feedback on the OAuth Network-log persistence PR: - Documented the double-save: `FetchRequestLogState`'s `saveSession` listener is the backstop; `BrowserNavigation`'s `beforeNavigate` hook is the primary flush for the redirect case. Notes the listener may lose the navigation race and is harmless when it duplicates (last-writer-wins, identical payload). - Reworded the keepalive comment in `RemoteInspectorClientStorage.saveSession`: the 64KB cap is general (the method is also reachable from the listener with the full session log), so a long session could exceed it and drop silently — acceptable since the persisted log is best-effort, not load-bearing. - Added a regression test that constructs `RemoteInspectorClientStorage` without a `fetchFn`, stubs `globalThis.fetch`, and asserts the default wrapper calls it (locks in the "Illegal invocation" fix, which callers otherwise swallow). Optional items (logger.warn on swallowed save errors; setupClientForServer dep churn) acknowledged on the PR, not changed. Also merges the squash-merged #1383 base from v2/main. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/mcp/remote/sessionStorage.test.ts | 24 +++++++++++++++++++ core/mcp/remote/sessionStorage.ts | 16 +++++++------ core/mcp/state/fetchRequestLogState.ts | 9 +++++++ 3 files changed, 42 insertions(+), 7 deletions(-) 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/core/mcp/remote/sessionStorage.ts b/core/mcp/remote/sessionStorage.ts index af101d699..9c24ee59c 100644 --- a/core/mcp/remote/sessionStorage.ts +++ b/core/mcp/remote/sessionStorage.ts @@ -74,14 +74,16 @@ export class RemoteInspectorClientStorage implements InspectorClientStorage { method: "POST", headers, body: JSON.stringify(serializedState), - // `saveSession` is only invoked right before the OAuth full-page - // redirect (via the client's `saveSession` event in - // `onBeforeOAuthRedirect`). Navigation is already scheduled by that - // point, so a normal fetch would be cancelled mid-flight and the + // `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. The - // payload is the pre-redirect log only (a few small auth entries, no - // captured bodies), comfortably under keepalive's 64KB cap. + // `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, }); diff --git a/core/mcp/state/fetchRequestLogState.ts b/core/mcp/state/fetchRequestLogState.ts index 032e18f15..35f8f4e6b 100644 --- a/core/mcp/state/fetchRequestLogState.ts +++ b/core/mcp/state/fetchRequestLogState.ts @@ -103,6 +103,15 @@ export class FetchRequestLogState extends TypedEventTarget, ): void => {