fix(ext/node): align scrypt behavior and performance with Node

[divybot]

Summary

Enables test-crypto-scrypt in node_compat suite.

Test plan

• cargo test --test node_compat -- test-crypto-scrypt

[fibibot]

Substance is right — switching the pure-Rust scrypt crate for AWS-LC's EVP_PBE_scrypt is the correct perf move (aws-lc-sys is already a workspace dep at Cargo.toml:182), and the new validateScryptParams lift correctly mirrors Node's parameter checks (RFC 7914 bounds: r*16 ≤ 32 ⇒ N < 2^(r*16), p*r ≤ 2^30 - 1, 128*N*r < maxmem). The new ERR_CRYPTO_INVALID_SCRYPT_PARAMS extends NodeRangeError which matches Node's class.

CI is red on a clippy lint, blocking everything

lint debug macos-x86_64 fails with clippy::undocumented-unsafe-blocks at ext/node_crypto/lib.rs:953:16 — the new unsafe { aws_lc_sys::EVP_PBE_scrypt(...) } block has no preceding // SAFETY: comment. Build cascades fail because clippy is treated as an error here.

A proper SAFETY comment for this block would call out the four preconditions the FFI requires:

// SAFETY:
// - `password.as_ptr()` and `password.len()` describe a valid contiguous slice of
//   bytes (StringOrBuffer invariant).
// - `salt.as_ptr()` and `salt.len()` likewise.
// - `output_buffer.as_mut_ptr()` points to at least `keylen` writable bytes
//   (caller invariant: the JS layer allocates `Buffer.alloc(keylen)` and passes
//   it through `#[anybuffer]`; the async path allocates `vec![0u8; keylen]`).
// - `1u64 << cost` is bounded because `cost = Math.log2(N)` with `N <= 2^53 - 1`
//   from JS validation, so `cost <= 53` and the shift cannot panic / overflow.
// - EVP_PBE_scrypt is thread-safe per AWS-LC docs; nothing here aliases.
let result = unsafe { aws_lc_sys::EVP_PBE_scrypt(...) };

More specifically — the bullet about output_buffer.len() >= keylen is a real invariant that's worth defending in code, not just in the comment:

assert!(output_buffer.len() >= keylen, "output_buffer too small for scrypt keylen");

The sync op gets output_buffer from a JS-allocated buffer and keylen separately as #[number] usize, so a mismatched call-site could violate this. The async op allocates internally so it's fine. The assert costs nothing and turns a potential out-of-bounds FFI write into a clean panic.

Other concerns I checked

• 1u64 << cost shift safety — JS validates N is a power of 2, then passes Math.log2(N) as cost: u64. Since JS Numbers cap at 2^53 - 1, cost ≤ 53 in practice and the shift is safe. But there's no Rust-side guard, so a non-JS caller (or a future op signature change) could break this. Defensive: if cost >= 64 { return Err(...) } or 1u64.checked_shl(cost as u32).ok_or(...)?.
• Type widening from u32 to u64/usize — keylen and the cost params are now #[number] which means JS Number → u64 / usize via the standard JsNumberFromI64 path. JS Number's safe-integer range is 2^53 - 1, so values in that range round-trip exactly; values above that would silently lose precision before reaching Rust. Worth a sanity validate at JS that keylen and maxmem are within Number.MAX_SAFE_INTEGER.
• keylen === 0 short-circuit added in both scryptSync and scrypt — matches Node's behavior of returning a zero-length Buffer immediately rather than calling EVP. Good defensive shape; also prevents an FFI call with keylen: 0 which OpenSSL/AWS-LC reject anyway.
• The // TODO(lev): key derivation failed, so what? comment is correctly removed — EVP_PBE_scrypt returns 1 on success and 0 on failure, so the if result == 1 { Ok(()) } else { Err(...) } branch is the canonical AWS-LC pattern.

Summary

Not blocking on substance. The clippy fix (one SAFETY comment) is the only thing CI is asking for; while you're at it, the optional assert!(output_buffer.len() >= keylen) would close a real soundness gap in the sync path, and 1u64.checked_shl(cost as u32) would make the shift defensive at the Rust boundary. CI 129 / 137 success, 2 cancelled (clippy cascade), 2 fail (lint + ci status), 2 skip, 1 cla pending.

[fibibot]

LGTM at 2bed4bc. The follow-up commit "document and guard scrypt ffi invariants" addresses every point from my prior COMMENT exactly:

• // SAFETY: block at lib.rs:961–971: covers all five invariants (password / salt slices, keylen writability, n overflow safety, AWS-LC thread-safety) — closely matches the template I'd suggested.
• assert!(output_buffer.len() >= keylen, ...) added at the top of scrypt(...) (lib.rs:953–956) — turns the potentially out-of-bounds FFI write into a clean panic at the Rust boundary.
• u32::try_from(cost).ok().filter(|cost| *cost < 64) + 1u64.checked_shl(cost) (lib.rs:957–960, 967): belt-and-suspenders defensive shift. Even if a future caller passes cost ≥ 64 directly (bypassing the JS validation that bounds cost via Math.log2(N) ≤ 53), the Rust side rejects it cleanly with "Invalid scrypt param" rather than panicking on the shift.

All relevant test node_compat debug shards × 4 platforms green at this head plus test unit_node debug × 4 platforms green. Only red is test node_compat (3/3) debug windows-x86_64 failing on parallel/test-dns-resolver-max-timeout.js: timeout1: 504, timeout2: 515 — that's the recurring Windows DNS-resolver-timing flake that's hit ~half the PRs I've reviewed today (the assertion is timeout1 > timeout2 with millisecond-granular timestamps and the OS scheduler doesn't always deliver). Worth re-triggering once the in-progress macos-x86_64 / linux-x86_64 release shards land.

Leftover from the original review: the unrelated ECDSA from_bytes → from_slice migration in sign.rs is still bundled here. Wait — that's PR #33774, not this one. Disregard. This PR is cleanly scoped to scrypt, substance + safety are in, ready to ship from my side once CI fully lands.

[divybot]

@littledivy heads-up: the bot's analysis of this PR's CI failures says they look unrelated/flaky and not addressable from this PR's diff. Verdict:

Please verify and either rerun the failing checks (admin needed), waive them, or merge if the green checks are sufficient. Pinged once; the bot won't re-engage on the same signals.

[littledivy]

LGTM