csrf: accept NaN for expiresIn/maxAge instead of throwing - #32288
csrf: accept NaN for expiresIn/maxAge instead of throwing#32288robobun wants to merge 2 commits into
Conversation
|
Updated 5:48 AM PT - Jun 20th, 2026
❌ @robobun, your commit eb6d073 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32288That installs a local version of the PR into your bun-32288 --bun |
|
Same src fix landed independently on That branch has an additional test asserting the error constructor/ |
WalkthroughThe local ChangesCSRF integer option parsing refactor
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — both nits addressed in 897bcf6, shim deletion routes to the established validate_integer_range path and the new error-code assertions lock it in.
Extended reasoning...
Overview
Deletes a 27-line local get_optional_int_u64 shim in src/runtime/api/csrf_jsc.rs and replaces its two call sites (expiresIn in csrf__generate, maxAge in csrf__verify) with the canonical JSValue::get_optional_int::<u64>. Adds a NaN-acceptance test and six error-code assertions to test/js/bun/util/csrf.test.ts.
I verified the target helper at src/jsc/JSValue.rs:1516 and src/jsc/JSGlobalObject.rs:1271: it has the exact signature used, treats NaN as the passed default (T::ZERO), and emits ERR_OUT_OF_RANGE for out-of-range / ERR_INVALID_ARG_TYPE for non-integer and non-number — matching the PR description and the new test assertions.
Security risks
This touches CSRF option parsing, but not the HMAC/crypto path. The only semantic change is NaN → 0 (no expiry) instead of throwing, which restores parity with the Zig reference and node's integer validators. NaN is an explicit caller error; the new test comment documents that this differs from omitting the option (24h default). No new attack surface — the shared validator is already used throughout the runtime and the actual token generation/verification logic is untouched.
Level of scrutiny
Low-to-medium. The src change is a -27/+2 mechanical refactor from a buggy local copy to a well-established shared helper; an independent branch (farm/3b9429a3) arrived at a byte-identical src diff. The behavior is now pinned by tests covering both the NaN happy path and all three rejection error codes on both options.
Other factors
I left two nits on the previous revision (test-title wording about "the default", and missing error-code coverage). Both were addressed in 897bcf6 and the inline threads are resolved. No bugs found by the bug-hunting system. Full suite (25 tests) reported passing.
There was a problem hiding this comment.
LGTM — both nits addressed in 897bcf6; shim deletion + shared validator is the right fix.
Extended reasoning...
Overview
This PR deletes a 27-line local get_optional_int_u64 shim in src/runtime/api/csrf_jsc.rs and replaces its two call sites (expiresIn in csrf__generate, maxAge in csrf__verify) with the shared JSValue::get_optional_int::<u64> helper. The shim's num.fract() != 0.0 check incorrectly rejected NaN; the shared helper routes through validate_integer_range, which treats NaN as the validator fallback (0) per node's integer-validator convention. Tests now cover NaN acceptance and lock in the restored ERR_OUT_OF_RANGE / ERR_INVALID_ARG_TYPE codes for negative, fractional, and non-number inputs on both options.
Security risks
CSRF is security-adjacent, but this change removes a divergent local reimplementation in favor of the canonical, runtime-wide validator — it does not introduce new validation logic. The one behavioral relaxation (NaN → 0 = no expiry, instead of throwing) restores parity with the pre-Rust-port Zig path and node's validators, and is now explicitly documented and tested. No injection, auth, or data-exposure surface is touched.
Level of scrutiny
Low-to-moderate. The source diff is a strict simplification: -27 lines of bespoke validation, two one-line call-site swaps to an existing, widely-used helper (verified get_optional_int is live in src/jsc/JSValue.rs and not cfg-gated). The behavioral delta is narrow and fully covered by the new tests.
Other factors
I left two nits on the first revision (test wording around "the default", and missing error-code assertions); both were addressed in 897bcf6 and the inline threads are resolved. The robobun note about the parallel farm/3b9429a3 branch's extra error tests is effectively covered by the six new toThrow assertions. The single CI failure (terminal.test.ts on macOS x64) is unrelated to this change. No CODEOWNERS entry for this path. No bugs flagged by the bug-hunting pass.
|
Diff is green. Rebased onto main twice:
Build 63609 (latest, eb6d073): 227/230 lanes passed. The 3 failures are all darwin-only and in unrelated subsystems:
Ready for a maintainer to merge. |
8cbf0aa to
44dfd25
Compare
There was a problem hiding this comment.
LGTM — both nits addressed; shim deletion routes to the shared validator and the new tests lock in NaN-acceptance and the restored error codes.
Extended reasoning...
Overview
The PR deletes a 27-line local get_optional_int_u64 shim in src/runtime/api/csrf_jsc.rs and replaces its two call sites (expiresIn in csrf__generate, maxAge in csrf__verify) with the canonical JSValue::get_optional_int::<u64> helper. The shim had a NaN bug (num.fract() != 0.0 is true for NaN) and collapsed three distinct error codes into one generic message. Tests in test/js/bun/util/csrf.test.ts add a NaN-acceptance case and six error-code assertions covering negative / fractional / non-number inputs on both options.
Security risks
None introduced. CSRF is security-adjacent, but this change doesn't touch HMAC generation, secret handling, constant-time comparison, or token verification logic — only the integer-option parsing path. The one semantic note (NaN → 0 = no expiry, which differs from omitting the option → 24h) was raised in my earlier review, is now explicitly documented in the test comment, and matches both the Zig reference (validateIntegerRange) and Node's integer validators. I confirmed at src/jsc/JSGlobalObject.rs:1325-1327 that validate_integer_range deliberately returns default for NaN.
Level of scrutiny
Low-to-medium. The src diff is a net deletion that swaps a buggy bespoke validator for a well-exercised shared one (get_optional_int is used across the runtime per the PR description, and I verified its definition at src/jsc/JSValue.rs:1516). No new code paths are introduced; behavior converges on the existing reference.
Other factors
- Both of my prior nits (test wording, missing error-code coverage) were addressed in the follow-up commit and the inline threads are resolved.
- robobun reports the test passes on all 224 completed lanes including ASAN; the only red is darwin agent capacity (infrastructure) and an unrelated PTY flake.
- A sibling branch (
farm/3b9429a3/csrf-range-error) reached a byte-identical src fix independently, and its extra error-code tests are now effectively covered by the assertions added here. - Bug hunting system found no issues.
The Rust port introduced a local get_optional_int_u64 shim in csrf_jsc.rs
whose num.fract() != 0.0 check is true for NaN, causing
Bun.CSRF.generate(secret, {expiresIn: NaN}) to throw ERR_INVALID_ARG_TYPE.
The Zig reference routes through validateIntegerRange which explicitly
treats NaN as the default value.
Drop the local shim and use JSValue::get_optional_int::<u64>, which already
delegates to validate_integer_range with the correct NaN handling. This also
restores the proper error codes (ERR_OUT_OF_RANGE for negatives,
ERR_INVALID_ARG_TYPE for non-integers) that the shim had collapsed into a
single generic message.
44dfd25 to
eb6d073
Compare
There was a problem hiding this comment.
LGTM — both nits addressed; the shim deletion just routes expiresIn/maxAge through the shared get_optional_int::<u64> path with full test coverage.
Extended reasoning...
Overview
This PR deletes a 27-line local get_optional_int_u64 shim in src/runtime/api/csrf_jsc.rs and replaces its two call sites (expiresIn in csrf__generate, maxAge in csrf__verify) with the shared JSValue::get_optional_int::<u64> helper. The shim's num.fract() != 0.0 check incorrectly rejected NaN; the shared helper routes through validate_integer_range, which treats NaN as the fallback (0) and emits proper ERR_OUT_OF_RANGE / ERR_INVALID_ARG_TYPE codes for the remaining rejection paths. The test file gains a NaN-acceptance test plus six error-code assertions covering negative, fractional, and non-number inputs on both options.
Security risks
CSRF is security-adjacent, but this change does not touch the HMAC generation, constant-time comparison, or token encoding logic — only the input-validation wrapper for two numeric options. The behavioral delta is that {expiresIn: NaN} now yields a non-expiring token instead of throwing; this restores the pre-Rust-port behavior (Zig's validateIntegerRange) and matches node's integer validators. No existing call site is silently weakened, since the previous behavior was a hard throw. I see no injection, auth-bypass, or data-exposure surface here.
Level of scrutiny
Low–medium. The src change is a net code deletion that converges on an existing, widely-used helper (get_optional_int at src/jsc/JSValue.rs:1516), so the new code path is already exercised across the runtime. The risk profile is closer to "remove duplicate buggy reimplementation" than "new validation logic".
Other factors
- Both of my earlier review nits (test the restored error codes; clarify the NaN-vs-omitted wording) were fully applied in 897bcf6 and are visible in the current diff; threads are resolved.
- The bug-hunting system found no issues.
- CI is reported green across builds 62449/62577/62733 including ASAN; remaining red is unrelated darwin infra/flakes.
- No CODEOWNERS entry matches
src/runtime/api/orcsrf. - An independent branch arrived at the byte-identical src fix, which raises confidence that this is the canonical resolution.
1 participant
Repro
Cause
The Rust port of
csrf_jsc.rsintroduced a localget_optional_int_u64shim whosenum.fract() != 0.0check evaluates to true for NaN (sinceNaN.fract()is NaN andNaN != 0.0is true), so it threw. The Zig reference routes throughvalidateIntegerRange(src/jsc/JSGlobalObject.zig) which explicitly treats NaN as the default value, following node's integer validators.#32519 later added a doc comment to the shim stating it "differs from
JSValue::get_optional_int::<u64>in rejecting NaN and in the error message wording expected by existing tests". Neither claim holds: the Zig reference accepted NaN (so rejecting it is a regression, not a choice), and no test asserted on the shim's error wording before this PR. 1.3.14 (pre-port) accepted{expiresIn: NaN}.Fix
Delete the local shim and call
options_value.get_optional_int::<u64>(global, "expiresIn")/"maxAge"directly. This also restores the proper error codes for the remaining rejection paths:ERR_OUT_OF_RANGEERR_INVALID_ARG_TYPE("integer")ERR_INVALID_ARG_TYPE("number")which the shim had collapsed into a single generic message.
Verification
test/js/bun/util/csrf.test.ts: new test assertsgenerate({expiresIn: NaN})returns a token andverify({maxAge: NaN})accepts it. The error-handling block now asserts all three error codes for bothexpiresInandmaxAge. All fail on main, pass with this change. Full suite (25 tests, 57 assertions) passes.