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

crypto: fix error handling in KeyObject.export #26455

Closed
wants to merge 1 commit into
base: master
from

Conversation

@tniessen
Copy link
Member

tniessen commented Mar 5, 2019

This change only affects KeyObject.export():

crypto.generateKeyPairSync('rsa', { modulusLength: 1024 }).publicKey.export()

Old error message:

TypeError: Cannot destructure property `format` of 'undefined' or 'null'.
    at parseKeyFormatAndType (internal/crypto/keys.js:154:48)
    at parseKeyEncoding (internal/crypto/keys.js:180:7)
    at parsePublicKeyEncoding (internal/crypto/keys.js:212:10)
    at PublicKeyObject.export (internal/crypto/keys.js:94:9)

New error message:

TypeError [ERR_INVALID_ARG_TYPE]: The "options" argument must be of type object. Received type undefined
    at parseKeyEncoding (internal/crypto/keys.js:176:11)
    at parsePublicKeyEncoding (internal/crypto/keys.js:215:10)
    at PublicKeyObject.export (internal/crypto/keys.js:94:9)
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@lpinca

lpinca approved these changes Mar 5, 2019

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Mar 5, 2019

semver-major, isn't it?

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 5, 2019

I prefer to think of this as a bug-fix of a (so far) rarely used API, but yeah, I guess it is technically a breaking change...

@addaleax
Copy link
Member

addaleax left a comment

I’m okay with semver-patch.

Show resolved Hide resolved lib/internal/crypto/keys.js Outdated
@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Mar 5, 2019

I thought the policy was "all error message changes are semver-major", so that we didn't have to guess the impact, we just called the changes semver-major (except by decision of TSC, they can always overrule anything if deemed in best interest of node).

Did that change? Do I misremember?

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 5, 2019

@sam-github I think that is still true, so unless the TSC is in favor of semver-patch (@addaleax, maybe others), this should technically be semver-major. (I forgot to add the label in the first place, sorry.)

@panva

This comment has been minimized.

Copy link
Contributor

panva commented Mar 5, 2019

Even for what i’d call missed input assertion or an “uncaught”? Interesting

@tniessen tniessen force-pushed the tniessen:crypto-improve-error-handling-in-parsekeyencoding branch from c6eefdb to a9b9639 Mar 5, 2019

@tniessen tniessen force-pushed the tniessen:crypto-improve-error-handling-in-parsekeyencoding branch from a9b9639 to 2accf31 Mar 5, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Mar 5, 2019

@nodejs/tsc PTAL about semverness.

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Mar 5, 2019

I can't find current policy. I find https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#breaking-changes-and-deprecations

Changing error messages for errors without error code;

That this is in the list implies that such changes are at least sometimes semver-major, but not that they are always. Maybe? IANAL.

Personally, I think its unlikely to cause trouble to introduce this as a patch.

But, my understanding was that historically Node.js guessing an error message change wouldn't cause trouble didn't work out so well, so .code was introduced, and error message changes were considered semver-major until a .code is introduced, at which point they become informational. My understanding has been wrong before, though.

@Trott

Trott approved these changes Mar 6, 2019

Copy link
Member

Trott left a comment

If we're trying to get a majority of TSC to 👍 landing this on master without the semver-major label: I'm OK with patch for this. 👍

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 8, 2019

@targos

targos approved these changes Mar 8, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Mar 8, 2019

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 9, 2019

Thanks for reviewing, everyone! Landed in 3afa5d7.

@nodejs/tsc Based on the previous discussion, I am not adding the semver-major label. Feel free to add it if you think it is appropriate.

@tniessen tniessen closed this Mar 9, 2019

tniessen added a commit that referenced this pull request Mar 9, 2019

crypto: improve error handling in parseKeyEncoding
This change only affects KeyObject.export().

PR-URL: #26455
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Mar 13, 2019

This does not land cleanly on v11. It also seems that even fixing the conflict is not enough as a test fails in that case. It would be nice if this would be manually backported if it should be backported at all.

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.