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

@@ -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<Crypto, "randomUUID" | "getRandomValues">
-        >
-      )[name];
+      delete (globalThis.crypto as Partial<Pick<Crypto, "randomUUID">>)[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");
-    let token: string | undefined;
-    withoutCryptoMember("randomUUID", () => {
-      withoutCryptoMember("getRandomValues", () => {
-        token = writeOAuthResumeSnapshot({
-          version: 1,
-          serverId: "a",
-          activeTab: "tools",
-          authKind: "reauth",
-          tabUi: {},
-        });
-      });
+    // `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 {
+      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);

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