node:crypto: implement argon2 and argon2Sync - #37015
Conversation
crypto.argon2/argon2Sync were stubs that always threw ERR_CRYPTO_ARGON2_NOT_SUPPORTED, the error Node reserves for builds whose OpenSSL lacks argon2 (OpenSSL < 3.2). Every official Node 24.7+ binary ships with argon2 working, so feature detection against Bun passed while the calls failed. BoringSSL has no argon2, but the pure-Rust rust-argon2 crate is already in-tree for Bun.password. Route node:crypto argon2 to it: - src/js/node/crypto.ts validates like check() in Node's lib/internal/crypto/argon2.js (same validators, names, and messages), then calls the native binding. - node_crypto_binding.rs adds sync and work-pool async argon2 jobs following the existing scrypt/pbkdf2 patterns. Inputs are copied out of JS at call time (Node copies async-job inputs the same way), the result Buffer is created on the JS thread. Outputs and error messages verified against Node v26.3.0 for the RFC 9106 / OpenSSL vectors from test-crypto-argon2.js. test-crypto-argon2-unsupported.js is removed: it asserts the unsupported error on builds reporting OpenSSL < 3.2, and Bun reports BoringSSL as 1.1.0 while argon2 now works regardless of OpenSSL. test-crypto-argon2.js still skips for the same version-gating reason, so the vectors live in test/js/node/crypto/argon2.test.ts.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesArgon2 support now includes Rust-backed synchronous and asynchronous APIs. The JavaScript wrappers validate arguments and normalize buffer-like inputs. The native binding performs derivation, allocation checks, scheduling, and error handling. Tests cover vectors, validation, buffers, concurrency, and resource limits. Argon2 binding
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
The argon2 implementation removed the last two throw sites and the test asserting the code, leaving the ErrorCode.ts entry dead.
|
Updated 11:35 PM PT - Aug 5th, 2026
✅ @autofix-ci[bot], your commit 4ebaafb58abcbad0f542a7e4206425a4708eefdf passed in 🧪 To try this PR locally: bunx bun-pr 37015That installs a local version of the PR into your bun-37015 --bun |
…k detached/SharedArrayBuffer behavior in tests Validation admits sizes rust-argon2 allocates infallibly (a 4 TiB memory request aborted the process, from the work-pool thread on the async path). Pre-fail the job against the synthetic allocation limit so both paths deliver the catchable 'Argon2 derivation failed' node produces, mirroring the scrypt guard in this file. Also pin down detached-buffer and SharedArrayBuffer input handling, verified against node, so the contract is enforced rather than only described.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/node/node_crypto_binding.rs`:
- Around line 1568-1571: Update the sync Argon2 failure branch guarded by
ctx.failed to attach ErrorCode::CRYPTO_OPERATION_FAILED to the created error,
matching the coded error behavior in scrypt_sync while preserving the existing
throw path.
- Around line 1536-1541: Update the failed branch in Argon2’s async execution
path to create the error with ErrorCode::CRYPTO_OPERATION_FAILED, matching
Scrypt::run_from_js and preserving the callback’s existing failure behavior.
Ensure the resulting error exposes the expected code property before passing it
to run_callback.
- Around line 1516-1521: Replace Argon2’s untyped failed flag in
src/runtime/node/node_crypto_binding.rs:1516-1521 with a typed failure cause,
preserving the rust_argon2::Error and representing allocation-limit pre-failure
distinctly. Update the async error at
src/runtime/node/node_crypto_binding.rs:1536-1541 and sync error at
src/runtime/node/node_crypto_binding.rs:1568-1571 to use
global.err(ErrorCode::CRYPTO_OPERATION_FAILED, ...) with the stored cause and an
actionable message identifying the failure and remedy; update the exact-message
assertions in test/js/node/crypto/argon2.test.ts:419-424 accordingly.
🪄 Autofix
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: e7573cfb-b43a-49cc-bc68-8368d6e88f5a
📒 Files selected for processing (5)
src/js/node/crypto.tssrc/jsc/bindings/ErrorCode.tssrc/runtime/node/node_crypto_binding.rstest/js/node/crypto/argon2.test.tstest/js/node/test/parallel/test-crypto-argon2-unsupported.js
💤 Files with no reviewable changes (2)
- test/js/node/test/parallel/test-crypto-argon2-unsupported.js
- src/jsc/bindings/ErrorCode.ts
There was a problem hiding this comment.
Thanks for addressing the ErrorCode cleanup. I didn't find any bugs in this revision, but this is a new node:crypto KDF implementation with native async work-pool jobs and Vec→JSC ownership transfer, so it warrants a human look.
Reviewed: checkArgon2 validation vs Node's lib/internal/crypto/argon2.js (order, bounds, error codes match); Argon2 ctx copies all inputs before scheduling so the worker never touches JS memory; output.leak() → JSValue::create_buffer matches the existing MarkedArrayBuffer_deallocator ownership pattern; allocation-limit pre-fail guards memory/tagLength on both sync and async paths; test vectors are the RFC 9106 / OpenSSL set from upstream test-crypto-argon2.js.
Extended reasoning...
Overview
Replaces the ERR_CRYPTO_ARGON2_NOT_SUPPORTED stubs for crypto.argon2/argon2Sync with a real implementation backed by the in-tree rust-argon2 crate. Adds ~90 lines of JS validation in src/js/node/crypto.ts mirroring Node's check(), ~200 lines in src/runtime/node/node_crypto_binding.rs (new Argon2 struct, CryptoJobCtx impl, sync/async host fns), a 427-line test file with RFC 9106 vectors and validation/detached-buffer/SAB/allocation-limit coverage, drops the now-unused ERR_CRYPTO_ARGON2_NOT_SUPPORTED registration, and removes the vendored test-crypto-argon2-unsupported.js.
Security risks
This is a password-hashing KDF in node:crypto. The actual cryptography is delegated to rust-argon2 (already used by Bun.password), so no new primitive is hand-rolled. Input validation bounds all numeric parameters before they reach native code, and buffer inputs are copied out of JS synchronously so the work-pool thread never touches JS-heap memory. The allocation-limit guard converts oversized memory/tagLength into a catchable error rather than an allocator abort. I don't see an injection or memory-safety hole, but the memory-ownership transfer (Vec::leak() → create_buffer → MarkedArrayBuffer_deallocator) and the async job lifecycle deserve maintainer eyes.
Level of scrutiny
High. This is a new user-facing crypto API surface with native async-job plumbing, an ownership-transfer pattern, and two documented deliberate divergences from Node (proper ERR_INVALID_ARG_TYPE for wrong-typed secret/associatedData where Node crashes with ERR_INTERNAL_ASSERTION; sequential lane computation vs Node's threaded lanes). Those are reasonable calls but should be signed off by a maintainer, not a bot.
Other factors
My prior nit (dead ERR_CRYPTO_ARGON2_NOT_SUPPORTED entry) was addressed in ad4bffc. The comment-cop and CodeRabbit threads are all resolved — the CodeRabbit ErrorCode::CRYPTO_OPERATION_FAILED suggestion was correctly rejected with Node-verified evidence that Node's argon2 failure carries no .code. Test coverage is thorough (RFC vectors, validation-order, offset views, detached/SAB, copy-at-call-time, concurrent async, subprocess allocation-limit). Given the scope and the crypto/threading surface, deferring to human review.
|
Related: #39021 adds |
Problem
crypto.argon2andcrypto.argon2Syncexist with Node-matching arity but are stubs that always throwERR_CRYPTO_ARGON2_NOT_SUPPORTED:Node only throws that error when its OpenSSL lacks argon2 (OpenSSL < 3.2), and every official Node 24.7+ binary bundles a new enough OpenSSL, so the API works on stock Node. Since Bun reports
process.versions.nodeas 26.3.0 andtypeof crypto.argon2 === "function"passes, feature detection succeeds and the call then fails.Fix
BoringSSL has no argon2, but the pure-Rust
rust-argon2crate is already in-tree forBun.password. This wiresnode:cryptoto it:src/js/node/crypto.ts: validation mirroringcheck()in Node'slib/internal/crypto/argon2.js, with the same validators, property names, and error messages (ERR_OUT_OF_RANGE,ERR_INVALID_ARG_TYPE,ERR_INVALID_ARG_VALUE).src/jsc/bindings/ErrorCode.ts: drops theERR_CRYPTO_ARGON2_NOT_SUPPORTEDregistration, which this change leaves with no callers.src/runtime/node/node_crypto_binding.rs: sync and async (work pool) jobs following the existing scrypt/pbkdf2 patterns. Inputs are copied out of JS at call time, matching Node's async-jobToCopysemantics and keeping the worker thread away from JS memory entirely; the resultBufferis created on the JS thread with ownership transfer (same as pbkdf2). Hashing runs single-threaded on the work-pool thread (lanesdetermines the output, not the thread count), likeBun.password.Verification
test-crypto-argon2.jspass, and every expected output and error message in the new test was generated with Node v26.3.0 (OpenSSL 3.5.6) side by side, including string/ArrayBuffer/offset-view inputs, omitted-vs-emptysecret/associatedData, detached buffers, and validation order.test/js/node/crypto/argon2.test.ts(36 tests). It fails on current main (the stub throws) and passes with this change. Detached-buffer, SharedArrayBuffer, and allocation-limit behavior are asserted by tests, not just verified manually.memory: 65536vectors are downsized tomemory: 4096(outputs regenerated with Node) so the file stays within debug/ASAN time budgets; the original 64 MiB,tagLength: 128vector was verified against the debug build manually.Vendored Node tests
test-crypto-argon2.jscontinues to skip: it gates onhasOpenSSL(3, 2)and Bun reports BoringSSL as 1.1.0. The bun-side test above carries its vectors.test-crypto-argon2-unsupported.jsis removed. It asserts the unsupported error precisely when OpenSSL < 3.2, which no longer describes Bun: argon2 works here regardless of the OpenSSL version, so the test's premise cannot be satisfied without keeping the feature broken.Notes
secret/associatedDatathrows a properERR_INVALID_ARG_TYPEnaming the property. Node passes no name togetArrayBufferOrViewthere and crashes withERR_INTERNAL_ASSERTION("'name' must be a string"), which its own message labels a Node bug.memory/tagLengthvalues that pass validation but exceed the allocation limit (default 4 GiB, the same bound scrypt's output uses) fail the job with node's catchable "Argon2 derivation failed" on both the sync and async paths, instead of aborting insiderust-argon2's infallible allocations. Covered by a subprocess test under a loweredBUN_FEATURE_FLAG_SYNTHETIC_MEMORY_LIMIT. Sizes under the limit that still exceed available RAM remain subject to the allocator, as withBun.password(same crate).ThreadMode::Sequential, asBun.passworddoes). Lanes determine the output, not the thread count, so results are identical to node; node additionally fans lanes out across OpenSSL threads, so multi-lane derivations complete faster there. Parallel lane computation is a possible follow-up with its own thread-count policy.