← Back to PRs

#23671: fix(security): use crypto.randomInt for session slug generation

by kevinWangSheng open 2026-02-22 15:26 View on GitHub →
agents size: XS
## Summary - Problem: Session slug generation uses `Math.random()` which is not cryptographically secure and produces predictable output - Why it matters: Session slugs serve as identifiers; predictable generation could allow session enumeration in adversarial environments - What changed: Replaced `Math.random()` with `crypto.randomInt()` from Node.js `node:crypto` module - What did NOT change: Slug format, word lists, collision retry logic, and fallback behavior remain identical ## Change Type - [x] Security hardening ## Scope - [x] Auth / tokens ## Security Impact - New permissions/capabilities? No - Secrets/tokens handling changed? No - New/changed network calls? No - Command/tool execution surface changed? No - Data access scope changed? No ## Compatibility / Migration - Backward compatible? Yes - Config/env changes? No - Migration needed? No ## Risks and Mitigations - Risk: crypto.randomInt() has slightly different distribution characteristics - Mitigation: For array index selection, the distribution is uniform and functionally equivalent <!-- greptile_comment --> <h3>Greptile Summary</h3> Replaces `Math.random()` with `crypto.randomInt()` in session slug generation for cryptographically secure randomness. The change touches two call sites: array index selection in `randomChoice()` and the fallback suffix in `createSessionSlug()`. The fallback replacement (`randomInt(46656)` with `padStart`) is slightly more robust than the original (`Math.random().toString(36).slice(2,5)`) because it guarantees exactly 3 base-36 characters. Tests are updated to mock `node:crypto` instead of `Math.random`. - Swapped `Math.floor(Math.random() * n)` → `randomInt(n)` for array index selection - Replaced `Math.random().toString(36).slice(2,5)` → `randomInt(46656).toString(36).padStart(3, "0")` for fallback suffix (36³ = 46656) - Tests now use `vi.mock("node:crypto")` with proper Vitest hoisting - No changes to slug format, word lists, collision retry logic, or public API <h3>Confidence Score: 5/5</h3> - This PR is safe to merge — it's a straightforward, correct swap from Math.random() to crypto.randomInt() with no behavioral regressions. - The change is minimal and well-scoped: two call sites replaced with cryptographically secure equivalents that produce functionally identical output. The math is correct (36³ = 46656 for 3-char base-36 strings), the fallback is actually slightly improved (padStart guarantees 3 characters), and the tests are properly updated to mock the new dependency. No API surface, slug format, or retry logic changes. - No files require special attention. <sub>Last reviewed commit: 1f3c0ea</sub> <!-- greptile_other_comments_section --> <sub>(5/5) You can turn off certain types of comments like style [here](https://app.greptile.com/review/github)!</sub> <!-- /greptile_comment -->

Most Similar PRs