Repository navigation
feat!: redesign bcrypt API while preserving stored hash verification - #1207
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ebacd2e69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Brooooooklyn
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06c3b9576b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abcf89c7aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4ebb5c5 to
7ebc07f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ebc07f50c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
3 similar comments
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
- Settle the public Promise before removing the abort listener, and remove it with the same options, so throwing or capture-flag EventTargets cannot hang a call or leak the listener. - Keep signal.reason as a non-enumerable AbortError cause, so callers can tell a TimeoutError or custom reason apart. - Throw TypeError for option values of the wrong type and keep RangeError for out-of-range or malformed values. - Copy only the 72 password bytes bcrypt reads for async work, and wipe those copies on drop; replace the unreachable!() salt branch. - Declare package exports so internal files cannot be deep-imported. - Stop declaring the WASI package as an optional dependency, so installs do not pull it and a missing native binary fails loudly. - Leave the version bump to the release step, add the 2.0.0 changelog, and preapprove this repo's own @node-rs/* packages. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
….timeout AbortSignal.timeout() uses a timer that does not keep the event loop alive, and the controlled backend has no native work that would. The test passed only when other tests kept the worker busy, and failed with "Promise returned by test never resolved" on the Linux and WASI jobs. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
With the bcrypt workspace back at 1.10.9, yarn's transparent workspaces link bcrypt-previous (npm:@node-rs/bcrypt@1.10.9) to the workspace itself whenever the lockfile is regenerated, so the previous-release tests would compare the new code with itself. No workspace depends on another, so only the workspace: protocol needs to link workspaces. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
main moved @napi-rs/cli from 3.9.0 to 3.10.5. Regenerate the bcrypt loaders and type declarations from native and wasm32-wasip1-threads builds so the committed files match what the release build produces. binding.js still parses as ES2019 for Node 10 and 12. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ed calls A signal whose removeEventListener throws made a successful call raise an unhandled rejection, which exits modern Node, and on abort it skipped the native cancellation. A signal whose reason getter throws stopped the abort listener before it settled, so the call stayed pending or resolved after the abort. Ignore cleanup failures once the call has settled, and read the optional reason defensively. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Bundle two more contract changes into the 2.0 break so the error and
rehash-on-login story is complete at release:
- Thrown errors carry Node-style codes: TypeError ERR_INVALID_ARG_TYPE,
RangeError ERR_OUT_OF_RANGE (including native InvalidArg translations),
and AbortError ABORT_ERR, matching Node's own AbortError.
- parseOptions(hash) returns { version, cost } through the verifier's
parser (bcrypt::HashParts), so every hash verify accepts is parseable,
including +4 costs and imported 2x labels; hashes verify always rejects
throw RangeError. Mirrors argon2's parseOptions in this repo.
- AbortSignalLike.removeEventListener declares the options argument the
adapter actually passes.
- Drop unused blowfish and quickcheck dependencies from the bcrypt crate
and the workspace.
Native and wasm32-wasip1-threads bindings are regenerated. api.cjs still
parses as ES2019 for Node 10 and 12.
Generated with [Devin](https://devin.ai)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- Use one options type for add/removeEventListener on AbortSignalLike, matching the comment that the same object is passed to both. - Give parseOptions its own arity message; it never had positional options to drop. - Cover the 60-byte non-ASCII parser branch in the test, which the previous 61-byte input missed. - Mention in the README that parseOptions throws for hashes verify always rejects. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
A stale platform package now fails at import time with code ERR_BCRYPT_INCOMPATIBLE_BINARY, so every error the package throws sits inside the documented ErrorCode contract. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Replace the bcrypt 0.19 dependency with bcrypt-rust 1.0, keeping the crate name so imports stay valid. The new crate is a drop-in for every API used here except one deliberate difference: its HashParts parser is strict and rejects the noncanonical '+4' cost spelling the old backend accepted. Verify and parseOptions now use a lenient parser that mirrors the old backend byte-for-byte (60 ASCII bytes, 2a/2b/2x/2y markers, u32::parse for cost), then hand the canonicalized hash to bcrypt::verify so the cost gate and constant-time compare stay in the library. ~20% faster than the C++ backend on hash and verify.
bcrypt: uniform call signatures via per-impl adapters, a fixed salt so the hash suite measures hashing only, a real sync compare for verify (it previously timed an unawaited async call with a hash inside the loop), one cost across suites, all five implementations in every suite, and a compact sorted table with latency, rme, ops/s, and a ratio to the baseline. argon2: replace the hand-rolled performance.now() harness with tinybench fixed-iteration runs, keep the per-iteration tag assertion and the native-sync / native-async / js-wasm grouping, and adopt the same report format.
TaskResult is a discriminated union; latency/throughput exist only on completed and aborted-with-statistics. CI type-checks benchmark files through packages/argon2/tsconfig.json, unlike oxnode which strips types.
…in bench comment
genSaltSync previously ran every call through options() + creation(),
allocating a fresh Object.create(null) copy even for {} and {cost}. Now
undefined uses a frozen empty options object and reusable options shapes
validate in place — identical error paths preserved for non-plain or
extra-key objects. Saves ~200ns of JS wrapper overhead per call.
Picks up the crypt_blowfish fused-round kernel shape (Brooooooklyn/bcrypt-rust@f65cd90): bcrypt() at cost 10 on x86_64 drops ~19% (138.2ms -> 112.6ms vs vendored C at 108.7ms); arm64 unchanged.
e98cb03 to
142af8f
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
genSaltSync now serves 16-byte salts from a 256-byte thread-local pool
refilled by SysRng (direct getrandom), amortizing one syscall over 16
salts without any userspace PRNG in between. The salt string is built
without format! machinery and the bcrypt base64 engine is constructed
once in a LazyLock instead of per call.
api.cjs gains a specialized fast path for the {cost,version} shapes:
getOwnPropertyNames/getOwnPropertySymbols cover the same key space as
Reflect.ownKeys at half the cost, the sync() closure is inlined, and
anything the fast path cannot prove valid funnels into a shared full-
validation path so error types, codes, and messages stay identical.
x86_64 sandbox (30k-iteration amortized loop, best of reps):
@node-rs/bcrypt ~380-560ns vs wasm OpenBSD ~550-730ns
per-call min: 210-250ns vs 320-370ns; bench.js min 290ns vs 860ns.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Picks up the xor_late asm boundary (Brooooooklyn/bcrypt-rust#8): LLVM no longer reassociates the fused round into two serial xors after the S-box loads. On x86_64 EPYC the raw kernel now beats crypt_blowfish C (~54.6ms vs ~54.9ms at cost 10) and JS hashSync/verifySync beat node bcrypt (C++); arm64/wasm32 untouched via cfg fallback. CodSpeed: -4% on all single-hash benches.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Bcrypt's positional salt API clips or pads text instead of parsing encoded salts, and its salt generator emits padding. This prepares bcrypt 2.0.0 with explicit creation and verification contracts while preserving existing stored-hash verification behavior.
API changes
2a,2b, and2yfor creation.rejectLongPasswordscreation policy that accepts exactly 72 bytes.TypeErrorwithcode: 'ERR_INVALID_ARG_TYPE'; values of the right type that are out of range or malformed throwRangeErrorwithcode: 'ERR_OUT_OF_RANGE'(nativeInvalidArgerrors are translated the same way).parseOptions(hash)returning{ version, cost }through the verifier's parser, mirroring argon2'sparseOptionsin this repo. Every hashverifycan accept is parseable, including the+4cost spelling and imported2xlabels; hashesverifyalways rejects throwRangeError. The strict creation parser is not involved. This gives rehash-on-login flows a supported way to read the stored cost.AbortSignalLike, without requiring global cancellation constructors. Pending public calls reject on abort even if native work is already running, with anAbortError(code: 'ABORT_ERR', like Node's) whosecauseissignal.reasonwhen defined.code: 'ERR_BCRYPT_INCOMPATIBLE_BINARY'. Generated bindings are refreshed with@napi-rs/cli3.10.5 from native andwasm32-wasip1-threadsbuilds. The WASI package is not an automatic dependency; browser builds install@node-rs/bcrypt-wasm32-wasiexplicitly.exports, so only the package root andpackage.jsoncan be imported.blowfishandquickcheckdependencies from the bcrypt crate and the workspace.Stored credentials
No database rewrite, prefix replacement, compatibility flag, or password reset is required. The hashing backend is now
bcrypt-rust(v1), whose strictHashPartsparser rejects some spellings the oldbcryptcrate accepted; the verifier therefore parses stored hashes with a lenient parser that mirrors the old backend byte-for-byte — same acceptance set including+4costs and2xlabels — then canonicalizes and verifies throughbcrypt::verify. Raw password bytes, long-password suffix equivalence, and thefalse-on-invalid contract are preserved. The stricter salt parser applies only to creation. Invalid UTF-8 hash bytes now returnfalseunder the documented error contract.Applications that authenticate by recomputing a hash with a separately saved original salt should migrate to
verify(password, storedHash). The migration guide covers this and the new call shapes.Release
binding.js(yarn buildinpackages/bcrypt), becausenapi versiondoes not update its expected platform-package version.CHANGELOG.mdhas a2.0.0 (Unreleased)entry; set the date at release..yarnrc.ymlpreapproves this repo's own@node-rs/*packages and disables transparent workspaces, sobcrypt-previous(npm:@node-rs/bcrypt@1.10.9) always comes from npm instead of linking to the workspace.Performance
Measured end-to-end on real x86_64 hardware (Cloudflare Sandbox, 4-vCPU EPYC) with a plain-process bench mirroring
benchmark/bcrypt.ts; on this PR's final build we are the fastest implementation in all three suites.bcrypt-rust's scalar kernel was rewritten in crypt_blowfish's fused-round shape and then given anxor_lateasm boundary so LLVM stops reassociating the fused round into two serial xors after the S-box loads. Raw kernel: 54.6ms vs C crypt_blowfish 54.9ms; JS level: hashSync 54.8-55.1ms vs node bcrypt (C++) 55.8-56.6ms, verifySync 54.8ms (next best: wasm Openwall 60.5ms). CodSpeed on bcrypt-rust#8 reports -4% on all single-hash benches. wasm32/arm64 keep the scalar fallback. Ships viaCargo.lockpinningbcrypt-rust1.0.2.getrandomper 16 salts, raw SysRng — no userspace PRNG), format-free salt string, LazyLock base64 engine, and a tightened api.cjs fast path. ~630-650ns vs wasm OpenBSD ~810-840ns, node bcrypt ~2.7-2.8µs.packages/bcrypt/benchmark/bcrypt.tswas rewritten for 1:1 comparisons (uniform adapters, fixed salt, cost 10 across all suites, sorted tinybench report) andargon2's harness was migrated to tinybench to match.Validation
supported-node.cjsandpolyfill-cancellation.cjspass there too (now also coveringparseOptionsand the error codes); the Node 10/12 CI jobs run them on the advertised runtimes.api.cjsstill parses as ES2019.parseOptionsand compared against the text of its own prefix and cost.--locked, warnings denied) pass.Cross-platform results will come from this PR's CI. No packages have been published.