From f5a3ccedbbf69497698786f7bb58dc0a596ff9c7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 13:36:30 -0400 Subject: [PATCH 1/3] 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 51b11c9f7..aa6242049 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 145171080..b0ebf13bf 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 1acdff1d1..500e549c0 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 4b007f09f..1796811c5 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 From f48cf2a90ded5c971446267e40c94830b76ff3c3 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 14:54:48 -0400 Subject: [PATCH 2/3] fix: narrow the crypto-member delete cast in the attempt-id tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Record` does not overlap `Crypto` (TS2352 under `tsc -b`). `Partial>` says the same thing in one legal cast and keeps `delete` operating on a known-optional property. 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 | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/clients/web/src/lib/oauthResume.test.ts b/clients/web/src/lib/oauthResume.test.ts index aa6242049..ce68c60ed 100644 --- a/clients/web/src/lib/oauthResume.test.ts +++ b/clients/web/src/lib/oauthResume.test.ts @@ -49,7 +49,11 @@ function withoutCryptoMember( if (original) { Object.defineProperty(globalThis.crypto, name, original); } else { - delete (globalThis.crypto as Record)[name]; + delete ( + globalThis.crypto as Partial< + Pick + > + )[name]; } } } From 8ed0bed086937c2feeba0deff05a6a1c75144eb1 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 15:53:34 -0400 Subject: [PATCH 3/3] fix: call getRandomValues directly, and test true crypto absence Copilot review round 1. - `newAttemptId` calls `globalThis.crypto.getRandomValues(...)` directly rather than through a bound alias. CodeQL's `js/insecure-randomness` browser model recognizes a secure RNG by that literal method call, so the indirection would have left the `Math.random` last resort classified as an unmitigated source. - The no-crypto test arm now removes `globalThis.crypto` entirely instead of hiding its two methods, matching the crypto-absence test in `src/test/core/auth/utils.test.ts`. Hiding only the methods left the global truthy, so the arm could not have caught a regression that read `globalThis.crypto` without the optional guard. `withoutCryptoMember` narrows to `randomUUID`, its only remaining caller. 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 | 58 +++++++++++++++---------- clients/web/src/lib/oauthResume.ts | 12 ++--- 2 files changed, 42 insertions(+), 28 deletions(-) diff --git a/clients/web/src/lib/oauthResume.test.ts b/clients/web/src/lib/oauthResume.test.ts index ce68c60ed..835dc7993 100644 --- a/clients/web/src/lib/oauthResume.test.ts +++ b/clients/web/src/lib/oauthResume.test.ts @@ -29,15 +29,15 @@ import { /** * 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. + * `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. + * + * This hides one *member*; the arm that needs the whole `crypto` global gone + * stubs `globalThis.crypto` itself instead (see below). */ -function withoutCryptoMember( - name: "randomUUID" | "getRandomValues", - body: () => void, -): void { +function withoutCryptoMember(name: "randomUUID", body: () => void): void { const original = Object.getOwnPropertyDescriptor(globalThis.crypto, name); Object.defineProperty(globalThis.crypto, name, { configurable: true, @@ -49,11 +49,7 @@ function withoutCryptoMember( if (original) { Object.defineProperty(globalThis.crypto, name, original); } else { - delete ( - globalThis.crypto as Partial< - Pick - > - )[name]; + delete (globalThis.crypto as Partial>)[name]; } } } @@ -516,19 +512,35 @@ describe("oauthResume", () => { }); it("writeOAuthResumeSnapshot falls back to Math.random with no crypto at all", () => { + // The whole global goes, not just its two methods — an exotic runtime with + // no WebCrypto at all, matching `src/test/core/auth/utils.test.ts`. Hiding + // only the methods would leave `globalThis.crypto` truthy and so would not + // catch a regression that reads it without the optional guard (Copilot). const mathSpy = vi.spyOn(Math, "random"); + // `crypto` IS an own property of the global (unlike the `Crypto.prototype` + // members above), so there is always a descriptor to restore. Asserted + // rather than `!`-ed so a change in that assumption fails loudly here. + const original = Object.getOwnPropertyDescriptor(globalThis, "crypto"); + expect(original).toBeDefined(); + Object.defineProperty(globalThis, "crypto", { + configurable: true, + writable: true, + value: undefined, + }); let token: string | undefined; - withoutCryptoMember("randomUUID", () => { - withoutCryptoMember("getRandomValues", () => { - token = writeOAuthResumeSnapshot({ - version: 1, - serverId: "a", - activeTab: "tools", - authKind: "reauth", - tabUi: {}, - }); + try { + token = writeOAuthResumeSnapshot({ + version: 1, + serverId: "a", + activeTab: "tools", + authKind: "reauth", + tabUi: {}, }); - }); + } finally { + if (original) { + Object.defineProperty(globalThis, "crypto", original); + } + } expect(mathSpy).toHaveBeenCalled(); expect(token).toEqual(expect.any(String)); expect(clearOwnOAuthResumeSnapshot(token)).toBe(true); diff --git a/clients/web/src/lib/oauthResume.ts b/clients/web/src/lib/oauthResume.ts index b0ebf13bf..235840f0f 100644 --- a/clients/web/src/lib/oauthResume.ts +++ b/clients/web/src/lib/oauthResume.ts @@ -240,11 +240,13 @@ function newAttemptId(): string { if (uuid) { return uuid(); } - const getRandomValues = globalThis.crypto?.getRandomValues?.bind( - globalThis.crypto, - ); - if (getRandomValues) { - const bytes = getRandomValues(new Uint8Array(16)); + // Called directly rather than through a bound alias: CodeQL's + // `js/insecure-randomness` browser model recognizes a secure RNG by the + // literal `crypto.getRandomValues(...)` method call, and an alias does not + // match it — so the indirection would leave the `Math.random` last resort + // below classified as an unmitigated source (Copilot). + if (globalThis.crypto?.getRandomValues) { + const bytes = globalThis.crypto.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)}`;