Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/guides/util/hash-a-password.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const password = "super-secure-pa$$word";
// use argon2 (default)
const argonHash = await Bun.password.hash(password, {
algorithm: "argon2id",
memoryCost: 8, // memory usage in kibibytes (minimum 8)
memoryCost: 8, // memory usage in kibibytes
timeCost: 3, // the number of iterations
});
```
Expand Down
2 changes: 1 addition & 1 deletion docs/runtime/hashing.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const password = "super-secure-pa$$word";
// use argon2 (default)
const argonHash = await Bun.password.hash(password, {
algorithm: "argon2id", // "argon2id" | "argon2i" | "argon2d"
memoryCost: 8, // memory usage in kibibytes (minimum 8)
memoryCost: 8, // memory usage in kibibytes
timeCost: 3, // the number of iterations
});

Expand Down
2 changes: 1 addition & 1 deletion packages/bun-types/bun.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3402,7 +3402,7 @@ declare module "bun" {
algorithm: "argon2id" | "argon2d" | "argon2i";

/**
* Memory usage, in kibibytes. Minimum 8.
* Memory usage, in kibibytes. Values below 8 still use 8 KiB.
*/
memoryCost?: number;
/**
Expand Down
11 changes: 5 additions & 6 deletions src/runtime/crypto/PasswordObject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,18 +129,17 @@ impl AlgorithmValue {

let memory_cost = memory_value.as_number();

// argon2 requires `memoryCost >= 8 * parallelism`;
// Bun hard-codes `parallelism = 1` (see
// `Argon2Params::to_params`), so the floor is 8.
if memory_cost < 8.0 || memory_cost.is_nan() {
// Values below 8 are computed with 8 blocks (see the
// vendored rust-argon2 patch), matching pre-Rust Bun.
if memory_cost < 1.0 || memory_cost.is_nan() {
return Err(global_object.throw_invalid_arguments(format_args!(
"Memory cost must be at least 8"
"Memory cost must be greater than 0"
)));
}
Comment thread
claude[bot] marked this conversation as resolved.

if memory_cost.fract() != 0.0 || memory_cost > f64::from(u32::MAX) {
return Err(global_object.throw_invalid_arguments(format_args!(
"Memory cost must be an integer between 8 and 4294967295"
"Memory cost must be an integer between 1 and 4294967295"
)));
}

Expand Down
46 changes: 23 additions & 23 deletions test/js/bun/util/password.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,17 +112,12 @@
}),
).toThrow();

// argon2 requires `memoryCost >= 8 * parallelism`; Bun hard-codes
// `parallelism = 1`, so anything below 8 must throw rather than be
// silently clamped (regression coverage for #30960).
for (const invalid of [1, 3, 7]) {
expect(() =>
hash(placeholder, {
algorithm: "argon2id",
memoryCost: invalid,
}),
).toThrow("Memory cost must be at least 8");
}
expect(() =>
hash(placeholder, {
algorithm: "argon2id",
memoryCost: 0,
}),
).toThrow("Memory cost must be greater than 0");

expect(() =>
hash(placeholder, {
Expand Down Expand Up @@ -175,14 +170,14 @@
algorithm: "argon2id",
memoryCost: 2 ** 32 + 4608,
}),
).toThrow("Memory cost must be an integer between 8 and 4294967295");
).toThrow("Memory cost must be an integer between 1 and 4294967295");

expect(() =>
hash(placeholder, {
algorithm: "argon2id",
memoryCost: 8.5,
}),
).toThrow("Memory cost must be an integer between 8 and 4294967295");
).toThrow("Memory cost must be an integer between 1 and 4294967295");

// Non-finite values: NaN and -Infinity fail the lower-bound check,
// +Infinity fails the integer/upper-bound check.
Expand All @@ -196,11 +191,11 @@
);
for (const memoryCost of [NaN, -Infinity]) {
expect(() => hash(placeholder, { algorithm: "argon2id", memoryCost })).toThrow(
"Memory cost must be at least 8",
"Memory cost must be greater than 0",
);
}
expect(() => hash(placeholder, { algorithm: "argon2id", memoryCost: Infinity })).toThrow(
"Memory cost must be an integer between 8 and 4294967295",
"Memory cost must be an integer between 1 and 4294967295",
);
});

Expand Down Expand Up @@ -345,8 +340,8 @@
expect(await password.verify("test", hashed)).toBeTrue();
});

describe.concurrent("argon2 hashes with memoryCost below 8 from earlier Bun versions still verify", () => {
// Generated by Bun 1.3.14, which accepted memoryCost < 8.
describe.concurrent("argon2 memoryCost below 8", () => {
// Generated by Bun 1.3.14.

Check warning on line 344 in test/js/bun/util/password.test.ts

View check run for this annotation

Claude / Claude Code Review

Stale test title/comment still describes 8 as the memoryCost minimum

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 3ebd5464 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").
Comment on lines +343 to +344

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

const legacy = {
argon2id:
"$argon2id$v=19$m=4,t=1,p=1$jaFm03353WIBtbqnvp4hx6Pd0Pk2keYfomedORTs6bI$Q+62iWiDQhCP3VFQvnMnGptmDAHFQGqY3d/dmRcGVOw",
Expand All @@ -359,19 +354,24 @@
};

for (const [name, hash] of Object.entries(legacy)) {
test(name, async () => {
test(`verifies legacy ${name}`, async () => {
expect(await password.verify("hello", hash)).toBeTrue();
expect(await password.verify("hellp", hash)).toBeFalse();
expect(password.verifySync("hello", hash)).toBeTrue();
expect(password.verifySync("hellp", hash)).toBeFalse();
});
}

test("hashing with memoryCost below 8 is still rejected", () => {
expect(() => password.hashSync("hello", { algorithm: "argon2id", memoryCost: 4 })).toThrow(
"Memory cost must be at least 8",
);
});
for (const memoryCost of [1, 4, 7]) {
for (const algorithm of ["argon2id", "argon2i", "argon2d"] as const) {
test(`hashes with ${algorithm} m=${memoryCost} as written`, async () => {
const hashed = await password.hash("hello", { algorithm, memoryCost, timeCost: 1 });
expect(hashed).toStartWith(`$${algorithm}$v=19$m=${memoryCost},t=1,p=1$`);
expect(await password.verify("hello", hashed)).toBeTrue();
expect(await password.verify("hellp", hashed)).toBeFalse();
});
}
}
});

const defaultAlgorithm = "argon2id";
Expand Down
Loading