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

[cliffhall]

Two findings from the v2.5.0 milestone-merge review (#2215). Both are in code that shipped on v2/main during the milestone, so neither belongs in the merge PR — that PR's tree is byte-identical to origin/v2/main and its whole verification argument rests on that identity. This is the same handling #2000 → #2092 got after the v2.2.0 merge review.

1. progressToastId collides across distinct progress streams

clients/web/src/utils/toasts/progressToasts.ts:

export function progressToastId(token: ProgressToken | undefined): string {
  return `progress-${String(token ?? "default")}`;
}

ProgressToken is string | number, so String(token) erases the type: the numeric token 7 and the string token "7" produce the same id. The absent case is worse — it hardcodes the sentinel "default", which a server is free to send as a genuine string token.

Because notifications keyed by the same id are replaced rather than stacked (that is the point of the id), a collision means two concurrent progress streams overwrite each other's toast: one stream's ticks silently retitle the other's, and when the first finishes its auto-close takes the survivor with it.

Fix: encode the token's type and its absence in the id — e.g. progress-n-7 / progress-s-7 / a sentinel that no String(token) can produce. Add collision cases to progressToasts.test.ts covering 7 vs "7" and undefined vs "default".

Reported by Copilot on #2215.

2. newAttemptId's insecure-context fallback can use crypto.getRandomValues

clients/web/src/lib/oauthResume.ts:

function newAttemptId(): string {
  const uuid = globalThis.crypto?.randomUUID?.bind(globalThis.crypto);
  if (uuid) return uuid();
  return `${Date.now().toString(36)}-${Math.random().toString(36).slice(2)}`;
}

CodeQL flags this as js/insecure-randomness (alert 72), tracing the value to its use as resumeSnapshot?.remoteSessionId in useOAuthRecovery.ts.

The alert overstates it. The doc comment above the function is accurate: an attempt id only has to be unique among the handful of redirect attempts one page can have in flight, it is never presented as a bearer credential, and the Math.random branch is a fallback taken only where crypto.randomUUID is unavailable — a file:// page or a plain-HTTP non-loopback host.

But the fallback is improvable on its own merits, independent of the alert. randomUUID needs a secure context; crypto.getRandomValues does not, and is present in every browser that has crypto at all. So the exact situation the fallback exists for is one where a CSPRNG is still available and we decline to use it. Switching to getRandomValues costs a couple of lines, keeps the same id shape, and retires the alert honestly rather than by dismissing it.

Keep the Math.random branch as the last resort for a crypto-less global, and keep the "never a security token" comment — it is the reason this is Medium and not urgent.

Not actionable

CodeQL alert 73, js/missing-rate-limiting on test-servers/src/test-server-oauth.ts's /oauth/revoke handler. The test servers are local, single-user fixtures whose entire purpose is to be driven by the Inspector on loopback; rate-limiting them would make several smokes slower and some of them flaky, and would defend nothing. Dismiss it on the alert rather than tracking it here.