diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx
index 23808833..dfa0f352 100644
--- a/clients/web/src/App.test.tsx
+++ b/clients/web/src/App.test.tsx
@@ -1589,3 +1589,56 @@ 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 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).
+ window.history.replaceState(
+ {},
+ "",
+ `/oauth/callback?code=abc123&state=${"a".repeat(64)}`,
+ );
+
+ renderWithMantine();
+
+ await waitFor(() => expect(notificationsMock.show).toHaveBeenCalled());
+ 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..176f6fcf 100644
--- a/clients/web/src/App.tsx
+++ b/clients/web/src/App.tsx
@@ -1472,10 +1472,15 @@ 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.
+ //
+ // 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.
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 +1489,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; rejecting to prevent a cross-site request. 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(
"",
);