serve: build request.url through one function on every transport

src/runtime/server/server_body.rs

@@ -3267,32 +3267,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<Vec<u8>> = 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<Vec<u8>> = ReqLike::header(req, b"host").map(<[u8]>::to_vec);
+            request_object.set_url_from_target(host.as_deref(), ReqLike::url(req), true);
             ctx.clear_req();
         }
 

src/runtime/webcore/Request.rs

@@ -876,8 +876,10 @@ 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.
+    /// 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.
     pub(crate) fn is_valid_host_header(host: &[u8]) -> bool {
         !host.is_empty()
             && host.iter().all(|&c| {
@@ -906,6 +908,35 @@ impl Request {
             })
     }
 
+    /// Eagerly sets `request.url` from a request line: `{scheme}://{Host}{path}` when the
+    /// client's Host can form a URL authority, otherwise the bare target. This is the same
+    /// policy `ensure_url` applies lazily for HTTP/1 (Host byte-set check, absolute-form
+    /// target reduced to its path, WHATWG canonicalization, bare path if the parser still
+    /// rejects the authority), for transports that must populate the URL before the
+    /// underlying request goes away.
+    pub(crate) fn set_url_from_target(&self, host: Option<&[u8]>, target: &[u8], https: bool) {
+        let path = Self::request_target_path(target);
+        if let Some(host) = host.filter(|h| Self::is_valid_host_header(h))
+            && !path.is_empty()
+            && path[0] == b'/'
+        {
+            let protocol: &[u8] = if https { b"https://" } else { b"http://" };
+            let mut url = Vec::with_capacity(protocol.len() + host.len() + path.len());
+            url.extend_from_slice(protocol);
+            url.extend_from_slice(host);
+            url.extend_from_slice(&path);
+            let raw = BunString::clone_utf8(&url);
+            let href = bun_url::href_from_string(&raw);
+            raw.deref();
+            if !href.is_empty() {
+                self.url.set(href);
+                return;
+            }
+            // `example.com:abc`, `exa%zzmple.com`: inside the byte set but not an authority.
+        }
+        self.url.set(BunString::clone_utf8(&path));
+    }
+
     pub(crate) fn ensure_url(&self) -> Result<(), AllocError> {
         if !self.url.get().is_empty() {
             return Ok(());
@@ -953,8 +984,9 @@ impl Request {
                                 self.url.set(href);
                             }
                         } else {
-                            // TODO: what is the right thing to do for invalid URLS?
-                            self.url.set(BunString::clone_utf8(url));
+                            // In the byte set but still not an authority (`host:abc`, bad
+                            // percent-encoding): same as no usable Host, keep the path.
+                            self.url.set(BunString::clone_utf8(&req_url));
                         }
 
                         return Ok(());
@@ -981,9 +1013,10 @@ impl Request {
                     }
 
                     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);
+                    } else {
+                        self.url.set(BunString::clone_utf8(&req_url));
                     }
 
                     return Ok(());

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"],
+    // Long enough that the URL is assembled on the heap instead of the 128-byte stack buffer.
+    [`${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://<host>/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,

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<string, string>) =>
+        fetch(`https://127.0.0.1:${port}${path}`, { headers, tls: { rejectUnauthorized: false } } as RequestInit).then(
+          r => r.text(),
+        );
+      const viaH3 = (path: string, headers: Record<string, string>) =>
+        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";

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,