Fix TLS/HTTP2 connection failure with IP addresses#28717
Fix TLS/HTTP2 connection failure with IP addresses#28717
Conversation
When servername is '' or unset with an IP host, the serverName sent to the native TLS layer was incorrectly computed: 1. tls.ts used || which treats '' as falsy, falling through to the host IP. Per RFC 6066, IPs must not be sent as SNI. Changed to ?? with a net.isIP() guard so '' is preserved and IP hosts default to '' (no SNI). 2. net.ts lookupAndConnect returned early for IP hosts without setting self._host, causing a second buntls call in internalConnect to use undefined as host and override the correct serverName with 'localhost'. Fixes #28716
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
|
Updated 7:20 AM PT - Mar 31st, 2026
❌ @robobun, your commit 0d5daf1 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 28717That installs a local version of the PR into your bun-28717 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughSets the socket's internal host for IP short-circuits and changes TLS serverName fallbacks to treat IP hosts as empty server names; adds regression tests validating TLS/HTTP2 connections to an IP with empty or omitted servername. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28716.test.ts`:
- Around line 34-36: Move the exitCode assertion to the end of the first test so
stdout/stderr checks run first; specifically, in the block containing
expect(stderr).not.toContain("UNABLE_TO_VERIFY_LEAF_SIGNATURE"),
expect(exitCode).toBe(0), and const origins = JSON.parse(stdout.trim()),
relocate the expect(exitCode).toBe(0) after parsing and asserting stdout (i.e.,
after const origins = JSON.parse(stdout.trim()) and any related assertions) to
follow subprocess-test convention and improve failure diagnostics.
🪄 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: 22654877-be09-486b-87e6-18fe827af137
📒 Files selected for processing (3)
src/js/node/net.tssrc/js/node/tls.tstest/regression/issue/28716.test.ts
Problem
http2.connectandtls.connectfail withUNABLE_TO_VERIFY_LEAF_SIGNATUREwhen connecting to an IP address withservername: ''or without an explicitservername. Node.js handles both cases correctly.Root Cause
Two bugs:
tls.ts: The[buntls]method used||to computeserverName:||treats''as falsy, so an explicitservername: ''falls through to the host IP. Per RFC 6066, IP addresses must not be sent as SNI. Changed to??with anet.isIP()guard so''is preserved and IP hosts default to''(no SNI).net.ts:lookupAndConnectreturns early for IP hosts without settingself._host. A second[buntls]call ininternalConnectthen usesself._host(undefined), overriding the correctserverNamewith'localhost'.Fix
tls.ts:serverName: this.servername ?? (net.isIP(host) ? "" : (host || "localhost"))— preserves empty string, auto-detects IP hostsnet.ts: Setself._host = hostbefore the early return for IP addressesFixes #28716