diff --git a/clients/web/src/lib/oauthResume.test.ts b/clients/web/src/lib/oauthResume.test.ts index 51b11c9f7..835dc7993 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` 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", 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 Partial>)[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,39 +487,64 @@ 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", { + // 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(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", () => { + // 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; try { - const token = writeOAuthResumeSnapshot({ + 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; + Object.defineProperty(globalThis, "crypto", original); } } - expect(globalThis.crypto.randomUUID).toEqual(expect.any(Function)); + 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", () => { diff --git a/clients/web/src/lib/oauthResume.ts b/clients/web/src/lib/oauthResume.ts index 145171080..235840f0f 100644 --- a/clients/web/src/lib/oauthResume.ts +++ b/clients/web/src/lib/oauthResume.ts @@ -227,12 +227,28 @@ 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(); } + // 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)}`; } 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