fix: use crypto.getRandomValues for randId to eliminate collision risk by danditomaso · Pull Request #1282 · meshtastic/web

danditomaso · 2026-07-12T20:26:11Z

Closes #1273.

randId() (used for device / saved-connection IDs) generated values with Math.floor(Math.random() * 1e9) — ~30 bits of entropy and no crypto guarantees. Swapped for crypto.getRandomValues(new Uint32Array(1))[0] for full 32-bit range.

Tests updated to drop the Math.random mock assertion and check the uint32 range plus uniqueness across 1000 samples.

Test plan

pnpm --filter @meshtastic/web test -- randId — green

Summary by CodeRabbit

Bug Fixes
- Improved generated identifier randomness for more reliable unique IDs.
- Expanded the identifier value range to cover the full unsigned 32-bit space.
- Updated checks to ensure generated identifiers stay within the supported range.
Tests
- Revised unit tests to simplify coverage and strengthen range and uniqueness assertions.

Math.random() gives ~30 bits of entropy and no cryptographic guarantees. Switch to crypto.getRandomValues over a Uint32 buffer so device / saved- connection IDs get full 32-bit range and don't collide across concurrent transports. Closes meshtastic#1273

vercel · 2026-07-12T20:26:16Z

@danditomaso is attempting to deploy a commit to the Meshtastic Team on Vercel.

A member of the Team first needs to authorize it.

coderabbitai · 2026-07-12T20:26:38Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e13f5a03-8aa1-4a85-9d39-a6c4126869c3

📥 Commits

Reviewing files that changed from the base of the PR and between e0d6f21 and 9c98066.

📒 Files selected for processing (1)

apps/web/src/core/utils/randId.ts

🚧 Files skipped from review as they are similar to previous changes (1)

apps/web/src/core/utils/randId.ts

📝 Walkthrough

Walkthrough

randId now returns cryptographically generated unsigned 32-bit values. Its tests remove Math.random spies, validate the numeric range, and check uniqueness across 1000 generated IDs.

Changes

Cryptographic identifier generation

Layer / File(s)	Summary
Update randId generation and validation `apps/web/src/core/utils/randId.ts`, `apps/web/src/core/utils/randId.test.ts`	`randId` uses `crypto.getRandomValues` with a `Uint32Array`; tests validate the unsigned 32-bit range and uniqueness across 1000 calls.

Estimated code review effort: 2 (Simple) | ~10 minutes

Poem

I’m a rabbit hopping light,
Crypto numbers bloom tonight.
Thirty-two bits leap and spin,
Fewer collisions, more wins!
Tests count thousand tracks,
Happy paws approve the facts.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change to randId and the collision-risk fix.
Description check	✅ Passed	The PR description covers the problem, fix, related issue, and testing, though it does not follow the full template structure.
Linked Issues check	✅ Passed	The change replaces Math.random with crypto.getRandomValues as requested by issue `#1273` and updates tests accordingly.
Out of Scope Changes check	✅ Passed	The diff is limited to randId and its tests, with no obvious unrelated changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

🧹 Nitpick comments (1)

apps/web/src/core/utils/randId.test.ts (1)
17-24: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Test is inherently probabilistic (extremely low flake risk).

With ~0.01% collision probability across 1000 samples from 2^32 space, this test could rarely flake in CI despite the comment acknowledging it. No action needed given the negligible probability and existing documentation, but worth being aware of if intermittent failures ever appear.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/web/src/core/utils/randId.test.ts` around lines 17 - 24, No code changes
are required; retain the existing randId uniqueness test and its explanatory
comment.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@apps/web/src/core/utils/randId.test.ts`:
- Around line 17-24: No code changes are required; retain the existing randId
uniqueness test and its explanatory comment.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ff91998e-8c79-4b82-b6fe-20fee8f354fb

📥 Commits

Reviewing files that changed from the base of the PR and between b0bde79 and e0d6f21.

📒 Files selected for processing (2)

apps/web/src/core/utils/randId.test.ts
apps/web/src/core/utils/randId.ts

Copilot

Pull request overview

This PR addresses collision risk in randId() by switching ID generation from Math.random() to cryptographically strong randomness via crypto.getRandomValues, and updates the unit tests accordingly.

Changes:

Updated randId() to return a uint32 generated via crypto.getRandomValues.
Adjusted tests to validate integer + uint32 range and removed the old Math.random-specific assertion.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
apps/web/src/core/utils/randId.ts	Switches ID generation to Web Crypto (`getRandomValues`) for improved entropy/collision resistance.
apps/web/src/core/utils/randId.test.ts	Updates tests for the new uint32-based behavior (but current uniqueness test is probabilistic).

+    for (let i = 0; i < 1000; i++) {
      results.add(randId());
    }
-
-    expect(results.size).toBeGreaterThan(95);
+    // With 32 bits of entropy over 1000 samples, collisions are vanishingly rare.
+    expect(results.size).toBe(1000);


Under strict + noUncheckedIndexedAccess (TS7 default), buf[0] is number | undefined. Uint32Array(1) guarantees length 1, so assert non-null to satisfy the number return type.

Copilot AI review requested due to automatic review settings July 12, 2026 20:26

coderabbitai Bot reviewed Jul 12, 2026

View reviewed changes

Copilot started reviewing on behalf of danditomaso July 12, 2026 20:29 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

Comment thread apps/web/src/core/utils/randId.test.ts

Comment on lines +19 to +23

for (let i = 0; i < 1000; i++) {

results.add(randId());

}

expect(results.size).toBeGreaterThan(95);

// With 32 bits of entropy over 1000 samples, collisions are vanishingly rare.

expect(results.size).toBe(1000);

Copilot AI mentioned this pull request Jul 14, 2026

fix(randId): replace Math.random with crypto.getRandomValues and deterministic tests #1297

Draft

4 tasks

danditomaso added 2 commits July 14, 2026 22:16

Merge branch 'main' into fix/randid-crypto

eda9baa

fix: assert Uint32Array index for strict TS7 compat

9c98066

Under strict + noUncheckedIndexedAccess (TS7 default), buf[0] is number | undefined. Uint32Array(1) guarantees length 1, so assert non-null to satisfy the number return type.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: use crypto.getRandomValues for randId to eliminate collision risk#1282

fix: use crypto.getRandomValues for randId to eliminate collision risk#1282
danditomaso wants to merge 3 commits into
meshtastic:mainfrom
danditomaso:fix/randid-crypto

danditomaso commented Jul 12, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

vercel Bot commented Jul 12, 2026

Uh oh!

coderabbitai Bot commented Jul 12, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

danditomaso commented Jul 12, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Summary by CodeRabbit

Uh oh!

vercel Bot commented Jul 12, 2026

Uh oh!

coderabbitai Bot commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danditomaso commented Jul 12, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 12, 2026 •

edited

Loading