Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

errors,crypto: use internal/errors.js #14724

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 23 additions & 23 deletions lib/crypto.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ const stream = require('stream');
const util = require('util');
const { isUint8Array } = process.binding('util');
const LazyTransform = require('internal/streams/lazy_transform');
const errors = require('internal/errors');

const DH_GENERATOR = 2;

Expand Down Expand Up @@ -302,7 +303,7 @@ Sign.prototype.update = Hash.prototype.update;

Sign.prototype.sign = function sign(options, encoding) {
if (!options)
throw new Error('No key provided to sign');
throw new errors.Error('ERR_CTYPTO_MISSING_KEY');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling: CTYPTOCRYPTO. Also, I am not sure whether we want to introduce a new error type just for this.


var key = options.key || options;
var passphrase = options.passphrase || null;
Expand All @@ -313,7 +314,7 @@ Sign.prototype.sign = function sign(options, encoding) {
if (options.padding === options.padding >> 0) {
rsaPadding = options.padding;
} else {
throw new TypeError('padding must be an integer');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.padding', 'integer');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for options, please use the ERR_INVALID_OPTION_VALUE error code :-)

}
}

Expand All @@ -322,7 +323,7 @@ Sign.prototype.sign = function sign(options, encoding) {
if (options.saltLength === options.saltLength >> 0) {
pssSaltLength = options.saltLength;
} else {
throw new TypeError('saltLength must be an integer');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.saltLength', 'integer');
}
}

Expand Down Expand Up @@ -363,7 +364,7 @@ Verify.prototype.verify = function verify(options, signature, sigEncoding) {
if (options.padding === options.padding >> 0) {
rsaPadding = options.padding;
} else {
throw new TypeError('padding must be an integer');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.padding', 'integer');
}
}

Expand All @@ -372,7 +373,7 @@ Verify.prototype.verify = function verify(options, signature, sigEncoding) {
if (options.saltLength === options.saltLength >> 0) {
pssSaltLength = options.saltLength;
} else {
throw new TypeError('saltLength must be an integer');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.saltLength', 'integer');
}
}

Expand Down Expand Up @@ -417,8 +418,8 @@ function DiffieHellman(sizeOrKey, keyEncoding, generator, genEncoding) {
if (typeof sizeOrKey !== 'number' &&
typeof sizeOrKey !== 'string' &&
!ArrayBuffer.isView(sizeOrKey)) {
throw new TypeError('First argument should be number, string, ' +
'Buffer, TypedArray, or DataView');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'first argument',
['number', 'string', 'Buffer', 'TypedArray', 'DataView']);
}

if (keyEncoding) {
Expand Down Expand Up @@ -561,7 +562,7 @@ DiffieHellman.prototype.setPrivateKey = function setPrivateKey(key, encoding) {

function ECDH(curve) {
if (typeof curve !== 'string')
throw new TypeError('"curve" argument should be a string');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'curve', 'string');

this._handle = new binding.ECDH(curve);
}
Expand Down Expand Up @@ -594,7 +595,7 @@ ECDH.prototype.getPublicKey = function getPublicKey(encoding, format) {
else if (format === 'uncompressed')
f = constants.POINT_CONVERSION_UNCOMPRESSED;
else
throw new TypeError('Bad format: ' + format);
throw new error.TypeError('ERR_INVALID_FORMAT', format);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not too happy with a new error code for this single line. How about ERR_INVALID_OPT_VALUE?

} else {
f = constants.POINT_CONVERSION_UNCOMPRESSED;
}
Expand All @@ -618,7 +619,7 @@ exports.pbkdf2 = function(password,
}

if (typeof callback !== 'function')
throw new Error('No callback provided to pbkdf2');
throw new errors.Error('ERR_CRYPTO_MISSING_PBKDF2_CALLBACK');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not ERR_INVALID_CALLBACK?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this situation has limitation for PBKDF2 only and callback will be never called for other algorithms.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using ERR_INVALID_CALLBACK would still be appropriate I think


return pbkdf2(password, salt, iterations, keylen, digest, callback);
};
Expand All @@ -632,8 +633,7 @@ exports.pbkdf2Sync = function(password, salt, iterations, keylen, digest) {
function pbkdf2(password, salt, iterations, keylen, digest, callback) {

if (digest === undefined) {
throw new TypeError(
'The "digest" argument is required and must not be undefined');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'digest', 'not undefined');
}

password = toBuf(password);
Expand Down Expand Up @@ -685,10 +685,10 @@ Certificate.prototype.exportChallenge =

exports.setEngine = function setEngine(id, flags) {
if (typeof id !== 'string')
throw new TypeError('"id" argument should be a string');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'id', 'string');

if (flags && typeof flags !== 'number')
throw new TypeError('"flags" argument should be a number, if present');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'flags', 'number or undefined');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to support multiple types, use an array

throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'flags', ['number', 'undefined']);

flags = flags >>> 0;

// Use provided engine for everything by default
Expand All @@ -702,7 +702,7 @@ const kMaxUint32 = Math.pow(2, 32) - 1;

function randomFillSync(buf, offset = 0, size) {
if (!isUint8Array(buf)) {
throw new TypeError('"buf" argument must be a Buffer or Uint8Array');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'buf', 'Buffer or Uint8Array');
}

assertOffset(offset, buf.length);
Expand All @@ -717,7 +717,7 @@ exports.randomFillSync = randomFillSync;

function randomFill(buf, offset, size, cb) {
if (!isUint8Array(buf)) {
throw new TypeError('"buf" argument must be a Buffer or Uint8Array');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'buf', 'Buffer or Uint8Array');
}

if (typeof offset === 'function') {
Expand All @@ -728,7 +728,7 @@ function randomFill(buf, offset, size, cb) {
cb = size;
size = buf.length - offset;
} else if (typeof cb !== 'function') {
throw new TypeError('"cb" argument must be a function');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'cb', 'function');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not ERR_INVALID_CALLBACK?

}

assertOffset(offset, buf.length);
Expand All @@ -740,29 +740,29 @@ exports.randomFill = randomFill;

function assertOffset(offset, length) {
if (typeof offset !== 'number' || offset !== offset) {
throw new TypeError('offset must be a number');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'offset', 'number');
}

if (offset > kMaxUint32 || offset < 0) {
throw new TypeError('offset must be a uint32');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'offset', 'unit32');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling: unit32uint32

}

if (offset > kBufferMaxLength || offset > length) {
throw new RangeError('offset out of range');
throw new errors.RangeError('ERR_BUFFER_OUT_OF_BOUNDS', 'offset');
}
}

function assertSize(size, offset, length) {
if (typeof size !== 'number' || size !== size) {
throw new TypeError('size must be a number');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'size', 'number');
}

if (size > kMaxUint32 || size < 0) {
throw new TypeError('size must be a uint32');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'size', 'unit32');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling: unit32uint32

}

if (size + offset > length || size > kBufferMaxLength) {
throw new RangeError('buffer too small');
throw new errors.RangeError('ERR_BUFFER_TOO_SMALL');
}
}

Expand Down
4 changes: 4 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,13 @@ module.exports = exports = {
E('ERR_ARG_NOT_ITERABLE', '%s must be iterable');
E('ERR_ASSERTION', '%s');
E('ERR_BUFFER_OUT_OF_BOUNDS', bufferOutOfBounds);
E('ERR_BUFFER_TOO_SMALL', 'Buffer too small');
E('ERR_CHILD_CLOSED_BEFORE_REPLY', 'Child closed before reply received');
E('ERR_CONSOLE_WRITABLE_STREAM',
'Console expects a writable stream instance for %s');
E('ERR_CPU_USAGE', 'Unable to obtain cpu usage %s');
E('ERR_CRYPTO_MISSNG_KEY', 'No key provided to sign');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error code has no indication that this is about sign.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ERR_CRYPTO_MISSIGN_SIGN_KEY is preferable?

E('ERR_CRYPTO_MISSING_PBKDF2_CALLBACK', 'No callback provided to pbkdf2');
E('ERR_DNS_SET_SERVERS_FAILED', (err, servers) =>
`c-ares failed to set servers: "${err}" [${servers}]`);
E('ERR_FALSY_VALUE_REJECTION', 'Promise was rejected with falsy value');
Expand Down Expand Up @@ -198,6 +201,7 @@ E('ERR_INVALID_FD', '"fd" must be a positive integer: %s');
E('ERR_INVALID_FILE_URL_HOST',
'File URL host must be "localhost" or empty on %s');
E('ERR_INVALID_FILE_URL_PATH', 'File URL path %s');
E('ERR_INVALID_FORMAT', 'Bad format: %s');
E('ERR_INVALID_HANDLE_TYPE', 'This handle type cannot be sent');
E('ERR_INVALID_HTTP_TOKEN', (name) => `${name} must be a valid HTTP token`);
E('ERR_INVALID_IP_ADDRESS', 'Invalid IP address: %s');
Expand Down