feat(auth): wire OAuth authorization-code flow into App.tsx (#1379) (#1383)

* feat(auth): inject MCP_INSPECTOR_API_TOKEN into served index.html (#1378) 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 `<script>window.__INSPECTOR_API_TOKEN__ = "…"</script>` (escaped against `</script>` 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) <noreply@anthropic.com> * feat(auth): wire OAuth authorization-code flow into App.tsx (#1379) 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) <noreply@anthropic.com> * fix(auth): inject token into prod SPA fallback + prime sessionStorage + no-store (#1378) 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) <noreply@anthropic.com> * fix(auth): address #1383 review — split OAuth/connect toasts, tighten 401 match (#1379) 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-05-30 20:26:42 -04:00 · 2026-05-30 20:26:42 -04:00 · 9622e8149f
commit 9622e8149f
parent 881ba4b5e8
3 changed files with 245 additions and 4 deletions
--- 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
@ -259,6 +268,11 @@ function App() {
  const connectStartRef = useRef<number | undefined>(undefined);
  const [latencyMs, setLatencyMs] = useState<number | undefined>(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.
@ -542,6 +556,95 @@ 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;
+    }
+
+    // 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;
+    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);
+      // 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 connect to "${server.name}"`,
+          message,
+          color: "red",
+        });
+      }
+    })();
+  }, [servers, setupClientForServer]);
+
  const onToggleConnection = useCallback(
    async (id: string) => {
      // Same server, already connected → disconnect.
@ -577,11 +680,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}"`,
--- a/clients/web/src/utils/oauthFlow.test.ts
+++ b/clients/web/src/utils/oauthFlow.test.ts
@ -0,0 +1,68 @@
+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("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);
+    expect(isUnauthorizedError(42)).toBe(false);
+  });
+});
--- a/clients/web/src/utils/oauthFlow.ts
+++ b/clients/web/src/utils/oauthFlow.ts
@ -0,0 +1,44 @@
+/**
+ * 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`); 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) {
+    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 /\bfailed\b[^\n]*\(401\)/i.test(message);
+}