Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/http/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@
InvalidCRL,
#[error("UnsupportedProxyProtocol")]
UnsupportedProxyProtocol,
#[error("ProxyConnectFailed")]
ProxyConnectFailed(u32),

Check warning on line 101 in src/http/error.rs

View check run for this annotation

Claude / Claude Code Review

ProxyConnectFailed Display impl drops the status code

The `Display` impl generated by `#[error("ProxyConnectFailed")]` drops the carried status code — this is the only non-transparent data-carrying variant in the enum, and the payload is the one thing a user needs to distinguish 407 (fix your auth) from 403 (proxy policy) from 502 (upstream unreachable). Consider `#[error("CONNECT tunnel failed, response {0}")]` (curl's wording) or at least `#[error("ProxyConnectFailed({0})")]` so any `Display` consumer gets the status for free. Note that `bun inst
Comment thread
robobun marked this conversation as resolved.
Outdated
#[error(transparent)]
Cert(#[from] CertError),
#[error(transparent)]
Expand Down Expand Up @@ -308,6 +310,7 @@
Self::FailedToOpenSocket => "FailedToOpenSocket",
Self::InvalidCRL => "InvalidCRL",
Self::UnsupportedProxyProtocol => "UnsupportedProxyProtocol",
Self::ProxyConnectFailed(_) => "ProxyConnectFailed",
Self::Cert(e) => <&'static str>::from(e),
Self::Alloc(_) => "OutOfMemory",
Self::Hpack(e) => <&'static str>::from(e),
Expand Down
40 changes: 17 additions & 23 deletions src/http/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4774,11 +4774,10 @@
// Content-Length in a successful response to CONNECT —
// the connection becomes an opaque tunnel and is never
// pooled, so the framing-desync concern below does not
// apply.
if self.flags.proxy_tunneling
&& self.proxy_tunnel.is_none()
&& response.status_code == 200
{
// apply. A non-2xx CONNECT reply is rejected outright
// below (ProxyConnectFailed), so no framing is needed for
// that case either.
Comment thread
robobun marked this conversation as resolved.
Outdated
if self.flags.proxy_tunneling && self.proxy_tunnel.is_none() {
continue;
}
// byte-level parse — header.value() is network bytes, not &str
Expand Down Expand Up @@ -4838,10 +4837,7 @@
// RFC 9110 section 9.3.6: as with Content-Length above, a
// client MUST ignore Transfer-Encoding in a successful
// response to CONNECT.
if self.flags.proxy_tunneling
&& self.proxy_tunnel.is_none()
&& response.status_code == 200
{
if self.flags.proxy_tunneling && self.proxy_tunnel.is_none() {
continue;
}
// RFC 9112 §7: transfer-coding names are case-insensitive.
Expand Down Expand Up @@ -4958,22 +4954,21 @@
}
}

// RFC 9110 §9.3.6: a non-200 response to CONNECT means the tunnel was
// not established. Surface the proxy's response to the caller, but
// never follow a Location header from it — a malicious proxy could
// otherwise redirect the request (body and custom headers included)
// to an attacker-chosen plaintext origin.
let mut is_proxy_connect_failure = false;
// RFC 9110 §9.3.6: any 2xx response to CONNECT means the tunnel is
// established and the bytes after the header section are from the
// origin. A non-2xx response means the tunnel was not established;
// the proxy's status/headers/body arrived over the plaintext
// client→proxy hop, so they MUST NOT be returned as an https-origin
// Response — reject the fetch instead (curl / Node undici / browsers
// all do this; see CVE-2009-2062). The socket is closed by the
// caller's close_and_fail on Err.
if self.flags.proxy_tunneling && self.proxy_tunnel.is_none() {
if response.status_code == 200 {
if response.status_code >= 200 && response.status_code < 300 {
// signal to continue the proxing
return Ok(ShouldContinue::ContinueStreaming);
}

Check failure on line 4969 in src/http/lib.rs

View check run for this annotation

Claude / Claude Code Review

CONNECT 204 pollutes state.content_length, breaking the origin response

Widening the tunnel-established check to any 2xx lets a `204 No Content` CONNECT reply reach `ContinueStreaming`, but the 1xx/204/304 block at lines 4932–4937 has already set `state.content_length = Some(0)`, and nothing on the tunnel-start path clears it. When the origin's real response arrives with `Content-Length: N`, the duplicate-CL guard at lines 4794–4800 sees `Some(0) != N` and returns `InvalidContentLength` (or silently truncates a close-delimited body to zero bytes). Clear `self.state.
Comment thread
robobun marked this conversation as resolved.
Outdated

// proxy denied connection so return proxy result (407, 403 etc)
self.flags.proxy_tunneling = false;
self.flags.disable_keepalive = true;
is_proxy_connect_failure = true;
return Err(crate::Error::ProxyConnectFailed(response.status_code));
}

let status_code = response.status_code;
Expand All @@ -4986,8 +4981,7 @@
// if is no redirect or if is redirect == "manual" just proceed
let is_redirect = status_code >= 300 && status_code <= 399;
if is_redirect {
if !is_proxy_connect_failure
&& self.redirect_type == FetchRedirect::Follow
if self.redirect_type == FetchRedirect::Follow
&& !location.is_empty()
&& self.remaining_redirect_count > 0
{
Expand Down Expand Up @@ -5246,7 +5240,7 @@
}
_ => {}
}
} else if !is_proxy_connect_failure && self.redirect_type == FetchRedirect::Error {
} else if self.redirect_type == FetchRedirect::Error {
// error out if redirect is not allowed
return Err(crate::Error::UnexpectedRedirect);
}
Expand Down
3 changes: 3 additions & 0 deletions src/runtime/webcore/fetch/FetchTasklet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1348,6 +1348,9 @@ impl FetchTasklet {
http::Error::RedirectURLInvalid => {
BunString::static_("Redirect URL in Location header is invalid.")
}
http::Error::ProxyConnectFailed(status) => BunString::create_format(format_args!(
"CONNECT tunnel failed, proxy responded with status {status}",
)),

http::Error::Cert(http::CertError::UNABLE_TO_GET_ISSUER_CERT) => {
BunString::static_("unable to get issuer certificate")
Expand Down
114 changes: 88 additions & 26 deletions test/js/bun/http/proxy-stress-errors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,31 +45,35 @@ describe("CONNECT failure status", () => {
proxyTls: [false, true] as const,
status: STATUSES,
})) {
test.concurrent(`${proxyTls ? "https" : "http"}-proxy CONNECT → ${status} is surfaced as-is`, async () => {
test.concurrent(`${proxyTls ? "https" : "http"}-proxy CONNECT → ${status} rejects the fetch`, async () => {
await using origin = await createAdversarialOrigin({ tls: true, body: "unreachable" });
await using proxy = await createAdversarialProxy({
tls: proxyTls,
connectStatus: status,
connectStatusBody: `proxy-said-${status}`,
});

const res = await fetch(origin.url, {
proxy: proxy.url,
keepalive: false,
tls: laxTls,
signal: AbortSignal.timeout(15_000),
// The proxy's reply came over the plaintext client→proxy hop with no
// TLS handshake to the origin; it must never surface as a Response
// attributed to the https origin. The proxy's status code is carried
// in the error message so auth/gateway failures are still debuggable.
await expect(
fetch(origin.url, {
proxy: proxy.url,
keepalive: false,
tls: laxTls,
signal: AbortSignal.timeout(15_000),
}),
).rejects.toMatchObject({
code: "ProxyConnectFailed",
message: expect.stringContaining(String(status)),
});
// The client surfaces the proxy's reply; it does NOT tunnel through.
expect(res.status).toBe(status);
expect(await res.text()).toBe(`proxy-said-${status}`);
// The origin must never have been reached.
expect(origin.requests.length).toBe(0);
});
}

// A 3xx CONNECT reply is surfaced, not followed (already covered for 307
// in proxy.test.ts; here we add 301/302 and assert the Location is not
// interpreted).
// A 3xx CONNECT reply rejects the fetch and its Location is never followed.
for (const status of [301, 302] as const) {
test.concurrent(`CONNECT → ${status} with Location is not followed`, async () => {
await using origin = await createAdversarialOrigin({ tls: true, body: "unreachable" });
Expand All @@ -79,14 +83,50 @@ describe("CONNECT failure status", () => {
connectReplyHeaders: { Location: bait.url },
});

const res = await fetch(origin.url, { proxy: proxy.url, keepalive: false, tls: laxTls });
expect(res.status).toBe(status);
expect(res.headers.get("location")).toBe(bait.url);
await expect(fetch(origin.url, { proxy: proxy.url, keepalive: false, tls: laxTls })).rejects.toMatchObject({
code: "ProxyConnectFailed",
message: expect.stringContaining(String(status)),
});
expect(bait.requests.length).toBe(0);
expect(origin.requests.length).toBe(0);
});
}

// RFC 9110 §9.3.6: any 2xx to CONNECT switches to tunnel mode. A proxy
// that sends a body alongside a non-200 2xx is violating the spec; the
// client must treat the post-header bytes as tunnel payload (so the inner
// TLS handshake fails), never as an https-origin Response body.
for (const proxyTls of [false, true] as const) {
test.concurrent(`${proxyTls ? "https" : "http"}-proxy CONNECT → 201 is treated as tunnel-established`, async () => {
await using origin = await createAdversarialOrigin({ tls: true, body: "unreachable" });
await using proxy = await createAdversarialProxy({
tls: proxyTls,
connectStatus: 201,
connectStatusBody: "BODY201x",
});

let outcome: { status: number; ok: boolean; body: string } | string;
try {
const res = await fetch(origin.url, {
proxy: proxy.url,
keepalive: false,
tls: laxTls,
signal: AbortSignal.timeout(15_000),
});
outcome = { status: res.status, ok: res.ok, body: await res.text() };
} catch (e) {
outcome = errcode(e);
}
// The proxy's plaintext body must not surface as origin content. The
// bytes after the 2xx header feed the TLS handshake, which fails; the
// exact error depends on whether the proxy hangs up first.
expect(outcome).not.toEqual({ status: 201, ok: true, body: "BODY201x" });
expect(typeof outcome).toBe("string");
expect(outcome).not.toBe("TimeoutError");
expect(origin.requests.length).toBe(0);
});
}

for (const proxyTls of [false, true] as const) {
test.concurrent(
`${proxyTls ? "https" : "http"}-proxy CONNECT → 101 fails even when the request asked to upgrade`,
Expand Down Expand Up @@ -153,13 +193,17 @@ describe("upstream unreachable via proxy", () => {

// Point at a refused port directly — the client will CONNECT to it,
// the proxy will fail to dial, and return 502.
const res = await fetch(`https://127.0.0.1:${dead.port}/`, {
proxy: proxy.url,
keepalive: false,
tls: laxTls,
signal: AbortSignal.timeout(15_000),
await expect(
fetch(`https://127.0.0.1:${dead.port}/`, {
proxy: proxy.url,
keepalive: false,
tls: laxTls,
signal: AbortSignal.timeout(15_000),
}),
).rejects.toMatchObject({
code: "ProxyConnectFailed",
message: expect.stringContaining("502"),
});
expect(res.status).toBe(502);
});

test.concurrent(`${proxyTls ? "https" : "http"}-proxy, absolute-form upstream refused → 502`, async () => {
Expand Down Expand Up @@ -193,9 +237,19 @@ describe("proxy authentication", () => {
tls: proxyTls,
auth: { user: "alice", pass: "s3cret" },
});
const res = await fetch(origin.url, { proxy: proxy.url, keepalive: false, tls: laxTls });
expect(res.status).toBe(407);
expect(res.headers.get("proxy-authenticate")).toContain("Basic");
const req = fetch(origin.url, { proxy: proxy.url, keepalive: false, tls: laxTls });
if (originTls) {
// CONNECT denied: fetch rejects, status carried in the message.
await expect(req).rejects.toMatchObject({
code: "ProxyConnectFailed",
message: expect.stringContaining("407"),
});
} else {
// Absolute-form http:// proxying: the 407 is a real Response.
const res = await req;
expect(res.status).toBe(407);
expect(res.headers.get("proxy-authenticate")).toContain("Basic");
}
expect(origin.requests.length).toBe(0);
});

Expand All @@ -205,12 +259,20 @@ describe("proxy authentication", () => {
tls: proxyTls,
auth: { user: "alice", pass: "s3cret" },
});
const res = await fetch(origin.url, {
const req = fetch(origin.url, {
proxy: `${proxyTls ? "https" : "http"}://alice:wrong@127.0.0.1:${proxy.port}`,
keepalive: false,
tls: laxTls,
});
expect(res.status).toBe(403);
if (originTls) {
await expect(req).rejects.toMatchObject({
code: "ProxyConnectFailed",
message: expect.stringContaining("403"),
});
} else {
const res = await req;
expect(res.status).toBe(403);
}
expect(origin.requests.length).toBe(0);
});

Expand Down
9 changes: 4 additions & 5 deletions test/js/bun/http/proxy-stress-lifecycle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,14 +180,13 @@ describe("proxy kills upstream", () => {
outcome = errcode(e);
}
// At "upstream-connected", the upstream's close happens before the
// tunnel is up; the proxy relays the 502 envelope it writes on
// upstream error, which the client surfaces as a 502 response.
// After the tunnel is up the close is relayed and the inner TLS
// fails. Either is acceptable; a hang is not.
// tunnel is up; the proxy's 502 CONNECT reply is surfaced as
// ProxyConnectFailed. After the tunnel is up the close is relayed
// and the inner TLS fails. Either is acceptable; a hang is not.
expect(outcome).not.toBe("TimeoutError");
expect(outcome).not.toBe("AbortError");
expect(outcome).toMatch(
/^resolved:502$|ECONNRESET|ConnectionClosed|ECONNREFUSED|ConnectionRefused|SocketError|EPIPE|ERR_TLS/,
/ProxyConnectFailed|ECONNRESET|ConnectionClosed|ECONNREFUSED|ConnectionRefused|SocketError|EPIPE|ERR_TLS/,
);
});
}
Expand Down
Loading
Loading