net: Validate port in createServer().listen() by dirceu · Pull Request #5732 · nodejs/node · GitHub

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

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

Maybe use a template string here?

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.

LGTM with a suggestion

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

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.

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

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

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?

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

I'll make a PR

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

Thanks for taking care of this. LGTM

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

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.

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

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

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.

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.

@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?

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?

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

Make sure we validate the port number in all kinds of `listen()` calls. Fixes: nodejs#5727

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

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

LGTM

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>

Landed in 02ac302. Thank you!

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>

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>

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>

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>

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>

dirceu mentioned this pull request Mar 15, 2016

net: _connectionKey given wrong value for large port #5727

Closed

mscdex added the net Issues and PRs related to the net subsystem. label Mar 16, 2016

cjihrig reviewed Mar 16, 2016
View reviewed changes

dirceu force-pushed the fix-5727 branch from 776f190 to 3ba1da1 Compare March 16, 2016 00:37

evanlucas reviewed Mar 16, 2016
View reviewed changes

dirceu force-pushed the fix-5727 branch from 3ba1da1 to 56815ab Compare March 16, 2016 00:45

jasnell mentioned this pull request Mar 16, 2016

net: stricter checking for internal/net isLegalPort #5733

Closed

4 tasks

jasnell reviewed Mar 16, 2016
View reviewed changes

dirceu force-pushed the fix-5727 branch from 56815ab to 0484ea1 Compare March 16, 2016 20:52

dirceu reviewed Mar 21, 2016
View reviewed changes

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

af298bd

Make sure we validate the port number in all kinds of `listen()` calls. Fixes: nodejs#5727

dirceu force-pushed the fix-5727 branch from 0484ea1 to af298bd Compare April 20, 2016 20:58

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

jasnell closed this Apr 20, 2016

cjihrig mentioned this pull request Apr 21, 2016

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

Merged

1 task

coderabbitai Bot mentioned this pull request Mar 25, 2026

fix(installer): implement auto-fallback for port 8080 conflicts (#51) NVIDIA/NemoClaw#703

Closed

7 tasks

coderabbitai Bot mentioned this pull request Apr 28, 2026

Add Next.js static-site + BFF sample project hmchangw/static-site-with-bff-example#1

Merged

net: Validate port in createServer().listen() by dirceu · Pull Request #5732 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

net: Validate port in createServer().listen()#5732dirceu wants to merge 1 commit intonodejs:masternodejs/node:masterfrom dirceu:fix-5727dirceu/node:fix-5727Copy head branch name to clipboard

Conversation

dirceu commented Mar 15, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirceu commented Mar 16, 2016

Uh oh!

evanlucas commented Mar 16, 2016

Uh oh!

evanlucas commented Mar 16, 2016

Uh oh!

cjihrig commented Mar 16, 2016

Uh oh!

jasnell commented Mar 16, 2016

Uh oh!

cjihrig commented Mar 16, 2016

Uh oh!

evanlucas commented Mar 16, 2016

Uh oh!

cjihrig commented Mar 16, 2016

Uh oh!

jasnell commented Mar 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Mar 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 20, 2016

Uh oh!

jasnell commented Apr 20, 2016

Uh oh!

jasnell commented Apr 20, 2016

Uh oh!

jasnell commented Apr 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

net: Validate port in createServer().listen()#5732
dirceu wants to merge 1 commit into
nodejs:masterfrom
dirceu:fix-5727