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.

Sign up for GitHub

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

Jump to bottom

dns: throw a TypeError in lookupService with invalid port #4839

Merged
merged 1 commit into from
Jan 25, 2016
Merged

dns: throw a TypeError in lookupService with invalid port #4839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,9 @@ exports.lookupService = function(host, port, callback) {
if (cares.isIP(host) === 0)
throw new TypeError('"host" argument needs to be a valid IP address');

if (typeof port !== 'number')
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could reuse isLegalPort() from lib/net.js.

EDIT: That would also requiring coercing the value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be better to coerce and check the result with isFinite(). Allowing a port number as a string here would be consistent with other subsystems that allow such inputs (e.g. http.request()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with accepting a string for the port for consistency, but will that make this semver-major since it is a behavior change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, in that case, I think that we should at least throw a TypeError if port is not a number first, and then, in a separate PR, change it to coerce the value to a number and make sure it is in the proper port range as a semver-major change. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I definitely think coercion of numbers should be discussed first. I also think that this PR should go forward since it fixes a bug regardless of that issue (non-number ports aren't accepted as of today).

throw new TypeError(`"port" argument must be a number, got "${port}"`);

callback = makeAsync(callback);

var req = new GetNameInfoReqWrap();
Expand Down
16 changes: 16 additions & 0 deletions test/parallel/test-dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,3 +145,19 @@ assert.doesNotThrow(function() {
hints: dns.ADDRCONFIG | dns.V4MAPPED
}, noop);
});

assert.throws(function() {
dns.lookupService('0.0.0.0');
}, /Invalid arguments/);

assert.throws(function() {
dns.lookupService('fasdfdsaf', 0, noop);
}, /"host" argument needs to be a valid IP address/);

assert.throws(function() {
dns.lookupService('0.0.0.0', '0', noop);
}, /"port" argument must be a number, got "0"/);

assert.doesNotThrow(function() {
dns.lookupService('0.0.0.0', 0, noop);
});