GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — tls: convertProtocols() error handling by pugbyte · Pull Request #23606 · nodejs/node

Conversation

pugbyte commented Oct 12, 2018

Copy link

Copy Markdown

The convertProtocols() function now throws a range error when the byte
length of a protocol is too long to fit in a Buffer.

Also added a test case in test/parallel/test-tls-basic-validations.js
to cover this.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji

All reactions

👍 1 reaction


          tls: convertProtocols() error handling

c72e9c3

The convertProtocols() function now throws a range error when the byte
length of a protocol is too long to fit in a Buffer.

Also added a test case in test/parallel/test-tls-basic-validations.js
to cover this.

nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label

Oct 12, 2018

addaleax reviewed

Oct 12, 2018

View reviewed changes

Comment thread

lib/tls.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

addaleax approved these changes

Oct 12, 2018

View reviewed changes


          tls: convertProtocols() amended an error message

85d2428

Changed the byte length error message for protocol to correctly state
that the length must be <= 255 not < 255

Amended the test case in test/parallel/test-tls-basic-validations.js

addaleax approved these changes

Oct 12, 2018

View reviewed changes

addaleax left a comment

Copy link

Copy Markdown

Member

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.

Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

addaleax added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Oct 12, 2018

addaleax commented Oct 12, 2018

Copy link

Copy Markdown

Member

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

We may or may not consider this semver-major; on the one hand, this adds a new error, on the other hand, output before this patch would have been completely broken and would have failed at another point.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell approved these changes

Oct 12, 2018

View reviewed changes

jasnell commented Oct 12, 2018

Copy link

Copy Markdown

Member

hmm... I think we need to mark as semver-major but let's see if there's @nodejs/tsc consensus on downgrading it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label

Oct 12, 2018

mcollina approved these changes

Oct 12, 2018

View reviewed changes

mcollina left a comment

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label

Oct 12, 2018

Trott commented Oct 12, 2018

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

BridgeAR reviewed

Oct 12, 2018

View reviewed changes

Comment thread

test/parallel/test-tls-basic-validations.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

BridgeAR removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Oct 12, 2018

BridgeAR commented Oct 12, 2018

Copy link

Copy Markdown

Member

I don't think that this has to be semver major since it was broken before otherwise.

addaleax reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          lib: added override for msg in ERR_OUT_OF_RANGE

5c4e7d8

Also changed corresponding usage in lib/tls.js

Amended the test case in test/parallel/test-tls-basic-validations.js

thefourtheye approved these changes

Oct 13, 2018

View reviewed changes

Comment thread

lib/tls.js Resolved

Uh oh!

There was an error while loading. Please reload this page.


          tls: minor change to error message in convertProtocols()

951692e

Trott commented Oct 13, 2018

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Oct 16, 2018

addaleax commented Oct 16, 2018

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott commented Oct 16, 2018

Copy link

Copy Markdown

Member

I think this should land as patch rather than major. I think the reason is as stated by @BridgeAR above, but I'm willing to supply more detail to the argument if there's disagreement about this.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

BridgeAR reviewed

Oct 17, 2018

View reviewed changes

Comment thread

lib/tls.js Outdated

		var len = Buffer.byteLength(c);
		if (len > 255) {
		throw new ERR_OUT_OF_RANGE('', '<= 255', len, 'The byte length of the ' +
		`protocol at index ${i} exceeds the maximum length.`);

BridgeAR Oct 17, 2018

Copy link

Copy Markdown

Member

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.

When looking at this signature it looks like a mistake that the first argument is an empty string.

I suggest to use the following instead:

ERR_OUT_OF_RANGE(str, range, input, replaceDefaultBoolean)

Using booleans as arguments is not great either but that way the first argument is at least used properly and the reader will hopefully be less surprised (as I would be). We might also use a different more expressive value than a boolean but that could be more error prone.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pugbyte Oct 17, 2018

Copy link

Copy Markdown

Author

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.

I've refactored as per your suggestion. Check it out.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

BridgeAR removed the semver-major PRs that contain breaking changes and should be released in the next major version. label

Oct 17, 2018

BridgeAR commented Oct 17, 2018

Copy link

Copy Markdown

Member

I removed the semver-major label due to the comments above. // cc @nodejs/tsc

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          lib: refactored the signature of ERROR_OUT_OF_RANGE

ca17a24

addaleax commented Oct 21, 2018

Copy link

Copy Markdown

Member

Fresh CI to include the latest changes: https://ci.nodejs.org/job/node-test-pull-request/18020/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott commented Oct 24, 2018

Copy link

Copy Markdown

Member

Landed in cdba3c1.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

targos pushed a commit that referenced this pull request

Oct 24, 2018


          tls: throw if protocol too long

09f25af

The convertProtocols() function now throws a range error when the byte
length of a protocol is too long to fit in a Buffer.

Also added a test case in test/parallel/test-tls-basic-validations.js
to cover this.

PR-URL: #23606
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>

pugbyte commented Oct 25, 2018

Copy link

Copy Markdown

Author

Thank you for the opportunity. This has been really eye-opening and I'll be much less shy about contributing to open-source projects in the future.

Trott reacted with hooray emoji

mmarchini reacted with heart emoji

All reactions

🎉 1 reaction
❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

targos mentioned this pull request

Oct 27, 2018

Release proposal: v11.1.0 #23922

Merged

This was referenced Nov 2, 2018

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#245

Closed

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#246

Closed

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#247

Closed

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#248

Closed

MylesBorins added lts-watch-v8.x labels

Nov 26, 2018

MylesBorins commented Nov 26, 2018

Copy link

Copy Markdown

Contributor

it seems like there was some debate regarding if this was Semver-Major or not. As such i'm unsure if it should be backported to LTS

thoughts?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

codebytere removed the lts-watch-v8.x label

Dec 13, 2018

codebytere pushed a commit that referenced this pull request

Dec 13, 2018


          tls: throw if protocol too long

93d4163

The convertProtocols() function now throws a range error when the byte
length of a protocol is too long to fit in a Buffer.

Also added a test case in test/parallel/test-tls-basic-validations.js
to cover this.

PR-URL: #23606
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>

codebytere added backported-to-v10.x and removed lts-watch-v10.x labels

Dec 13, 2018

MylesBorins pushed a commit that referenced this pull request

Dec 26, 2018


          tls: throw if protocol too long

3170cb4

The convertProtocols() function now throws a range error when the byte
length of a protocol is too long to fit in a Buffer.

Also added a test case in test/parallel/test-tls-basic-validations.js
to cover this.

PR-URL: #23606
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>

codebytere mentioned this pull request

Jan 4, 2019

v10.15.1 proposal #25346

Merged

codebytere added a commit that referenced this pull request

Jan 29, 2019


          2019-01-29, Version 10.15.1 'Dubnium' (LTS)

3867ef0

* doc:
  * add oyyd to collaborators (Ouyang Yadong) [#24300](#24300)
* tls:
  * throw if protocol too long (Andre Jodat-Danbrani) [#23606](#23606)

codebytere added a commit that referenced this pull request

Jan 29, 2019


          2019-01-29, Version 10.15.1 'Dubnium' (LTS)

d56212e

* doc:
  * add oyyd to collaborators (Ouyang Yadong) [#24300](#24300)
* tls:
  * throw if protocol too long (Andre Jodat-Danbrani) [#23606](#23606)

PR-URL: #25346

This was referenced Jan 29, 2019

2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-android-nodejs#280

Closed

2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-nodejs#258

Closed

2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-android-nodejs#281

Closed

2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-nodejs#259

Closed

2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-android-nodejs#282

Closed

2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-nodejs#260

Closed

2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-android-nodejs#283

Closed

2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-nodejs#261

Closed

2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-android-nodejs#284

Closed

2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere beevelop/docker-nodejs#262

Closed

Reviewers

Trott

Trott left review comments

BridgeAR

BridgeAR left review comments

mcollina

mcollina approved these changes

jasnell

jasnell approved these changes

addaleax

addaleax approved these changes

+1 more reviewer

thefourtheye

thefourtheye approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

code-and-learn

Issues related to the Code-and-Learn events and PRs submitted during the events.

tls

Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.