crypto: accept argon2 PHC params in any order in Bun.password.verify - #32314
crypto: accept argon2 PHC params in any order in Bun.password.verify#32314robobun wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesArgon2 PHC verification
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reproduced with: bun -e 'const h=Bun.password.hashSync("password",{algorithm:"argon2id",memoryCost:16,timeCost:2});const r=h.replace(/m=(\d+),t=(\d+),p=(\d+)/,"t=$2,m=$1,p=$3");console.log(Bun.password.verifySync("password",r))'Fix is up in this PR; full Rebased onto main after #33909 (the CI status (build 72253): 284/286 green including |
|
Updated 8:35 AM PT - Jul 12th, 2026
❌ @robobun, your commit 63d1ac5 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32314That installs a local version of the PR into your bun-32314 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the merged pre-scan looks correct and the DoS limits are still enforced on every path rust-argon2 would actually compute on — but since this loosens input acceptance in the password-verification hot path, it's worth a quick human look.
Extended reasoning...
Overview
This PR modifies argon2::str_verify in src/runtime/crypto/pwhash.rs to accept PHC strings whose m=/t=/p= parameters appear in any order, restoring parity with the prior Zig phc_format behavior and with hashes produced by other ecosystems (PHP, Go). It does so by folding the two existing passes (the v=19 splice and the verify-time DoS-limit scan) into a single pre-scan that captures each param by name and, when needed, rebuilds the string in canonical $<alg>$v=19$m,t,p$<salt>$<hash> form before handing off to rust-argon2. The common case (v=19 present, canonical order) stays Cow::Borrowed with no allocation. A new test in password.test.ts covers all six permutations (sync + async, right + wrong password), composition with a missing v= segment, the DoS limit on a reordered segment, and rejection of missing/duplicate/unknown params.
Security risks
This is the argon2 password-verification path, so the relevant question is whether the refactor opens any way to (a) bypass the verify-time DoS limits, or (b) accept a hash that should be rejected. I traced every break 'norm Cow::Borrowed fall-through (no leading $, no alg separator, v= with no terminator, no params terminator, pair without =, value not u32-parseable, unknown key, duplicate key, fewer than three of m/t/p): in each case the unmodified string is handed to rust-argon2::decode_string, which requires the literal positional m,t,p shape and rejects with DecodingFail. So no fall-through reaches the actual hash computation, and every path that does compute has had the MAX_VERIFY_* limits applied. The v != "19" early-reject is preserved. The only intentional loosening is accepting reordered params, which matches the PHC spec and prior Bun behavior. I don't see an injection, bypass, or resource-exhaustion vector introduced here.
Level of scrutiny
High — this is crypto/auth code that loosens input acceptance in a verification function. Per the review guidelines I won't auto-approve changes in this category even when they look correct; a maintainer should sanity-check the fall-through reasoning above.
Other factors
The bug-hunting system found nothing. No CODEOWNERS entry covers this path. Test coverage for the new behavior is thorough (permutations, missing-version composition, DoS-limit preservation, malformed/duplicate/unknown rejection), and the full password.test.ts suite passes per the PR description. The diff is a clean consolidation of two adjacent passes into one with no change to the surrounding API surface.
1959892 to
f84ee24
Compare
There was a problem hiding this comment.
I didn't find any issues, but this loosens parsing on the password-verification path (security-sensitive crypto), so it's worth a human sanity check before merging.
Extended reasoning...
Overview
This PR changes argon2::str_verify in src/runtime/crypto/pwhash.rs to accept PHC-encoded argon2 hashes whose m=/t=/p= parameters appear in any order, restoring parity with the prior Zig phc_format behaviour and with hashes produced by other ecosystems (PHP, Go). It folds the existing version-splice and DoS-limit pre-scan into a single pass that captures each param pair and, when needed, rebuilds a canonical $<alg>$v=19$m=..,t=..,p=..$<salt>$<hash> string before delegating to rust-argon2. A new test in password.test.ts covers all six permutations, the missing-v= composition, the DoS-limit check on a reordered segment, and rejection of missing/duplicate/unknown params.
Security risks
This is the Bun.password.verify code path — auth/crypto by definition. The change broadens the set of encoded strings accepted on verify. I traced the logic and believe it's safe: the rebuilt string reuses the original key=value slices verbatim (just reordered), the per-param DoS limits are still enforced before any compute, and every malformed/unknown/duplicate case falls through as Cow::Borrowed to be rejected by rust-argon2 without hashing. One minor behavioural shift: parse failures in the params loop now break 'norm instead of continue, so a params segment with one malformed pair no longer gets the other pairs limit-checked here — but rust-argon2's decoder rejects such strings before any compute, so there's no DoS-bypass window. I did not spot a way to smuggle a higher cost or a different algorithm past the normaliser.
Level of scrutiny
High. Password verification is a security boundary, and this change relaxes input acceptance on that boundary. Even though the diff is small (~70 LoC net), well-commented, and thoroughly tested, my policy is not to auto-approve crypto/auth changes.
Other factors
- No CODEOWNERS entry covers these files.
- No outstanding human review comments.
- The bug-hunting system found no issues.
- Test coverage is comprehensive and the PR description includes a clear repro and before/after verification.
Deferring to a human reviewer purely on the security-sensitivity criterion; the implementation itself looks correct to me.
Zig std's phc_format deserialises the m/t/p parameters by field name in any order; rust-argon2's decode_string requires the literal positional order m,t,p and returns DecodingFail otherwise. Hashes emitted by other ecosystems (PHP password_hash, some Go wrappers) do not all use the canonical order, so verify() regressed to InvalidEncoding on strings that earlier Bun releases accepted. The pre-scan in str_verify already walked the params segment order-agnostically for the DoS limit check; extend it to capture each pair by name and rebuild the string in canonical m,t,p order (with the version segment inserted if absent) before handing off to rust-argon2. The happy path (version present, params already canonical) is still zero-alloc via Cow::Borrowed. Unknown / duplicate / malformed params fall through unchanged for rust-argon2 to reject as before.
f84ee24 to
d06d931
Compare
![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/crypto/pwhash.rs`:
- Around line 228-260: Update the parameter parsing in the normalization flow
around m_pair, t_pair, and p_pair so recognized values are stored without
returning WeakParameters immediately. First validate the complete set—exactly
one each of m, t, and p, with no malformed, duplicate, or unknown entries—and
preserve malformed encodings for rust-argon2 InvalidEncoding; only after this
validation apply MAX_VERIFY_MEMORY_COST, MAX_VERIFY_TIME_COST, and
MAX_VERIFY_PARALLELISM and return WeakParameters for oversized values. Add a
test covering a malformed or incomplete parameter set that also contains an
oversized recognized value, ensuring it remains InvalidEncoding regardless of
order.
🪄 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: b072f9a3-fa18-4e51-b904-140e39955f56
📒 Files selected for processing (2)
src/runtime/crypto/pwhash.rstest/js/bun/util/password.test.ts
…structurally valid A params segment that is both malformed (missing/duplicate/unknown key) and carries an oversized value now consistently surfaces as InvalidEncoding regardless of where the oversized value sits; the WeakParameters limit check fires only on a structurally-valid m/t/p triple.
There was a problem hiding this comment.
No bugs found, but deferring to a human reviewer since this touches the Bun.password.verify crypto path.
Checked: the reorder pass only rewrites the params segment when exactly one each of m/t/p is present (any other shape falls through unchanged to rust-argon2); the DoS limit check still fires on reordered segments and now runs only after structural validation so malformed+oversized stays InvalidEncoding; the canonical fast path stays Cow::Borrowed with no allocation; tail retains its leading $ so the rebuilt string is well-formed.
Extended reasoning...
Overview
Extends the existing PHC pre-scan in argon2::str_verify (src/runtime/crypto/pwhash.rs) to accept m=/t=/p= in any order, restoring parity with the earlier Zig phc_format behavior. The two pre-existing passes (splice missing v=19; enforce verify-time DoS limits) are merged into a single scan that also captures each param pair and, when non-canonical, rebuilds the string as $<alg>$v=19$m,t,p$<salt>$<hash> before delegating to rust-argon2::verify_encoded. A new test in test/js/bun/util/password.test.ts covers all six permutations, composition with a missing v= segment, DoS-limit enforcement on reordered params, and rejection of malformed/duplicate/unknown/oversized-but-malformed segments. CodeRabbit's one finding (order-dependent WeakParameters vs InvalidEncoding) was addressed in 63d1ac5 and the thread is resolved.
Security risks
This is a password-verification code path. The change loosens accepted input encodings, so the key question is whether it can cause a mismatched password to verify or bypass the DoS limits. I traced through and believe not: the normalizer only rewrites when the params segment is exactly {m,t,p} with parseable u32 values, otherwise it passes the original string through unchanged for rust-argon2 to reject; the DoS limit check is preserved and now applied after structural validation; and the actual constant-time hash comparison remains entirely inside rust-argon2. There is no new user-controlled allocation sizing (String::with_capacity is bounded by the input length).
Level of scrutiny
High — Bun.password is a user-facing credential-verification API. Even though the change is a bounded string-normalization step in front of an unchanged third-party verifier, crypto/auth paths in this repo warrant human sign-off rather than bot approval.
Other factors
Test coverage is thorough (permutations, sync+async, wrong-password, no-version composition, DoS limit, malformed variants). No outstanding reviewer comments. No CODEOWNERS entry for this path. The bug-hunting system found no issues.
1 participant
Repro
Bun 1.3.x (Zig) prints
truefor the same input.Cause
Zig std's
phc_format.deserializeintoHashResult{m,t,p}parseskey=valuepairs by field name and accepts them in any order. The Rust port delegates torust-argon2'sverify_encoded, whosedecode_optionsrequires the literal positional orderm,t,pand returnsDecodingFail(mapped toInvalidEncoding) otherwise.Hashes emitted by other ecosystems (PHP
password_hash, some Go argon2 wrappers) do not all use the canonicalm,t,porder, so externally-generated hashes that verified on earlier Bun releases now fail.Fix
str_verifyalready pre-scans the PHC string to splice in a missingv=19segment and to enforce the verify-time DoS limits, iterating the params segment order-agnostically. Extend that single pass to capture eachm=/t=/p=pair and, when the order is not already canonical (orv=is absent), rebuild the string as$<alg>$v=19$<m>,<t>,<p>$<salt>$<hash>before handing off torust-argon2. The common case (v=19present,m,t,palready canonical) staysCow::Borrowedwith no allocation. Unknown / duplicate / malformed params fall through unchanged forrust-argon2to reject as before.Verification
New test in
test/js/bun/util/password.test.tscovers all sixm/t/ppermutations (sync + async, correct and wrong password), the composition with a missingv=segment, the DoS limit check on a reordered segment, and rejection of missing/duplicate/unknown params.USE_SYSTEM_BUN=1 bun test test/js/bun/util/password.test.ts -t 'm/t/p in any order'→ fail (InvalidEncoding)bun bd test test/js/bun/util/password.test.ts→ 69 pass / 0 fail[review] gate passed · iteration 7 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 7
evidence per changed file