Bun.randomUUIDv7: reject timestamps >= 2^48 and NaN instead of truncating - #34021
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughChangesUUIDv7 timestamp validation
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Comment |
|
Updated 11:59 PM PT - Jul 12th, 2026
❌ @autofix-ci[bot], your commit f543936 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34021That installs a local version of the PR into your bun-34021 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the 48-bit bound is correctly derived from RFC 9562, the Date/NaN/number paths now agree, and the boundary (2^48-1 accepted, 2^48 rejected) is tested. The inline nit is test-coverage only; the runtime change is sound.
Extended reasoning...
Overview
Tightens timestamp validation in bun_random_uuid_v7 (src/runtime/webcore/Crypto.rs) so all three input paths — number, Date, and the NaN edge case — enforce the same [0, 2^48-1] range that UUID7::init can actually encode. Previously the number path accepted up to 2^53-1 and truncated mod 2^48, the Date path bypassed validation entirely via .max(0.0) as u64, and NaN fell through validate_integer_range's NaN→default mapping to 0. Adds a timestamp range validation describe block covering the boundary and rejection cases.
Security risks
None. This narrows accepted input (fail-closed), doesn't touch entropy generation, and the only new code paths are throw_range_error returns.
Level of scrutiny
Low-to-moderate. Small, self-contained change to a Bun-specific utility API with a clear RFC-cited rationale. I verified: (1i64 << 48) - 1 is exactly representable as f64 (< 2^53), so the > MAX_TIMESTAMP as f64 comparison is exact; the post-check date as u64 cast is lossless in [0, 2^48-1]; and u64::try_from(i64).unwrap() after min: 0 cannot panic. The Date path now matches the number path's existing min: 0 semantics (negative numbers already threw), so rejecting pre-epoch Dates is a consistency fix rather than a new design choice.
Other factors
The one inline finding is a test-quality nit (missing Date(-1) row and bare .toThrow()) — worth addressing but not a correctness issue with the fix. The PR description states 14 tests pass under bun bd test. This is a user-visible behavior change (silent-garbage → throw), but converting silent truncation that breaks UUIDv7's sort-order guarantee into a RangeError is unambiguously the right direction.
There was a problem hiding this comment.
LGTM — both earlier nits addressed (Date(-1) row added, .toThrow(RangeError) on every case).
What was reviewed:
- The three new throw paths (Date-range, explicit NaN,
IntegerRange.max) — each covered by the rejection matrix;2**48-1boundary accepted on both number and Date paths. MAX_TIMESTAMP as f64is exactly representable (< 2^53), so the Date upper-bound comparison is exact.as_number()guarded byis_number();u64::try_from(..).unwrap()is infallible after the [0, 2^48-1] range check.
Extended reasoning...
Overview
Tightens input validation in bun_random_uuid_v7 (src/runtime/webcore/Crypto.rs): introduces MAX_TIMESTAMP = (1<<48)-1, validates the Date-argument path against [0, 2^48-1] (was date.max(0.0) as u64 with no upper bound), rejects numeric NaN explicitly before it reaches validate_integer_range's NaN→default fallback, and sets IntegerRange.max so the RangeError message matches what's actually encodable. Tests add a timestamp range validation block covering the accept boundary, a 7-case rejection matrix across both call shapes, and an assertion that the error message advertises 281474976710655 rather than 9007199254740991.
Security risks
None. The change only adds stricter validation on a user-provided timestamp before it's packed into 6 bytes of a UUID. No new allocations, no pointer arithmetic, no changes to the entropy path or the crypto primitives themselves.
Level of scrutiny
Low-to-medium. ~25 lines of Rust in a single function, plus tests. The logic is straightforward range checking using existing helpers (throw_range_error, validate_integer_range). This is a Bun-native API (not Node/Web compat), so tightening validation to reject previously-silently-truncated inputs is the right call and doesn't need to match an external spec beyond RFC 9562's 48-bit field width, which it does.
Other factors
- I left two test-quality nits on the previous revision (missing
Date(-1)coverage for thedate < 0.0clause; bare.toThrow()). Both were applied in 2d56369 and are visible in the current diff. - Checked that
is_number()guardsas_number()(per the exception-safety guidance for non-throwing accessors on user values), that(1i64 << 48) - 1is exactly representable as f64 so the Date comparison has no rounding edge, and that the post-validationdate as u64/u64::try_from(...).unwrap()casts are infallible given the validated range. - The one user-visible behavior change beyond the bug fix — pre-epoch
Datenow throws instead of clamping to timestamp 0 — is intentional, called out in the PR description, and now has a dedicated test row. - Bug-hunting system found nothing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/runtime/webcore/Crypto.rs`:
- Around line 219-252: Extract the duplicated RangeErrorOptions construction
from the timestamp validation logic into a named local helper or reusable value
near the MAX_TIMESTAMP declaration. Update both the invalid-Date branch and the
NaN branch to use it, preserving the existing min, max, field_name, and default
values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5fdd7e8-ab09-48d7-a445-e8f2d3077761
📒 Files selected for processing (2)
src/runtime/webcore/Crypto.rstest/js/bun/util/randomUUIDv7.test.ts
There was a problem hiding this comment.
LGTM — all prior feedback addressed (Date(-1) row, .toThrow(RangeError), (undefined, ts) routing + coverage, range_opts dedup).
What was reviewed:
- 48-bit bound:
(1<<48)-1accepted/encoded exactly;2**48, NaN, Invalid Date, pre-epoch/over-range Date all reject via the sameRangeErrorOptions. - Arg routing after dropping the
!encoding_value.is_undefined()guard — traced(),(ts),("hex"),("hex", ts),(undefined, ts),("hex", undefined); no regression, andarguments_undef::<2>().lenreflects actual arg count so thelen == 1single-arg arm still fires. date as u64cast is safe post-validation (finite,[0, 2^48-1], integer-valued); Infinity still rejected by the existingvalidate_integer_rangepath.
Extended reasoning...
Overview
Tightens Bun.randomUUIDv7 timestamp validation to the RFC 9562 §5.7 48-bit unix_ts_ms field width, and fixes a pre-existing arg-routing bug that dropped the timestamp for the (undefined, ts) overload. Two files: ~20 net Rust lines in src/runtime/webcore/Crypto.rs (a MAX_TIMESTAMP constant, a shared range_opts, explicit Date/NaN range checks, and a one-token guard change on the timestamp_value selector) and ~45 test lines adding a boundary-accept case, a 7-entry × 3-call-shape rejection matrix asserting RangeError, an error-message assertion on the advertised bound, and a rewrite of the pre-existing test("timestamp") to use a fixed 2021 timestamp so it can no longer pass coincidentally against Date.now().
Security risks
None. This is input validation on a UUID helper; no auth, crypto primitives, filesystem, or network involved. The change fails closed (throws) where it previously silently produced wrong output.
Level of scrutiny
Low–medium. Small, self-contained validation fix on a leaf utility API. The user-visible behavior change — inputs in [2^48, 2^53-1], NaN, and out-of-range/invalid Dates now throw instead of silently truncating to a wrong timestamp — is unambiguously a bug fix (silent truncation broke UUIDv7's sort-order guarantee), not an API design decision. The (undefined, ts) routing fix aligns runtime behavior with the published .d.ts signature.
Other factors
This is my third pass on the PR. Both earlier rounds of inline feedback (test-quality nits at 2d56369; the (undefined, ts) routing gap at 5103558) and CodeRabbit's dedup suggestion (4d2be91) are all applied and marked resolved. The bug-hunting system found nothing on the current revision. I re-traced every call shape through the new timestamp_value selector against arguments_undef::<2>() semantics (.len = actual arg count capped at 2) and confirmed no existing form regresses. The float→u64 cast on the Date path is sound after the new finite/[0, 2^48-1] guard. Test coverage now exercises every clause of the new validation across all three overloads.
There was a problem hiding this comment.
LGTM — both earlier review rounds are addressed and all threads are resolved.
What was reviewed:
- 48-bit bound, NaN, and Date validation paths in
bun_random_uuid_v7; each newthrow_range_errorsite is covered by a.toThrow(RangeError)case includingDate(-1). - Arg-routing change to
arguments.len > 1: traced("hex"),("hex", undefined),(undefined, ts), and single-arg number/Date forms — all route correctly. - Rewritten
test("timestamp")now uses a fixed 2021 ts so it actually detects a dropped argument; accept/reject matrices cover all three call shapes.
Extended reasoning...
Overview
Two files: src/runtime/webcore/Crypto.rs (the bun_random_uuid_v7 host function) and test/js/bun/util/randomUUIDv7.test.ts. The Rust change caps the accepted timestamp at (1 << 48) - 1 to match the RFC 9562 §5.7 field width, rejects NaN before it reaches validate_integer_range's NaN→default mapping, validates extracted Date timestamps against the same range (replacing the old .max(0.0) as u64 clamp), and fixes the timestamp_value selector so (undefined, ts) reads ptr[1]. A shared range_opts local deduplicates the two new throw_range_error sites per CodeRabbit feedback.
Security risks
None. This is input validation on a UUID-generation utility; tightening the accepted range and rejecting NaN/invalid Dates strictly reduces the space of accepted inputs. No auth, crypto primitives, or permissions are touched (despite the file name, this is the randomUUIDv7 helper, not key material).
Level of scrutiny
Low-to-moderate. The change is ~19 lines of Rust in a single function plus ~45 lines of tests. It's a focused input-validation fix with a clear RFC citation for the bound. The one user-visible behavior change beyond the range cap — pre-epoch Date and NaN now throw instead of silently encoding timestamp 0 — is the obviously-correct choice (silent truncation broke sort order, which is UUIDv7's whole point).
Other factors
I've reviewed this PR twice already. The first round asked for a Date(-1) row and .toThrow(RangeError) — applied in 2d56369. The second flagged the pre-existing (undefined, ts) arg-routing bug — fixed in 5103558 with the selector guard simplified to arguments.len > 1 and the coincidentally-passing test("timestamp") rewritten to a fixed past timestamp. CodeRabbit's dedup suggestion was applied in 4d2be91. All inline threads are resolved. The PR description includes fail-on-main / pass-on-PR evidence for both ASAN debug and release builds (15 tests pass). I re-traced the new selector against ("hex"), ("hex", undefined), (ts), and ({}, ts) — no regressions. No CODEOWNERS on this path.
|
The diff is green: CI reds on builds 72157 and 72165 are unrelated to this change:
Remaining yellows are known install/napi flakes that passed on retry. Ready for review. |
|
@robobun fix conflcits and rebase |
…ting The UUIDv7 unix_ts_ms field is 48 bits (RFC 9562 section 5.7). UUID7::init writes only the low 6 bytes of the timestamp, but the range check used the IntegerRange default max of Number.MAX_SAFE_INTEGER (2^53-1). Timestamps in [2^48, 2^53-1] were accepted and truncated mod 2^48, so 2**48 encoded as epoch 0 and sorted before every real UUID. The RangeError message also advertised the unreachable 2^53-1 bound. validate_integer_range maps NaN to the default (0), so NaN produced a timestamp-0 UUID. The Date path (date.max(0.0) as u64) bypassed all validation, so new Date(8.64e15), Invalid Date, and pre-epoch Dates silently encoded 0 or truncated. The timestamp_value selector also required the first argument to be a string before reading arguments.ptr[1], so randomUUIDv7(undefined, ts) silently ignored ts and used the current time. Cap the range at 2^48-1, reject NaN and out-of-range Dates with ERR_OUT_OF_RANGE, and route arguments.ptr[1] whenever arguments.len > 1. Rejection tests run in-process (they throw before touching the process-global UUID_V7_LAST_TIMESTAMP); the 2^48-1 accept test runs in a subprocess to avoid parking the global at year 10889.
00a8f8e to
8dd20db
Compare
|
Rebased onto main (8dd20db). #34022 made |
UUID7::next did ts.wrapping_add(1) on 12-bit counter rollover with no
48-bit bound. After <=4096 calls at 2^48-1 the bumped timestamp reached
2^48, which UUID7::init then encodes as six zero bytes: the wrap-to-epoch-0
symptom this PR fixes for direct input, reached via the internal rollover
path instead. Clamp the bump at (1<<48)-1.
Also dropped the vacuous test('timestamp') (it compared (undefined,
Date.now()) to Date.now() within 32ms, which cannot distinguish a dropped
argument from an honored one). Coverage for (undefined, ts) lives in the
2^48-1 subprocess accept test and the rejection matrix.
|
Post-rebase CI (build 72410): Ready for merge. |
There was a problem hiding this comment.
LGTM — all prior review feedback has been applied and the final diff is a focused validation fix.
Checked the three throw sites (Date, NaN, validate_integer_range) reach the same 48-bit bound and error class; MAX_TIMESTAMP as f64 is exact (< 2^53) so the Date comparison admits 2**48-1 and rejects 2**48.
Verified the arguments.len > 1 guard change doesn't regress ("hex", ts) or the single-arg overload — the len == 1 && encoding_value.is_undefined() arm still routes (ts) correctly.
Confirmed the dropped test("timestamp") is fully covered by the rejection matrix's (undefined, ts) row and the 2**48-1 subprocess accept test.
The (ts + 1).min(2^48-1) clamp cannot overflow since ts is already ≤ 2^48-1 at that point.
Extended reasoning...
Overview
Three-file change to Bun.randomUUIDv7: (1) src/runtime/webcore/Crypto.rs tightens timestamp validation to the RFC 9562 §5.7 48-bit unix_ts_ms range — adds max: (1<<48)-1 to the existing IntegerRange, rejects NaN before the NaN→default mapping in validate_integer_range, validates extracted Date values against [0, 2^48-1] instead of .max(0.0)-clamping, and fixes the timestamp_value selector so (undefined, ts) reaches the check. (2) src/jsc/uuid.rs clamps the counter-rollover timestamp bump at 2^48-1 instead of wrapping_add. (3) test/js/bun/util/randomUUIDv7.test.ts adds a 7-row rejection matrix across all three call shapes, a RangeError-message assertion, and two subprocess tests for the boundary accept and the rollover clamp; deletes the pre-existing test("timestamp") which was vacuous (compared Date.now() to Date.now() within 32ms).
Security risks
None. The file is named Crypto.rs but this path is UUID timestamp encoding, not cryptographic primitives — no key material, TLS, auth, or randomness-quality changes. The change strictly tightens input validation (previously-accepted garbage inputs now throw RangeError), which is the safe direction. The date as u64 cast is guarded by is_finite() && >= 0 && <= 2^48-1, and Date internal values are integers per ECMA-262 TimeClip, so no precision loss.
Level of scrutiny
Medium. This is a user-facing behavior change (previously-silent inputs now throw) in a stable Bun API, so it warrants care — but the change is unambiguously a bug fix aligning with the RFC and the type signature, not a design decision. The native diff is ~20 lines of straightforward guard logic reusing existing helpers (throw_range_error, validate_integer_range, RangeErrorOptions). No new allocations, no GC/lifetime concerns, no threading changes.
Other factors
This is my fourth pass on the PR. Every prior comment (mine and CodeRabbit's) has been applied and resolved: .toThrow(RangeError) instead of bare .toThrow(), the Date(-1) row, the hoisted range_opts, the (undefined, ts) routing fix, dropping the vacuous test("timestamp"), and the uuid.rs rollover clamp with its 5000-iteration subprocess test. The gate evidence in the description shows clean fail-before/pass-after on both debug-ASAN and release. Jarred requested a rebase, which was done; the one hunk lost in that rebase was the test("timestamp") rewrite, which I flagged and was resolved by deleting the test (its coverage now lives in the rejection matrix and the subprocess accept test — both of which fail on the pre-fix binary, unlike the original). The new subprocess tests use expect(stderr).toBe(""), which CLAUDE.md discourages, but this matches the existing subprocess tests already on main in the same file from #34022, and each test's real assertion is on stdout content — the stderr check is supplementary.
2 participants
Repro
Cause
The UUIDv7
unix_ts_msfield is 48 bits (RFC 9562 section 5.7).UUID7::initwrites only the low 6 bytes of the timestamp, but the range check inbun_random_uuid_v7used theIntegerRangedefaultmaxofNumber.MAX_SAFE_INTEGER(2^53-1). So[2^48, 2^53-1]was accepted and then truncated mod 2^48:2**48encoded as epoch 0 and sorted before every real UUID. The RangeError message also advertised the unreachable 2^53-1 bound.validate_integer_rangemaps NaN to the passed default (0), soNaNproduced a timestamp-0 UUID instead of throwing.The
Dateargument path (date.max(0.0) as u64) bypassed all validation, sonew Date(8.64e15)truncated andnew Date(NaN)/ pre-epoch dates silently encoded 0.The
timestamp_valueselector required the first argument to be a string before readingarguments.ptr[1], soBun.randomUUIDv7(undefined, ts)(valid per theencoding?: ...type signature) silently ignoredtsand used the current time.UUID7::next(from #34022) didts.wrapping_add(1)on 12-bit counter rollover with no 48-bit clamp: after <=4096 calls pinned at2**48-1, the bumped timestamp reaches2**48and encodes as epoch 0 (same symptom via the internal rollover path).Fix
src/runtime/webcore/Crypto.rs: setmax: (1 << 48) - 1on theIntegerRange; reject NaN withERR_OUT_OF_RANGEbeforevalidate_integer_range's NaN-to-default mapping; validate extracted Date timestamps against[0, 2^48-1]; routearguments.ptr[1]wheneverarguments.len > 1.src/jsc/uuid.rs: clamp the counter-rollover timestamp bump at(1<<48)-1.Verification
The
timestamp range validationblock covers rejection of2**48,2**53-1,NaN,new Date(-1),new Date(2**48),new Date(8.64e15), and Invalid Date withRangeErroracross all three call shapes (("hex", ts),(undefined, ts),(ts)); acceptance and exact encoding of2**48-1(subprocess, since it parks the process-global timestamp); a 5000-iteration subprocess asserting counter rollover at2**48-1does not wrap to epoch 0; and an assertion that the RangeError message advertises281474976710655rather than9007199254740991.The pre-existing
test("timestamp")was dropped: it compared(undefined, Date.now())toDate.now()within 32ms, which cannot distinguish a dropped argument from an honored one, and cannot be rewritten in-process post-#34022 since the process-global timestamp never moves backward. Its intended coverage lives in the subprocess accept test and the rejection matrix.[review] gate passed · iteration 5 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 1 rejected · iteration 5
evidence per changed file