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: deprecate passing falsy hostname to dns.lookup #23173

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
16 changes: 16 additions & 0 deletions doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -2214,6 +2214,22 @@ the `_handle` property of the `Cipher`, `Decipher`, `DiffieHellman`,
Using the `_handle` property to access the native object is deprecated because
improper use of the native object can lead to crashing the application.

<a id="DEP0XXX"></a>
### DEP0XXX: dns.lookup() supports for falsy hostname
oyyd marked this conversation as resolved.
Show resolved Hide resolved
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/23173
description: Runtime deprecation.
-->

Type: Runtime

Previous versions of Node.js support `dns.lookup()` a falsy hostname like
oyyd marked this conversation as resolved.
Show resolved Hide resolved
`dns.lookup(false)` for the reason of backward compatibility long before.
oyyd marked this conversation as resolved.
Show resolved Hide resolved
This behavior is undocumented and seems useless in real world apps. We
might make invalid hostname throw in the future.
oyyd marked this conversation as resolved.
Show resolved Hide resolved

[`--pending-deprecation`]: cli.html#cli_pending_deprecation
[`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
[`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array
Expand Down
6 changes: 4 additions & 2 deletions lib/dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ const {
getDefaultResolver,
setDefaultResolver,
Resolver,
validateHints
validateHints,
emitInvalidHostnameWarning,
} = require('internal/dns/utils');
const {
ERR_INVALID_ARG_TYPE,
Expand Down Expand Up @@ -93,7 +94,7 @@ function lookup(hostname, options, callback) {

// Parse arguments
if (hostname && typeof hostname !== 'string') {
throw new ERR_INVALID_ARG_TYPE('hostname', ['string', 'falsy'], hostname);
throw new ERR_INVALID_ARG_TYPE('hostname', 'string', hostname);
} else if (typeof options === 'function') {
callback = options;
family = 0;
Expand All @@ -114,6 +115,7 @@ function lookup(hostname, options, callback) {
throw new ERR_INVALID_OPT_VALUE('family', family);

if (!hostname) {
emitInvalidHostnameWarning(hostname);
if (all) {
process.nextTick(callback, null, []);
} else {
Expand Down
6 changes: 4 additions & 2 deletions lib/internal/dns/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
const {
bindDefaultResolver,
Resolver: CallbackResolver,
validateHints
validateHints,
emitInvalidHostnameWarning,
} = require('internal/dns/utils');
const { codes, dnsException } = require('internal/errors');
const { isIP, isIPv4, isLegalPort } = require('internal/net');
Expand Down Expand Up @@ -56,6 +57,7 @@ function onlookupall(err, addresses) {
function createLookupPromise(family, hostname, all, hints, verbatim) {
return new Promise((resolve, reject) => {
if (!hostname) {
emitInvalidHostnameWarning(hostname);
if (all)
resolve([]);
else
Expand Down Expand Up @@ -100,7 +102,7 @@ function lookup(hostname, options) {

// Parse arguments
if (hostname && typeof hostname !== 'string') {
throw new ERR_INVALID_ARG_TYPE('hostname', ['string', 'falsy'], hostname);
throw new ERR_INVALID_ARG_TYPE('hostname', 'string', hostname);
} else if (options !== null && typeof options === 'object') {
hints = options.hints >>> 0;
family = options.family >>> 0;
Expand Down
18 changes: 17 additions & 1 deletion lib/internal/dns/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,26 @@ function validateHints(hints) {
}
}

let invalidHostnameWarningEmitted = false;

function emitInvalidHostnameWarning(hostname) {
oyyd marked this conversation as resolved.
Show resolved Hide resolved
if (invalidHostnameWarningEmitted) {
return;
}
invalidHostnameWarningEmitted = true;
process.emitWarning(
`The provided hostname "${hostname}" is not a valid ` +
'hostname, and is supported in the dns module solely for compatibility.',
'DeprecationWarning',
'DEP0XXX'
);
}

module.exports = {
bindDefaultResolver,
getDefaultResolver,
setDefaultResolver,
validateHints,
Resolver
Resolver,
emitInvalidHostnameWarning,
};
20 changes: 19 additions & 1 deletion test/parallel/test-dns-lookup.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,31 @@ cares.getaddrinfo = () => internalBinding('uv').UV_ENOENT;
const err = {
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: /^The "hostname" argument must be one of type string or falsy/
message: /^The "hostname" argument must be of type string\. Received type number/
};

common.expectsError(() => dns.lookup(1, {}), err);
common.expectsError(() => dnsPromises.lookup(1, {}), err);
}

common.expectWarning({
// For 'internal/test/binding' module.
'internal/test/binding': [
'These APIs are exposed only for testing and are not ' +
'tracked by any versioning system or deprecation process.'
],
// For dns.promises.
'ExperimentalWarning': [
'The dns.promises API is experimental'
],
// For calling `dns.lookup` with falsy `hostname`.
'DeprecationWarning': [
'The provided hostname "false" is not a valid ' +
'hostname, and is supported in the dns module solely for compatibility.',
'DEP0XXX',
],
});

common.expectsError(() => {
dns.lookup(false, 'cb');
}, {
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ assert.deepStrictEqual(dns.getServers(), []);
const errorReg = common.expectsError({
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: /^The "hostname" argument must be one of type string or falsy/
message: /^The "hostname" argument must be of type string\. Received type .*/
}, 10);

assert.throws(() => dns.lookup({}, common.mustNotCall()), errorReg);
Expand Down