From ae703ab0e7528ae215b6dd8e20dfa9b64f05a11e Mon Sep 17 00:00:00 2001 From: Jacob Tomlinson Date: Mon, 30 Mar 2026 08:57:30 -0700 Subject: [PATCH] infra: harden identifier entropy and delay jitter (#57744) * infra: harden identifier entropy and delay jitter * test: make randomness hardening deterministic in CI --- extensions/tlon/src/monitor/approval.test.ts | 31 +++++++++++++++++ extensions/tlon/src/monitor/approval.ts | 4 ++- src/agents/session-slug.test.ts | 35 ++++++++++++++++--- src/agents/session-slug.ts | 16 +++++++-- src/auto-reply/reply/reply-dispatcher.ts | 3 +- src/auto-reply/reply/reply-flow.test.ts | 5 +-- src/infra/retry.test.ts | 33 ++++++++++++++++++ src/infra/retry.ts | 3 +- src/infra/secure-random.test.ts | 36 +++++++++++++++++++- src/infra/secure-random.ts | 17 ++++++++- ui/src/ui/uuid.test.ts | 3 +- ui/src/ui/uuid.ts | 17 ++------- 12 files changed, 170 insertions(+), 33 deletions(-) create mode 100644 extensions/tlon/src/monitor/approval.test.ts diff --git a/extensions/tlon/src/monitor/approval.test.ts b/extensions/tlon/src/monitor/approval.test.ts new file mode 100644 index 00000000000..69599789d75 --- /dev/null +++ b/extensions/tlon/src/monitor/approval.test.ts @@ -0,0 +1,31 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const cryptoMocks = vi.hoisted(() => ({ + randomBytes: vi.fn(), +})); + +vi.mock("node:crypto", () => ({ + randomBytes: cryptoMocks.randomBytes, +})); + +let generateApprovalId: typeof import("./approval.js").generateApprovalId; + +beforeEach(async () => { + vi.resetModules(); + cryptoMocks.randomBytes.mockReset(); + ({ generateApprovalId } = await import("./approval.js")); +}); + +describe("generateApprovalId", () => { + it("uses secure hex entropy while preserving the ID format", () => { + cryptoMocks.randomBytes.mockReturnValueOnce(Buffer.from("a1b2c3", "hex")); + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(1_717_171_717_171); + + try { + expect(generateApprovalId("dm")).toBe("dm-1717171717171-a1b2c3"); + expect(cryptoMocks.randomBytes).toHaveBeenCalledWith(3); + } finally { + nowSpy.mockRestore(); + } + }); +}); diff --git a/extensions/tlon/src/monitor/approval.ts b/extensions/tlon/src/monitor/approval.ts index 549be04a88a..d9dfcb053f1 100644 --- a/extensions/tlon/src/monitor/approval.ts +++ b/extensions/tlon/src/monitor/approval.ts @@ -5,6 +5,8 @@ * a notification and can approve or deny the request. */ +// Extensions cannot import core internals directly, so use node:crypto here. +import { randomBytes } from "node:crypto"; import type { PendingApproval } from "../settings.js"; export type { PendingApproval }; @@ -32,7 +34,7 @@ export type CreateApprovalParams = { */ export function generateApprovalId(type: ApprovalType): string { const timestamp = Date.now(); - const randomPart = Math.random().toString(36).substring(2, 6); + const randomPart = randomBytes(3).toString("hex"); return `${type}-${timestamp}-${randomPart}`; } diff --git a/src/agents/session-slug.test.ts b/src/agents/session-slug.test.ts index 917ea8ab4de..a227c0f0d67 100644 --- a/src/agents/session-slug.test.ts +++ b/src/agents/session-slug.test.ts @@ -1,26 +1,51 @@ -import { afterEach, describe, expect, it, vi } from "vitest"; -import { createSessionSlug } from "./session-slug.js"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const randomMocks = vi.hoisted(() => ({ + generateSecureInt: vi.fn(), +})); + +let createSessionSlug: typeof import("./session-slug.js").createSessionSlug; + +beforeEach(async () => { + vi.resetModules(); + randomMocks.generateSecureInt.mockReset(); + vi.doMock("../infra/secure-random.js", () => ({ + generateSecureInt: randomMocks.generateSecureInt, + })); + ({ createSessionSlug } = await import("./session-slug.js")); +}); describe("session slug", () => { afterEach(() => { + vi.doUnmock("../infra/secure-random.js"); vi.restoreAllMocks(); }); it("generates a two-word slug by default", () => { - vi.spyOn(Math, "random").mockReturnValue(0); + randomMocks.generateSecureInt.mockReturnValue(0); const slug = createSessionSlug(); expect(slug).toBe("amber-atlas"); }); it("adds a numeric suffix when the base slug is taken", () => { - vi.spyOn(Math, "random").mockReturnValue(0); + randomMocks.generateSecureInt.mockReturnValue(0); const slug = createSessionSlug((id) => id === "amber-atlas"); expect(slug).toBe("amber-atlas-2"); }); it("falls back to three words when collisions persist", () => { - vi.spyOn(Math, "random").mockReturnValue(0); + randomMocks.generateSecureInt.mockReturnValue(0); const slug = createSessionSlug((id) => /^amber-atlas(-\d+)?$/.test(id)); expect(slug).toBe("amber-atlas-atlas"); }); + + it("uses secure fallback suffix entropy when word collisions persist", () => { + randomMocks.generateSecureInt.mockReturnValue(0); + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(1_717_171_717_171); + const slug = createSessionSlug( + (id) => /^amber-atlas(?:-\d+)?$/.test(id) || /^amber-atlas-atlas(?:-\d+)?$/.test(id), + ); + expect(slug).toBe("amber-atlas-atlas-aaa"); + nowSpy.mockRestore(); + }); }); diff --git a/src/agents/session-slug.ts b/src/agents/session-slug.ts index 0aee27a344b..c376b322d4d 100644 --- a/src/agents/session-slug.ts +++ b/src/agents/session-slug.ts @@ -1,3 +1,5 @@ +import { generateSecureInt } from "../infra/secure-random.js"; + const SLUG_ADJECTIVES = [ "amber", "briny", @@ -101,7 +103,17 @@ const SLUG_NOUNS = [ ]; function randomChoice(values: string[], fallback: string) { - return values[Math.floor(Math.random() * values.length)] ?? fallback; + return values[generateSecureInt(values.length)] ?? fallback; +} + +const SLUG_FALLBACK_ALPHABET = "abcdefghijklmnopqrstuvwxyz0123456789"; + +function createFallbackSuffix(length: number): string { + let suffix = ""; + for (let i = 0; i < length; i += 1) { + suffix += SLUG_FALLBACK_ALPHABET[generateSecureInt(SLUG_FALLBACK_ALPHABET.length)] ?? "x"; + } + return suffix; } function createSlugBase(words = 2) { @@ -141,6 +153,6 @@ export function createSessionSlug(isTaken?: (id: string) => boolean): string { if (threeWord) { return threeWord; } - const fallback = `${createSlugBase(3)}-${Math.random().toString(36).slice(2, 5)}`; + const fallback = `${createSlugBase(3)}-${createFallbackSuffix(3)}`; return isIdTaken(fallback) ? `${fallback}-${Date.now().toString(36)}` : fallback; } diff --git a/src/auto-reply/reply/reply-dispatcher.ts b/src/auto-reply/reply/reply-dispatcher.ts index 483bdcc21dd..5df01c7ac22 100644 --- a/src/auto-reply/reply/reply-dispatcher.ts +++ b/src/auto-reply/reply/reply-dispatcher.ts @@ -1,5 +1,6 @@ import type { TypingCallbacks } from "../../channels/typing.js"; import type { HumanDelayConfig } from "../../config/types.js"; +import { generateSecureInt } from "../../infra/secure-random.js"; import { sleep } from "../../utils.js"; import type { GetReplyOptions, ReplyPayload } from "../types.js"; import { registerDispatcher } from "./dispatcher-registry.js"; @@ -37,7 +38,7 @@ function getHumanDelay(config: HumanDelayConfig | undefined): number { if (max <= min) { return min; } - return Math.floor(Math.random() * (max - min + 1)) + min; + return min + generateSecureInt(max - min + 1); } export type ReplyDispatcherOptions = { diff --git a/src/auto-reply/reply/reply-flow.test.ts b/src/auto-reply/reply/reply-flow.test.ts index ff57751a7b7..146606d076f 100644 --- a/src/auto-reply/reply/reply-flow.test.ts +++ b/src/auto-reply/reply/reply-flow.test.ts @@ -1916,7 +1916,6 @@ describe("createReplyDispatcher", () => { it("delays block replies after the first when humanDelay is natural", async () => { vi.useFakeTimers(); - const randomSpy = vi.spyOn(Math, "random").mockReturnValue(0); const deliver = vi.fn().mockResolvedValue(undefined); const dispatcher = createReplyDispatcher({ deliver, @@ -1931,14 +1930,12 @@ describe("createReplyDispatcher", () => { await Promise.resolve(); expect(deliver).toHaveBeenCalledTimes(1); - await vi.advanceTimersByTimeAsync(799); + await vi.advanceTimersByTimeAsync(2499); expect(deliver).toHaveBeenCalledTimes(1); await vi.advanceTimersByTimeAsync(1); await dispatcher.waitForIdle(); expect(deliver).toHaveBeenCalledTimes(2); - - randomSpy.mockRestore(); vi.useRealTimers(); }); diff --git a/src/infra/retry.test.ts b/src/infra/retry.test.ts index 5a8ebf2fd69..49ae2534d67 100644 --- a/src/infra/retry.test.ts +++ b/src/infra/retry.test.ts @@ -1,6 +1,14 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { resolveRetryConfig, retryAsync } from "./retry.js"; +const randomMocks = vi.hoisted(() => ({ + generateSecureFraction: vi.fn(), +})); + +vi.mock("./secure-random.js", () => ({ + generateSecureFraction: randomMocks.generateSecureFraction, +})); + type NumberRetryCase = { name: string; fn: ReturnType; @@ -71,6 +79,7 @@ afterEach(() => { beforeEach(() => { vi.clearAllTimers(); vi.useRealTimers(); + randomMocks.generateSecureFraction.mockReset(); }); describe("retryAsync", () => { @@ -181,6 +190,30 @@ describe("retryAsync", () => { const delays = await runRetryAfterCase(params); expect(delays[0]).toBe(expectedDelay); }); + + it("uses secure jitter when configured", async () => { + vi.useFakeTimers(); + randomMocks.generateSecureFraction.mockReturnValue(1); + const fn = vi.fn().mockRejectedValueOnce(new Error("boom")).mockResolvedValueOnce("ok"); + const delays: number[] = []; + + try { + const promise = retryAsync(fn, { + attempts: 2, + minDelayMs: 100, + maxDelayMs: 200, + jitter: 0.5, + onRetry: (info) => delays.push(info.delayMs), + }); + await vi.runAllTimersAsync(); + await expect(promise).resolves.toBe("ok"); + expect(delays).toEqual([150]); + expect(randomMocks.generateSecureFraction).toHaveBeenCalledTimes(1); + } finally { + vi.clearAllTimers(); + vi.useRealTimers(); + } + }); }); describe("resolveRetryConfig", () => { diff --git a/src/infra/retry.ts b/src/infra/retry.ts index 8d4e794ac44..dab49eaf33c 100644 --- a/src/infra/retry.ts +++ b/src/infra/retry.ts @@ -1,4 +1,5 @@ import { sleep } from "../utils.js"; +import { generateSecureFraction } from "./secure-random.js"; export type RetryConfig = { attempts?: number; @@ -63,7 +64,7 @@ function applyJitter(delayMs: number, jitter: number): number { if (jitter <= 0) { return delayMs; } - const offset = (Math.random() * 2 - 1) * jitter; + const offset = (generateSecureFraction() * 2 - 1) * jitter; return Math.max(0, Math.round(delayMs * (1 + offset))); } diff --git a/src/infra/secure-random.test.ts b/src/infra/secure-random.test.ts index 938b2ba95c0..97bb5ed8d01 100644 --- a/src/infra/secure-random.test.ts +++ b/src/infra/secure-random.test.ts @@ -3,20 +3,31 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; const cryptoMocks = vi.hoisted(() => ({ randomBytes: vi.fn((bytes: number) => Buffer.alloc(bytes, 0xab)), + randomInt: vi.fn(), randomUUID: vi.fn(), })); vi.mock("node:crypto", () => ({ randomBytes: cryptoMocks.randomBytes, + randomInt: cryptoMocks.randomInt, randomUUID: cryptoMocks.randomUUID, })); +let generateSecureFraction: typeof import("./secure-random.js").generateSecureFraction; +let generateSecureHex: typeof import("./secure-random.js").generateSecureHex; +let generateSecureInt: typeof import("./secure-random.js").generateSecureInt; let generateSecureToken: typeof import("./secure-random.js").generateSecureToken; let generateSecureUuid: typeof import("./secure-random.js").generateSecureUuid; beforeEach(async () => { vi.resetModules(); - ({ generateSecureToken, generateSecureUuid } = await import("./secure-random.js")); + ({ + generateSecureFraction, + generateSecureHex, + generateSecureInt, + generateSecureToken, + generateSecureUuid, + } = await import("./secure-random.js")); }); describe("secure-random", () => { @@ -56,4 +67,27 @@ describe("secure-random", () => { expect(token).toBe(expectedToken); expect(token).toMatch(/^[A-Za-z0-9_-]*$/); }); + + it("generates secure hex strings", () => { + cryptoMocks.randomBytes.mockClear(); + + expect(generateSecureHex(4)).toBe(Buffer.alloc(4, 0xab).toString("hex")); + expect(cryptoMocks.randomBytes).toHaveBeenCalledWith(4); + }); + + it("maps random bytes into a unit interval fraction", () => { + cryptoMocks.randomBytes.mockReturnValueOnce(Buffer.from([0x80, 0x00, 0x00, 0x00])); + + expect(generateSecureFraction()).toBe(0.5); + expect(cryptoMocks.randomBytes).toHaveBeenCalledWith(4); + }); + + it("delegates bounded integer generation to crypto.randomInt", () => { + cryptoMocks.randomInt.mockReturnValueOnce(2).mockReturnValueOnce(7); + + expect(generateSecureInt(5)).toBe(2); + expect(generateSecureInt(3, 9)).toBe(7); + expect(cryptoMocks.randomInt).toHaveBeenNthCalledWith(1, 5); + expect(cryptoMocks.randomInt).toHaveBeenNthCalledWith(2, 3, 9); + }); }); diff --git a/src/infra/secure-random.ts b/src/infra/secure-random.ts index 05c961e5048..bbca0a95473 100644 --- a/src/infra/secure-random.ts +++ b/src/infra/secure-random.ts @@ -1,4 +1,4 @@ -import { randomBytes, randomUUID } from "node:crypto"; +import { randomBytes, randomInt, randomUUID } from "node:crypto"; export function generateSecureUuid(): string { return randomUUID(); @@ -7,3 +7,18 @@ export function generateSecureUuid(): string { export function generateSecureToken(bytes = 16): string { return randomBytes(bytes).toString("base64url"); } + +export function generateSecureHex(bytes = 16): string { + return randomBytes(bytes).toString("hex"); +} + +/** Returns a cryptographically secure fraction in the range [0, 1). */ +export function generateSecureFraction(): number { + return randomBytes(4).readUInt32BE(0) / 0x1_0000_0000; +} + +export function generateSecureInt(maxExclusive: number): number; +export function generateSecureInt(minInclusive: number, maxExclusive: number): number; +export function generateSecureInt(a: number, b?: number): number { + return typeof b === "number" ? randomInt(a, b) : randomInt(a); +} diff --git a/ui/src/ui/uuid.test.ts b/ui/src/ui/uuid.test.ts index bb85f289aaf..a5f05116511 100644 --- a/ui/src/ui/uuid.test.ts +++ b/ui/src/ui/uuid.test.ts @@ -31,8 +31,7 @@ describe("generateUUID", () => { it("still returns a v4 UUID when crypto is missing", () => { const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); try { - const id = generateUUID(null); - expect(id).toMatch(/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/); + expect(() => generateUUID(null)).toThrow("Web Crypto is required for UUID generation"); expect(warnSpy).toHaveBeenCalled(); } finally { warnSpy.mockRestore(); diff --git a/ui/src/ui/uuid.ts b/ui/src/ui/uuid.ts index 0f74316ba39..019b7fd6105 100644 --- a/ui/src/ui/uuid.ts +++ b/ui/src/ui/uuid.ts @@ -20,25 +20,12 @@ function uuidFromBytes(bytes: Uint8Array): string { )}-${hex.slice(20)}`; } -function weakRandomBytes(): Uint8Array { - const bytes = new Uint8Array(16); - const now = Date.now(); - for (let i = 0; i < bytes.length; i++) { - bytes[i] = Math.floor(Math.random() * 256); - } - bytes[0] ^= now & 0xff; - bytes[1] ^= (now >>> 8) & 0xff; - bytes[2] ^= (now >>> 16) & 0xff; - bytes[3] ^= (now >>> 24) & 0xff; - return bytes; -} - function warnWeakCryptoOnce() { if (warnedWeakCrypto) { return; } warnedWeakCrypto = true; - console.warn("[uuid] crypto API missing; falling back to weak randomness"); + console.warn("[uuid] crypto API missing; refusing insecure UUID generation"); } export function generateUUID(cryptoLike: CryptoLike | null = globalThis.crypto): string { @@ -53,5 +40,5 @@ export function generateUUID(cryptoLike: CryptoLike | null = globalThis.crypto): } warnWeakCryptoOnce(); - return uuidFromBytes(weakRandomBytes()); + throw new Error("Web Crypto is required for UUID generation"); }