fix: type-discriminate progress toast ids, CSPRNG attempt-id fallback

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uw1s4LRBUzrFwLzPT4mAJn
Signed-off-by: cliffhall <cliff@futurescale.com>

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<string, unknown>)[name];
+    }
+  }
+}
+
 describe("oauthResume", () => {
   const storage = new Map<string, string>();
 
@@ -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,

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)}`;
 }
 

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);
   });
 });
 

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: "<message> — <progress> / <total> (NN%)". The fraction