Repository navigation
Conversation
HttpThread::connect returned early for unix sockets before the custom SSL_CTX path ran, so ca/cert/key (which are baked into an SSL_CTX) were silently dropped while per-request options like rejectUnauthorized and servername continued to work. Dial unix through the custom SSL_CTX when one is needed, same as TCP.
|
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)
WalkthroughChangesUnix Socket TLS
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:24 AM PT - Jul 26th, 2026
❌ @robobun, your commit d18e65d has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35885That installs a local version of the PR into your bun-35885 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http/HTTPThread.rs (1)
501-517: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCache-hit proxy dispatch needs the same protocol guard as the other paths.
Theclient.http_proxybranch here should reject unsupported proxy schemes the same way the cache-miss and fallback paths do; otherwise cachedSSLConfigs can take a different connect path for the same request. Consider factoring the shared dispatch into one helper to keep the branches aligned.🤖 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 `@src/http/HTTPThread.rs` around lines 501 - 517, The cache-hit branch in the custom SSL context flow must validate proxy schemes before dispatching through client.http_proxy, matching the cache-miss and fallback paths. Update the dispatch around custom_ssl_context_map and ctx.connect to reuse the existing protocol guard or shared helper, while preserving Unix-socket and direct-connect behavior.
🤖 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.unix.test.ts`:
- Line 249: Replace the shared sockPath helper’s tmpdirSync usage with tempDir
from harness, declaring the disposable directly with using inside each affected
it() body and retaining it for the socket’s lifetime. Update all affected test
cases and remove the helper pattern that discards the tempDir resource.
- Around line 248-287: Wrap the “tls over unix socket” describe block containing
the `sockPath`, `validTls`, and mTLS tests in the existing Windows platform skip
mechanism, so neither test runs on Windows while behavior on supported platforms
remains unchanged.
---
Outside diff comments:
In `@src/http/HTTPThread.rs`:
- Around line 501-517: The cache-hit branch in the custom SSL context flow must
validate proxy schemes before dispatching through client.http_proxy, matching
the cache-miss and fallback paths. Update the dispatch around
custom_ssl_context_map and ctx.connect to reuse the existing protocol guard or
shared helper, while preserving Unix-socket and direct-connect behavior.
🪄 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: 56fe5e3c-01b3-42e2-90ab-f1b5f69cda2d
📒 Files selected for processing (2)
src/http/HTTPThread.rstest/js/web/fetch/fetch.unix.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Re the out-of-diff note on |
|
CI on d18e65d: the new
Ready for review. |
There was a problem hiding this comment.
LGTM — all prior feedback (redundant server.stop alongside using, tempDir vs tmpdirSync, and covering the SSL_CTX cache-hit arm) has been addressed and the threads are resolved.
What was reviewed:
- Traced every path through the reordered
HttpThread::connect: non-SSL+unix, SSL+unix without custom tls, SSL+unix with custom tls (cache hit and miss), and SSL+TCP/proxy — all preserve prior behavior except the intended fix. - Confirmed
init_with_client_configand the rest of the'custom_ctxblock do not touchclient.unix_socket_path, so the updatedRawSliceinvariant comment holds. - unix-path precedence over
http_proxyis preserved in both new arms, matching the default-context path.
Extended reasoning...
Overview
The PR reorders HttpThread::connect in src/http/HTTPThread.rs so the per-request custom SSL_CTX block ('custom_ctx) runs before the unix-socket dial, and adds a !unix_path.is_empty() arm to both the cache-hit and cache-miss branches so the unix socket is dialed through the custom context. The default-context unix dial moved from before 'custom_ctx to after it. Two new tests in test/js/web/fetch/fetch.unix.test.ts cover private-CA verification and mTLS over a unix socket, with the CA test looping twice to exercise the cache-hit arm.
Security risks
None introduced. The change is in the direction of enabling proper certificate verification and client-cert presentation over unix sockets — before this fix, users had to set rejectUnauthorized: false to make HTTPS-over-unix work at all with a private CA. rejectUnauthorized, servername, and checkServerIdentity are read off HTTPClient per request and are unaffected by which SSL_CTX dials the socket; only ca/cert/key (SSL_CTX-level) change behavior, and only on the unix+custom-tls path that previously ignored them.
Level of scrutiny
Moderate. HTTPThread::connect is on the hot path for every HTTP client request, but the change is a pure control-flow reorder with no new unsafe blocks, no new allocations, and no lifetime changes. I verified the RawSlice invariant (backing client.unix_socket_path is not mutated inside 'custom_ctx — init_with_client_config only reads tls_props), and traced each of the six connect paths (SSL/non-SSL × unix/proxy/TCP × custom-ctx/default) to confirm behavior is preserved everywhere except the targeted fix. unix-path precedence over http_proxy is retained in both new arms.
Other factors
All three prior review threads (two from me, one from CodeRabbit) are resolved: the redundant server.stop(true) alongside using is gone, tempDir replaced tmpdirSync in the new tests, and the CA test now loops twice — the author verified the second iteration is load-bearing (fails with ConnectionRefused when the cache-hit connect_socket arm is reverted). The author reported CI green on all lanes including Windows for 0b82aee with only known-flake failures unrelated to this diff; d18e65d only adds the second fetch iteration. No CODEOWNERS entry covers src/http. The pre-existing asymmetry in the cache-hit proxy branch (missing has_http_like_protocol() guard) predates this PR and the author correctly deferred it.
…uest header (#35886) ### Problem - `Host: whatever` on a request to `https://a.example/` fails with `ERR_TLS_CERT_ALTNAME_INVALID` because the server answers for the header's name (#26579). - Cause: `get_tls_hostname()` (`src/http/lib.rs`) preferred the Host header over `url.hostname` for SNI and the certificate hostname check. ### Fix - `get_tls_hostname()` resolves to `tls.servername` when set, otherwise `url.hostname`. The Host header still goes on the wire and no longer touches TLS. - Remove the plumbing that carried the header to TLS, including its term in the connection pool keys (list in Notes). The proxy-tunnel inner SNI uses `get_tls_hostname()` too. - Correct because the TLS identity is the authority the client dialled (RFC 6125 / RFC 9525). Node's `fetch` (undici) and curl do the same. `tls.servername` stays the opt-in for "dial an IP, verify a name". - Verified: `test/js/web/fetch/fetch.tls.test.ts` (three cases) and `test/js/bun/http/proxy.test.ts` (tunnel case) fail on stock bun and pass here. Neighbouring TLS, proxy, keep-alive and h2 suites pass (Notes). ### Breaking change - `fetch("https://10.0.0.5/", { headers: { Host: "internal.example" } })` against a certificate for `internal.example` used to pass verification and now fails with `ERR_TLS_CERT_ALTNAME_INVALID`. Migration: add `tls: { servername: "internal.example" }`. Node's `fetch` already fails this case, so the migrated code is also portable. ### Background - SNI is the hostname in the TLS ClientHello. A server with several certificates picks one by it. - Certificate verification compares the SAN entries to the name the client meant to reach. If SNI and that name differ, an SNI-routing server returns a certificate that cannot pass. - The keep-alive pool, h2 session registry and TLS session cache share one key. Requests with different Host headers now share one connection, as for plain HTTP. - #35889 changed only the verification target and kept the Host header as SNI. Against an SNI-routing server that fails every request. <details><summary>Notes</summary> Node's `fetch` deletes a caller-supplied Host header before dispatch (undici `lib/web/fetch/index.js`, `httpRequest.headersList.delete('host')`, present since undici 5). Bun keeps sending the header, which users rely on (#20173); only its effect on TLS changes here. Node's `https.request` is different: its agent derives `servername` from the Host header in JS (`calculateServerName`). Bun's `node:http` client is a socket-based port of that code and does not go through this HTTP client, so it is unaffected. Removed plumbing, in full: `HTTPClient.hostname`, `FetchOptions.hostname`, `FetchTasklet.hostname`, `async_http::Options.hostname`, the `init_sync` hostname parameter (15 call sites), `InternalStateFlags.clear_hostname_on_redirect` and its set/consume sites in `do_redirect`, `do_redirect_multiplexed` and `handle_response_metadata`, the Host branch of `proxy_auth_hash()`, the direct-TLS `proxy_auth_hash()` terms in `HTTPContext::connect`, `release_socket` and `session_cache::install` (now CONNECT tunnels only, which is what the hash covers), `h2::ClientSession.host_header_hash`, `h2::PendingConnect.host_header_hash`, `strip_port_from_host()`. Suites run with the fix: `fetch.tls`, `fetch-keepalive`, `fetch-http2-adversarial`, `fetch.tls.wildcard`, `fetch.unix`, `fetch-proxy-tls-intern-race`, `fetch-tls-abortsignal-timeout`, `regression/issue/27890`, `bun-serve-ssl`, `node/tls/ssl-ctx-cache`, all of `bun/http/proxy.test.ts`. `fetch.test.ts` has 41 failures in my container that reproduce on stock bun too (`localhost` binds to `::1` there); none touch this diff. Branch history: the first version of this PR (July) conflicted with main after the h2 `PeerVerification` and TLS session cache changes. This is the same change re-applied on current main, plus the tests from #35889 that fit this design (the URL-host verify cases and the `checkServerIdentity` hostname check) and a CONNECT tunnel case. #35889's unix-socket `SSL_CTX` fix is a separate bug and lives in #35885. </details> Fixes #26579
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-26, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
2 participants
What does this PR do?
fetch(url, { unix, tls })silently droppedtls.ca,tls.cert, andtls.keywhile still honoringtls.rejectUnauthorized,tls.servername, andtls.checkServerIdentity. The sametlsobject works correctly on a TCP fetch to the same server.Net effect: HTTPS over a unix socket could not use a private CA and could not do mTLS. A
cafor the correct root still failed withDEPTH_ZERO_SELF_SIGNED_CERT, and a client cert was never presented to arequestCertserver. The only way to make it work wasrejectUnauthorized: false.Cause
HttpThread::connecttook the unix-socket early return before the per-request customSSL_CTXpath ('custom_ctx) had a chance to run, so the unix connect always used the process-default HTTPS context.ca/cert/keyareSSL_CTX-level options baked into that per-request context;rejectUnauthorized/servername/checkServerIdentityare read per request offHTTPClientregardless of which context dialed the socket, which is why those kept working.Fix
When
tls_propsrequires a custom context, dial the unix socket through that context (both cache-hit and cache-miss arms), mirroring the TCP path. The default-context unix dial now runs only when no custom context is needed.Verification
New tests in
test/js/web/fetch/fetch.unix.test.tscover both the private-CA case and the mTLS case. Both fail on canary withDEPTH_ZERO_SELF_SIGNED_CERTand pass with this change.fetch.tls.test.ts,fetch-tls-cert.test.ts, andssl-ctx-cache.test.tsremain green.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/fetch/fetch.unix.test.ts