Skip to content

fix(auth): key in-flight PKCE verifiers by flow, not server URL - #2742

Merged
cliffhall merged 3 commits into
v2/mainfrom
v2/fix/2726-per-flow-pkce-verifier
Oct 20, 2026
Merged

cliffhall merged 3 commits into
v2/mainfrom
v2/fix/2726-per-flow-pkce-verifier

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #2726

Problem

The web client kept each server's PKCE codeVerifier in the backend's shared oauth.json, keyed by server URL only. Two OAuth flows to the same server that overlapped in time (two tabs, or several people using one deployed Inspector) shared that one slot. The later flow overwrote the earlier one's verifier, so the earlier flow's token exchange failed PKCE with invalid_grant.

Fix

The in-flight verifier now belongs to its flow, not to the server:

  • core/auth/pkce-flow-store.ts (new) adds a PkceFlowStore keyed by the auth session id that generateOAuthState() already puts in the OAuth state (the 64-hex authId). createWebStoragePkceFlowStore() backs it with the initiating tab's sessionStorage, which survives the full-page redirect to the authorization server and back and is never visible to another tab. Each entry also records its server URL, so a state replayed against a different server can't borrow that server's verifier.
  • BaseOAuthClientProvider takes an optional flowStore. state() remembers the flow it just started; the SDK calls it immediately before saveCodeVerifier(). resumeAuthorizationFlow(state) binds the callback leg to its flow. When a flow store is configured, codeVerifier() never falls back to the shared slot. A state with no stored verifier fails with a clear error ("No code verifier saved for this authorization flow…") instead of using another flow's verifier.
  • The callback state is passed through OAuthManager.completeOAuthFlow → InspectorClient.completeOAuthFlow / resumeAfterOAuth. The web /oauth/callback handler and the node loopback runner (CLI/TUI) both forward it. After a successful exchange the spent verifier is cleared.
  • The web environment (environmentFactory.ts) sets oauth.flowStore. CLI/TUI set none and keep their current storage path unchanged.

Why not key it inside servers[url]? The remote backend merges whole servers[url] entries, so a second tab's write would still replace the first tab's map. That is why the verifier lives in the tab.

Only a completed flow's results (client information and tokens) reach oauth.json. So the CLI/TUI --use-stored-auth hand-off of a web-acquired token is untouched.

Out of scope

The issue's "Related" note covers two things this PR does not change: DCR client info and tokens are still shared across browsers by server URL, and two browsers with a stale view can each run their own DCR. The verifier collision is the defect in the title and the one measured, and it is what this PR fixes. The SEP-2352 discovery state also stays server-keyed. The SDK reads it before state() exists, and it is a per-server discovery cache that is the same for every flow to that server.

Tests

  • inspectorClient-oauth-concurrent-flows-e2e.test.ts (new, integration) runs the real remote backend and a test authorization server that checks PKCE S256. It reproduces the issue's sequence: tab A starts a flow, tab B starts and finishes one, then tab A's callback page finishes. It has three cases:
    1. With the flow store, both tabs connect.
    2. Control: without a flow store, tab A fails exactly as reported. This shows the test detects the defect.
    3. A callback whose state has no stored verifier (it lands in a tab that never saw the flow) fails with the clear error.
  • pkce-flow-store.test.ts (new) covers the round-trip, flows kept apart, the server-URL check, clearing, corrupt entries, and the sessionStorage default.
  • providers.test.ts adds per-flow keying, two overlapping flows on fresh callback providers, no fallback to the shared slot, a foreign state, flow-scoped invalidateCredentials("verifier"), and the unchanged path without a flow store.
  • The runner and TUI tests now assert that the callback state is forwarded.

npm run format and npm run local:gate pass. There is no visible UI change: tab A now connects instead of showing the re-authentication banner. The integration test covers that path, so there are no screenshots.

🤖 Generated with Claude Code

Overlapping OAuth flows to the same server (two tabs, or several users
of one deployed Inspector) shared one codeVerifier slot in the backend's
oauth.json, so the later flow overwrote the earlier one's verifier and
the earlier exchange failed PKCE with invalid_grant.

The web client now keeps each flow's verifier in the initiating tab's
sessionStorage, keyed by the OAuth `state` authId, and the callback
exchange reads the verifier for the `state` it returned with. Keying
inside servers[url] would not help, since the remote backend merges
whole server entries. A callback whose `state` has no stored verifier
fails with a clear error rather than borrowing another flow's.

CLI/TUI set no flow store and keep the existing storage path; the
runner now forwards the callback `state` too.

Closes #2726

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Signed-off-by: cliffhall <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The cross-server isolation test reads fresh empty storage, so it cannot detect a broken server-binding check.

1 open issue
What changed in this PR

Fixes overlapping web OAuth flows by storing PKCE verifiers per flow in the initiating tab, while preserving CLI/TUI storage behavior.

Changes:

  • Adds a sessionStorage-backed, flow-keyed verifier store.
  • Forwards callback state through OAuth completion and clears completed flows.
  • Adds regression coverage and documents the storage split.
File Description
specification/​v2_storage.md Documents per-tab verifier storage.
core/​mcp/​types.ts Adds optional flow-store configuration.
core/​mcp/​oauthManager.ts Binds callbacks and clears completed flows.
core/​mcp/​inspectorClient.ts Forwards callback state.
core/​auth/​providers.ts Saves and retrieves flow-specific verifiers.
core/​auth/​pkce-flow-store.ts Implements the verifier store.
core/​auth/​node/​runner-interactive-oauth.ts Forwards loopback callback state.
core/​auth/​index.ts Exports flow-store APIs.
clients/​web/​src/​test/​integration/​mcp/​inspectorClient-oauth-concurrent-flows-e2e.test.ts Covers overlapping flows and missing verifiers.
clients/​web/​src/​test/​core/​auth/​runner-interactive-oauth.test.ts Checks callback-state forwarding.
clients/​web/​src/​test/​core/​auth/​providers.test.ts Covers provider flow isolation.
clients/​web/​src/​test/​core/​auth/​pkce-flow-store.test.ts Tests verifier storage and server binding.
clients/​web/​src/​lib/​environmentFactory.ts Enables per-tab verifier storage.
clients/​web/​src/​hooks/​useOAuthRecovery.ts Passes callback state into recovery.
clients/​tui/​__tests__/​App.test.tsx Checks TUI callback-state forwarding.

Review effort: Balanced

Comment thread clients/web/src/test/core/auth/pkce-flow-store.test.ts Outdated
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Signed-off-by: cliffhall <[email protected]>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 1: one finding (the cross-server flow-store test read from a fresh storage area, so it could not detect a broken server-binding check). It was a real defect in this PR's test, fixed in 3fd5b5b and verified by mutation (removing the server check now fails the test). Replied in the thread. Requesting round 2.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

Flow isolation and callback propagation are consistent, with targeted regression coverage and no unresolved findings.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review loop closed: round 2 was clean (no inline comments, "No open issues", round-1 finding marked resolved). Stopping on the first clean round per pr-flow.

Resolves the runner-interactive-oauth.ts conflict by keeping both sides:
completeOAuthFlow's new state parameter (#2726) and
checkAuthChallengeSatisfied's new options parameter (#2724).

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Signed-off-by: cliffhall <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Web: concurrent OAuth flows to the same server overwrite each other's PKCE verifier (shared oauth.json keyed by server URL)

2 participants