fix(auth): explicit re-authorize when the OAuth callback state was lost (#1808) - #1810
Conversation
…1808) A `/oauth/callback` that arrives with no recorded SEP-2352 discovery state made the SDK throw `AuthorizationServerMismatchError` and the Inspector dead-ended, surfacing the raw SDK text with no path forward. The SDK folds two very different failures into that one error class, so `core/auth/issuerBinding.ts` classifies them structurally: a recoverable "lost authorization state" (the SDK puts prose, not an issuer URL, in the `recordedIssuer` slot) versus a genuine cross-AS `issuer_mismatch`. Only the first gets a recovery affordance. Web: the callback failure now raises a dedicated banner ("Authorization state was lost" / "Authorize again") whose action clears the stale OAuth state for that server and starts a fresh authorization. A genuine mismatch stays a red, non-dismissible security notification with both issuers named and no one-click retry. Node runners (CLI/TUI): the same classification rewrites the callback rejection into the shared actionable copy instead of the SDK text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt
|
@claude review |
|
Claude finished @cliffhall's task in 6m 26s —— View job Review: explicit re-authorize when the OAuth callback state was lost (#1808)
The UX design here is right — the two-way split, the "never paper over a genuine mismatch" stance, 🔴 Blocker —
|
The callback-leg classifier gated on `err.mcpBrand`, but the SDK declares
that brand in a `static {}` block, so it lives on the constructor and is
never reachable from an instance. The predicate always returned false, so
`findIssuerBindingFailure()` never matched and the #1808 recovery banner
was inert in production — the callback dead-ended exactly as before.
Use the SDK's own `AuthorizationServerMismatchError.isInstance()`, which is
cross-copy safe by construction (it consults a `Symbol.for()`-keyed brand
set), with a `name` fallback for a serialization boundary. Correct the
module doc, which cited cross-bundle safety as the reason to avoid
`instanceof` — the very thing the SDK's branded `hasInstance` provides.
Every fixture now constructs a real `AuthorizationServerMismatchError`
instead of a hand-rolled object with an own `mcpBrand`, which is why a
green `npm run ci` hid this. Adds a test pinning the brand's placement so
the feature cannot silently go inert again.
Review follow-ups: drop the unused `isLostAuthorizationStateError`, remove
the duplicated banner title default, resolve the banner title/action label
where the banner is raised, keep the clear-failure notification open, and
bound remote-supplied issuers in user-facing copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt
|
Thanks — the blocker was real and the diagnosis was right. Addressed below. 🔴 Blocker — fixed, using
|
|
@claude review Please focus on whether the blocker is genuinely resolved — that the classifier now matches a real SDK error end to end. The minor items from the last round are addressed; I've declined the storage-based discriminator with reasoning in the reply above. Please don't re-raise style preferences or re-litigate that decision unless you've found a correctness problem. |
|
Claude finished @cliffhall's task in 5m 16s —— View job Re-review: is the blocker genuinely resolved?
Yes — the blocker is resolved, verified end to end against the published SDK, not by inspection. No new correctness problems found. How I verified itI bundled the actual Probe output — real
|
|
Thanks — that end-to-end verification (bundling the branch's actual classifier against a real SDK install, and constructing the error from a second physically separate copy to prove the cross-copy claim) is exactly the right ground truth for this fix. Confirming the On the one informational item: I'm leaving the missing No further changes. Closing out the review loop here. |
Manual smoke test (web)Ran this branch in the real app — How the failure was induced (the real condition, not a mock): connect Results1. Lost state → the new banner (reproduced twice)
2. Recovery action works end to end Clicking Authorize again cleared the stale OAuth state (visible as a fresh DCR 3. Console: zero errors across the whole run. Not verified: the genuine
|
|
@claude review |
|
Claude finished @cliffhall's task in 5m 0s —— View job Review (round 3): explicit re-authorize when the OAuth callback state was lost (#1808)
No correctness problems found. The blocker from round 1 is resolved, and I re-verified it from scratch rather than re-reading round 2 — plus checked two things neither earlier round did: what the recovery action actually clears, and whether "connect" is provably the right toggle direction. Both hold. Blocker: re-verified independentlyI bundled the merged Probe output — real
|
Address Copilot review on #1847: - server.ts: restrict client-supplied per-send headers to the `Mcp-Param-` prefix (string values only) via `mcpParamHeadersOnly` before applying them upstream, so the per-send channel can't inject arbitrary headers (e.g. Authorization). `settings.headers` remains the channel for configured upstream headers. Covered by new unit tests. - Fix stale `#1810` breadcrumbs → `#1846` in the two new tests. - Trim the verbose mirroring comments in inspectorClient/types/remote transport. - Drop the redundant base64-on-the-wire integration test (encoding is unit- tested; wire delivery is proven by the verbatim test), remove two unneeded test casts, and cover the boolean `true` branch. Not changed (declined with reason on the PR): buildMcpParamHeaders remains a faithful port of the SDK's helper and does not re-validate values against the declared JSON Schema type — arg types are enforced by schema validation on both sides, and matching the SDK's conforming wire output is the module's goal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd
…call (#1847) * fix(core): mirror SEP-2243 x-mcp-header args to Mcp-Param-* on tools/call (#1846) A strict modern (2026-07-28) HTTP server (e.g. the GitHub MCP Server) rejects a tools/call whose argument carries an `x-mcp-header` annotation when the matching `Mcp-Param-*` request header is missing (-32020 "header mismatch"). The Inspector never sent that header. The SDK only mirrors inside `client.callTool()` (and skips it entirely in a browser environment), but the Inspector routes tools/call through `client.request()` for manual MRTR driving (#1704), so the SDK never mirrors for us — on any transport. On top of that, the remote (web) transport dropped per-request headers before they reached the backend's upstream fetch. - core/json/xMcpHeader.ts: port the SDK's non-public `buildMcpParamHeaders` + value encoding (Base64 sentinel for non-ASCII/whitespace/empty values) and add `mcpParamHeadersForTool`. - InspectorClient.attemptToolCall: on a modern connection, build the headers from the tool's declarations + call arguments and attach them to the tools/call request options. `Protocol.request` forwards `headers` to the transport and preserves them across MRTR retry legs, so the direct StreamableHTTP transport applies them. Fixes CLI/TUI. - RemoteClientTransport.requestSend + /api/mcp/send + RemoteSendRequest: forward per-send headers and apply them to the backend's upstream transport.send. The browser can't set cross-origin headers, but the Node backend can — where the Inspector's real request is issued. Fixes web. Verified end-to-end against the live GitHub MCP Server (modern HTTP). Closes #1846 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd * review: harden backend header allowlist, fix breadcrumbs, trim comments Address Copilot review on #1847: - server.ts: restrict client-supplied per-send headers to the `Mcp-Param-` prefix (string values only) via `mcpParamHeadersOnly` before applying them upstream, so the per-send channel can't inject arbitrary headers (e.g. Authorization). `settings.headers` remains the channel for configured upstream headers. Covered by new unit tests. - Fix stale `#1810` breadcrumbs → `#1846` in the two new tests. - Trim the verbose mirroring comments in inspectorClient/types/remote transport. - Drop the redundant base64-on-the-wire integration test (encoding is unit- tested; wire delivery is proven by the verbatim test), remove two unneeded test casts, and cover the boolean `true` branch. Not changed (declined with reason on the PR): buildMcpParamHeaders remains a faithful port of the SDK's helper and does not re-validate values against the declared JSON Schema type — arg types are enforced by schema validation on both sides, and matching the SDK's conforming wire output is the module's goal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd * review: mirror Mcp-Param-* on the task-augmented tools/call too Address the maintainer review on #1847: - `callToolStream` (the "Run as task" path) built its own request options and never mirrored, so a task-augmented `tools/call` on an annotated tool was still rejected by a strict modern server with -32020. It now mirrors like the plain path. - Extract the two blocks both `tools/call` entry points had duplicated — `convertStringToolArgs` (the string-arg coercion) and `applyMirroredParamHeaders` (the SEP-2243 mirroring) — so the paths can't drift again. That duplication is why the task path was missed. - Regression test: `callToolStream` against the modern `get_weather` server asserts the POST carries `Mcp-Param-City` (verified failing without the fix). - README: the modern-network showcase claimed mirroring is skipped in the browser so the web client gets -32020, and that CLI/TUI mirror. Both are now wrong — the Inspector mirrors itself, on every client. AGENTS: note that xMcpHeader.ts now also builds the wire headers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UkhhCHryVp5H3dqEnhwZ7t * docs: correct the stale "browser omits Mcp-Param-*" copy Both statements predate this PR and are now the opposite of true, per the wire capture on #1847 (proxy-observed upstream headers, web leg included). - ToolDetailPanel: the mirrored-headers note claimed the browser omits `Mcp-Param-*`. It now says the web client's headers are applied by the Node backend that issues the upstream request. - createRemoteFetch: the header comment claimed an `x-mcp-header`-annotated tool is "uncallable from the web client against a strict modern server" — the exact wrong conclusion to reach while debugging a -32020. It now records that the Inspector builds the headers itself, and that they do NOT travel this `/api/fetch` proxy path: they ride the transport's per-send `headers` through `/api/mcp/send`. Copy only — no test asserted the old note text (ToolDetailPanel.test.tsx matches the section title only). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UkhhCHryVp5H3dqEnhwZ7t --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: cliffhall <cliff@futurescale.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd
Closes #1808
A
/oauth/callbackthat arrives with no recorded SEP-2352 discovery state made the SDK throwand the Inspector dead-ended: the raw SDK text landed in the generic re-auth banner's detail line, and the only real escape was the (undiscoverable) "clear OAuth state" action.
Per the decision on the issue this implements the explicit re-authorize prompt, not a silent auto-restart.
Distinguishing the two failures
The SDK folds two very different situations into one error class, and exposes no discriminator:
recordedIssuerthe SDK passes"discoveryState was not available on the callback leg; …"http(s)issuer URLcore/auth/issuerBinding.tskeys off that shape — the sentinel phrase or "not anhttp(s)URL" — so a reworded SDK sentence still classifies correctly. Detection is brand/shape based (mcpBrand === "mcp.AuthorizationServerMismatchError"), notinstanceof, because the error can come from a different copy of the SDK than the one the module imports; and it walkscause/data.causelikefindNestedAuthError, since the era-negotiation probe buries the original rejection.Only
lost_authorization_stategets the recovery affordance. A genuineissuer_mismatchis never papered over.What the user sees
clearServerOAuthState) and then starts a fresh connect → authorization, so the manual escape hatch is no longer needed. The SDK's wording never reaches the user.runRunnerInteractiveOAuthrewrites the callback rejection into the same shared copy (original error preserved ascause). This path is much harder to reach there — Node OAuth state is file-backed and the redirect + callback happen in one process run — but the copy is now correct instead of raw SDK text if it does.Changes
core/auth/issuerBinding.ts(new) —findIssuerBindingFailure()/isLostAuthorizationStateError().core/auth/oauthUx.ts— shared copy:lostAuthorizationState{Title,Message,ActionLabel},issuerMismatch{Title,Message},issuerBindingFailureCopy().core/auth/node/runner-interactive-oauth.ts— rewrite the callback-leg rejection through that copy.clients/web/src/App.tsx— classify in the/oauth/callbackcatch; new bannerkind; the banner action clears state then reconnects.clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.tsx— optionaltitle/actionLabel(defaults unchanged), plus acloseButtonLabelso the icon-only close button has an accessible name.Tests
clients/web/src/test/core/auth/issuerBinding.test.ts— 15 cases: both classifications, the sentinel, prose and non-http(s)recorded values,cause/data.cause/ multi-level walks, cyclic chains, half-shaped and differently-branded errors.clients/web/src/test/core/auth/oauthUx.test.ts— the new copy, including that the lost-state message never containsdiscoveryStateand the mismatch message never offers a retry.clients/web/src/test/core/auth/runner-interactive-oauth.test.ts— both rewrites on the Node runner path (causepreserved).clients/web/src/App.test.tsx— new#1808block: the affordance appears for lost state, the button clears OAuth state and rebuilds the client, a genuine mismatch produces the security notification and no button, and an unrelated failure still falls back to the generic banner.ReAuthBanner.test.tsx+ newReAuthBanner.stories.tsx(both banner shapes, a11y-clean).npm run ciis green from the repo root (validate 3,707 tests; coverage gate 4,778 incl. integration; cli 304, tui 282, launcher 5; smokes; 462 Storybook play tests).🤖 Generated with Claude Code
https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt