Conversation
…ut brackets
A URL hands the NO_PROXY matcher the bracketed IPv6 hostname ("[::1]" for
http://[::1]/), but NO_PROXY lists conventionally hold the bare address
(NO_PROXY=localhost,127.0.0.1,::1), so fetch, WebSocket, bun install and
friends kept sending such requests through the proxy unless the entry was
spelled "[::1]". Strip the brackets from both the hostname and the entry
before comparing, so either spelling bypasses; "[::1]:port" entries still
match the host:port form. An address entry is only ever matched exactly,
never as a domain suffix.
The matcher was duplicated between bun_dotenv::Loader::is_no_proxy (env
backed callers) and bun_http::ProxySettings (fetch's captured copy, hop 0
and redirects); both now share bun_dotenv::no_proxy_matches.
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
|
Status: reproduced on the released binary (1.4.0, linux x64) with a proxy on 127.0.0.1 and an origin on ::1: The bare-address rows added in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Adds harness.hasIPv6Loopback(), a bind probe of ::1, and gates both new blocks on it. isIPv6() is pinned to false on BuildKite Linux even though loopback binds there, so it would skip the tests on the lanes that matter.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 3:40 AM PT - Aug 11th, 2026
✅ @robobun, your commit 12310cbbd2a6aa0841c4cc780046991d18fe81da passed in 🧪 To try this PR locally: bunx bun-pr 37429That installs a local version of the PR into your bun-37429 --bun |
There was a problem hiding this comment.
LGTM — the earlier IPv6-gating and comment-length feedback are both addressed, and no new issues found on re-review.
Checked that the consolidated no_proxy_matches preserves the old bun_http behavior for non-IPv6 hostnames (the IPv6 branch is only entered when strip_ipv6_brackets(hostname) is Some), and that is_no_proxy's host.unwrap_or(b"") is equivalent to the previous if let Some(h) = host guard. bun_http already depends on bun_dotenv, so no new crate edge. describe.concurrent.skipIf(...) and using _ = Bun.listen(...) in hasIPv6Loopback() both match existing test patterns.
Extended reasoning...
Overview
Folds the two duplicate NO_PROXY matchers (Loader::is_no_proxy in src/dotenv/env_loader.rs and the copy in src/http/lib.rs) into one exported bun_dotenv::no_proxy_matches, and teaches it to strip brackets from an IPv6 URL hostname so a bare ::1 entry matches http://[::1]/. Adds a hasIPv6Loopback() harness probe and IPv6-gated test blocks in proxy.test.ts and websocket-proxy.test.ts.
Security risks
NO_PROXY governs proxy bypass. The change is a strict superset for IPv6 targets (a bare-address entry that previously failed to match now matches), aligning with curl/Go/pip/Node's node:http. Nothing that previously went direct now goes through the proxy, and for non-IPv6 hostnames the domain-suffix branch is byte-identical to before. No new untrusted-input parsing beyond what was already present.
Level of scrutiny
Moderate. The matcher consolidation is a small, mechanical dedup with one new branch; the crate dependency (bun_http → bun_dotenv) already existed. I traced the non-IPv6 path against both removed implementations and it is unchanged; host.unwrap_or(b"") is observationally equivalent to the old if let Some(h) guard since the entry is never empty in the port branch.
Other factors
My prior review's IPv6-availability concern was addressed with hasIPv6Loopback() (bind probe of ::1, cached), which the author justified over isIPv6() — a reasonable trade-off since isIPv6() is hard-pinned to false on BuildKite Linux where the loopback does bind. The comment-cop feedback on long comments in env_loader.rs was addressed in 594d329. Test coverage is thorough (bare/bracketed/port/negative rows across env-proxy, explicit {proxy}, redirect re-resolve, and the WebSocket/env-loader path), and robobun confirmed the bare-address rows fail on the released binary.
bun info against a registry on [::1] exercises Loader::get_http_proxy_for, which fetch and WebSocket do not go through. The v4-mapped rows in proxy.test.ts give the address compare hex digits in both cases; they are skipped on Windows, whose sockets cannot reach a v4-mapped address.
There was a problem hiding this comment.
I re-reviewed after c3a25d7 / 594d329 / 12310cb and didn't find any further issues — the hasIPv6Loopback() bind probe is a better answer than the isIPv6() guard I'd suggested, and the three IPv6 blocks (proxy.test.ts, websocket-proxy.test.ts, bun-info.test.ts) are now all gated on it. Because this changes proxy-bypass semantics for NO_PROXY across fetch/WebSocket/install, a maintainer sign-off on the matching rules would still be worthwhile.
What was reviewed:
no_proxy_matches: bracket-stripping only fires when the URL hostname is itself a bracketed literal, so non-IPv6 targets keep the exact prior suffix path; degenerate entries ([::1,[]) fall through without over-matching.bun_http→bun_dotenvcall replaces a byte-for-byte duplicate;bun_httpalready depended onbun_dotenv, so no new layering.- New tests drain pipes concurrently, use
port: 0, clear ambient proxy env per case, and cover the negative rows (::2,[::1]:1,::1:<port>,1) so the matcher can't silently widen.
Extended reasoning...
Overview
Folds two copied NO_PROXY matchers (Loader::is_no_proxy in src/dotenv/env_loader.rs and no_proxy_matches in src/http/lib.rs) into one exported bun_dotenv::no_proxy_matches, and teaches it to strip [...] from both the URL hostname and the NO_PROXY entry when the target is an IPv6 literal. Adds hasIPv6Loopback() to test/harness.ts and new gated IPv6 test blocks in proxy.test.ts, websocket-proxy.test.ts, and bun-info.test.ts.
Security risks
NO_PROXY decides whether a request bypasses the configured proxy, so widening its match set is security-adjacent. The widening is narrowly scoped: it only applies when the URL hostname is a bracketed IPv6 literal, and only strips brackets from entries that are themselves bracketed — a bare entry with 2+ colons is already unambiguously an address. The domain-suffix branch is now explicitly skipped for IPv6 targets (previously it ran but could never match on [::1] for lack of a . boundary), so no domain-name entry can accidentally exempt an IPv6 host. Entries that matched before still match; the only new matches are bare-address ↔ bracketed-host, which curl/Go/Node/undici already accept.
Level of scrutiny
Medium-high. The core logic change is ~25 lines and mechanically sound, and the deduplication is exactly what REVIEW.md asks for ("one implementation, in the right place"). But proxy-bypass routing is the kind of behavior a maintainer should sign off on rather than a bot, and the PR also introduces a new harness helper whose interaction with the BuildKite-Linux isIPv6() pin is a judgment call the author made deliberately.
Other factors
My prior inline comment (unguarded ::1 bind in beforeAll) was addressed with a bind-probe helper rather than isIPv6(), with a reasoned explanation (isIPv6() is pinned false on BuildKite Linux where ::1 does bind, so it would skip the tests everywhere they matter). The comment-cop verbose-comment flags were addressed by trimming to single lines. All review threads are resolved. Test coverage spans all three entry points (fetch env-proxy + explicit option + redirect hop, WebSocket via Bun__isNoProxy, package-manager via get_http_proxy_for) with both positive and negative rows. CI build #92158 was still in progress at review time.
…40093) ### Problem - `test/js/bun/http/bun-server.test.ts` takes 9 to 16 s on every CI lane. The time is fixed waits, not work. - The GC stress test runs 400 rounds or 8 s, and since #37074 it tests nothing: a graceful `stop()` closes the idle parked connections, so every late request meets a closed socket (0 of 4770 dispatched on main). - Other fixed costs: 3 x 1000 ms CPU samples during the echo burst, 30-iteration GC loops with 10 ms sleeps, two 100 ms timers in the HEAD probe. ### Fix - The stress test targets node:http, whose `close()` leaves busy connections keep-alive afterwards, as in Node. Each round holds a request per parked connection across `close()`, drops the binding, runs `Bun.gc(true)`, and requires every late request to be answered by the original handler. With the downgrade gate removed it fails in round 1 (3 of 3). On main 162 of 162 late requests are answered. - Every other timer becomes the event it waited for (list in the notes). Top-level tests run concurrently where `test.concurrent(` fits on one line. - Stronger checks on full responses, `pendingRequests` around `stop()`, exact status lines, and piped `stdout`/`stderr`/`exitCode` for every fixture (list in the notes). - Verified, same machine and build: debug+ASAN 88.8 s to 25 s, release 10.4 s to 2.1 s, Windows x64 canary 11.1 s to 2.1 s. Pass set unchanged. ### Background - `Bun.gc(false)` is `Heap::collectSync`: collection only. `Bun.gc(true)` is `Heap::collectNow(Sync)`, which also sweeps, so dead cells with destructors are finalized inside the call. - The downgrade gate (#35130): the JS wrapper is the handlers' only GC root. `is_drained()` lets it go Weak only when no request, listener, websocket, or connection is left. Without it the wrapper is collected while connections are open and its finalizer closes them. - `heapStats().objectTypeCounts` counts prototypes with instances. `count - 1` while one instance is alive is the per-build floor the old 30-iteration drain produced. <details><summary>Notes</summary> Runs of this revision: debug+ASAN `bun bd test` 2 runs (25.5 s, 26.2 s), release `USE_SYSTEM_BUN=1` 5 runs (2.11 to 2.16 s), Windows x64 canary build of the same commit 4 runs (2.10 to 2.12 s, 79 pass). In this container three tests fail on main and on this branch alike (`rejected promise handled by error method`, `parse source map and fetch small stream`, `abrubtly close a upload request`): they bind `hostname: "localhost"`, which lands on `::1` here while the client dials `127.0.0.1`. #38818 fixes that in bun, so the fixtures stay as they are. The local runs unset `HTTP_PROXY`/`HTTPS_PROXY`: this container's egress proxy does not honor a bare `::1` in `NO_PROXY` for the new `http://[::1]:port/` request (#37429). CI has no proxy. Stress fixture evidence: - main's fixture on main (release, 400 rounds): answered 0, closed 4770. Debug build, 40 rounds: answered 0, closed 366. - PR fixture on main: answered 162 of 162, release (about 160 ms) and debug (5.8 s standalone). - gate removed (`is_drained()` returns `is_closed()`), debug build: `FAIL round 1: a parked connection closed`, 3 of 3. The finalizer's `app.close()` lands before the same round's late requests are answered. - bun 1.3.14 is not a control for the new fixture: its node:http `close()` closes busy connections too. - The first revision kept the Bun.serve fixture with `Bun.gc(true)` and 16 rounds (round-2 detection on 1.3.14, 60 of 60, versus round 15 to 153 with `Bun.gc(false)`). Review found the late requests unreachable on main, hence the retarget. Assertions added: `{status, text}` on plain responses, `pendingRequests` before and after `stop()` plus the exact status line in the drain fixtures (`HTTP/1.1 200 OK`, `HTTP/1.1 500 Internal Server Error`, `""` for the force-closed connection), the negotiated subprotocol and the close frame in the custom-protocol test, `AbortError` plus the exact byte count for the truncated upload, `redirected`/`url` on the redirect, `Completed: 10` from the unref fixture, and `{stdout, stderr, exitCode}` via `toEqual` for every spawned fixture. The nitro fixture runs with `PORT=0` instead of port 3000. Concurrency: six long-named tests (or tests with a timeout argument) stay sequential `test(` because prettier reflows `test.concurrent(` calls that exceed 120 columns and would reindent their bodies. The two `await server.stop()` tests also stay sequential: they assert that a fetch to the freed port fails. Timers replaced by events: the abort-signal stream tests await the server-side `abort`; the truncated upload half-closes once the server consumed the chunk; the HEAD probe sends `Connection: close` and reads to `end`; the CPU fixture samples 3 x 500 ms after the last echo; the heapStats loops poll `objectTypeCounts` with a 5 s deadline. `bun test` runs consecutive concurrent tests as one group (20 at a time, 5 under ASAN); a plain `test(` ends the group. Fixed waits removed: the `Bun.sleep(15)`/`Bun.sleep(10)` pairs in the two abort-signal stream tests, `setTimeout(..., 100)` in the upload test and in `doHead`, the `Bun.sleep(10)` in six GC loops, the 8 s floor of the stress test, the 1000 ms CPU windows, and the 30-iteration `drain(0)` baseline loops. The `Bun.sleep(15)` in "abort signal on server should only fire if aborted" stays: a negative check with no event to await, inside a concurrent block. `Bun.spawnSync` in two "Server" tests blocked the event loop of the concurrent block. Both use `Bun.spawn` now. `tryWritePending` kept the written prefix on a partial write (`data.slice(0, written)`). It keeps the unsent tail now. Release per-test after the change: the idle-CPU fixture (1.5 s) bounds the file. Everything else is under 0.2 s. No file under `src/` changes. The edited CPU fixture is referenced only by this test file. </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/bun/http/bun-server.test.ts <!-- robobun:evidence:end -->
|
Bare and bracketed IPv6 NO_PROXY entries match on main since #42692, which also folded the two matchers into one ( |
2 participants
Problem
With a proxy configured, a
NO_PROXYentry holding a bare IPv6 address does not exempt requests to that address. Only the bracketed spelling does:new WebSocket("ws://[::1]:port", { proxy })behaves the same way, and so does anything else that goes through the env loader (bun installagainst an IPv6 registry,bun upgrade,bun create).The bare form is the one people actually write:
NO_PROXY=localhost,127.0.0.1,::1is what curl, Go (docker and friends), pip and Node'snode:httpproxy support read, and it is the value typical proxied CI / dev containers ship with. Bun's ownnode:httpport already accepts it (it compares the unbracketed host), sohttp.get("http://[::1]/")bypassed the proxy whilefetch("http://[::1]/")in the same process did not.Cause
Both NO_PROXY matchers,
Loader::is_no_proxyinsrc/dotenv/env_loader.rs(WebSocket viaBun__isNoProxy, install, upgrade, create) and its copyno_proxy_matchesinsrc/http/lib.rs(fetch hop 0 and redirect hops, added in #33651), compare the hostname exactly as the URL parser hands it over. For an IPv6 literal that is the bracketed string[::1], so the entry only matches when it is written with brackets too.Fix
The two copies are folded into one
bun_dotenv::no_proxy_matches, whichLoader::is_no_proxyandbun_http::ProxySettings::resolveboth call. When the URL hostname is a bracketed IPv6 literal, the brackets are removed from it and from the entry before comparing, so::1and[::1]both matchhttp://[::1]/. Accepting both is a strict superset of what Bun accepted before (undici'sEnvHttpProxyAgentreads the bracketed form, so existing[::1]entries keep working) and adds the form the rest of the ecosystem reads; an entry with two or more colons, bracketed or not, cannot mean anything other than an IPv6 address, so nothing that previously matched changes meaning.Two deliberate details:
.-boundary suffix semantic for an address (curl does address matching only for IP targets as well). Previously the suffix rule was applied to the bracketed string; with a canonical IPv6 serialization it could never match, so no observable behavior is lost.[::1]:8080still matches the URL'shost([::1]:8080) as before. A bare entry never carries a port, because::1:8080is itself a valid IPv6 address; curl and Go read it the same way.Matching stays textual, as before and as in Node's
httpand undici: an entry has to use the same spelling as the URL host (::1forhttp://[::1]/, which is also what the WHATWG serialization produces for fetch and WebSocket;bun installcompares against the registry URL as written). Address-level comparison of alternative spellings such as0:0:0:0:0:0:0:1would be a separate change.Tests
All IPv6 blocks are gated on a new
harness.hasIPv6Loopback(), a bind probe of::1.isIPv6()was not used because it is pinned to false on BuildKite Linux even though the loopback binds there (bun-server.test.tsbinds::1unconditionally on every lane), so it would have skipped these tests on the lanes that matter.test/js/bun/http/proxy.test.ts, new block "NO_PROXY matches an IPv6 literal with or without brackets": a subprocess fetcheshttp://[::1]:<origin>/withhttp_proxypointing at a proxy that answers every request itself, so the response body says which route was taken. Rows:::1,localhost,127.0.0.1,::1,0.0.0.0,127.0.0.1, ::1,[::1],[::1]:<port>go direct;::2,[::1]:1,::1:<port>,1still go through the proxy. Three rows againsthttp://[::ffff:7f00:1]/(the v4-mapped loopback, reaching an origin on 127.0.0.1; skipped on Windows, whose sockets are IPv6-only by default) give the address compare hex digits in both cases:::FFFF:7F00:1and[::ffff:7f00:1]go direct,::ffff:7f00:2does not. Two more tests cover the explicitfetch(url, { proxy })option and a 302 from 127.0.0.1 intohttp://[::1]/(the redirect re-evaluation path inbun_http).test/js/web/websocket/websocket-proxy.test.ts, new "IPv6 literal target" rows in the existing NO_PROXY block:ws://[::1]:<port>with an explicit auth-requiring proxy andNO_PROXYset to::1,localhost,127.0.0.1,::1,[::1](connects) and::2(407 from the proxy). This is theBun__isNoProxyentry point into the env loader.test/cli/install/bun-info.test.ts, new block:bun infoagainst a registry on[::1]withhttp_proxypointing at a proxy that 404s everything, forNO_PROXY=::1,localhost,127.0.0.1,::1,[::1](prints the package name) and::2(fails with the proxy's 404). This is theLoader::get_http_proxy_forentry point, which install, audit, upgrade and create share and which neither fetch nor WebSocket goes through.On the released binary the bare-form rows fail (6 in proxy.test.ts, 2 in websocket-proxy.test.ts, 2 in bun-info.test.ts) and the bracketed and negative rows pass; with this change all three files pass in full.
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/bun/http/proxy.test.ts