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
|
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