diff --git a/src/runtime/server/server_body.rs b/src/runtime/server/server_body.rs index 6ade25711365..3d489a08fac3 100644 --- a/src/runtime/server/server_body.rs +++ b/src/runtime/server/server_body.rs @@ -3263,32 +3263,10 @@ where )) })); // NOTE: `ReqLike::{url,header}` both borrow `&mut req`; the - // returned slices alias the same uWS-owned header buffer. Format - // the `https://{host}` prefix while `host` is borrowed so the - // second `&mut req` borrow for `url` is unconflicted. - let prefix: Option> = ReqLike::header(req, b"host") - .filter(|host| Request::is_valid_host_header(host)) - .map(|host| { - let fmt = bun_fmt::HostFormatter { - is_https: true, - host, - port: None, - }; - let mut s = Vec::new(); - let _ = write!(&mut s, "https://{}", fmt); - s - }); - let path = ReqLike::url(req); - if !path.is_empty() && path[0] == b'/' { - if let Some(mut s) = prefix { - s.extend_from_slice(path); - request_object.url.set(BunString::clone_utf8(&s)); - } else { - request_object.url.set(BunString::clone_utf8(path)); - } - } else { - request_object.url.set(BunString::clone_utf8(path)); - } + // returned slices alias the same uWS-owned header buffer, so copy + // `host` out before borrowing again for the target. + let host: Option> = ReqLike::header(req, b"host").map(<[u8]>::to_vec); + request_object.set_synthesized_url(host.as_deref(), ReqLike::url(req)); ctx.clear_req(); } diff --git a/src/runtime/webcore/Request.rs b/src/runtime/webcore/Request.rs index e6d55c7d1163..5d289e815af9 100644 --- a/src/runtime/webcore/Request.rs +++ b/src/runtime/webcore/Request.rs @@ -822,20 +822,10 @@ impl Request { if let Some(req) = self.request_context.get_request() { // S008: `uws::Request` is an `opaque_ffi!` ZST handle — safe deref. let req = bun_opaque::opaque_deref(req); - let req_url = Self::request_target_path(req.url()); - if !req_url.is_empty() && req_url[0] == b'/' { - if let Some(host) = req - .header(b"host") - .filter(|host| Self::is_valid_host_header(host)) - { - // With `port: None`, HostFormatter always emits exactly `host`, so the - // formatted byte-count is just `host.len()`. Avoid the `core::fmt::write` - // vtable dispatch that `bun_fmt::count(format_args!(...))` incurs — this - // runs once per request via JSC extra-memory accounting. - return self.get_protocol().len() + host.len() + req_url.len(); - } - } - return req_url.len(); + let (host, path) = Self::url_parts(req.header(b"host"), req.url()); + // Canonicalizing can change the length slightly; this is an estimate for + // JSC's extra-memory accounting, so the joined length is close enough. + return host.map_or(0, |host| self.get_protocol().len() + host.len()) + path.len(); } 0 @@ -876,9 +866,11 @@ impl Request { } /// RFC 3986 3.2.2 `uri-host [ ":" port ]` byte set. A Host value outside it, or an empty - /// one, cannot form a URL authority, so `request.url` synthesis falls back to the - /// configured host instead of pasting the client bytes into the URL. - pub(crate) fn is_valid_host_header(host: &[u8]) -> bool { + /// one, cannot form a URL authority, so `request.url` synthesis falls back to the bare + /// request-target instead of pasting the client bytes into the URL. A value inside the + /// set can still be rejected by the URL parser (`host:abc`, bad percent-encoding); the + /// callers apply the same fallback in that case. + fn is_valid_host_header(host: &[u8]) -> bool { !host.is_empty() && host.iter().all(|&c| { c.is_ascii_alphanumeric() @@ -906,6 +898,66 @@ impl Request { }) } + /// The one decision behind `request.url` on every transport: the path the request + /// asked for, plus the client's Host / `:authority` if it can be a URL authority. + /// A host is dropped when it is missing, when the path is not origin-form, or when it + /// contains a byte that cannot appear in an authority (`/`, `?`, `#`, `@`, `\`, ...), + /// so client bytes never end up in the URL anywhere but the host position. + fn url_parts<'a>( + host: Option<&'a [u8]>, + target: &'a [u8], + ) -> (Option<&'a [u8]>, Cow<'a, [u8]>) { + let path = Self::request_target_path(target); + let host = host + .filter(|host| Self::is_valid_host_header(host)) + .filter(|_| path.first() == Some(&b'/')); + (host, path) + } + + /// `{protocol}{host}{path}` as the URL parser canonicalizes it, or the bare path when + /// there is no usable host or the parser still rejects it (`host:abc`, `exa%zzmple`). + fn synthesize_url(protocol: &'static [u8], host: Option<&[u8]>, target: &[u8]) -> BunString { + let (host, path) = Self::url_parts(host, target); + let Some(host) = host else { + return BunString::clone_utf8(&path); + }; + + // Join straight into a WTF string: parsing one is a refcount bump and an + // already-canonical URL comes back as the same string, so this is one allocation + // per request regardless of target length. Joining into a scratch buffer first adds + // a second target-sized allocation per request (req-url-leak.test.ts catches it). + // The host is ASCII by construction, so only the path decides the encoding. + let joined = if strings::is_all_ascii(&path) { + let (joined, bytes) = + BunString::create_uninitialized_latin1(protocol.len() + host.len() + path.len()); + let (a, rest) = bytes.split_at_mut(protocol.len()); + let (b, c) = rest.split_at_mut(host.len()); + a.copy_from_slice(protocol); + b.copy_from_slice(host); + c.copy_from_slice(&path); + joined + } else { + let mut bytes = Vec::with_capacity(protocol.len() + host.len() + path.len()); + bytes.extend_from_slice(protocol); + bytes.extend_from_slice(host); + bytes.extend_from_slice(&path); + BunString::clone_utf8(&bytes) + }; + let href = bun_url::href_from_string(&joined); + joined.deref(); + if href.is_empty() { + return BunString::clone_utf8(&path); + } + href + } + + /// For transports whose request goes away before JS can read `request.url` lazily + /// (HTTP/3), so it is populated up front from the same policy `ensure_url` uses. + pub(crate) fn set_synthesized_url(&self, host: Option<&[u8]>, target: &[u8]) { + self.url + .set(Self::synthesize_url(self.get_protocol(), host, target)); + } + pub(crate) fn ensure_url(&self) -> Result<(), AllocError> { if !self.url.get().is_empty() { return Ok(()); @@ -914,84 +966,11 @@ impl Request { if let Some(req) = self.request_context.get_request() { // S008: `uws::Request` is an `opaque_ffi!` ZST handle — safe deref. let req = bun_opaque::opaque_deref(req); - let req_url = Self::request_target_path(req.url()); - if !req_url.is_empty() && req_url[0] == b'/' { - if let Some(host) = req - .header(b"host") - .filter(|host| Self::is_valid_host_header(host)) - { - // With `port: None`, HostFormatter always emits exactly `host`. Compute the - // length and assemble the URL with straight slice copies instead of going - // through `core::fmt::write` (which is not monomorphized and shows up in - // per-request profiles). - let protocol = self.get_protocol(); - let url_bytelength = protocol.len() + host.len() + req_url.len(); - - debug_assert!(self.size_of_url() == url_bytelength); - - if url_bytelength < 128 { - let mut buffer = [0u8; 128]; - let url = { - let mut at = 0; - buffer[at..at + protocol.len()].copy_from_slice(protocol); - at += protocol.len(); - buffer[at..at + host.len()].copy_from_slice(host); - at += host.len(); - buffer[at..at + req_url.len()].copy_from_slice(&req_url); - at += req_url.len(); - &buffer[..at] - }; - - debug_assert!(self.size_of_url() == url.len()); - - let href = bun_url::href_from_string(&BunString::from_bytes(url)); - if !href.is_empty() { - if core::ptr::eq(href.byte_slice().as_ptr(), url.as_ptr()) { - self.url.set(BunString::clone_latin1(&url[..href.length()])); - href.deref(); - } else { - self.url.set(href); - } - } else { - // TODO: what is the right thing to do for invalid URLS? - self.url.set(BunString::clone_utf8(url)); - } - - return Ok(()); - } - - if strings::is_all_ascii(host) && strings::is_all_ascii(&req_url) { - let (new_url, bytes) = - BunString::create_uninitialized_latin1(url_bytelength); - self.url.set(new_url); - // exact space was counted above - let (a, rest) = bytes.split_at_mut(protocol.len()); - let (b, c) = rest.split_at_mut(host.len()); - a.copy_from_slice(protocol); - b.copy_from_slice(host); - c.copy_from_slice(&req_url); - } else { - // slow path - let mut temp_url: Vec = Vec::with_capacity(url_bytelength); - temp_url.extend_from_slice(protocol); - temp_url.extend_from_slice(host); - temp_url.extend_from_slice(&req_url); - // `defer bun.default_allocator.free(temp_url)` → Vec drops at scope end - self.url.set(BunString::clone_utf8(&temp_url)); - } - - let href = bun_url::href_from_string(&self.url.get()); - // TODO: what is the right thing to do for invalid URLS? - if !href.is_empty() { - self.url.set(href); - } - - return Ok(()); - } - } - - debug_assert!(self.size_of_url() == req_url.len()); - self.url.set(BunString::clone_utf8(&req_url)); + self.url.set(Self::synthesize_url( + self.get_protocol(), + req.header(b"host"), + req.url(), + )); } Ok(()) } diff --git a/test/js/bun/http/request-smuggling.test.ts b/test/js/bun/http/request-smuggling.test.ts index 641208f1c86f..5ced0a41b137 100644 --- a/test/js/bun/http/request-smuggling.test.ts +++ b/test/js/bun/http/request-smuggling.test.ts @@ -1682,6 +1682,32 @@ describe("Host header field values in request.url", () => { expect(response.slice(response.indexOf("\r\n\r\n") + 4)).toBe("/index"); }); + test.each([ + ["1.2.3.4.5"], + ["[::1"], + // The same fallback for a joined URL much longer than the other cases. + [`${Buffer.alloc(130, "x").toString()}.example:abc`], + ])( + "request.url is the request-target when the Host header %j is in the authority byte set but does not parse", + async host => { + await using server = Bun.serve({ + port: 0, + hostname: "127.0.0.1", + fetch(req) { + return new Response(req.url); + }, + }); + + const response = await sendRawRequest( + server, + `GET /index HTTP/1.1\r\nHost: ${host}\r\nConnection: close\r\n\r\n`, + ); + expect(response).toStartWith("HTTP/1.1 200"); + // Not `http:///index`, which `new URL()` rejects. + expect(response.slice(response.indexOf("\r\n\r\n") + 4)).toBe("/index"); + }, + ); + test.each([ ["example.com", "http://example.com/index"], ["example.com:8080", "http://example.com:8080/index"], @@ -1754,13 +1780,15 @@ describe("Host header field values in request.url", () => { // RFC 3986 `uri-host [ ":" port ]`: unreserved / sub-delims / "%" / ":" / "[" / "]". // Every byte in [0x7f, 0xff] is outside that set, so neither URL uses any of them. - const isHostByte = (char: string) => /^[A-Za-z0-9._~%!$&'()*+,;=:\[\]-]$/.test(char); + // "%", ":", "[" and "]" are inside it, but `a%b`, `a:b`, `a[b` and `a]b` are not + // authorities the URL parser accepts, so those fall back to the request-target too. + const formsUrlAuthority = (char: string) => /^[A-Za-z0-9._~!$&'()*+,;=-]$/.test(char); async function checkByte(byte: number) { const char = String.fromCharCode(byte); const host = `a${char}b`; - // Request::is_valid_host_header decides whether the Host header becomes the - // request URL's authority; the request itself is served either way. + // Request::is_valid_host_header and then the URL parser decide whether the Host + // header becomes the request URL's authority; the request itself is served either way. // The two probes run sequentially so each batch keeps at most one socket per byte open. const http11 = await sendRawRequest(server, `GET /p HTTP/1.1\r\nHost: ${host}\r\nConnection: close\r\n\r\n`); const http10 = await sendRawRequest(server, `GET /p HTTP/1.0\r\nHost: ${host}\r\n\r\n`); @@ -1784,7 +1812,7 @@ describe("Host header field values in request.url", () => { bytes.map(byte => { const char = String.fromCharCode(byte); // `req.url` carries the lowercased host (URL host normalization). - const url = isHostByte(char) ? `http://a${char.toLowerCase()}b/p` : "/p"; + const url = formsUrlAuthority(char) ? `http://a${char.toLowerCase()}b/p` : "/p"; return { char, http11Accepted: true, diff --git a/test/js/bun/http/serve-http3.test.ts b/test/js/bun/http/serve-http3.test.ts index 78e37448cbbb..6a01321fc245 100644 --- a/test/js/bun/http/serve-http3.test.ts +++ b/test/js/bun/http/serve-http3.test.ts @@ -53,6 +53,7 @@ const server = serve({ "/file-route": Bun.file(process.env.BIG_FILE), }, async fetch(req) { + if (req.headers.get("x-raw-url")) return new Response(req.url); const url = new URL(req.url); if (url.pathname === "/hello") { return new Response("hello over h3", { @@ -222,6 +223,37 @@ describe("Bun.serve HTTP/3", () => { }); }); + // request.url is synthesized from Host + target. A Host that can't be a URL + // authority must not be pasted into it (HTTP/1 already refused; HTTP/3 used + // to produce "https://evil.example/x#/admin/secret" here), and a valid one is + // canonicalized the same way on both transports. + test("request.url applies the same Host policy as HTTP/1", async () => { + await withServer(async port => { + const viaH1 = (path: string, headers: Record) => + fetch(`https://127.0.0.1:${port}${path}`, { headers, tls: { rejectUnauthorized: false } } as RequestInit).then( + r => r.text(), + ); + const viaH3 = (path: string, headers: Record) => + fetchH3(port, path, { headers }).then(r => r.text()); + + const evil = { host: "evil.example/x#", "x-raw-url": "1" }; + expect(await viaH3("/admin/secret", evil)).toBe("/admin/secret"); + expect(await viaH1("/admin/secret", evil)).toBe("/admin/secret"); + + const odd = { host: "EXAMPLE.com:443", "x-raw-url": "1" }; + expect(await viaH3("/p", odd)).toBe("https://example.com/p"); + expect(await viaH1("/p", odd)).toBe("https://example.com/p"); + + // Inside the Host byte set but still not a URL authority: falls back to + // the path on both transports rather than an unparsable absolute URL. + for (const host of ["example.com:abc", "example.com:99999", "exa%zzmple.com"]) { + const hdrs = { host, "x-raw-url": "1" }; + expect(await viaH3("/p", hdrs)).toBe("/p"); + expect(await viaH1("/p", hdrs)).toBe("/p"); + } + }); + }); + test("POST echoes body, status, request headers", async () => { await withServer(async port => { const body = "the quick brown fox jumps over the lazy dog"; diff --git a/test/js/bun/http/serve.test.ts b/test/js/bun/http/serve.test.ts index 342f62110092..bb41332c5210 100644 --- a/test/js/bun/http/serve.test.ts +++ b/test/js/bun/http/serve.test.ts @@ -597,6 +597,9 @@ it("request.url should be based on the Host header", async () => { it.each([ ["HTTP/1.0", "GET /helloooo HTTP/1.0\r\nHost: a/b\r\n\r\n"], ["HTTP/1.1", "GET /helloooo HTTP/1.1\r\nHost: a b\r\nConnection: close\r\n\r\n"], + // In the authority byte set, but the URL parser still rejects them. + ["HTTP/1.1 (bad port)", "GET /helloooo HTTP/1.1\r\nHost: example.com:abc\r\nConnection: close\r\n\r\n"], + ["HTTP/1.1 (bad escape)", "GET /helloooo HTTP/1.1\r\nHost: exa%zzmple.com\r\nConnection: close\r\n\r\n"], ])("request.url is the request-target when the %s Host header is not a valid authority", async (_version, payload) => { using server = Bun.serve({ port: 0,