From 8ed0bed086937c2feeba0deff05a6a1c75144eb1 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 15:53:34 -0400 Subject: [PATCH] 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) Claude-Session: https://claude.ai/code/session_01Uw1s4LRBUzrFwLzPT4mAJn Signed-off-by: cliffhall --- clients/web/src/lib/oauthResume.test.ts | 60 +++++++++++++++---------- clients/web/src/lib/oauthResume.ts | 12 ++--- 2 files changed, 43 insertions(+), 29 deletions(-) diff --git a/clients/web/src/lib/oauthResume.test.ts b/clients/web/src/lib/oauthResume.test.ts index ce68c60e..835dc799 100644 --- a/clients/web/src/lib/oauthResume.test.ts +++ b/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 - > - )[name]; + delete (globalThis.crypto as Partial>)[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); diff --git a/clients/web/src/lib/oauthResume.ts b/clients/web/src/lib/oauthResume.ts index b0ebf13b..235840f0 100644 --- a/clients/web/src/lib/oauthResume.ts +++ b/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)}`;