net: stricter checking for internal/net isLegalPort#5733

jasnell · 2016-03-16T03:49:02Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

net

Description of change

Please provide a description of the change here.

Add stricter testing for the isLegalPort method in internal/net.
This ensures that odd inputs such as isLegalPort(true) and
isLegalPort([1]) aren't acceptable as valid port inputs.

Refs: #5732

jasnell · 2016-03-16T03:49:25Z

/cc @evanlucas @cjihrig @dirceu

jasnell · 2016-03-16T04:05:23Z

semver-major because this causes previously acceptable (but quite odd) input values to be rejected.

evanlucas · 2016-03-16T04:06:32Z

lib/internal/net.js

should this be loose equal to catch undefined early too?
EDIT: Scratch that, for some reason I thought you were returning false. Disregard

Why is null an accepted value?

wasn't sure if null should be rejected or treated as equivalent to 0. I'm happy rejecting nulls also.

I think if we're monkeying with this function, we should get it to a point where it ONLY accepts valid ports (numbers and numeric strings). Then, if null or undefined is a valid value for a given calling context, that context can be responsible for allowing those values.

jasnell · 2016-03-17T03:43:19Z

@nodejs/ctc

jasnell · 2016-03-18T18:11:18Z

@cjihrig ... PTAL!

cjihrig · 2016-03-18T18:31:10Z

LGTM.

Here are some other values you might want to test, taken from https://github.com/nodejs/node/blob/master/test/parallel/test-net-create-connection.js.

Failures: '-0x1', '0x'
Passing: 0x${n.toString(16)}

jasnell · 2016-03-18T18:39:23Z

@cjihrig ... done! also included the other new radix modes, e.g. '-0o1' and '-0b1'

thefourtheye · 2016-03-19T03:29:29Z

lib/internal/net.js

Is + in >>> expression necessary? Internally it will be converted to number, right?

Add stricter testing for the isLegalPort method in internal/net. This ensures that odd inputs such as isLegalPort(true) and isLegalPort([1]) aren't acceptable as valid port inputs.

jasnell · 2016-03-21T18:11:51Z

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

jasnell · 2016-03-21T18:49:46Z

CI is green

thefourtheye · 2016-03-21T18:57:28Z

LGTM.

Add stricter testing for the isLegalPort method in internal/net. This ensures that odd inputs such as isLegalPort(true) and isLegalPort([1]) aren't acceptable as valid port inputs. PR-URL: #5733 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

jasnell · 2016-03-21T19:10:00Z

Landed in d0edabe

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

jasnell added net Issues and PRs related to the net subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 16, 2016

evanlucas reviewed Mar 16, 2016
View reviewed changes

jasnell mentioned this pull request Mar 16, 2016
net: Validate port in createServer().listen() #5732
Closed
4 tasks

jasnell force-pushed the better-islegalport branch from 5aefc48 to 45fed90Compare March 18, 2016 18:11

thefourtheye reviewed Mar 19, 2016
View reviewed changes

lib/internal/net.js Outdated
Copy link
Contributor
thefourtheyeMar 19, 2016
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is + in >>> expression necessary? Internally it will be converted to number, right?

net: stricter checking for internal/net isLegalPort
551e3d2
Add stricter testing for the isLegalPort method in internal/net. This ensures that odd inputs such as isLegalPort(true) and isLegalPort([1]) aren't acceptable as valid port inputs.

jasnell force-pushed the better-islegalport branch from 56970fb to 551e3d2Compare March 21, 2016 18:11

jasnell closed this Mar 21, 2016

jasnell mentioned this pull request Apr 19, 2016
What is new in v6? #6264
Closed

jasnell mentioned this pull request Apr 26, 2016
2016-04-26, Version 6.0.0 (Current) Release #6383
Closed

Uh oh!

net: stricter checking for internal/net isLegalPort#5733

net: stricter checking for internal/net isLegalPort #5733

Uh oh!

Conversation

jasnell commented Mar 16, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

jasnell commented Mar 16, 2016

Uh oh!

jasnell commented Mar 16, 2016

Uh oh!

evanlucasMar 16, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrigMar 18, 2016

Choose a reason for hiding this comment

Uh oh!

jasnellMar 18, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrigMar 18, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 17, 2016

Uh oh!

jasnell commented Mar 18, 2016

Uh oh!

cjihrig commented Mar 18, 2016

Uh oh!

jasnell commented Mar 18, 2016

Uh oh!

thefourtheyeMar 19, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 21, 2016

Uh oh!

jasnell commented Mar 21, 2016

Uh oh!

thefourtheye commented Mar 21, 2016

Uh oh!

jasnell commented Mar 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants