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

fix(web): open the monitoring sidebar when a connect fails during OAuth (#2108)

Browse source 
The failure column was gated on the `"error"` connection status, which an
OAuth failure never reaches: the parent drives the auth legs itself and tears
the client down on failure, so the session settles at `"disconnected"`. The
sidebar therefore stayed shut on exactly the failure the Network tab exists to
explain — the case #2048 was closed on.

Gate it on `erroredServerId` instead, the parent's canonical "this server's
last connect attempt failed" flag, and set that flag on every OAuth
connect-attempt failure: the pre-redirect discovery/DCR leg, the callback
leg's token exchange, an OAuth-storage load failure, and a provider that
redirects back with an error instead of a code.

Nothing else was needed to make the requests reachable. `auth` fetches are
already tracked into the Network store, which does not clear on disconnect and
is restored from the session on the callback leg, so discovery, DCR and the
token exchange are all present. Tab selection stays content-gated rather than
hard-coded, so Network leads because it is the one that captured something.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-24 19:51:11 -04:00
commit f7224144ec
4 changed files with 211 additions and 3 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -54,6 +54,10 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
// 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;
// And for `authenticate()`, the pre-redirect OAuth leg (discovery + DCR), so a
// test can exercise a failure there — the case that never reaches the "error"
// connection status (#2108).
let nextAuthenticateRejection: unknown = null;
class FakeInspectorClient extends EventTarget {
connect = vi.fn(() => {
if (nextConnectRejection !== null) {
@ -108,6 +112,14 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
}
return Promise.resolve(undefined);
});
authenticate = vi.fn(() => {
if (nextAuthenticateRejection !== null) {
const err = nextAuthenticateRejection;
nextAuthenticateRejection = null;
return Promise.reject(err);
}
return Promise.resolve(undefined);
});
checkAuthChallengeSatisfied = vi.fn().mockResolvedValue(true);
clearOAuthTokens = vi.fn().mockResolvedValue(undefined);
}
@ -129,6 +141,10 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
__rejectNextResumeAfterOAuth: (err: unknown) => {
nextResumeRejection = err;
},
// Test-only: arm the next authenticate() to reject (pre-redirect OAuth leg).
__rejectNextAuthenticate: (err: unknown) => {
nextAuthenticateRejection = err;
},
};
});
@ -692,6 +708,10 @@ const rejectNextResumeAfterOAuth = (
}
).__rejectNextResumeAfterOAuth;
const rejectNextAuthenticate = (
McpIndex as unknown as { __rejectNextAuthenticate: (err: unknown) => void }
).__rejectNextAuthenticate;
const fetchLogInstances = (
FetchLogModule as unknown as { __fetchLogInstances: EventTarget[] }
).__fetchLogInstances;
@ -716,6 +736,33 @@ describe("App failed-connection card border (#1621)", () => {
);
});
// An OAuth leg that fails never reaches the `"error"` connection status — the
// handler tears the client down, leaving the session `"disconnected"` — so the
// flag is the only signal downstream that a connect attempt died. Without it
// the monitoring sidebar stays shut on exactly the failure the Network tab
// exists to explain (#2108).
it("flags the server when the OAuth authorization leg fails (#2108)", async () => {
const user = userEvent.setup();
renderWithMantine(<App />);
expect(screen.getByTestId("errored-server")).toHaveTextContent("none");
// 401 from the server -> App starts the authorization-code flow, whose
// discovery/DCR round-trip then fails (e.g. an invalid
// `/.well-known/oauth-authorization-server`).
rejectNextConnect(Object.assign(new Error("HTTP 401"), { status: 401 }));
rejectNextAuthenticate(new Error("discovery 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("OAuth authorization failed"),
}),
);
});
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.
@ -2454,6 +2501,41 @@ describe("App OAuth callback issuer-binding failures (#1808)", () => {
expect(screen.queryByText("Re-authentication required")).toBeNull();
});
// The callback leg is a connect attempt too, so a failure there must flag the
// server — that is what opens the monitoring sidebar onto the OAuth requests
// (discovery, DCR and the token exchange, restored from the pre-redirect
// session) rather than leaving the user with only a banner (#2108).
it("flags the server when the callback leg fails (#2108)", async () => {
renderCallbackWithFailure(new Error("token exchange rejected"));
await waitFor(() =>
expect(screen.getByTestId("errored-server")).toHaveTextContent("A"),
);
});
// 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).
it("flags the server when the provider returns an error to the callback (#2108)", async () => {
writeOAuthResumeSnapshot({
version: 1,
serverId: "A",
activeTab: "Tools",
authKind: "reauth",
tabUi: {},
});
window.history.replaceState(
{},
"",
`${OAUTH_CALLBACK_PATH}?error=access_denied`,
);
renderWithMantine(<App />);
await waitFor(() =>
expect(screen.getByTestId("errored-server")).toHaveTextContent("A"),
);
});
it("clears the stale OAuth state and reconnects when the affordance is used", async () => {
const user = userEvent.setup();
renderCallbackWithFailure(

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

@ -2841,6 +2841,7 @@ function App() {
if (!params.successful) {
const pendingId = resumeSnapshot?.serverId;
if (pendingId) {
setFailedServerId(pendingId);
queueMicrotask(() => {
showReAuthBanner(pendingId, generateOAuthErrorDescription(params));
});
@ -2876,6 +2877,7 @@ function App() {
await webOAuthStorage.load();
} catch (err) {
connectStartRef.current = undefined;
setFailedServerId(server.id);
queueMicrotask(() => {
showReAuthBanner(server.id, err instanceof Error ? err : String(err));
});
@ -2891,6 +2893,13 @@ 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}"`,
@ -3091,6 +3100,13 @@ function App() {
});
return;
}
// The connect attempt failed, same as any other handshake error —
// flag the card (#1621) and, with it, open the monitoring sidebar
// onto the OAuth requests that explain the failure (#2108). This
// leg never reaches the `"error"` connection status (the
// `disconnect()` above settles it at `"disconnected"`), so this
// flag is the only signal the view has that a connect attempt died.
setFailedServerId(id);
const message =
authErr instanceof Error ? authErr.message : String(authErr);
setConnectErrorMessage(message);

81
clients/web/src/components/views/InspectorView/InspectorView.test.tsx
View file

@ -1689,6 +1689,87 @@ describe("InspectorView", () => {
expect(screen.queryByRole("radio", { name: "Console" })).toBeNull();
});
// An OAuth failure is torn down by the parent, so the session rests at
// "disconnected" rather than "error" — the status gate the failure column
// used to carry never saw it, and the requests explaining the failure were
// unreachable (#2108). The parent's `erroredServerId` is the signal instead.
it("opens the monitoring sidebar to Network when OAuth fails without an error status (#2108)", async () => {
const { rerender } = renderWithMantine(
<StatefulInspectorViewHost
{...makeProps({
servers: [httpServer],
activeServer: "beta",
connectionStatus: "connecting",
})}
/>,
);
expect(
screen.queryByRole("button", { name: "Close monitoring sidebar" }),
).toBeNull();
rerender(
<StatefulInspectorViewHost
{...makeProps({
servers: [httpServer],
// The failure's teardown clears the active server and settles the
// session at "disconnected" — never "error".
activeServer: undefined,
erroredServerId: "beta",
connectionStatus: "disconnected",
initializeResult: undefined,
network: [
{
id: "a1",
timestamp: new Date(),
method: "GET",
url: "https://as.example.com/.well-known/oauth-authorization-server",
requestHeaders: {},
responseStatus: 404,
responseStatusText: "Not Found",
category: "auth",
},
],
})}
/>,
);
expect(
await screen.findByRole("button", { name: "Close monitoring sidebar" }),
).toBeInTheDocument();
expect(screen.getByRole("radio", { name: "Network" })).toBeChecked();
});
it("does not re-open the sidebar on a mount that starts already flagged (#2108)", () => {
// Same props as the failure above, but as the *initial* render: there is
// no transition, so a user who closed the column isn't fought on remount.
renderWithMantine(
<StatefulInspectorViewHost
{...makeProps({
servers: [httpServer],
activeServer: undefined,
erroredServerId: "beta",
connectionStatus: "disconnected",
initializeResult: undefined,
network: [
{
id: "a1",
timestamp: new Date(),
method: "GET",
url: "https://as.example.com/.well-known/oauth-authorization-server",
requestHeaders: {},
responseStatus: 404,
responseStatusText: "Not Found",
category: "auth",
},
],
})}
/>,
);
expect(
screen.queryByRole("button", { name: "Close monitoring sidebar" }),
).toBeNull();
});
it("surfaces Console (stderr), not Network, in the failure column for a stdio server (#1621)", async () => {
const stdioErr: ServerEntry = {
id: "beta",

35
clients/web/src/components/views/InspectorView/InspectorView.tsx
View file

@ -388,6 +388,13 @@ export interface InspectorViewProps {
* the Servers screen draws a red border until another server is connected or
* a new connection is attempted. Independent of `activeServer`, which the
* parent clears on the failure's `disconnect` event.
*
* It is also the sole signal that opens the monitoring sidebar onto the
* failure's diagnostics (#2108). That used to be gated on the `"error"`
* connection status, which an OAuth failure never reaches — the parent drives
* that leg and tears the client down itself, settling the session at
* `"disconnected"`. So the parent must set this for *every* connect-attempt
* failure, auth legs included, and clear it as each new attempt starts.
*/
erroredServerId?: string;
/**
@ -817,18 +824,33 @@ export function InspectorView({
// connecting/disconnected → error, never connected → error. This keeps the
// auto-open aligned with the red-border (`erroredServerId`), which the parent
// also sets only for connect-attempt failures.
//
// A connect attempt that fails *during OAuth* (#2108) never reaches the
// `"error"` status at all: the auth leg is driven from the parent, which
// tears the client down on failure, so the session settles at
// `"disconnected"`. So the failure arm also fires on the parent raising
// `erroredServerId` — the canonical "this server's last connect attempt
// failed" flag, which the parent clears at the start of every new attempt.
// Tracked through a ref for the same reason as the status: a mount that
// starts already flagged must not re-open a column the user closed.
const prevStatusRef = useRef(connectionStatus);
const prevErroredServerIdRef = useRef(erroredServerId);
useEffect(() => {
const prev = prevStatusRef.current;
prevStatusRef.current = connectionStatus;
const prevErrored = prevErroredServerIdRef.current;
prevErroredServerIdRef.current = erroredServerId;
const becameConnected =
connectionStatus === "connected" && prev !== "connected";
const becameError =
connectionStatus === "error" && prev !== "error" && prev !== "connected";
(connectionStatus === "error" &&
prev !== "error" &&
prev !== "connected") ||
(erroredServerId !== undefined && prevErrored !== erroredServerId);
if (becameConnected || becameError) {
setMonitorPinned(true);
}
}, [connectionStatus, setMonitorPinned]);
}, [connectionStatus, erroredServerId, setMonitorPinned]);
const appTools = useMemo<Tool[]>(() => {
return tools.filter((tool) => {
@ -924,7 +946,14 @@ export function InspectorView({
// (set by the parent only for connect-attempt failures, not mid-session
// crashes) so a crash of a previously-connected server doesn't reorganize the
// column into the failure tab set — matching the auto-open effect above.
const failed = connectionStatus === "error" && erroredServerId !== undefined;
//
// Keyed on `erroredServerId` alone rather than on the `"error"` status
// (#2108): an OAuth leg that fails is torn down by the parent, so the session
// rests at `"disconnected"` and a status gate would never see the failure —
// which is exactly the case a server developer needs the Network tab for.
// `!connected` is the only status condition left, and it holds vacuously
// whenever the flag is set (the parent clears it as each new attempt starts).
const failed = !connected && erroredServerId !== undefined;
const monitorAvailable = useMemo<string[]>(() => {
if (connected) return availableTabs.filter((t) => MONITOR_TABS.includes(t));
if (failed) {