http: proxy stress test suite + fix streamed upload and on_writable UAF through tunnel#32635
Merged
Conversation
…AF through tunnel Adds comprehensive stress testing of the HTTP client proxy code paths: 661 tests across 5 new files plus a shared adversarial proxy/origin helper. test/js/bun/http/proxy-stress-helpers.ts adversarial proxy + origin test/js/bun/http/proxy-stress-matrix.test.ts protocol x framing x encoding x body x keepalive test/js/bun/http/proxy-stress-lifecycle.test.ts RST/abort/close at every tunnel stage test/js/bun/http/proxy-stress-errors.test.ts CONNECT failures, auth, unreachable, TLS verify test/js/bun/http/proxy-stress-concurrent.test.ts pool churn + subprocess memory probes test/js/bun/http/proxy-stress-adversarial.test.ts stacked conditions + WebSocket proxy tunnel The adversarial proxy supports both CONNECT tunneling and absolute-form forwarding over a plain-TCP or TLS outer socket, with hooks to RST at any stage, split/trickle bytes, force CONNECT status codes, require auth, and track per-connection wire state. The adversarial origin shapes responses (content-length / chunked / close-delimited, all compression encodings) and can truncate at any byte offset. Two pre-existing bugs surfaced by the suite and fixed here in src/http/lib.rs: 1. Streamed request body (ReadableStream / async-iterator) through a CONNECT tunnel to an https origin hangs forever. The ProxyHeaders arm of on_writable computed has_sent_body from request_body().is_empty(), which is always true for HTTPRequestBody::Stream, so the request jumped to Done before the stream was ever signalled. Even with that fixed, write_to_stream_using_buffer wrote streamed body bytes to the outer socket instead of through the inner TLS session. Fixed by matching send_initial_request_payload's Bytes-only has_sent_body check and routing write_to_stream_using_buffer through ProxyTunnel::write when a tunnel is active. 2. heap-use-after-free in HTTPClient::on_writable when a TLS alert is buffered alongside the inner handshake ServerHello flight. on_handshake -> on_writable -> proxy.on_writable -> SSLWrapper::flush -> handle_reading processes the alert, fires on_close -> close_and_fail, which frees the ThreadlocalAsyncHTTP embedding *self via the result callback; control returns to on_writable and touches freed self. Fixed by checking socket.is_closed() after proxy.on_writable() before touching self again (same hazard already documented in start_proxy_handshake).
Collaborator
Author
|
Updated 4:10 PM PT - Jun 23rd, 2026
❌ @robobun, your commit 554db3e has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32635That installs a local version of the PR into your bun-32635 --bun |
Collaborator
Author
|
Independently reproduced and fixed the |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFixes the ChangesProxy tunnel fixes and stress test coverage
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Collaborator
Author
|
Re the duplicate-detector flag for #32636: that PR is a focused fix for bug 1 (streamed body through tunnel) only, with a single regression test added to |
Collaborator
Author
|
Overlaps with (and supersedes) #32636, which I've closed. The Branch |
…ects, 1xx, IPv6, HTTP/1.0) 102 additional tests: early server reply (413/400/500) mid-upload across all proxy/origin combos; 3-5 hop redirect chains mixing http and https; 512B-16KB response header values; 1xx informational before 200 through tunnel; IPv6 literal origin; many proxy.headers entries; proxy:'' force-direct; HTTP/1.0 origin; redirect:manual/error; response body consumed as text/arrayBuffer/bytes/blob/json. Also drop unused errcode import from proxy-stress-concurrent.test.ts.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Drop stale design-narration comments and redundant stageHit promise from the abort-at-each-stage block in proxy-stress-lifecycle.test.ts; unused imports in proxy-stress-concurrent/matrix were already cleaned by autofix.ci + the previous commit.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@src/http/lib.rs`:
- Around line 2723-2765: The proxy tunnel write path is missing socket state
validation that exists in the ProxyHeaders path. Before the first
ProxyTunnel::write call that writes from the buffer, add a guard check to return
an error if socket.is_closed() or socket.is_shutdown() is true. This prevents
inner TLS from buffering bytes indefinitely on a dead outer connection, similar
to the protection already implemented in the ProxyHeaders path. Reference the
socket validation pattern used elsewhere in the codebase to ensure consistency.
- Around line 3143-3148: The `original_request_body` match expression in the
`has_sent_body` calculation does not properly reject `Sendfile` requests before
proxy tunneling, allowing them to reach the `ProxyBody::Sendfile` handler which
unconditionally panics. Add a guard before proxy tunneling is enabled (similar
to how `can_offer_h2()` and `can_try_h3_alt_svc()` work) that returns a
recoverable error when `Sendfile` is encountered in a proxy tunnel scenario, or
modify the match expression to explicitly handle `HTTPRequestBody::Sendfile` and
reject it with an appropriate error instead of allowing it to proceed and panic.
In `@test/js/bun/http/proxy-stress-adversarial.test.ts`:
- Around line 322-331: The server is bound to the IPv4 address 127.0.0.1 on line
322, but the fetch request on line 330 uses localhost which can resolve to
either IPv4 or IPv6 depending on the system configuration, causing
inconsistency. Replace localhost in the fetch URL with 127.0.0.1 to match the
server binding address and ensure the fetch request uses the same host that the
server is listening on.
In `@test/js/bun/http/proxy-stress-concurrent.test.ts`:
- Around line 112-115: The current assertions in the proxyTls conditional block
and the HTTP-proxy rejectUnauthorized case (around lines 226-231) are too
lenient and do not enforce connection reuse properly. In the proxyTls case where
expect(proxy.connectCount()).toBeLessThanOrEqual(5) is used, tighten this
assertion to enforce actual connection reuse by reducing the maximum allowed
CONNECT count significantly below five. Similarly, in the rejectUnauthorized
test case around lines 229-231, tighten the assertion to properly enforce that
the strict tunnel is being reused rather than creating a new connection for each
request. The goal is to make these assertions fail if connection pooling
regresses.
- Around line 79-80: Remove the per-test timeout values from the test blocks in
the proxy-stress-concurrent.test.ts file. Specifically, remove the second
argument (30_000 and other timeout values) that specifies the timeout duration
for each test, as Bun already has built-in timeouts and setting explicit
per-test timeouts weakens hang detection. This applies to the test blocks at
lines 79-80, 183, 232, and 273-274 in the file, where you should remove the
timeout parameter while keeping the test name and callback function intact.
- Around line 413-414: The current assertion using toBeGreaterThanOrEqual is too
lenient and allows the test to pass even when all requests fail. Replace the
check on result.completed and result.failed with an exact equality assertion
that completed plus failed equals iterations, then add mode-specific validation
checks below it that verify the expected outcomes for each test mode (complete,
concurrent-32, redirect, and abort modes should each have distinct failure
expectations to ensure abort paths are properly exercised and other modes
maintain expected completion rates).
- Around line 333-350: The test does not actually verify that the parked tunnel
was evicted before making the second request. Before writing the stray bytes to
liveClients, capture close event promises from each client socket so you can
wait for them to complete. Assert that liveClients is not empty and that a
parked tunnel exists before attempting eviction. After writing the bytes, await
all the close promises to ensure the eviction has actually completed before
proceeding to the second fetch request. In the finally block cleanup, destroy
any remaining live sockets to ensure proper resource cleanup.
In `@test/js/bun/http/proxy-stress-errors.test.ts`:
- Around line 247-295: In the first test case, when proxyTls is true, the outer
HTTPS proxy handshake fails before sending the CONNECT request, causing the test
to pass for the wrong reason. Restrict the first test to only run with HTTP
proxies by adding a condition that skips it when proxyTls is true, or provide
separate tls configuration options for the proxy connection versus the inner
origin connection. In the second test case, the checkServerIdentity function
unconditionally rejects all hostnames including the proxy itself. Modify the
checkServerIdentity callback to conditionally return undefined (no error) when
the hostname matches the proxy, and return new Error("pinned") only when it
matches the origin hostname. Additionally, add assertions to both test cases to
verify that the proxy received at least one CONNECT request (using a method like
proxy.connectCount() or similar) to confirm the tunnel was actually attempted
before the inner TLS failure occurred.
In `@test/js/bun/http/proxy-stress-helpers.ts`:
- Around line 55-58: The restoreProxyEnv function currently sets
previously-absent proxy environment variables to empty strings, which leaves
stale empty values that can interfere with subsequent tests. Modify the function
to properly delete environment variables that were originally absent (when
saved[key] is undefined) by setting them to undefined or using delete, rather
than setting them to empty strings, so that undefined variables remain properly
deleted and do not cause cross-test pollution.
- Line 232: The upstream error handler (around line 379-383) and upstream close
handler (around line 385-391) are using socket.destroy() which closes the
connection immediately and can discard the 502 response before it's sent.
Replace the destroy() calls with client.end() to allow responses to flush
gracefully, following the same pattern already demonstrated elsewhere in the
file (see lines 250-251 and 407-412 where client.write() is followed by
client.end()). Additionally, add boolean flags to track whether the client
socket has already been closed, preventing race conditions where both the error
and close handlers might fire and attempt to close the same socket twice, which
causes the upstream refused to 502 error tests to be flaky.
In `@test/js/bun/http/proxy-stress-lifecycle.test.ts`:
- Around line 315-352: Replace the polling mechanism that checks the want
predicate on proxy.connections[0] (the poller async function using setImmediate)
with a stage callback approach. Add a non-mutating stage callback parameter to
the proxy helper that gets invoked when each proxy stage is reached, and drive
the abort from that callback by calling ac.abort() and signalStage() when the
target stage parameter is reached. This ensures the abort happens at the actual
observable proxy stage event rather than relying on polling a connection record
that may never flip to the expected state.
In `@test/js/bun/http/proxy-stress-matrix.test.ts`:
- Around line 229-232: The assertion `expect(chunks).toBeGreaterThanOrEqual(1)`
in the proxy stress matrix test is too weak because a value of 1 is already
guaranteed by the earlier check `got === payload.length`, so this doesn't
actually detect if the body was buffered into a single read as the comment
suggests it should. Change the assertion to `expect(chunks).toBeGreaterThan(1)`
to properly enforce that multiple chunks are received, which will correctly
validate that the stream is not pre-buffering the 128KB body before exposing it
to the stream consumer.
🪄 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: e79a45c0-f6fb-48fe-9cd8-0db0d893198c
📒 Files selected for processing (8)
src/http/lib.rstest/js/bun/http/proxy-stress-adversarial.test.tstest/js/bun/http/proxy-stress-concurrent.test.tstest/js/bun/http/proxy-stress-errors.test.tstest/js/bun/http/proxy-stress-helpers.tstest/js/bun/http/proxy-stress-lifecycle.test.tstest/js/bun/http/proxy-stress-matrix.test.tstest/js/bun/http/proxy-stress-memory-fixture.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/bun/http/proxy-stress-protocol.test.ts`:
- Around line 362-378: The test for 'proxy: "" forces direct' does not actually
set up an ambient proxy environment, so it passes regardless of whether proxy:
"" is being honored. To properly test that proxy: "" bypasses ambient proxies,
you need to configure the proxy URL created with createAdversarialProxy as the
ambient proxy in the environment before calling fetch, either by setting it as
an environment variable in an isolated subprocess context or using env isolation
to prevent race conditions with concurrent tests. Then keep the existing
assertion that proxy.connections.length equals 0 to prove the ambient proxy was
actually bypassed by the empty string proxy parameter.
- Around line 258-314: The test assertion on line 309 is too lenient and accepts
"resolved:502" as a valid outcome even though the beforeAll block (lines
262-273) has already confirmed that IPv6 loopback (::1) is available on this
host. Since ::1 is provably listenable, the proxy should successfully dial the
IPv6 origin and return a 200 response rather than failing with a 502. Tighten
the expectation in the contain() call to only accept the "200:v6" outcome, and
if the createAdversarialProxy helper is not properly handling bracketed IPv6
address format (like [::1]), normalize the IPv6 address format in that helper to
ensure correct connection handling.
🪄 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: 49039992-80a3-477c-9239-ab58b5f6fd44
📒 Files selected for processing (2)
test/js/bun/http/proxy-stress-lifecycle.test.tstest/js/bun/http/proxy-stress-protocol.test.ts
….test.ts src/http/lib.rs: guard write_to_stream_using_buffer's tunnel path on socket.is_closed()/is_shutdown(), matching the ProxyHeaders arm, so inner TLS writes don't buffer forever on a dead outer socket. proxy-stress-helpers.ts: strip IPv6 brackets before net.connect (getaddrinfo rejects '[::1]'); use client.end() instead of destroy() on upstream error/close so 502 replies flush; drop stale consumer list from the header comment. proxy-stress-protocol.test.ts: tighten IPv6 test to assert 200 now that the helper dials ::1 correctly; recast the proxy:'' test to document that an empty-string proxy option is currently treated as absent (fetch.rs get_length > 0 check), not as 'explicitly no proxy' per the FetchTasklet comment, so ambient env is not overridden. proxy-stress-errors.test.ts: assert connectCount() in the inner-TLS verification tests so an outer-proxy TLS failure can't mask them. proxy-stress-concurrent.test.ts: wait for the parked socket's close event in the idle-eviction test; tighten memory-probe assertions to exact iteration count + mode-specific completed/failed expectations; assert http-proxy reject_unauthorized reuse exactly. proxy-stress-matrix.test.ts: drop the vacuous chunks>=1 assertion. proxy-stress-adversarial.test.ts: fetch 127.0.0.1 to match the never-reply origin's bind address. proxy-stress-headers.test.ts: 76 new tests covering many response headers, duplicate Set-Cookie, Content-Type matrix, 206 Content-Range, decompress:false, CONNECT envelope shape, long URLs, proxy switching, and Content-Encoding after decompress.
Contributor
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/bun/http/proxy-stress-headers.test.ts`:
- Around line 289-306: The test "Content-Encoding header after decompress"
verifies the body is decompressed correctly but does not verify that the
Content-Encoding header is actually removed from the response, which contradicts
the stated behavior in the test comments. Add an assertion after the existing
body check that verifies the Content-Encoding header is absent by checking that
res.headers.get("content-encoding") returns a falsy value (null or undefined),
mirroring the inverse assertion pattern used in the decompress:false test case.
🪄 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: 8785cda4-127a-44bd-bdf5-6c7cc8d25103
📒 Files selected for processing (8)
src/http/lib.rstest/js/bun/http/proxy-stress-adversarial.test.tstest/js/bun/http/proxy-stress-concurrent.test.tstest/js/bun/http/proxy-stress-errors.test.tstest/js/bun/http/proxy-stress-headers.test.tstest/js/bun/http/proxy-stress-helpers.tstest/js/bun/http/proxy-stress-matrix.test.tstest/js/bun/http/proxy-stress-protocol.test.ts
💤 Files with no reviewable changes (1)
- test/js/bun/http/proxy-stress-matrix.test.ts
…ress Bun keeps the Content-Encoding header after transparent decompression (issue #5668). The comment claimed it was stripped; correct the comment and add the matching assertion so the tunnel path is verified to match direct fetch.
…r comments The file header listed folding, long header names, and User-Agent as covered, but none of those are tested in this file. write_proxy_connect does not send User-Agent on the CONNECT envelope, so drop that from the section comment as well.
- proxy-stress-helpers.ts: remove unused onConnection/forceUpstream options and absoluteFormCount() method (leftovers from abandoned designs) - proxy-stress-protocol.ts: gate IPv6 tests with describe.skipIf(!isIPv6()) instead of a bare early return, so hosts without ::1 report SKIPPED - proxy-stress-errors.ts: make the h2-not-offered test observe the actual inner-TLS ALPN result via a raw tls server that echoes socket.alpnProtocol - proxy-stress-adversarial.ts: tighten interleaved proxy/direct assertions (bytesUp unchanged across direct requests; http-proxy connectCount === 1)
…ertion
The separate { mode, growth } toEqual with expect.any(Number) could never
fail. Fold mode + the rounded ratio into the threshold check via a
withinBound:true key so they appear in the diff when the leak bound trips.
… stageReached - proxy-stress-adversarial.ts: the large binary frame round-trip test now rejects on a clean close before the echo arrives, matching the adjacent echo test. A clean server close fires onclose (not onerror), so without this the test hung to framework timeout on regression. - proxy-stress-lifecycle.ts: remove the write-only stageReached flag and its tautological assertion. await poller resolving is itself proof the stage was reached.
…m's handler SSLWrapper::write_data fires trigger_close_callback before returning a fatal Err, which runs proxy_tunnel::on_close -> close_and_fail and can synchronously free the HTTPClient via the result callback. Propagating that Err to write_to_stream's Err arm then dereferences freed memory. Match the pre-existing ProxyTunnel::write callers in on_writable: on a non-WantRead/WantWrite Err, return Ok(true) so write_to_stream only releases the independently-allocated stream buffer and returns without touching self. Also guard createAdversarialProxy's onData against re-entry between head parse and upstream connect, which could double-push a connection record and orphan the first dial when an absolute-form client sends body bytes in a separate packet before loopback connect completes.
…ally Matches the cleanup pattern used by every other inline-server test in the suite (duplicate Set-Cookie, h2 ALPN, 1xx/early-reply/HTTP1.0 origins). Moves the proxy creation inside the try so a failure there does not leak the listening origin.
Contributor
There was a problem hiding this comment.
All prior review feedback has been addressed and I found nothing new this round, but the src/http/lib.rs changes touch UAF-prone proxy-tunnel lifecycle paths (three separate liveness/Err-propagation fixes around ProxyTunnel::write / SSLWrapper callbacks) that warrant a human sign-off.
Extended reasoning...
Overview
This PR adds a 661-test proxy stress suite (9 new files under test/js/bun/http/, ~3,500 lines) and fixes three bugs in src/http/lib.rs (~100 lines): (1) streamed request bodies through a CONNECT tunnel were never sent because has_sent_body was computed from the empty Stream buffer and write_to_stream_using_buffer wrote plaintext to the outer socket instead of routing through ProxyTunnel::write; (2) a heap-use-after-free in on_writable when a TLS alert is buffered with the inner handshake flight (the socket.is_closed() guard after proxy.on_writable()); (3) a UAF found during this review where the new tunnel branch's fatal Err from ProxyTunnel::write propagated to write_to_stream's handler after trigger_close_callback had already freed *self — fixed by returning Ok(true) to match the sibling callers' pattern.
Security risks
None introduced. The changes are defensive (liveness guards, routing plaintext through the inner TLS instead of the outer socket). The test suite only spins up loopback servers with self-signed certs and does not touch auth, permissions, or untrusted input parsing in production code.
Level of scrutiny
High for the src/http/lib.rs changes. This is production HTTP-client code on the proxy-tunnel write path, dealing with raw pointer dereferencing (proxy_tunnel::raw_as_mut), SSL wrapper callback ordering, and the exact UAF class this PR is fixing — one of the three fixes was surfaced during review, which confirms the area is subtle. The fixes look correct and well-commented, mirror established patterns elsewhere in the file (the let Ok(...) else { return; } idiom at the other two ProxyTunnel::write call sites), and are backed by deterministic ASAN repros, but memory-safety changes of this kind should have a human reviewer verify the lifecycle reasoning.
The test files are lower risk (new coverage only, no behavior changes to existing tests) but are large enough that flakiness on non-Linux CI lanes is worth watching once they land.
Other factors
This PR has been through extensive iterative review: ~12 inline findings from me and ~14 from CodeRabbit, every one addressed or declined with a clear rationale by the author. The UAF guard at line 2918 was independently reproduced on a separate branch. The duplicate PR #32636 (bug 1 only) was closed in favor of this one. CI showed musl build failures on commit 68fa164 (build-bun, not test failures); the most recent commit 440b4de is test-only, so those may be infra-related — worth confirming the latest CI run is green before merge. The Sendfile-through-proxy panic flagged by CodeRabbit is pre-existing on main and intentionally out of scope here.
Collaborator
Author
CI statusThe diff is green on every proxy-related test across all lanes. Two consecutive builds each had a single unrelated failure on the same
Neither test uses All 32 review threads resolved; claude[bot]'s final review found nothing new and recommends human sign-off for the |
Comment on lines
+2715
to
+2781
| // When tunneling through a proxy the stream body must go through | ||
| // the inner TLS session (ProxyTunnel::write), not the outer socket: | ||
| // writing plaintext to `socket` here would interleave it with the | ||
| // tunnel's TLS records and corrupt the stream at the origin. | ||
| // WantRead/WantWrite from the inner SSL are treated as backpressure | ||
| // so the next onWritable retries the flush. Encrypted bytes reach | ||
| // the outer socket via the SSLWrapper's write_encrypted callback, | ||
| // same as the ProxyHeaders/ProxyBody::Bytes paths. | ||
| if let Some(proxy_ptr) = self.proxy_tunnel.as_ref().map(|p| p.as_ptr()) { | ||
| // Same guard as the ProxyHeaders arm: inner TLS writes succeed | ||
| // at the SSL layer and buffer on a dead outer socket forever. | ||
| if socket.is_closed() || socket.is_shutdown() { | ||
| return Err(err!(ConnectionClosed)); | ||
| } | ||
| let proxy = proxy_tunnel::raw_as_mut(proxy_ptr); | ||
| let to_send_len = buffer.slice().len(); | ||
| if to_send_len > 0 { | ||
| match ProxyTunnel::write(proxy, buffer.slice()) { | ||
| Ok(amount) => { | ||
| self.state.request_sent_len += amount; | ||
| buffer.cursor += amount; | ||
| if amount < to_send_len { | ||
| if !data.is_empty() { | ||
| let _ = buffer.write(data); | ||
| } | ||
| return Ok(true); | ||
| } | ||
| if buffer.is_empty() { | ||
| buffer.reset(); | ||
| } | ||
| } | ||
| Err(e) if e == err!(WantRead) || e == err!(WantWrite) => { | ||
| if !data.is_empty() { | ||
| let _ = buffer.write(data); | ||
| } | ||
| return Ok(true); | ||
| } | ||
| // A fatal SSL_write error fires trigger_close_callback | ||
| // before returning, which runs proxy on_close → | ||
| // close_and_fail and may have freed *self via the | ||
| // result callback. Match the other ProxyTunnel::write | ||
| // callers: bail without touching self. Ok(true) makes | ||
| // write_to_stream only release the (independently | ||
| // allocated) stream buffer and return. | ||
| Err(_) => return Ok(true), | ||
| } | ||
| } | ||
| if !data.is_empty() { | ||
| match ProxyTunnel::write(proxy, data) { | ||
| Ok(sent) => { | ||
| self.state.request_sent_len += sent; | ||
| if sent < data.len() { | ||
| let _ = buffer.write(&data[sent..]); | ||
| return Ok(true); | ||
| } | ||
| } | ||
| Err(e) if e == err!(WantRead) || e == err!(WantWrite) => { | ||
| let _ = buffer.write(data); | ||
| return Ok(true); | ||
| } | ||
| // See the matching arm above: on_close already ran. | ||
| Err(_) => return Ok(true), | ||
| } | ||
| } | ||
| return Ok(false); | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
Can you simplify this code? It's very repetitive. And the comment is mostly unnecessary.
Collaborator
Author
There was a problem hiding this comment.
Simplified in 554db3e. Collapsed both match blocks into let Ok(n) = ... else { ...; return Ok(true); } (all Err variants end up as backpressure anyway), dropped the redundant is_empty() guards, and cut the comments down to two short blocks. Net: 67 lines → 39.
Collapse the two match blocks into let-else form. All Err variants (WantRead, WantWrite, fatal) return Ok(true) as backpressure, so the distinction is unnecessary at this level. Trim the comments to the one line that matters for safety.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Comprehensive stress testing of the HTTP client proxy code paths (fetch and WebSocket, both
ProxyTunnelandWebSocketProxyTunnel): 661 tests across 5 new files plus a shared adversarial proxy/origin helper and a subprocess memory-probe fixture.proxy-stress-helpers.tsproxy-stress-matrix.test.ts{http,https}proxy ×{http,https}origin × framing × encoding × body-size × keepalive response matrix; upload matrix across string/Uint8Array/Blob/FormData/ReadableStream/async-iterator; streamed response viagetReader(); trickled 1-byte-per-tick downstream; split CONNECT envelope; RFC 9110 §9.3.6 ignored-header handling; hop-by-hop stripping; method matrix; cross-scheme redirects.proxy-stress-lifecycle.test.tsproxy-stress-errors.test.tsproxy-stress-concurrent.test.tsreject_unauthorizedpool gate (lax → strict forces fresh CONNECT); 4× concurrent 4MB echo; idle pooled tunnel receiving stray data is evicted; 12 subprocess memory probes (6 modes × 2 proxy schemes, 300-1200 iterations each, RSS-growth bounded).proxy-stress-adversarial.test.tscheckServerIdentityapprove/reject/GC-loop; path/query edges;verbose:true;AbortSignal.timeout; interleaved proxy/direct to same origin; CONNECT target shape; WebSocket proxy matrix (ws/wss × http/https proxy) with echo, 256KB binary, RST at CONNECT, 407 without auth, rapid close, wss-via-https-proxy open/close churn under GC.Full suite runs in ~5 min under debug+ASAN.
Bugs found and fixed
Two pre-existing bugs surfaced by the suite (both reproduce on main without any test-helper changes). Fixed here in
src/http/lib.rs.1. Streamed request body through a CONNECT tunnel never sent
fetch()to anhttps://origin through any proxy with aReadableStream/ async-iterator body hangs forever: CONNECT succeeds, inner TLS handshake completes, theTransfer-Encoding: chunkedrequest head is written, then nothing. String/Uint8Array/Blob/FormData bodies are fine;http://origins (absolute-form, no tunnel) are fine.Cause (two layers):
RequestStage::ProxyHeadersarm ofon_writablecomputeshas_sent_body = self.request_body().is_empty(). ForHTTPRequestBody::Streamthe bytes buffer is always empty here, so the request jumps straight toRequestStage::Doneand theis_streaming_request_bodysignal path a few lines below never runs. The non-proxysend_initial_request_payloadalready gates this onmatches!(original_request_body, HTTPRequestBody::Bytes(_)).ProxyBodyis reached, itsStreamcase callsflush_stream→write_to_stream_using_buffer→write_to_socket(socket, …), which writes plaintext chunked bytes to the outer proxy socket instead of through the inner TLS session. TheBytescase right next to it correctly routes throughProxyTunnel::write.Fix: mirror the non-proxy path's
Bytes-onlyhas_sent_bodycheck (and matchingdebug_assert) in theProxyHeadersarm; inwrite_to_stream_using_buffer, route throughProxyTunnel::writewhenself.proxy_tunnel.is_some(), treatingWantRead/WantWritefrom the inner SSL as backpressure. Encrypted output reaches the outer socket via the existingwrite_encryptedcallback, same as theBytespath. This also covers theHTTPThreaddrain loop which calls the sameflush_stream.2.
heap-use-after-freeinHTTPClient::on_writablewhen a TLS alert is buffered with the inner handshake flightIf the origin closes or sends a TLS alert in the same buffer as its ServerHello flight (origin rejecting client cert, corrupted stream via misbehaving proxy, origin crash), the client's
on_handshake → on_writable → proxy.on_writable → SSLWrapper::flush → handle_readingchain processes the alert, fireson_close → close_and_fail, which runs the result callback and frees theThreadlocalAsyncHTTPembedding*self. Control returns toon_writable, which immediately readsself.state.flags.is_waiting_for_cert_checkon freed memory. Same bug class already documented instart_proxy_handshake's comment.Fix:
close_and_fail→terminate_socketsynchronously marks the outer socket closed, and the socket handle is owned by the event loop (outlives the client). Checksocket.is_closed()immediately afterproxy.on_writable()and return before touchingself.Deterministic ASAN repro in
proxy-stress-lifecycle.test.ts("TLS alert in same buffer as inner handshake"): a CONNECT proxy that double-writes every client→upstream byte, making the origin's TLS stack abort with an alert. Looped 20× withASAN_OPTIONS=…:abort_on_error=1so the HTTP-thread UAF aborts the subprocess before the main thread's clean exit wins the race.Verification
Fail-before (src/ stashed,
bun bd):proxy-stress-matrix.test.ts -t "https-origin POST ReadableStream|https-origin POST async-iterator"→ 8 fail (hang/timeout)proxy-stress-lifecycle.test.ts -t "TLS alert in same buffer"→ 1 fail (subprocess aborts withheap-use-after-free)Existing
test/js/bun/http/proxy.test.ts(48 tests) still passes.Related
SSLWrappercallback chain.ProxyTunnelclose path but only in the.zigreference files.