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.

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

net: Validate port in createServer().listen() #5732

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@dirceu
Copy link
Contributor

commented Mar 15, 2016

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

Make sure we validate the port number in all kinds of listen() calls.

Fixes: #5727

@cjihrig

View changes

test/parallel/test-regress-GH-5727.js Outdated

// The first argument is a configuration object
assert.throws(function() {
net.Server().listen({ port: -1 >>> 0 }, assert.fail);

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 16, 2016

Contributor

I think we have common.fail() for cases like this, instead of assert.fail().

@cjihrig

View changes

test/parallel/test-regress-GH-5727.js Outdated
@@ -0,0 +1,24 @@
'use strict';
var common = require('../common');

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 16, 2016

Contributor

Can you make these vars into consts.

@cjihrig

View changes

test/parallel/test-regress-GH-5727.js Outdated
// The first argument is a configuration object
assert.throws(function() {
net.Server().listen({ port: -1 >>> 0 }, assert.fail);
}, /"port" option should be >= 0 and < 65536/i);

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 16, 2016

Contributor

The port and regex could be pulled out into variables.

@dirceu dirceu force-pushed the dirceu:fix-5727 branch Mar 16, 2016

@evanlucas

View changes

lib/net.js Outdated
@@ -1270,6 +1270,12 @@ function emitListeningNT(self) {
}


function throwIfInvalidPort(port) {
if (typeof port !== 'undefined' && !isLegalPort(port))
throw new RangeError('"port" option should be >= 0 and < 65536: ' + port);

This comment has been minimized.

Copy link
@evanlucas

evanlucas Mar 16, 2016

Member

Maybe use a template string here?

@dirceu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2016

Thanks for the review @cjihrig! Fixed the points you raised.

By the way, refactoring existing tests to use common ES6 idioms (const, let, maybe arrow functions) could make a good issue for good-first-contribution.

@evanlucas

This comment has been minimized.

Copy link
Member

commented Mar 16, 2016

LGTM with a suggestion

@evanlucas

This comment has been minimized.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2016

LGTM if the CI is happy.

By the way, refactoring existing tests to use common ES6 idioms (const, let, maybe arrow functions) could make a good issue for good-first-contribution.

That's not always ideal because things get backported to older versions of Node. Once 0.10 and 0.12 go away, that might be a good idea.

@dirceu dirceu force-pushed the dirceu:fix-5727 branch Mar 16, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 16, 2016

Hm... the internal/net isLegalPort method is a bit wonky. isLegalPort(true) and isLegalPort(false) each return true, which is just odd...

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2016

Yea, isLegalPort() really only does range validation for strings and numbers.

@evanlucas

This comment has been minimized.

Copy link
Member

commented Mar 16, 2016

Agree. I moved it into internal/net.js. I didn't make any changes to how it was implemented though to make it backwards-compatible? Maybe we should update it for 6.0?

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2016

I'd be OK with that... as long as it doesn't accept undefined as a valid port.

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 16, 2016

I'll make a PR

@jasnell jasnell referenced this pull request Mar 16, 2016

Closed

net: stricter checking for internal/net isLegalPort #5733

3 of 4 tasks complete
@jasnell

View changes

lib/net.js Outdated
@@ -1270,6 +1270,12 @@ function emitListeningNT(self) {
}


function throwIfInvalidPort(port) {
if (typeof port !== 'undefined' && !isLegalPort(port))

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 16, 2016

Member

If #5733 lands, then the typeof port !== 'undefined' check would be unnecessary.

@jasnell

View changes

lib/net.js Outdated
@@ -1270,6 +1270,12 @@ function emitListeningNT(self) {
}


function throwIfInvalidPort(port) {
if (typeof port !== 'undefined' && !isLegalPort(port))
throw new RangeError(`"port" option should be >= 0 and < 65536: ${port}`);

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 16, 2016

Member

s/option/argument

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 16, 2016

Member

I would actually recommend refactoring this just a bit and moving it into internal/net as a utility method. There are a couple of other places where we check the port and throw if it's not valid that can be updated to use the utility.

function assertPort(port) {
  if (!isLegalPort(port)) 
    throw new RangeError('"port" argument must be >= 0 and < 65536');
}

Adding the the : #{port} to the end just seems unnecessary to me.

@dirceu dirceu force-pushed the dirceu:fix-5727 branch Mar 16, 2016

@trevnorris

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2016

Thanks for taking care of this. LGTM



function assertPort(port) {
if (typeof port !== 'undefined' && !isLegalPort(port))

This comment has been minimized.

Copy link
@dirceu

dirceu Mar 21, 2016

Author Contributor

Can I remove typeof port !== 'undefined' && since #5733 already landed?

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 21, 2016

Contributor

I don't think so. The changes in #5733 make isLegalPort() validate the value of port. undefined isn't a valid port, and is specific to this use case.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 21, 2016

Contributor

Although you could change it to port === undefined :-)

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 21, 2016

Member

-1... the current code allows undefined to pass through.

This comment has been minimized.

Copy link
@trevnorris

trevnorris Mar 21, 2016

Contributor

Is the following a test?

typeof net.createServer(()=>{}).listen().address().port === 'number';

If undefined is meant to be allowed to pass through then should probably document that at least in the form of a test.

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 22, 2016

Member

Looking at the code where assertPort is being used, we see:

        assertPort(h.port);
        if (h.host)
          listenAfterLookup(h.port | 0, h.host, backlog, h.exclusive);
        else
          listen(self, null, h.port | 0, 4, backlog, undefined, h.exclusive);

Notice that listen is called with h.port | 0, so that when h.port is undefined, assertPort falls through and it defaults to 0... specifically to account for when h.port is not explicitly specified.

This comment has been minimized.

Copy link
@dirceu

dirceu Mar 22, 2016

Author Contributor

@trevnorris: we do have a test that uses undefined (https://github.com/dirceu/node/blob/fix-5727/test/parallel/test-net-listen-port-option.js#L7). Should I change it to explicitly check if the port is a number?

@jasnell: got it. Speaking of which, wouldn't be clearer to use something like this?

    let port = h.port | 0;
    assertPort(port);
    if (h.host)
      listenAfterLookup(port, h.host, backlog, h.exclusive);
    else
      listen(self, null, port, 4, backlog, undefined, h.exclusive);

Otherwise, is there anything else that could be improved on the PR?

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 20, 2016

Member

Sorry, just getting back to this one. Since #5733 landed, isLegalPort(undefined) now returns false. @dirceu, not certain if that changes any of the details that you're doing here. Can you take another look?

This comment has been minimized.

Copy link
@dirceu

dirceu Apr 20, 2016

Author Contributor

@jasnell I just rebased with master and everything seems to be OK.

@jasnell

View changes

test/parallel/test-regress-GH-5727.js Outdated
const net = require('net');

const invalidPort = -1 >>> 0;
const errorMessage = '"port" argument must be >= 0 and < 65536';

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 20, 2016

Member

This actually needs to be a regex, not a string.

const errorMessage = /"port" argument must be \>= 0 and \< 65536/;

This comment has been minimized.

Copy link
@dirceu

dirceu Apr 20, 2016

Author Contributor

Fixed.

net: Validate port in createServer().listen()
Make sure we validate the port number in all kinds of `listen()` calls.

Fixes: #5727

@dirceu dirceu force-pushed the dirceu:fix-5727 branch to af298bd Apr 20, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

Marking this semver major as it builds on the prior semver-major #5733

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

LGTM

jasnell added a commit that referenced this pull request Apr 20, 2016

net: Validate port in createServer().listen()
Make sure we validate the port number in all kinds of `listen()` calls.

Fixes: #5727
PR-URL: #5732
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

Landed in 02ac302. Thank you!

@jasnell jasnell closed this Apr 20, 2016

@cjihrig cjihrig referenced this pull request Apr 21, 2016

Merged

test: don't assume IPv6 in test-regress-GH-5727 #6319

1 of 1 task complete

cjihrig added a commit that referenced this pull request Apr 21, 2016

test: don't assume IPv6 in test-regress-GH-5727
test/parallel/test-regress-GH-5727 assumed that one of the
servers would be listening on IPv6. This breaks when the machine
running the test doesn't have IPv6. This commit builds the
connection key that is compared dynamically.

Refs: #5732
PR-URL: #6319
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

net: Validate port in createServer().listen()
Make sure we validate the port number in all kinds of `listen()` calls.

Fixes: nodejs#5727
PR-URL: nodejs#5732
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

test: don't assume IPv6 in test-regress-nodejsGH-5727
test/parallel/test-regress-GH-5727 assumed that one of the
servers would be listening on IPv6. This breaks when the machine
running the test doesn't have IPv6. This commit builds the
connection key that is compared dynamically.

Refs: nodejs#5732
PR-URL: nodejs#6319
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

jasnell added a commit that referenced this pull request Apr 26, 2016

net: Validate port in createServer().listen()
Make sure we validate the port number in all kinds of `listen()` calls.

Fixes: #5727
PR-URL: #5732
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell added a commit that referenced this pull request Apr 26, 2016

test: don't assume IPv6 in test-regress-GH-5727
test/parallel/test-regress-GH-5727 assumed that one of the
servers would be listening on IPv6. This breaks when the machine
running the test doesn't have IPv6. This commit builds the
connection key that is compared dynamically.

Refs: #5732
PR-URL: #6319
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.