dns: reject '/' '*' and other non-LDH chars in dns.lookup hostname#33990
dns: reject '/' '*' and other non-LDH chars in dns.lookup hostname#33990robobun wants to merge 6 commits into
Conversation
c-ares validates lookup names against its record-name charset [A-Za-z0-9-*._/] (it keeps '/' for RFC 2317 CNAME targets and '*' for wildcard CNAMEs), so ares_getaddrinfo happily turns 'evil.tld/path' or 'a*b.tld' into a real wire QNAME. Node.js on glibc refuses these locally with EAI_NONAME because glibc getaddrinfo only accepts [A-Za-z0-9._-]. The gap meant Bun's dns.lookup (and net.connect / http.get on top of it) would resolve a DNS-illegal hostname, connect to whatever it returned, and emit the raw string as the Host header. Add a query-side charset guard in Resolver::do_lookup that rejects any ASCII byte outside [A-Za-z0-9._-] with ENOTFOUND before the name reaches a backend. Non-ASCII bytes pass through (IDN), and IPv6 literals are exempted via is_ip_address since they contain ':'. dns.resolve* is unchanged: Node.js routes those through c-ares too and accepts '/' and '*' there.
|
Status: fix verified locally, all review threads resolved. Fail-before ( CI: the diff is green on everything it touches. Build 71981's remaining red lane is |
|
Updated 6:09 PM PT - Jul 11th, 2026
❌ @robobun, your commit ea29847 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33990That installs a local version of the PR into your bun-33990 --bun |
WalkthroughDNS lookup now validates ASCII hostname characters before resolution while allowing underscores and IP literals. Linux-only tests verify local rejection, absence of DNS and HTTP leakage, valid control resolutions, and subprocess results. ChangesDNS lookup validation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/dns/dns-lookup-invalid-hostname.test.ts`:
- Around line 45-52: Extend the DNS invalid-hostname fixture with a non-ASCII
IDN hostname that should remain accepted, then update the assertions around the
test’s query results to verify that hostname alongside the existing valid
underscore and numeric-literal cases. Preserve the current rejection coverage
and ensure the expected unique query names include the non-ASCII case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f4441c1-594a-42c1-99b1-05324317721d
📒 Files selected for processing (4)
src/dns/lib.rssrc/runtime/dns_jsc/dns.rstest/js/node/dns/dns-lookup-invalid-hostname-fixture.tstest/js/node/dns/dns-lookup-invalid-hostname.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Asserting stderr is exactly empty before JSON.parse(stdout) hides the actual DNS result when a debug build emits a benign diagnostic. Check stderr together with exitCode after the semantic assertions so a failure diff shows both.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/dns/dns-lookup-invalid-hostname.test.ts (1)
1-9: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the rationale comment within the three-line limit.
This nine-line block violates the repository rule that code comments must be concise and no longer than three lines. Keep only the invariant being tested here; move the platform/backend explanation to the PR description or documentation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/node/dns/dns-lookup-invalid-hostname.test.ts` around lines 1 - 9, Shorten the introductory comment above the dns.lookup invalid-hostname test to at most three lines, retaining only the invariant that ASCII characters outside [A-Za-z0-9._-] must be rejected locally with ENOTFOUND. Remove the subprocess, platform, backend, and c-ares implementation rationale from this code comment.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/dns/dns-lookup-invalid-hostname.test.ts`:
- Around line 24-26: Update the subprocess handling around Promise.all so the
test asserts a successful exit using exitCode and includes stderr diagnostics
before calling JSON.parse on stdout. Keep the JSON parsing and subsequent
semantic assertions after this subprocess-result check.
---
Outside diff comments:
In `@test/js/node/dns/dns-lookup-invalid-hostname.test.ts`:
- Around line 1-9: Shorten the introductory comment above the dns.lookup
invalid-hostname test to at most three lines, retaining only the invariant that
ASCII characters outside [A-Za-z0-9._-] must be rejected locally with ENOTFOUND.
Remove the subprocess, platform, backend, and c-ares implementation rationale
from this code comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0a03e00b-b3e9-46a3-9030-946502fa08fe
📒 Files selected for processing (2)
src/dns/lib.rstest/js/node/dns/dns-lookup-invalid-hostname.test.ts
ares_inet_pton does not parse the RFC 4007 zone suffix, so
is_ip_address("fe80::1%lo") is false and the new guard rejected it
before the system/libc backend (which does parse the zone) could see it.
Strip a leading %zone before the IP-literal check. node:dns paths were
already unaffected (isIP() short-circuits on %zone in JS); this restores
direct Bun.dns.lookup on the non-c-ares backends.
The v4/v6 cases go through node:dns which short-circuits numeric literals
via isIP() before reaching Resolver::do_lookup, so they did not exercise
the is_ip_address exemption in the guard. Add a Bun.dns.lookup('::1')
case that does.
Use a trailing dot on the underscore control name so c-ares skips
resolv.conf search expansion regardless of the host's ndots setting; the
wire QNAME and the assertion are unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/dns_jsc/dns.rs`:
- Around line 5206-5214: In the validation condition surrounding the `%` scan,
replace the iterator-based position lookup with strings::index_of(name, b"%").
Preserve the existing split-at-index behavior and fallback to the full name when
no percent sign is found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9cfeae96-6499-41a3-b837-6621fa304956
📒 Files selected for processing (3)
src/runtime/dns_jsc/dns.rstest/js/node/dns/dns-lookup-invalid-hostname-fixture.tstest/js/node/dns/dns-lookup-invalid-hostname.test.ts
There was a problem hiding this comment.
No issues found in this revision — the scoped-IPv6 exemption, native IP-literal coverage, and ndots hermeticity from earlier rounds are all addressed. Deferring to a human because this tightens hostname validation on the dns.lookup path that node:net/node:http route through; it's a small change but a user-visible security-boundary shift worth a maintainer's sign-off.
Checked: is_valid_lookup_name against glibc's LDH+_ rule vs. c-ares's record-name charset; the %zone-strip → is_ip_address exemption for plain and scoped IPv6 (and that non-IP prefixes like evil.com%lo are still rejected); empty-name and %-only inputs fall through to the existing handling; dns.resolve* and the native fetch/Bun__addrinfo_* DNS path are untouched.
Extended reasoning...
Overview
Adds bun_dns::is_valid_lookup_name (accepts [A-Za-z0-9._-] + non-ASCII bytes) in src/dns/lib.rs and wires it into the existing length/NUL rejection guard in Resolver::do_lookup (src/runtime/dns_jsc/dns.rs), with an IP-literal exemption that strips a trailing RFC 4007 %zone before calling strings::is_ip_address. Two new test files under test/js/node/dns/ spin up a UDP DNS stub and an HTTP origin in a subprocess to prove invalid names are rejected before any packet leaves and that underscore names, IP literals, and scoped IPv6 still resolve.
Security risks
The change is security-positive in intent: it stops attacker-influenced /, *, space, etc. from becoming wire QNAMEs or Host: header values via mis-split URL strings, aligning Bun's c-ares path with glibc getaddrinfo's EAI_NONAME behavior that Node.js relies on. The risk surface is the tightening itself — any legitimate hostname pattern outside the accepted set would newly fail. I verified the predicate matches glibc's documented LDH+underscore rule, non-ASCII bytes pass through (so IDN handling is unchanged), IP literals are exempted including scoped IPv6, and dns.resolve* (where / and * are legitimate for RFC 2317 / wildcard records) is left alone. No injection, auth, or data-exposure risk introduced.
Level of scrutiny
High. Resolver::do_lookup sits under Bun.dns.lookup, node:dns lookup, net.connect, and the node:http/https client — every JS-initiated hostname resolution on the Node-compat surface flows through this guard. It does not touch the native fetch() / bun install DNS path (those go through Bun__addrinfo_*, not do_lookup), which bounds the blast radius, but it's still a behavior change on a critical, security-adjacent boundary. The IP-literal exemption logic already had one regression (scoped IPv6) caught and fixed during review, which reinforces that the edge cases here are subtle enough to merit a maintainer's eyes.
Other factors
Three prior review rounds (mine and CodeRabbit's) surfaced test-hygiene and correctness edge cases; the author addressed every one with clear rationale and follow-up commits. The test is hermetic (local UDP stub via dns.setServers, rooted control name to defeat ndots search expansion), Linux-gated where c-ares is the backend, exercises both the node:dns wrapper and the native Bun.dns.lookup path so each clause of the guard is load-bearing, and was verified fail-before/pass-after. The Rust change is ~20 lines and follows the local pattern (std slice methods, matching the pre-existing name.contains(&0) on the adjacent line). This is in good shape — I'm deferring on policy grounds (security-sensitive input validation, user-visible behavior change), not because of any open concern with the code.
1 participant
What
dns.lookup(and everything on top of it:net.connect, thenode:http/httpsclient) now rejects hostnames containing/,*, or any other ASCII byte outside[A-Za-z0-9._-]locally withENOTFOUND, before any packet is built. This matches Node.js on glibc.Why
c-ares validates lookup names against its record-name charset
[A-Za-z0-9-*._/](it keeps/for RFC 2317 reverse-delegation CNAMEs and*for wildcard CNAMEs). That charset is correct for record values but too lax for the query side: ahost/pathora*bstring passed toares_getaddrinfobecomes a real QNAME on the wire. glibcgetaddrinforefuses these withEAI_NONAMEbefore building any packet (the LDH rule plus_), which is what Node.js relies on.Before this change, on Linux (where c-ares is the
dns.lookupbackend):Bun 1.4.0:
Node.js v26:
(no packets sent)
A mis-split
evil.tld/pathURL string reachingdns.lookupused to flow attacker-controlled bytes into resolver logs, and under a wildcard zone would resolve, after which Bun connects and sends the application's headers withHost: evil.tld/path:PORT(not a valid RFC 9112uri-host, so downstream routers/caches can parse it divergently).How
Resolver::do_lookupalready rejects overlong and NUL-containing names; extend that guard withbun_dns::is_valid_lookup_name, which accepts[A-Za-z0-9._-]plus any non-ASCII byte (IDN passes through unchanged, as in glibc). IPv6 literals contain:so are exempted viastrings::is_ip_address. The check runs before backend dispatch, so it applies to the c-ares, libc, libinfo, and libuv paths alike.dns.resolve*is left alone: Node.js routes those through c-ares and accepts/and*there as well.Verification
no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/dns/dns-lookup-invalid-hostname.test.ts