Repository navigation
Conversation
|
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 (3)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughRedirect handling now preserves Host overrides and Unix-socket routing for same-origin redirects. Cross-origin redirects clear both states. HTTP/1.1 pooling rechecks keep-alive eligibility, and Unix-socket fetch tests cover the updated behavior. ChangesRedirect routing
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, fix, implementation details, policy choice, and verification results. It does not use the exact template headings, but it provides the required information and is complete enough for review. Comment |
|
Updated 3:34 PM PT - Aug 26th, 2026
❌ @robobun, your commit c560a63 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 38394That installs a local version of the PR into your bun-38394 --bun |
|
Status: ready for review. Rebased onto main (c560a63). CI on this head (build 106399) has two red tests, both also red on main's own build 106385 for the rebase base
Everything else is green or passed on retry, and the run before the rebase (build 106010) was 181 of 181 green. Reproduced on bun 1.4.0 and on a debug build of main: a unix server answering Proof is |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The refactor is clean and the ordering invariants (pool/close before the flag is consumed, state.reset() after) are preserved on both redirect paths. Because it changes redirect connection routing and credential-carrying behavior in the core HTTP client — and relies on the bitwise-copy ownership dance for unix_socket_path — a maintainer look would still be worthwhile.
What was reviewed
is_keep_alive_possible()folded back inline: still seesunix_socket_path(now forgotten only after the pool/close decision), so unix connections are still never pooled.redirect_is_cross_originis set unconditionally per hop and no longer cleared in the helper — verifiedstate.reset()runs right after in bothdo_redirectanddo_redirect_multiplexed, so no stale-flag carryover.- Cross-origin exit still forgets (not drops) the path; same-origin keeps it, which is safe since the HTTP-thread copy never owned it to begin with.
- Test precondition
server.url.origin !== originguards the/leavecase against the two origins accidentally coinciding.
Extended reasoning...
Overview
The PR fixes fetch(url, { unix }) so that a same-origin redirect is followed over the unix socket instead of dialing TCP to the URL's placeholder host:port. It touches src/http/lib.rs (both do_redirect and do_redirect_multiplexed), renames clear_hostname_on_redirect → redirect_is_cross_origin in src/http/InternalState.rs, extracts a shared leave_origin_for_cross_origin_redirect helper, and adds a comprehensive describe block plus strengthens the existing pooling test in test/js/web/fetch/fetch.unix.test.ts.
Security risks
This is itself a security fix: previously a same-origin redirect from a unix server would carry Authorization/Cookie to whatever TCP host:port the placeholder URL named (nothing was stripped because the origin matched). The new behavior keeps the hop on the socket for same-origin and, for cross-origin, still drops the socket path and still strips credentials via the existing cross-origin header strip — so a hop can only leave the socket without credentials. I checked that the is_same_origin computation and the credential-strip logic in handle_response_metadata are unchanged; the flag is now assigned unconditionally (= !is_same_origin) rather than only set-to-true, which is equivalent given state.reset() clears it between hops.
Level of scrutiny
High. This is the core HTTP client's redirect path, involves credential routing, and leans on a subtle memory-ownership pattern (the HTTP-thread HTTPClient is a ptr::read copy of the JS-thread original, so unix_socket_path must be ManuallyDrop-forgotten rather than dropped). The refactor moves that forget from unconditional-and-early to conditional-and-late; I traced that the pool/close decision in do_redirect still runs with the path present (so is_keep_alive_possible() still returns false for unix), and that do_redirect_multiplexed calls the helper before state.reset() discards the flag. The reasoning checks out, but this is exactly the kind of ordering-sensitive change a maintainer familiar with the HTTP-thread copy semantics should confirm.
Other factors
Test coverage is thorough (relative/absolute/chain/307-resend/placeholder/cross-origin-exit) with strong assertions on both what the unix and TCP servers actually received, including the authorization header value. The updated pooling test now also verifies a plain TCP fetch after the unix redirect reaches the TCP server. The PR description documents that the new tests fail on main and pass with the fix, and that adjacent redirect suites still pass. No prior human review on the PR yet.
|
Nothing to change from the review above. For the maintainer pass it asks for, the one policy choice in this PR is the cross-origin case:
curl ( |
715f8ef to
97305d7
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
With fetch(url, { unix }), do_redirect forgot the socket path before
dialing the next hop, so every followed redirect (including a relative
Location from the unix server) was dialed over TCP to the URL's host:port,
and since that hop is same-origin the Authorization header went with it.
Keep the socket path across same-origin hops; a cross-origin hop still
leaves the socket and dials the Location's authority, as before.
954c4eb to
c560a63
Compare
This branch has not been deployed
2 participants
Problem
fetch(url, { unix }): a redirect from the server on the unix socket is followed over TCP to the URL's host:port, even for a relativeLocation(Location: /v1/next). With the usual placeholder URL (http://localhost/...) the fetch fails withConnectionRefused.Authorizationmeant for the unix server goes to whatever the URL's host:port names. curl and undici follow such redirects over the socket.do_redirectanddo_redirect_multiplexed(src/http/lib.rs) clearunix_socket_pathbeforestart()dials the next hop. Added in Support unix domain sockets in fetch() #9097 so aLocationnaming another server is followed over TCP, it also fired for same-origin hops.Fix
unix_socket_pathand is dialed over the same socket. A cross-origin hop still clears it, dials the new authority over TCP, and still losesAuthorization/Cookie/Hostthrough the existing strip. So a hop can only leave the socket without the credentials.handle_response_metadataalready computesis_same_originfor the header strip. It now also records it in a new state flag,redirect_is_cross_origin, whichleave_unix_socket_for_cross_origin_redirectreads on both redirect paths, after the pool/close decision that reads the socket path.test/js/web/fetch/fetch.unix.test.ts, 6 of 15 tests fail without thesrc/httpchange, all pass with it. Alsofetch-redirect,fetch-url-after-redirect,fetch-keepalive, and the cross-origin redirect tests infetch.tls.test.tsandfetch.test.ts.Background
unixmakes fetch connect to a unix socket instead of resolving the URL's host. The URL only supplies theHostheader and path, and is what a relativeLocationresolves against.handle_response_metadataparses a 3xx, rewritesself.urland decides whether the hop is same-origin.do_redirectreleases or closes the 3xx connection and callsstart(), which connects for the new hop.HttpThread::connectdials the unix socket wheneverunix_socket_pathis non-empty, otherwise the proxy or the URL's host:port.Notes
redirects over a unix socketblock: relativeLocation, absoluteLocationon the same authority, a two-hop chain, a 307 resending its body, thehttp://localhost/placeholder form, and the cross-origin exit without credentials. The existing pooling test now also checks that a plain fetch of the URL's host:port after the redirected unix fetch reaches the TCP server.authorization=Bearer daemon-token, and the placeholder form getsConnectionRefused. A made-up hostname in the URL triggers a DNS lookup of that name.curl --unix-socket -Land undici'ssocketPathkeep every hop on the socket, including cross-origin ones. This PR keeps the Support unix domain sockets in fetch() #9097 behavior for cross-origin hops, which the existing "handle redirect to non-unix" test asserts. Switching to curl semantics is one line (drop the clear inleave_unix_socket_for_cross_origin_redirect) plus flipping that test and the "leaves the socket" test.is_keep_alive_possible()reads.clear_hostname_on_redirectflag and cleared theHostoverride in the same helper. fetch: never derive TLS SNI or certificate identity from the Host request header #35886 has since removed the Host override and that flag from main, so the flag here is new and the helper only touches the socket path. Earlier still,unix_socket_pathwas an ownedZigStringSliceshared bitwise with the JS-threadAsyncHTTP; main now stores a borrowed&'a [u8], so the clear is a plain assignment.HTTP_PROXY/HTTPS_PROXYset in the environment,fetch(url, { unix })writes a proxy-form request (orCONNECT) onto the unix socket.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