Skip to content
Open
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
14 changes: 14 additions & 0 deletions src/http/HTTPThread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,20 @@ impl HttpThread {
.connect_socket(client, unix_path.slice());
}

// A `:port` that fails to parse must not collapse into the scheme
// default via `get_port_auto()` below, or the request is silently
// sent to port 80/443 of that host. The WHATWG parser never produces
// these, but env-proxy values (`HTTP_PROXY=http://host:107688`) and
// S3/registry endpoints reach here unvalidated.
if client.url.has_invalid_port() {
return Err(crate::Error::InvalidPort);
}
if let Some(proxy) = &client.http_proxy {
if proxy.has_invalid_port() {
return Err(crate::Error::InvalidProxyPort);
}
}

if IS_SSL {
'custom_ctx: {
let Some(tls) = client.tls_props.clone() else {
Expand Down
6 changes: 6 additions & 0 deletions src/http/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ pub enum Error {
AbortedBeforeConnecting,
#[error("InvalidURL")]
InvalidURL,
#[error("InvalidPort")]
InvalidPort,
#[error("InvalidProxyPort")]
InvalidProxyPort,
#[error("ERR_TLS_CERT_ALTNAME_INVALID")]
ERR_TLS_CERT_ALTNAME_INVALID,
#[error("ClientAborted")]
Expand Down Expand Up @@ -276,6 +280,8 @@ impl Error {
Self::Timeout => "Timeout",
Self::AbortedBeforeConnecting => "AbortedBeforeConnecting",
Self::InvalidURL => "InvalidURL",
Self::InvalidPort => "InvalidPort",
Self::InvalidProxyPort => "InvalidProxyPort",
Self::ERR_TLS_CERT_ALTNAME_INVALID => "ERR_TLS_CERT_ALTNAME_INVALID",
Self::ClientAborted => "ClientAborted",
Self::HTTP2Unsupported => "HTTP2Unsupported",
Expand Down
6 changes: 6 additions & 0 deletions src/runtime/webcore/fetch/FetchTasklet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1387,6 +1387,12 @@ impl FetchTasklet {
http::Error::RedirectURLInvalid => {
BunString::static_("Redirect URL in Location header is invalid.")
}
http::Error::InvalidPort => BunString::static_(
"Invalid port number in URL. Ports must be a number between 0 and 65535.",
),
http::Error::InvalidProxyPort => BunString::static_(
"Invalid port number in proxy URL. Ports must be a number between 0 and 65535.",
),

http::Error::Cert(http::CertError::UNABLE_TO_GET_ISSUER_CERT) => {
BunString::static_("unable to get issuer certificate")
Expand Down
14 changes: 14 additions & 0 deletions src/runtime/webcore/s3/credentials_jsc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,20 @@ pub(crate) fn get_credentials_with_options(
let utf8 = str.to_utf8();
let endpoint = utf8.slice();
let url = URL::parse(endpoint);
// `get_port_auto()` would silently turn an
// unparseable port into 80/443 and send signed
// requests there; reject it up front.
if url.has_invalid_port() {
str.deref();
return Err(global_object
.err(
bun_jsc::ErrorCode::INVALID_ARG_VALUE,
format_args!(
"endpoint port must be a number between 0 and 65535"
),
)
.throw());
}
let normalized_endpoint = url.host_with_path();
if !normalized_endpoint.is_empty() {
new_credentials.credentials.endpoint =
Expand Down
21 changes: 21 additions & 0 deletions src/s3_signing/credentials.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,9 @@ impl S3Credentials {
host = &self.endpoint[..index];
extra_path = &self.endpoint[index..];
}
if host_has_invalid_port(host) {
return Err(SignError::InvalidEndpoint);
}
// only the host part is needed here
break 'brk_host Box::<[u8]>::from(host);
} else {
Expand Down Expand Up @@ -1436,3 +1439,21 @@ fn is_valid_host_component(value: &[u8]) -> bool {
.iter()
.all(|&c| c.is_ascii_alphanumeric() || c == b'-' || c == b'.' || c == b'_')
}

/// `host` is `hostname[:port]` or `[ipv6][:port]`. Returns `true` when a
/// `:port` is present but does not parse as a u16; connecting with such a
/// host silently targets the scheme default port (80/443) instead.
fn host_has_invalid_port(host: &[u8]) -> bool {
let port: &[u8] = if host.first() == Some(&b'[') {
match strings::index_of(host, b"]:") {
Some(i) => &host[i + 2..],
None => return false,
}
} else {
match strings::index_of_char(host, b':') {
Some(i) => &host[i as usize + 1..],
None => return false,
}
};
!port.is_empty() && bun_core::fmt::parse_int::<u16>(port, 10).is_err()
}
8 changes: 8 additions & 0 deletions src/url/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,14 @@ impl<'a> URL<'a> {
bun_core::fmt::parse_int::<u16>(self.port, 10).ok()
}

/// `true` when the URL carries a `:port` component whose text does not
/// parse as a u16 (`:107688`, `:9000abc`). `get_port_auto()` silently
/// substitutes the scheme default for these, so consumers that connect
/// with user-supplied URLs must reject them first.
pub fn has_invalid_port(&self) -> bool {
!self.port.is_empty() && self.get_port().is_none()
}

pub fn get_port_auto(&self) -> u16 {
self.get_port().unwrap_or_else(|| self.get_default_port())
}
Expand Down
44 changes: 44 additions & 0 deletions test/js/bun/http/proxy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2320,3 +2320,47 @@ describe("http_proxy env var scheme is case-insensitive", () => {
expect(exitCode).toBe(0);
});
});

// An env proxy whose `:port` text does not parse as a u16 (overflow like
// 107688 or 4295009448, out of range 99999, trailing garbage 9000abc) used to
// collapse into the scheme default via get_port_auto(): every proxied request
// was silently sent to port 80/443 of the proxy host. It must be rejected
// before any connection is attempted; curl and Node (ERR_PROXY_INVALID_CONFIG)
// both error on these.
describe("env proxy with unparseable port is rejected", () => {
const cases = [
["HTTP_PROXY", "107688", "http"], // 42152 + 2^16
["HTTP_PROXY", "4295009448", "http"], // 42152 + 2^32
["http_proxy", "99999", "http"],
["HTTP_PROXY", "9000abc", "http"],
["HTTPS_PROXY", "107688", "https"],
] as const;

test.concurrent.each(cases)("%s=http://127.0.0.1:%s", async (envKey, badPort, targetScheme) => {
// Target is port 1 on loopback: the fetch must fail on the proxy URL
// before any connection is attempted, so nothing ever dials the target.
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`try { await fetch(${JSON.stringify(`${targetScheme}://127.0.0.1:1/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: undefined,
HTTPS_PROXY: undefined,
https_proxy: undefined,
[envKey]: `http://127.0.0.1:${badPort}`,
},
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 InvalidProxyPort");
expect(exitCode).toBe(0);
});
});
75 changes: 71 additions & 4 deletions test/js/bun/s3/s3.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1786,7 +1786,9 @@ describe("s3 multipart upload id validation", () => {

await using proc = Bun.spawn({
cmd: [bunExe(), "-e", fixture],
env: bunEnv,
// The fixture's S3 endpoint is a loopback server; ambient proxy env
// (set in some CI/dev containers) must not intercept it.
env: { ...bunEnv, HTTP_PROXY: undefined, http_proxy: undefined, HTTPS_PROXY: undefined, https_proxy: undefined },
stdout: "pipe",
stderr: "pipe",
});
Expand Down Expand Up @@ -1844,7 +1846,9 @@ describe("s3 upload stream body error", () => {
`;
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", fixture],
env: bunEnv,
// The fixture's S3 endpoint is a loopback server; ambient proxy env
// (set in some CI/dev containers) must not intercept it.
env: { ...bunEnv, HTTP_PROXY: undefined, http_proxy: undefined, HTTPS_PROXY: undefined, https_proxy: undefined },
stdout: "pipe",
stderr: "pipe",
});
Expand Down Expand Up @@ -1904,7 +1908,9 @@ describe("s3 upload stream body error", () => {
`;
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", fixture],
env: bunEnv,
// The fixture's S3 endpoint is a loopback server; ambient proxy env
// (set in some CI/dev containers) must not intercept it.
env: { ...bunEnv, HTTP_PROXY: undefined, http_proxy: undefined, HTTPS_PROXY: undefined, https_proxy: undefined },
stdout: "pipe",
stderr: "pipe",
});
Expand Down Expand Up @@ -1984,7 +1990,9 @@ describe("s3 upload stream body error", () => {
`;
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", fixture],
env: bunEnv,
// The fixture's S3 endpoint is a loopback server; ambient proxy env
// (set in some CI/dev containers) must not intercept it.
env: { ...bunEnv, HTTP_PROXY: undefined, http_proxy: undefined, HTTPS_PROXY: undefined, https_proxy: undefined },
stdout: "pipe",
stderr: "pipe",
});
Expand Down Expand Up @@ -2043,3 +2051,62 @@ describe("presigned url signature", () => {
}
});
});

// An endpoint whose `:port` text does not parse as a u16 used to collapse
// into the scheme default via get_port_auto(): the SigV4-signed request was
// silently sent to port 80/443 of the endpoint host, and presign() emitted a
// URL carrying the bogus port verbatim.
describe.concurrent("s3 endpoint with unparseable port", () => {
const base = { accessKeyId: "a", secretAccessKey: "b", bucket: "bk" };
const badPorts = ["107688", "4295009448", "99999", "9000abc"];

it("S3Client constructor rejects it", () => {
for (const port of badPorts) {
try {
new Bun.S3Client({ ...base, endpoint: `http://127.0.0.1:${port}` });
expect.unreachable();
} catch (e: any) {
expect(e?.code).toBe("ERR_INVALID_ARG_VALUE");
expect(e?.message).toContain("endpoint port");
}
}
});

it("per-file endpoint option rejects it", () => {
try {
Bun.s3.file("key", { ...base, endpoint: "http://127.0.0.1:107688" });
expect.unreachable();
} catch (e: any) {
expect(e?.code).toBe("ERR_INVALID_ARG_VALUE");
}
});

it("valid ports still work", () => {
expect(() => new Bun.S3Client({ ...base, endpoint: "http://127.0.0.1" })).not.toThrow();
const presigned = new Bun.S3Client({ ...base, endpoint: "http://127.0.0.1:9000" }).presign("k");
expect(new URL(presigned).port).toBe("9000");
});

it("S3_ENDPOINT env var: presign errors instead of emitting the URL", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`try { console.log("URL", Bun.s3.presign("k")); } catch (e) { console.log("ERR", e?.code); }`,
],
env: {
...bunEnv,
S3_ENDPOINT: "http://127.0.0.1:107688",
S3_BUCKET: "bk",
S3_ACCESS_KEY_ID: "a",
S3_SECRET_ACCESS_KEY: "b",
},
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 ERR_S3_INVALID_ENDPOINT");
expect(exitCode).toBe(0);
});
});