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