From f5a3ccedbbf69497698786f7bb58dc0a596ff9c7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 13:36:30 -0400 Subject: [PATCH] fix: type-discriminate progress toast ids, CSPRNG attempt-id fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings from the v2.5.0 milestone-merge review (#2215). 1. `progressToastId` collided across distinct progress streams. `ProgressToken` is `string | number`, so `String(token)` mapped the numeric token 7 and the string token "7" onto one id — and because notifications keyed by the same id are *replaced* rather than stacked, two concurrent streams overwrote each other's toast. The absent case was worse: it hardcoded the sentinel "default", which a server is free to send as a genuine string token. The id now carries the token's type (`progress-n:7` / `progress-s:7`) and gives the no-token case a prefix of its own (`progress-none`) that no token value can produce. 2. `newAttemptId`'s fallback now prefers `crypto.getRandomValues`. `randomUUID` needs a secure context; `getRandomValues` does not and exists in every browser that has `crypto` at all — so the exact situation the fallback exists for (a `file://` page, a plain-HTTP non-loopback host) still has a CSPRNG on hand. `Math.random` stays as the last resort for a `crypto`-less global, and the "never a security token" comment stays too. Retires CodeQL alert 72 (`js/insecure-randomness`) honestly rather than by dismissing it. Tests: collision cases for 7 vs "7" and the absent token vs "default" / "none", a distinctness sweep over the whole id space; and the attempt-id fallback test split into a getRandomValues arm (asserting the CSPRNG is used and `Math.random` is not) and a no-crypto-at-all arm. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uw1s4LRBUzrFwLzPT4mAJn Signed-off-by: cliffhall --- clients/web/src/lib/oauthResume.test.ts | 90 +++++++++++++------ clients/web/src/lib/oauthResume.ts | 14 +++ .../src/utils/toasts/progressToasts.test.ts | 31 ++++++- .../web/src/utils/toasts/progressToasts.ts | 15 +++- 4 files changed, 121 insertions(+), 29 deletions(-) diff --git a/clients/web/src/lib/oauthResume.test.ts b/clients/web/src/lib/oauthResume.test.ts index 51b11c9f..aa624204 100644 --- a/clients/web/src/lib/oauthResume.test.ts +++ b/clients/web/src/lib/oauthResume.test.ts @@ -26,6 +26,34 @@ import { EMPTY_NETWORK_UI, } from "../components/screens/screenUiState.js"; +/** + * Run `body` with one `crypto` member hidden, then restore it. + * + * `randomUUID` and `getRandomValues` are inherited from `Crypto.prototype`, so + * there is normally no OWN descriptor to put back — restoring only when one + * existed would leave the `undefined` own property in place and force every + * later test in this file onto the fallback path. + */ +function withoutCryptoMember( + name: "randomUUID" | "getRandomValues", + body: () => void, +): void { + const original = Object.getOwnPropertyDescriptor(globalThis.crypto, name); + Object.defineProperty(globalThis.crypto, name, { + configurable: true, + value: undefined, + }); + try { + body(); + } finally { + if (original) { + Object.defineProperty(globalThis.crypto, name, original); + } else { + delete (globalThis.crypto as Record)[name]; + } + } +} + describe("oauthResume", () => { const storage = new Map(); @@ -44,6 +72,9 @@ describe("oauthResume", () => { afterEach(() => { vi.unstubAllGlobals(); + // This project does not set `restoreMocks` globally (see `src/test/setup.ts`), + // so the `crypto`/`Math.random` spies below must be reverted by hand. + vi.restoreAllMocks(); }); it("consumeOAuthResumeSnapshot reads once then clears storage", () => { @@ -456,41 +487,50 @@ describe("oauthResume", () => { expect(readOAuthResumeSnapshot()?.attemptId).toBe(token); }); - it("writeOAuthResumeSnapshot falls back when randomUUID is unavailable", () => { + it("writeOAuthResumeSnapshot falls back to getRandomValues without randomUUID", () => { // `crypto.randomUUID` needs a secure context, which a plain-HTTP - // non-loopback host is not. - const original = Object.getOwnPropertyDescriptor( - globalThis.crypto, - "randomUUID", - ); - Object.defineProperty(globalThis.crypto, "randomUUID", { - configurable: true, - value: undefined, - }); - try { - const token = writeOAuthResumeSnapshot({ + // non-loopback host is not. `crypto.getRandomValues` does not, so the + // fallback is still a CSPRNG rather than `Math.random`. + const randomSpy = vi.spyOn(globalThis.crypto, "getRandomValues"); + const mathSpy = vi.spyOn(Math, "random"); + let token: string | undefined; + withoutCryptoMember("randomUUID", () => { + token = writeOAuthResumeSnapshot({ version: 1, serverId: "a", activeTab: "tools", authKind: "reauth", tabUi: {}, }); - expect(token).toEqual(expect.any(String)); - expect(clearOwnOAuthResumeSnapshot(token)).toBe(true); - } finally { - // `randomUUID` is inherited from `Crypto.prototype`, so there is - // normally no OWN descriptor to put back — restoring only when one - // existed would leave the `undefined` own property in place and force - // every later test in this file onto the fallback path. - if (original) { - Object.defineProperty(globalThis.crypto, "randomUUID", original); - } else { - delete (globalThis.crypto as { randomUUID?: unknown }).randomUUID; - } - } + }); + expect(randomSpy).toHaveBeenCalledOnce(); + expect(mathSpy).not.toHaveBeenCalled(); + // 16 random bytes, hex-encoded. + expect(token).toMatch(/^[0-9a-f]{32}$/); + expect(clearOwnOAuthResumeSnapshot(token)).toBe(true); expect(globalThis.crypto.randomUUID).toEqual(expect.any(Function)); }); + it("writeOAuthResumeSnapshot falls back to Math.random with no crypto at all", () => { + const mathSpy = vi.spyOn(Math, "random"); + let token: string | undefined; + withoutCryptoMember("randomUUID", () => { + withoutCryptoMember("getRandomValues", () => { + token = writeOAuthResumeSnapshot({ + version: 1, + serverId: "a", + activeTab: "tools", + authKind: "reauth", + tabUi: {}, + }); + }); + }); + expect(mathSpy).toHaveBeenCalled(); + expect(token).toEqual(expect.any(String)); + expect(clearOwnOAuthResumeSnapshot(token)).toBe(true); + expect(globalThis.crypto.getRandomValues).toEqual(expect.any(Function)); + }); + it("clearOAuthResumeSnapshot swallows removeItem failures", () => { vi.stubGlobal("sessionStorage", { getItem: () => null, diff --git a/clients/web/src/lib/oauthResume.ts b/clients/web/src/lib/oauthResume.ts index 14517108..b0ebf13b 100644 --- a/clients/web/src/lib/oauthResume.ts +++ b/clients/web/src/lib/oauthResume.ts @@ -227,12 +227,26 @@ export function writeOAuthResumeSnapshot( * context, which a `file://` page or a plain-HTTP non-loopback host is not), * and otherwise a value that only has to be unique among the handful of * redirect attempts one page can have in flight — never a security token. + * + * The fallback chain matters even so. `crypto.getRandomValues` is *not* + * gated on a secure context and exists in every browser that has `crypto` at + * all — so precisely the situation `randomUUID` is unavailable in still has a + * CSPRNG on hand, and declining to use it would be a gratuitous downgrade + * (CodeQL `js/insecure-randomness`, alert 72, flags exactly that). `Math.random` + * survives only as the last resort for a `crypto`-less global. */ function newAttemptId(): string { const uuid = globalThis.crypto?.randomUUID?.bind(globalThis.crypto); if (uuid) { return uuid(); } + const getRandomValues = globalThis.crypto?.getRandomValues?.bind( + globalThis.crypto, + ); + if (getRandomValues) { + const bytes = getRandomValues(new Uint8Array(16)); + return Array.from(bytes, (b) => b.toString(16).padStart(2, "0")).join(""); + } return `${Date.now().toString(36)}-${Math.random().toString(36).slice(2)}`; } diff --git a/clients/web/src/utils/toasts/progressToasts.test.ts b/clients/web/src/utils/toasts/progressToasts.test.ts index 1acdff1d..500e549c 100644 --- a/clients/web/src/utils/toasts/progressToasts.test.ts +++ b/clients/web/src/utils/toasts/progressToasts.test.ts @@ -13,12 +13,37 @@ describe("PROGRESS_TOAST_AUTOCLOSE_MS", () => { describe("progressToastId", () => { it("keys by the progress token", () => { - expect(progressToastId("abc")).toBe("progress-abc"); - expect(progressToastId(7)).toBe("progress-7"); + expect(progressToastId("abc")).toBe("progress-s:abc"); + expect(progressToastId(7)).toBe("progress-n:7"); }); it("shares one id when the server sends no token", () => { - expect(progressToastId(undefined)).toBe("progress-default"); + expect(progressToastId(undefined)).toBe("progress-none"); + }); + + it("does not collide a numeric token with the same-looking string token", () => { + expect(progressToastId(7)).not.toBe(progressToastId("7")); + }); + + it("does not collide the absent token with any token a server can send", () => { + const absent = progressToastId(undefined); + for (const token of ["default", "none", "", "0"]) { + expect(progressToastId(token)).not.toBe(absent); + } + expect(progressToastId(0)).not.toBe(absent); + }); + + it("gives each distinct token its own id", () => { + const ids = [ + progressToastId(undefined), + progressToastId(0), + progressToastId(7), + progressToastId("0"), + progressToastId("7"), + progressToastId("none"), + progressToastId("default"), + ]; + expect(new Set(ids).size).toBe(ids.length); }); }); diff --git a/clients/web/src/utils/toasts/progressToasts.ts b/clients/web/src/utils/toasts/progressToasts.ts index 4b007f09..1796811c 100644 --- a/clients/web/src/utils/toasts/progressToasts.ts +++ b/clients/web/src/utils/toasts/progressToasts.ts @@ -11,8 +11,21 @@ export const PROGRESS_TOAST_AUTOCLOSE_MS = 5000; // rather than flooding the corner. The injected `progressToken` correlates a // stream with the request that triggered it; when absent (the common case — // the inspector doesn't expose a caller token), all ticks share one toast. +// +// The token's *type* is part of the key. `ProgressToken` is `string | number`, +// so a bare `String(token)` maps the number 7 and the string "7" — two +// distinct streams per the spec — onto one id, and the two streams then +// overwrite each other's toast (id collision means replacement, which is the +// whole point of the id). The `n:`/`s:` discriminator keeps them apart, and +// the no-token case gets a prefix of its own rather than the sentinel +// `"default"`, which a server is free to send as a genuine string token. export function progressToastId(token: ProgressToken | undefined): string { - return `progress-${String(token ?? "default")}`; + if (token === undefined) { + return "progress-none"; + } + return typeof token === "number" + ? `progress-n:${token}` + : `progress-s:${token}`; } // One-line toast body: " — / (NN%)". The fraction