tls: add code for ERR_TLS_INVALID_PROTOCOL_METHOD by sam-github · Pull Request #24729 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation12 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

sam-github

Add an error code property to invalid secureProtocol method
exceptions.

Node errors are supposed to have a .code property, so use the new C++ errors framework.

@joyeecheung Its not clear to me whether this has to be semver-major. I changed the error type from Error (env->ThrowError()) to TypeError, which would be semver-major. The .message should not have changed. If its possible to make this semver-minor by changing the error type, I will.... then introduce a tiny semver-major PR to change the type to TypeError for future releases.

If any kind of change like this to the errors is semver-major by definition, then I won't bother splitting it up.

Checklist

@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added c++

Issues and PRs that require attention from people who are familiar with C++.

crypto

Issues and PRs related to the crypto subsystem.

labels

Nov 29, 2018

@sam-github sam-github added semver-major

PRs that contain breaking changes and should be released in the next major version.

tls

Issues and PRs related to the tls subsystem.

and removed crypto

Issues and PRs related to the crypto subsystem.

labels

Nov 29, 2018

Trott

@Trott

@sam-github

Add an error code property to invalid secureProtocol method exceptions.

@sam-github

@Trott

joyeecheung

Choose a reason for hiding this comment

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

LGTM. Regarding semver-ness, I don't feel strongly that this should be semver-major, but it probably doesn't hurt too much to stay on the safe side?

@sam-github

jasnell

@sam-github

Last ci was green, despite what github shows above (thanks @Trott for restarting it).

@Trott

@Trott Trott added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Dec 2, 2018

@danbev

danbev pushed a commit that referenced this pull request

Dec 3, 2018

@sam-github @danbev

Add an error code property to invalid secureProtocol method exceptions.

PR-URL: #24729 Reviewed-By: Joyee Cheung joyeec9h3@gmail.com Reviewed-By: James M Snell jasnell@gmail.com

refack pushed a commit to refack/node that referenced this pull request

Jan 14, 2019

@sam-github @refack

Add an error code property to invalid secureProtocol method exceptions.

PR-URL: nodejs#24729 Reviewed-By: Joyee Cheung joyeec9h3@gmail.com Reviewed-By: James M Snell jasnell@gmail.com

@sam-github

OK, not backporting this to 11.x is hurting the backport of TLS1.3 to 11.x.

TLS1.3 tests have many assertions on TLS error's .code property, which were introduced in #25093, which wasn't backported because this wasn't backported.

I propose backporting this to 11.x as semver-minor by changing the type of the error back to Error from TypeError.

The 11.x backport would then flow into 10.x as semver-minor as a pre-req for TLS1.3, so this affects LTS.

@targos @joyeecheung @nodejs/lts do you agree with what I propose?

sam-github added a commit to sam-github/node that referenced this pull request

Mar 28, 2019

@sam-github

Add an error code property to invalid secureProtocol method exceptions.

PR-URL: nodejs#24729 Reviewed-By: Joyee Cheung joyeec9h3@gmail.com Reviewed-By: James M Snell jasnell@gmail.com

sam-github added a commit to sam-github/node that referenced this pull request

Mar 28, 2019

@sam-github

In nodejs#24729, the error was changed to be a TypeError, which is the standard type for this kind of error. However, it was Error in 11.x and earlier, so revert that single aspect, so the backport can be semver-minor.

sam-github added a commit to sam-github/node that referenced this pull request

Apr 11, 2019

@sam-github

Add an error code property to invalid secureProtocol method exceptions.

PR-URL: nodejs#24729 Reviewed-By: Joyee Cheung joyeec9h3@gmail.com Reviewed-By: James M Snell jasnell@gmail.com

sam-github added a commit to sam-github/node that referenced this pull request

Apr 11, 2019

@sam-github

In nodejs#24729, the error was changed to be a TypeError, which is the standard type for this kind of error. However, it was Error in 11.x and earlier, so revert that single aspect, so the backport can be semver-minor.

BethGriggs pushed a commit that referenced this pull request

Apr 15, 2019

@sam-github @BethGriggs

Add an error code property to invalid secureProtocol method exceptions.

Backport-PR-URL: #26951 PR-URL: #24729 Reviewed-By: Joyee Cheung joyeec9h3@gmail.com Reviewed-By: James M Snell jasnell@gmail.com

BethGriggs pushed a commit that referenced this pull request

Apr 15, 2019

@sam-github @BethGriggs

In #24729, the error was changed to be a TypeError, which is the standard type for this kind of error. However, it was Error in 11.x and earlier, so revert that single aspect, so the backport can be semver-minor.

PR-URL: #26951 Reviewed-By: Rod Vagg rod@vagg.org Reviewed-By: Beth Griggs Bethany.Griggs@uk.ibm.com

codebytere added a commit that referenced this pull request

Apr 19, 2019

@codebytere

Notable changes:

codebytere added a commit that referenced this pull request

Apr 19, 2019

@codebytere

Notable changes:

BethGriggs added a commit that referenced this pull request

Apr 22, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

This was referenced

Apr 23, 2019

codebytere added a commit that referenced this pull request

Apr 30, 2019

@codebytere

Notable changes:

PR-URL: #27314

codebytere added a commit that referenced this pull request

Apr 30, 2019

@codebytere

Notable changes:

PR-URL: #27314

Labels

author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

c++

Issues and PRs that require attention from people who are familiar with C++.

semver-major

PRs that contain breaking changes and should be released in the next major version.

tls

Issues and PRs related to the tls subsystem.