diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 044e2e961..2903d606f 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 @@ -259,6 +268,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. @@ -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}"`, diff --git a/clients/web/src/utils/oauthFlow.test.ts b/clients/web/src/utils/oauthFlow.test.ts new file mode 100644 index 000000000..9896edd7b --- /dev/null +++ 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); + }); +}); diff --git a/clients/web/src/utils/oauthFlow.ts b/clients/web/src/utils/oauthFlow.ts new file mode 100644 index 000000000..928ccd250 --- /dev/null +++ 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); +}