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.

Sign up for GitHub

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

Jump to bottom

[v11.x backport] tls: add code for ERR_TLS_INVALID_PROTOCOL_METHOD #26952

Closed
wants to merge 2 commits into from
Closed

[v11.x backport] tls: add code for ERR_TLS_INVALID_PROTOCOL_METHOD #26952

wants to merge 2 commits into from

Conversation

sam-github
Copy link
Contributor

Backport-of: #24729

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@sam-github
tls: add code for ERR_TLS_INVALID_PROTOCOL_METHOD
84a1b18 
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
tls: revert change to invalid protocol error type
71ca71a 
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.
@nodejs-github-bot
Copy link
Collaborator

@sam-github build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/3079/pipeline

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Mar 28, 2019
@sam-github sam-github changed the base branch from master to v11.x-staging March 28, 2019 01:34
@sam-github sam-github added v11.x tls Issues and PRs related to the tls subsystem. labels Mar 28, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@sam-github sam-github mentioned this pull request Mar 28, 2019
Closed
4 tasks
@sam-github
Copy link
Contributor Author

sam-github commented Mar 28, 2019 •
edited
Loading

@nodejs/crypto @nodejs/lts @nodejs/tsc PTAL, trivial backport, just cherry-picked the semver-major, then reverted the change of the error type so it is Error again, not TypeError.

I'm not sure the original PR even should have been semver-major, it seems we're taking a looser approach to these fine details of error types nowadays, I guess we are finding that user's don't actually depend on them?

Passing an invalid string as the secureProtocol option value is pretty fatal, it might happen during development, but not during runtime. The exact type of the error returned doesn't seem so important.

@sam-github
Copy link
Contributor Author

Bundling this into #26951 (comment)

@sam-github sam-github closed this Mar 28, 2019
@sam-github sam-github deleted the backport-24729-to-v11.x branch March 28, 2019 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@targos targos targos approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tls Issues and PRs related to the tls subsystem. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sam-github @nodejs-github-bot @targos @BridgeAR