Bun.password: accept memoryCost below 8 when hashing again

[cirospaciari]

Stacked on #39596 (base branch claude/bun-password-legacy-argon2-memory).

What does this PR do?

Bun.password.hash(pw, { memoryCost: 1..7 }) works again, as it did before the Rust port. The PHC string carries the user's m ($argon2id$v=19$m=4,t=2,p=1$…) and the hash is computed with argon2's padded 8 blocks — byte-compatible with what Bun 1.3.x produced and verified. The docs' memoryCost: 4 example is valid again.

History: the port first silently clamped these to m=8 (#30960), then rejected them with "Memory cost must be at least 8" (#36999). #39596 vendors rust-argon2 without the m >= 8 * lanes floor so verification of such hashes works; this PR lifts the matching restriction on hashing. The lower bound returns to memoryCost >= 1 with the pre-port error wording (Memory cost must be greater than 0, …an integer between 1 and 4294967295).

How did you verify your code works?

• Drove the debug binary: hashSync({ algorithm: "argon2id", memoryCost: 4, timeCost: 1 }) → $argon2id$v=19$m=4,t=1,p=1$…, verifies true / false for a wrong password; same for m=1 argon2d and m=7 argon2i. m=0, -3, NaN throw "greater than 0"; 2.5, Infinity, 2**32 throw the integer-range error.
• Interop: 9 hashes generated by this build with m ∈ {1,4,7} × {argon2id, argon2i, argon2d} all verify on Bun 1.3.14, and 1.3.14's m<8 hashes verify here.
• bun bd test test/js/bun/util/password.test.ts: 84 pass, 0 fail (new round-trip tests for m=1/4/7 × 3 variants; the "must be at least 8" assertions updated).

[robobun]

^{Updated 5:25 PM PT - Aug 18th, 2026}

@cirospaciari, your commit 3ebd546 is building: #100933

[claude]

LGTM — the docs/types feedback from the earlier pass is addressed, and no new issues turned up on the follow-up commit.

What was reviewed:

• The < 8.0 → < 1.0 bound and both error strings in PasswordObject.rs — matches the pre-port timeCost wording and the NaN/fract/u32::MAX guards are unchanged.
• New round-trip tests (m∈{1,4,7} × 3 variants) assert the encoded m= verbatim and verify true/false; updated error-message assertions match the new strings.
• Confirmed the three "minimum 8" references (bun.d.ts JSDoc, both hashing docs) are now updated as the author described.
• The leftover "at the 8 minimum" test title was flagged again and ruled out as cosmetic — its assertions still hold.

Extended reasoning...

Overview

Stacked on #39596. This PR reverts the memoryCost lower bound in Bun.password.hash from 8 back to 1, restoring the pre-Rust-port behavior where memoryCost: 1..7 is accepted, encoded verbatim in the PHC string, and computed with argon2's internal 8-block padding (the padding itself lives in the base PR's vendored rust-argon2). Touches: src/runtime/crypto/PasswordObject.rs (4 lines: one comparison, one comment, two error strings), test/js/bun/util/password.test.ts (updated error-message assertions + 9 new round-trip tests), packages/bun-types/bun.d.ts (JSDoc), and two docs pages.

Security risks

This is the password-hashing module, so crypto-adjacent, but the change does not touch any cryptographic computation. Values below 8 still hash with 8 KiB of memory (per the vendored patch in #39596); only the input-validation floor and the encoded m= label change. The security posture is identical to Bun 1.3.x, which shipped this exact behavior — the PR description confirms bidirectional interop with 1.3.14. A user passing memoryCost: 1 gets a weak hash, but that was already true at memoryCost: 8 and is an explicit opt-in; the API default (65536) is untouched. No new attack surface: the upper bound, integer/NaN checks, and verify-side ceiling checks are all unchanged.

Level of scrutiny

Low-to-medium. The Rust diff is a mechanical 4-line revert of a validation floor to its pre-port value with matching error wording (mirroring the adjacent timeCost block exactly). The substantive work — patching rust-argon2 to pad rather than reject — is in the base PR, not here. The new tests are strong: they assert the exact PHC prefix ($${algorithm}$v=19$m=${memoryCost},t=1,p=1$) and both the positive and negative verify path, across all three argon2 variants and three sub-8 values.

Other factors

My earlier review on this PR flagged three stale "minimum 8" references in bun.d.ts and the docs; the author addressed all three in the follow-up commit (verified in the current diff: JSDoc now reads "Values below 8 still use 8 KiB", both docs drop "(minimum 8)"), and marked the thread resolved. The only remaining nit — the historically-worded "at the 8 minimum" regression-test title — was already called out as optional in the first review and was independently ruled out by verifiers this run as non-user-facing with still-correct assertions. The bug-hunting system found nothing else. Given the author addressed prior feedback, the change is a small revert to shipped behavior, and coverage is thorough, approving.

[cirospaciari]

lets keep only verify and not accept < 8 for safety

[claude]

🟡 nit: the adjacent test title "argon2 memoryCost at the 8 minimum is encoded faithfully" (line ~330) and its inline comment this pins the minimum at 8 as advertised are now stale — after this PR 8 is the padding threshold, not the minimum, and after 3ebd546 nothing advertises 8 as a minimum anymore. The assertions are still correct; only the wording could be updated (e.g. "at the m=8 padding boundary").

Extended reasoning...

What's stale

The previous review round flagged four "minimum 8" references. Commit 3ebd546 fixed the three user-facing ones (bun.d.ts, hashing.mdx, hash-a-password.mdx) and the thread was resolved with the author's reply "updated all three". The fourth item — the test immediately above the modified describe block — was not touched:

• Title (password.test.ts:330): "argon2 memoryCost at the 8 minimum is encoded faithfully (regression for #30960)"
• Inline comment (:336-338): Before the fix, values below 8 were rounded up while still reporting m=8; this pins the minimum at 8 as advertised.

After this PR the floor is 1, and after 3ebd546 neither the docs nor the .d.ts "advertise" 8 as anything — so both the "8 minimum" in the title and "as advertised" in the comment now describe a state of the world this PR is explicitly undoing.

Why re-raise it when the thread is resolved

The author's follow-up reply enumerated exactly three fixes ("updated all three"), matching the three user-facing references the comment led with. The test title/comment was the trailing "while there" item — the phrasing of the reply suggests it was overlooked rather than consciously declined, and with the thread now marked resolved it won't otherwise resurface. Mentioning the leftover once, as a non-blocking nit, is the lowest-friction way to close the loop.

Step-by-step

1. At PR HEAD, PasswordObject.rs:134 accepts memory_cost >= 1.0; the new describe at :343 round-trips m=1/4/7. So 8 is not the minimum.
2. At PR HEAD, rg 'inimum 8' returns zero user-facing hits (3ebd546 removed them). So nothing "advertises" 8 as a minimum.
3. password.test.ts:330 still reads "at the 8 minimum" and :338 still reads "pins the minimum at 8 as advertised" — both premises are now false.
4. The assertion itself — expect(hashed).toContain("m=8,t=1,p=1") — remains correct and useful: m=8 is still encoded verbatim, and it's still the boundary below which the vendored rust-argon2 pads internally. Only the description of why 8 is interesting is wrong.

Addressing the refutation

One verifier argued this is below the standalone reporting threshold: it's a non-user-facing regression-test title whose assertions remain valid, the prior review already surfaced it, and REVIEW.md's "Name things truthfully" targets code identifiers rather than test titles. Taking each point:

• Non-user-facing / assertions correct — agreed, which is exactly why this is filed as nit, not normal. It should not block merge.
• Already surfaced — it was, but as a secondary "while there" clause; the author's "updated all three" reply and the resolved thread mean the remainder is now invisible unless re-mentioned. Re-raising the unaddressed remainder of a resolved thread is different from duplicating a live open comment.
• No REVIEW.md rule — REVIEW.md's comment guidance ("Only comment what the code cannot say") and "When changing output/defaults/messages, grep the suite for assertions on the old behavior and update them in the same PR" both apply in spirit: a comment that says "as advertised" when the advertisement was just deleted in this PR is the kind of drift that guidance targets. That said, the refutation is right that this is not egregious — hence nit.

The prior review's own reasoning did concede "That's fair" to the below-threshold objection for the test item taken alone — but then bundled it anyway because the three user-facing items justified the comment. Now that those three are fixed and the thread is closed, the calculus is: one lightweight nit vs. leaving a comment that will read as factually wrong to the next person touching this file. The former seems like the smaller cost.

Suggested fix

Purely wording — e.g. retitle to "argon2 memoryCost of 8 is encoded faithfully (regression for #30960)" and reword the trailing comment clause to something like this pins the m=8 boundary where rust-argon2's internal padding kicks in, or simply drop the "as advertised" tail. No assertion changes needed.