Bun.CSRF: validate expiresIn and maxAge with the shared integer validator - #40697
Merged
Merged
Conversation
…ator The Rust port of csrf_jsc.rs added a local get_optional_int_u64 helper. It threw TypeError [ERR_INVALID_ARG_TYPE] for every bad value, including out-of-range values, and it rejected NaN. The Zig version used the shared getOptionalInt, which routes through validateIntegerRange. Delete the local helper and call JSValue::get_optional_int::<u64>. Out of range values now throw RangeError [ERR_OUT_OF_RANGE], non-integers and non-numbers throw TypeError [ERR_INVALID_ARG_TYPE], and NaN is treated as 0, the same as undefined. This matches the rest of the API surface.
Contributor
|
Warning Review limit reached
On-demand reviews are free for the next 24 days. After that, they cost $0.25 per reviewed file. Or wait 25 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
Collaborator
Author
|
Status: ready for review. Reproduced on 1.4.0 and 1.4.1 with the script in the PR body. All four values threw Revision 2 (2e1b269): Verification:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
get_optional_int passes 0 as the validator default, and bun_csrf treats 0 as "no expiry". A NaN from a bad computation would then issue a token that never expires. Call validate_integer_range with DEFAULT_EXPIRATION_MS instead, so NaN gives the same 24h an absent property gets. The test decodes the token and checks the embedded expiresIn field.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
dylan-conway
approved these changes
Aug 28, 2026
Contributor
There was a problem hiding this comment.
Thanks — the fail-open concern from my earlier review is addressed: get_optional_duration_ms now passes csrf::DEFAULT_EXPIRATION_MS as the validator default, and the new test reads bytes 24..32 of the token to prove expiresIn: NaN embeds 86400000 rather than the 0 no-expiry sentinel. I have no remaining findings; since this changes behavior on a CSRF security primitive, a human sign-off is still worthwhile.
What was reviewed:
- Traced
validate_integer_range::<u64>insrc/jsc/JSGlobalObject.rs: NaN →default, non-number/non-integer →ERR_INVALID_ARG_TYPE, out-of-range (incl.Infinity) →ERR_OUT_OF_RANGE;IntegerRange::default().maxisMAX_SAFE_INTEGER, so thei64::try_fromon the reported bound cannot panic. - Confirmed
DEFAULT_EXPIRATION_MS = 24 * 60 * 60 * 1000insrc/csrf/lib.rsand that0is the "skip expiry check" sentinel there — the chosen default keeps NaN fail-closed. - Checked
target.get(global, field_name)still filtersundefinedtoNone, so an absent property never reaches the validator and bothexpiresIn/maxAgekeep their existing 24h initializers.
Extended reasoning...
Overview
This PR replaces a bespoke get_optional_int_u64 helper in src/runtime/api/csrf_jsc.rs with get_optional_duration_ms, which delegates to the shared JSGlobalObject::validate_integer_range::<u64>. Both Bun.CSRF.generate({ expiresIn }) and Bun.CSRF.verify({ maxAge }) now emit Node-style RangeError [ERR_OUT_OF_RANGE] for out-of-range values and TypeError [ERR_INVALID_ARG_TYPE] for non-number / non-integer inputs, and treat NaN as the 24-hour default expiration. Six new parameterized tests in test/js/bun/util/csrf.test.ts cover the matrix for both option names.
Security risks
The relevant risk was the one I flagged on the first revision: routing NaN through get_optional_int::<u64> would have selected T::ZERO as the default, and bun_csrf treats 0 as "no expiry" — a fail-open on a CSRF token. The current revision passes csrf::DEFAULT_EXPIRATION_MS (86400000) as the validator's default instead, and I confirmed at JSGlobalObject.rs:1295-1298 that is_nan() returns exactly that default. The new test decodes the big-endian u64 at bytes 24..32 of a generated token and asserts it equals 24 * 60 * 60 * 1000 (and separately that explicit expiresIn: 0 still embeds 0n), so the test distinguishes the two defaults and would fail on a regression to the fail-open behavior. No other security-relevant paths are touched; the HMAC, secret handling, and encoding paths are unchanged.
Level of scrutiny
Moderate. The diff is ~30 lines of Rust plus tests and is a mechanical swap to an existing, widely-used shared validator. However, it changes user-facing error semantics and the NaN handling of a security primitive, and one iteration of this PR did contain a fail-open. That concern has been addressed with code and a targeted test, but a human should still sign off on a behavior change to Bun.CSRF.
Other factors
Test quality is good per REVIEW.md: describe.each over both option names, exact { constructor, code, message } assertions rather than bare toThrow(), and the NaN test asserts the strongest invariant (the actual embedded value) rather than just "does not throw". I verified IntegerRange::default() sets max = MAX_SAFE_INTEGER, so the i64::try_from(range.max).expect(...) inside the validator cannot panic on this call site. The min: 0 bound is redundant with T::MIN_I128 for u64 but harmless and documents intent. All threads on the timeline are author-self-resolved, but the intervening commits demonstrably changed the code to address the objection.
robobun
pushed a commit
that referenced
this pull request
Aug 28, 2026
…rship contracts compile errors Rebased onto main as a single commit; the branch history (with its merge commits) is not preserved. Adds a zero-cost branded scope layer over the raw JSC FFI (`src/jsc/scope.rs`: `Scope<'s>`, `Local<'s>`) that makes two boundary bug classes compile errors for code that stays on the scoped API: - a JS value escaping its host call unrooted (persisting requires the explicit `Scope::persist` -> `Strong`); - a JS-heap view (`Local::array_buffer_bytes`) held across an operation that can re-enter user JS (`&mut Scope`), e.g. a coercion that detaches the buffer. Codegen integration: - `#[bun_jsc::host_fn(scoped)]`: functions written as `fn(scope: &mut Scope, callframe) -> JsResult<Local>` get a macro-synthesized wrapper under their original name and unscoped signature, so js2native / `.classes.ts` / direct-call wiring stays byte-compatible. User `cfg`/doc/lint attributes propagate to the public wrapper and the extern shims. - `ZIG_EXPORT(tag, reenters_js | no_user_js)` effect markers on the C++ declarations; explicitly classified functions get branded wrappers generated into `bun_jsc::cpp::scoped` (`&mut Scope` / `&Scope`), unclassified and `null_is_throw` functions get none. All classified exports are verified against their C++ (`toMatch` and `putMayBeIndex` are `reenters_js`: a non-uint32 `lastIndex` goes through ToNumber, and index puts on exotic receivers reach `defineOwnProperty` traps). Migration: ~470 host functions are converted to the scoped form (behavior-preserving); the remaining escape hatches (`unscoped_global()` / `unscoped_bun_vm()` / `.unscoped()` and unscoped `#[host_fn]`s) are pinned per file by `test/internal/source-lints/scope-escapes.test.ts`. Also expresses the `make_*_with_bytes_no_copy` "pointer stays valid until the deallocator runs" contracts as ownership transfer (`typed_array_from_owned_slice` / `_from_vec` / `_from_owner`, `ForeignBytes`, `external_string_from_utf16`, `OwnedUrl`, `OwnedTextCodec`, `StoreRef::adopt`, `bun_sys::Mmap`, `EventLoop::scope`), replacing hand-paired create/destroy and leak-and-remember-to-free code paths. The static `Bun.CryptoHasher.hash` / `Bun.password.verifySync` argument detach bugs that motivated the layer were fixed independently on main (#36165) by coercing every argument first; here the same behavior is expressed through deferred `materialize` under the shared scope borrow, so reordering the view capture before a coercion is a borrow error. Both main's regression tests and the layer's suites pass. Rebase onto main (461 commits): 40 files conflicted; resolved by taking main's text and re-applying only the scope transformation. Changes that main made obsolete were dropped (TextEncoderStreamEncoder host fns, EventLoop::with_pipe_read_buffer, JSC__JSMap__size global arg, the sendHelperChild scoping). Follow-ups main's newer code required: JSValue::create_buffer_from_foreign now returns JsResult (the binding became fallible on main), ArrayBufferSink::end_from_js uses or_pending_exception (empty-jsvalue-laundering lint), TextDecoder createForStream uses struct update syntax (clippy, since the PR removes TextDecoder's Drop impl), ForeignBytes recorded in the vm-thread-door inventory, scope-escape limits regenerated, and the ratchet's regeneration mode is gated on an explicit --update flag. Second rebase (30 more commits, onto 8bc4d2a): two conflicts from the zero-fill removal (#39417). The zstd sync functions keep main's Failure enum and create_buffer_from_box behind the scoped signatures, and the latin1 TextDecoder path keeps main's uninitialized Vec but hands it to JSC through external_string_from_utf16_vec instead of the raw to_external_u16, matching the file's other two decode paths. Third rebase (6 more commits, onto 0002bf8): conflicts were all with the dead-code sweeps (#39420, #39448). Dropped the scoping of things main deleted (Bun.nanoseconds' host fn, ArrayBufferSink::to_js, the unreachable csrf error arm, three unused node:: re-exports) and kept the ownership refactor of ArrayBufferSink::end_from_js. Inventories regenerated; the jsresult-swallow one also picks up a count #39448 left stale on main. Fourth rebase (17 more commits, onto 258517a): do_publish keeps #39389's shape (topic JSString held and ensure_still_alive'd across the message conversion) under the scoped signature, and the valkey publish scoping sits after the command block #29339 added. Fifth rebase (42 more commits, onto 6948a12): OwnedTextCodec is gone with the WebKit codecs (#39485); TextDecoder keeps main's encoding_rs path and this PR's external_string_from_utf16_vec hand-offs, including on the new path. ArrayBuffer::from_owned_bytes stays deleted (no callers; its u32 cast that #39558 fixed never existed in the replacements), and the no-copy deallocator contract now states both the Err-path timing (the deallocator can run before Err, per #39558) and the cross-thread timing this PR's Send bounds rely on. Sixth rebase (18 more commits, onto 681a49b): three dead-code conflicts. The scoped js_assert_settings goes away with the native assertSettings (#38900, node:http2 validates in JS now), ParseArgumentsCfg's unused Default impl stays removed (#39585), and TimeoutObject keeps main's generated cached-accessor import next to the scoped imports. Seventh rebase (14 more commits, onto 32e8703): valkey subscribe() keeps the structure #39547 gave it (channel type check first, dial plus send_rejection() before a listener is stored, no trailing else) with the rejection and the new check spelled through the scope. Eighth rebase (36 more commits, onto 56c4e3d): the three expect matcher utils take #36912's propagating print_value; the conflict was only the line wrapping. memory_pressure.rs (new on main) is added to the scope-escape limits. Ninth rebase (17 more commits, onto 72ec6e2, which includes the #39839 build fix): FileSink::on_close combines this PR's with_mut probe with the parameterless ReadableStream::done() and is_some() guard from #39732. Tenth rebase (5 more commits, onto 4448a2e): the Windows cluster handle path keeps #39804's `?` on attach_windows_socket_payload under the scoped argument spelling. Eleventh rebase (7 more commits, onto a21f02a): pbkdf2/pbkdf2Sync take the bodies #39922 gave them (from_js also returns the callback, pbkdf2 returns undefined, length 6) under the scoped signatures. Twelfth rebase (16 more commits, onto 6fb7102): the safe vm_loop_ctx is re-applied onto #40002's Cell-based upgrade client, including inside the new clear_data's with_mut. Thirteenth rebase (15 more commits, onto d0f6486): the assert binding is scoped (5 hatches now, was 1); #39995's named-pipe live-count testing fn is scoped like its neighbours. Fourteenth rebase (12 more commits, onto e8300da): cron_remove's tail takes #40024's safe ThisPtr start_linux call under the scoped return; the rest of cron.rs's Cell/ThisPtr rewrite auto-merged. node_crypto_binding's scope-escape limit rises by the two unscoped argon2 host fns #37015 added. Fifteenth rebase (6 more commits, onto 1423031): two conflicts with the defer-comment sweep (#40051): PasswordObject's verifySync keeps this PR's deferred materialize of both arguments, and NodeHTTPResponse's on_resolve keeps the scoped call, both without the removed defer comments. Sixteenth rebase (31 more commits, onto 01008f8): the socket ref()/unref() host fns take #39856's bodies (hold the loop while connecting, apply the recorded state on open) under the scoped signatures. Seventeenth rebase (8 more commits, onto 4bb20e5): #40055 made the websocket upgrade client's loop-context plumbing safe itself (and dropped the adapter), so this PR's vm_loop_ctx change there is retired and both http_jsc files are main's. Eighteenth rebase (8 more commits, onto 3e347b3): the rootError crash hook keeps #37181's one-argument handle_root_error under the scoped signature. Nineteenth rebase (6 more commits, onto 7a5d837): #40251 removed the exception checks that follow already-checked calls and made JSString::to_slice / view, JSValue::get_zig_string and handle_ipc_message return JsResult. Eight files conflicted inside scoped bodies (BunObject, CryptoHasher, PasswordObject, ipc_host, node_util_binding, server_body, expect, ObjectURLRegistry); main's control flow is kept (the guards go, the ? is added) under the scoped spellings. The four has_exception checks left in BunObject.rs are the ones main kept (print_table / format2 swallow nested throws). Twentieth rebase (9 more commits, onto f2fe7d3): 32 files conflicted, nearly all with #40238 (bun_core::String owns its WTF ref). Main's ownership idioms replace this PR's: OwnedString / scopeguard deref wrappers and manual .deref() calls go (String drops its ref), into_js replaces transfer_to_js (Scope::transfer_string now consumes the String), JSValue::get_zig_string is gone so Local::get_zig_string becomes Local::to_js_string_view (the JSStringView guard keeps the cell alive), and to_slice_or_null collapses into to_slice. OwnedUrl is retired: main's whatwg::Parsed is the same RAII handle, so src/jsc/URL.rs and js_valkey.rs are main's again. ScopeFunctions.rs is rebuilt from main's text (the strings module is gone, names are &'static str) with the 14 host fns scoped and rustfmt applied; jest.rs and expect.rs take main's literals under this PR's wrapping. CachedStructure keeps main's assume_init_mut / drop_in_place sequence over this PR's slice-taking create_structure. UDP address getters add the ? main's create_sock_addr now needs. Scope-escape limits drop by one in BunObject, node_util_binding and server_body and by two in FormData (hatches replaced by scoped calls). Twenty-first rebase (7 more commits, onto 8335017): one import-line conflict in node_fs_binding.rs, where #38383 added the SystemErrorJsc trait import next to this PR's scoped imports. Both kept; no inventory changes. Twenty-second rebase (6 more commits, onto 0823e50): 39 files conflicted, all with #40374 (Utf8Bytes<'a> / EncodedSlice<'a>). Main's types replace the PR's spellings inside scoped bodies: Local::to_slice is now Local::to_utf8 (Utf8Bytes<'static>), ZigString::init(..).to_js and create_utf8_for_js calls become scope.string_utf8 / scope.string, and ScopedStringOrBuffer names StringOrBuffer<'static>. Main's owned_utf16_into_js supersedes this PR's external_string_from_utf16*, so src/jsc/ZigString.rs stays deleted and bun_string_jsc.rs and TextDecoder.rs are main's again. Scope-escape limits drop in filesystem_router (13 to 7), server_body (17 to 15) and Listener (11 to 9). Twenty-third rebase (9 more commits, onto adc354d): two files. FileSystemRouter::routes takes #40410's fallible JSValue::from_entries (mapped into the scope), and advanceTimersByTime keeps #40414's NaN check and main's message text under the scoped throws. The jsresult-swallow inventory is main's again (#40410 fixed the FakeTimers entry). Twenty-fourth rebase (9 more commits, onto 82123d3): six files, all with #40478 (RefPtr releases on Drop). This PR's StoreRef::adopt is retired: main's RefPtr<Store> is the same owning handle, so webcore_types.rs is main's again and store_backed_buffer_to_js moves a RefPtr<Store> into the JS object as the *_from_owner owner (the view closure reaches the bytes through Store::data_mut). The sql event-loop guard keeps this PR's safe EventLoop::scope under main's renamed ref guard; expect.rs keeps this PR's wrapping over main's RefPtr comments. The vm-thread-door inventory follows main's StoreRef-to-Store rename. Twenty-fifth rebase (23 more commits, onto 0e395c2): four files, all with #40511 (async fs calls no longer pin Buffer paths). pbkdf2 and scrypt take main's from_js_async parsers (ThreadIsolated params) under the scoped signatures, StringOrBuffer keeps main's from_js_async next to this PR's from_js_scoped / from_js_deferred, and the BlobOrStringOrBuffer::from_js_async this PR's insertion sat beside is gone with main. Import merges in node.rs and MarkdownObject.rs. The vm-thread-door inventory follows main's ThreadSafe-to-ThreadIsolated rename. Twenty-sixth rebase (8 more commits, onto 72ffcd8): one import-line conflict in ffi_body.rs, where #40592 added ErrorCode next to this PR's scoped imports. Both kept; no inventory changes. Twenty-seventh rebase (24 more commits, onto 49ff888): five files, all with #40516 (refcounted types own their teardown). The serve-plugins .then callbacks adopt their ref through main's RefPtr::from_raw under the scoped argument spellings (this PR's ServePluginsRef guard is gone with main's newtypes), FileSink keeps this PR's with_mut spelling over main's RefPtr<FileSink> construction (create is main's one-liner), the StatWatcher deinit hook stays deleted next to the scoped do_ref, and ipc_host.rs / socket_body.rs are import and return-spelling merges. No inventory changes. Twenty-eighth rebase (36 more commits, onto 69c6138): one import-line conflict in csrf_jsc.rs, where #40697 added IntegerRange next to this PR's scoped imports. Both kept; no inventory changes.
This was referenced Aug 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Bun.CSRF.generate({ expiresIn })andBun.CSRF.verify({ maxAge })throwTypeError [ERR_INVALID_ARG_TYPE]for every bad value. Out-of-range values such as-1and2 ** 53must throwRangeError [ERR_OUT_OF_RANGE].NaNthrows where the option should be treated as absent.get_optional_int_u64helper insrc/runtime/api/csrf_jsc.rs:27. The Rust port added it. The Zig version called the sharedgetOptionalInt, which routes throughvalidateIntegerRange. The helper's doc comment cites tests that do not exist.Fix
get_optional_int_u64. Both options now go throughJSGlobalObject::validate_integer_range::<u64>(src/jsc/JSGlobalObject.rs:1241), withdefault = DEFAULT_EXPIRATION_MSandmin = 0.-1,2 ** 53,Infinity) throwRangeError [ERR_OUT_OF_RANGE]. Non-integers (1.5) and non-numbers ("100") throwTypeError [ERR_INVALID_ARG_TYPE].NaNgives the 24h default, the same as an absent property.0on purpose.JSValue::get_optional_intpasses0as the validator default, andbun_csrftreats0as "no expiry". ANaNfrom a bad computation would then issue a token that never expires.test/js/bun/util/csrf.test.ts(6 new tests fail on 1.4.0, pass with this change). The whole file passes (31 tests).Background
validate_integer_rangeis Bun's port of node's integer option validators. It returns the default forundefinedandNaN, throwsERR_INVALID_ARG_TYPEwhen the value is not a number or not an integer, andERR_OUT_OF_RANGEwhen the value is outside[min, max], with min and max clamped to the safe integer range. Foru64that is[0, 9007199254740991].JSValue::getreturnsNonefor a missing property and forundefined. So{ expiresIn: undefined }and no option both keep the 24h default. OnlyNaNreaches the validator's default path.timestamp (8) | nonce (16) | expiresIn (8, big-endian u64) | HMAC.bun_csrf::verifyskips the expiry check when that field is0, and skips themaxAgecheck whenmax_age_msis0. The test reads bytes 24..32 to check which defaultNaNselects.Supersedes #32288, which mapped
NaNto0and is stale.Notes
Repro on 1.4.0 and 1.4.1:
With this change:
The same applies to
verify({ maxAge }).The Zig source before the port (
src/runtime/api/csrf_jsc.zigat b8ecc78) read both options withoptions_value.getOptionalInt(globalObject, "expiresIn", u64). That mappedNaNto0too. The first revision of this PR did the same throughget_optional_int::<u64>. Review pointed out that0disables expiry, so the second revision passesDEFAULT_EXPIRATION_MSas the default instead. The newNaNtest fails against the first revision, which shows it tells the two defaults apart.Other call sites in the tree handle the same hazard explicitly:
js_bun_spawn_bindings.rsrejects aNaNtimeoutbefore the validator because0would mean no timeout, andsocket_body.rsrejects aNaNtos.The test does not assert the
nullcase. The shared validator reportsReceived objectfornull(fromjsTypeStringForValue). That wording is owned by the shared helper, not by this file.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file