diff --git a/src/http/HTTPThread.rs b/src/http/HTTPThread.rs index c832568d9d69..8baaf20d196f 100644 --- a/src/http/HTTPThread.rs +++ b/src/http/HTTPThread.rs @@ -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 { diff --git a/src/http/error.rs b/src/http/error.rs index b7edb71da82a..943b6e142318 100644 --- a/src/http/error.rs +++ b/src/http/error.rs @@ -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")] @@ -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", diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index f477af732475..e13d49c3488d 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -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") diff --git a/src/runtime/webcore/s3/credentials_jsc.rs b/src/runtime/webcore/s3/credentials_jsc.rs index 7b249313929b..bfbf6779a267 100644 --- a/src/runtime/webcore/s3/credentials_jsc.rs +++ b/src/runtime/webcore/s3/credentials_jsc.rs @@ -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 = diff --git a/src/s3_signing/credentials.rs b/src/s3_signing/credentials.rs index d9de006e97fa..23387f66c65d 100644 --- a/src/s3_signing/credentials.rs +++ b/src/s3_signing/credentials.rs @@ -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 { @@ -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::(port, 10).is_err() +} diff --git a/src/url/lib.rs b/src/url/lib.rs index 94860e3f1070..6727423db073 100644 --- a/src/url/lib.rs +++ b/src/url/lib.rs @@ -420,6 +420,14 @@ impl<'a> URL<'a> { bun_core::fmt::parse_int::(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()) } diff --git a/test/js/bun/http/proxy.test.ts b/test/js/bun/http/proxy.test.ts index f53965c66cb9..7357def96902 100644 --- a/test/js/bun/http/proxy.test.ts +++ b/test/js/bun/http/proxy.test.ts @@ -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); + }); +}); diff --git a/test/js/bun/s3/s3.test.ts b/test/js/bun/s3/s3.test.ts index 645b84c390cd..ca4a1e6bbd53 100644 --- a/test/js/bun/s3/s3.test.ts +++ b/test/js/bun/s3/s3.test.ts @@ -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", }); @@ -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", }); @@ -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", }); @@ -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", }); @@ -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); + }); +});