From 4d893c16ba4b475d84c1ab49ee70454e1e9e844d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 30 May 2026 17:15:34 -0400 Subject: [PATCH 1/4] 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/4] 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 025f06af19e6b1d0152c0a2f9f0b2bf5fb328ddb Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 30 May 2026 20:01:28 -0400 Subject: [PATCH 3/4] 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 4/4] =?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); }