Repository navigation
Conversation
Bun.password.hash accepted any timeCost >= 1 and memoryCost >= 8, while Bun.password.verify runs a prescan that rejects PHC params above t > 65536, m > 4194304, or p > 64 with WeakParameters. A hash produced with e.g. timeCost: 65537 therefore could never be verified by Bun. Separately, the cost options were read via JSC ToInt32 (mod 2^32 wrap), so values like memoryCost: 2**32 + 64 silently became m=64 instead of throwing. hash() now validates timeCost and memoryCost against the same upper bounds that verify() enforces, comparing the f64 value directly so out-of-range inputs throw instead of wrapping. bcrypt cost gets the same wrap-free range check.
|
Warning Review limit reached
On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file. Or wait 36 seconds for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found — the validation tightening looks correct and well-tested, but since this touches crypto option parsing in src/runtime/crypto/ and CODEOWNER'd packages/bun-types/, deferring to a human for sign-off.
Extended reasoning...
Overview
This PR aligns Bun.password.hash's argon2 timeCost/memoryCost upper bounds with the ceilings that str_verify already enforces on incoming PHC strings, so hash() can no longer produce a hash that Bun's own verify() refuses. It also fixes silent mod-2^32 wrapping by comparing the raw f64 (as_number()) instead of coerce_to_i32, and gives bcrypt cost the same treatment. Changes: two consts made pub(crate) in pwhash.rs, three option-parse blocks in PasswordObject.rs, new tests in password.test.ts, and a JSDoc update in bun.d.ts (added after my earlier nit).
Security risks
The change strictly narrows accepted input — it rejects more values, not fewer — and does not touch the crypto primitives themselves. NaN/Infinity/out-of-range now throw instead of wrapping to a low cost, which is a security improvement (previously 2**32 + 2 silently became timeCost: 2). The is_number() guard precedes each as_number() call, and the f64 range check runs before the as u8/as u32 cast, so no new UB or truncation surprises. Fractional values still truncate toward zero, matching the old coerce_to_i32 behavior.
Level of scrutiny
This is input validation in src/runtime/crypto/, which is security-adjacent even though no cipher code changes. Per policy I don't auto-approve crypto-path changes. Additionally, packages/bun-types/ is CODEOWNER'd (@alii), so the .d.ts hunk needs owner review even though it's a two-line JSDoc tweak.
Other factors
The bug-hunting pass found nothing. Test coverage is thorough: one-past-ceiling values, 2**32 + N wrapping, 2**31 boundary, all three argon2 variants, bcrypt cost, plus a release-only round-trip at the exact t=65536 ceiling. The PR description confirms USE_SYSTEM_BUN=1 fails 6 tests and bun bd test passes. My earlier nit (document the new bounds in bun.d.ts) was addressed in a460898. The change is small and mechanically sound; deferral is purely because of the crypto/CODEOWNER policy gates, not any concern with the code.
There was a problem hiding this comment.
I didn't find any bugs — the f64 range checks after is_number() are sound and the bounds derive from the single source of truth in pwhash.rs — but since this tightens the accepted value range on a user-facing crypto API and overlaps #32279, it's worth a human sign-off.
Extended reasoning...
Overview
This PR aligns Bun.password.hash()'s argon2 timeCost/memoryCost upper bounds with the ceilings that Bun.password.verify() already enforces on PHC params, and fixes silent mod-2³² wrapping of out-of-i32-range cost options (including bcrypt cost). Changes: PasswordObject.rs swaps coerce_to_i32 for as_number() + f64 range checks at three sites; pwhash.rs promotes two private constants to pub(crate); bun.d.ts documents the new bounds; password.test.ts adds boundary/wrap coverage.
Security risks
None identified. The change is input-validation-only and strictly tightens accepted ranges — no crypto primitive logic is touched. The is_number() guard precedes every as_number() call, so the f64 read is sound; NaN/±Infinity are correctly rejected by RangeInclusive::contains; the post-check as u8/as u32 casts are safe because the f64 is proven in-range. The removed & 0x3F mask on bcrypt cost was redundant given the 4..=31 check.
Level of scrutiny
Moderate. The code is mechanically simple and well-tested, but it lives in src/runtime/crypto/ and changes the accepted value range of a user-facing password-hashing API — previously-accepted calls with timeCost > 65536 or memoryCost > 4194304 will now throw. That's the correct behavior (Bun was producing hashes its own verify refused), but it's a user-visible API contract change that a maintainer should acknowledge.
Other factors
- My earlier docs nit was addressed in a460898 and the thread is resolved.
- The PR description notes overlap with #32279 (which touches the same three
PasswordObject.rssites); a human should confirm the intended merge order. - Test coverage is thorough: boundary-at-ceiling round-trip, one-past-ceiling rejection, 2³²-wrap cases across all three argon variants plus bcrypt, and the release-only t=65536 hash→verify round-trip.
|
CI status at 216f464:
Nothing red touches |
…d-argon2-cost-bounds # Conflicts: # src/runtime/crypto/PasswordObject.rs # test/js/bun/util/password.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Merged The bug still reproduces on bun 1.4.3: |
Problem
Bun.password.hashaccepts argon2timeCostup to 4294967295 andmemoryCostup to 4294967295 KiB.Bun.password.verifyrefuses any PHC string witht > 65536,m > 4194304, orp > 64(PASSWORD_WEAK_PARAMETERS, a DoS prescan instr_verify,src/runtime/crypto/pwhash.rs:230). Sohash()can produce a hash that Bun's ownverify()rejects:AlgorithmValue::from_js(src/runtime/crypto/PasswordObject.rs:100-135). It range-checks both options againstu32::MAX, not against the verify ceilings.Fix
MAX_VERIFY_TIME_COST(65536) andMAX_VERIFY_MEMORY_COST(4194304) frompwhash::argon2aspub(crate)and use them as the upper bounds infrom_js. One check per option now covers integer, lower bound, and upper bound, the same shape the bcryptcostcheck already has."Time cost must be an integer between 1 and 65536","Memory cost must be an integer between 8 and 4194304".bun.d.tsJSDoc documents the same bounds.hash()accepts,verify()accepts.test/js/bun/util/password.test.ts(release bun 1.4.3 fails 7, debug build passes 78, skips 9 debug-gated slow cases). A release-only test round-tripstimeCost: 65536through hash and verify.Background
$argon2id$v=19$m=...,t=...,p=...$).verify()must run argon2 with whatever the string says, so an attacker-supplied string with hugem/tis a CPU and memory DoS.str_verifyprescans the params and rejects values above fixed ceilings before it computes anything.hash()options come from the developer, not an attacker, so they had only the lower bounds argon2 itself needs (t >= 1,m >= 8 * pwithp = 1). Bun.udpSocket, Bun.password: range-check numeric options before ToInt32 narrowing #36999 later added the integer andu32::MAXchecks. Neither matched the verify ceilings.Notes
maininto this branch on 2026-09-06 to resolve conflicts with Bun.udpSocket, Bun.password: range-check numeric options before ToInt32 narrowing #36999 (range-check before ToInt32 narrowing) and Bun.password: verify argon2 hashes with memoryCost below 8 again #39596 (legacym < 8verify). Bun.udpSocket, Bun.password: range-check numeric options before ToInt32 narrowing #36999 already fixed the mod-2^32 wrap that the first version of this PR also addressed, so the remaining diff is only the upper bounds and the messages.get_truthypassesNaNand0through (it filters only undefined, null, and empty string), soNaN,±Infinity, and fractional values all hit the single range check:NaN.fract() != 0.0is true, andRangeInclusive::contains(&NaN)is false.coerce_to_i32call sites. Those sites no longer exist on main.Previous description (before #36999 landed):
Repro
Cause
src/runtime/crypto/PasswordObject.rsreadstimeCost/memoryCostviacoerce_to_i32(ECMA ToInt32, wraps mod 2^32) and validates only the lower bounds (>= 1/>= 8).src/runtime/crypto/pwhash.rsstr_verifyruns a DoS prescan on the PHC params of every input, rejectingt > 1<<16,m > 1<<22,p > 64asWeakParameters. The two bounds disagree, sohash()can mint PHC strings thatverify()refuses. The bcryptcostoption has the same ToInt32 wrap (2**32 + 5silently becomescost: 5).Fix
Expose
MAX_VERIFY_TIME_COST/MAX_VERIFY_MEMORY_COSTfrompwhash::argon2and apply them as upper bounds inAlgorithmValue::from_js. Compare the JS number asf64(viaas_number()) before casting so out-of-range and non-finite values throw instead of wrapping. The bcryptcostrange check gets the same f64 treatment.Error messages now state both bounds (
"Time cost must be between 1 and 65536","Memory cost must be between 8 and 4194304").Overlaps #32279, which swaps
coerce_to_i32for saturatingcoerce::<i32>at the same three sites but intentionally leaves the missing upper bound as an orthogonal concern. This PR supersedes thePasswordObject.rshunk of #32279; the other files in that PR are untouched here.Verification
test/js/bun/util/password.test.ts:USE_SYSTEM_BUN=1 bun testfails 6 tests (hash acceptstimeCost: 65537,memoryCost: 4194305, and2**32 + N)bun bd testpasses 72, skips 9 (debug-gated slow argon2 cases)[review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file