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): keep EMA out of the callback flag, and cover the two untested arms

Browse source 
Review round 2.

The flag in the resume catch sat above the EMA guard, so an unconfigured
enterprise client was marked a failed connect — which contradicts the round-1
argument for leaving EMA unflagged in the two connect-path arms. Moved below
the guard so all three agree, and still above the classification fan-out so
every arm that is a real failure carries it.

Adds the two tests the round flagged as missing: an OAuth storage `load()`
that rejects (spied on `RemoteOAuthStorage.prototype`, so every other test
keeps the real storage), and a *rejecting* challenge check — the distinct
control-flow decision introduced last round, where a broken check is surfaced
rather than treated as an unsatisfied challenge and used as grounds to navigate
the page away. That one asserts no resume snapshot was written, which is what
`prepareOAuthRedirect` would have done first.

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

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

@ -14,6 +14,7 @@ import {
} from "./test/renderWithMantine";
import userEvent from "@testing-library/user-event";
import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
import { RemoteOAuthStorage } from "@inspector/core/auth/remote/storage-remote.js";
// Spy on the toast layer so the progress-notification tests can assert the
// show/update calls without mounting Mantine's <Notifications/> portal.
@ -63,6 +64,10 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
// test can exercise a failure there — the case that never reaches the "error"
// connection status (#2108).
let nextAuthenticateRejection: unknown = null;
// And for the auth-recovery challenge check, whose *rejection* is a distinct
// control-flow decision from its resolving `false` (#2108): a throw is
// surfaced as a failed attempt rather than falling through to the redirect.
let nextChallengeCheckRejection: unknown = null;
class FakeInspectorClient extends EventTarget {
connect = vi.fn(() => {
if (connectRejections.length > 0) {
@ -123,7 +128,14 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
}
return Promise.resolve(undefined);
});
checkAuthChallengeSatisfied = vi.fn().mockResolvedValue(true);
checkAuthChallengeSatisfied = vi.fn(() => {
if (nextChallengeCheckRejection !== null) {
const err = nextChallengeCheckRejection;
nextChallengeCheckRejection = null;
return Promise.reject(err);
}
return Promise.resolve(true);
});
clearOAuthTokens = vi.fn().mockResolvedValue(undefined);
}
const instances: FakeInspectorClient[] = [];
@ -149,6 +161,10 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
__rejectNextAuthenticate: (err: unknown) => {
nextAuthenticateRejection = err;
},
// Test-only: arm the next checkAuthChallengeSatisfied() to reject.
__rejectNextChallengeCheck: (err: unknown) => {
nextChallengeCheckRejection = err;
},
};
});
@ -710,6 +726,7 @@ type ArmingHooks = typeof McpIndex & {
__rejectNextConnect: (err: unknown) => void;
__rejectNextResumeAfterOAuth: (err: unknown) => void;
__rejectNextAuthenticate: (err: unknown) => void;
__rejectNextChallengeCheck: (err: unknown) => void;
};
const { __rejectNextConnect: rejectNextConnect } = McpIndex as ArmingHooks;
@ -717,6 +734,8 @@ const { __rejectNextResumeAfterOAuth: rejectNextResumeAfterOAuth } =
McpIndex as ArmingHooks;
const { __rejectNextAuthenticate: rejectNextAuthenticate } =
McpIndex as ArmingHooks;
const { __rejectNextChallengeCheck: rejectNextChallengeCheck } =
McpIndex as ArmingHooks;
const fetchLogInstances = (
FetchLogModule as unknown as { __fetchLogInstances: EventTarget[] }
@ -799,6 +818,37 @@ describe("App failed-connection card border (#1621)", () => {
);
});
// The other half of the guarded arm. A *rejecting* challenge check is not the
// same as one that resolves `false`: the latter means "re-authorize", the
// former means the check itself broke, so it is surfaced rather than used as
// grounds to navigate the whole page to the auth server.
it("surfaces a rejecting challenge check instead of redirecting (#2108)", async () => {
const user = userEvent.setup();
renderWithMantine(<App />);
rejectNextConnect(
new AuthRecoveryRequiredError(
new URL("https://as.example.com/authorize"),
{ reason: "unauthorized" },
),
);
rejectNextChallengeCheck(new Error("challenge check failed"));
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"),
message: "challenge check failed",
}),
);
// No redirect was prepared: `prepareOAuthRedirect` persists a resume
// snapshot before navigating, and nothing wrote one.
expect(readOAuthResumeSnapshot()).toBeUndefined();
});
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.
@ -2549,6 +2599,38 @@ describe("App OAuth callback issuer-binding failures (#1808)", () => {
);
});
// The callback leg can also die before the token exchange, if the persisted
// OAuth state cannot be read back at all. Spied on the prototype rather than
// mocked at the module, so every other test keeps the real storage.
it("flags the server when the OAuth storage fails to load (#2108)", async () => {
const loadSpy = vi
.spyOn(RemoteOAuthStorage.prototype, "load")
.mockRejectedValueOnce(new Error("storage unavailable"));
try {
writeOAuthResumeSnapshot({
version: 1,
serverId: "A",
activeTab: "Tools",
authKind: "reauth",
tabUi: {},
});
window.history.replaceState({}, "", `${OAUTH_CALLBACK_PATH}?code=test`);
renderWithMantine(<App />);
await waitFor(() =>
expect(screen.getByTestId("errored-server")).toHaveTextContent("A"),
);
// Red border only, by design: this arm returns before a client is built,
// so nothing restores the Network log — and a failed local read of
// `oauth.json` would not be in it anyway.
expect(
await screen.findByText(/storage unavailable/),
).toBeInTheDocument();
} finally {
loadSpy.mockRestore();
}
});
// 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).

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

@ -2902,13 +2902,6 @@ function App() {
});
} catch (err) {
connectStartRef.current = undefined;
// The token exchange (or the re-handshake behind it) failed. Flag the
// server (#1621) so the monitoring sidebar opens onto the OAuth
// requests that explain it (#2108) — the rebuilt client restored the
// pre-redirect `auth` fetch entries from the session, so discovery,
// DCR and the token exchange are all there. Set before the
// classification fan-out below so every arm carries it.
setFailedServerId(server.id);
if (isEmaClientNotConfiguredError(err)) {
notifications.show({
title: `Cannot connect to "${server.name}"`,
@ -2918,6 +2911,19 @@ function App() {
});
return;
}
// The token exchange (or the re-handshake behind it) failed. Flag the
// server (#1621) so the monitoring sidebar opens onto the OAuth
// requests that explain it (#2108) — the rebuilt client restored the
// pre-redirect `auth` fetch entries from the session, so discovery,
// DCR and the token exchange are all there.
//
// Below the EMA guard, not above it: an unconfigured enterprise client
// is a *configuration* error rather than a failed attempt, and both
// connect-path arms already return on it without flagging. Flagging it
// only here would make the three disagree about what the red border
// means. Above every other arm, so the classification fan-out that
// follows carries it whichever way it goes.
setFailedServerId(server.id);
// SEP-2352 issuer binding (#1808). Two very different failures share
// one SDK error class, so classify before falling through to the
// generic re-auth banner (whose detail line would otherwise be the raw