From d59599fc187c559ee231882773e1c5a2b932fc3d Mon Sep 17 00:00:00 2001 From: Yoshiya Hinosawa Date: Wed, 16 Oct 2024 20:58:44 +0900 Subject: [PATCH] fix(ext/node): fix dns.lookup result ordering (#26264) partially unblocks #25470 This PR aligns the resolution of `localhost` hostname to Node.js behavior. In Node.js `dns.lookup("localhost", (_, addr) => console.log(addr))` prints ipv6 address `::1`, but it prints ipv4 address `127.0.0.1` in Deno. That difference causes some errors in the work of enabling `createConnection` option in `http.request` (#25470). This PR fixes the issue by aligning `dns.lookup` behavior to Node.js. This PR also changes the following behaviors (resolving TODOs): - `http.createServer` now listens on ipv6 address `[::]` by default on linux/mac - `net.createServer` now listens on ipv6 address `[::]` by default on linux/mac These changes are also alignments to Node.js behaviors. --- ext/node/polyfills/http.ts | 6 ++--- ext/node/polyfills/internal/dns/utils.ts | 14 ++--------- .../polyfills/internal_binding/cares_wrap.ts | 15 ++++++++---- ext/node/polyfills/net.ts | 24 ++++++------------- tests/unit_node/http_test.ts | 8 +++++-- tests/unit_node/tls_test.ts | 12 +++++++--- 6 files changed, 38 insertions(+), 41 deletions(-) diff --git a/ext/node/polyfills/http.ts b/ext/node/polyfills/http.ts index f3f6f86ed8..349caeea69 100644 --- a/ext/node/polyfills/http.ts +++ b/ext/node/polyfills/http.ts @@ -50,6 +50,7 @@ import { urlToHttpOptions } from "ext:deno_node/internal/url.ts"; import { kEmptyObject } from "ext:deno_node/internal/util.mjs"; import { constants, TCP } from "ext:deno_node/internal_binding/tcp_wrap.ts"; import { notImplemented, warnNotImplemented } from "ext:deno_node/_utils.ts"; +import { isWindows } from "ext:deno_node/_util/os.ts"; import { connResetException, ERR_HTTP_HEADERS_SENT, @@ -1586,9 +1587,8 @@ export class ServerImpl extends EventEmitter { port = options.port | 0; } - // TODO(bnoordhuis) Node prefers [::] when host is omitted, - // we on the other hand default to 0.0.0.0. - let hostname = options.host ?? "0.0.0.0"; + // Use 0.0.0.0 for Windows, and [::] for other platforms. + let hostname = options.host ?? (isWindows ? "0.0.0.0" : "[::]"); if (hostname == "localhost") { hostname = "127.0.0.1"; } diff --git a/ext/node/polyfills/internal/dns/utils.ts b/ext/node/polyfills/internal/dns/utils.ts index 226fce93dd..1e0c3d9ed6 100644 --- a/ext/node/polyfills/internal/dns/utils.ts +++ b/ext/node/polyfills/internal/dns/utils.ts @@ -416,20 +416,10 @@ export function emitInvalidHostnameWarning(hostname: string) { ); } -let dnsOrder = getOptionValue("--dns-result-order") || "ipv4first"; +let dnsOrder = getOptionValue("--dns-result-order") || "verbatim"; export function getDefaultVerbatim() { - switch (dnsOrder) { - case "verbatim": { - return true; - } - case "ipv4first": { - return false; - } - default: { - return false; - } - } + return dnsOrder !== "ipv4first"; } /** diff --git a/ext/node/polyfills/internal_binding/cares_wrap.ts b/ext/node/polyfills/internal_binding/cares_wrap.ts index 6feb7faf0d..cbfea40b22 100644 --- a/ext/node/polyfills/internal_binding/cares_wrap.ts +++ b/ext/node/polyfills/internal_binding/cares_wrap.ts @@ -75,11 +75,18 @@ export function getaddrinfo( const recordTypes: ("A" | "AAAA")[] = []; - if (family === 0 || family === 4) { - recordTypes.push("A"); - } - if (family === 0 || family === 6) { + if (family === 6) { recordTypes.push("AAAA"); + } else if (family === 4) { + recordTypes.push("A"); + } else if (family === 0 && hostname === "localhost") { + // Ipv6 is preferred over Ipv4 for localhost + recordTypes.push("AAAA"); + recordTypes.push("A"); + } else if (family === 0) { + // Only get Ipv4 addresses for the other hostnames + // This simulates what `getaddrinfo` does when the family is not specified + recordTypes.push("A"); } (async () => { diff --git a/ext/node/polyfills/net.ts b/ext/node/polyfills/net.ts index 48e1d0de87..35d273be93 100644 --- a/ext/node/polyfills/net.ts +++ b/ext/node/polyfills/net.ts @@ -1871,23 +1871,13 @@ function _setupListenHandle( // Try to bind to the unspecified IPv6 address, see if IPv6 is available if (!address && typeof fd !== "number") { - // TODO(@bartlomieju): differs from Node which tries to bind to IPv6 first - // when no address is provided. - // - // Forcing IPv4 as a workaround for Deno not aligning with Node on - // implicit binding on Windows. - // - // REF: https://github.com/denoland/deno/issues/10762 - // rval = _createServerHandle(DEFAULT_IPV6_ADDR, port, 6, fd, flags); - - // if (typeof rval === "number") { - // rval = null; - address = DEFAULT_IPV4_ADDR; - addressType = 4; - // } else { - // address = DEFAULT_IPV6_ADDR; - // addressType = 6; - // } + if (isWindows) { + address = DEFAULT_IPV4_ADDR; + addressType = 4; + } else { + address = DEFAULT_IPV6_ADDR; + addressType = 6; + } } if (rval === null) { diff --git a/tests/unit_node/http_test.ts b/tests/unit_node/http_test.ts index f85b1466b5..f1ff77bb34 100644 --- a/tests/unit_node/http_test.ts +++ b/tests/unit_node/http_test.ts @@ -318,10 +318,14 @@ Deno.test("[node/http] IncomingRequest socket has remoteAddress + remotePort", a // deno-lint-ignore no-explicit-any const port = (server.address() as any).port; const res = await fetch( - `http://127.0.0.1:${port}/`, + `http://localhost:${port}/`, ); await res.arrayBuffer(); - assertEquals(remoteAddress, "127.0.0.1"); + if (Deno.build.os === "windows") { + assertEquals(remoteAddress, "127.0.0.1"); + } else { + assertEquals(remoteAddress, "::1"); + } assertEquals(typeof remotePort, "number"); server.close(() => resolve()); }); diff --git a/tests/unit_node/tls_test.ts b/tests/unit_node/tls_test.ts index 93eacd5ec2..847ec2dde3 100644 --- a/tests/unit_node/tls_test.ts +++ b/tests/unit_node/tls_test.ts @@ -32,13 +32,15 @@ for ( ) { Deno.test(`tls.connect sends correct ALPN: '${alpnServer}' + '${alpnClient}' = '${expected}'`, async () => { const listener = Deno.listenTls({ + hostname: "localhost", port: 0, key, cert, alpnProtocols: alpnServer, }); const outgoing = tls.connect({ - host: "localhost", + host: "::1", + servername: "localhost", port: listener.addr.port, ALPNProtocols: alpnClient, secureContext: { @@ -61,6 +63,7 @@ Deno.test("tls.connect makes tls connection", async () => { const ctl = new AbortController(); let port; const serve = Deno.serve({ + hostname: "localhost", port: 0, key, cert, @@ -71,7 +74,8 @@ Deno.test("tls.connect makes tls connection", async () => { await delay(200); const conn = tls.connect({ - host: "localhost", + host: "::1", + servername: "localhost", port, secureContext: { ca: rootCaCert, @@ -102,6 +106,7 @@ Deno.test("tls.connect mid-read tcp->tls upgrade", async () => { const { promise, resolve } = Promise.withResolvers(); const ctl = new AbortController(); const serve = Deno.serve({ + hostname: "localhost", port: 8443, key, cert, @@ -111,7 +116,8 @@ Deno.test("tls.connect mid-read tcp->tls upgrade", async () => { await delay(200); const conn = tls.connect({ - host: "localhost", + host: "::1", + servername: "localhost", port: 8443, secureContext: { ca: rootCaCert,