v2.5.0 merge review: progress-toast id collisions, and a CSPRNG-capable fallback for newAttemptId #2216

Activity

1. cliffhall

   added this to the v2.6.0 milestone on Sep 1, 2026
2. cliffhall

   added

   bugSomething isn't workingSomething isn't working

   v2Issues and PRs for v2Issues and PRs for v2

   on Sep 1, 2026
3. cliffhall

   assigned

   cliffhall

   and unassigned

   cliffhall

   on Sep 1, 2026
4. ump45nose commented on Sep 3, 2026

   

   ump45nose

   on Sep 3, 2026

   More actions

   Reproduced both findings locally and verified the fix. Per this repo's issues-only policy I'm not opening a PR — here is the prompt, the before/after, and how I verified it.

   1. progressToastId collision

   ProgressToken is string | number (the SDK schema is z.union([z.string(), z.number().int()])), so String(token) erases the type and the numeric 7 collides with the string "7"; the ?? "default" sentinel also collides with a genuine string token "default". Confirmed against the shipped source.

   Fix — encode the type (and absence) into the id so no two distinct streams can share one:

   export function progressToastId(token: ProgressToken | undefined): string {
     if (token === undefined) {
       return "progress-u-undefined";
     }
     return typeof token === "number"
       ? `progress-n-${token}`
       : `progress-s-${token}`;
   }

   useProgressToasts.ts calls progressToastId(...) rather than building ids itself, so no other call site changes — only the two test expectations that asserted the old progress-abc / progress-7 / progress-default strings.

   2. newAttemptId CSPRNG fallback

   crypto.randomUUID needs a secure context; crypto.getRandomValues does not. The exact situation the fallback exists for (a file:// page or plain-HTTP non-loopback host) is one where a CSPRNG is still available, so the current Math.random fallback declines to use it. Verified that getRandomValues is on the Crypto global independently of randomUUID.

   Fix — prefer getRandomValues before the Math.random last resort:

   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 = new Uint32Array(2);
       getRandomValues(bytes);
       return `${bytes[0].toString(36)}${bytes[1].toString(36)}`;
     }
     return `${Date.now().toString(36)}-${Math.random().toString(36).slice(2)}`;
   }

   The "never a security token" doc comment is kept and expanded — it is still the reason this is a Medium, not urgent.

   Verification

   • Added collision tests to progressToasts.test.ts: 7 vs "7" and undefined vs "default" must not share an id.
   • Added a crypto-less test to oauthResume.test.ts driving Math.random via a spy, plus the existing "randomUUID unavailable" test now exercises the getRandomValues branch.
   • npx vitest run --project=unit src/utils/toasts/progressToasts.test.ts src/hooks/useProgressToasts.test.tsx src/lib/oauthResume.test.ts → 55 passed.
   • eslint --max-warnings 0 and prettier --check pass on all touched files.

   AI assistance disclosure: AI was used to discover this opportunity and draft the change or text. The submission was checked against the prepared artifact and recorded verification evidence.
5. cliffhall

   self-assigned this

   on Sep 5, 2026
6. cliffhall

   linked a pull request that will close this issuefix: type-discriminate progress toast ids, and a CSPRNG fallback for newAttemptId #2259on Sep 5, 2026
7. cliffhall

   closed this as completedon Sep 5, 2026