Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
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 @@ pub enum Error {
InvalidCRL,
#[error("UnsupportedProxyProtocol")]
UnsupportedProxyProtocol,
#[error("CONNECT tunnel failed, response {0}")]
ProxyConnectFailed(u32),
#[error(transparent)]
Cert(#[from] CertError),
#[error(transparent)]
Expand Down Expand Up @@ -308,6 +310,7 @@ impl Error {
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
57 changes: 21 additions & 36 deletions src/http/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4770,15 +4770,9 @@ impl<'a> HTTPClient<'a> {
for (header_i, header) in response.headers.list.iter().enumerate() {
match hash_header_name(header.name()) {
h if h == hash_header_const(b"Content-Length") => {
// RFC 9110 section 9.3.6: a client MUST ignore
// 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
{
// RFC 9110 §9.3.6: ignore Content-Length in a response to
// CONNECT (2xx → opaque tunnel, non-2xx → rejected below).
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 +4832,7 @@ impl<'a> HTTPClient<'a> {
// 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 @@ -4921,6 +4912,21 @@ impl<'a> HTTPClient<'a> {
print_response(response);
}

// RFC 9110 §9.3.6: any 2xx to CONNECT establishes the tunnel; a
// non-2xx CONNECT reply travelled over the plaintext client→proxy
// hop so it must reject the fetch, never surface as an https-origin
// Response (CVE-2009-2062). Dispatched here, before the state writes
// below, because ProxyTunnel does not reset `state` between the
// CONNECT leg and the origin leg.
Comment thread
robobun marked this conversation as resolved.
Outdated
if self.flags.proxy_tunneling && self.proxy_tunnel.is_none() {
if response.status_code >= 200 && response.status_code < 300 {
// signal to continue the proxing
return Ok(ShouldContinue::ContinueStreaming);
}

return Err(crate::Error::ProxyConnectFailed(response.status_code));
Comment thread
robobun marked this conversation as resolved.
Outdated
}

if pretend_304 {
response.status_code = 304;
}
Expand All @@ -4931,8 +4937,6 @@ impl<'a> HTTPClient<'a> {
// [...] cannot contain a message body or trailer section.
// Therefore in these cases set content-length to 0, so the response body is always ignored
// and is not waited for (which could cause a timeout).
// This applies regardless of whether we're using a proxy tunnel or not,
// since these status codes NEVER have a body per the HTTP spec.
if (response.status_code >= 100 && response.status_code < 200)
|| response.status_code == 204
|| response.status_code == 304
Expand All @@ -4958,24 +4962,6 @@ impl<'a> HTTPClient<'a> {
}
}

// 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;
if self.flags.proxy_tunneling && self.proxy_tunnel.is_none() {
if response.status_code == 200 {
// signal to continue the proxing
return Ok(ShouldContinue::ContinueStreaming);
}

// 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;
}

let status_code = response.status_code;

if status_code == 407 {
Expand All @@ -4986,8 +4972,7 @@ impl<'a> HTTPClient<'a> {
// 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 +5231,7 @@ impl<'a> HTTPClient<'a> {
}
_ => {}
}
} 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
172 changes: 146 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,108 @@ 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);
});
}

// A 204 CONNECT reply is a valid 2xx tunnel-established status. The client
// must not let the CONNECT leg's 204 → content_length=0 write leak into the
// origin leg (ProxyTunnel does not reset state between the two), or the
// origin's Content-Length: N would be rejected as a duplicate-CL conflict.
for (const connectStatus of [202, 204] as const) {
test.concurrent(
`CONNECT → ${connectStatus} establishes the tunnel and the origin body arrives intact`,
async () => {
await using origin = await createAdversarialOrigin({ tls: true, body: "hello-through-2xx-tunnel" });

const sockets = new Set<net.Socket>();
const proxy = net.createServer(client => {
sockets.add(client);
client.on("close", () => sockets.delete(client));
client.on("error", () => {});
let head = "";
const onHead = (d: Buffer) => {
head += d.toString("latin1");
const end = head.indexOf("\r\n\r\n");
if (end < 0) return;
client.removeListener("data", onHead);
const [, target] = head.split("\r\n")[0].split(" ");
const colon = target.lastIndexOf(":");
const upstream = net.connect(Number(target.slice(colon + 1)), "127.0.0.1");
sockets.add(upstream);
upstream.on("close", () => (sockets.delete(upstream), client.end()));
upstream.on("error", () => client.destroy());
upstream.once("connect", () => {
client.write(`HTTP/1.1 ${connectStatus} ${connectStatus === 204 ? "No Content" : "Accepted"}\r\n\r\n`);
client.pipe(upstream);
upstream.pipe(client);
});
};
client.on("data", onHead);
});
proxy.listen(0, "127.0.0.1");
await once(proxy, "listening");
const port = (proxy.address() as net.AddressInfo).port;

try {
const res = await fetch(origin.url, {
proxy: `http://127.0.0.1:${port}`,
keepalive: false,
tls: laxTls,
signal: AbortSignal.timeout(15_000),
});
expect(await res.text()).toBe("hello-through-2xx-tunnel");
expect(res.status).toBe(200);
expect(res.headers.get("content-length")).toBe(String("hello-through-2xx-tunnel".length));
expect(origin.requests.length).toBe(1);
} finally {
for (const s of sockets) s.destroy();
proxy.close();
}
},
);
}

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 +251,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 +295,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 +317,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