fix: report a terminal OAuth token-endpoint refusal instead of a dead retry

[cliffhall]

Closes #2280

Split out of #2282, where this was tangled with the *.localhost origin work for #1944. That issue turns out to be ambiguous about which layer it means, and the reporter has since confirmed OAuth is involved — which points at their MCP server being on a *.localhost host rather than the Inspector being hosted on one. This half is independent of that question and correct on every answer to it, so it should not wait.

The bug

InsecureTokenEndpointError appeared nowhere in our source. It fell through to the generic auth-failure path and rendered a "Re-authentication required" banner with a Re-authenticate button.

That button can never work. The SDK's assertSecureTokenEndpoint runs inside executeTokenRequest, the error deliberately does not extend OAuthError, and auth() special-cases it to rethrow rather than fall through to a fresh /authorize redirect. Clicking re-ran the same flow to the same refusal, and the only text on screen was the raw SDK message — which names the three exempt host literals and says nothing about which lever to reach for.

So the Inspector presented a terminal configuration error as a retryable auth error.

This is not specific to *.localhost. It fires for any token endpoint outside the SDK's exemption — host.docker.internal (#1911), a LAN hostname, a reverse-proxy name, a mistyped scheme.

The fix

Recognize it (core/auth/insecureTokenEndpoint.ts, mirroring issuerBinding.ts's brand-plus-name classifier, walking cause / data.cause because era negotiation and the transport wrappers bury the rejection) and surface it as the configuration error it is: naming the endpoint, saying retrying cannot help, and pointing at the per-server Token URL override (#1906) as the one lever available locally. No action affordance.

Six paths reach that refusal and all six now classify it:

Path	Why it is distinct
connect handshake	connect() rejects directly
authenticate() during connect	the refresh path
the satisfied-challenge connect retry	a satisfied challenge still ends in a token exchange
post-redirect callback	resumeAfterOAuth rejects
command / background	a mid-session silent refresh
deferred tab-visible resume	re-armed on every focus — an unbounded loop on a terminal error
the re-auth banner action	the one a user reaches by clicking Re-authenticate

They share one wrapper so a seventh cannot silently omit the banner clear, and every clear is scoped to its own serverId — these paths are asynchronous, so a late continuation for server A must not erase a banner server B raised in the meantime.

Reproducing it

New fixture oauth-insecure-token-endpoint-http.json, documented in docs/test-servers.md. Its issuerUrl is http://localhost.:8091 — the root-anchored spelling of localhost, which every resolver sends to loopback (so the flow runs for real) but which is none of the SDK's three exempt literals (so the exchange is refused). It reproduces the class without needing an /etc/hosts entry or dnsmasq.

⚠️ Don't run it while 8091 is taken. Its port is hard-coded inside the OAuth issuer, so a relocated server would announce one port while its metadata named another — discovery reaches an unrelated process and the refusal either never fires or fires for the wrong reason. docs/test-servers.md says to check lsof -nP -iTCP:8091 before believing what the flow does.

(An earlier revision added a transport.strictPort option to enforce this. It has been dropped — it was a new config option, its validation and 18 tests, invented to support one fixture. test-servers/src is byte-identical to v2/main.)

What this does not do

Making such an endpoint actually work has to land in the SDK: the assertion takes no options and there is no hook we can reach. That is typescript-sdk#2591, with an open fix at #2597 I have asked for review on. #1911 reached the same conclusion. This PR changes the reporting, not the outcome — which for the #1944 reporter is currently the only thing we can ship.

Screenshots

Both arms drive the real OAuth flow end to end against the new fixture: add the server, connect, complete the authorization, and let the redirect come back so the token exchange is actually attempted.

Before (v2/main) — a "Re-authentication required" banner with a Re-authenticate button that cannot succeed. The only text is the raw SDK message, which names the three exempt literals and nothing actionable. The card is also flagged red with the monitoring sidebar pulled open, presenting a configuration error as a failed connect attempt.

After — a non-expiring "Token endpoint is not secure" notice naming the offending endpoint, stating plainly that re-authenticating cannot change this, and giving both remedies. No action affordance, and no red card.

The "after" shot is deliberately a re-capture on the current head, not the one from #2282: the copy changed twice during review, and an image showing superseded wording would misrepresent what merges. Visible in it are all of those fixes — "without sending this request" (round 1, since the notice also serves refresh paths where credentials were sent), "OAuth Settings" / "Token URL override" (round 1, the section names the UI actually renders), "not HTTPS" and the bracketed [::1] (round 2, since a bare IPv6 literal is not a legal URL host).

Testing

npm run format + npm run local:gate pass. Coverage includes the classifier's cause chains (nested, data.cause, self-referential loop), all six hook paths asserting position — no banner, no failed-server flag, no generic toast — the cross-server banner guard, and and the cross-server banner guard.

[Copilot]

🟡 Changes recommended

The notice contains misleading guidance, and one strict-port failure path can leave stale global server state.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR reports insecure OAuth token endpoints as terminal configuration errors instead of offering a futile retry.

Changes:

• Adds error classification and terminal notification handling across OAuth paths.
• Adds a fixed-port OAuth reproduction fixture.
• Expands unit and integration coverage.

File summaries

File	Description
test-servers/src/test-server-http.ts	Adds strict port binding.
test-servers/src/resolve-config.ts	Propagates strictPort.
test-servers/src/load-config.ts	Validates strict-port configuration.
test-servers/src/composable-test-server.ts	Defines the strict-port option.
test-servers/configs/oauth-insecure-token-endpoint-http.json	Adds the reproduction fixture.
docs/test-servers.md	Documents reproduction and expected behavior.
core/auth/oauthUx.ts	Adds terminal-error notification copy.
core/auth/insecureTokenEndpoint.ts	Classifies nested SDK errors.
clients/web/src/utils/oauthUx.ts	Re-exports notification copy.
clients/web/src/test/integration/mcp/strict-port.test.ts	Tests strict-port behavior.
clients/web/src/test/core/auth/oauthUx.test.ts	Tests user-facing copy.
clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts	Tests error classification.
clients/web/src/lib/insecureTokenEndpointNotice.ts	Implements the terminal notice.
clients/web/src/lib/insecureTokenEndpointNotice.test.ts	Tests notice rendering.
clients/web/src/hooks/useOAuthRecovery.ts	Handles terminal errors during recovery.
clients/web/src/hooks/useOAuthRecovery.test.tsx	Covers recovery paths.
clients/web/src/hooks/useConnectionLifecycle.ts	Handles connection-path refusals.
clients/web/src/hooks/useConnectionLifecycle.test.tsx	Covers connection paths.
.claude/skills/test-servers/SKILL.md	Adds the fixture to the skill catalogue.

Review details

• Files reviewed: 19/19 changed files
• Comments generated: 3
• Review effort level: Balanced

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Review round 1 — addressed

Two real findings, both copy accuracy, and both worse than their size suggests — this is the message a stuck user reads to find out what to do.

Finding	Fix
"before any credentials were sent" is false on refresh / re-auth paths	Scoped to the request actually refused
"Server Settings → Authorization" names a section that does not exist	Now "OAuth Settings" / "Token URL override", as rendered

The second is the one I would most regret shipping. I wrote the section name from memory; ServerSettingsForm.tsx renders the accordion as OAuth Settings and the field as Token URL override. So the notice would have sent someone to a nonexistent settings section at exactly the moment it was telling them to go reconfigure something — a dead end inside a message about a dead end.

The first is subtler and equally wrong: this notice serves mid-session refresh too, where credentials were legitimately sent earlier. Someone an hour into a session would read the absolute claim as describing a different failure.

Both are pinned by tests including negative assertions on the old strings, so the wording cannot drift back.

The third finding was against strictPort's precondition, which no longer exists — that option was dropped in 6c1b532, after the review ran. It was correct against the code as it stood.

npm run format + npm run local:gate → EXIT=0, zero failures.

[Copilot]

🟡 Changes recommended

The guidance uses an invalid IPv6 URL spelling and repeatedly misattributes the TLS check to SEP-2207.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 14/14 changed files
• Comments generated: 3
• Review effort level: Balanced

[cliffhall]

Review round 2 — both addressed

Finding	Fix
::1 is not a legal URL host	[::1] in the notice and the reproduction guide
SEP-2207 means something else in this repo	Label removed from all nine files

The ::1 one is the kind of defect I find most embarrassing: the sentence whose entire job is to unblock a stuck user contained advice that could not be copied. new URL("http://::1/token") throws. The exemption list still reads ::1 — that is the host string the SDK compares — while the remedy reads [::1], and the doc comment now says why, so nobody "corrects" one into the other.

On the SEP label, worth recording where it came from. I did not invent it: the SDK's own source comments say SEP-2207: refuse to send credentials to a non-TLS, non-loopback token endpoint, directly above assertSecureTokenEndpoint and again on the error class. I copied that without checking it against our own usage — and specification/v2_auth_hardening.md already uses SEP-2207 for OIDC refresh / offline_access, with a matching e2e test. So grepping the identifier here would have returned two unrelated things.

I replaced it with plain description rather than substituting RFC 8252 §7.3, which is about loopback redirect URIs rather than token-endpoint TLS. Having taken one citation on trust and been wrong, asserting a different one seemed like the same mistake again. The code now says what the check does and names the SDK error class.

Round 1's third finding remains resolved by deletion — strictPort no longer exists.

npm run format + npm run local:gate → EXIT=0, zero failures.

[Copilot]

🟢 Approval recommended

Runtime behavior is thoroughly handled and tested; remaining findings are non-blocking documentation and regression-test refinements.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts:25

• This test does not check the invariant in its title: name and tokenEndpoint would remain unchanged if a future SDK release made this class an OAuthError, so the test would still pass and the terminal-only policy would not be revisited as the comment intends. Add an assertion against the actual OAuth error hierarchy/brand (or another property unique to that hierarchy).

• Files reviewed: 14/14 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Review round 3 — addressed

🟢 verdict, one inline nit — and one substantive finding in the Suppressed comments block, which is the sixth time on these PRs that the block held the more interesting item. Worth saying out loud for anyone skimming: expanding that <details> is not optional.

The nit: a doubled "terminal terminal", collateral from the round-2 SEP label removal.

The suppressed finding, which was a fair hit. My test was titled "is not an OAuthError, which is why the retry path must not claim it" and asserted only name and typeof tokenEndpoint. Neither touches the OAuth hierarchy. So if a future SDK reparented the class, the test would keep passing while its title became false — and the comment promising that "#2280 handling should be revisited if that changes" would never fire. A test that has quietly stopped testing its subject is worse than no test, because it reads as coverage.

It now asserts both OAuthError.isInstance(err) and instanceof. Verified by mutation rather than assumed: flipping the assertion to simulate the reparenting fails exactly this test and nothing else — 1 failed, 15 passed — so it detects the regression it claims to, and no other test was already covering it.

npm run format + npm run local:gate → EXIT=0, zero failures.

[Copilot]

🟢 Approval recommended

The behavior is correctly implemented and comprehensively tested; remaining feedback is non-blocking documentation and maintainability cleanup.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

docs/test-servers.md:464

• Use “an” before /etc/hosts.

• Files reviewed: 14/14 changed files
• Comments generated: 2
• Review effort level: Balanced

[cliffhall]

Review round 4 — both addressed, plus the suppressed nit

🟢 verdict, two non-blocking findings, and the first is the one worth reading.

I asserted an invariant I had not built. My round-2 comment said a single wrapper "stops a future path from omitting the banner clear". True inside useOAuthRecovery only — useConnectionLifecycle hand-wrote the notice-plus-scoped-clear pair three times. So the exact failure this PR spent two rounds fixing was still one careless edit away, in the change that claimed it could not be.

There is now one reportTerminalInsecureTokenEndpoint in lib/, both hooks route through it, and zero hand-written pairs remain. It is generic over the banner shape rather than importing ReAuthBannerState, because that type lives in useOAuthRecovery.ts which already imports this module — naming it would close a cycle, and all the helper needs is a serverId.

The stale header, I deleted rather than corrected. It said "the three OAuth failure paths"; there are seven. Updating the number just resets the same clock — and this comment rotted within a single PR review, which is about as clear a signal as you get that the list should not exist. It now describes the contract, with an explicit line on why callers are not enumerated.

Also fixed the suppressed a /etc/hosts → an /etc/hosts.

npm run format + npm run local:gate → EXIT=0, zero failures.

[Copilot]

🔵 Needs a closer look

The classifier documentation and test incorrectly claim structured-clone support, and the notice helper retains conflicting JSDoc.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts:56

• This test constructs a plain object directly, so it does not verify the stated serialization behavior. In particular, a real structured clone loses both the custom name and tokenEndpoint; use an actual JSON round trip to test the fallback that works, and avoid claiming structured-clone support.
  core/auth/insecureTokenEndpoint.ts:42
• The structured-clone claim is inaccurate: structuredClone() normalizes a custom Error subclass to Error and drops custom fields such as tokenEndpoint, so this classifier cannot recognize that result. Narrow this contract to the JSON-serialization fallback that the implementation actually supports; otherwise future callers may rely on a boundary that silently defeats the classifier.

clients/web/src/lib/insecureTokenEndpointNotice.ts:39

• This leftover JSDoc block describes only showing the notice, but it now sits immediately before the report-and-clear helper and duplicates the accurate block below. Remove it so generated/source documentation has one unambiguous contract.

/**
 * Show the terminal notice when `err` is the SDK's `InsecureTokenEndpointError`.
 *
 * @returns `true` when it was handled (the caller should stop), `false` when
 * `err` is some other failure and the caller's normal handling applies.
 */

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Review round 5 — all three addressed

Zero inline comments; three findings in Suppressed comments, two of them substantive. That block has now carried the more interesting item on seven occasions across these PRs.

The structured-clone claim was false, and I had been repeating it since round 1. I documented the name fallback as surviving "a structured clone or a JSON hop". Verified:

structuredClone(new InsecureTokenEndpointError(url))
  -> name: "Error", tokenEndpoint: undefined, constructor: Error

structuredClone normalizes a custom Error subclass back to Error, so neither arm has anything left to match. A caller routing this through structuredClone — or postMessage, which uses it — would silently get the generic retryable handling back. That is precisely the defect this PR exists to remove, documented as a supported boundary.

My test could not have caught it, which is the part worth dwelling on: it hand-built a plain look-alike object, so it asserted what I believed the boundary does rather than what it does. It now performs a real JSON round trip and asserts the prototype is gone, and a second case pins that structuredClone is not recognized — converting a false claim into a tested limitation. The doc gives the remedy: serialize the fields explicitly across such a boundary.

The third finding was an orphaned JSDoc block my round-4 refactor stranded above the new helper; the lower-level function has its own contract back.

npm run format + npm run local:gate → EXIT=0, zero failures.

[Copilot]

🔵 Needs a closer look

Connection-time refusals can still emit an observable error status that opens the failure UI before teardown.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/hooks/useConnectionLifecycle.ts:656

• connect() has already synchronously dispatched statusChange("error") before this catch runs (core/mcp/inspectorClient.ts:2437-2440). Awaiting disconnect() cannot retract that transition; while close() is awaited, useInspectorClient can commit the error state and InspectorView permanently sets monitorPinned (InspectorView.tsx:599-614), also briefly painting the card red. The new test mocks connect() without emitting status, so it does not exercise this behavior. Suppress the error transition inside InspectorClient.connect() for this classified refusal (then let this teardown settle it as disconnected), and cover the real status sequence.

This issue also appears on line 699 of the same file.

clients/web/src/hooks/useConnectionLifecycle.ts:703

• The satisfied-challenge retry has the same observable status leak: the second client.connect() dispatches "error" before throwing this refusal, so disconnecting in this catch can occur only after InspectorView has observed and pinned the failure UI. Route this refusal through a non-error connect status path in InspectorClient rather than relying on a later disconnect to hide an event already emitted.

            // The terminal token-endpoint refusal (#2280). The retried `connect()` above can raise the
            // terminal refusal on its own — a satisfied challenge still ends in
            // a token exchange — and reporting that as a failed connect attempt
            // is doubly wrong here: the card goes red and the message is the
            // raw SDK text, on the one path where the Inspector had just told

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Review round 6 — verified, and declined for scope

Zero inline comments; the finding is in Suppressed comments. I checked its throw site rather than accepting or dismissing it, because the closing note on #2284 specifically warned that the EMA reasoning does not transfer here. It splits:

Common path — no leak, and no change warranted. With no stored tokens isOAuthAuthorized() is false, so authProvider is deliberately omitted (inspectorClient.ts ~2093, with a comment saying why: connect should surface a plain 401 rather than have the SDK open a browser before the callback server is listening). The refusal then arrives from authenticate() — a separate method that never touches this.status — so there is no transition to retract. Same outcome as the EMA arm, by a different mechanism.

Narrow path — the finding is correct. With stored tokens authProvider is attached, the SDK transport refreshes inside client.connect(transport), and that call sits inside the try at 2174. InsecureTokenEndpointError is neither AuthRecoveryRequiredError nor a 401, so isConnectAuthRecoveryError is false and the catch at 2437 dispatches statusChange("error"). A later disconnect() cannot retract it, and monitorPinned is useLocalStorage, so it persists.

Reaching that requires stored tokens plus a now-unusable token endpoint — configuration drift, or an issuer changed after a successful authorization. Worth noting the fixture in this PR cannot reproduce it, since it refuses every exchange and so never stores tokens.

Why I am not fixing it here

The proposed remedy is to suppress the error transition inside InspectorClient.connect(). That is a change to connect-status semantics in core/, shared by web, CLI and TUI across 15 connect() call sites, and it works by leaving the status at "connecting" until a caller settles it — the hazard the existing isConnectAuthRecoveryError comment already documents, where a caller that forgets leaves the toggle spinning while holding the active-server lock.

Trading a leak that needs configuration drift to reach for a new failure mode reachable from any client is a bad exchange, and this PR is a messaging fix: replace a Re-authenticate button that cannot work with a message that says what to do. That is delivered, tested and green. Expanding it into core connection semantics is exactly the scope creep that took the predecessor PR to 24 rounds.

Recorded here rather than acted on, deliberately. If it is ever seen in practice, this comment has the analysis and the precise precondition to reproduce it.

[Copilot]

🟢 Approval recommended

The terminal-error behavior is consistently implemented, scoped safely across servers, and comprehensively tested.

Review details

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The terminal error is consistently classified, surfaced without retry affordances, and thoroughly covered across affected paths.

Review details

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Balanced