fix(Bun.password): accept optional v= segment in PHC bcrypt hashes#32316
fix(Bun.password): accept optional v= segment in PHC bcrypt hashes#32316robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 11 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)
Comment |
|
Updated 9:11 AM PT - Jul 8th, 2026
❌ @robobun, your commit cf98009 has some failures in 🧪 To try this PR locally: bunx bun-pr 32316That installs a local version of the PR into your bun-32316 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Change looks correct and well-tested to me, but since this is the password-verification parse path I'd like a human to sign off.
Extended reasoning...
Overview
This PR adjusts the hand-rolled PHC string parser in src/runtime/crypto/pwhash.rs::bcrypt::verify_phc to accept (and discard) an optional v=<version>$ segment between the $bcrypt$ algorithm id and the r=N rounds parameter. It also updates the doc comment and adds a regression test in test/js/bun/util/password.test.ts that exercises v=19, v=2b, and the no-version case across sync/async and auto-detected/explicit-algorithm paths.
Security risks
This is the bcrypt verification codepath for Bun.password.verify, so it is security-sensitive by nature. The change itself only relaxes the parser to accept a spec-defined optional segment; the rounds, salt decode, digest decode, recompute, and constant-time compare are all untouched. Discarding the version value cannot cause a hash to verify against the wrong digest, and any input that previously parsed successfully still parses to the same (rounds, salt, hash) triple — the new branch only fires on inputs that used to be rejected as InvalidEncoding. I don't see a way for this to weaken verification.
Level of scrutiny
Because it sits in crypto/password-hashing code, I'm deferring rather than auto-approving even though the diff is ~10 lines and mirrors the argon2 handling already present in the same file. The logic is straightforward Rust string parsing (strip_prefix + split_once) with proper error propagation on a malformed/truncated v= segment, and it restores the Zig std.crypto.phc_format behavior described in the PR body.
Other factors
No CODEOWNERS cover these paths. The bug-hunting pass found nothing. Test coverage is good (positive verify, negative verify, both sync and async). No outstanding human review comments on the PR.
There was a problem hiding this comment.
I didn't find any issues — the parser change is small and mirrors the argon2 path — but since this is in the password-verification codepath I'd like a human to confirm before merge.
Extended reasoning...
Overview
This PR adds ~10 lines to verify_phc in src/runtime/crypto/pwhash.rs so the hand-rolled PHC bcrypt parser skips an optional v=<version>$ segment between $bcrypt$ and r=N, restoring parity with both the PHC string-format spec and the Zig std.crypto.phc_format implementation this code replaced. The doc comment is updated and a regression test in test/js/bun/util/password.test.ts covers v=19 / v=2b / no-version, sync and async, auto-detected and explicit "bcrypt", with both matching and non-matching passwords.
Security risks
The change only widens what the parser accepts; the version value is read and discarded (bcrypt has no PHC-level versioning that affects the digest). Rounds, salt decode, 23-byte digest recompute, and the constant-time comparison are all untouched. I don't see a way for the discarded segment to influence verification outcome, bypass the cost check, or alter the comparison. Malformed inputs (v= with no trailing $) still surface as InvalidEncoding.
Level of scrutiny
Despite being mechanically simple and well-tested, this lives inside Bun.password.verify's bcrypt path — security-sensitive by definition. My policy is to defer crypto/auth changes to a human regardless of size.
Other factors
No CODEOWNERS cover these files. No prior human review on the PR. The bug-hunting pass found nothing. The argon2 branch in the same file already handles v= identically, so the pattern is established in-tree.
|
Rebased onto On build 70490 the only failure is Ready for review. |
The PHC string format allows an optional $v=<version>$ segment between the algorithm id and the parameters. Zig std.crypto.phc_format parses and discards it for bcrypt, but the hand-rolled parser in pwhash.rs expected r= immediately after $bcrypt$, so verifySync on a hash like $bcrypt$v=19$r=4$... threw InvalidEncoding instead of returning a bool.
robobun
force-pushed
the
farm/fdc38f35/bcrypt-phc-version-segment
branch
from
62fedec to
cf98009
Compare
There was a problem hiding this comment.
Change looks correct and mirrors the argon2 path in the same file, but since this is in the password-verification code path I'd prefer a human sign-off.
Extended reasoning...
Overview
This PR adds ~10 lines to src/runtime/crypto/pwhash.rs so the hand-rolled PHC-format bcrypt parser skips an optional v=<version>$ segment before reading r=, and adds a test in test/js/bun/util/password.test.ts covering v=19, v=2b, and no-version cases across sync/async and auto/explicit algorithm selection. This restores behavior the Zig implementation had via std.crypto.phc_format.deserialize, which the Rust port's parser dropped.
Security risks
The change is in Bun.password.verify's bcrypt PHC path — security-sensitive by location. Functionally it only widens what the string parser accepts (per the PHC spec, v= is an optional segment) and discards the value; the salt/hash base64 decode, rounds bounds check, digest recomputation, and constant-time compare are all untouched. I don't see a way this weakens verification — inputs that previously threw InvalidEncoding now go through the same comparison as before. Malformed v= with no trailing $ still errors via split_once('$').ok_or_else(invalid).
Level of scrutiny
The diff is small, mechanical, and mirrors the argon2 v= handling ~40 lines up in the same file. On its own merits I'd be comfortable approving, but password verification is squarely in the "auth/crypto" category where I default to deferring for a human glance rather than auto-approving.
Other factors
Bug-hunting found nothing. The new test asserts both true and false results (so it's not vacuously passing on a parser that returns true for everything), covers the no-v= case to guard against regressing the existing format, and the author confirmed it fails on the released binary with PASSWORD_INVALID_ENCODING. No outstanding reviewer comments and no prior review from me on this PR.
1 participant
Repro
Before: throws with
code: "PASSWORD_INVALID_ENCODING".After: returns
false(and returnstruefor a matching password).Cause
The PHC string format defines an optional
$v=<version>$segment between the algorithm id and the parameters. Zig'sstd.crypto.phc_format.deserialize(used bybcrypt.strVerify) parsed and discarded it, so$bcrypt$v=19$r=4$<salt>$<hash>decoded and ran the comparison. The Rust port's hand-rolled parser insrc/runtime/crypto/pwhash.rswent straight from$bcrypt$tor=, so"v=19".strip_prefix("r=")failed and surfaced asInvalidEncoding.Fix
Skip an optional
v=...$segment before readingr=, mirroring what the argon2 path in the same file already does.Verification
Added a test in
test/js/bun/util/password.test.tscoveringv=19andv=2b, sync and async, auto-detected and explicit"bcrypt". Fails on main withPASSWORD_INVALID_ENCODING, passes with this change. Fullpassword.test.tsis green.