From 46fabeaf4df5ddd67a947e6a7082f52662917184 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 24 Aug 2026 20:40:46 -0400 Subject: [PATCH] fix(web): keep EMA out of the callback flag, and cover the two untested arms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- clients/web/src/App.test.tsx | 84 +++++++++++++++++++++++++++++++++++- clients/web/src/App.tsx | 20 ++++++--- 2 files changed, 96 insertions(+), 8 deletions(-) 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