diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index dfa0f352..1d1c42f9 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -404,6 +404,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ })); import App from "./App"; +import { OAUTH_PENDING_SERVER_KEY } from "./utils/oauthFlow"; import * as McpIndex from "@inspector/core/mcp/index.js"; import * as FetchLogModule from "@inspector/core/mcp/state/fetchRequestLogState.js"; import { useManagedRequestorTasks } from "@inspector/core/react/useManagedRequestorTasks.js"; @@ -1625,9 +1626,16 @@ describe("App OAuth callback state validation", () => { }); it("does not reject when the state param parses to a valid authId", async () => { - // A well-formed 64-char-hex state is accepted; the handler proceeds past - // the rejection guard (it fails later for other reasons in this harness, - // but never shows the 'OAuth callback rejected' toast). + // A well-formed 64-char-hex state passes the shape guard, so the handler + // proceeds to the server-matching step. With no server registered under the + // seeded pending id, that step surfaces the "could not be matched" toast — + // asserting on that specific downstream toast proves the state was accepted + // (never the "OAuth callback rejected" toast) rather than relying on an + // indirect "some toast fired" check. + window.sessionStorage.setItem( + OAUTH_PENDING_SERVER_KEY, + "server-that-does-not-exist", + ); window.history.replaceState( {}, "", @@ -1636,7 +1644,13 @@ describe("App OAuth callback state validation", () => { renderWithMantine(); - await waitFor(() => expect(notificationsMock.show).toHaveBeenCalled()); + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "OAuth callback could not be matched", + }), + ), + ); expect(notificationsMock.show).not.toHaveBeenCalledWith( expect.objectContaining({ title: "OAuth callback rejected" }), ); diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 176f6fcf..01f977df 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1473,10 +1473,20 @@ function App() { // rebuilt client can restore those `auth` entries. Read it before the // URL is cleared below. // - // CSRF hardening: a `state` that is present but does not parse to our - // expected 64-char-hex authId shape is a forgery indicator — the value - // did not originate from `generateOAuthState`. Reject the callback with a - // clear error instead of silently proceeding with an undefined sessionId. + // Defense-in-depth: a `state` that is present but does not parse to our + // expected 64-char-hex authId shape did not originate from + // `generateOAuthState`, so reject the callback instead of silently + // proceeding with an undefined sessionId. This is a shape check, not full + // state-matching — the primary CSRF protection remains PKCE + // (`code_verifier`); this layer catches malformed/forged `state` early. + // + // Intentional asymmetry: a present-but-malformed `state` (including the + // empty string `?state=`) is rejected, but a wholly absent `state` + // (`stateParam === null`) is *not* — it falls through with + // `sessionId = undefined` and is matched via `OAUTH_PENDING_SERVER_KEY` + // instead. Rejecting the null case would turn any provider error redirect + // that omits `state` into a misleading "rejected" toast, hiding the real + // OAuth error surfaced by the `!params.successful` branch below. const stateParam = new URLSearchParams(window.location.search).get("state"); const parsedState = stateParam ? parseOAuthState(stateParam) : null; const stateRejected = stateParam !== null && parsedState === null; @@ -1493,7 +1503,7 @@ function App() { notifications.show({ title: "OAuth callback rejected", message: - "OAuth callback carried an unrecognized state parameter; rejecting to prevent a cross-site request. Please try connecting again.", + "OAuth callback carried an unrecognized state parameter that did not originate from this session. Please try connecting again.", color: "red", }); return;