Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
7 changes: 2 additions & 5 deletions src/http/HTTPThread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -566,10 +566,7 @@ impl HttpThread {
client.set_custom_ssl_ctx(ctx_nn);
// Keepalive is now supported for custom SSL contexts
let result = if let Some(url) = client.http_proxy.clone() {
if url.protocol.is_empty()
|| url.protocol == b"https"
|| url.protocol == b"http"
{
if url.protocol.is_empty() || url.has_http_like_protocol() {
custom_context.connect(client, url.hostname, url.get_port_auto())
} else {
return Err(crate::Error::UnsupportedProxyProtocol);
Expand All @@ -585,7 +582,7 @@ impl HttpThread {
if let Some(url) = client.http_proxy.clone() {
if !url.href.is_empty() {
// https://github.com/oven-sh/bun/issues/11343
if url.protocol.is_empty() || url.protocol == b"https" || url.protocol == b"http" {
if url.protocol.is_empty() || url.has_http_like_protocol() {
return self.context::<IS_SSL>().connect(
client,
url.hostname,
Expand Down
20 changes: 17 additions & 3 deletions src/http/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5219,8 +5219,18 @@ impl<'a> HTTPClient<'a> {
} else {
&location[0..i]
};
let is_http = protocol_name == b"http";
if is_http || protocol_name == b"https" {
let is_http = strings::eql_case_insensitive_ascii(
protocol_name,
b"http",
true,
);
if is_http
|| strings::eql_case_insensitive_ascii(
protocol_name,
b"https",
true,
)
{
} else {
return Err(crate::Error::UnsupportedRedirectProtocol);
}
Expand Down Expand Up @@ -5293,7 +5303,11 @@ impl<'a> HTTPClient<'a> {
return Err(crate::Error::RedirectURLTooLong);
}

let is_http = protocol_name == b"http";
let is_http = strings::eql_case_insensitive_ascii(
protocol_name,
b"http",
true,
);

if is_http {
string_builder.count(b"http:");
Expand Down
12 changes: 7 additions & 5 deletions src/url/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ impl<'a> URL<'a> {
}

pub fn is_file(&self) -> bool {
self.protocol == b"file"
strings::eql_case_insensitive_ascii(self.protocol, b"file", true)
}

/// host + path without the ending slash, protocol, searchParams and hash
Expand Down Expand Up @@ -377,17 +377,19 @@ impl<'a> URL<'a> {
b"http"
}

// RFC 3986 §3.1: the scheme is case-insensitive. `URL::parse` borrows
// `protocol` from the input without normalizing, so compare accordingly.
#[inline]
pub fn is_https(&self) -> bool {
self.protocol == b"https"
strings::eql_case_insensitive_ascii(self.protocol, b"https", true)
}
#[inline]
pub fn is_s3(&self) -> bool {
self.protocol == b"s3"
strings::eql_case_insensitive_ascii(self.protocol, b"s3", true)
}
#[inline]
pub fn is_http(&self) -> bool {
self.protocol == b"http"
strings::eql_case_insensitive_ascii(self.protocol, b"http", true)
}
Comment thread
robobun marked this conversation as resolved.

pub fn display_hostname(&self) -> &[u8] {
Expand Down Expand Up @@ -445,7 +447,7 @@ impl<'a> URL<'a> {
}

pub fn has_http_like_protocol(&self) -> bool {
self.protocol == b"http" || self.protocol == b"https"
self.is_http() || self.is_https()
}

pub fn get_port(&self) -> Option<u16> {
Expand Down
87 changes: 87 additions & 0 deletions test/js/bun/http/proxy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2155,3 +2155,90 @@ test("non-200 CONNECT response from proxy is surfaced and its Location header is
await once(proxy, "close");
}
});

// RFC 3986 §3.1: the URL scheme is case-insensitive. The explicit
// `fetch(url, { proxy })` option goes through the WHATWG URL parser, which
// lowercases the scheme; the http_proxy / HTTP_PROXY environment variables go
// through bun_url::URL::parse, which borrows the scheme as-is. Before the fix,
// `http_proxy=HTTP://host:port` failed every request with
// UnsupportedProxyProtocol while the identical string via `{ proxy }` worked.
// curl and Node's undici EnvHttpProxyAgent both accept the uppercase form.
describe("http_proxy env var scheme is case-insensitive", () => {
const cases = [
["http_proxy", "HTTP"],
["HTTP_PROXY", "Http"],
["http_proxy", "hTtP"],
] as const;

test.concurrent.each(cases)("%s=%s://... is accepted", async (envKey, scheme) => {
using origin = Bun.serve({ port: 0, fetch: () => new Response("origin") });
const proxy = net.createServer(clientSocket => {
clientSocket.on("error", () => {});
clientSocket.once("data", data => {
const body = "PROXIED " + data.toString("latin1").split("\r\n")[0];
clientSocket.end(`HTTP/1.1 200 OK\r\nContent-Length: ${body.length}\r\nConnection: close\r\n\r\n${body}`);
});
});
proxy.listen(0, "127.0.0.1");
await once(proxy, "listening");
const proxyPort = (proxy.address() as net.AddressInfo).port;

try {
const targetUrl = `http://127.0.0.1:${origin.port}/x`;
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`const r = await fetch(${JSON.stringify(targetUrl)}, { keepalive: false }); console.log(r.status, await r.text());`,
],
env: {
...bunEnv,
NO_PROXY: undefined,
no_proxy: undefined,
HTTP_PROXY: undefined,
http_proxy: undefined,
HTTPS_PROXY: undefined,
https_proxy: undefined,
[envKey]: `${scheme}://127.0.0.1:${proxyPort}`,
},
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(stdout.trim()).toBe(`200 PROXIED GET ${targetUrl} HTTP/1.1`);
expect(exitCode).toBe(0);
} finally {
proxy.close();
await once(proxy, "close");
}
});

// An unrecognized scheme must still be rejected loudly instead of silently
// going direct.
test.concurrent("socks5:// is still rejected with UnsupportedProxyProtocol", async () => {
using origin = Bun.serve({ port: 0, fetch: () => new Response("origin") });
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`try { await fetch(${JSON.stringify(`http://127.0.0.1:${origin.port}/x`)}); console.log("OK"); } catch (e) { console.log("ERR", e?.code ?? e?.name); }`,
],
env: {
...bunEnv,
NO_PROXY: undefined,
no_proxy: undefined,
HTTP_PROXY: undefined,
http_proxy: "socks5://127.0.0.1:1",
HTTPS_PROXY: undefined,
https_proxy: undefined,
},
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(stdout.trim()).toBe("ERR UnsupportedProxyProtocol");
expect(exitCode).toBe(0);
});
});
46 changes: 46 additions & 0 deletions test/js/web/fetch/fetch-redirect.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -282,3 +282,49 @@ it("fetch() does not leak intermediate redirect URLs in multi-hop chains", async
// page retention inflate RSS even with no leak, so widen the threshold.
expect(secondHalfMiB).toBeLessThan(isASAN ? 400 : 12);
}, 60_000);

// RFC 3986 §3.1: the URL scheme is case-insensitive. The Location header is
// taken from the response verbatim and its scheme sliced out before WHATWG
// normalization runs, so the http/https check has to compare case-insensitively
// or `Location: HTTPS://host/...` is rejected with UnsupportedRedirectProtocol.
describe("fetch() follows a redirect whose Location scheme is not lowercase", () => {
it.concurrent.each(["HTTP", "Http", "hTtP"])("Location: %s://...", async scheme => {
await using final = Bun.serve({
port: 0,
fetch: () => new Response("FINAL"),
});

const sockets = new Set<net.Socket>();
const server = net.createServer(socket => {
sockets.add(socket);
socket.on("close", () => sockets.delete(socket));
socket.on("error", () => {});
socket.once("data", () => {
socket.end(
`HTTP/1.1 302 Found\r\nLocation: ${scheme}://127.0.0.1:${final.port}/final\r\nContent-Length: 0\r\nConnection: close\r\n\r\n`,
);
});
});
await once(server.listen(0, "127.0.0.1"), "listening");
const { port } = server.address() as net.AddressInfo;

try {
const res = await fetch(`http://127.0.0.1:${port}/start`);
expect({
status: res.status,
redirected: res.redirected,
url: res.url,
body: await res.text(),
}).toEqual({
status: 200,
redirected: true,
url: `http://127.0.0.1:${final.port}/final`,
body: "FINAL",
});
} finally {
for (const s of sockets) s.destroy();
server.close();
await once(server, "close");
}
});
});
Loading