Skip to content

fix(auth): explicit re-authorize when the OAuth callback state was lost (#1808) - #1810

Merged
cliffhall merged 2 commits into
v2/mainfrom
fix/1808-oauth-callback-lost-state
Jul 27, 2026
Merged

fix(auth): explicit re-authorize when the OAuth callback state was lost (#1808)#1810
cliffhall merged 2 commits into
v2/mainfrom
fix/1808-oauth-callback-lost-state

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #1808

A /oauth/callback that arrives with no recorded SEP-2352 discovery state made the SDK throw

AuthorizationServerMismatchError: discoveryState was not available on the callback leg

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

situation recordedIssuer the SDK passes
nothing was recorded (recoverable) the sentence "discoveryState was not available on the callback leg; …"
genuine cross-AS mismatch (security signal) an absolute http(s) issuer URL

core/auth/issuerBinding.ts keys off that shape — the sentinel phrase or "not an http(s) URL" — so a reworded SDK sentence still classifies correctly. Detection is brand/shape based (mcpBrand === "mcp.AuthorizationServerMismatchError"), not instanceof, because the error can come from a different copy of the SDK than the one the module imports; and it walks cause / data.cause like findNestedAuthError, since the era-negotiation probe buries the original rejection.

Only lost_authorization_state gets the recovery affordance. A genuine issuer_mismatch is never papered over.

What the user sees

  • Lost state (web): a banner headed "Authorization state was lost" explaining in plain language that the stored authorization state was dropped (new browser session, another tab, state cleared mid-flow), with an "Authorize again" button. The action clears the stale/partial OAuth state for that server (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.
  • Genuine mismatch (web): a red, non-auto-closing notification headed "Authorization server mismatch" naming both issuers and stating the code was not exchanged — no retry button.
  • CLI / TUI: the runners have no banner, so their affordance is the message itself: runRunnerInteractiveOAuth rewrites the callback rejection into the same shared copy (original error preserved as cause). 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/callback catch; new banner kind; the banner action clears state then reconnects.
  • clients/web/src/components/groups/ReAuthBanner/ReAuthBanner.tsx — optional title / actionLabel (defaults unchanged), plus a closeButtonLabel so 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 contains discoveryState and 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 (cause preserved).
  • clients/web/src/App.test.tsx — new #1808 block: 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 + new ReAuthBanner.stories.tsx (both banner shapes, a11y-clean).

npm run ci is 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

…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
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jul 27, 2026
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 6m 26s —— View job


Review: explicit re-authorize when the OAuth callback state was lost (#1808)

  • Gather context (diff, changed files)
  • Review core/auth/issuerBinding.ts classification logic
  • Review core/auth/oauthUx.ts copy + web re-exports
  • Review App.tsx callback handling + banner action
  • Review ReAuthBanner + Node runner path
  • Review tests / coverage
  • Verify the brand check against the real published SDK build

The UX design here is right — the two-way split, the "never paper over a genuine mismatch" stance, cause preservation, and the copy separation are all well judged. But the detection predicate never matches a real SDK error, so as written the feature is inert in production. Details and a verified fix below.


🔴 Blocker — mcpBrand is a static class property; instances don't carry it

core/auth/issuerBinding.ts:77-81 gates everything on candidate.mcpBrand === "mcp.AuthorizationServerMismatchError". In @modelcontextprotocol/client@2.0.0-beta.5 that brand is defined in a static {} block on the constructor, not on the prototype or the instance, so err.mcpBrand on a thrown error is undefined. Static class fields are not inherited by instances.

I ran the PR's predicate verbatim against an error built by the real published SDK:

PR predicate on REAL sdk error -> false
instanceof                     -> true
isInstance                     -> true
wrapped cause isInstance       -> true
How the SDK actually brands these errors (from the published dist/index.mjs)
var AuthorizationServerMismatchError = class extends OAuthClientFlowError {
  static { Object.defineProperty(this, "mcpBrand", { value: "mcp.AuthorizationServerMismatchError" }); }
  constructor(recordedIssuer, currentIssuer) { /* … */ }
};

// base class:
var OAuthClientFlowError = class extends Error {
  static [Symbol.hasInstance](value) { return brandedHasInstance(this, value); }
  static isInstance(value) { return brandedHasInstance(this, value); }
  constructor(message) { super(message); this.name = new.target.name; stampErrorBrands(this, new.target); }
};

const BRANDS = Symbol.for("mcp.sdk.errorBrands");   // instances carry a Set here

So a real instance exposes: name === "AuthorizationServerMismatchError", recordedIssuer, currentIssuer, and [Symbol.for("mcp.sdk.errorBrands")] = Set { "mcp.AuthorizationServerMismatchError", "mcp.OAuthClientFlowError" } — and no mcpBrand. Confirmed empirically:

name: AuthorizationServerMismatchError
instance.mcpBrand: undefined        own mcpBrand: false
static: mcp.AuthorizationServerMismatchError
brands: [ 'mcp.AuthorizationServerMismatchError', 'mcp.OAuthClientFlowError' ]

Consequence: findIssuerBindingFailure() always returns undefined in production, so App.tsx:2578 falls straight through to the generic re-auth banner with the raw SDK text — i.e. exactly the #1808 symptom. The Node runner rewrite is equally inert.

Two secondary points worth folding into the fix:

  1. The module doc's rationale for avoiding instanceof is not true for this SDK. core/auth/issuerBinding.ts:59-65 says instanceof is unreliable across SDK copies — the SDK explicitly solved that: brandedHasInstance compares the instance's Symbol.for()-keyed brand set against the class's own brand and only falls back to prototype instanceof. Because the symbol comes from the global registry, instanceof / isInstance are cross-bundle safe by construction. That's the intended API for this exact concern.
  2. TypeScript couldn't catch this because the check casts to an ad-hoc { mcpBrand?: unknown; … } shape (issuerBinding.ts:72-76). The SDK's .d.mts declares only readonly recordedIssuer / readonly currentIssuer on the class — no instance mcpBrand — so using the real type would have surfaced it at compile time.

Suggested shape (core/auth already imports SDK error classes directly — see core/auth/utils.ts:1 for UnauthorizedError):

import { AuthorizationServerMismatchError } from "@modelcontextprotocol/client";

function isAuthorizationServerMismatchShape(
  err: unknown,
): err is AuthorizationServerMismatchShape {
  if (err === null || typeof err !== "object") return false;
  const candidate = err as { recordedIssuer?: unknown; currentIssuer?: unknown; name?: unknown };
  if (typeof candidate.recordedIssuer !== "string" || typeof candidate.currentIssuer !== "string") {
    return false;
  }
  // Cross-copy safe: the SDK stamps each instance with a `Symbol.for()`-keyed
  // brand set and overrides `Symbol.hasInstance` to read it, so this matches an
  // error thrown by a different copy of the SDK too.
  return (
    AuthorizationServerMismatchError.isInstance(err) ||
    // Belt-and-braces: neither the brand set nor the prototype survives a
    // serialization boundary, but `name` does.
    candidate.name === "AuthorizationServerMismatchError"
  );
}

Fix this →


🟠 Medium — no test constructs a real SDK error, which is why the blocker slipped a green npm run ci

Every fixture is hand-rolled with Object.defineProperty(err, "mcpBrand", …) on the instance (issuerBinding.test.ts:12-21, runner-interactive-oauth.test.ts:255, App.test.tsx:2215-2224), so all 15 + 2 + 4 cases assert against a shape the SDK never produces. The comment at issuerBinding.test.ts:7-11 ("a plain object is the honest fixture here") is what encodes the mistake.

At minimum, one case per layer should build new AuthorizationServerMismatchError(recorded, current) from @modelcontextprotocol/client and route it through the classifier / the /oauth/callback catch. Keep the synthetic fixtures for the shape-tolerance cases (cyclic chains, half-shaped errors) — just don't let them be the only fixtures.


🟡 Minor

  • isLostAuthorizationStateError is unused. Exported from core/auth/issuerBinding.ts:159 and re-exported as public core API (core/auth/index.ts:112), but no production caller — only its own test. Either use it in App.tsx/the runner in place of the failure.kind === … comparison, or drop it and keep the surface minimal.
  • Two sources for one default title (ReAuthBanner.tsx:19 + :45). ReAuthAlert.withProps({ title: DEFAULT_REAUTH_BANNER_TITLE }) is now dead — the component always passes title={title}, which already defaults. Drop it from withProps (or drop the parameter default) so the default lives in one place.
  • Banner state carries message but derives title/actionLabel from kind at render (App.tsx:886-892 vs :4337-4346). The kind → copy mapping now exists in two places. Storing the resolved title/actionLabel (you already compute copy in the catch) would keep issuerBindingFailureCopy the single source of truth and make a future third kind a one-line change. Relatedly, copy.title is computed and then unused in the lost-state branch at App.tsx:2580.
  • Clear-failure notification at App.tsx:2905-2909 has no autoClose: false, unlike the sibling security notification — the user loses the reason after 5s on a path where they now have neither a banner nor a working flow.

🔵 Informational

  • currentIssuer in the mismatch notification is remote-supplied (it's metadata.issuer from the configured server's AS metadata). Mantine escapes it so there's no injection risk, and for a debugging tool showing it is the right call — but the SDK explicitly warns against surfacing attacker-controllable issuer values on the sibling IssuerMismatchError, so consider truncating to a sane length to prevent notification-layout abuse.
  • A more robust discriminator exists locally. Rather than shape-sniffing the SDK's prose, the Inspector could ask its own store whether discoveryState exists for that server (OAuthStorage.discoveryState / providers.ts:326) — absent state is the lost-state case by construction, and it survives any SDK rewording. Worth considering as the primary signal with the sentinel as a fallback.
  • The !http(s) → lost state fallback is sound in practice. I checked the SDK's three throw sites: the callback-leg pair is the reachable one, and the third (stale client-info issuer stamp, index.mjs:700) is guarded by provider.saveClientInformation === undefined, which core/auth/providers.ts:220 implements — so it can't reach the Inspector and be misfiled as a mismatch. Good.

✅ Nice work

findIssuerBindingFailureDeep mirroring findNestedAuthError's cycle guard and data.cause walk keeps one traversal idiom in the codebase; the copy functions are pure and well tested; the runner rewrite preserves the original as cause; the closeButtonLabel a11y fix and the new Storybook story are both welcome; AGENTS.md was updated per the maintenance rules.

Note: mcp-docs isn't connected in this session, so instead of the protocol docs I verified the brand behavior directly against the published @modelcontextprotocol/client@2.0.0-beta.5 build — which is what the blocker above rests on.
· branch fix/1808-oauth-callback-lost-state

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

Copy link
Copy Markdown
Member Author

Thanks — the blocker was real and the diagnosis was right. Addressed below.

🔴 Blocker — fixed, using isInstance

Confirmed independently against the installed build before your review landed: mcpBrand is defined in a static {} block, so err.mcpBrand is undefined and "mcpBrand" in err is false on every real instance. The predicate could never match, and the feature was inert.

I'd first fixed this by reading err.constructor.mcpBrand, but your point that isInstance is cross-copy safe by construction is the better answer — brandedHasInstance consults a Symbol.for("mcp.sdk.errorBrands") set, which is exactly the cross-bundle concern the original comment cited to justify avoiding instanceof. So the rationale in that comment wasn't just unnecessary, it was backwards. Now:

return (
  AuthorizationServerMismatchError.isInstance(err) ||
  candidate.name === "AuthorizationServerMismatchError"
);

with the typeof … === "string" field checks kept as a precondition, and the module doc corrected.

🟠 Medium — fixed

All fixtures in issuerBinding.test.ts, App.test.tsx, and runner-interactive-oauth.test.ts now construct a real new AuthorizationServerMismatchError(...). I verified the fix is load-bearing by reverting only the predicate: 14 tests fail, and pass again with it restored — so this class of bug can no longer slip a green run.

Added a SDK brand placement test that asserts "mcpBrand" in err === false and that isInstance matches, so if a future SDK moves the brand this fails loudly rather than silently going inert again.

🟡 Minor — all four applied

  • isLostAuthorizationStateError unused — dropped, along with its core/auth/index.ts re-export and test. Kept the surface minimal rather than finding it a caller.
  • Two sources for the default title — removed title from ReAuthAlert.withProps; the parameter default is now the only one.
  • kind → copy duplicated — banner state now carries the resolved title/actionLabel from issuerBindingFailureCopy at the point it's raised, so the render site just passes them through. This also consumes the copy.title you spotted going unused.
  • Clear-failure notification — now autoClose: false.

🔵 Informational

  • Issuer truncation — taken. truncateIssuerForDisplay bounds both issuers at 120 chars, with a test. Agreed it's presentation hygiene rather than an injection concern.
  • OAuthStorage.discoveryState as the primary discriminator — declining. It is a more robust signal in the abstract, but I don't think it's worth reshaping a security-relevant branch late in this PR. The current classifier degrades safely: the !http(s) arm catches an SDK rewording without relying on the sentinel, and your own audit of the three throw sites confirmed the only other reachable one is guarded by provider.saveClientInformation === undefined, so nothing can be misfiled as a mismatch. The storage-based check would also introduce an async read into a synchronous classification path and couple core/auth's pure error classification to storage state. If the sentinel does drift later, that's the time to make the swap.
  • Third throw site audit — thanks, that was a useful confirmation of the !http(s) fallback's safety; I've left the behavior as-is on the strength of it.

npm run ci passes from the repo root.

@cliffhall

Copy link
Copy Markdown
Member Author

@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

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 5m 16s —— View job


Re-review: is the blocker genuinely resolved?

  • Read the fix commit (edddcdad)
  • Verify the predicate against a real SDK error
  • Verify the corrected module doc's cross-copy claim
  • Verify the error survives the full path to the App.tsx catch
  • Verify the fixtures are real SDK errors and load-bearing

Yes — the blocker is resolved, verified end to end against the published SDK, not by inspection. No new correctness problems found.


How I verified it

I bundled the actual core/auth/issuerBinding.ts from this branch (esbuild, SDK left external) against a real @modelcontextprotocol/client@2.0.0-beta.5 install and fed it errors built at the SDK's own throw sites.

Probe output — real AuthorizationServerMismatchError through the shipped classifier
instance has own mcpBrand: false | 'mcpBrand' in err: false
isInstance: true
name: AuthorizationServerMismatchError

lost      -> {"kind":"lost_authorization_state","currentIssuer":"https://as.example.com"}
mismatch  -> {"kind":"issuer_mismatch","recordedIssuer":"https://recorded.example.com","currentIssuer":"https://current.example.com"}

cause     -> {"kind":"lost_authorization_state",...}    # new Error(msg, { cause: err })
2-level   -> {"kind":"issuer_mismatch",...}             # cause → cause
data.cause-> {"kind":"lost_authorization_state",...}
serialized-> {"kind":"lost_authorization_state",...}    # prototype+brands stripped, name kept
unrelated -> undefined                                   # plain Error
lookalike -> undefined                                   # right fields, wrong brand/name

The lost fixture uses the SDK's verbatim string from dist/index.mjs:685; mismatch mirrors :687.

Four things this settles:

  1. The predicate now matches. Both classifications land correctly on real instances, through cause, data.cause, and multi-level chains — the shapes the era-negotiation probe actually produces.
  2. The corrected module doc is accurate, not just less wrong. I installed a second, physically separate copy of the SDK and constructed the error from copy B:
    same class object?        false
    A.isInstance(errFromB):   true
    plain instanceof:         true
    classify errFromB ->      {"kind":"lost_authorization_state",...}
    
    So isInstance genuinely is cross-copy safe, and issuerBinding.ts:58-71 states the reason correctly (Symbol.for("mcp.sdk.errorBrands") + overridden Symbol.hasInstance). isInstance is also properly declared on OAuthClientFlowError in the SDK's .d.mts:28, and it's invoked on the class (not detached), which the SDK's own doc warns about.
  3. The lost_authorization_state arm is genuinely reachable for this codebase. dist/index.mjs:685 only throws the sentinel variant when provider.saveDiscoveryState !== undefined — otherwise it just console.warns. core/auth/providers.ts:322 implements saveDiscoveryState/discoveryState, so the Inspector takes the throw branch. The feature isn't just wired, it's on a live path.
  4. The error reaches the catch unmodified. sdkAuthmcpAuth (no try/catch at all, core/auth/mcpAuth.ts) → oauthManager.completeOAuthFlow (dispatches oauthError, then bare throw error, oauthManager.ts:370-376) → resumeAfterOAuth (no catch around completeOAuthFlow, inspectorClient.ts:5482) → App.tsx:2586. Nothing re-wraps or stringifies it. And since auth() runs browser-side (only fetchFn is proxied), the instance is local — so the name arm really is belt-and-braces, exactly as the comment claims.

The suite is now load-bearing

Fixtures across all three layers construct real new AuthorizationServerMismatchError(...). I checked that this actually bites by restoring only the old predicate:

Failed Tests 14
Test Files  3 failed (3)
Tests       14 failed | 80 passed (94)

restored → all green (69 passed across the four core/banner files; 4 passed in the #1808 App block). That matches your count exactly. The SDK brand placement test (issuerBinding.test.ts:26-43) pinning "mcpBrand" in err === false and isInstance is the right guard — if a future SDK relocates the brand, it fails loudly instead of the feature going quietly inert again.

Minor items — confirmed addressed

isLostAuthorizationStateError and its index.ts re-export are gone; title is out of ReAuthAlert.withProps (parameter default is now the sole source); banner state carries the resolved title/actionLabel from issuerBindingFailureCopy and the render site just forwards them (App.tsx:4348-4349), which also consumes the previously-dead copy.title; the clear-failure notification is autoClose: false; truncateIssuerForDisplay bounds both issuers. I also checked showReAuthBanner (App.tsx:1636) builds a fresh object, so a generic banner can't inherit a stale title/kind from a prior lost-state banner.

🔵 Informational (no action implied)

  • await onToggleConnection(serverId) at App.tsx:2924 sits in a void (async () => …)() with no .catch, and onToggleConnection can still reject in uncommon paths (the second client.connect() in the AuthRecoveryRequiredError branch, setupClientForServer). Worth noting only because the deep-link caller at :2869 does catch — but :2967 and :4406 don't either, so this matches the prevailing pattern rather than deviating from it.
  • I did not re-litigate the storage-based discriminator; your reasoning (async read into a synchronous classifier, coupling pure error classification to storage state, !http(s) arm as the rewording backstop) is sound and I found nothing that undermines it.

Verdict

Blocker cleared. The classification is correct on real SDK errors, on the reachable throw site, through the real error chain, and the tests would now catch a regression of this class. CI on edddcdad was still in progress when I finished; nothing in my local runs suggests it won't be green.

Note: mcp-docs isn't connected in this session, so the verification above is against the published @modelcontextprotocol/client@2.0.0-beta.5 build rather than the protocol docs — which is the right ground truth for this particular claim anyway.
· branch fix/1808-oauth-callback-lost-state

@cliffhall

Copy link
Copy Markdown
Member Author

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 lost_authorization_state arm is on a live path via saveDiscoveryState being implemented, and tracing the error unmodified through to the App.tsx catch, both go beyond what I'd checked.

On the one informational item: I'm leaving the missing .catch on await onToggleConnection(serverId) as-is. As you note, the neighbouring call sites at :2967 and :4406 don't catch either, so adding one here would make this path inconsistent with the prevailing pattern for a rejection that's uncommon and already surfaces through the connection-status UI. If that's worth changing it should be changed uniformly, not just on the path this PR happens to touch.

No further changes. Closing out the review loop here.

@cliffhall

Copy link
Copy Markdown
Member Author

Manual smoke test (web)

Ran this branch in the real app — clients/web dev server plus the oauth-step-up-demo composable test AS on :8081 — driving the browser with Playwright.

How the failure was induced (the real condition, not a mock): connect oauth-step-up-demo → stop on the test AS consent page → delete the discoveryState entry for that server from ~/.mcp-inspector/storage/oauth.json → click Authorize. That is exactly "state was recorded on the authorize leg and gone by the callback leg".

Results

1. Lost state → the new banner (reproduced twice)

  • Heading "Authorization state was lost", body naming the server and explaining new browser session / different tab / state cleared mid-flow, ending "Authorize again to reconnect."
  • No AuthorizationServerMismatchError or discoveryState text anywhere in the UI — the SDK wording never reaches the user.
  • a11y: role="alert" named by the title, an Authorize again button, and the icon-only close button now exposes the accessible name "Dismiss" (the new closeButtonLabel).

2. Recovery action works end to end

Clicking Authorize again cleared the stale OAuth state (visible as a fresh DCR client_id on the new /oauth/authorize request) and restarted authorization in one click. Completing consent connected the server: status Connected, initialize + notifications/initialized + tools/list in the Protocol view, banner gone. The undiscoverable "clear OAuth state" escape hatch was not needed.

3. Console: zero errors across the whole run.

Not verified: the genuine issuer_mismatch branch

I could not force it from outside the SDK. When cached discovery state exists, the "current" issuer is read from that same cached metadata, so hand-editing the recorded issuer makes recorded == current and the mismatch check never trips; the discovery-time path is separately guarded by the SDK's RFC 8414 issuer validation. Reaching it for real would need a purpose-built rogue AS.

Worth noting the tamper attempt did not misclassify: editing the recorded issuer to a bogus value fell through to the ordinary re-auth banner ("Existing OAuth client information is required when exchanging an authorization code"), not to the lost-state affordance. That branch stays covered by the unit tests in issuerBinding.test.ts.

Test environment restored afterwards (oauth.json back to its prior contents, servers stopped, no working-tree changes).

@cliffhall
cliffhall requested a review from BobDickinson July 27, 2026 21:52
@modelcontextprotocol modelcontextprotocol deleted a comment from claude Bot Jul 27, 2026
@cliffhall
cliffhall merged commit 7784553 into v2/main Jul 27, 2026
3 checks passed
@cliffhall
cliffhall deleted the fix/1808-oauth-callback-lost-state branch July 27, 2026 22:08
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 5m 0s —— View job


Review (round 3): explicit re-authorize when the OAuth callback state was lost (#1808)

  • Fetch the merged PR head (edddcdad) and diff
  • Independently re-verify the classifier against a real SDK install
  • Review the post-fix code (issuerBinding.ts, oauthUx.ts, App.tsx, banner, runner)
  • Trace the recovery action's actual effect (clear → reconnect)
  • Review tests
  • Post review

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 independently

I bundled the merged core/auth/issuerBinding.ts (esbuild, SDK external) against a fresh @modelcontextprotocol/client@2.0.0-beta.5 install and fed it errors built at the SDK's own throw sites, using the verbatim sentinel from dist/index.mjs:685.

Probe output — real AuthorizationServerMismatchError through the shipped classifier
'mcpBrand' in lost: false | isInstance: true | name: AuthorizationServerMismatchError
lost                   {"kind":"lost_authorization_state","currentIssuer":"https://as.example.com"}
mismatch               {"kind":"issuer_mismatch","recordedIssuer":"https://recorded.example.com",...}
cause                  {"kind":"lost_authorization_state",...}
2-level                {"kind":"issuer_mismatch",...}
data.cause             {"kind":"lost_authorization_state",...}
serialized(name-only)  {"kind":"lost_authorization_state",...}
plain error            undefined
lookalike              undefined
cyclic                 undefined
non-http recorded      {"kind":"lost_authorization_state",...}
empty recorded         {"kind":"lost_authorization_state",...}

One thing worth recording that hadn't been checked: the name fallback arm isn't theoretical. this.name = new.target.name and the two issuer fields are own enumerable properties, so they survive JSON.stringify where the prototype and the Symbol.for-keyed brand set do not:

JSON.stringify(lost) = {"name":"AuthorizationServerMismatchError","recordedIssuer":"discoveryState was not available on the callback leg; …","currentIssuer":"https://as.example.com"}

So both arms of the predicate are load-bearing, and the comment at core/auth/issuerBinding.ts:58-71 is accurate on both counts.

The recovery action does what the banner promises

Not previously traced end to end. Both branches of clearServerOAuthState converge on the same full clear, so the retry can't behave differently depending on whether the server happened to be the active connection:

  • active → inspectorClient.clearOAuthTokens()oauthManager.clearOAuthTokens()storage.clear(serverUrl) (core/mcp/oauthManager.ts:393-403, plus in-memory oauthFlowState reset)
  • inactive → oauthStorage.clear(serverUrl) directly

And the comment at App.tsx:2896-2899 ("connecting is always the right toggle direction") is provable rather than just plausible: onToggleConnection takes the disconnect branch only when connectionStatus === "connected" (App.tsx:2652-2656), which the /oauth/callback failure path can't be. If serversRef no longer holds the server, the clear is skipped and it falls through to the toggle — the benign direction.

Coverage is complete across entry points

completeOAuthFlow has exactly two production callers — resumeAfterOAuth (web, App.tsx:2564) and runRunnerInteractiveOAuth (core/auth/node/runner-interactive-oauth.ts:90) — and both CLI (clients/cli/src/cliOAuth.ts:215) and TUI (clients/tui/src/App.tsx:610) route through the latter. No third path can still surface raw SDK text.

Tests

Fixtures are real SDK errors at all three layers, and the SDK brand placement block (issuerBinding.test.ts:26-43) pins both halves of the original mistake — "mcpBrand" in err === false and isInstance matching — so a future SDK relocating the brand fails loudly instead of the feature going quietly inert. That's the right shape of regression guard for this class of bug.


🟡 Minor (non-blocking, merged as-is is fine)

  • truncateIssuerForDisplay isn't re-exported. It's exported from core/auth/oauthUx.ts:271 but absent from both core/auth/index.ts and clients/web/src/utils/oauthUx.ts, unlike every other copy helper added in this PR. Its test reaches it via the deep path, so nothing breaks — just an inconsistent surface.
  • Two import idioms for one feature. App.tsx pulls the copy from ./utils/oauthUx but the classifier from the deep @inspector/core/auth/issuerBinding.js, even though core/auth/index.ts:110-113 now re-exports findIssuerBindingFailure.
  • Two scheduling idioms in one catch block. App.tsx:2593 sets the lost-state banner directly while the generic fallback four lines later goes through queueMicrotask (:2610). Both run in the same async continuation so it's harmless — I checked there's no competing setReAuthBanner(null) on this path — but the asymmetry invites a reader to assume the microtask matters.

🔵 Informational

  • The one shape where the security signal is suppressed. The SDK computes recordedIssuer as cachedState?.authorizationServerMetadata?.issuer ?? cachedState?.authorizationServerUrl (dist/index.mjs:684) — the second disjunct being the Inspector's own persisted value. If that were ever persisted in a form that doesn't parse as http(s), a genuine mismatch would take the !http(s) arm and get the friendly affordance. The blast radius is small: the SDK throws before the code is exchanged, and the recovery only clears local state and starts a fresh authorize — so the outcome is "no warning" rather than "credential leaked". Noting the residual only; not proposing the storage-based discriminator, which you've already ruled on.
  • The runner's lost-state copy is the shared web wording, so a CLI/TUI user sees "a new browser session, a different tab" — accurate enough (the runners do open a browser) and clearly the intended tradeoff of sharing one string.

✅ Verdict

Ship as merged. The classification is correct on real SDK errors including through cause / data.cause / serialization, the recovery action's clear is equivalent on both branches, the toggle direction is provably right, and the suite now fails if the predicate regresses. The minor items above are consistency polish for a follow-up, not defects.

mcp-docs is connected this session, but the claims here are about a specific published SDK build's runtime branding rather than the protocol spec, so the ground truth is the @modelcontextprotocol/client@2.0.0-beta.5 package itself — which is what I ran against.
· branch fix/1808-oauth-callback-lost-state

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.

OAuth callback leg dead-ends when discovery state is missing (AuthorizationServerMismatchError)

1 participant