fetch: strip IPv6 brackets before TLS cert verification#30674
Conversation
|
Updated 3:28 PM PT - May 14th, 2026
❌ @robobun, your commit 8400029 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30674That installs a local version of the PR into your bun-30674 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
WalkthroughThe PR fixes IPv6 hostname handling in TLS certificate verification by normalizing bracketed IPv6 addresses. The ChangesIPv6 TLS Hostname Normalization
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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/web/fetch/fetch.tls.test.ts`:
- Around line 129-138: The test currently asserts stdout, stderr and exitCode in
one object equality; change it to assert stdout and stderr first and then assert
exitCode last: after awaiting proc.stdout.text(), proc.stderr.text(), and
proc.exited into stdout, stderr, exitCode, replace the single expect({ stdout,
stderr, exitCode }).toEqual(...) with separate assertions such as
expect(stdout).toBe("native: Hello World\ncallback hostname: ::1\njs: Hello
World\n"), expect(stderr).toBe(""), and finally expect(exitCode).toBe(0) so
stdout/stderr failures are reported before the process exit code (refer to proc,
stdout, stderr, exitCode in this test).
🪄 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: 2c6c7201-9026-4e99-a9df-2efadb2b94ee
📒 Files selected for processing (2)
src/http/http.zigtest/js/web/fetch/fetch.tls.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
robobun
force-pushed
the
farm/1cba0bed/ipv6-tls-checkserveridentity
branch
from
40ea450 to
a524878
Compare
| /// Strips surrounding `[` `]` from an IPv6 literal, e.g. "[::1]" -> "::1". | ||
| /// Leaves non-bracketed hostnames unchanged. | ||
| fn strip_ipv6_brackets(host: &[u8]) -> &[u8] { | ||
| if host.len() >= 2 && host[0] == b'[' && host[host.len() - 1] == b']' { | ||
| &host[1..host.len() - 1] | ||
| } else { | ||
| host | ||
| } | ||
| client.url.hostname | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing, not introduced here — but ProxyTunnel::on_open (src/http/ProxyTunnel.rs:248) computes the inner-TLS SNI hostname via this.hostname.unwrap_or(this.url.hostname) instead of calling get_tls_hostname, so a proxied fetch('https://[::1]:PORT/') still passes "[::1]" to bun_core::is_ip_address (which returns false for bracketed input) and sends it as the SNI server_name. Cert verification on that path is fixed by this PR (it routes through check_server_identity → get_tls_hostname), so #30668's user-visible symptom is resolved either way — but if you want the "single chokepoint" claim to actually hold for SNI, swapping line 248 for get_tls_hostname(this, false) would also give the proxy-tunnel path port-stripping and tls_props.server_name precedence for free.
Extended reasoning...
What this is
The PR description says getTlsHostname is the "single chokepoint" so that "every fetch TLS consumer (native cert check, SNI's isIPAddress check, and the user-supplied checkServerIdentity callback) now sees the bare address ::1". For the direct (non-proxied) connection path that's true — get_tls_hostname is called at src/http/lib.rs:1592 and the result feeds the SNI is_ip_address gate. But the proxy-tunnel inner-TLS handshake has its own, older copy of the hostname-selection logic that bypasses the chokepoint.
src/http/ProxyTunnel.rs:248:
let _hostname = this.hostname.unwrap_or(this.url.hostname);
if bun_core::is_ip_address(_hostname) {
// null SNI
} else {
// NUL-terminate _hostname and pass to configure_http_client_with_alpn as SNI
}This does not call get_tls_hostname, so it picks up neither the existing strip_port_from_host nor the new strip_ipv6_brackets (nor the tls_props.server_name override).
Step-by-step
fetch('https://[::1]:9443/', { proxy: 'http://proxyhost:8080', tls: { ca, rejectUnauthorized: true } }).- Outer plain-HTTP connection to the proxy succeeds;
CONNECT [::1]:9443establishes the tunnel. ProxyTunnel::on_openfires to start the inner TLS handshake to the origin.this.url.hostname == "[::1]"(the URL parser keeps brackets, persrc/url/url.zig:429-432and the PR description itself).bun_core::is_ip_address("[::1]")→false. It callsares_inet_pton(AF_INET, ...)thenares_inet_pton(AF_INET6, ...)directly on the input (src/bun_core/lib.rs:2240-2251);inet_ptondoes not accept bracketed IPv6, so both fail.- Falls into the
elsebranch, NUL-terminates"[::1]", and passes it as theserver_nametoconfigure_http_client_with_alpn— an RFC 6066 §3 violation (literal IP addresses are not permitted in SNIHostName).
What is and isn't fixed on this path
- Cert verification (the actual Bug: tls.checkServerIdentity rejecting valid IPv6 hostnames in bracket #30668 symptom): fixed.
ProxyTunnel's identity check goes throughcheck_server_identity(..., allow_proxy_url=false)→get_tls_hostnameatsrc/http/lib.rs:1487, which now strips brackets. SoERR_TLS_CERT_ALTNAME_INVALIDis gone for proxied IPv6 origins too. - SNI emission: not fixed. Only
on_openis missed, because it duplicates the hostname-selection logic locally.
grep confirms get_tls_hostname has exactly two call sites (lib.rs:1487 cert check, lib.rs:1592 direct-path SNI); ProxyTunnel.rs:248 is a third hostname consumer that doesn't go through it.
Why file it at all (addressing the "no observable effect" objection)
It's true that most TLS servers ignore an unrecognized/garbage server_name rather than aborting the handshake, and that for an IP-literal origin the "correct" behaviour is to send no SNI anyway — so the practical blast radius is small. I'm flagging it as pre-existing, not a blocker, for three reasons:
- It's the same root cause this PR is fixing (bracketed IPv6 leaking into a TLS-hostname consumer that gates on
is_ip_address), in a directly adjacent code path — not an unrelated drive-by. - The PR description and the new doc comment on
get_tls_hostnameboth name "SNI'sis_ip_addresscheck" as a covered consumer; the proxy-tunnel SNI'sis_ip_addresscheck atProxyTunnel.rs:252is not covered, so the claim is slightly overstated. - The one-line fix has independent value beyond bracket-stripping: routing line 248 through
get_tls_hostname(this, false)would also give the proxy-tunnel inner handshakestrip_port_from_hostandtls_props.server_nameprecedence, which it currently lacks relative to the direct path atlib.rs:1592.
Suggested follow-up
// src/http/ProxyTunnel.rs:248
let _hostname = crate::get_tls_hostname(this, false);(make get_tls_hostname pub(crate) if it isn't already). That makes the chokepoint claim hold for all three fetch-TLS hostname consumers.
robobun
force-pushed
the
farm/1cba0bed/ipv6-tls-checkserveridentity
branch
from
bb69ec9 to
744b528
Compare
|
CI status: diff-green, lane-broken. The only failures across all lanes of https://buildkite.com/bun/bun/builds/54232 are on
These tests were added by the recent keepalive PRs (#30627 / #30640) and appear to be deterministically broken on this ASAN lane (same failure on unrelated PR #54218 / My |
The URL parser preserves the surrounding brackets on IPv6 hostnames
("[::1]"). When that form reached TLS certificate verification, two
paths broke:
- Native fast path: strings::is_ip_address("[::1]") returns false, so
check_x509_server_identity took the DNS-name branch and never compared
against the cert's IP SAN entries.
- User-supplied tls.checkServerIdentity callback: the hostname was
handed to the user verbatim; if the user then forwarded it to
node:tls.checkServerIdentity, net.isIP("[::1]") === 0 meant the same
DNS-name fallthrough and ERR_TLS_CERT_ALTNAME_INVALID.
Node.js strips brackets in urlToHttpOptions before either path runs.
Mirror that at the single chokepoint get_tls_hostname in src/http/lib.rs,
so every fetch TLS consumer (native cert check, SNI's is_ip_address
check, and the user-supplied checkServerIdentity callback) sees the
bare address.
Test gated on isIPv6() (matches valkey-tls-verify.test.ts) and placed
in its own file so the subprocess env doesn't race with other concurrent
tests in fetch.tls.test.ts. Asserts stdout/stderr before exitCode per
CLAUDE.md.
Fixes #30668
CI on f537883 hit an unrelated flake on the debian-13 x64-asan lane (test/js/web/fetch/fetch-tcp-keepalive.test.ts: "fetch sockets have TCP keepalive enabled" + "node:http globalAgent (keepAlive: true) enables SO_KEEPALIVE", both checking SO_KEEPALIVE via /proc/net/tcp). Same tests failed on unrelated PR #54218 at the same sha snapshot, confirming flake rather than regression — this PR doesn't touch TCP keepalive code.
robobun
force-pushed
the
farm/1cba0bed/ipv6-tls-checkserveridentity
branch
from
1384b14 to
8400029
Compare
|
Latest build https://buildkite.com/bun/bun/builds/54348 at sha 8400029 shows the same two failures ( |
1 participant
Fixes #30668
Repro
Cause
The URL parser keeps the surrounding
[/]on IPv6 hostnames (bracketsare part of
url.hostname). That form is fine for theHost:headerand DNS, but breaks TLS hostname verification on two paths:
check_x509_server_identitygates onstrings::is_ip_address(hostname).is_ip_address("[::1]")is false,so it takes the DNS-name branch and never compares against the cert's
IP:::1SAN. Result:ERR_TLS_CERT_ALTNAME_INVALID.checkServerIdentitycallback. The hostname washanded to the user verbatim. If the user forwarded it to
node:tls.checkServerIdentity, the implementation'snet.isIP("[::1]") === 0check likewise falls through to CN matchingand rejects.
Node.js strips brackets in
urlToHttpOptions(lib/internal/url.js)before the hostname reaches
tls.checkServerIdentity, so its owncheckServerIdentitynever needs to handle bracketed input.Fix
Mirror Node's strip-at-the-edge contract at the single chokepoint
get_tls_hostnameinsrc/http/lib.rs. Every fetch TLS consumer(native cert check, SNI's
is_ip_addresscheck, and the user-suppliedcheckServerIdentitycallback that the FetchTasklet invokes) now seesthe bare address
::1.Non-bracketed inputs are passed through unchanged, so DNS hostnames
and non-bracketed IPv4 literals are unaffected.
Verification
test/js/web/fetch/fetch.tls.test.tsexercises both paths end-to-end against
https://[::1]:port/with acert carrying
IP:::1SAN, and asserts that the user callbackreceives the bracket-stripped
"::1".ERR_TLS_CERT_ALTNAME_INVALID), passes with it.fetch.tls.test.ts,fetch.tls.wildcard.test.ts,node-tls-connect-hostname-verification.test.tsall still pass.