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

rumkin · 2017-08-10T02:33:11Z

Change crypto.js to use internal/errors.js module.

See #11273 for more info.

cc @jasnell

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

errors, crypto

refack · 2017-08-10T02:55:28Z

@rumkin thank you for this contribution.
I've run a quick CI to check if tests need to be updated with the new errors - https://ci.nodejs.org/job/node-test-commit-linuxone/7864/

not ok 282 parallel/test-crypto-dh
  ---
  duration_ms: 0.176
  severity: fail
  stack: |-
    assert.js:632
          throw actual;
          ^
    
    TypeError [ERR_INVALID_ARG_TYPE]: The first argument must be one of type number, string, Buffer, TypedArray, or DataView
        at new DiffieHellman (crypto.js:421:11)
        at Object.DiffieHellman (crypto.js:416:12)
        at assert.throws (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-dh.js:29:10)
        at tryBlock (assert.js:602:5)
        at innerThrows (assert.js:621:18)
        at Function.throws (assert.js:648:3)
        at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-dh.js:28:8)
        at Module._compile (module.js:573:30)
        at Object.Module._extensions..js (module.js:584:10)
        at Module.load (module.js:507:32)
  ...
not ok 290 parallel/test-crypto-engine
  ---
  duration_ms: 0.92
  severity: fail
  stack: |-
    assert.js:632
          throw actual;
          ^
    
    TypeError [ERR_INVALID_ARG_TYPE]: The "id" argument must be of type string
        at Object.setEngine (crypto.js:688:11)
        at /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-engine.js:11:10
        at tryBlock (assert.js:602:5)
        at innerThrows (assert.js:621:18)
        at Function.throws (assert.js:648:3)
        at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-engine.js:10:8)
        at Module._compile (module.js:573:30)
        at Object.Module._extensions..js (module.js:584:10)
        at Module.load (module.js:507:32)
        at tryModuleLoad (module.js:470:12)
  ...
not ok 299 parallel/test-crypto-pbkdf2
  ---
  duration_ms: 0.138
  severity: fail
  stack: |-
    assert.js:632
          throw actual;
          ^
    
    Error [ERR_CRYPTO_MISSING_PBKDF2_CALLBACK]: No callback provided to pbkdf2
        at Object.exports.pbkdf2 (crypto.js:622:11)
        at /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-pbkdf2.js:59:10
        at tryBlock (assert.js:602:5)
        at innerThrows (assert.js:621:18)
        at Function.throws (assert.js:648:3)
        at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-pbkdf2.js:58:8)
        at Module._compile (module.js:573:30)
        at Object.Module._extensions..js (module.js:584:10)
        at Module.load (module.js:507:32)
        at tryModuleLoad (module.js:470:12)
  ...
not ok 300 parallel/test-crypto-random
  ---
  duration_ms: 0.112
  severity: fail
  stack: |-
    assert.js:632
          throw actual;
          ^
    
    TypeError [ERR_INVALID_ARG_TYPE]: The "offset" argument must be of type number
        at assertOffset (crypto.js:743:11)
        at Object.randomFillSync (crypto.js:708:3)
        at assert.throws (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-random.js:158:14)
        at tryBlock (assert.js:602:5)
        at innerThrows (assert.js:621:18)
        at Function.throws (assert.js:648:3)
        at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-random.js:157:12)
        at Module._compile (module.js:573:30)
        at Object.Module._extensions..js (module.js:584:10)
        at Module.load (module.js:507:32)
  ...
not ok 303 parallel/test-crypto-sign-verify
  ---
  duration_ms: 0.174
  severity: fail
  stack: |-
    assert.js:632
          throw actual;
          ^
    
    TypeError [ERR_INVALID_ARG_TYPE]: The "options.padding" property must be of type integer
        at Sign.sign (crypto.js:317:13)
        at assert.throws (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-sign-verify.js:208:12)
        at tryBlock (assert.js:602:5)
        at innerThrows (assert.js:621:18)
        at Function.throws (assert.js:648:3)
        at __dirname.forEach (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-sign-verify.js:205:14)
        at Array.forEach (<anonymous>)
        at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-sign-verify.js:204:6)
        at Module._compile (module.js:573:30)
        at Object.Module._extensions..js (module.js:584:10)
  ...

This PR should include adjusted tests or new ones according to https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md#testing-new-errors

tniessen

Please add tests according to this document and test your changes locally (if possible).

tniessen · 2017-08-10T15:56:24Z

lib/crypto.js

@@ -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');


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

tniessen · 2017-08-10T15:57:14Z

lib/internal/errors.js

 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');


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

ERR_CRYPTO_MISSIGN_SIGN_KEY is preferable?

tniessen · 2017-08-10T15:58:58Z

lib/crypto.js

@@ -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');


Why not ERR_INVALID_CALLBACK?

tniessen · 2017-08-10T15:59:02Z

lib/crypto.js

@@ -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');


Why not ERR_INVALID_CALLBACK?

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

Using ERR_INVALID_CALLBACK would still be appropriate I think

tniessen · 2017-08-10T16:04:15Z

lib/crypto.js

@@ -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);


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

tniessen · 2017-08-10T16:05:22Z

lib/crypto.js

  }

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


Spelling: unit32 → uint32

tniessen · 2017-08-10T16:05:26Z

lib/crypto.js

  }

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


Spelling: unit32 → uint32

jasnell · 2017-08-10T20:40:03Z

lib/crypto.js

@@ -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');


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

jasnell · 2017-08-10T20:42:49Z

lib/crypto.js


  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');


to support multiple types, use an array

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

…odes for options

rumkin · 2017-08-10T21:08:50Z

@jasnell @refack error codes are fixed.

@refack renamed ERR_CRYPTO_MISSING_KEY -> ERR_CRYPTO_MISSING_SIGN_KEY.

mhdawson

Interesting that no tests need to be updated... But CI run will validate that and changes LGTM.

mhdawson · 2017-08-18T15:18:51Z

CI run: https://ci.nodejs.org/job/node-test-pull-request/9737/

rumkin · 2017-08-18T15:21:44Z

@mhdawson I will update tests to check error codes on the weekend.

refack · 2017-08-18T15:55:45Z

@mhdawson failing tests are in #14724 (comment) under the fold.

mhdawson · 2017-08-18T16:19:56Z

Ah ok thanks, did not catch that, of course CI run was very red :)

rumkin · 2017-08-20T20:01:58Z

I've update common.expectsError source to support error.code. But I think it should be placed in another PR, isn't it?

refack

Very good progress!

refack · 2017-08-20T21:48:12Z

test/common/index.js

@@ -709,6 +709,11 @@ exports.expectsError = function expectsError(fn, settings, exact) {
      assert(error instanceof type,
             `${error.name} is not instance of ${type.name}`);
    }
+    if ('code' in settings) {


This is a very good idea! I think L703 agrees with your train of thought 😉

node/test/common/index.js

Line 703 in 592766b

assert.strictEqual(error.code, settings.code);

I promise that I will sleep before write code next time 😅

refack

Need a few tweaks

refack · 2017-08-20T21:52:31Z

test/parallel/test-crypto-dh.js

-  /^TypeError: First argument should be number, string, Buffer, TypedArray, or DataView$/;
-
-assert.throws(() => {
+common.expectsError(function() {


This is obviously good, but you can notice that the RegExp included also the Type (/^TypeError: .../).
So you should add

type: TypeError

to all the assertion below.

refack · 2017-08-20T21:55:41Z

I've update common.expectsError source to support error.code. But I think it should be placed in another PR, isn't it?

Like I said, you had a good idea, it's just already there 😉

(Context: expectsError was actually introduced to first of all validate the code. That is what was missing from assert.throws.)

refack · 2017-08-20T21:57:06Z

Quick check: https://ci.nodejs.org/job/node-test-commit-linuxone/8128/

refack · 2017-08-20T22:17:19Z

test/parallel/test-crypto-dh.js

@@ -198,9 +203,12 @@ if (availableCurves.has('prime256v1') && availableCurves.has('secp256k1')) {
  firstByte = ecdh1.getPublicKey('buffer', 'hybrid')[0];
  assert(firstByte === 6 || firstByte === 7);
  // format value should be string
-  assert.throws(() => {
+  common.expectsError(function() {


Seems like this does not throw an errors.Error:

not ok 294 parallel/test-crypto-dh --- duration_ms: 1.543 severity: fail stack: |- assert.js:42 throw new errors.AssertionError({ ^ AssertionError [ERR_ASSERTION]: undefined === 'ERR_INVALID_OPT_VALUE' at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:703:12) at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:509:15) at expectedException (assert.js:597:19) at innerThrows (assert.js:631:21) at Function.throws (assert.js:648:3) at Object.expectsError (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:742:12) at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-dh.js:206:10) at Module._compile (module.js:573:30) at Object.Module._extensions..js (module.js:584:10) at Module.load (module.js:507:32) ...

refack · 2017-08-20T22:18:41Z

test/parallel/test-crypto-random.js

      crypto.randomFillSync(buf, 1, 10);
    }, errMessages.bufferTooSmall);

-    assert.throws(() => {
+    common.expectsError(() => {


Here the RegExp doesn't match:

not ok 300 parallel/test-crypto-random --- duration_ms: 0.225 severity: fail stack: |- assert.js:42 throw new errors.AssertionError({ ^ AssertionError [ERR_ASSERTION]: The "size" argument must be of type uint32 does not match /"size" .* unint32/ at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:722:9) at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:509:15) at expectedException (assert.js:597:19) at innerThrows (assert.js:631:21) at Function.throws (assert.js:648:3) at Object.expectsError (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:742:12) at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-random.js:225:14) at Module._compile (module.js:573:30) at Object.Module._extensions..js (module.js:584:10) at Module.load (module.js:507:32) ...

refack · 2017-08-20T22:19:19Z

test/parallel/test-crypto-sign-verify.js

@@ -234,9 +240,11 @@ const modSize = 1024;

 // Test throws exception when key options is null
 {
-  assert.throws(() => {
+  expectsError(() => {


forgot the common.:

not ok 303 parallel/test-crypto-sign-verify --- duration_ms: 0.515 severity: fail stack: |- /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-sign-verify.js:243 expectsError(() => { ^ ReferenceError: expectsError is not defined at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-sign-verify.js:243:3) at Module._compile (module.js:573:30) at Object.Module._extensions..js (module.js:584:10) at Module.load (module.js:507:32) at tryModuleLoad (module.js:470:12) at Function.Module._load (module.js:462:3) at Function.Module.runMain (module.js:609:10) at startup (bootstrap_node.js:158:16) at bootstrap_node.js:598:3 ...

refack · 2017-08-20T22:21:15Z

@rumkin while the CI is a very useful tool, you should try to run the test suite locally, it'll save review back-and-forth.
See https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-5-test

rumkin · 2017-08-21T14:04:14Z

@refack

while the CI is a very useful tool, you should try to run the test suite locally, it'll save review back-and-forth.

Yep, I'm searching the way to run tests myself.

BridgeAR · 2017-08-31T17:13:11Z

@rumkin please have a look at https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-5-test

BridgeAR · 2017-09-13T19:17:32Z

Ping @rumkin

rumkin · 2017-09-14T18:11:53Z

@BridgeAR I'm here. Just bought more RAM to run test on my own.

BridgeAR · 2017-09-19T21:42:28Z

@rumkin this needs a rebase and did you get the tests to work on your machine? They should not require a huge amount of memory though (as far as I know).

rumkin · 2017-09-21T01:47:10Z

@BridgeAR I'm running tests in VM. That's why I had been need more RAM. Anyway I've fixed errors. And ready to rebase changes.

rumkin · 2017-09-22T09:48:52Z

@BridgeAR @refack after rebasing I figured that there isn't significant changes in my PR. The only difference was in error codes. So I'm closing this PR. Thanks for help.

BridgeAR · 2017-09-22T20:28:34Z

@rumkin I am sorry to head that. The refactoring of the crypto module landed just a few days ago. Thanks a lot for your contribution anyway!

rumkin · 2017-09-23T18:55:49Z

@BridgeAR It's ok. Now we sure everything done right. I think it's very important for crypto module.

errors,crypto: use internal/errors.js

98f0ce1

nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Aug 10, 2017

refack self-assigned this Aug 10, 2017

tniessen reviewed Aug 10, 2017

View reviewed changes

errors,crypto: fix typos and update error codes

a399166

jasnell reviewed Aug 10, 2017

View reviewed changes

errors,crypto: remove error code for pbkdf2 callback, replace error c…

fd8d0c4

…odes for options

rumkin force-pushed the master branch from 807f775 to fd8d0c4 Compare August 10, 2017 21:03

jasnell approved these changes Aug 11, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 11, 2017

mhdawson approved these changes Aug 18, 2017

View reviewed changes

rumkin force-pushed the master branch from 4988ce4 to 592766b Compare August 20, 2017 19:56

refack reviewed Aug 20, 2017

View reviewed changes

refack suggested changes Aug 20, 2017

View reviewed changes

stouf mentioned this pull request Aug 24, 2017

Error.name vs Error.code nodejs/help#789

Closed

rumkin added 2 commits September 21, 2017 04:40

errors,crypto,test: fix saltLength error code, fix crypto tests'

6f1b70d

test: fix typos and reference error in crypto tests

98da1dc

rumkin force-pushed the master branch from 592766b to 98da1dc Compare September 21, 2017 01:42

rumkin closed this Sep 22, 2017

refack removed their assignment Oct 12, 2018

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

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

Conversation

rumkin commented Aug 10, 2017

refack commented Aug 10, 2017

tniessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rumkin commented Aug 10, 2017

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Aug 18, 2017

rumkin commented Aug 18, 2017

refack commented Aug 18, 2017

mhdawson commented Aug 18, 2017

rumkin commented Aug 20, 2017

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Aug 20, 2017

refack commented Aug 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Aug 20, 2017

rumkin commented Aug 21, 2017

BridgeAR commented Aug 31, 2017

BridgeAR commented Sep 13, 2017

rumkin commented Sep 14, 2017

BridgeAR commented Sep 19, 2017

rumkin commented Sep 21, 2017 • edited Loading

rumkin commented Sep 22, 2017

BridgeAR commented Sep 22, 2017

rumkin commented Sep 23, 2017

rumkin commented Sep 21, 2017 •

edited

Loading