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

dns: coerce port to number in lookupService #4883

Closed
wants to merge 1 commit into from

Conversation

evanlucas
Copy link
Contributor

Previously, port could be any number in dns.lookupService. This change
throws a TypeError if port is outside the range of 0-65535. It also
coerces the port to a number.

This includes the commit from #4882 as it depends on it.

I have separated them into two PRs for the sake of cherry-picking to release branches though as this is semver-major.

@evanlucas evanlucas added dns Issues and PRs related to the dns subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jan 26, 2016
@bnoordhuis
Copy link
Member

LGTM

@evanlucas
Copy link
Contributor Author

/cc @mscdex since this was your suggestion

@mscdex
Copy link
Contributor

mscdex commented Jan 26, 2016

LGTM

2 similar comments
@cjihrig
Copy link
Contributor

cjihrig commented Jan 26, 2016

LGTM

@JungMinu
Copy link
Member

LGTM

@saghul
Copy link
Member

saghul commented Jan 27, 2016

LGTM. Why is this semver-major?

@evanlucas
Copy link
Contributor Author

It changes behavior. Before one could pass 10000000 as the port. Now a TypeError will be thrown.

It also accepts a string that can be coerced into a number that is a valid port. Before it did not

@jasnell
Copy link
Member

jasnell commented Jan 27, 2016

@evanlucas
Copy link
Contributor Author

Rebased and running CI one more time: https://ci.nodejs.org/job/node-test-pull-request/1470/

@evanlucas
Copy link
Contributor Author

This probably warrants a doc update and some additional tests. I will add those and then run CI again. Sorry for omitting them in the first place.

@evanlucas
Copy link
Contributor Author

Ok, docs updated and additional tests added. PTAL

@@ -126,6 +126,10 @@ on some operating systems (e.g FreeBSD 10.1).
Resolves the given `address` and `port` into a hostname and service using
the operating system's underlying `getnameinfo` implementation.

If `address` is not a valid IP address, a TypeError will be thrown.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add backticks around TypeError here and below.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 1, 2016

Still LGTM with one small comment.

Previously, port could be any number in dns.lookupService. This change
throws a TypeError if port is outside the range of 0-65535. It also
coerces the port to a number.
@evanlucas
Copy link
Contributor Author

Fixed

@jasnell
Copy link
Member

jasnell commented Feb 1, 2016

LGTM

jasnell pushed a commit that referenced this pull request Feb 1, 2016
Previously, port could be any number in dns.lookupService. This change
throws a TypeError if port is outside the range of 0-65535. It also
coerces the port to a number.

PR-URL: #4883
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Feb 1, 2016

Landed in f3be421

@jasnell jasnell closed this Feb 1, 2016
@evanlucas evanlucas deleted the dnsnet-internal2 branch February 1, 2016 17:38
@jasnell jasnell mentioned this pull request Mar 17, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Previously, port could be any number in dns.lookupService. This change
throws a TypeError if port is outside the range of 0-65535. It also
coerces the port to a number.

PR-URL: nodejs#4883
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants