infra: harden identifier entropy and delay jitter (#57744)

* infra: harden identifier entropy and delay jitter

* test: make randomness hardening deterministic in CI
This commit is contained in:
Jacob Tomlinson 2026-03-30 08:57:30 -07:00 committed by GitHub
parent 32a4a47d60
commit ae703ab0e7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
12 changed files with 170 additions and 33 deletions

View File

@ -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();
}
});
});

View File

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

View File

@ -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();
});
});

View File

@ -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;
}

View File

@ -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 = {

View File

@ -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();
});

View File

@ -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<typeof vi.fn>;
@ -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", () => {

View File

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

View File

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

View File

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

View File

@ -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();

View File

@ -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");
}