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

[cliffhall]

Closes #1379.

Stacked on #1382 (#1378). Base is v2/auto-inject-api-token so the diff shows only the OAuth wiring. Rebase onto v2/main once #1382 merges. (Local OAuth testing needs #1378's token injection, which is why this stacks.)

Problem

OAuth-protected MCP servers couldn't be connected to from the v2 web client. The full OAuth pipeline already exists in core/, but App.tsx never invoked it — so a connect attempt 401'd and surfaced the toast the reporter saw:

Failed to connect to "ts-test-oauth-server"
Remote send failed (401): {"error":"invalid_token","error_description":"Missing Authorization header"}

Change

All the core primitives (authenticate(), completeOAuthFlow(), BrowserOAuthStorage, BrowserNavigation, backend-proxied OAuth fetches) were already in place; this wires the two missing entry points in App.tsx:

1. Auto-trigger on 401 — in onToggleConnection's catch, isUnauthorizedError(err) detects an upstream 401 and calls client.authenticate(), which runs discovery + DCR (proxied through the backend) and redirects the page to the auth server via BrowserNavigation. The initiating server id is persisted to sessionStorage first, because the OAuth state carries only {mode}:{authId} and the full-page redirect wipes React state.
2. /oauth/callback handler — a mount effect that, once servers hydrate, parses the callback params, recovers the pending server, rebuilds its InspectorClient, runs completeOAuthFlow(code) (the 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, so once tokens land in BrowserOAuthStorage the outbound request carries the bearer token.

The pure pieces (constants + isUnauthorizedError) are extracted to src/utils/oauthFlow.ts with unit tests; the React orchestration stays in App.tsx.

Acceptance criteria

• Connect on an OAuth-protected server (no saved tokens) completes the flow end-to-end → Connected.
• Connection Info modal (feat(connection-info): wire Connection Info modal to the connection handshake #1377) shows the OAuth section populated (access token).
• error= callback surfaces a toast instead of silently retrying.
• Reload at the bare origin after auth doesn't re-trigger the exchange (URL replaced).
• Existing non-OAuth servers (stdio, plain HTTP) connect with no behavior change (401-only branch).

Testing

Verified end-to-end in a real browser against the MCP TypeScript SDK demo OAuth server (resource :3000, auth server :3001, DCR): Connect → redirect → auto-approve → /oauth/callback → Connected (82ms), with the access token shown in the Connection Info modal and the URL cleaned back to /.

npm run validate (1851 unit tests + coverage gate), npm run test:integration (491), and npm run test:storybook (322) all green.

Out of scope (per issue)

Guided OAuth step-through UI, explicit "Sign in"/"Sign out" affordances, DCR corner cases (#571).

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Wires the existing OAuth authorization-code primitives into the web client's App.tsx: a 401 on connect now auto-triggers client.authenticate() and a new mount effect handles the /oauth/callback page load to exchange the code and reconnect. Pure helpers (OAUTH_CALLBACK_PATH, OAUTH_PENDING_SERVER_KEY, isUnauthorizedError) are extracted into utils/oauthFlow.ts with unit tests, while the React orchestration stays in App.tsx.

Changes:

• Add utils/oauthFlow.ts with the callback path constant, the sessionStorage key for the pending server id, and a robust isUnauthorizedError predicate (covers err.status, err.code, and (401) in the message).
• In App.tsx's onToggleConnection connect-catch: detect 401, persist the initiating server id to sessionStorage, call client.authenticate() (full-page redirect); toast on auth failure.
• Add a mount useEffect that, once servers hydrate, parses /oauth/callback params, recovers the pending server, runs completeOAuthFlow(code) then connect(), and cleans the URL with replaceState("/"); toasts on error= or unmatched server, guarded by a one-shot ref.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
clients/web/src/utils/oauthFlow.ts	New shared constants + isUnauthorizedError helper for the App-level OAuth wiring.
clients/web/src/utils/oauthFlow.test.ts	Unit tests covering the constants and the various 401-detection branches.
clients/web/src/App.tsx	Imports the new helpers, replaces the inline /oauth/callback literal, adds the callback useEffect, and extends the connect-catch with the 401 → authenticate() path.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[cliffhall]

Server Connected

Connection Info

Tools Working

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 9s —— View job

---

Code Review

Solid, well-documented wiring. The extraction of pure logic into oauthFlow.ts with focused tests is the right shape, and the comments explain why (PKCE survives in BrowserOAuthStorage, full-page redirect wipes React state, single-use code → replaceState("/") before async work). The 401-only catch branch correctly leaves stdio/plain-HTTP behavior untouched. End-to-end screenshots confirm the happy path.

A few suggestions, mostly nits — none are blockers.

---

1. Toast message conflates two failure modes — App.tsx:601-616

try {
  await client.completeOAuthFlow(params.code);
  connectStartRef.current = Date.now();
  await client.connect();
} catch (err) {
  // …
  title: `Failed to complete OAuth for "${server.name}"`,

If completeOAuthFlow succeeds but connect() then fails (network blip, server restarted, etc.), the user sees "Failed to complete OAuth" — but OAuth actually completed and the tokens are persisted. A retry by clicking Connect again will skip OAuth entirely and just connect. Worth splitting so the toast title matches what actually failed:

try {
  await client.completeOAuthFlow(params.code);
} catch (err) {
  // toast: "OAuth token exchange failed"
  return;
}
try {
  connectStartRef.current = Date.now();
  await client.connect();
} catch (err) {
  connectStartRef.current = undefined;
  // toast: `Failed to connect to "${server.name}"`
}

Fix this →

2. isUnauthorizedError regex can theoretically false-positive — oauthFlow.ts:40

/\(401\)/.test(message) is a fine fallback for the transport's formatted "... (401): ..." pattern, and the structured err.status === 401 check fires first so this rarely matters. But an error whose message happens to embed (401) from a different context (e.g., a tool result spliced into a connect error message) would trip the OAuth flow. Realistic risk is low; if you want a tighter match, anchor to the transport's wording, e.g. /\bfailed[^\n]*\(401\)/i. Tests look good either way.

3. pendingId is removed before the server lookup — App.tsx:571-599

const pendingId = window.sessionStorage.getItem(OAUTH_PENDING_SERVER_KEY) ?? undefined;
window.sessionStorage.removeItem(OAUTH_PENDING_SERVER_KEY);
// …
window.history.replaceState({}, "", "/");
// …
const server = pendingId ? servers.find((s) => s.id === pendingId) : undefined;
if (!server) { /* toast + return */ }

If pendingId is present but no longer matches a server (e.g., the user deleted/renamed it in another tab), the key is gone and the URL is clean — recovery requires a fresh Connect click. That's probably fine, just worth being explicit that this is the design.

4. URL cleanup happens before the exchange — App.tsx:577

The comment explains the rationale (replay protection of a single-use code), and that's the right call. The tradeoff is that a transient completeOAuthFlow failure can't be recovered by reloading /oauth/callback; the user has to restart the whole flow. Worth a one-line mention in the toast ("Please try connecting again.") so the user knows reload won't help.

5. No tests cover the React orchestration

The PR description correctly calls out that pure logic lives in oauthFlow.ts with unit tests and the orchestration is in App.tsx. The branches that aren't exercised by the unit suite — the mount effect's path check / pending-key/ servers-empty/ error= branches, and the 401 catch in onToggleConnection — are validated by the manual end-to-end run. That's acceptable for now given the per-file 90% coverage gate may still pass, but a small App.test.tsx exercising the four callback effect branches (pathname !== callback, params.successful === false, missing pendingId, happy path with mocked InspectorClient) would harden against future regressions.

6. Minor — setupClientForServer dependency churn in the callback effect

useEffect(..., [servers, setupClientForServer]) re-runs whenever any of the 9 state-manager deps in setupClientForServer changes. The ref + path guards short-circuit cleanly, so this is harmless waste, but if it ever starts mattering you can move the guard ahead of the early returns (set the ref to true once pathname === OAUTH_CALLBACK_PATH even before servers hydrates) and accept the cost of dropping the callback if servers never load.

---

Things that look right

• oauthCallbackHandledRef after the early returns is intentional — lets the effect wait for servers to hydrate, then fires exactly once.
• connectStartRef = Date.now() placed right before connect() (not before completeOAuthFlow) so handshake latency excludes the token exchange.
• setupClientForServer(server) returns the client directly to the closure rather than relying on the state setter to flush — avoids the re-render lag.
• sessionStorage use for OAUTH_PENDING_SERVER_KEY is correct (origin-isolated, cleared on tab close, survives the redirect).
• redirectUrlProvider hoisted to module scope — good (no per-render churn for BrowserOAuthClientProvider).

Summary

Looks good to merge. Items 1 and 3/4's toast copy are the most user-visible — happy to ship as-is or tweak if you want. The rest are observations.

• Read changed files
• Read related OAuth core (authenticate, completeOAuthFlow, BrowserOAuthStorage, BrowserNavigation)
• Review for correctness, security, edge cases
• Review test coverage
• Post review
  · Branch: v2/wire-oauth-flow

[cliffhall]

Thanks for the thorough review — addressed the actionable items in b5de744.

1 + 4. Toast conflated OAuth-exchange vs connect failures — Fixed. Split the callback effect into two try/catch blocks: a completeOAuthFlow failure now reads "OAuth token exchange failed … Please try connecting again." (the single-use code is spent and the URL was already cleared, so a reload can't retry), while a post-OAuth connect() failure reads "Failed to connect" — since OAuth actually succeeded and re-clicking Connect reuses the persisted tokens with no second authorization. Added a comment explaining why conflating them would mislead the user into re-authorizing.

2. isUnauthorizedError regex could false-positive — Tightened. The message fallback now anchors on the transport's own wording, /\bfailed\b[^\n]*\(401\)/i (matches Remote (connect|send|events stream) failed (401): …) instead of a bare (401), so an unrelated (401) spliced into an error message can't trip the OAuth flow. Added a test for that case. (The structured err.status === 401 check still fires first as the primary path.)

3. pendingId removed before the server lookup — Made the design explicit with a comment: clearing the pending id + URL before the lookup is intentional, so a server deleted/renamed mid-flow surfaces the error and requires a fresh Connect rather than leaving stale callback state around.

5. No tests for the React orchestration — Acknowledged; deferring as a follow-up. Standing up App.test.tsx means mocking InspectorClient/useServers/the 9 state managers to render the full InspectorView, which is a sizable harness for the wiring component. The pure branches stay covered in oauthFlow.test.ts, and the effect's paths are exercised by the e2e run. Happy to file an issue for the focused callback-effect test if you'd like it tracked.

6. setupClientForServer dep churn in the effect — Acknowledged as harmless (the ref + path guards short-circuit). Left as-is.

Thanks for the positive callouts on the ref-after-early-returns pattern, the connectStartRef placement, and the module-scoped redirectUrlProvider — kept those.

Note: this PR's base is now v2/main (#1382 merged), and these changes are propagated down the rest of the stack (#1385, #1387). validate (1856 on this branch), test:integration, and test:storybook all green.