diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 69438655..13e04fad 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -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 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(); - // 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(); + + // 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, diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index f2025854..e5424a11 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -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({