diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx
index 23808833..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";
@@ -1589,3 +1590,69 @@ describe("App history pin/replay", () => {
);
});
});
+
+// The `/oauth/callback` handler must reject a returned `state` that does not
+// parse to the expected 64-char-hex authId shape (a forgery indicator) instead
+// of silently proceeding. See #1562.
+describe("App OAuth callback state validation", () => {
+ const originalUrl = window.location.href;
+
+ beforeEach(() => {
+ notificationsMock.show.mockClear();
+ window.sessionStorage.clear();
+ });
+
+ afterEach(() => {
+ window.history.replaceState({}, "", originalUrl);
+ });
+
+ it("rejects an unparseable state param with a clear error toast", async () => {
+ window.history.replaceState(
+ {},
+ "",
+ "/oauth/callback?code=abc123&state=not-a-valid-state",
+ );
+
+ renderWithMantine();
+
+ await waitFor(() =>
+ expect(notificationsMock.show).toHaveBeenCalledWith(
+ expect.objectContaining({
+ title: "OAuth callback rejected",
+ color: "red",
+ }),
+ ),
+ );
+ });
+
+ it("does not reject when the state param parses to a valid authId", async () => {
+ // 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(
+ {},
+ "",
+ `/oauth/callback?code=abc123&state=${"a".repeat(64)}`,
+ );
+
+ renderWithMantine();
+
+ 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 a08dedbc..01f977df 100644
--- a/clients/web/src/App.tsx
+++ b/clients/web/src/App.tsx
@@ -1472,10 +1472,25 @@ function App() {
// session key the pre-redirect page saved the fetch log under, so the
// rebuilt client can restore those `auth` entries. Read it before the
// URL is cleared below.
+ //
+ // 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 sessionId = stateParam
- ? (parseOAuthState(stateParam)?.authId ?? undefined)
- : undefined;
+ const parsedState = stateParam ? parseOAuthState(stateParam) : null;
+ const stateRejected = stateParam !== null && parsedState === null;
+ const sessionId = parsedState?.authId ?? undefined;
const pendingId =
window.sessionStorage.getItem(OAUTH_PENDING_SERVER_KEY) ?? undefined;
window.sessionStorage.removeItem(OAUTH_PENDING_SERVER_KEY);
@@ -1484,6 +1499,16 @@ function App() {
// the (now single-use) authorization code through the exchange again.
window.history.replaceState({}, "", "/");
+ if (stateRejected) {
+ notifications.show({
+ title: "OAuth callback rejected",
+ message:
+ "OAuth callback carried an unrecognized state parameter that did not originate from this session. Please try connecting again.",
+ color: "red",
+ });
+ return;
+ }
+
if (!params.successful) {
notifications.show({
title: "OAuth authorization failed",
diff --git a/clients/web/src/test/core/auth/utils.test.ts b/clients/web/src/test/core/auth/utils.test.ts
index 712b95ee..08189dca 100644
--- a/clients/web/src/test/core/auth/utils.test.ts
+++ b/clients/web/src/test/core/auth/utils.test.ts
@@ -1,4 +1,4 @@
-import { describe, it, expect } from "vitest";
+import { describe, it, expect, afterEach } from "vitest";
import {
parseHttpUrl,
parseOAuthCallbackParams,
@@ -51,6 +51,17 @@ describe("parseOAuthCallbackParams", () => {
});
describe("generateOAuthState", () => {
+ const originalCrypto = globalThis.crypto;
+
+ afterEach(() => {
+ // Restore the real WebCrypto after tests that stub/remove it.
+ Object.defineProperty(globalThis, "crypto", {
+ configurable: true,
+ writable: true,
+ value: originalCrypto,
+ });
+ });
+
it("should generate 64-char hex state", () => {
const state = generateOAuthState();
expect(state).toMatch(/^[a-f0-9]{64}$/i);
@@ -61,6 +72,31 @@ describe("generateOAuthState", () => {
const s2 = generateOAuthState();
expect(s1).not.toBe(s2);
});
+
+ it("throws when the crypto global is entirely absent (no silent Math.random fallback)", () => {
+ // Simulate an exotic runtime with no WebCrypto at all. Rather than minting
+ // a guessable CSRF token, generateOAuthState must fail loudly.
+ Object.defineProperty(globalThis, "crypto", {
+ configurable: true,
+ writable: true,
+ value: undefined,
+ });
+ expect(() => generateOAuthState()).toThrow(
+ /crypto\.getRandomValues is not available/,
+ );
+ });
+
+ it("throws when crypto.getRandomValues is missing", () => {
+ // crypto exists but lacks getRandomValues — still refuse to degrade.
+ Object.defineProperty(globalThis, "crypto", {
+ configurable: true,
+ writable: true,
+ value: {},
+ });
+ expect(() => generateOAuthState()).toThrow(
+ /crypto\.getRandomValues is not available/,
+ );
+ });
});
describe("parseOAuthState", () => {
diff --git a/core/auth/utils.ts b/core/auth/utils.ts
index 638a4080..123274f8 100644
--- a/core/auth/utils.ts
+++ b/core/auth/utils.ts
@@ -50,20 +50,17 @@ export const parseOAuthCallbackParams = (location: string): CallbackParams => {
* @returns A random state for the OAuth 2.0 flow.
*/
export const generateOAuthState = (): string => {
- // Generate a random state
- const array = new Uint8Array(32);
-
- // Use crypto.getRandomValues (available in both browser and Node.js)
- if (typeof crypto !== "undefined" && crypto.getRandomValues) {
- crypto.getRandomValues(array);
- } else {
- // Fallback for environments without crypto.getRandomValues
- // This should not happen in modern environments
- for (let i = 0; i < array.length; i++) {
- array[i] = Math.floor(Math.random() * 256);
- }
+ // OAuth state is a CSRF token — it MUST be unpredictable. crypto.getRandomValues
+ // is available in every supported runtime (browsers, Node ≥15); if it's somehow
+ // missing, fail loudly rather than silently degrading to Math.random (whose
+ // output is predictable from a small amount of observed state).
+ if (typeof crypto === "undefined" || !crypto.getRandomValues) {
+ throw new Error(
+ "crypto.getRandomValues is not available; refusing to generate an OAuth state with a non-cryptographic RNG.",
+ );
}
-
+ const array = new Uint8Array(32);
+ crypto.getRandomValues(array);
return Array.from(array, (byte) => byte.toString(16).padStart(2, "0")).join(
"",
);