Parse IP literals strictly: 127.1, 0x7f000001 and ::1/64 are host names - #43979
Conversation
…_ip_address These three asked c-ares ares_inet_pton, which is inet_net_pton underneath. It also reads "127.1" (as 127.1.0.0), "10", "0x7f000001", zero-padded octets and a trailing "/bits" as an address, and it stops at a NUL. For an IPv6 address with "/bits" it writes only the prefix bytes, so "::1/64" and "2001:db8::1/0" both came out as "::". is_ip_address and is_ipv6_address now ask parse_strict, the core::net parser: a dotted quad or an IPv6 address and nothing else. to_ip_address asks it for IPv6 and keeps inet_aton for the IPv4 shorthand that the resolver reads. parse_strict returns early for an input above 45 bytes, the longest text of an address, and stays out of line. c-ares is left only as the inet_aton of Windows.
|
Status: ready for review. CI is running. How to reproduce on main (no network needed) // An IPv6 text with a prefix length is dialed as "::", and text after a space is ignored.
const v6 = Bun.listen({ hostname: "::1", port: 0, socket: { open: s => void s.end(), data() {} } });
const v4 = Bun.listen({ hostname: "127.0.0.1", port: 0, socket: { open: s => void s.end(), data() {} } });
for (const [hostname, port] of [["::1/64", v6.port], ["2001:db8::1/0", v6.port], ["127.0.0.1 db.example", v4.port]]) {
const result = await Bun.connect({ hostname, port, socket: { open: s => void s.end(), data() {} } }).then(
() => "connected",
e => e.code,
);
console.log(JSON.stringify(hostname), result);
}
v6.stop(true);
v4.stop(true);
On Windows main also dials 127.1.0.0 for The TLS side: Proof in the tree. With bun bd test test/js/bun/net/socket-dns-error.test.ts
bun bd test test/js/bun/udp/udp_socket.test.ts
bun bd test test/js/web/fetch/fetch.tls.test.ts
bun bd test test/js/web/fetch/fetch.tls.wildcard.test.ts
bun bd test test/js/bun/net/tls-reject-before-client-cert.test.ts
bun bd test test/js/bun/http/proxy.test.ts
bun bd test test/js/web/websocket/websocket-unix.test.ts
bun bd test test/bake/dev/esm.test.ts |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. WalkthroughIP address classification and conversion now use strict parsing in several paths, while non-Windows IPv4 conversion retains ChangesStrict IP Address Recognition
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Strict IP handling follows the stated scope: resolver shorthand remains platform-specific, while TLS treats non-strict forms as hostnames. No concrete blocker was established; normal checks should confirm the changed tests. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
The commit that added the attribute gives the reason: inlined into its ten callers the parser adds code at each of them.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the reordered to_ip_address (the strict v6 attempt now runs before the 512-byte guard, but parse_strict_v6 caps at 45 bytes, so no unbounded scan and empty input still returns None), the ::ffff:010.0.0.1 zero-padded mapped form (rejected by core::net, matching net.isIP), and the Windows-only ares_inet_pton declaration (its single caller sys::aton passes the NUL-terminated buffer, and the AF_INET = 2 value matches ws2def.h). A human look at the TLS/SNI and certificate-matching callers is still worthwhile given the intentional behavior change.
Extended reasoning...
The only source change is src/bun_core/ip_address.rs, which switches is_ip_address/is_ipv6_address and the IPv6 arm of to_ip_address from c-ares inet_net_pton to the core::net parser; the other eight files add tests. It touches security-sensitive surfaces: SNI selection, certificate identity matching in src/boringssl/lib.rs, and the Bake dev server Host-header check. Three confirmed findings are posted inline, so the change already needs a human; this note only records what else was examined and ruled out.
…nd in the dev server Host check On Windows to_ip_address asked c-ares for IPv4, which reads "127.1" as 127.1.0.0 and takes a trailing "/bits". The Windows resolver reads a dotted quad only, so Bun.connect dialed hosts that dns.lookup refuses. The Windows arm is now the strict parse, and c-ares is gone from the module. The dev server Host check accepts what the resolver reads as a number: an IP literal, or the inet_aton shorthand made of digits, dots, hex letters and x. So wget and Python at http://127.1:PORT keep working, and a host with a name after whitespace stays refused.
inet_aton stops at whitespace or a NUL and reads what comes before, so to_ip_address took "127.0.0.1 db.example" for 127.0.0.1. It now takes a host only when inet_aton read all of it. The dev server Host check asks to_ip_address and needs no check of its own. The new rows that expect ENOTFOUND are decided without a lookup. The rows that need the resolver of the platform moved to the UDP tests, where send() asks to_ip_address and throws at once.
Each host name has a byte that isspace() takes. On main inet_aton reads the IPv4 address before that byte, and Bun.connect dials it. The rows also check the syscall and the hostname of the error.
There was a problem hiding this comment.
I reviewed the latest push (the last three commits) and found no bugs; the points raised in earlier passes are addressed in the code. Because this changes TLS identity matching, SNI selection and the dev server's DNS-rebinding Host check, and has platform-gated behavior, a human look is still worthwhile before merging.
What was reviewed:
to_ip_addressIPv4 arm: the POSIX whitespace/NUL rejection afterinet_atonand the Windowsparse_strictfallback now give the same verdict asis_ip_addressfor the whitespace inputs the earlier thread named; the%zoneon the Windows fallback is harmless since no IPv4 text contains%.socket-dns-error.test.tsENOTFOUND rows: every row is now rejected locally byis_valid_hostname(whitespace,:,[are not hostname bytes), so none reaches the system resolver; the/bitsIPv4 rows moved to the UDP test wheresend()fails synchronously.is_allowed_host_header:host_without_portruns first, so127.1:3000reaches theto_ip_addressbranch as127.1; the leading-digit guard keeps names from ever hittinginet_aton.- Cfg parity:
Ipv4Addrimport andsysmodule are gated together with the POSIX arm, and both arms returnOption<IpAddr>, so the Windows branch should type-check.
Extended reasoning...
The change replaces c-ares ares_inet_pton with core::net's strict parser in src/bun_core/ip_address.rs for is_ip_address, is_ipv6_address and the v6 arm of to_ip_address, keeps BSD inet_aton for the IPv4 arm on POSIX with a whitespace/NUL rejection, uses strict parsing on Windows, and widens is_allowed_host_header in src/runtime/bake/DevServer.rs to accept resolver shorthand; eight test files gain rows. It touches security-sensitive surface: TLS certificate identity matching and SNI selection (via is_ip_address callers in http/proxy/websocket) and the dev server DNS-rebinding Host check. The commits since the last review addressed the earlier inline threads (DNS-dependent test rows, Windows IPv4 parity, whitespace consistency), and the bug hunt ran dry with no findings. Not approved outright because the change is intentionally breaking for TLS hostname verification of hex/zero-padded//bits hosts and has platform-divergent behavior that maintainers should weigh.
… IP host (#42766) ### Problem - `new Bun.SQL("postgres://u@[::1]:5432/db?sslmode=verify-full")` rejects a certificate that has `IP:::1` with `ERR_TLS_CERT_ALTNAME_INVALID`. `mysql://` does the same. - `parseOptions` copies `URL.hostname`, brackets kept, into `tls.serverName` (`src/js/internal/sql/shared.ts:2159`). Both adapters check the certificate against that text, and `"[::1]"` is not an IP address. - Both adapters send an IP literal as SNI (`127.0.0.1`). RFC 6066 section 3 forbids that. ### Fix - `SSLConfig::server_name_bytes()` (`src/sql_jsc/jsc.rs`) returns the name through `bun_core::ip_address::strip_ipv6_brackets`. The identity checks of both adapters read the name there. - New `SSLConfig::sni()` returns no name for an IP literal, bracketed or not. Both `adopt_tls` sites use it. libpq and fetch send none either. - Verified: `test/js/sql/sql-tls-ip-literal-host.test.ts` (new, 8 cases). 6 fail on main 0d73249, all pass here. ### Background - `sslmode=verify-full` checks that the certificate names the host. An IP host matches only an IP SAN entry, and only as a bare address (`check_x509_server_identity`, `src/boringssl/lib.rs:452`). - SNI is the host name a TLS client sends in its first message. `adopt_tls` sets it. - Considered a strip in `parseOptions` (the first version). That was a second copy of `strip_ipv6_brackets`, which fetch, RedisClient and Bun.connect use, and it changed `sql.options`. ### Downsides - Bun.SQL sends no SNI for an IP-literal name. A TLS proxy that routes on such a value loses it. - A zone-scoped address (`hostname: "::1%lo"`) still fails `verify-full`, and without brackets it still goes out as SNI: `is_ip_address` accepts no `%zone`. - Cost per TLS connection: one `is_ip_address` call (at most 45 bytes, no allocation) and a bracket check at each name read. <details><summary>Notes</summary> - History of this PR. The first version removed the brackets in `parseOptions` (JS). Main then gained `bun_core::ip_address::strip_ipv6_brackets` and moved every other client to it. A review noted that the JS helper disagreed with it (`[db]` lost its brackets too). aa4d699 moves the fix to the two native accessors and restores `parseOptions` to its state on main. So this PR no longer touches `src/js/internal/sql/shared.ts`, and it does not conflict with #42054 or #41761, which edit that block. - `sql.options.tls.serverName` and `sql.options.hostname` keep the brackets, as on main. Only the native reads see the bare address. - When this PR opened (canary 09bb546), Bun.SQL also accepted a certificate whose only SAN is `DNS:[::1]`, and a hostname mismatch gave an `Error` with an empty `code` and `message`. Main changed both since: #43873 makes a name that is not a hostname match no certificate name, and #43694 rejects the mismatch inside the handshake with `ERR_TLS_CERT_ALTNAME_INVALID`. This PR changes neither. - Probed on the debug build with a mock TLS server on 127.0.0.1 and an explicit `tls.serverName`. `"[::1]"` and `"::1"` under `verify-full`: connects, no SNI. `"[db]"` under `verify-full`: `ERR_TLS_CERT_ALTNAME_INVALID`, as on main. `"[fe80::1%lo]"` under `require`: no SNI. `"fe80::1%lo"` and `"127.1"` under `require`: sent as SNI, because `is_ip_address` takes neither as an IP literal (#43979). `"localhost"`: sent as SNI. - Observed on main 0d73249: peer SNI `127.0.0.1` for host `127.0.0.1`. With this change: none. - The cases that dial `[::1]` gate on `isIPv6()` (Buildkite Linux has no IPv6 loopback). The other cases run on every lane. One of them dials 127.0.0.1 with `tls.serverName: "[::1]"`, so every lane checks a bracketed name against the `IP:::1` entry. - Two gaps in the same `parseOptions` block are on main and stay out of this PR: `tls.servername` (the Node spelling) is ignored, which #42054 owns, and `tls: true` without an sslmode derives no `serverName`, which #26369 tracks. A `BunFile` given as `tls` with an explicit sslmode loses the file there, which #41761 owns. - Same bug class as #30668, which #30674 fixed for fetch and WebSocket. The dial path already removes the brackets (`src/uws_sys/socket.rs:791`). - Other suites run on the merged debug build (main 601af5a): `postgres-pgsslmode-env`, `sql-mysql-tls-plaintext-injection`, all of `adapter-env-var-precedence`, and `test/js/bun/net/tls-reject-before-client-cert.test.ts` (121 pass, 9 skip). I ran the new file 40 times on the earlier head: 320 of 320 cases pass. The container TLS suites (`tls-sql`, `local-sql`) need Docker and run in CI. </details> <!-- robobun:evidence:begin --> --- **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/sql/adapter-env-var-precedence.test.ts <!-- robobun:evidence:end -->
2 participants
Follow-up to #43873, found by code reading. Supersedes #39931.
Problem
bun_core::ip_addressparses with c-aresares_inet_pton, which isinet_net_pton. It also takes127.1,0x7f000001, zero-padded octets and a trailing/bits.Bun.connectandBun.udpSocketdial::for::1/64. On Windows they dial 127.1.0.0 for127.1, and elsewhere 127.0.0.1 for127.0.0.1 db.example.Bun.connect,RedisClientandBun.SQLverify0x7f000001against a certificate for 127.0.0.1. fetch sends no SNI for127.1.Fix
is_ip_address,is_ipv6_address, the IPv6 arm ofto_ip_addressand its IPv4 arm on Windows use thecore::netparser.inet_atonand takes a host only wheninet_atonread it all. The dev server Host check accepts that shorthand too.Background
is_ip_addressfor the callers that sql: strip IPv6 brackets from the TLS server name, send no SNI for an IP host #42766 and sql, redis: call tls.checkServerIdentity, honor tls.servername, send SNI from RedisClient #42054 add.Downsides
/bitsform no longer matches an IP address in a certificate, only the same text.Bun.connectanswersENOTFOUNDfor"127.0.0.1\n"and any host with text after whitespace. On Windows also for0x7f000001and127.0.0.1/32..text-1,024 bytes. Instructions per call fall, with two exceptions: IPv4 shorthand at the dev server (127.1: 471 to 1,391) and::1/64into_ip_address(520 to 538).Notes
What changes, by caller
is_ip_addresssrc/http/lib.rs,ProxyTunnel.rs,WebSocketUpgradeClient.rs,WebSocketProxyTunnel.rstls.connectsrc/boringssl/lib.rs(file not touched)0x7f000001matched IP:127.0.0.1bun installDNS prefetch,src/url/lib.rs08.1.1.1) is now prefetchedis_ip_addressandto_ip_addresssrc/runtime/bake/DevServer.rs(the one call site that changes)127.1), the rest is refused ([::1/64])to_ip_address, IPv6 armBun.connect,Bun.listen,Bun.udpSocket,is_valid_hostname,strip_ipv6_brackets(20 call sites)::1/64was the address::ENOTFOUND,Invalid address, brackets stayto_ip_address, IPv4 arm on Windows127.1was 127.1.0.0,127.0.0.1/32was an addressto_ip_address, IPv4 arm elsewhere127.0.0.1 db.examplewas 127.0.0.1, becauseinet_atonstops at whitespace or a NUL127.1stays 127.0.0.1is_ipv6_addressnormalize_dns_name, brackets of a displayed URL::1/64was IPv6is_valid_hostnamerejects it firstShorthand gets to the TLS callers from
tls.serverName, a proxy inHTTP_PROXYorHTTPS_PROXY, a tarball URL and awss+unix:URL host. Afetchorwss:URL host, theproxyoption and a registry URL go through the URL parser first, which writes127.1as127.0.0.1.is_ipv6_addressandto_ip_addressmust change together.do_lookup(src/runtime/dns_jsc/dns.rs) asksis_valid_hostnamefirst, and that asksto_ip_address. With a strictis_ipv6_addressalone,::1/64would stay on the c-ares backend, and c-ares reads it as a literal.Breaking changes, measured. Linux rows: release builds of one tree that differ only in the two source files. Windows rows: a debug build of this branch against the canary of main (29d9638), on Windows Server 2019 x64.
caset,tls.serverName0x7f000001,127.000.000.001,127.0.0.1/32,127.0.0.1/8,::1/128ERR_TLS_CERT_ALTNAME_INVALIDBun.connect,socket.upgradeTLS,RedisClient(rediss://0x7f000001),Bun.SQLsslmode=verify-fullERR_TLS_CERT_ALTNAME_INVALIDHTTP_PROXY=https://0x7f000001:PORTERR_TLS_CERT_ALTNAME_INVALID, SNI0x7f000001. On WindowsENOTFOUNDbun installofhttps://0x7f000001:PORT/pkg.tgzwith--cafileERR_TLS_CERT_ALTNAME_INVALID downloading tarball127.1against a certificate withDNS:127.1, orCN=127.1and no dNSName127.1,10against IP:127.0.0.1Bun.connectto::1/64,::1/0,2001:db8::1/0,[::1/64]::1ENOTFOUNDBun.udpSocketsend()to::1/64,2001:db8::1/0::1Invalid addressBun.connectto127.1ENOTFOUNDBun.connectto0x7f000001,127.000.000.001,127.0.0.1/32ENOTFOUNDBun.connectto1.2.3.4/8ENOTFOUNDdns.lookupof each of these, in Bun and in Node.js v26.3.0ENOTFOUNDENOTFOUNDBun.connectto127.1,0x7f000001,2130706433Bun.connectto127.0.0.1 db.allowed.example,"127.0.0.1\n", and the 7 other host names of #39931"12\t7.0.0.1": dials 0.0.0.12)ENOTFOUND, with no lookupBun.udpSocketsend()to127.0.0.1 rebound.example, or to127.0.0.1and a NUL and more textInvalid addressThe dev server Host check
Hostheader127.0.0.1,[::1],localhost127.1:PORT,0x7f000001,127.000.000.001(raw, wget 1.25.0, Python 3.13.5 urllib)[::1/64],[::ffff:127.1],1.2.3.4/8,08.1.1.12130706433,0.0.0.0x1(inet_atonreads them, c-ares did not)127.0.0.1 rebound-host.example(inet_atonstops at the space)rebound-host.exampleAn earlier commit of this branch made the check strict, which answered 403 to wget and Python at
http://127.1:PORT. That stopped no DNS rebinding request: a URL parser writes a dotted quad or rejects the host, so a browser cannot send these forms. The check now takes an IP literal, or a host that starts with a digit and thatto_ip_addressreads as IPv4. Over 25,792Hostvalues, 1,924 go from allowed to refused and 539 from refused to allowed. Each of the 539 has a number as its last label, so none is a DNS name. #40946 reports that the check cannot be configured, and #40733 addsdevelopment.allowedHosts.Measurements. Release builds on Linux, one tree, the two source files from main against this branch.
size -A,nm -S):.text58,301,116 to 58,300,092 bytes. File size 80,995,912 bytes in both.HTTPClient::on_open::<true>1,684 to 1,474 bytes,check_x509_server_identity1,481 to 1,336,parse_strict447 to 303,to_ip_address477 to 724,is_allowed_host_header579 to 468.gdbstepifrom entry to return, callees included, 3 equal runs per input):is_allowed_host_headerto_ip_addressregistry.npmjs.orglocalhost/example.comexample.com)127.0.0.1::1([::1]as a Host)127.10x7f000001::1/64(verdict changes)parse_strictinlined by LLVM (.text+1,024 bytes against main),parse_strictfor the IPv6 arm too (to_ip_address("localhost")386 to 404), the Host check withto_ip_addressfor every host (registry.npmjs.org596 to 895), and a check of the bytes in the Host check in place of the change into_ip_address.objdump):is_allowed_host_headersub $0x228,%rsptosub $0x18,%rsp,on_open::<true>0x278to0x68. Calls into c-ares per check 2 to 0. No allocator call before or after.is_ip_address, through SNI on the real binaries: 141 server names, 19 go from address to name, 0 go the other way. The result equalsnet.isIPwithout a zone on every name.wss+unix:: 28 of 28 shorthand cells change and equaltls.connect. 0 of 8 literal cells change.bun install(gdbbreakpoint count):localhost1 to 1,127.10 to 0,127.0.0.10 to 0,08.1.1.10 to 1.bun run rust:check-all: 12 of 12 targets, 0 errors.perf,valgrindandstraceare not on this machine.Tests. On Linux with
src/from main each of the 8 files fails, and only new rows fail in them (58). With this branch they pass:fetch.tls.test.ts56,fetch.tls.wildcard.test.ts158,proxy.test.ts96,websocket-unix.test.ts8,test/bake/dev/esm.test.ts18,socket-dns-error.test.ts29,udp_socket.test.ts230,tls-reject-before-client-cert.test.ts121. On Windows with this branch the same files pass (websocket-unix.test.tsis skipped there), and on the canary of main 10 of the 12 UDP rows fail. The 7 rows for #39931 came after the Windows run. No new row needs a resolver: a row that expectsENOTFOUNDhas a byte thatis_valid_hostnamerefuses, and the rows for/bitsand for the Windows shorthand are UDP rows, wheresend()throws at once. Same failure set on both Linux builds forresolve-dns.test.ts,node-dns.test.js,socket.test.ts,serve.test.tsandnode-dgram.test.js(they need the network or a user that is not root). The tarball case has no test: the download uses the HTTP client path that the fetch rows pin.Other open PRs
src/boringssl/lib.rstoparse_strict. After this PR that gives the same answer. Its hostname rule and its change intls.tsare still needed: 9 of its 11 IP rows pass on this branch alone, and the other 2 are zone rows. Its certificate grid and the one here cover the same names.to_ip_addressrefuse whitespace in the whole host. This PR has that change for IPv4, because the dev server Host check asksto_ip_addressand must refuse127.0.0.1 rebound-host.example. The check here runs afterinet_atonsucceeds, so a host name does not pay for it. The nine host names of its test are rows ofsocket-dns-error.test.tshere. One difference: Reject an IP literal that inet_aton does not consume whole #39931 also refuses whitespace in a%zone. This PR leaves the zone as it is on main, becausenode:dgramtakes any zone text:setMulticastInterface("::%lo junk")succeeds in Node.js v26.3.0 and on main, and throwsEINVALwith a check of the zone into_ip_address.inet_atonarm now does too, so those three hunks are covered for a host with no%zone. Its other hunks are still needed.is_ip_addressfor an SNI decision and get the strict answer with no change.Not in this PR
ares_inet_pton:net.SocketAddressandnet.BlockList(SocketAddress.rs),dns.setServers(dns.rs),node_cluster_binding.rs,c_ares.rs, andcanonicalize_ipinsrc/boringssl/lib.rs.new net.SocketAddress({ address: "::1/64" })succeeds, and Node.js throwsERR_INVALID_ADDRESS. List of breaking changes for 1.4 #28792 lists that as a breaking change, and dns: reject non-canonical IP strings in reverse(), lookupService() and setLocalAddress() #36185 covers thednsones.Bun.connectandupgradeTLS(socket_body.rs,tls_socket_functions.rs),Bun.SQL, HTTP/3 andnode:quic. They send127.0.0.1as SNI.[node:tls]Hangs on Google Cloud SQL PSC Connection Due to UnhandledFatal Alertin TLS handshake #20727 was this defect intls.connect.fe80::1%eth0, andnet.isIPsays 6 for it.Bun.connectdials::1for::1%eth0 junkwith scope 0, as libuv does for a zone that names no interface. Node.jsnet.connectanswersEAI_ADDRFAMILYfor it, becausenet.isIPsays 0. That check belongs to the caller, not toto_ip_address, whichnode:dgramshares.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/websocket/websocket-unix.test.ts, test/js/web/fetch/fetch.tls.test.ts, test/js/bun/udp/udp_socket.test.ts, test/js/bun/net/tls-reject-before-client-cert.test.ts, test/js/bun/net/socket-dns-error.test.ts, test/js/bun/http/proxy.test.ts, test/bake/dev/esm.test.ts