fix(node:crypto): align scrypt error code with Node.js#6745
Open
JosephDoUrden wants to merge 2 commits into
Open
fix(node:crypto): align scrypt error code with Node.js#6745JosephDoUrden wants to merge 2 commits into
JosephDoUrden wants to merge 2 commits into
Conversation
The native scrypt path threw a generic Error('Scrypt failed') so
node:crypto.scrypt(...) and scryptSync(...) returned an error without the
code: 'ERR_CRYPTO_INVALID_SCRYPT_PARAMS' Node.js sets for invalid params.
Wrap the native call so the failure surfaces as a RangeError with that
code and the 'Invalid scrypt params' message.
BoringSSL's EVP_PBE_scrypt also does not enforce
128 * r * (N + p + 2) <= maxmem the way OpenSSL does, so cases like
{N:32768, r:8, p:1, maxmem:0} (coalesced to the 32 MiB default) succeeded
in workerd while throwing on Node. Add the missing memory check on the
JS side, run alongside the native call so the error path matches Node's
async (callback) and sync (throw) shapes. Uses BigInt because the
operands can each be up to 2**32 - 1, and their product would overflow
a JS Number's safe integer range.
Tests now assert err.code on bad and toobig vectors, and a regression
case mirroring the original issue is added to toobig.
Fixes cloudflare#6639
jasnell
reviewed
May 9, 2026
jasnell
reviewed
May 9, 2026
Address review feedback: the previous catch threw a fresh
ERR_CRYPTO_INVALID_SCRYPT_PARAMS for anything, which discarded the
already-coded error from checkScryptMemory and would have masked
unrelated failures from the native call.
normaliseScryptError now only rewrites the exact generic Error
('Scrypt failed', no .code) that the workerd C++ scrypt path emits.
Anything that already carries a .code (such as the
ERR_CRYPTO_INVALID_SCRYPT_PARAMS thrown by the JS-side memory check)
or anything with a different message passes through unchanged.
Contributor
Author
|
The native getScrypt throws a plain Error('Scrypt failed') with no .code, so a plain rej(err) would leave the bad/toobig cases without the code the issue is asking for. normaliseScryptError only rewraps that exact shape, anything already coded (like checkScryptMemory's throw) or with a different message goes through unchanged. @jasnell |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6639. The native scrypt path threw a generic
Error('Scrypt failed'), sonode:crypto.scrypt(...)andscryptSync(...)came back without thecode: 'ERR_CRYPTO_INVALID_SCRYPT_PARAMS'Node.js sets for invalid params. Wrapped the native call so the failure surfaces as aRangeErrorwith that code and theInvalid scrypt paramsmessage.Second part of the issue: BoringSSL's
EVP_PBE_scryptdoes not enforce128 * r * (N + p + 2) <= maxmemthe way OpenSSL does, so cases like{N:32768, r:8, p:1, maxmem:0}(coalesced to the 32 MiB default) succeeded in workerd while throwing on Node. Added the missing memory check on the JS side, run alongside the native call so the error path matches Node's async (callback) and sync (throw) shapes. Uses BigInt because the operands can each be up to 2**32 - 1, and their product would overflow a JS Number's safe integer range.Tests:
crypto_scrypt-testnow assertserr.codeon the existingbadandtoobigvectors, plus a regression case{N:32768, p:1, r:8, maxmem:0}straight from the issue. Verified locally withbazel test //src/workerd/api/node/tests/...(355 pass) andbazel test //src/node:node@eslint.