From 4d893c16ba4b475d84c1ab49ee70454e1e9e844d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 30 May 2026 17:15:34 -0400 Subject: [PATCH 01/12] 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 02/12] 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 03/12] 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 33ae753a49dcde898f3eca0a543897d1c9716e57 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 30 May 2026 19:34:05 -0400 Subject: [PATCH 04/12] feat(network): show auth response bodies with masked secrets + reveal toggle (#1386) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Auth-category Network entries showed request body/headers/status but never the response body (rendered "(empty)"), because buildEffectiveAuthFetch deliberately skipped capturing it to avoid surfacing access_token / refresh_token. That hid the most useful thing to inspect when debugging OAuth — the token exchange. Capture auth response bodies, but mask sensitive OAuth fields by default behind a click-to-reveal toggle so they aren't exposed at a glance during a screen-share: - inspectorClient: wire updateResponseBody on the auth fetcher. - src/utils/maskSecrets.ts: maskSecretsInBody() masks access_token, refresh_token, id_token, client_secret (case-insensitive, nested) in JSON bodies; reports whether anything was masked. Non-JSON / secret-free bodies pass through untouched. - NetworkEntry BodyPreview: when a body has masked fields, render it masked with a Reveal/Hide toggle (copy honors the shown view). Masking is a UI concern; the raw entry is unchanged so reveal shows the real value. access_token / refresh_token live in the post-redirect /token response, which is never written to the session-restore files (#1384); only pre-redirect bodies (public discovery, DCR /register) persist, so no bearer token hits disk. Verified end-to-end: the /token response shows masked by default (access_token: "••••••••"), Reveal exposes the raw value, and discovery / the public DCR /register response (no secret) render with no toggle. Added util + component unit tests and a story play function. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../NetworkEntry/NetworkEntry.stories.tsx | 23 ++++- .../groups/NetworkEntry/NetworkEntry.test.tsx | 40 +++++++++ .../groups/NetworkEntry/NetworkEntry.tsx | 35 +++++++- clients/web/src/utils/maskSecrets.test.ts | 85 +++++++++++++++++++ clients/web/src/utils/maskSecrets.ts | 78 +++++++++++++++++ core/mcp/inspectorClient.ts | 16 ++-- 6 files changed, 268 insertions(+), 9 deletions(-) create mode 100644 clients/web/src/utils/maskSecrets.test.ts create mode 100644 clients/web/src/utils/maskSecrets.ts diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx index e056c23b2..16b8657ea 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx @@ -1,4 +1,5 @@ import type { Meta, StoryObj } from "@storybook/react-vite"; +import { expect, userEvent, within } from "storybook/test"; import type { FetchRequestEntry } from "../../../../../../core/mcp/types.js"; import { NetworkEntry } from "./NetworkEntry"; @@ -38,7 +39,13 @@ const authEntry: FetchRequestEntry = { responseStatus: 200, responseStatusText: "OK", responseHeaders: { "content-type": "application/json" }, - responseBody: '{"access_token":"x","token_type":"bearer"}', + responseBody: JSON.stringify({ + access_token: "eyJhbGciOiJSUzI1NiwidHlwIjoiSldUIn0.payload.sig", + token_type: "Bearer", + expires_in: 3600, + refresh_token: "rt_9f8e7d6c5b4a", + scope: "mcp:tools", + }), duration: 120, category: "auth", }; @@ -99,6 +106,20 @@ export const TransportSuccessExpanded: Story = { export const AuthSuccess: Story = { args: { entry: authEntry, isListExpanded: true }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + // Token-response secrets are masked by default — the raw token must not be + // visible until the user explicitly reveals it. + await expect(canvas.getByText("Secrets hidden")).toBeInTheDocument(); + await expect(canvasElement.textContent).not.toContain("eyJhbGciOiJSUzI1"); + await expect(canvasElement.textContent).toContain("••••••••"); + // Non-secret fields stay visible. + await expect(canvasElement.textContent).toContain("Bearer"); + + await userEvent.click(canvas.getByRole("button", { name: "Reveal" })); + await expect(canvas.getByText("Secrets revealed")).toBeInTheDocument(); + await expect(canvasElement.textContent).toContain("eyJhbGciOiJSUzI1"); + }, }; export const HttpError: Story = { diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx index 6dc1381a2..2e8030c5e 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx @@ -196,4 +196,44 @@ describe("NetworkEntry", () => { await user.click(screen.getByRole("button", { name: "Expand" })); expect(screen.getByText(/Body too large to preview/)).toBeInTheDocument(); }); + + it("masks token-response secrets until revealed, then shows the raw value", async () => { + const user = userEvent.setup(); + const authEntry: FetchRequestEntry = { + ...baseEntry, + category: "auth", + url: "http://localhost:3001/token", + requestBody: undefined, + responseBody: JSON.stringify({ + access_token: "super-secret-token", + token_type: "Bearer", + }), + }; + const { container } = renderWithMantine( + , + ); + // Masked by default: the reveal affordance is present and the raw secret + // is nowhere in the DOM, but non-secret fields still render. + expect(screen.getByText("Secrets hidden")).toBeInTheDocument(); + expect(container.textContent).not.toContain("super-secret-token"); + expect(container.textContent).toContain("••••••••"); + expect(container.textContent).toContain("Bearer"); + + await user.click(screen.getByRole("button", { name: "Reveal" })); + + expect(screen.getByText("Secrets revealed")).toBeInTheDocument(); + expect(container.textContent).toContain("super-secret-token"); + + // Toggling back re-masks. + await user.click(screen.getByRole("button", { name: "Hide" })); + expect(container.textContent).not.toContain("super-secret-token"); + }); + + it("does not add a reveal toggle for non-secret bodies", () => { + renderWithMantine(); + expect(screen.queryByText("Secrets hidden")).not.toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: "Reveal" }), + ).not.toBeInTheDocument(); + }); }); diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx index b7040832d..cf0f5a7a4 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx @@ -13,6 +13,7 @@ import { import type { FetchRequestEntry } from "@inspector/core/mcp/types.js"; import { isLongLivedStreamResponse } from "@inspector/core/mcp/fetchTracking.js"; import { ContentViewer } from "../../elements/ContentViewer/ContentViewer"; +import { maskSecretsInBody } from "../../../utils/maskSecrets"; export interface NetworkEntryProps { entry: FetchRequestEntry; @@ -126,7 +127,16 @@ function HeadersTable({ headers }: { headers: Record }) { ); } +const RevealButton = Button.withProps({ + variant: "subtle", + size: "compact-xs", +}); + function BodyPreview({ body }: { body: string }) { + // Reveal state for masked secrets. Hooks run before any early return so the + // order stays stable across the too-large / has-secrets branches. + const [revealed, setRevealed] = useState(false); + const tooLarge = body.length > MAX_INLINE_BODY_CHARS; if (tooLarge) { return ( @@ -135,7 +145,30 @@ function BodyPreview({ body }: { body: string }) { ); } - return ; + + // OAuth responses (token exchange, DCR) carry bearer-grade secrets. Mask + // them by default and gate the raw values behind an explicit reveal so they + // aren't exposed at a glance during a screen-share. Bodies without secrets + // render as-is with no toggle. + const { masked, hasSecrets } = maskSecretsInBody(body); + if (!hasSecrets) { + return ; + } + + const shown = revealed ? body : masked; + return ( + + + + {revealed ? "Secrets revealed" : "Secrets hidden"} + + setRevealed((v) => !v)}> + {revealed ? "Hide" : "Reveal"} + + + + + ); } export function NetworkEntry({ entry, isListExpanded }: NetworkEntryProps) { diff --git a/clients/web/src/utils/maskSecrets.test.ts b/clients/web/src/utils/maskSecrets.test.ts new file mode 100644 index 000000000..1c4353fa4 --- /dev/null +++ b/clients/web/src/utils/maskSecrets.test.ts @@ -0,0 +1,85 @@ +import { describe, it, expect } from "vitest"; +import { maskSecretsInBody, MASK_PLACEHOLDER } from "./maskSecrets"; + +describe("maskSecretsInBody", () => { + it("masks token fields in a token-exchange response and flags secrets", () => { + const body = JSON.stringify({ + access_token: "abc.def.ghi", + token_type: "Bearer", + expires_in: 3600, + refresh_token: "r3fr3sh", + scope: "mcp:tools", + }); + const { masked, hasSecrets } = maskSecretsInBody(body); + expect(hasSecrets).toBe(true); + const parsed = JSON.parse(masked); + expect(parsed.access_token).toBe(MASK_PLACEHOLDER); + expect(parsed.refresh_token).toBe(MASK_PLACEHOLDER); + // Non-secret fields pass through untouched. + expect(parsed.token_type).toBe("Bearer"); + expect(parsed.expires_in).toBe(3600); + expect(parsed.scope).toBe("mcp:tools"); + // The raw secret never appears in the masked output. + expect(masked).not.toContain("abc.def.ghi"); + expect(masked).not.toContain("r3fr3sh"); + }); + + it("masks id_token and client_secret (case-insensitive keys)", () => { + const body = JSON.stringify({ + ID_Token: "jwt", + Client_Secret: "shhh", + client_id: "public-123", + }); + const { masked, hasSecrets } = maskSecretsInBody(body); + expect(hasSecrets).toBe(true); + const parsed = JSON.parse(masked); + expect(parsed.ID_Token).toBe(MASK_PLACEHOLDER); + expect(parsed.Client_Secret).toBe(MASK_PLACEHOLDER); + // client_id is not a secret. + expect(parsed.client_id).toBe("public-123"); + }); + + it("masks secrets nested in objects and arrays", () => { + const body = JSON.stringify({ + data: { tokens: [{ access_token: "deep" }] }, + }); + const { masked, hasSecrets } = maskSecretsInBody(body); + expect(hasSecrets).toBe(true); + expect(JSON.parse(masked).data.tokens[0].access_token).toBe( + MASK_PLACEHOLDER, + ); + }); + + it("reports no secrets (and leaves content intact) for discovery metadata", () => { + const body = JSON.stringify({ + issuer: "http://localhost:3001/", + authorization_endpoint: "http://localhost:3001/authorize", + scopes_supported: ["mcp:tools"], + }); + const { masked, hasSecrets } = maskSecretsInBody(body); + expect(hasSecrets).toBe(false); + expect(JSON.parse(masked).authorization_endpoint).toBe( + "http://localhost:3001/authorize", + ); + }); + + it("does not flag an empty-string secret value", () => { + const { hasSecrets } = maskSecretsInBody( + JSON.stringify({ access_token: "" }), + ); + expect(hasSecrets).toBe(false); + }); + + it("returns non-JSON bodies unchanged with no secrets", () => { + const body = "code=abc&code_verifier=xyz"; // form-encoded, not JSON + const { masked, hasSecrets } = maskSecretsInBody(body); + expect(hasSecrets).toBe(false); + expect(masked).toBe(body); + }); + + it("does not treat pure reformatting as masking", () => { + // Minified JSON with no secret keys → reserialized but hasSecrets false. + const { hasSecrets } = maskSecretsInBody('{"a":1,"b":[2,3]}'); + expect(hasSecrets).toBe(false); + }); +}); diff --git a/clients/web/src/utils/maskSecrets.ts b/clients/web/src/utils/maskSecrets.ts new file mode 100644 index 000000000..9b12a5b6a --- /dev/null +++ b/clients/web/src/utils/maskSecrets.ts @@ -0,0 +1,78 @@ +/** + * Masks sensitive OAuth values inside a captured HTTP body for display in the + * Network tab. OAuth token-exchange and registration responses carry + * credentials (`access_token`, `refresh_token`, …); we show the body so it's + * inspectable, but mask those values by default so they aren't exposed at a + * glance during a screen-share. The raw body is preserved by the caller and + * shown only when the user explicitly reveals it. + */ + +// JSON keys whose values are bearer-grade secrets. Matched case-insensitively. +// `client_secret` covers DCR responses for confidential clients; the token +// fields cover the `/token` exchange and refresh responses. +const SENSITIVE_KEYS = new Set([ + "access_token", + "refresh_token", + "id_token", + "client_secret", +]); + +// What a masked value is replaced with. A fixed-width dotted string keeps the +// shape recognizable as "a value was here" without hinting at its length. +export const MASK_PLACEHOLDER = "••••••••"; + +function isSensitiveKey(key: string): boolean { + return SENSITIVE_KEYS.has(key.toLowerCase()); +} + +function maskValue(key: string, value: unknown): unknown { + if (isSensitiveKey(key) && typeof value === "string" && value.length > 0) { + return MASK_PLACEHOLDER; + } + return maskNode(value); +} + +function maskNode(node: unknown): unknown { + if (Array.isArray(node)) { + return node.map((item) => maskNode(item)); + } + if (node !== null && typeof node === "object") { + const out: Record = {}; + for (const [key, value] of Object.entries( + node as Record, + )) { + out[key] = maskValue(key, value); + } + return out; + } + return node; +} + +export interface MaskResult { + /** The body with sensitive values replaced; pretty-printed when JSON. */ + masked: string; + /** True when at least one sensitive value was masked. */ + hasSecrets: boolean; +} + +/** + * Mask sensitive fields in a JSON body. Non-JSON bodies (or JSON without any + * sensitive keys) are returned unchanged with `hasSecrets: false`, so callers + * can skip the reveal affordance entirely. The masked JSON is re-serialized + * pretty-printed; the caller keeps the original string for the revealed view. + */ +export function maskSecretsInBody(body: string): MaskResult { + let parsed: unknown; + try { + parsed = JSON.parse(body); + } catch { + return { masked: body, hasSecrets: false }; + } + const maskedNode = maskNode(parsed); + const masked = JSON.stringify(maskedNode, null, 2); + // The serialized output differs from the input only when something was + // masked (whitespace-only reformatting aside), but compare the structures + // directly so reformatting alone never trips the "has secrets" flag. + const hasSecrets = JSON.stringify(maskedNode) !== JSON.stringify(parsed); + return { masked, hasSecrets }; +} diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index e0f2254ce..d68b2a6df 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -315,16 +315,18 @@ export class InspectorClient extends InspectorClientEventTarget { private buildEffectiveAuthFetch(): typeof fetch { const base = this.fetchFn ?? fetch; - // Note: we deliberately do NOT wire `updateResponseBody` for the auth - // fetcher. OAuth token-exchange responses contain `access_token` and - // `refresh_token`; capturing them into FetchRequestLogState would - // surface live credentials in the Network tab body preview, which is - // easy to leak during a screen-share. Headers + status are still - // tracked. If a future need calls for inspecting auth bodies, add - // explicit secret redaction first. + // Capture auth response bodies (OAuth discovery, DCR, token exchange) so + // they're inspectable in the Network tab. Token-exchange responses carry + // `access_token` / `refresh_token`; the Network UI masks those (and other + // known secret fields) behind a click-to-reveal toggle so they aren't + // surfaced at a glance during a screen-share. Masking is a display + // concern, kept in the UI layer rather than mutating the captured entry, + // so the raw body stays available for the user who explicitly reveals it. return createFetchTracker(base, { trackRequest: (entry) => this.dispatchFetchRequest({ ...entry, category: "auth" }), + updateResponseBody: (id, body) => + this.dispatchFetchRequestBodyUpdate(id, body), }); } From 025f06af19e6b1d0152c0a2f9f0b2bf5fb328ddb Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 30 May 2026 20:01:28 -0400 Subject: [PATCH 05/12] 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 06/12] =?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 07/12] =?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 => { From 88aa58393ea7151a72631e4db6e4039b3dc61507 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 30 May 2026 20:48:04 -0400 Subject: [PATCH 08/12] =?UTF-8?q?feat(network):=20address=20#1387=20review?= =?UTF-8?q?=20=E2=80=94=20form-encoded=20masking,=20cleaner=20flag,=20a11y?= =?UTF-8?q?=20(#1386)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-review feedback on the auth-response-body masking PR: - Extended masking to form-urlencoded bodies (finding #1): the token *request* is `application/x-www-form-urlencoded` and carries `code` / `code_verifier` / `client_secret` / `refresh_token`. `maskSecretsInBody` now masks those in form bodies too (preserving formatting; placeholder not percent-encoded). `code`/`code_verifier`/`client_assertion` are form-only sensitive keys — deliberately NOT masked in JSON, where `code` is usually an error/status code. - Replaced the double-stringify `hasSecrets` heuristic with an explicit masked-flag propagated out of `maskNode` (finding #2) — robust if the transform ever grows non-identity behavior, and one fewer serialization. - Reset reveal state on body change via `key={body}` remount instead of a setState-in-effect (finding #3; avoids the cascading-render lint rule). - a11y (finding #4): `aria-label` on the Reveal/Hide button and `aria-live` on the hidden/revealed status text. - Security tripwire comment near the `saveSession` listener (finding #6): captured auth bodies are unmasked at source (masking is UI-only), so any new post-token-exchange persistence path must redact first. Tests: form-encoded masking (token + refresh requests), JSON `code` NOT masked, non-object JSON pass-through, default-fetch wrapper. Story play + NetworkEntry tests updated for the new aria-labels and the now-masked form request body. Finding #5 (pretty-print asymmetry) is already handled by ContentViewer's JSON formatting; #4-aria and #6 are the doc/a11y bits. Also merges the squash-merged #1385 base from v2/main. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../NetworkEntry/NetworkEntry.stories.tsx | 20 ++- .../groups/NetworkEntry/NetworkEntry.test.tsx | 10 +- .../groups/NetworkEntry/NetworkEntry.tsx | 29 ++-- clients/web/src/utils/maskSecrets.test.ts | 45 ++++++- clients/web/src/utils/maskSecrets.ts | 125 +++++++++++++----- core/mcp/state/fetchRequestLogState.ts | 9 ++ 6 files changed, 184 insertions(+), 54 deletions(-) diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx index 16b8657ea..700489d92 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx @@ -108,16 +108,26 @@ export const AuthSuccess: Story = { args: { entry: authEntry, isListExpanded: true }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - // Token-response secrets are masked by default — the raw token must not be - // visible until the user explicitly reveals it. - await expect(canvas.getByText("Secrets hidden")).toBeInTheDocument(); + // Both bodies carry secrets: the form request (`code=…`) and the JSON + // response (`access_token`). Both are masked by default — the raw values + // must not be visible until explicitly revealed. + const hidden = canvas.getAllByText("Secrets hidden"); + await expect(hidden.length).toBeGreaterThanOrEqual(2); await expect(canvasElement.textContent).not.toContain("eyJhbGciOiJSUzI1"); await expect(canvasElement.textContent).toContain("••••••••"); // Non-secret fields stay visible. await expect(canvasElement.textContent).toContain("Bearer"); - await userEvent.click(canvas.getByRole("button", { name: "Reveal" })); - await expect(canvas.getByText("Secrets revealed")).toBeInTheDocument(); + // Reveal every masked body and confirm the raw response token appears. + const revealButtons = canvas.getAllByRole("button", { + name: "Reveal secrets in body", + }); + for (const button of revealButtons) { + await userEvent.click(button); + } + await expect(canvas.getAllByText("Secrets revealed").length).toBe( + revealButtons.length, + ); await expect(canvasElement.textContent).toContain("eyJhbGciOiJSUzI1"); }, }; diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx index 2e8030c5e..e2caaf70e 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx @@ -219,13 +219,17 @@ describe("NetworkEntry", () => { expect(container.textContent).toContain("••••••••"); expect(container.textContent).toContain("Bearer"); - await user.click(screen.getByRole("button", { name: "Reveal" })); + await user.click( + screen.getByRole("button", { name: "Reveal secrets in body" }), + ); expect(screen.getByText("Secrets revealed")).toBeInTheDocument(); expect(container.textContent).toContain("super-secret-token"); // Toggling back re-masks. - await user.click(screen.getByRole("button", { name: "Hide" })); + await user.click( + screen.getByRole("button", { name: "Hide secrets in body" }), + ); expect(container.textContent).not.toContain("super-secret-token"); }); @@ -233,7 +237,7 @@ describe("NetworkEntry", () => { renderWithMantine(); expect(screen.queryByText("Secrets hidden")).not.toBeInTheDocument(); expect( - screen.queryByRole("button", { name: "Reveal" }), + screen.queryByRole("button", { name: "Reveal secrets in body" }), ).not.toBeInTheDocument(); }); }); diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx index cf0f5a7a4..8b34b4ef5 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx @@ -134,7 +134,10 @@ const RevealButton = Button.withProps({ function BodyPreview({ body }: { body: string }) { // Reveal state for masked secrets. Hooks run before any early return so the - // order stays stable across the too-large / has-secrets branches. + // order stays stable across the too-large / has-secrets branches. The reveal + // state resets when the body content changes because callers key + // `` by `body` (remounting on swap), so a previously-revealed + // view never persists across a content change. const [revealed, setRevealed] = useState(false); const tooLarge = body.length > MAX_INLINE_BODY_CHARS; @@ -146,10 +149,10 @@ function BodyPreview({ body }: { body: string }) { ); } - // OAuth responses (token exchange, DCR) carry bearer-grade secrets. Mask - // them by default and gate the raw values behind an explicit reveal so they - // aren't exposed at a glance during a screen-share. Bodies without secrets - // render as-is with no toggle. + // OAuth responses (token exchange, DCR) and the token request carry + // bearer-grade secrets. Mask them by default and gate the raw values behind + // an explicit reveal so they aren't exposed at a glance during a + // screen-share. Bodies without secrets render as-is with no toggle. const { masked, hasSecrets } = maskSecretsInBody(body); if (!hasSecrets) { return ; @@ -159,10 +162,15 @@ function BodyPreview({ body }: { body: string }) { return ( - + {revealed ? "Secrets revealed" : "Secrets hidden"} - setRevealed((v) => !v)}> + setRevealed((v) => !v)} + aria-label={ + revealed ? "Hide secrets in body" : "Reveal secrets in body" + } + > {revealed ? "Hide" : "Reveal"} @@ -225,7 +233,7 @@ export function NetworkEntry({ entry, isListExpanded }: NetworkEntryProps) { Request Body - + )} {entry.responseHeaders && ( @@ -242,7 +250,10 @@ export function NetworkEntry({ entry, isListExpanded }: NetworkEntryProps) { Response Body {entry.responseBody ? ( - + ) : ( {isLongLivedStream(entry) diff --git a/clients/web/src/utils/maskSecrets.test.ts b/clients/web/src/utils/maskSecrets.test.ts index 1c4353fa4..a1c7b270c 100644 --- a/clients/web/src/utils/maskSecrets.test.ts +++ b/clients/web/src/utils/maskSecrets.test.ts @@ -70,16 +70,57 @@ describe("maskSecretsInBody", () => { expect(hasSecrets).toBe(false); }); - it("returns non-JSON bodies unchanged with no secrets", () => { - const body = "code=abc&code_verifier=xyz"; // form-encoded, not JSON + it("returns a form body with no sensitive params unchanged", () => { + const body = "grant_type=authorization_code&redirect_uri=http://x/cb"; const { masked, hasSecrets } = maskSecretsInBody(body); expect(hasSecrets).toBe(false); expect(masked).toBe(body); }); + it("masks sensitive params in a form-encoded token request, preserving other params", () => { + const body = + "grant_type=authorization_code&code=AUTHCODE&code_verifier=VERIFIER&client_id=public-1&redirect_uri=http://x/cb"; + const { masked, hasSecrets } = maskSecretsInBody(body); + expect(hasSecrets).toBe(true); + expect(masked).toContain(`code=${MASK_PLACEHOLDER}`); + expect(masked).toContain(`code_verifier=${MASK_PLACEHOLDER}`); + expect(masked).not.toContain("AUTHCODE"); + expect(masked).not.toContain("VERIFIER"); + // Non-secret params are untouched (and formatting preserved). + expect(masked).toContain("grant_type=authorization_code"); + expect(masked).toContain("client_id=public-1"); + expect(masked).toContain("redirect_uri=http://x/cb"); + }); + + it("masks refresh_token and client_secret in a form-encoded refresh request", () => { + const body = + "grant_type=refresh_token&refresh_token=RTVAL&client_secret=CSVAL"; + const { masked, hasSecrets } = maskSecretsInBody(body); + expect(hasSecrets).toBe(true); + expect(masked).not.toContain("RTVAL"); + expect(masked).not.toContain("CSVAL"); + }); + + it("does NOT mask a JSON `code` field (e.g. a JSON-RPC error code)", () => { + // `code` is form-only sensitive; in JSON it's usually an error/status code. + const { masked, hasSecrets } = maskSecretsInBody( + JSON.stringify({ code: "some-string-code", message: "boom" }), + ); + expect(hasSecrets).toBe(false); + expect(JSON.parse(masked).code).toBe("some-string-code"); + }); + it("does not treat pure reformatting as masking", () => { // Minified JSON with no secret keys → reserialized but hasSecrets false. const { hasSecrets } = maskSecretsInBody('{"a":1,"b":[2,3]}'); expect(hasSecrets).toBe(false); }); + + it("passes through valid-but-non-object JSON (string / number / null) untouched", () => { + for (const raw of ['"abc"', "42", "null", "true"]) { + const { masked, hasSecrets } = maskSecretsInBody(raw); + expect(hasSecrets).toBe(false); + expect(JSON.parse(masked)).toEqual(JSON.parse(raw)); + } + }); }); diff --git a/clients/web/src/utils/maskSecrets.ts b/clients/web/src/utils/maskSecrets.ts index 9b12a5b6a..28fc032cb 100644 --- a/clients/web/src/utils/maskSecrets.ts +++ b/clients/web/src/utils/maskSecrets.ts @@ -1,51 +1,110 @@ /** * Masks sensitive OAuth values inside a captured HTTP body for display in the - * Network tab. OAuth token-exchange and registration responses carry - * credentials (`access_token`, `refresh_token`, …); we show the body so it's - * inspectable, but mask those values by default so they aren't exposed at a - * glance during a screen-share. The raw body is preserved by the caller and - * shown only when the user explicitly reveals it. + * Network tab. OAuth token-exchange / registration responses carry credentials + * (`access_token`, `refresh_token`, …) and the token *request* (a + * `application/x-www-form-urlencoded` body) carries `code` / `code_verifier` / + * `client_secret`. We show the body so it's inspectable, but mask those values + * by default so they aren't exposed at a glance during a screen-share. The raw + * body is preserved by the caller and shown only when the user reveals it. + * + * Both JSON and form-encoded bodies are handled; anything else (or a body with + * no sensitive keys) passes through unchanged with `hasSecrets: false`. */ -// JSON keys whose values are bearer-grade secrets. Matched case-insensitively. -// `client_secret` covers DCR responses for confidential clients; the token -// fields cover the `/token` exchange and refresh responses. -const SENSITIVE_KEYS = new Set([ +// Keys masked in JSON bodies — bearer-grade secrets only. `code` is +// deliberately NOT here: a JSON body's `code` is usually something else (e.g. +// a JSON-RPC error `code`), and we don't want to mask those. +const JSON_SENSITIVE_KEYS = new Set([ "access_token", "refresh_token", "id_token", "client_secret", ]); +// Keys masked in form-encoded bodies — the JSON set plus the single-use OAuth +// request material that only appears as form params (authorization code, PKCE +// verifier, private-key-JWT client assertion). +const FORM_SENSITIVE_KEYS = new Set([ + ...JSON_SENSITIVE_KEYS, + "code", + "code_verifier", + "client_assertion", +]); + // What a masked value is replaced with. A fixed-width dotted string keeps the // shape recognizable as "a value was here" without hinting at its length. export const MASK_PLACEHOLDER = "••••••••"; -function isSensitiveKey(key: string): boolean { - return SENSITIVE_KEYS.has(key.toLowerCase()); +interface MaskedNode { + node: unknown; + masked: boolean; } -function maskValue(key: string, value: unknown): unknown { - if (isSensitiveKey(key) && typeof value === "string" && value.length > 0) { - return MASK_PLACEHOLDER; - } - return maskNode(value); -} - -function maskNode(node: unknown): unknown { +// Recursively mask sensitive string values in a parsed JSON node, tracking +// whether anything was masked (so the caller never has to infer it by +// comparing serializations — reformatting alone can't trip the flag, and it's +// robust if this function ever grows non-identity transforms). +function maskNode(node: unknown): MaskedNode { if (Array.isArray(node)) { - return node.map((item) => maskNode(item)); + let masked = false; + const out = node.map((item) => { + const r = maskNode(item); + masked = masked || r.masked; + return r.node; + }); + return { node: out, masked }; } if (node !== null && typeof node === "object") { + let masked = false; const out: Record = {}; for (const [key, value] of Object.entries( node as Record, )) { - out[key] = maskValue(key, value); + if ( + JSON_SENSITIVE_KEYS.has(key.toLowerCase()) && + typeof value === "string" && + value.length > 0 + ) { + out[key] = MASK_PLACEHOLDER; + masked = true; + } else { + const r = maskNode(value); + out[key] = r.node; + masked = masked || r.masked; + } } - return out; + return { node: out, masked }; } - return node; + return { node, masked: false }; +} + +// Mask sensitive params in a form-urlencoded body, preserving the original +// formatting (we only swap the value, so the placeholder isn't percent-encoded +// the way `URLSearchParams.toString()` would mangle it). A non-form string +// (no `key=value` pairs with a sensitive key) falls through untouched. +function maskFormBody(body: string): MaskResult { + let hasSecrets = false; + const masked = body + .split("&") + .map((pair) => { + const eq = pair.indexOf("="); + if (eq === -1) return pair; + const rawKey = pair.slice(0, eq); + const value = pair.slice(eq + 1); + let key: string; + try { + key = decodeURIComponent(rawKey); + } catch { + key = rawKey; + } + if (FORM_SENSITIVE_KEYS.has(key.toLowerCase()) && value.length > 0) { + hasSecrets = true; + return `${rawKey}=${MASK_PLACEHOLDER}`; + } + return pair; + }) + .join("&"); + return { masked: hasSecrets ? masked : body, hasSecrets }; } export interface MaskResult { @@ -56,23 +115,19 @@ export interface MaskResult { } /** - * Mask sensitive fields in a JSON body. Non-JSON bodies (or JSON without any - * sensitive keys) are returned unchanged with `hasSecrets: false`, so callers - * can skip the reveal affordance entirely. The masked JSON is re-serialized - * pretty-printed; the caller keeps the original string for the revealed view. + * Mask sensitive fields in an HTTP body for display. JSON bodies are masked + * structurally (and re-serialized pretty-printed); non-JSON bodies are treated + * as form-encoded. Bodies with no sensitive keys return unchanged with + * `hasSecrets: false` so callers can skip the reveal affordance. The caller + * keeps the original string for the revealed view. */ export function maskSecretsInBody(body: string): MaskResult { let parsed: unknown; try { parsed = JSON.parse(body); } catch { - return { masked: body, hasSecrets: false }; + return maskFormBody(body); } - const maskedNode = maskNode(parsed); - const masked = JSON.stringify(maskedNode, null, 2); - // The serialized output differs from the input only when something was - // masked (whitespace-only reformatting aside), but compare the structures - // directly so reformatting alone never trips the "has secrets" flag. - const hasSecrets = JSON.stringify(maskedNode) !== JSON.stringify(parsed); - return { masked, hasSecrets }; + const { node, masked } = maskNode(parsed); + return { masked: JSON.stringify(node, null, 2), hasSecrets: masked }; } diff --git a/core/mcp/state/fetchRequestLogState.ts b/core/mcp/state/fetchRequestLogState.ts index 35f8f4e6b..daaf2b863 100644 --- a/core/mcp/state/fetchRequestLogState.ts +++ b/core/mcp/state/fetchRequestLogState.ts @@ -112,6 +112,15 @@ export class FetchRequestLogState extends TypedEventTarget, ): void => { From e4998308c673d35706bc14a165504ac88aa6b96d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 30 May 2026 22:16:03 -0400 Subject: [PATCH 09/12] =?UTF-8?q?refactor(network):=20address=20#1387=20th?= =?UTF-8?q?ird-pass=20review=20=E2=80=94=20content-type=20masking,=20whole?= =?UTF-8?q?sale=20mask,=20dedup=20(#1386)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third-pass code-review feedback on the auth-body masking PR: - maskSecretsInBody now takes the body's content-type (finding #1): `*json*` → JSON masking, form-urlencoded → form masking, any other known type (HTML/plaintext/XML) → no masking; absent/unknown → sniff as before. Removes the implicit "non-JSON ⇒ form" guess for error pages etc. NetworkEntry passes the request/response `content-type` header through to BodyPreview. - Mask any non-empty value under a sensitive key wholesale (finding #2): a non-standard object/array value under e.g. `access_token` is replaced rather than recursed into, so it can't leak. Empty-string still not flagged. - Extracted `isSensitiveKey(set, key)` to dedupe the JSON/form key checks (finding #3). - Reworded `MaskResult.masked` doc to cover the form (non-pretty-printed) case (finding #6). Tests: non-string-value-under-sensitive-key wholesale mask; repeated form param; empty form value not flagged; content-type honored (JSON type skips form masking, text/plain skips masking, explicit form type masks). Finding #5 (story re-hide) skipped per reviewer — unit test already covers re-mask. Finding #4 (form edge cases) added. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../groups/NetworkEntry/NetworkEntry.tsx | 21 +++- clients/web/src/utils/maskSecrets.test.ts | 55 +++++++++++ clients/web/src/utils/maskSecrets.ts | 96 +++++++++++++------ 3 files changed, 139 insertions(+), 33 deletions(-) diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx index 8b34b4ef5..e44cd88fd 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx @@ -132,7 +132,13 @@ const RevealButton = Button.withProps({ size: "compact-xs", }); -function BodyPreview({ body }: { body: string }) { +function BodyPreview({ + body, + contentType, +}: { + body: string; + contentType?: string; +}) { // Reveal state for masked secrets. Hooks run before any early return so the // order stays stable across the too-large / has-secrets branches. The reveal // state resets when the body content changes because callers key @@ -152,8 +158,10 @@ function BodyPreview({ body }: { body: string }) { // OAuth responses (token exchange, DCR) and the token request carry // bearer-grade secrets. Mask them by default and gate the raw values behind // an explicit reveal so they aren't exposed at a glance during a - // screen-share. Bodies without secrets render as-is with no toggle. - const { masked, hasSecrets } = maskSecretsInBody(body); + // screen-share. The entry's content-type scopes which parser runs (so a + // plaintext/HTML error body is never guessed at). Bodies without secrets + // render as-is with no toggle. + const { masked, hasSecrets } = maskSecretsInBody(body, contentType); if (!hasSecrets) { return ; } @@ -233,7 +241,11 @@ export function NetworkEntry({ entry, isListExpanded }: NetworkEntryProps) { Request Body - + )} {entry.responseHeaders && ( @@ -253,6 +265,7 @@ export function NetworkEntry({ entry, isListExpanded }: NetworkEntryProps) { ) : ( diff --git a/clients/web/src/utils/maskSecrets.test.ts b/clients/web/src/utils/maskSecrets.test.ts index a1c7b270c..9e293ba41 100644 --- a/clients/web/src/utils/maskSecrets.test.ts +++ b/clients/web/src/utils/maskSecrets.test.ts @@ -50,6 +50,19 @@ describe("maskSecretsInBody", () => { ); }); + it("masks a non-string value under a sensitive key wholesale (no leak via recursion)", () => { + // A non-standard server wrapping the token in an object: the value must be + // replaced wholesale rather than recursed into (where the inner `value` + // key isn't sensitive and would otherwise leak). + const body = JSON.stringify({ + access_token: { value: "leaky-secret", expires_in: 3600 }, + }); + const { masked, hasSecrets } = maskSecretsInBody(body); + expect(hasSecrets).toBe(true); + expect(masked).not.toContain("leaky-secret"); + expect(JSON.parse(masked).access_token).toBe(MASK_PLACEHOLDER); + }); + it("reports no secrets (and leaves content intact) for discovery metadata", () => { const body = JSON.stringify({ issuer: "http://localhost:3001/", @@ -123,4 +136,46 @@ describe("maskSecretsInBody", () => { expect(JSON.parse(masked)).toEqual(JSON.parse(raw)); } }); + + it("masks every occurrence of a repeated sensitive form param", () => { + const { masked, hasSecrets } = maskSecretsInBody("code=AAA&code=BBB"); + expect(hasSecrets).toBe(true); + expect(masked).toBe(`code=${MASK_PLACEHOLDER}&code=${MASK_PLACEHOLDER}`); + }); + + it("does not flag an empty form param value", () => { + const { masked, hasSecrets } = maskSecretsInBody( + "grant_type=refresh_token&client_secret=", + ); + expect(hasSecrets).toBe(false); + expect(masked).toBe("grant_type=refresh_token&client_secret="); + }); + + it("honors an explicit content-type: form params under a JSON type are not form-masked", () => { + // `code` is form-only sensitive; with a JSON content-type the body goes + // through the JSON parser (fails to parse → untouched), not form masking. + const { hasSecrets } = maskSecretsInBody( + "code=AAA&code_verifier=BBB", + "application/json", + ); + expect(hasSecrets).toBe(false); + }); + + it("skips masking for a known non-JSON, non-form content-type", () => { + // A plaintext/HTML error body that happens to contain `access_token=…` + // must not be guessed at as form-encoded when the content-type says text. + const body = "Error: access_token=leaked has expired"; + const { masked, hasSecrets } = maskSecretsInBody(body, "text/plain"); + expect(hasSecrets).toBe(false); + expect(masked).toBe(body); + }); + + it("uses the form parser for an explicit form content-type", () => { + const { masked, hasSecrets } = maskSecretsInBody( + "refresh_token=RT", + "application/x-www-form-urlencoded", + ); + expect(hasSecrets).toBe(true); + expect(masked).toBe(`refresh_token=${MASK_PLACEHOLDER}`); + }); }); diff --git a/clients/web/src/utils/maskSecrets.ts b/clients/web/src/utils/maskSecrets.ts index 28fc032cb..5b1dd9285 100644 --- a/clients/web/src/utils/maskSecrets.ts +++ b/clients/web/src/utils/maskSecrets.ts @@ -7,8 +7,10 @@ * by default so they aren't exposed at a glance during a screen-share. The raw * body is preserved by the caller and shown only when the user reveals it. * - * Both JSON and form-encoded bodies are handled; anything else (or a body with - * no sensitive keys) passes through unchanged with `hasSecrets: false`. + * Content-type selects the parser: `*json*` → JSON masking, form-urlencoded → + * form masking, any other known type → no masking. When the content-type is + * absent/unknown the body is sniffed (parse as JSON first, else treat as + * form). See `maskSecretsInBody`. */ // Keys masked in JSON bodies — bearer-grade secrets only. `code` is @@ -35,15 +37,29 @@ const FORM_SENSITIVE_KEYS = new Set([ // shape recognizable as "a value was here" without hinting at its length. export const MASK_PLACEHOLDER = "••••••••"; +function isSensitiveKey(set: ReadonlySet, key: string): boolean { + return set.has(key.toLowerCase()); +} + +// Whether a value under a sensitive key should be masked. Strings are masked +// when non-empty (an empty `access_token` carries nothing); any non-string, +// non-null value (a non-standard object/array/number wrapper) is masked +// wholesale so it can't leak through the recursion under a sensitive key. +function isMaskableValue(value: unknown): boolean { + if (value === null || value === undefined) return false; + if (typeof value === "string") return value.length > 0; + return true; +} + interface MaskedNode { node: unknown; masked: boolean; } -// Recursively mask sensitive string values in a parsed JSON node, tracking -// whether anything was masked (so the caller never has to infer it by -// comparing serializations — reformatting alone can't trip the flag, and it's -// robust if this function ever grows non-identity transforms). +// Recursively mask sensitive values in a parsed JSON node, tracking whether +// anything was masked (so the caller never has to infer it by comparing +// serializations — reformatting alone can't trip the flag, and it's robust if +// this function ever grows non-identity transforms). function maskNode(node: unknown): MaskedNode { if (Array.isArray(node)) { let masked = false; @@ -60,11 +76,7 @@ function maskNode(node: unknown): MaskedNode { for (const [key, value] of Object.entries( node as Record, )) { - if ( - JSON_SENSITIVE_KEYS.has(key.toLowerCase()) && - typeof value === "string" && - value.length > 0 - ) { + if (isSensitiveKey(JSON_SENSITIVE_KEYS, key) && isMaskableValue(value)) { out[key] = MASK_PLACEHOLDER; masked = true; } else { @@ -78,6 +90,24 @@ function maskNode(node: unknown): MaskedNode { return { node, masked: false }; } +export interface MaskResult { + /** The body with sensitive values replaced; pretty-printed for JSON, otherwise the original shape with values substituted. */ + masked: string; + /** True when at least one sensitive value was masked. */ + hasSecrets: boolean; +} + +function maskJsonBody(body: string): MaskResult { + let parsed: unknown; + try { + parsed = JSON.parse(body); + } catch { + return { masked: body, hasSecrets: false }; + } + const { node, masked } = maskNode(parsed); + return { masked: JSON.stringify(node, null, 2), hasSecrets: masked }; +} + // Mask sensitive params in a form-urlencoded body, preserving the original // formatting (we only swap the value, so the placeholder isn't percent-encoded // the way `URLSearchParams.toString()` would mangle it). A non-form string @@ -97,7 +127,7 @@ function maskFormBody(body: string): MaskResult { } catch { key = rawKey; } - if (FORM_SENSITIVE_KEYS.has(key.toLowerCase()) && value.length > 0) { + if (isSensitiveKey(FORM_SENSITIVE_KEYS, key) && value.length > 0) { hasSecrets = true; return `${rawKey}=${MASK_PLACEHOLDER}`; } @@ -107,27 +137,35 @@ function maskFormBody(body: string): MaskResult { return { masked: hasSecrets ? masked : body, hasSecrets }; } -export interface MaskResult { - /** The body with sensitive values replaced; pretty-printed when JSON. */ - masked: string; - /** True when at least one sensitive value was masked. */ - hasSecrets: boolean; -} - /** - * Mask sensitive fields in an HTTP body for display. JSON bodies are masked - * structurally (and re-serialized pretty-printed); non-JSON bodies are treated - * as form-encoded. Bodies with no sensitive keys return unchanged with - * `hasSecrets: false` so callers can skip the reveal affordance. The caller - * keeps the original string for the revealed view. + * Mask sensitive fields in an HTTP body for display. + * + * `contentType` (the body's `content-type` header, if known) picks the parser: + * - `*json*` → JSON masking (re-serialized pretty) + * - `application/x-www-form-urlencoded` → form masking (shape preserved) + * - any other known type (HTML, plaintext, XML, …) → no masking + * - absent/unknown → sniff: parse as JSON, else treat as form + * + * Bodies with no sensitive keys return unchanged with `hasSecrets: false` so + * callers can skip the reveal affordance. The caller keeps the original string + * for the revealed view. */ -export function maskSecretsInBody(body: string): MaskResult { - let parsed: unknown; +export function maskSecretsInBody( + body: string, + contentType?: string, +): MaskResult { + const ct = (contentType ?? "").toLowerCase(); + if (ct) { + if (ct.includes("json")) return maskJsonBody(body); + if (ct.includes("x-www-form-urlencoded")) return maskFormBody(body); + // Known, non-JSON/non-form content type → don't guess; leave it alone. + return { masked: body, hasSecrets: false }; + } + // No content-type hint: sniff. Valid JSON → JSON masking; otherwise form. try { - parsed = JSON.parse(body); + JSON.parse(body); } catch { return maskFormBody(body); } - const { node, masked } = maskNode(parsed); - return { masked: JSON.stringify(node, null, 2), hasSecrets: masked }; + return maskJsonBody(body); } From af7ad0d4201921e0d34ed3ffeaa559ee35699409 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 30 May 2026 22:46:03 -0400 Subject: [PATCH 10/12] =?UTF-8?q?test(network):=20address=20#1387=20fourth?= =?UTF-8?q?-pass=20nits=20=E2=80=94=20content-type=20contract,=20key,=20fo?= =?UTF-8?q?rm=20test=20(#1386)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth-pass review (LGTM) — minor, non-blocking items: - Documented the content-type matching contract on `maskSecretsInBody`: substring match (`*json*` / `*x-www-form-urlencoded*`), and we trust the wire's own label (a mislabeled body renders raw — acceptable for the screen-share threat model) (finding #1). - Key `` by content-type + body (not body alone) so a header-only change would also reset the reveal state (finding #3; not reachable today). - Storybook `AuthSuccess`: assert revealed count `>=` reveal-button count to mirror the `hidden >= 2` check, so adding a non-masked body later can't drift the assertion silently (finding #4). - Added a `NetworkEntry` unit test for form-encoded request-body masking (code/code_verifier) — previously exercised only via the Storybook play. Finding #2 (silent decodeURIComponent fallback on malformed keys) acknowledged as a known quirk outside the threat model; no change. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../NetworkEntry/NetworkEntry.stories.tsx | 6 ++-- .../groups/NetworkEntry/NetworkEntry.test.tsx | 31 +++++++++++++++++++ .../groups/NetworkEntry/NetworkEntry.tsx | 10 +++--- clients/web/src/utils/maskSecrets.ts | 6 ++++ 4 files changed, 45 insertions(+), 8 deletions(-) diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx index 700489d92..6d9f64df5 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.stories.tsx @@ -125,9 +125,9 @@ export const AuthSuccess: Story = { for (const button of revealButtons) { await userEvent.click(button); } - await expect(canvas.getAllByText("Secrets revealed").length).toBe( - revealButtons.length, - ); + await expect( + canvas.getAllByText("Secrets revealed").length, + ).toBeGreaterThanOrEqual(revealButtons.length); await expect(canvasElement.textContent).toContain("eyJhbGciOiJSUzI1"); }, }; diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx index e2caaf70e..514e78e80 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.test.tsx @@ -233,6 +233,37 @@ describe("NetworkEntry", () => { expect(container.textContent).not.toContain("super-secret-token"); }); + it("masks a form-encoded request body (code/verifier) until revealed", async () => { + const user = userEvent.setup(); + const authEntry: FetchRequestEntry = { + ...baseEntry, + category: "auth", + url: "http://localhost:3001/token", + requestHeaders: { "content-type": "application/x-www-form-urlencoded" }, + requestBody: + "grant_type=authorization_code&code=SECRETCODE&code_verifier=SECRETVERIFIER", + responseStatus: undefined, + responseStatusText: undefined, + responseHeaders: undefined, + responseBody: undefined, + }; + const { container } = renderWithMantine( + , + ); + expect(screen.getByText("Secrets hidden")).toBeInTheDocument(); + expect(container.textContent).not.toContain("SECRETCODE"); + expect(container.textContent).not.toContain("SECRETVERIFIER"); + expect(container.textContent).toContain("••••••••"); + // Non-secret param stays visible. + expect(container.textContent).toContain("grant_type=authorization_code"); + + await user.click( + screen.getByRole("button", { name: "Reveal secrets in body" }), + ); + expect(container.textContent).toContain("SECRETCODE"); + expect(container.textContent).toContain("SECRETVERIFIER"); + }); + it("does not add a reveal toggle for non-secret bodies", () => { renderWithMantine(); expect(screen.queryByText("Secrets hidden")).not.toBeInTheDocument(); diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx index e44cd88fd..fad014280 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx @@ -141,9 +141,9 @@ function BodyPreview({ }) { // Reveal state for masked secrets. Hooks run before any early return so the // order stays stable across the too-large / has-secrets branches. The reveal - // state resets when the body content changes because callers key - // `` by `body` (remounting on swap), so a previously-revealed - // view never persists across a content change. + // state resets when the body or its content-type changes because callers key + // `` by both (remounting on swap), so a previously-revealed view + // never persists across a content (or masking) change. const [revealed, setRevealed] = useState(false); const tooLarge = body.length > MAX_INLINE_BODY_CHARS; @@ -242,7 +242,7 @@ export function NetworkEntry({ entry, isListExpanded }: NetworkEntryProps) { Request Body @@ -263,7 +263,7 @@ export function NetworkEntry({ entry, isListExpanded }: NetworkEntryProps) { {entry.responseBody ? ( diff --git a/clients/web/src/utils/maskSecrets.ts b/clients/web/src/utils/maskSecrets.ts index 5b1dd9285..4071c0f76 100644 --- a/clients/web/src/utils/maskSecrets.ts +++ b/clients/web/src/utils/maskSecrets.ts @@ -149,6 +149,12 @@ function maskFormBody(body: string): MaskResult { * Bodies with no sensitive keys return unchanged with `hasSecrets: false` so * callers can skip the reveal affordance. The caller keeps the original string * for the revealed view. + * + * `contentType` is matched by substring (`*json*`, `*x-www-form-urlencoded*`) + * and we trust the wire's own label — a body mislabeled by the server (e.g. + * JSON sent as `text/html`) takes the "no masking" branch and renders raw. + * That's acceptable: the threat model is a screen-share viewer, not an + * adversary who controls the response's content-type. */ export function maskSecretsInBody( body: string, From 2237d2d479a47d01be712cebc6e44785ba5bdb12 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 31 May 2026 02:42:12 -0400 Subject: [PATCH 11/12] =?UTF-8?q?perf(network):=20address=20#1387=20fifth-?= =?UTF-8?q?pass=20=E2=80=94=20memoize=20masking,=20mask=20DCR=20mgmt=20tok?= =?UTF-8?q?en=20(#1386)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fifth-pass review (LGTM): - Memoized `maskSecretsInBody` in `BodyPreview` (recommended item #1): a Reveal/Hide click no longer re-parses + re-walks the body; cost is once per mount, and the `key` remount re-runs it on body/content-type change. Guarded so a too-large body is never parsed (the hook runs unconditionally, with the size check inside the memo). - Added `registration_access_token` (RFC 7592 DCR management credential, same bearer class) to the masked key set, and a confidential-client `/register` response fixture test (#5) asserting client_secret + registration_access_token masked, client_id/metadata visible. - Tightened the `isMaskableValue` doc to state the exact contract: non-null, non-empty-string values are masked wholesale (#3). Items #2 (transport parse cost — capped by the memo) and #4 (test-only MASK_PLACEHOLDER export) left as-is per the review. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../groups/NetworkEntry/NetworkEntry.tsx | 30 ++++++++++++++----- clients/web/src/utils/maskSecrets.test.ts | 24 +++++++++++++++ clients/web/src/utils/maskSecrets.ts | 12 +++++--- 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx index fad014280..7cf8da595 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from "react"; +import { useEffect, useMemo, useState } from "react"; import { Badge, Button, @@ -147,6 +147,27 @@ function BodyPreview({ const [revealed, setRevealed] = useState(false); const tooLarge = body.length > MAX_INLINE_BODY_CHARS; + + // OAuth responses (token exchange, DCR) and the token request carry + // bearer-grade secrets. Mask them by default and gate the raw values behind + // an explicit reveal so they aren't exposed at a glance during a + // screen-share. The entry's content-type scopes which parser runs (so a + // plaintext/HTML error body is never guessed at). Bodies without secrets + // render as-is with no toggle. + // + // Memoized so a Reveal/Hide click (a re-render) doesn't re-parse and re-walk + // the body; the cost is paid once per mount, and the `key={…}` remount on + // body/content-type change re-runs it. Skipped for too-large bodies so we + // never parse something we won't display (the hook must run unconditionally, + // hence the in-memo guard rather than an early return above it). + const { masked, hasSecrets } = useMemo( + () => + tooLarge + ? { masked: body, hasSecrets: false } + : maskSecretsInBody(body, contentType), + [tooLarge, body, contentType], + ); + if (tooLarge) { return ( @@ -155,13 +176,6 @@ function BodyPreview({ ); } - // OAuth responses (token exchange, DCR) and the token request carry - // bearer-grade secrets. Mask them by default and gate the raw values behind - // an explicit reveal so they aren't exposed at a glance during a - // screen-share. The entry's content-type scopes which parser runs (so a - // plaintext/HTML error body is never guessed at). Bodies without secrets - // render as-is with no toggle. - const { masked, hasSecrets } = maskSecretsInBody(body, contentType); if (!hasSecrets) { return ; } diff --git a/clients/web/src/utils/maskSecrets.test.ts b/clients/web/src/utils/maskSecrets.test.ts index 9e293ba41..39c8dac15 100644 --- a/clients/web/src/utils/maskSecrets.test.ts +++ b/clients/web/src/utils/maskSecrets.test.ts @@ -63,6 +63,30 @@ describe("maskSecretsInBody", () => { expect(JSON.parse(masked).access_token).toBe(MASK_PLACEHOLDER); }); + it("masks a confidential-client DCR (/register) response", () => { + // RFC 7591/7592 happy path: client_secret and the registration management + // token are bearer-grade and masked; client_id and metadata stay visible. + const body = JSON.stringify({ + client_id: "dyn-123", + client_secret: "REG-SECRET", + registration_access_token: "REG-RAT", + registration_client_uri: "http://localhost:3001/register/dyn-123", + client_id_issued_at: 1735689600, + }); + const { masked, hasSecrets } = maskSecretsInBody(body); + expect(hasSecrets).toBe(true); + const parsed = JSON.parse(masked); + expect(parsed.client_secret).toBe(MASK_PLACEHOLDER); + expect(parsed.registration_access_token).toBe(MASK_PLACEHOLDER); + expect(masked).not.toContain("REG-SECRET"); + expect(masked).not.toContain("REG-RAT"); + // Non-secret fields untouched. + expect(parsed.client_id).toBe("dyn-123"); + expect(parsed.registration_client_uri).toBe( + "http://localhost:3001/register/dyn-123", + ); + }); + it("reports no secrets (and leaves content intact) for discovery metadata", () => { const body = JSON.stringify({ issuer: "http://localhost:3001/", diff --git a/clients/web/src/utils/maskSecrets.ts b/clients/web/src/utils/maskSecrets.ts index 4071c0f76..3fd4d9a5d 100644 --- a/clients/web/src/utils/maskSecrets.ts +++ b/clients/web/src/utils/maskSecrets.ts @@ -16,11 +16,14 @@ // Keys masked in JSON bodies — bearer-grade secrets only. `code` is // deliberately NOT here: a JSON body's `code` is usually something else (e.g. // a JSON-RPC error `code`), and we don't want to mask those. +// `registration_access_token` is the DCR management credential (RFC 7592), +// same bearer class as `access_token`. const JSON_SENSITIVE_KEYS = new Set([ "access_token", "refresh_token", "id_token", "client_secret", + "registration_access_token", ]); // Keys masked in form-encoded bodies — the JSON set plus the single-use OAuth @@ -41,10 +44,11 @@ function isSensitiveKey(set: ReadonlySet, key: string): boolean { return set.has(key.toLowerCase()); } -// Whether a value under a sensitive key should be masked. Strings are masked -// when non-empty (an empty `access_token` carries nothing); any non-string, -// non-null value (a non-standard object/array/number wrapper) is masked -// wholesale so it can't leak through the recursion under a sensitive key. +// Whether a value under a sensitive key should be masked. The contract is +// "any non-null, non-empty-string value": strings are masked when non-empty +// (an empty `access_token` carries nothing), and any non-string value +// (object/array/number/boolean wrapper — pathological for OAuth, but a safe +// default) is masked wholesale so it can't leak through the recursion. function isMaskableValue(value: unknown): boolean { if (value === null || value === undefined) return false; if (typeof value === "string") return value.length > 0; From 06fcae0e9e50e874c5938989f2372e732b543897 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 31 May 2026 08:08:53 -0400 Subject: [PATCH 12/12] docs(network): clarify body-update emits list event only (#1387 sixth pass) (#1386) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sixth-pass review (LGTM, all nits). Added a comment on `onFetchRequestBodyUpdate` noting it re-emits only `fetchRequestsChange`, not the per-entry `fetchRequest` event — list-reading consumers pick up the body on the next render; a future per-entry subscriber would need its own event. Other items left as-is per the review: full-string `BodyPreview` key (#1, micro-perf only if profiled), NaN under a sensitive key (#3, consistent with the mask-non-null-non-empty contract), `&`-only form separator (#4, OAuth never emits `;`). Co-Authored-By: Claude Opus 4.8 (1M context) --- core/mcp/state/fetchRequestLogState.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/core/mcp/state/fetchRequestLogState.ts b/core/mcp/state/fetchRequestLogState.ts index daaf2b863..9761a0d83 100644 --- a/core/mcp/state/fetchRequestLogState.ts +++ b/core/mcp/state/fetchRequestLogState.ts @@ -92,6 +92,11 @@ export class FetchRequestLogState extends TypedEventTarget