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

fix(web): guard the auth-recovery retry, and say what the banner-only arms do

Browse source 
Review round 1.

The satisfied-challenge arm re-connects from inside the outer `catch` with two
unguarded awaits, so a rejection there escaped `onToggleConnection` entirely —
an unhandled rejection with no toast, no red border and no sidebar, which is
this issue's failure mode in its most invisible form. Surface it as the failed
connect attempt it is. A throw from `checkAuthChallengeSatisfied` lands there
too rather than falling through to the redirect: it is not the same as the
challenge being unsatisfied, and navigating the page away on the strength of an
error would bury it.

The provider-error and OAuth-storage-load arms return before a client is
rebuilt, so they raise the red border and deliberately not the sidebar — the
diagnostic is the provider's `error` param (already in the banner) or a local
storage failure, neither of which is in the network log. Said so at both sites
and on the test, which asserts the flag and nothing more on purpose.

Also drops the three `as unknown as` casts on the test-only arming hooks for a
single intersection cast that keeps the module's real shape checked, and turns
the armed connect rejection into a FIFO queue so a test can arm the two
consecutive connects the recovery retry makes.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-24 20:23:26 -04:00
commit 1998724634
2 changed files with 98 additions and 23 deletions
Download patch file Download diff file Expand all files Collapse all files

82
clients/web/src/App.test.tsx
View file

@ -13,6 +13,7 @@ import {
act,
} from "./test/renderWithMantine";
import userEvent from "@testing-library/user-event";
import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
// Spy on the toast layer so the progress-notification tests can assert the
// show/update calls without mounting Mantine's <Notifications/> portal.
@ -48,9 +49,13 @@ const { messageLogClear } = vi.hoisted(() => ({ messageLogClear: vi.fn() }));
vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
const actual =
await importOriginal<typeof import("@inspector/core/mcp/index.js")>();
// When set, the next `connect()` rejects with this value (one-shot), so a test
// can exercise the handshake-failure path. Cleared after it fires.
let nextConnectRejection: unknown = null;
// Each armed value makes one `connect()` reject, in FIFO order, so a test can
// exercise the handshake-failure path — or a two-connect sequence such as the
// auth-recovery retry, where the first call rejects with the recovery error
// and the retry behind it fails too. A queue rather than a single slot: with
// one slot the second arming would overwrite the first, and the retry would
// silently succeed.
const connectRejections: unknown[] = [];
// Same one-shot arming for the `/oauth/callback` token exchange, so a test can
// exercise the callback-leg failure paths (#1808).
let nextResumeRejection: unknown = null;
@ -60,10 +65,8 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
let nextAuthenticateRejection: unknown = null;
class FakeInspectorClient extends EventTarget {
connect = vi.fn(() => {
if (nextConnectRejection !== null) {
const err = nextConnectRejection;
nextConnectRejection = null;
return Promise.reject(err);
if (connectRejections.length > 0) {
return Promise.reject(connectRejections.shift());
}
return Promise.resolve(undefined);
});
@ -134,8 +137,9 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
// Test-only handle so the test can grab the live instance and fire events.
__clientInstances: instances,
// Test-only: arm the next connect() to reject (handshake-failure path).
// Call it more than once to arm consecutive calls.
__rejectNextConnect: (err: unknown) => {
nextConnectRejection = err;
connectRejections.push(err);
},
// Test-only: arm the next resumeAfterOAuth() to reject (callback-leg failure).
__rejectNextResumeAfterOAuth: (err: unknown) => {
@ -698,19 +702,21 @@ const clientInstances = (
McpIndex as unknown as { __clientInstances: EventTarget[] }
).__clientInstances;
const rejectNextConnect = (
McpIndex as unknown as { __rejectNextConnect: (err: unknown) => void }
).__rejectNextConnect;
// The mock factory adds three test-only arming hooks to the module namespace.
// Intersecting with `typeof McpIndex` keeps the real module's shape checked and
// narrows this to a single cast — `as unknown as` would discard the former and
// is what AGENTS.md rules out.
type ArmingHooks = typeof McpIndex & {
__rejectNextConnect: (err: unknown) => void;
__rejectNextResumeAfterOAuth: (err: unknown) => void;
__rejectNextAuthenticate: (err: unknown) => void;
};
const rejectNextResumeAfterOAuth = (
McpIndex as unknown as {
__rejectNextResumeAfterOAuth: (err: unknown) => void;
}
).__rejectNextResumeAfterOAuth;
const rejectNextAuthenticate = (
McpIndex as unknown as { __rejectNextAuthenticate: (err: unknown) => void }
).__rejectNextAuthenticate;
const { __rejectNextConnect: rejectNextConnect } = McpIndex as ArmingHooks;
const { __rejectNextResumeAfterOAuth: rejectNextResumeAfterOAuth } =
McpIndex as ArmingHooks;
const { __rejectNextAuthenticate: rejectNextAuthenticate } =
McpIndex as ArmingHooks;
const fetchLogInstances = (
FetchLogModule as unknown as { __fetchLogInstances: EventTarget[] }
@ -763,6 +769,36 @@ describe("App failed-connection card border (#1621)", () => {
);
});
// The auth-recovery arm re-connects from inside the outer `catch`, so before
// #2108 a rejection there escaped `onToggleConnection` entirely as an
// unhandled rejection: no toast, no red border, no sidebar.
it("flags the server when the satisfied-challenge retry fails (#2108)", async () => {
const user = userEvent.setup();
renderWithMantine(<App />);
// First connect asks for recovery; the fake reports the challenge already
// satisfied, so App retries the connect — and that retry fails.
rejectNextConnect(
new AuthRecoveryRequiredError(
new URL("https://as.example.com/authorize"),
{
reason: "unauthorized",
},
),
);
rejectNextConnect(new Error("handshake failed after re-authorization"));
await user.click(screen.getByText("connect"));
await waitFor(() =>
expect(screen.getByTestId("errored-server")).toHaveTextContent("A"),
);
expect(notificationsMock.show).toHaveBeenCalledWith(
expect.objectContaining({
title: expect.stringContaining("Failed to connect"),
}),
);
});
it("clears the flag when a new connection attempt starts", async () => {
// Report status "error" so the *second* connect click is treated as a fresh
// attempt (not a disconnect of a live session), exercising the clear.
@ -2516,6 +2552,12 @@ describe("App OAuth callback issuer-binding failures (#1808)", () => {
// The other callback arm: the provider redirected back with an error instead
// of a code, so no token exchange is even attempted. Still a connect attempt
// that failed, so the server carries the same flag (#2108).
//
// The flag is deliberately all this asserts. That arm returns before a client
// is rebuilt, so no Network entries are restored and the content-gated column
// stays shut — which is right here: the provider's `error` param is the whole
// diagnostic and the re-auth banner is already showing it. Asserting a
// sidebar would be asserting behavior this arm should not have.
it("flags the server when the provider returns an error to the callback (#2108)", async () => {
writeOAuthResumeSnapshot({
version: 1,

39
clients/web/src/App.tsx
View file

@ -2841,6 +2841,12 @@ function App() {
if (!params.successful) {
const pendingId = resumeSnapshot?.serverId;
if (pendingId) {
// Red border only (#1621), not a sidebar. This arm returns before a
// client is rebuilt, so the persisted `auth` entries are never
// restored and the content-gated column stays shut — correctly: the
// provider's own `error` param is the whole diagnostic, and the
// re-auth banner below is already showing it. The flag is still right,
// because the attempt did fail.
setFailedServerId(pendingId);
queueMicrotask(() => {
showReAuthBanner(pendingId, generateOAuthErrorDescription(params));
@ -2877,6 +2883,9 @@ function App() {
await webOAuthStorage.load();
} catch (err) {
connectStartRef.current = undefined;
// Red border only, for the same reason as the provider-error arm above
// — and here the network log would not help anyway: this is a failure
// to read local OAuth storage, not a request that went out.
setFailedServerId(server.id);
queueMicrotask(() => {
showReAuthBanner(server.id, err instanceof Error ? err : String(err));
@ -3057,9 +3066,33 @@ function App() {
// against the right client. The redirect unloads this page, so there's
// nothing to do after the await on the success path.
if (err instanceof AuthRecoveryRequiredError) {
if (await client.checkAuthChallengeSatisfied(err.authChallenge)) {
connectStartRef.current = Date.now();
await client.connect();
try {
if (await client.checkAuthChallengeSatisfied(err.authChallenge)) {
connectStartRef.current = Date.now();
await client.connect();
return;
}
} catch (recoveryErr) {
// Both awaits above are unguarded connect work sitting inside a
// `catch`, so a rejection escapes `onToggleConnection` altogether:
// no toast, no red border, no sidebar — the #2108 failure mode in
// its most invisible form. Surface it as the failed connect attempt
// it is. A throw from `checkAuthChallengeSatisfied` lands here too
// rather than falling through to `prepareOAuthRedirect`: it is not
// the same as the challenge being *unsatisfied*, and navigating the
// whole page away on the strength of an error would bury it.
connectStartRef.current = undefined;
setFailedServerId(id);
const message =
recoveryErr instanceof Error
? recoveryErr.message
: String(recoveryErr);
setConnectErrorMessage(message);
notifications.show({
title: `Failed to connect to "${target.name}"`,
message,
color: "red",
});
return;
}
prepareOAuthRedirect({