Skip to content

Commit

Permalink
crypto: remove ERR_CRYPTO_SCRYPT_INVALID_PARAMETER
Browse files Browse the repository at this point in the history
It is confusing to have both ERR_CRYPTO_SCRYPT_INVALID_PARAMETER and
ERR_CRYPTO_INVALID_SCRYPT_PARAMS. The former was the original error
code, added in 371103d, but parameter
validation gradually changed and now produces
ERR_CRYPTO_INVALID_SCRYPT_PARAMS for all parameter validation errors
coming from OpenSSL, as well as different error codes for validation
errors coming from JavaScript. The only remaining use of
ERR_CRYPTO_SCRYPT_INVALID_PARAMETER is in the validation logic that
ensures that no two synonymous options were passed. We already have an
error code for that particular case, ERR_INCOMPATIBLE_OPTION_PAIR, so
replace these last instances of ERR_CRYPTO_SCRYPT_INVALID_PARAMETER with
that error code and remove ERR_CRYPTO_SCRYPT_INVALID_PARAMETER. If there
ever is need again for such an error code, we can just use
ERR_CRYPTO_INVALID_SCRYPT_PARAMS.

Refs: #35093
Refs: #21525
Refs: #20816
PR-URL: #53305
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
  • Loading branch information
tniessen authored Jul 10, 2024
1 parent 059f2f4 commit 33a6d1f
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 16 deletions.
23 changes: 15 additions & 8 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1010,7 +1010,8 @@ An invalid message length was provided.
added: v15.0.0
-->

Invalid scrypt algorithm parameters were provided.
One or more [`crypto.scrypt()`][] or [`crypto.scryptSync()`][] parameters are
outside their legal range.

<a id="ERR_CRYPTO_INVALID_STATE"></a>

Expand Down Expand Up @@ -1070,13 +1071,6 @@ A crypto operation failed for an otherwise unspecified reason.
The PBKDF2 algorithm failed for unspecified reasons. OpenSSL does not provide
more details and therefore neither does Node.js.

<a id="ERR_CRYPTO_SCRYPT_INVALID_PARAMETER"></a>

### `ERR_CRYPTO_SCRYPT_INVALID_PARAMETER`

One or more [`crypto.scrypt()`][] or [`crypto.scryptSync()`][] parameters are
outside their legal range.

<a id="ERR_CRYPTO_SCRYPT_NOT_SUPPORTED"></a>

### `ERR_CRYPTO_SCRYPT_NOT_SUPPORTED`
Expand Down Expand Up @@ -3271,6 +3265,18 @@ The UTF-16 encoding was used with [`hash.digest()`][]. While the
causing the method to return a string rather than a `Buffer`, the UTF-16
encoding (e.g. `ucs` or `utf16le`) is not supported.

<a id="ERR_CRYPTO_SCRYPT_INVALID_PARAMETER"></a>

### `ERR_CRYPTO_SCRYPT_INVALID_PARAMETER`

<!-- YAML
removed: REPLACEME
-->

An incompatible combination of options was passed to [`crypto.scrypt()`][] or
[`crypto.scryptSync()`][]. New versions of Node.js use the error code
[`ERR_INCOMPATIBLE_OPTION_PAIR`][] instead, which is consistent with other APIs.

<a id="ERR_FS_INVALID_SYMLINK_TYPE"></a>

### `ERR_FS_INVALID_SYMLINK_TYPE`
Expand Down Expand Up @@ -4046,6 +4052,7 @@ An error occurred trying to allocate memory. This should never happen.
[`--no-addons`]: cli.md#--no-addons
[`--unhandled-rejections`]: cli.md#--unhandled-rejectionsmode
[`Class: assert.AssertionError`]: assert.md#class-assertassertionerror
[`ERR_INCOMPATIBLE_OPTION_PAIR`]: #err_incompatible_option_pair
[`ERR_INVALID_ARG_TYPE`]: #err_invalid_arg_type
[`ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST`]: #err_missing_message_port_in_transfer_list
[`ERR_MISSING_TRANSFERABLE_IN_TRANSFER_LIST`]: #err_missing_transferable_in_transfer_list
Expand Down
8 changes: 4 additions & 4 deletions lib/internal/crypto/scrypt.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ const {

const {
codes: {
ERR_CRYPTO_SCRYPT_INVALID_PARAMETER,
ERR_CRYPTO_SCRYPT_NOT_SUPPORTED,
ERR_INCOMPATIBLE_OPTION_PAIR,
},
} = require('internal/errors');

Expand Down Expand Up @@ -92,7 +92,7 @@ function check(password, salt, keylen, options) {
validateUint32(N, 'N');
}
if (options.cost !== undefined) {
if (has_N) throw new ERR_CRYPTO_SCRYPT_INVALID_PARAMETER();
if (has_N) throw new ERR_INCOMPATIBLE_OPTION_PAIR('N', 'cost');
N = options.cost;
validateUint32(N, 'cost');
}
Expand All @@ -102,7 +102,7 @@ function check(password, salt, keylen, options) {
validateUint32(r, 'r');
}
if (options.blockSize !== undefined) {
if (has_r) throw new ERR_CRYPTO_SCRYPT_INVALID_PARAMETER();
if (has_r) throw new ERR_INCOMPATIBLE_OPTION_PAIR('r', 'blockSize');
r = options.blockSize;
validateUint32(r, 'blockSize');
}
Expand All @@ -112,7 +112,7 @@ function check(password, salt, keylen, options) {
validateUint32(p, 'p');
}
if (options.parallelization !== undefined) {
if (has_p) throw new ERR_CRYPTO_SCRYPT_INVALID_PARAMETER();
if (has_p) throw new ERR_INCOMPATIBLE_OPTION_PAIR('p', 'parallelization');
p = options.parallelization;
validateUint32(p, 'parallelization');
}
Expand Down
1 change: 0 additions & 1 deletion lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1168,7 +1168,6 @@ E('ERR_CRYPTO_INVALID_KEY_OBJECT_TYPE',
'Invalid key object type %s, expected %s.', TypeError);
E('ERR_CRYPTO_INVALID_STATE', 'Invalid state for operation %s', Error);
E('ERR_CRYPTO_PBKDF2_ERROR', 'PBKDF2 error', Error);
E('ERR_CRYPTO_SCRYPT_INVALID_PARAMETER', 'Invalid scrypt parameter', Error);
E('ERR_CRYPTO_SCRYPT_NOT_SUPPORTED', 'Scrypt algorithm not supported', Error);
// Switch to TypeError. The current implementation does not seem right.
E('ERR_CRYPTO_SIGN_KEY_REQUIRED', 'No key provided to sign', Error);
Expand Down
22 changes: 19 additions & 3 deletions test/parallel/test-crypto-scrypt.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,16 @@ const good = [
},
];

// Test vectors that should fail.
// Test vectors that contain invalid parameters.
const bad = [
{ N: 1, p: 1, r: 1 }, // N < 2
{ N: 3, p: 1, r: 1 }, // Not power of 2.
{ N: 1, cost: 1 }, // Both N and cost
{ p: 1, parallelization: 1 }, // Both p and parallelization
];

// Test vectors that contain incompatible options.
const incompatibleOptions = [
{ N: 1, cost: 1 }, // Both N and cost
{ p: 1, parallelization: 1 }, // Both p and parallelization
{ r: 1, blockSize: 1 }, // Both r and blocksize
];

Expand Down Expand Up @@ -176,6 +180,18 @@ for (const options of bad) {
expected);
}

for (const options of incompatibleOptions) {
const [short, long] = Object.keys(options).sort((a, b) => a.length - b.length);
const expected = {
message: `Option "${short}" cannot be used in combination with option "${long}"`,
code: 'ERR_INCOMPATIBLE_OPTION_PAIR',
};
assert.throws(() => crypto.scrypt('pass', 'salt', 1, options, () => {}),
expected);
assert.throws(() => crypto.scryptSync('pass', 'salt', 1, options),
expected);
}

for (const options of toobig) {
const expected = {
message: /Invalid scrypt params:.*memory limit exceeded/,
Expand Down

0 comments on commit 33a6d1f

Please sign in to comment.