fix(ext/node): throw ERR_INVALID_ARG_VALUE for falsy dns.lookup hostname - #36385
fix(ext/node): throw ERR_INVALID_ARG_VALUE for falsy dns.lookup hostname#36385MeGaurav4 wants to merge 1 commit into
Conversation
Deno Individual Contributor License AgreementAll contributors have signed the CLA. Thank you! This is an automated message from CLA Assistant |
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Approach looks right to me. deno_node::NODE_VERSION is 26.3.0 (ext/node/lib.rs:54), so adopting the post-EOL Node behavior is correct, and the error type / argument name / reason string match Node's lib/dns.js:202-204 and lib/internal/dns/promises.js:137-140 exactly. Validation ordering is preserved too, so dns.lookup("", { family: 7 }) still reports the options error first.
I also double-checked the "no internal consumers" claim and it holds:
net.ts:1053defaultsconst host = options.host || "localhost", and_lookupAndConnectMultipleis only reachable from_lookupAndConnect, so bothlookup()call sites get a defaulted host.dgramis safe as well:lookup4/lookup6ininternal/dgram.ts:47-68default to127.0.0.1/::1, andbinddefaults the address to0.0.0.0/::.
A few things to address before merge — see the inline comments, plus one item that isn't in the diff:
tests/node_compat/config.jsonc:1066-1069 needs updating. parallel/test-dns-lookup.js is currently ignored with the reason:
Fixture predates nodejs/node#39793 (Node 17): asserts that
dns.lookup(false, {all:true}, cb)and the promise variant throw/reject with ERR_INVALID_ARG_VALUE, but current Node calls cb(null, []) / resolves with []. Deno matches current Node — see #34801.
This PR inverts exactly that justification. The fixture asserts precisely the new behavior:
await assert.rejects(dnsPromises.lookup(false, { hints: 0, family: 0, all: true }), {
code: 'ERR_INVALID_ARG_VALUE',
});
assert.throws(() => dns.lookup(false, { hints: 0, family: 0, all: true }, common.mustNotCall()), {
code: 'ERR_INVALID_ARG_VALUE',
});Please try dropping the ignore and running it — that's the upstream fixture for this exact behavior and it's the highest-value test change available here. If it still fails for unrelated reasons, at minimum rewrite the reason so it isn't actively misleading.
One more nit: it would be good to mention in the commit body that this reverts the behavior introduced for #34801, so the history is greppable.
| @@ -1,5 +1,5 @@ | |||
| // Copyright 2018-2026 the Deno authors. MIT license. | |||
| import { assert, assertEquals, fail } from "@std/assert"; | |||
| import { assert, assertEquals, assertRejects, assertThrows, fail } from "@std/assert"; | |||
There was a problem hiding this comment.
This line is 86 chars, so tools/format.js hasn't been run — dprint wraps it and CI will fail on the format check:
import {
assert,
assertEquals,
assertRejects,
assertThrows,
fail,
} from "@std/assert";| if (!hostname) { | ||
| if (all) { | ||
| resolve([]); | ||
| } else { | ||
| resolve({ address: null, family: family === 6 ? 6 : 4 }); | ||
| } | ||
| return; | ||
| throw new ERR_INVALID_ARG_VALUE( | ||
| "hostname", | ||
| hostname, | ||
| "must be a non-empty string", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Prefer reject(...) + return here rather than throw inside the Promise executor. It's observably equivalent in this position, but throwing inside an executor is a footgun (silently swallowed once resolve/reject has been called), and this file mirrors Node, which uses reject:
// lib/internal/dns/promises.js:137
if (!hostname) {
reject(new ERR_INVALID_ARG_VALUE('hostname', hostname,
'must be a non-empty string'));
return;
}so:
if (!hostname) {
reject(
new ERR_INVALID_ARG_VALUE(
"hostname",
hostname,
"must be a non-empty string",
),
);
return;
}| Deno.test("[node/dns] lookup with falsy hostname throws", () => { | ||
| for (const hostname of [undefined, null, ""]) { | ||
| const result = await new Promise< | ||
| { error: unknown; address: unknown; family: unknown } | ||
| >((resolve) => { | ||
| // deno-lint-ignore no-explicit-any | ||
| dns.lookup(hostname as any, (error, address, family) => { | ||
| resolve({ error, address, family }); | ||
| }); | ||
| }); | ||
| assertEquals(result, { error: null, address: null, family: 4 }); | ||
| } | ||
|
|
||
| // family argument is honored when 6. | ||
| const result6 = await new Promise< | ||
| { error: unknown; address: unknown; family: unknown } | ||
| >((resolve) => { | ||
| dns.lookup( | ||
| // deno-lint-ignore no-explicit-any | ||
| undefined as any, | ||
| 6, | ||
| (error, address, family) => resolve({ error, address, family }), | ||
| assertThrows( | ||
| () => { | ||
| // deno-lint-ignore no-explicit-any | ||
| dns.lookup(hostname as any, () => {}); | ||
| }, | ||
| TypeError, | ||
| "must be a non-empty string", | ||
| ); | ||
| }); | ||
| assertEquals(result6, { error: null, address: null, family: 6 }); | ||
|
|
||
| // options.family = 6 | ||
| const resultOpt6 = await new Promise< | ||
| { error: unknown; address: unknown; family: unknown } | ||
| >((resolve) => { | ||
| dns.lookup( | ||
| // deno-lint-ignore no-explicit-any | ||
| undefined as any, | ||
| { family: 6 }, | ||
| (error, address, family) => resolve({ error, address, family }), | ||
| ); | ||
| }); | ||
| assertEquals(resultOpt6, { error: null, address: null, family: 6 }); | ||
|
|
||
| // options.all = true returns an empty array via the callback. | ||
| const resultAll = await new Promise< | ||
| { error: unknown; addresses: unknown } | ||
| >((resolve) => { | ||
| dns.lookup( | ||
| // deno-lint-ignore no-explicit-any | ||
| undefined as any, | ||
| { all: true }, | ||
| // deno-lint-ignore no-explicit-any | ||
| (error, addresses: any) => resolve({ error, addresses }), | ||
| ); | ||
| }); | ||
| assertEquals(resultAll, { error: null, addresses: [] }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The rewritten tests are correct, but they're noticeably narrower than the ones being removed. Worth keeping coverage for:
falseas the hostname — that's the case Node's own fixture uses, and the one that distinguishes "falsy" from "empty string" (it skipsvalidateStringatdns.ts:200and falls through to the new check).- The
{ all: true }and{ family: 6 }option forms, so that a future refactor reintroducing an early return in theallbranch gets caught.
Both are cheap to fold into the existing loop.
2 participants
Node.js moved the falsy-hostname case of
dns.lookupto end-of-life (nodejs/node a5f9ca1f, Node v25+):undefined,nulland""now throwERR_INVALID_ARG_VALUEsynchronously instead of invoking the callback with(null, null, family). Deno's current behavior (callbacks/resolves with a null address) matches the deprecated pre-v25 path.Root cause: both
lookupin ext/node/polyfills/dns.ts andcreateLookupPromisein ext/node/polyfills/internal/dns/promises.ts treat a falsy hostname as "no host given" and synthesize a null address. Node'sif (!hostname) throw(lib/dns.js:202) applies to both the callback and the promise APIs.Fix: throw
ERR_INVALID_ARG_VALUEwith the same message as Node in both paths, and update the regression tests (added for #34801) to assert the throw/rejection. No internal consumers relied on the old behavior:net.connectalways defaults the host to "localhost" before calling lookup.Closes #36378
Verification
ERR_INVALID_ARG_VALUE("must be a non-empty string"), lookup("localhost") unaffected, same for dns/promises (rejects)net/dgramcallers verified to never pass a falsy hostnameAI disclosure: an automated coding agent assisted with drafting this PR (analysis, code, tests) and running verification. All changes were reviewed and approved by a human before submission.