test(tls): bind the Bun.connect hostname-verify listener to 127.0.0.1 (host-dependent ECONNREFUSED) - #36543
Conversation
There was a problem hiding this comment.
LGTM — one-line test de-flake that binds the listener to 127.0.0.1 while the client still dials "localhost", so the hostname-verification assertion is unchanged.
What was reviewed:
- Confirmed the client's
hostname: "localhost"(mismatch) andserverName: "agent1"(match) paths are untouched, so the property under test is preserved. - Checked that
altnameMismatchReasonin the same file already bindsBun.listento127.0.0.1— this brings the sibling test in line. - The added comment documents the AI_ADDRCONFIG listen/connect asymmetry, which is the non-obvious "why".
Extended reasoning...
Overview
Single-line functional change in test/js/node/tls/node-tls-connect-hostname-verification.test.ts: the Bun.listen bind hostname goes from "localhost" to "127.0.0.1", plus a six-line comment explaining the host-dependent ECONNREFUSED. No production code is touched.
Security risks
None. Test-only; the change narrows a bind address to loopback and does not alter TLS validation, trust anchors, or any runtime path.
Level of scrutiny
Low. This is a targeted test de-flake with a well-understood root cause (listen resolves without AI_ADDRCONFIG, connect resolves with it, so on IPv6-loopback-only hosts the listener lands on ::1 while the client only tries 127.0.0.1). The PR cites #35160 as the same class of fix, and the altnameMismatchReason helper a few lines up in this file already uses hostname: "127.0.0.1" for its Bun.listen call, so the change also matches local convention.
Other factors
The repo's de-flake rule is "keep asserting the property the original assertion protected." That holds here: both Bun.connect calls still pass hostname: "localhost", so the cert-CN-vs-connected-hostname mismatch (and the serverName: "agent1" positive control) are exercised exactly as before. The added comment is durable "why" content rather than narration. No prior reviews or outstanding comments on the PR.
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
The 'reports authorized=false when a CA-trusted cert does not match the connected hostname' test fails with ECONNREFUSED on hosts where the only configured IPv6 address is loopback. Bun.listen resolves the bind host without AI_ADDRCONFIG and picks ::1, while Bun.connect resolves with AI_ADDRCONFIG which filters ::1 on such hosts, so the client dials 127.0.0.1 and is refused. Node's net.connect behaves the same way, so this is not a runtime divergence. Bind the listener to 127.0.0.1; the client still connects to the name 'localhost' so the hostname-derived certificate check this test covers is unchanged.
06a16b6 to
f581bdc
Compare
There was a problem hiding this comment.
LGTM — one-line test de-flake that binds the listener to 127.0.0.1 while keeping the client on "localhost", so the hostname-vs-CN assertion is preserved.
What was reviewed:
- Confirmed the test's invariant is unchanged: client still connects to
"localhost"(DNS-name path), cert isCN=agent1, soERR_TLS_CERT_ALTNAME_INVALIDis still exercised. - Matches the existing
altnameMismatchReasonhelper in the same file, which already binds to127.0.0.1;withServer(for thetls.connectcases) listens on all interfaces so is unaffected. - Jarred's inline question about the client hostname was answered and the thread is resolved.
Extended reasoning...
Overview
Single-line change in test/js/node/tls/node-tls-connect-hostname-verification.test.ts: the Bun.listen bind hostname moves from "localhost" to "127.0.0.1", plus a comment explaining why. The client-side Bun.connect({hostname: "localhost"}) is unchanged.
Security risks
None. Test-only change to a local listener bind address. No production code, no credentials, no TLS configuration semantics touched.
Level of scrutiny
Low. This is a host-environment de-flake with a well-understood root cause (AI_ADDRCONFIG on the connect side vs. AI_PASSIVE-only on the listen side, causing the listener to land on ::1 while the client only tries 127.0.0.1 on hosts with loopback-only IPv6). The PR description demonstrates Node behaves identically and cites prior art (#35160). The key thing to check is that the test still asserts what it was written to assert — it does, because certificate hostname verification is driven by the client's connect hostname ("localhost"), not the server's bind address.
Other factors
- The
altnameMismatchReasonhelper ~80 lines up in the same file already useshostname: "127.0.0.1"for itsBun.listen, so this change brings the twoBun.listensites into consistency rather than diverging. - The
tls.connecttests in the first describe block usewithServer, which callsserver.listen(0)with no host and therefore binds all interfaces — so they don't share this failure mode and correctly weren't touched. - Jarred asked whether the client hostname on line 218 should also change; the author explained it must stay
"localhost"to keep exercising the DNS-name/CN mismatch path (vs. the IP-SAN path), and the thread is marked resolved. - The added comment is durable (documents a non-obvious host-config interaction) and matches repo comment guidance.
What
Bun.connect TLS hostname verification > reports authorized=false when a CA-trusted cert does not match the connected hostnameintest/js/node/tls/node-tls-connect-hostname-verification.test.tsfails withECONNREFUSEDon hosts whose only configured IPv6 address is loopback (e.g. a container with::1 localhostin/etc/hostsand no global IPv6):Why this is not a runtime bug
Bun.listen({hostname: "localhost"})resolves viagetaddrinfowithAI_PASSIVEonly (noAI_ADDRCONFIG,bsd.c:bsd_create_listen_socket) and binds the first result,::1.Bun.connect({hostname: "localhost"})resolves via Bun's DNS layer, which passesAI_ADDRCONFIGon Unix (dns.rs:get_hints). On a host with no non-loopback IPv6 address,AI_ADDRCONFIGfilters::1, leaving only127.0.0.1. Happy-eyeballs runs but never sees::1.Node v26.3.0 behaves identically:
Fix
Bind the listener to
127.0.0.1. The client still connects to the name"localhost", so the assertion this test exists for (thatBun.connectverifies the presented certificate against itshostnameoption when noserverNameis given) is unchanged: the certificate is forCN=agent1, notlocalhost, and must remain unauthorized. Connecting to"localhost"reaches127.0.0.1on every host configuration that mapslocalhostto127.0.0.1at all.Same root cause as #35160 (different test).
Verification
On a host with
::1 localhostfirst and no global IPv6:Test-only change.
[stamp-90s] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file