-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
src: add .code and SSL specific error properties #25093
Conversation
@jasnell PTAL |
35dc539
to
18c6c1f
Compare
@sam-github LGTM apart from I'm seeing the same test failures locally as reported by the CI run. $ out/Release/node --tls-v1.1 /Users/danielbevenius/work/nodejs/node/test/parallel/test-tls-cli-min-version-1.1.js
assert.js:86
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected
+ undefined
- 'ERR_SSL_VERSION_TOO_LOW'
at common.mustCall (/Users/danielbevenius/work/nodejs/node/test/parallel/test-tls-min-max-version.js:31:14)
at /Users/danielbevenius/work/nodejs/node/test/common/index.js:373:15
at /Users/danielbevenius/work/nodejs/node/test/common/index.js:373:15
at maybeCallback (/Users/danielbevenius/work/nodejs/node/test/fixtures/tls-connect.js:97:7)
at Server.<anonymous> (/Users/danielbevenius/work/nodejs/node/test/fixtures/tls-connect.js:84:7)
at Server.emit (events.js:188:13)
at TLSSocket.onSocketTLSError (_tls_wrap.js:728:29)
at TLSSocket.emit (events.js:188:13)
at TLSSocket._tlsError (_tls_wrap.js:608:8)
at TLSSocket.emit (events.js:188:13)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a comment
SSL errors have a long structured message, but lacked the standard .code property which can be used for stable comparisons. Add a `code` property, as well as the 3 string components of an SSL error: `reason`, `library`, and `function`.
18c6c1f
to
04e841c
Compare
@danbev Oops, reordered the error number peek to after the error message long-form printf while addressing some comments, which doesn't work (printf clears the err number). Tests are passing again locally, lets see how CI does. |
Landed in ca9c0c9 |
SSL errors have a long structured message, but lacked the standard .code property which can be used for stable comparisons. Add a `code` property, as well as the 3 string components of an SSL error: `reason`, `library`, and `function`. PR-URL: #25093 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Should this be backported to |
SSL errors have a long structured message, but lacked the standard .code property which can be used for stable comparisons. Add a `code` property, as well as the 3 string components of an SSL error: `reason`, `library`, and `function`. Backport-PR-URL: https://github.com/nodejs/node/25376 PR-URL: nodejs#25093 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Depends on #24729, semver-major, so not a backport candidate. |
SSL errors have a long structured message, but lacked the standard .code property which can be used for stable comparisons. Add a `code` property, as well as the 3 string components of an SSL error: `reason`, `library`, and `function`. PR-URL: nodejs#25093 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
SSL errors have a long structured message, but lacked the standard .code property which can be used for stable comparisons. Add a `code` property, as well as the 3 string components of an SSL error: `reason`, `library`, and `function`. PR-URL: nodejs#25093 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
SSL errors have a long structured message, but lacked the standard .code property which can be used for stable comparisons. Add a `code` property, as well as the 3 string components of an SSL error: `reason`, `library`, and `function`. PR-URL: nodejs#25093 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
SSL errors have a long structured message, but lacked the standard .code property which can be used for stable comparisons. Add a `code` property, as well as the 3 string components of an SSL error: `reason`, `library`, and `function`. PR-URL: nodejs#25093 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
SSL errors have a long structured message, but lacked the standard .code property which can be used for stable comparisons. Add a `code` property, as well as the 3 string components of an SSL error: `reason`, `library`, and `function`. Backport-PR-URL: #26951 PR-URL: #25093 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Notable changes: * deps: add s390 asm rules for OpenSSL-1.1.1 (Shigeki Ohtsu) [#19794](#19794) * src: add .code and SSL specific error properties (Sam Roberts) [#25093](#25093) * tls: * add --tls-min-v1.2 CLI switch (Sam Roberts) [#26951](#26951) * supported shared openssl 1.1.0 (Sam Roberts) [#26951](#26951) * revert default max toTLSv1.2 (Sam Roberts) [#26951](#26951) * revert change to invalid protocol error type (Sam Roberts) [#26951](#26951) * support TLSv1.3 (Sam Roberts) [#26209](#26209) * add code for ERR\_TLS\_INVALID\_PROTOCOL\_METHOD (Sam Roberts) [#24729](#24729)
Notable changes: * deps: add s390 asm rules for OpenSSL-1.1.1 (Shigeki Ohtsu) [#19794](#19794) * src: add .code and SSL specific error properties (Sam Roberts) [#25093](#25093) * tls: * add --tls-min-v1.2 CLI switch (Sam Roberts) [#26951](#26951) * supported shared openssl 1.1.0 (Sam Roberts) [#26951](#26951) * revert default max toTLSv1.2 (Sam Roberts) [#26951](#26951) * revert change to invalid protocol error type (Sam Roberts) [#26951](#26951) * support TLSv1.3 (Sam Roberts) [#26209](#26209) * add code for ERR\_TLS\_INVALID\_PROTOCOL\_METHOD (Sam Roberts) [#24729](#24729)
Notable changes: * deps: add s390 asm rules for OpenSSL-1.1.1 (Shigeki Ohtsu) [#19794](#19794) * src: add .code and SSL specific error properties (Sam Roberts) [#25093](#25093) * tls: * add --tls-min-v1.2 CLI switch (Sam Roberts) [#26951](#26951) * supported shared openssl 1.1.0 (Sam Roberts) [#26951](#26951) * revert default max toTLSv1.2 (Sam Roberts) [#26951](#26951) * revert change to invalid protocol error type (Sam Roberts) [#26951](#26951) * support TLSv1.3 (Sam Roberts) [#26209](#26209) * add code for ERR\_TLS\_INVALID\_PROTOCOL\_METHOD (Sam Roberts) [#24729](#24729) PR-URL: #27314
Notable changes: * deps: add s390 asm rules for OpenSSL-1.1.1 (Shigeki Ohtsu) [#19794](#19794) * src: add .code and SSL specific error properties (Sam Roberts) [#25093](#25093) * tls: * add --tls-min-v1.2 CLI switch (Sam Roberts) [#26951](#26951) * supported shared openssl 1.1.0 (Sam Roberts) [#26951](#26951) * revert default max toTLSv1.2 (Sam Roberts) [#26951](#26951) * revert change to invalid protocol error type (Sam Roberts) [#26951](#26951) * support TLSv1.3 (Sam Roberts) [#26209](#26209) * add code for ERR\_TLS\_INVALID\_PROTOCOL\_METHOD (Sam Roberts) [#24729](#24729) PR-URL: #27314
SSL errors have a long structured message, but lacked the standard .code
property which can be used for stable comparisons. Add a
code
property, as well as the 3 string components of an SSL error:
reason
,library
, andfunction
.This is WIP, TBD is move the property strings to the isolate, and add docs and more tests.
I've seen a reasonable amount of code that is doing string matching on the error string, looking for the reason part, I think exposing the properties makes more sense. Also, I think the intention is for all Errors to expose
.code
properties for stable programmatic inspection.I've gone some way to exposing these properties for node_crypto.cc, too, but I'm not done. Once this PR lands I'll keep working on it.
/to @nodejs/crypto
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes