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: improve crypto error messages #3100

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@thefourtheye
Copy link
Contributor

thefourtheye commented Sep 28, 2015

Introduce a new MACRO to check if the data is a String object and
updated existing MACROs to include the actual object description to
be printed if there is an error.

cc @nodejs/crypto

@thefourtheye thefourtheye force-pushed the thefourtheye:improve-crypto-error-messages branch Sep 28, 2015

@@ -818,7 +835,7 @@ void SecureContext::SetOptions(const FunctionCallbackInfo<Value>& args) {
SecureContext* sc = Unwrap<SecureContext>(args.Holder());

if (args.Length() != 1 || !args[0]->IntegerValue()) {
return sc->env()->ThrowTypeError("Bad parameter");
return sc->env()->ThrowTypeError("Options must be an integer value");

This comment has been minimized.

@YurySolovyov

YurySolovyov Sep 29, 2015

maybe "Parameter must be an integer value" ? since it looks like it is the only one possible param here

This comment has been minimized.

@thefourtheye

thefourtheye Oct 6, 2015

Author Contributor

@YuriSolovyov Ya, that is why I left it as the function name hints. I don't have a strong opinion about this name anyway.

@thefourtheye

This comment has been minimized.

Copy link
Contributor Author

thefourtheye commented Oct 6, 2015

Bump!

@bnoordhuis

View changes

src/node_crypto.cc Outdated
@@ -33,17 +33,24 @@
#define OPENSSL_CONST
#endif

#define THROW_AND_RETURN_IF_NOT_STRING_OR_BUFFER(val) \
#define THROW_AND_RETURN_IF_NOT_STRING_OR_BUFFER(val, obj) \

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 6, 2015

Member

obj is a pretty bad name. Maybe call it prefix?

@bnoordhuis

View changes

src/node_crypto.cc Outdated
@@ -609,7 +621,7 @@ void SecureContext::SetCert(const FunctionCallbackInfo<Value>& args) {
SecureContext* sc = Unwrap<SecureContext>(args.Holder());

if (args.Length() != 1) {
return env->ThrowTypeError("Bad parameter");
return env->ThrowTypeError("Only mandatory Certificate string is expected");

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 6, 2015

Member

"Certificate string is mandatory" or maybe "Certificate string expected"? The current message reads a little odd.

@bnoordhuis

View changes

src/node_crypto.cc Outdated
@@ -642,7 +654,7 @@ void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {
(void) &clear_error_on_return; // Silence compiler warning.

if (args.Length() != 1) {
return env->ThrowTypeError("Bad parameter");
return env->ThrowTypeError("CA Certificate argument is mandatory");

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 6, 2015

Member

I'd write it as `"CA certificate", i.e. without capitalizing the second word. Applies to some of the other messages too.

@bnoordhuis

View changes

src/node_crypto.cc Outdated
if (args.Length() != 1 || !args[0]->IsString()) {
return sc->env()->ThrowTypeError("Bad parameter");
if (args.Length() != 1) {
return env->ThrowTypeError("SessionID context argument is mandatory");

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 6, 2015

Member

"Session ID context? That's what you write a few lines down.

@bnoordhuis

View changes

src/node_crypto.cc Outdated
if (args.Length() == 0 || !args[0]->IsString()) {
return sign->env()->ThrowError("Must give signtype string as argument");
if (args.Length() == 0) {
return env->ThrowError("Signtype argument is mandatory");

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 6, 2015

Member

I realize you copied this from the existing error message but you write 'Sign type' a few lines down.

@bnoordhuis

View changes

src/node_crypto.cc Outdated
if (args.Length() == 0 || !args[0]->IsString()) {
return verify->env()->ThrowError("Must give verifytype string as argument");
if (args.Length() == 0) {
return env->ThrowError("Verifytype argument is mandatory");

This comment has been minimized.

@bnoordhuis
@bnoordhuis

View changes

test/parallel/test-tls-basic-validations.js Outdated
}), /TypeError: Ticket keys must be a buffer/);

assert.throws(() => tls.createSecurePair({}),
/Error: First argument must be a tls module SecureContext/);

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 6, 2015

Member

Femto nit: 2nd argument should line up with the 1st one.

@thefourtheye thefourtheye force-pushed the thefourtheye:improve-crypto-error-messages branch 2 times, most recently Oct 10, 2015

@thefourtheye

This comment has been minimized.

Copy link
Contributor Author

thefourtheye commented Oct 10, 2015

@bnoordhuis I fixed as per your suggestions. PTAL.

@bnoordhuis

View changes

src/node_crypto.cc Outdated
if (args.Length() != 1 || !args[0]->IsString())
return env->ThrowTypeError("First argument should be a string");
if (args.Length() != 1)
return env->ThrowTypeError("ECDH Curve name argument is mandatory");

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 10, 2015

Member

"ECDH curve name" here and below?

@bnoordhuis

View changes

src/node_crypto.cc Outdated
@@ -865,7 +885,8 @@ void SecureContext::SetSessionTimeout(const FunctionCallbackInfo<Value>& args) {
SecureContext* sc = Unwrap<SecureContext>(args.Holder());

if (args.Length() != 1 || !args[0]->IsInt32()) {
return sc->env()->ThrowTypeError("Bad parameter");
return sc->env()->ThrowTypeError(
"Session Timeout must be a 32-bit integer");

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 10, 2015

Member

"Session timeout" and "PFX certificate argument" a few lines below. I'm going to stop pointing those out from now on.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Oct 10, 2015

Still quite a few places where words are Incongruously Capitalized.

@thefourtheye thefourtheye force-pushed the thefourtheye:improve-crypto-error-messages branch Oct 10, 2015

@thefourtheye thefourtheye force-pushed the thefourtheye:improve-crypto-error-messages branch Oct 10, 2015

@thefourtheye thefourtheye force-pushed the thefourtheye:improve-crypto-error-messages branch Oct 10, 2015

@thefourtheye

This comment has been minimized.

Copy link
Contributor Author

thefourtheye commented Oct 10, 2015

@bnoordhuis Sorry, I think I misunderstood your initial comments. I just thought that names are inconsistent. I didn't think about the case. I hope I updated all the places properly this time. PTAL.

@thefourtheye

This comment has been minimized.

Copy link
Contributor Author

thefourtheye commented Oct 25, 2015

Bump!

test: check options to create{Server,SecureContext}
Introduce a new test to validate the options being passed to
`createServer` and `createSecureContext`.

PR-URL: #3100

@thefourtheye thefourtheye force-pushed the thefourtheye:improve-crypto-error-messages branch to d9dabd2 Mar 25, 2016

@thefourtheye

This comment has been minimized.

Copy link
Contributor Author

thefourtheye commented Mar 25, 2016

Okay, now I addressed @bnoordhuis's comments.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Mar 26, 2016

LGTM if @bnoordhuis is happy and CI is green.
New CI: https://ci.nodejs.org/job/node-test-pull-request/2072/

@jasnell jasnell added semver-major and removed stalled labels Mar 26, 2016

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Mar 26, 2016

semver-major because of the changes to the error messages.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Mar 26, 2016

LGTM

1 similar comment
@indutny

This comment has been minimized.

Copy link
Member

indutny commented Mar 26, 2016

LGTM

thefourtheye added a commit that referenced this pull request Mar 26, 2016

crypto: improve error messages
Introduce a new MACRO to check if the data is a String object and
update existing MACROs to include the actual object description to
be printed in case of an error.

PR-URL: #3100
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@thefourtheye

This comment has been minimized.

Copy link
Contributor Author

thefourtheye commented Mar 26, 2016

Thanks for the reviews :-) Landed in 41feaa8.

@thefourtheye thefourtheye deleted the thefourtheye:improve-crypto-error-messages branch Mar 26, 2016

@jasnell jasnell referenced this pull request Apr 19, 2016

Closed

What is new in v6? #6264

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* OS X
  * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7
    [#6402](#6402).
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* OS X
  * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7
    [#6402](#6402).
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* OS X
  * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7
    [#6402](#6402).
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).
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.