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

@@ -5,7 +5,9 @@ module.exports = { isLegalPort };
 // Check that the port number is not NaN when coerced to a number,
 // is an integer and that it falls within the legal range of port numbers.
 function isLegalPort(port) {
-  if (typeof port === 'string' && port.trim() === '')
+  if (port === null) return true;


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

    return false;
-  return +port === (port >>> 0) && port >= 0 && port <= 0xFFFF;
+  return +port === (+port >>> 0) && port <= 0xFFFF;


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 <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

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 45fed90 Compare March 18, 2016 18:11

thefourtheye reviewed Mar 19, 2016
View reviewed changes

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 551e3d2 Compare 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: stricter checking for internal/net isLegalPort #5733

net: stricter checking for internal/net isLegalPort #5733

jasnell commented Mar 16, 2016

jasnell commented Mar 16, 2016

jasnell commented Mar 16, 2016

evanlucas Mar 16, 2016

cjihrig Mar 18, 2016

jasnell Mar 18, 2016

cjihrig Mar 18, 2016

jasnell commented Mar 17, 2016

jasnell commented Mar 18, 2016

cjihrig commented Mar 18, 2016

jasnell commented Mar 18, 2016

thefourtheye Mar 19, 2016

jasnell commented Mar 21, 2016

jasnell commented Mar 21, 2016

thefourtheye commented Mar 21, 2016

jasnell commented Mar 21, 2016

net: stricter checking for internal/net isLegalPort #5733

net: stricter checking for internal/net isLegalPort #5733

Conversation

jasnell commented Mar 16, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

jasnell commented Mar 16, 2016

jasnell commented Mar 16, 2016

evanlucas Mar 16, 2016

Choose a reason for hiding this comment

cjihrig Mar 18, 2016

Choose a reason for hiding this comment

jasnell Mar 18, 2016

Choose a reason for hiding this comment

cjihrig Mar 18, 2016

Choose a reason for hiding this comment

jasnell commented Mar 17, 2016

jasnell commented Mar 18, 2016

cjihrig commented Mar 18, 2016

jasnell commented Mar 18, 2016

thefourtheye Mar 19, 2016

Choose a reason for hiding this comment

jasnell commented Mar 21, 2016

jasnell commented Mar 21, 2016

thefourtheye commented Mar 21, 2016

jasnell commented Mar 21, 2016