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

tls: convertProtocols() error handling #23606

Closed
wants to merge 5 commits into
base: master
from

Conversation

@RaymondSacks
Copy link

RaymondSacks commented Oct 12, 2018

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
Andre Jodat-Danbrani
tls: convertProtocols() error handling
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.
Show resolved Hide resolved lib/tls.js Outdated
Andre Jodat-Danbrani
tls: convertProtocols() amended an error message
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
Copy link
Member

addaleax left a comment

Thanks!

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Oct 12, 2018

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.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Oct 12, 2018

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

@mcollina
Copy link
Member

mcollina left a comment

LGTM

@Trott

This comment has been minimized.

@BridgeAR BridgeAR removed the author ready label Oct 12, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Oct 12, 2018

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

Andre Jodat-Danbrani
lib: added override for msg in ERR_OUT_OF_RANGE
Also changed corresponding usage in lib/tls.js

Amended the test case in test/parallel/test-tls-basic-validations.js
Show resolved Hide resolved lib/tls.js Outdated
@Trott

This comment has been minimized.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Oct 16, 2018

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 16, 2018

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.

lib/tls.js Outdated
@@ -60,6 +63,10 @@ function convertProtocols(protocols) {
const lens = new Array(protocols.length);
const buff = Buffer.allocUnsafe(protocols.reduce((p, c, i) => {
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.`);

This comment has been minimized.

@BridgeAR

BridgeAR Oct 17, 2018

Member

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.

This comment has been minimized.

@RaymondSacks

RaymondSacks Oct 17, 2018

Author

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

@BridgeAR BridgeAR removed the semver-major label Oct 17, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Oct 17, 2018

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

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Oct 21, 2018

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

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 24, 2018

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/.)

@RaymondSacks

This comment has been minimized.

Copy link
Author

RaymondSacks commented Oct 25, 2018

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.

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Nov 26, 2018

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?

codebytere added a commit that referenced this pull request Dec 13, 2018

tls: throw if protocol too long
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

MylesBorins added a commit that referenced this pull request Dec 26, 2018

tls: throw if protocol too long
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

@codebytere codebytere referenced this pull request Jan 4, 2019

Merged

v10.15.1 proposal #25346

codebytere added a commit that referenced this pull request Jan 29, 2019

2019-01-29, Version 10.15.1 'Dubnium' (LTS)
* 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)
* 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)
* doc:
  * add oyyd to collaborators (Ouyang Yadong) [#24300](#24300)
* tls:
  * throw if protocol too long (Andre Jodat-Danbrani) [#23606](#23606)

codebytere added a commit to codebytere/node that referenced this pull request Jan 29, 2019

2019-01-29, Version 10.15.1 'Dubnium' (LTS)
* doc:
  * add oyyd to collaborators (Ouyang Yadong) [nodejs#24300](nodejs#24300)
* tls:
  * throw if protocol too long (Andre Jodat-Danbrani) [nodejs#23606](nodejs#23606)

PR-URL: nodejs#25346

codebytere added a commit that referenced this pull request Jan 29, 2019

2019-01-29, Version 10.15.1 'Dubnium' (LTS)
* doc:
  * add oyyd to collaborators (Ouyang Yadong) [#24300](#24300)
* tls:
  * throw if protocol too long (Andre Jodat-Danbrani) [#23606](#23606)

PR-URL: #25346
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment