modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

refactor: one shared reporter for the terminal refusal (review round 4)

Browse source 
In round 2 I claimed a single wrapper "stops a future path omitting the banner
clear". That was only true inside useOAuthRecovery. useConnectionLifecycle
hand-wrote the notice-plus-scoped-clear pair three times, so the exact failure
mode I said was impossible was one careless edit away — in the PR that claimed
it.

reportTerminalInsecureTokenEndpoint now lives in lib/ and both hooks route
through it; zero hand-written pairs remain. It is generic over the banner shape
rather than importing ReAuthBannerState, since useOAuthRecovery already imports
this module and naming its type would close a cycle. All it needs is a serverId
to compare.

The helper's header also still said "the three OAuth failure paths". There are
seven. Rather than update the count I removed the enumeration and recorded why:
a list of callers is a comment that rots on the next round, which is what just
happened to it.

Also fixes "a /etc/hosts" -> "an /etc/hosts".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-07 10:59:56 -04:00
commit c2d566ad8d
4 changed files with 91 additions and 44 deletions
Download patch file Download diff file Expand all files Collapse all files

51
clients/web/src/hooks/useConnectionLifecycle.ts
View file

@ -30,7 +30,7 @@ import {
getActiveEnterpriseManagedAuthIdp,
} from "@inspector/core/client/types.js";
import { isEmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js";
import { showInsecureTokenEndpointNotice } from "../lib/insecureTokenEndpointNotice";
import { reportTerminalInsecureTokenEndpoint } from "../lib/insecureTokenEndpointNotice";
import { findInsecureTokenEndpoint } from "@inspector/core/auth/insecureTokenEndpoint.js";
import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
import type { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js";
@ -654,14 +654,12 @@ export function useConnectionLifecycle({
// arm below already disconnects for the same reason.
if (findInsecureTokenEndpoint(err)) {
await client.disconnect().catch(() => {});
showInsecureTokenEndpointNotice(err, target.name);
// Clear a banner left by an earlier failure: its Re-authenticate
// button is just as dead as the one this arm declines to offer, and
// the user cannot tell which failure it belongs to. Scoped to this
// server, so an async continuation cannot erase another's.
setReAuthBanner((prev) =>
prev && prev.serverId === id ? null : prev,
);
reportTerminalInsecureTokenEndpoint({
err,
serverId: id,
serverName: target.name,
setReAuthBanner,
});
return;
}
@ -706,12 +704,14 @@ export function useConnectionLifecycle({
// the user the authorization *worked*. Placed after the teardown
// above, which this arm needs for the same reason the generic one
// does, and before the flag it must not set.
if (showInsecureTokenEndpointNotice(recoveryErr, target.name)) {
// Only this server's banner — an async continuation must not
// erase one raised for a server the user has since switched to.
setReAuthBanner((prev) =>
prev && prev.serverId === id ? null : prev,
);
if (
reportTerminalInsecureTokenEndpoint({
err: recoveryErr,
serverId: id,
serverName: target.name,
setReAuthBanner,
})
) {
return;
}
setFailedServerId(id);
@ -767,10 +767,14 @@ export function useConnectionLifecycle({
}
// See the note on the handshake arm above (#2280). The
// disconnect already happened at the top of this catch.
if (showInsecureTokenEndpointNotice(authErr, target.name)) {
setReAuthBanner((prev) =>
prev && prev.serverId === id ? null : prev,
);
if (
reportTerminalInsecureTokenEndpoint({
err: authErr,
serverId: id,
serverName: target.name,
setReAuthBanner,
})
) {
return;
}
// The connect attempt failed, same as any other handshake error —
@ -1021,7 +1025,14 @@ export function useConnectionLifecycle({
// worst place to lose the guidance, since they have just been told
// retrying is the fix. No banner clear is needed: this callback
// already cleared it before starting.
if (showInsecureTokenEndpointNotice(err, server?.name)) {
if (
reportTerminalInsecureTokenEndpoint({
err,
serverId,
serverName: server?.name,
setReAuthBanner,
})
) {
return;
}
const message = err instanceof Error ? err.message : String(err);

24
clients/web/src/hooks/useOAuthRecovery.ts
View file

@ -31,7 +31,7 @@ import {
emaStepUpSuccessMessage,
} from "@inspector/core/auth/oauthUx.js";
import { isEmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js";
import { showInsecureTokenEndpointNotice } from "../lib/insecureTokenEndpointNotice";
import { reportTerminalInsecureTokenEndpoint as reportTerminalInsecureTokenEndpointNotice } from "../lib/insecureTokenEndpointNotice";
import type { OAuthDetails } from "../components/groups/ConnectionInfoContent/ConnectionInfoContent";
import { oauthDetailsFromConnectionState } from "../components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState";
import { getWebRemoteOAuthStorage } from "../lib/remoteOAuthStorage";
@ -408,21 +408,13 @@ export function useOAuthRecovery({
err: unknown,
serverId: string | undefined,
serverName?: string,
): boolean => {
if (!showInsecureTokenEndpointNotice(err, serverName)) {
return false;
}
// Clear only *this* server's banner. The command and deferred-resume
// paths are asynchronous, so server A can reject long after the user
// switched away and server B raised a banner of its own; an unconditional
// clear would then erase B's, which is still valid and still actionable.
// Functional so it sees the queued state rather than the render-time
// value, matching how `setPendingReauth` guards its own late restore.
setReAuthBanner((prev) =>
prev && prev.serverId === serverId ? null : prev,
);
return true;
},
): boolean =>
reportTerminalInsecureTokenEndpointNotice({
err,
serverId,
serverName,
setReAuthBanner,
}),
[setReAuthBanner],
);

58
clients/web/src/lib/insecureTokenEndpointNotice.ts
View file

@ -1,17 +1,18 @@
/**
* Surfaces the SDK's terminal token-endpoint refusal to post credentials to a non-TLS token
* endpoint as the terminal configuration error it is (#2280).
* Surfaces the SDK's refusal to post credentials to a non-TLS token endpoint as
* the terminal configuration error it is (#2280).
*
* Lives in `lib/` rather than `utils/` because showing a notification is a side
* effect; the copy it renders is pure and lives in `@inspector/core/auth`.
*
* Shaped as a claim-or-decline predicate rather than a plain `show(...)` so the
* three OAuth failure paths that need it — the connect handshake, the post-
* redirect callback, and the re-auth banner funnel — can each spend one line on
* it and keep their existing fall-through intact:
* Shaped as a claim-or-decline predicate rather than a plain `show(...)` so
* every OAuth failure path that needs it can spend one line and keep its own
* fall-through intact. Deliberately not enumerating the callers here: they have
* gone from three to seven over this PR's review, and a list is a comment that
* rots on the next one.
*
* ```ts
* if (showInsecureTokenEndpointNotice(err, server.name)) return;
* if (reportTerminalInsecureTokenEndpoint({ err, serverId: id, serverName, setReAuthBanner })) return;
* ```
*
* `autoClose: false` matches the other non-recoverable OAuth notices (issuer
@ -22,6 +23,7 @@
* reading. Don't describe this as non-dismissible.
*/
import type { Dispatch, SetStateAction } from "react";
import { notifications } from "@mantine/notifications";
import { findInsecureTokenEndpoint } from "@inspector/core/auth/insecureTokenEndpoint.js";
import {
@ -35,6 +37,48 @@ import {
* @returns `true` when it was handled (the caller should stop), `false` when
* `err` is some other failure and the caller's normal handling applies.
*/
/**
* Report the refusal **and** clear the re-auth banner for that server.
*
* This is the form every caller should use. The notice and the scoped banner
* clear are one invariant, not two steps: a banner left behind carries a
* Re-authenticate button as dead as the one this declines to offer, and the
* user cannot tell which failure it belongs to. Keeping them together in one
* place is what stops a future path doing half of it — an earlier revision made
* that claim while hand-writing the pair at three call sites in a second hook,
* which is exactly how it goes wrong.
*
* The clear is scoped to `serverId` and applied as a functional update: these
* paths are asynchronous, so a late continuation for server A must not erase a
* banner server B raised in the meantime.
*
* Generic over the banner shape rather than importing `ReAuthBannerState`:
* `useOAuthRecovery` already imports this module, so naming its type here would
* close a cycle. All this needs is a `serverId` to compare.
*
* @returns `true` when it was handled (the caller should stop), `false` when
* `err` is some other failure and the caller's normal handling applies.
*/
export function reportTerminalInsecureTokenEndpoint<
TBanner extends { serverId: string },
>({
err,
serverId,
serverName,
setReAuthBanner,
}: {
err: unknown;
serverId: string | undefined;
serverName?: string;
setReAuthBanner: Dispatch<SetStateAction<TBanner | null>>;
}): boolean {
if (!showInsecureTokenEndpointNotice(err, serverName)) {
return false;
}
setReAuthBanner((prev) => (prev && prev.serverId === serverId ? null : prev));
return true;
}
export function showInsecureTokenEndpointNotice(
err: unknown,
serverName?: string,

2
docs/test-servers.md
View file

@ -461,7 +461,7 @@ The value now rides the normalized `AuthChallenge` as a string — it has to be
⚠️ **Do not run this fixture while port 8091 is already taken.** Like every fixture here it walks to the next free port on `EADDRINUSE` — but its `issuerUrl` is a *fixed string*, so a relocated server announces 8092 while all its OAuth metadata still points at whatever unrelated process holds 8091. The symptom is confusing rather than obvious: discovery reaches the wrong process, and the refusal this fixture exists to demonstrate either never fires or fires for the wrong reason. If the flow does not end at the notice described below, check that 8091 is actually this server (`lsof -nP -iTCP:8091 -sTCP:LISTEN`) before believing anything you see.
The trailing dot is the whole trick, and it is doing real work rather than being a curiosity. `localhost.` is the *root-anchored* spelling of `localhost`: every resolver on the machine sends it to the loopback interface, so the fixture is reachable and the flow runs for real — but the SDK's `assertSecureTokenEndpoint` exempts only the three literals `localhost`, `127.0.0.1` and `::1`, and `localhost.` is none of them. So the credential-carrying request is refused with `InsecureTokenEndpointError` while everything else about the server works. It is the same over-narrow exemption that makes `http://tenant.app.localhost:3300` fail ([#1944](https://github.com/modelcontextprotocol/inspector/issues/1944), [typescript-sdk#2591](https://github.com/modelcontextprotocol/typescript-sdk/issues/2591)), reproducible without a `/etc/hosts` entry or dnsmasq.
The trailing dot is the whole trick, and it is doing real work rather than being a curiosity. `localhost.` is the *root-anchored* spelling of `localhost`: every resolver on the machine sends it to the loopback interface, so the fixture is reachable and the flow runs for real — but the SDK's `assertSecureTokenEndpoint` exempts only the three literals `localhost`, `127.0.0.1` and `::1`, and `localhost.` is none of them. So the credential-carrying request is refused with `InsecureTokenEndpointError` while everything else about the server works. It is the same over-narrow exemption that makes `http://tenant.app.localhost:3300` fail ([#1944](https://github.com/modelcontextprotocol/inspector/issues/1944), [typescript-sdk#2591](https://github.com/modelcontextprotocol/typescript-sdk/issues/2591)), reproducible without an `/etc/hosts` entry or dnsmasq.
Add the server, click **Connect**, and complete the authorization. The redirect comes back with a code, the Inspector goes to exchange it, and the SDK refuses.