Skip to content

fetch: reject the fetch when an HTTP proxy refuses CONNECT to an https origin - #35965

Open
robobun wants to merge 6 commits into
mainfrom
farm/09d8d79b/fetch-connect-reject-non-2xx
Open

fetch: reject the fetch when an HTTP proxy refuses CONNECT to an https origin#35965
robobun wants to merge 6 commits into
mainfrom
farm/09d8d79b/fetch-connect-reject-non-2xx

Conversation

@robobun

@robobun robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

What

fetch("https://...", { proxy }) now rejects when the proxy answers CONNECT with a non-2xx status, instead of resolving to a Response whose url is the https origin but whose status/headers/body came from the plaintext proxy hop. Any 2xx (not just 200) now establishes the tunnel, per RFC 9110.

Why

The CONNECT leg of an HTTP proxy is plaintext. When a proxy (or any MITM on the client→proxy hop) replies to CONNECT with e.g. 403, 407, 302, or even 201 Created, no TLS handshake to the origin has happened. Before this change Bun handed that reply to JS as a Response attributed to the https origin:

// proxy answers CONNECT with: HTTP/1.1 201 Created\r\nContent-Length: 8\r\n\r\nBODY201x
const res = await fetch("https://origin.invalid/never-fetched-over-tls", { proxy });
// res.status === 201, res.ok === true, res.url === "https://origin.invalid/...", await res.text() === "BODY201x"

A hostile proxy could inject Set-Cookie, Location, or HTML under the https origin's identity without a single verified TLS byte (the CVE-2009-2062 class). curl exits 56 (CONNECT tunnel failed, response 403); Node/undici throws TypeError: fetch failed; browsers report ERR_TUNNEL_CONNECTION_FAILED. None surface the body.

How

handle_response_metadata now dispatches on the CONNECT reply before the header loop and the status-code-driven state writes, so nothing from the CONNECT leg touches self.state (ProxyTunnel does not reset it between the CONNECT leg and the origin leg):

  • 2xxContinueStreaming (start the inner TLS handshake). RFC 9110 §9.3.6: "Any 2xx (Successful) response indicates that the sender ... will switch to tunnel mode immediately after the response header section." A proxy that sends 201 + body is non-conforming; those bytes feed the TLS handshake and it fails, which is what curl does.
  • non-2xxErr(ProxyConnectFailed(status)). Surfaces to JS as { code: "ProxyConnectFailed", message: "CONNECT tunnel failed, proxy responded with status <n>" } so 407/502 are still debuggable. The caller's close_and_fail tears the socket down.

Dispatching before the header loop also fixes a pre-existing leak where a proxy sending Content-Encoding: gzip on a 200 CONNECT reply left state.encoding = Gzip into the origin leg and fed the origin's identity body to the gzip decoder. The per-header Content-Length / Transfer-Encoding continue skips and the is_proxy_connect_failure redirect guard are removed as dead.

The WebSocket client's handle_proxy_response is widened to the same 200..300 range (and the byte-prefix fast path dropped) so new WebSocket("wss://...", { proxy }) agrees with fetch on the same RFC point.

Absolute-form proxying (http:// target through a proxy) is unchanged: there is no CONNECT and no origin TLS, so a proxy's 407/403 there remains a real Response.

bun install through a refusing proxy now reports ProxyConnectFailed downloading package manifest ... (and retries up to max_retry_count, since it is now a connect-level error) instead of GET <url> - 407. Skipping retry for ProxyConnectFailed(s) with s < 500, and surfacing the status through err.name(), are left for a follow-up rather than widening this PR into the install retry classifier.

Verification

Before / after
# before (bun 1.4.0-canary)
=== MODE=403 ===
RESOLVED 403 ok=false url=https://origin.invalid/never-fetched-over-tls {"content-length":"26","content-type":"text/html","set-cookie":"sess=INJECTED-BY-PROXY; Path=/"} "<h1>PROXY BODY UNDER TLS</"
=== MODE=201 ===
RESOLVED 201 ok=true url=https://origin.invalid/never-fetched-over-tls {"content-length":"8"} "BODY201x"

# after
=== MODE=403 ===
REJECTED Error ProxyConnectFailed CONNECT tunnel failed, proxy responded with status 403
=== MODE=201 ===
REJECTED Error ConnectionRefused Unable to connect. Is the computer able to access the url?

New describe("CONNECT response is never attributed to the https origin") in test/js/bun/http/proxy.test.ts covers 403/302/407/500 + the 201 spec-break case; proxy-stress-errors.test.ts adds 202/204/Content-Encoding: gzip/Transfer-Encoding: chunked CONNECT-reply state-leak cases that tunnel to a real https origin. All fail on main and pass with this change. Existing proxy-stress-errors.test.ts / proxy-stress-lifecycle.test.ts assertions that codified the old contract are updated to expect rejection.


[review] gate passed · iteration 3 · 7 files touched

fails on main (without fix)
ASAN without fix: 35 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/http/proxy-stress-errors.test.ts test/js/bun/http/proxy-stress-lifecycle.test.ts test/js/bun/http/proxy.test.ts
bun test v1.4.0 (9dd5bbe00)

test/js/bun/http/proxy-stress-errors.test.ts:
62 |           proxy: proxy.url,
63 |           keepalive: false,
64 |           tls: laxTls,
65 |           signal: AbortSignal.timeout(15_000),
66 |         }),
67 |       ).rejects.toMatchObject({
                     ^
error: expect(received).rejects.toMatchObject(expected)

Expected promise that rejects
Received promise that resolved: Promise { <resolved> }

      at <anonymous> (/workspace/bun/test/js/bun/http/proxy-stress-errors.test.ts:67:17)
      at <anonymous> (/workspace/bun/test/js/bun/http/proxy-stress-errors.test.ts:67:17)
      at <anonymous> (/workspace/bun/test/js/bun/http/proxy-stress-errors.test.ts:67:17)
(fail) CONNECT failure status > http-proxy CONNECT → 502 rejects the fetch [695.09ms]
62 |           proxy: proxy.url,
63 |           keepalive: false,
64 |           tls: laxTls,
65 |           signal: AbortSignal.ti
... (truncated)

release without fix: 2 skipped
bun test v1.4.0-canary.1 (5d4e1aee8)

test/js/bun/http/proxy-stress-errors.test.ts:
(pass) CONNECT failure status > http-proxy CONNECT → 201 is treated as tunnel-established [22.82ms]
(pass) CONNECT failure status > CONNECT → 302 with Location is not followed [30.27ms]
(pass) CONNECT failure status > CONNECT → 301 with Location is not followed [31.18ms]
(pass) CONNECT failure status > https-proxy CONNECT → 504 rejects the fetch [33.40ms]
(pass) CONNECT failure status > https-proxy CONNECT → 503 rejects the fetch [33.89ms]
(pass) CONNECT failure status > https-proxy CONNECT → 502 rejects the fetch [34.46ms]
(pass) CONNECT failure status > https-proxy CONNECT → 500 rejects the fetch [34.92ms]
(pass) CONNECT failure status > https-proxy CONNECT → 407 rejects the fetch [35.48ms]
(pass) CONNECT failure status > https-proxy CONNECT → 403 rejects the fetch [35.93ms]
(pass) CONNECT failure status > https-proxy CONNECT → 400 rejects the fetch [36.20ms]
(pass) CONNECT failure status > http-proxy CONNECT → 504 rejects the fetch [36.48ms]
(pass) CONNECT failure status > http-proxy CONNECT → 503 rejects the fetch [36.99ms]
(pass) CONNECT failure status > 
... (truncated)
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/http/proxy-stress-errors.test.ts test/js/bun/http/proxy-stress-lifecycle.test.ts test/js/bun/http/proxy.test.ts
bun test v1.4.0 (9dd5bbe00)

test/js/bun/http/proxy-stress-errors.test.ts:
(pass) CONNECT failure status > http-proxy CONNECT → 502 rejects the fetch [600.17ms]
(pass) CONNECT failure status > http-proxy CONNECT → 500 rejects the fetch [618.49ms]
(pass) CONNECT failure status > http-proxy CONNECT → 407 rejects the fetch [636.05ms]
(pass) CONNECT failure status > http-proxy CONNECT → 403 rejects the fetch [657.01ms]
(pass) CONNECT failure status > http-proxy CONNECT → 400 rejects the fetch [936.73ms]
(pass) CONNECT failure status > https-proxy CONNECT → 407 rejects the fetch [303.81ms]
(pass) CONNECT failure status > https-proxy CONNECT → 403 rejects the fetch [324.66ms]
(pass) CONNECT failure status > https-proxy CONNECT → 400 rejects the fetch [342.22ms]
(pass) CONNECT failure status > http-proxy CONNECT → 504 rejects the fetch [360.49ms]
(pass) CONNECT failure status > http-proxy CONNECT → 50
... (truncated)

release with fix: 2 skipped
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 780ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/6] gen generated_host_exports.rs
generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 240 extern-C blocks audited
[1/6] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m   Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m   Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m   Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys)
�[1m�[92m   Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety)
�[1m�[92m   Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys)
�[1m�[92m   Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys)
�[1m�[92m   Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd)
�[1m�[92m   Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp)
�[1m�[92m   Compiling�[0m bun_output v
... (truncated)
diff hotspot
src/http/error.rs                                  |   3 +
 src/http/lib.rs                                    |  66 +++-----
 .../websocket_client/WebSocketUpgradeClient.rs     |  29 +---
 src/runtime/webcore/fetch/FetchTasklet.rs          |   3 +
 test/js/bun/http/proxy-stress-errors.test.ts       | 175 ++++++++++++++++++---
 test/js/bun/http/proxy-stress-lifecycle.test.ts    |   9 +-
 test/js/bun/http/proxy.test.ts                     | 149 ++++++++++++++++--
 7 files changed, 321 insertions(+), 113 deletions(-)

gate history · 4 passed · 0 rejected · iteration 3

evidence per changed file
file                                                     reads  edits  tests
src/http/error.rs                                            2      4      0
src/http/lib.rs                                              8     12      0
src/http_jsc/websocket_client/WebSocketUpgradeClient.rs      1      2      0
src/runtime/webcore/fetch/FetchTasklet.rs                    2      1      0
test/js/bun/http/proxy-stress-errors.test.ts                 3      5      0
test/js/bun/http/proxy-stress-lifecycle.test.ts              1      1      0
test/js/bun/http/proxy.test.ts                               1      4      0

A non-2xx response to CONNECT travels over the plaintext client->proxy
hop with no TLS handshake to the https origin. Surfacing it as a
Response with res.url set to the https origin lets a hostile proxy (or
any MITM on that hop) hand JS attacker-controlled status/headers/body
(Set-Cookie, Location, HTML) under the https origin's identity, and a
non-200 2xx like 201 even passed res.ok.

RFC 9110 s9.3.6: any 2xx to CONNECT switches to tunnel mode (so 201+body
now feeds the inner TLS handshake, which fails, instead of resolving as
ok content); non-2xx now rejects with code 'ProxyConnectFailed' whose
message carries the proxy's status code. Matches curl, Node/undici, and
browsers.
@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 5 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c72bfcfd-f118-4749-8ebe-505ce524823a

📥 Commits

Reviewing files that changed from the base of the PR and between 44f6469 and 9dd5bbe.

📒 Files selected for processing (7)
  • src/http/error.rs
  • src/http/lib.rs
  • src/http_jsc/websocket_client/WebSocketUpgradeClient.rs
  • src/runtime/webcore/fetch/FetchTasklet.rs
  • test/js/bun/http/proxy-stress-errors.test.ts
  • test/js/bun/http/proxy-stress-lifecycle.test.ts
  • test/js/bun/http/proxy.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 2:05 PM PT - Jul 26th, 2026

@robobun, your commit 9dd5bbe has 1 failures in Build #82626 (All Failures):

  • 📦 Binary size — 12 over 0.50 MB
  • targetthis build canary: main #79916
    sizeΔ
    bun-darwin-aarch6458.15 MB57.58 MB+581.0 KB
    bun-darwin-x6463.48 MB62.95 MB+544.5 KB
    bun-linux-aarch6470.98 MB70.42 MB+576.0 KB
    bun-linux-x6472.48 MB71.95 MB+544.0 KB
    bun-linux-aarch64-musl64.88 MB64.32 MB+576.0 KB
    bun-linux-x64-musl66.98 MB66.45 MB+544.0 KB
    bun-linux-aarch64-android78.47 MB77.97 MB+512.0 KB
    bun-linux-x64-android80.62 MB80.10 MB+529.2 KB
    bun-freebsd-x6483.07 MB82.56 MB+528.0 KB
    bun-freebsd-aarch6484.84 MB84.31 MB+544.0 KB
    bun-windows-x6480.26 MB79.70 MB+574.5 KB
    bun-windows-aarch6470.87 MB70.34 MB+541.5 KB

    Add [skip size check] to the commit message if this increase is intentional.


🧪   To try this PR locally:

bunx bun-pr 35965

That installs a local version of the PR into your bun-35965 executable, so you can run:

bun-35965 --bun

Comment thread src/http/lib.rs Outdated
Comment thread src/http/error.rs Outdated
…ONNECT check above 204/304 content_length write

A 204 CONNECT reply would have written state.content_length = Some(0)
before the tunnel-established return, which leaks into the origin leg
(ProxyTunnel does not reset state) and rejects the origin's
Content-Length as a duplicate-CL conflict. Move the CONNECT dispatch
above the status-code-driven state writes; add 202/204-tunnel tests.
Comment thread src/http/lib.rs Outdated
Comment thread src/http/lib.rs Outdated
Comment thread src/http/lib.rs Outdated
Comment thread src/http/lib.rs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 src/http/lib.rs:4920-4940 — The b62c16c hoist put the CONNECT dispatch above the status-code-driven writes, but the header loop at 4770–4914 still runs first and its Content-Encoding (4812–4834), Connection (4874–4888), and Alt-Svc (4897–4910) arms write into self.state without the proxy_tunneling && proxy_tunnel.is_none() skip that Content-Length (4780) and Transfer-Encoding (4840) have — so a hostile proxy sending Content-Encoding: gzip on its 2xx CONNECT reply still leaks state.encoding into the origin leg (ProxyTunnel doesn't reset it → the origin's identity body is fed to the gzip decoder → CompressionFailed). This is pre-existing for the status==200 case, but it's the same class you just hoisted for, and hoisting the CONNECT dispatch above the header loop instead lets both per-header continue skips at 4780/4840 (and their comment-cop-flagged justifying comments) be deleted.

    Extended reasoning...

    What

    The b62c16c follow-up hoisted the proxy_tunneling && proxy_tunnel.is_none() dispatch (now lib.rs:4933–4940) above the status-code-driven state writes — pretend_304 and the 204/304 content_length = Some(0) block — so those can no longer leak from the CONNECT leg into the origin leg. But the header loop at lib.rs:4770–4914 still runs before the CONNECT dispatch, and three of its arms mutate self.state without the CONNECT-leg skip that Content-Length (4780) and Transfer-Encoding (4840) already have:

    • Content-Encoding (4812–4834): sets self.state.encoding = Gzip/Deflate/Brotli/Zstd and self.state.content_encoding_i.
    • Connection (4874–4888): a 2xx with Connection: close clears self.state.flags.allow_keepalive.
    • Alt-Svc (4897–4910): records against self.url.hostname — the origin's hostname.

    Nothing on the tunnel-transition path resets these: start_proxy_handshake doesn't touch state.encoding, and grep 'encoding' src/http/ProxyTunnel.rs has no matches (on_open/on_handshake reset only response_stage/request_stage/request_sent_len).

    Step-by-step: Content-Encoding leak

    1. Client sends CONNECT origin:443 HTTP/1.1.
    2. A hostile proxy replies HTTP/1.1 200 OK\r\nContent-Encoding: gzip\r\n\r\n.
    3. handle_response_metadata runs: the header loop reaches the Content-Encoding arm at 4812 (no CONNECT skip) → self.state.encoding = Gzip, content_encoding_i = i.
    4. Line 4933 matches (proxy_tunneling && proxy_tunnel.is_none()), line 4934 matches (2xx) → return Ok(ContinueStreaming). state.encoding is still Gzip.
    5. Inner TLS handshake completes; ProxyTunnel::on_handshake resets stages but not state.encoding.
    6. Origin replies HTTP/1.1 200 OK\r\nContent-Length: N\r\n\r\n<plain body> (no Content-Encoding header, so the arm at 4812 never fires to overwrite the stale value).
    7. handle_response_body sees state.encoding.is_compressed() → feeds the origin's identity bytes to the gzip decoder → CompressionFailed, and content_encoding_i points at a header index in the origin's header list that isn't a Content-Encoding header.

    The Connection leak is benign (keepalive suppression on a tunnel that isn't pooled the same way). The Alt-Svc leak is gated on self.is_https(), which at lib.rs:2686–2691 returns proxy.is_https() when http_proxy is set — so an http:// proxy can't hit it, but an https:// proxy's CONNECT-reply Alt-Svc would be recorded against the origin's hostname (further gated behind the experimental h3_alt_svc_enabled() flag).

    Why raise it here even though it's pre-existing

    For the status==200 case this leak already existed before this PR — the header loop ran before the old status == 200 check too — so this is not a regression the PR introduces. But:

    • REVIEW.md, Correctness: "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep)". The class here is "CONNECT-leg state writes leak into the origin leg because ProxyTunnel doesn't reset state", and b62c16c already fixed the status-code-driven half of it in response to review.
    • The comment-cop bot has already flagged both per-header skip comments (line 4779 and the block above 4932) with "if you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code". Hoisting above the header loop is that fix.
    • The comment at 4929–4932 is carefully scoped to "the status-code-driven state writes below", so it isn't strictly wrong — but the header-driven writes above it are the same hazard and the reader is left to infer that they're handled ad-hoc per-arm rather than structurally.

    Fix

    Hoist the if self.flags.proxy_tunneling && self.proxy_tunnel.is_none() { … } block from line 4933 to immediately before the header loop (after computing response's status but before the for (header_i, header) in … at 4770). Every output of the header loop — location, pretend_304, is_server_sent_events, and every self.state write — is irrelevant to a CONNECT reply (2xx → tunnel, non-2xx → Err), so nothing is lost. The verbose print_response at 4916–4918 can either move above the dispatch too or stay where it is (CONNECT replies then wouldn't be printed, which is arguably fine). Then delete the two per-header continue skips at 4780 and 4840 and their justifying comments — net simplification.

    Severity

    Nit. Pre-existing for the only status any conforming proxy sends (200); requires a hostile or non-conforming proxy to send unusual headers on a 2xx CONNECT reply; concrete impact is DoS (CompressionFailed) or keepalive suppression rather than the origin-attribution attack this PR fixes; the Alt-Svc case is https-proxy-only and behind an experimental flag. Worth fixing here because it's the same class the author just did one round of hoisting for, the fix is a net simplification, and it resolves the two open comment-cop findings.

Comment thread src/http/lib.rs Outdated
The header loop's Content-Encoding / Connection / Alt-Svc arms wrote
self.state before the CONNECT check ran, so a hostile proxy sending
Content-Encoding: gzip on its 2xx CONNECT reply leaked state.encoding
into the origin leg and the origin's identity body was fed to the gzip
decoder (pre-existing for status==200). Dispatching before the loop
also deletes the per-header CL/TE skips.
Comment thread src/http/lib.rs
@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the header-loop state-leak finding in 5d4e1ae: hoisted the CONNECT dispatch above the header loop so none of the CONNECT reply's headers touch self.state. That also deletes the per-header CL/TE continue skips. Added tests for 200 + Content-Encoding: gzip and 200 + Transfer-Encoding: chunked on the CONNECT reply alongside the 202/204 cases; the gzip one fails on main with ZlibError (pre-existing for status==200) and passes with this change.

The bun install retry-on-4xx-CONNECT point is noted in the PR body and left for a follow-up together with surfacing the status code through err.name(); it's a bounded UX regression on an already-failing config path and I'd rather not widen this security fix into the install retry classifier.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 src/http/lib.rs:4915-4924 — The b62c16c hoist puts the CONNECT dispatch above the status-code-driven state writes, but the header loop above it still writes self.state for a 2xx CONNECT reply: the Content-Encoding arm (4807-4829) sets state.encoding and the Connection arm (4869-4883) sets state.flags.allow_keepalive, neither with the proxy_tunneling && proxy_tunnel.is_none() skip that CL (4775) and TE (4835) have — so a proxy answering CONNECT with e.g. Content-Encoding: gzip leaves state.encoding = Gzip into the origin leg and the origin's plaintext body is fed to the gzip decoder. This is pre-existing for CONNECT → 200 (the PR only widened it to 201-299), and requires a non-compliant proxy putting Content-Encoding on a bodiless CONNECT reply, so not blocking — mentioning as the same-class sibling of the content_length leak just fixed. The comment's "before the state writes below" is also slightly off since these header-loop writes are above.

    Extended reasoning...

    What

    The b62c16c follow-up hoisted the CONNECT dispatch above pretend_304 and the 204/304 content_length = Some(0) write so status-code-driven state can't leak into the origin leg. But the response-header loop at [lib.rs:4770-4909] runs before that dispatch and two of its arms also mutate self.state without the self.flags.proxy_tunneling && self.proxy_tunnel.is_none() skip that this PR just widened on the Content-Length (line 4775) and Transfer-Encoding (line 4835) arms:

    • Content-Encoding ([lib.rs:4807-4829]) — writes self.state.encoding = Gzip/Deflate/Brotli/Zstd and self.state.content_encoding_i with no CONNECT-leg skip.
    • Connection ([lib.rs:4869-4883]) — gated on 200..=299, writes self.state.flags.allow_keepalive with no CONNECT-leg skip.

    Nothing on the tunnel-start path resets these: start_proxy_handshake touches only response_message_buffer, and ProxyTunnel's on_open/on_handshake reset only response_stage/request_stage/request_sent_len (grep of ProxyTunnel.rs for encoding/allow_keepalive/state.reset → no matches).

    Step-by-step (Content-Encoding)

    1. Client sends CONNECT origin:443 HTTP/1.1.
    2. Proxy replies HTTP/1.1 200 OK\r\nContent-Encoding: gzip\r\n\r\n (RFC-violating — a CONNECT reply has no body — but nothing forbids the header).
    3. handle_response_metadata runs the header loop: the Content-Encoding arm at 4807 sets self.state.encoding = Encoding::Gzip. Then at 4921 proxy_tunneling && proxy_tunnel.is_none() && 200..300return Ok(ContinueStreaming).
    4. Inner TLS handshake completes; on_handshake resets response_stage = ProxyHeaders but leaves state.encoding = Gzip.
    5. Origin replies HTTP/1.1 200 OK\r\nContent-Length: N\r\n\r\n<plaintext> with no Content-Encoding header. handle_response_metadata re-enters (now proxy_tunnel.is_some()), and since there is no Content-Encoding header the arm at 4807 never runs to overwrite the stale value — state.encoding is still Gzip.
    6. Body handling reaches InternalState::process_body_buffer, which sees encoding == Gzip and feeds the origin's plaintext bytes to the gzip decoder → the fetch rejects with a decompression error instead of resolving.

    For Connection: close on a 2xx CONNECT reply, state.flags.allow_keepalive = false similarly survives into the origin leg and prevents the tunnel from being pooled at the keep-alive check (~lib.rs:2170) — a much milder effect (perf only).

    Why the earlier fix doesn't cover it

    The comment at [lib.rs:4918-4920] says "Dispatched here, before the state writes below, because ProxyTunnel does not reset state between the CONNECT leg and the origin leg." That's accurate for the writes below (pretend_304, the 204/304 content_length write), but the header loop's Content-Encoding/Connection writes are above the dispatch, so the ordering guard doesn't protect them. The CL and TE arms are safe only because they carry an explicit if self.flags.proxy_tunneling && self.proxy_tunnel.is_none() { continue; } — the same guard the two remaining state-writing arms need.

    Relation to the PR / severity

    This is pre-existing for CONNECT → 200: before this PR, a 200 CONNECT reply already went through the same unguarded arms and returned ContinueStreaming. The PR only widened the surface to 201-299 (which previously fell through to the surface-as-Response else-branch), and it touched the immediately-adjacent CL/TE skips by dropping their && status_code == 200 — making Content-Encoding/Connection the only header-loop arms still writing self.state on the CONNECT leg. A Content-Encoding header on a bodiless CONNECT reply is unusual and the impact (decompression error / unpooled tunnel) is DoS-class only; a hostile proxy can already DoS by not tunneling. So: nit, not blocking — flagged because REVIEW.md's "Fix the whole class in the same PR … grep for every sibling site sharing the pattern" applies directly, and the added comment's wording invites a future reader to move the block back down.

    Fix

    Add the same skip to the two remaining arms:

    h if h == hash_header_const(b"Content-Encoding") => {
        if self.flags.proxy_tunneling && self.proxy_tunnel.is_none() {
            continue;
        }
        if !self.flags.disable_decompression { ... }
    }
    ...
    h if h == hash_header_const(b"Connection") => {
        if self.flags.proxy_tunneling && self.proxy_tunnel.is_none() {
            continue;
        }
        if response.status_code >= 200 && response.status_code <= 299 { ... }
    }

    (or, equivalently, hoist a single if proxy_tunneling && proxy_tunnel.is_none() { continue; } to the top of the loop body and drop the per-arm copies — every header on a CONNECT reply is discarded either way now that non-2xx returns Err and 2xx returns ContinueStreaming). Either also makes the "before the state writes below" comment fully accurate.

  • 🟡 src/http/lib.rs:4922 — The WebSocket client's CONNECT dispatch at WebSocketUpgradeClient.rs:990-998/1025 still checks only b"HTTP/1.1 200 " / status_code != 200, so a proxy that answers CONNECT with 202/204 — which this PR now accepts (and tests) for fetch — still fails new WebSocket("wss://…", { proxy }) with ProxyConnectFailed. WebSocket already had the security-correct reject-on-non-200 behavior so the CVE fix isn't needed there; only the RFC 9110 §9.3.6 any-2xx widening is missing. Worth applying the same 200..300 range at both sites (and widening or dropping the byte-prefix fast path) so the two CONNECT parsers stay consistent — or noting the exclusion in the PR per REVIEW.md's "grep for every sibling site" rule.

    Extended reasoning...

    What

    This PR widens fetch()'s CONNECT tunnel-established check from status_code == 200 to >= 200 && < 300 (src/http/lib.rs:4922), citing RFC 9110 §9.3.6: "Any 2xx (Successful) response indicates that the sender … will switch to tunnel mode immediately after the response header section." The sibling CONNECT dispatch in the WebSocket client was not updated:

    • WebSocketUpgradeClient.rs:990-998 — byte-prefix fast path: !body.starts_with(b"HTTP/1.1 200 ") && !body.starts_with(b"HTTP/1.0 200 ")terminate(ProxyConnectFailed)
    • WebSocketUpgradeClient.rs:1025 — parsed check: if response.status_code != 200terminate(ProxyConnectFailed) (or ProxyAuthenticationRequired for 407)

    So a proxy that replies HTTP/1.1 204 No Content\r\n\r\n now establishes the tunnel for fetch("https://…", { proxy }) but still fails new WebSocket("wss://…", { proxy }) with ProxyConnectFailed — the two clients disagree on the same RFC-defined protocol point.

    Why REVIEW.md flags this

    Correctness → Fix the whole class in the same PR: "Grep for every sibling site sharing the pattern: parallel switch arms, sync/async twins, fast/slow paths, POSIX/Windows branches, SSL/non-SSL variants … If a site is intentionally excluded, say so in the PR." The WebSocket CONNECT parser is exactly the sibling site of the fetch CONNECT parser this PR changes, and the PR description doesn't mention excluding it.

    Why nothing prevents it

    The WebSocket client has its own hand-rolled CONNECT-reply parser (it doesn't route through handle_response_metadata), so the fix in src/http/lib.rs doesn't reach it. The byte-prefix fast path at :990-998 fires before the picohttp parse and rejects on the first packet if the status literal isn't 200 ; even if that fast path is bypassed (e.g. short first read), the parsed check at :1025 rejects any non-200.

    Step-by-step

    1. new WebSocket("wss://origin/", { proxy: "http://127.0.0.1:PORT" }).
    2. Client sends CONNECT origin:443 HTTP/1.1\r\n…\r\n\r\n.
    3. Proxy replies HTTP/1.1 204 No Content\r\n\r\n and starts relaying — a valid tunnel-established reply per the RFC section this PR cites, and one this PR added an explicit fetch test for at proxy-stress-errors.test.ts:135 ("CONNECT → 204 establishes the tunnel and the origin body arrives intact").
    4. handle_proxy_response runs: is_first && body.len() > 13 → true; body.starts_with(b"HTTP/1.1 200 ") → false; body.starts_with(b"HTTP/1.0 200 ") → false → terminate(ProxyConnectFailed).
    5. WebSocket onerror fires; the origin is never reached. The equivalent fetch through the same proxy succeeds.

    Impact

    No security or correctness regression: WebSocket already rejected non-200 CONNECT replies before this PR (so the CVE-2009-2062 fix isn't needed there), and its behavior is unchanged. The gap is RFC-compliance/consistency only, and non-200 2xx CONNECT replies are rare in practice. But it is exactly the sibling-site pattern REVIEW.md calls out, and after this PR the two clients observably diverge on identical proxy behavior.

    Fix

    Widen both checks to the same range fetch now uses:

    // drop the byte-prefix fast path (or widen it to check for "HTTP/1." + digit + " 2")
    // ...
    if !(200..300).contains(&response.status_code) {
        if response.status_code == 407 {} else { terminate(ProxyConnectFailed) }
        return;
    }

    If intentionally deferred, a mention in the PR description satisfies the REVIEW.md rule.

    Severity

    Nit — no regression, no security impact, rare in practice; consistency/RFC-compliance only.

Match the fetch client's RFC 9110 s9.3.6 behaviour so a proxy that
replies 202/204 to CONNECT works for both clients. Drop the byte-prefix
fast path; the picohttp parse with the max_http_header_size() cap
handles it.
@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

On the two additional findings:

  • Header-loop state leak (Content-Encoding/Connection/Alt-Svc): already addressed in 5d4e1ae, which hoisted the CONNECT dispatch above the header loop entirely (the review was against the earlier b62c16c layout). Tests for 200 + Content-Encoding: gzip and 200 + Transfer-Encoding: chunked on the CONNECT reply are in proxy-stress-errors.test.ts.

  • WebSocket CONNECT sibling: widened in 9dd5bbe to (200..300) and dropped the byte-prefix fast path so the two CONNECT parsers agree on RFC 9110 §9.3.6. 182 WebSocket-through-proxy tests pass.

@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: the only hard failure across the last four builds (82568, 82587, 82607, 82626) is binary-size, which is comparing against stale canary main #79916 (~1850 builds behind) and reports the same +512-576 KB on neighbouring PR builds (e.g. #82565). The net Rust diff here is negative. All test lanes that have run are green on the proxy/CONNECT/fetch/websocket paths; the remaining YELLOWs (hot.test.ts, watch-many-dirs.test.ts, complex-workspace.test.ts, webview-chrome.test.ts, css.test.ts, fastutf8stream-reopen.js, and one proxy-stress-errors deadPort() race that #35400 fixes) passed on retry and are unrelated.

Ready for review.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prior review findings (204/header-loop state leak, Display dropping the status, WebSocket sibling) are all addressed in the current diff, and this run found no new issues. Deferring to a human for sign-off since this is a security-relevant behavior change (CVE-2009-2062 class) that flips fetch's contract from resolve→reject on non-2xx CONNECT and has an acknowledged bun install retry-on-4xx side effect left for follow-up.

What was reviewed:

  • CONNECT dispatch is now above the header loop and the 204/304 content_length write; verified no self.state mutation precedes the early return, and the ContinueStreaming path still reaches start_proxy_handshake.
  • 101 CONNECT still fails with UnrequestedUpgrade (handled at lib.rs:3689 before handle_response_metadata), so that existing test remains valid.
  • Absolute-form (http origin) 407/403 path is untouched — the early return is gated on proxy_tunneling && proxy_tunnel.is_none().
Extended reasoning...

Overview

Changes handle_response_metadata in src/http/lib.rs to dispatch on the CONNECT reply before any header/status processing: 2xx → ContinueStreaming (start inner TLS), non-2xx → Err(ProxyConnectFailed(status))close_and_fail. Adds the ProxyConnectFailed(u32) error variant with a status-carrying Display, a matching FetchTasklet message, and widens the WebSocket client's CONNECT check to (200..300) while dropping its byte-prefix fast path. Three test files gain ~20 new cases and update ~10 existing ones from expect-resolve to expect-reject.

Security risks

This is the security fix: it stops a hostile proxy (or any MITM on the plaintext client→proxy hop) from surfacing arbitrary status/headers/body under an https origin's identity. The change fails closed — anything that isn't 2xx now rejects instead of resolving. The 2xx-widening is RFC-mandated and matches curl; a non-conforming proxy that sends 2xx + body feeds those bytes into the TLS handshake, which fails loudly. I did not find a way for a hostile proxy to reach a worse state than before.

Level of scrutiny

High. This is core HTTP client code shared by fetch, bun install, S3, and (via the sibling change) WebSocket. It changes the observable contract for a documented option, and the placement of the early return relative to self.state writes was already shown to be load-bearing twice in this PR's review history. A maintainer should confirm the resolve→reject flip and the deferred bun install retry/.name() follow-ups are acceptable.

Other factors

All four of my earlier findings are resolved in the current head (9dd5bbe): the 204 content_length leak and the Content-Encoding/TE header-loop leak are fixed by hoisting the dispatch above both; #[error(...)] now carries the status; the WebSocket path agrees on the 2xx range. New tests cover 202/204/gzip-CE/chunked-TE CONNECT replies tunneling to a real https origin, and 403/302/407/500/201 rejection cases. The 101-CONNECT test is unaffected because that status is short-circuited before handle_response_metadata runs. The one remaining known behavior change — bun install now retrying 4xx CONNECT up to max_retry_count and losing the status in its message — is documented in the PR body as a bounded follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants