http: don't pool a connection when bytes trail a bodyless response

src/http/lib.rs

@@ -3773,6 +3773,15 @@
         }
 
         if should_continue == ShouldContinue::Finished {
+            // The message ended with its header block, so whatever follows it
+            // in this packet was never requested (RFC 9112 section 6.3). As
+            // with the Content-Length overshoot in handle_response_body, the
+            // response is still delivered, but the connection's framing can't
+            // be trusted any more: close it instead of pooling it. do_redirect
+            // honours this flag too via is_keep_alive_possible.
+            if !to_read.is_empty() {
+                self.state.flags.allow_keepalive = false;
+            }
             if self.state.flags.is_redirect_pending {
                 self.do_redirect::<IS_SSL>(ctx, socket);
                 return;

test/js/web/fetch/fetch.test.ts

@@ -3254,6 +3254,140 @@ it("does not reuse a keep-alive connection whose response carried more bytes tha
   }
 });
 
+it("does not reuse a keep-alive connection when bytes follow a response that ended at its header block", async () => {
+  // A 204/304, a Content-Length: 0 response, any response to HEAD, and a followed
+  // 3xx are complete once their header block is in. Bytes after that in the same
+  // packet are the bodyless counterpart of the Content-Length overshoot above: the
+  // connection's framing can no longer be trusted, so the response is delivered but
+  // the connection must be closed instead of going back to the pool (this includes
+  // the redirect path, which pools the socket before following the Location).
+  // Without trailing bytes each of these responses must still be pooled and reused.
+  const injected = "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 8\r\n\r\ninjected";
+  const responses: Record<string, { head: string; junk: string }> = {
+    "/204": { head: "HTTP/1.1 204 No Content\r\nConnection: keep-alive\r\n\r\n", junk: injected },
+    "/304": { head: 'HTTP/1.1 304 Not Modified\r\nETag: "x"\r\nConnection: keep-alive\r\n\r\n', junk: injected },
+    "/empty": { head: "HTTP/1.1 200 OK\r\nContent-Length: 0\r\nConnection: keep-alive\r\n\r\n", junk: injected },
+    // Answered with HEAD: Content-Length describes the GET body, nothing may follow.
+    // The junk variant sends that body anyway.
+    "/head": { head: "HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection: keep-alive\r\n\r\n", junk: "hello" },
+    "/303": {
+      head: "HTTP/1.1 303 See Other\r\nLocation: /legit\r\nContent-Length: 0\r\nConnection: keep-alive\r\n\r\n",
+      junk: injected,
+    },
+  };
+  const legit =
+    "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 6\r\nConnection: keep-alive\r\n\r\nlegit!";
+
+  let connections = 0;
+  const sockets: net.Socket[] = [];
+  const server = net.createServer(socket => {
+    connections++;
+    sockets.push(socket);
+    socket.on("error", () => {});
+    let buffered = "";
+    socket.on("data", data => {
+      buffered += data.toString("latin1");
+      while (true) {
+        const headerEnd = buffered.indexOf("\r\n\r\n");
+        if (headerEnd === -1) return;
+        const head = buffered.slice(0, headerEnd);
+        // Consume the request body (the redirect case is a POST) before answering, so
+        // body bytes are never mistaken for the next request.
+        let requestEnd = headerEnd + 4;
+        if (/^transfer-encoding:.*\bchunked\b/im.test(head)) {
+          const terminator = buffered.indexOf("0\r\n\r\n", requestEnd);
+          if (terminator === -1) return;
+          requestEnd = terminator + 5;
+        } else {
+          requestEnd += Number(/^content-length:\s*(\d+)/im.exec(head)?.[1] ?? 0);
+          if (buffered.length < requestEnd) return;
+        }
+        buffered = buffered.slice(requestEnd);
+
+        const target = head.split("\r\n")[0].split(" ")[1];
+        const [path, query] = target.split("?");
+        const response = responses[path];
+        if (response) {
+          socket.write(response.head + (query === "junk" ? response.junk : ""));
+        } else {
+          socket.write(legit);
+        }
+      }
+    });
+  });
+  // 127.0.0.1 explicitly: counting connections requires the server to listen on the
+  // address fetch() connects to.
+  await once(server.listen(0, "127.0.0.1"), "listening");
+  const { port } = server.address() as AddressInfo;
+  const origin = `http://127.0.0.1:${port}`;
+
+  const cases: { path: string; init?: () => RequestInit }[] = [
+    { path: "/204" },
+    { path: "/304" },
+    { path: "/empty" },
+    { path: "/head", init: () => ({ method: "HEAD" }) },
+    // The redirect path only pools the socket once the upload is known to be
+    // complete, which today is the case for streamed bodies, so a streamed POST is
+    // what exercises that path's pooling decision.
+    {
+      path: "/303",
+      init: () => ({
+        method: "POST",
+        body: new ReadableStream({
+          start(controller) {
+            controller.enqueue(new TextEncoder().encode("payload"));
+            controller.close();
+          },
+        }),
+      }),
+    },
+  ];
+
+  try {
+    // Warm the pool with one clean connection so every case below starts out
+    // reusing a pooled socket.
+    expect(await (await fetch(`${origin}/legit`)).text()).toBe("legit!");
+    expect(connections).toBe(1);
+
+    const results: { request: string; status: number; body: string; newConnections: number }[] = [];
+    for (const { path, init } of cases) {
+      for (const junk of [true, false]) {
+        const before = connections;
+        const response = await fetch(`${origin}${path}${junk ? "?junk" : ""}`, init?.());
+        const body = await response.text();
+        // The follow-up request must never be served by the connection that
+        // carried the mis-framed response (and whatever else it may still deliver),
+        // while a cleanly framed bodyless response must keep its connection pooled.
+        const followUp = await fetch(`${origin}/legit`);
+        expect(await followUp.text()).toBe("legit!");
+        results.push({
+          request: `${path}${junk ? " + trailing bytes" : ""}`,
+          status: response.status,
+          body,
+          newConnections: connections - before,
+        });
+      }
+    }
+
+    expect(results).toEqual([
+      { request: "/204 + trailing bytes", status: 204, body: "", newConnections: 1 },
+      { request: "/204", status: 204, body: "", newConnections: 0 },
+      { request: "/304 + trailing bytes", status: 304, body: "", newConnections: 1 },
+      { request: "/304", status: 304, body: "", newConnections: 0 },
+      { request: "/empty + trailing bytes", status: 200, body: "", newConnections: 1 },
+      { request: "/empty", status: 200, body: "", newConnections: 0 },
+      { request: "/head + trailing bytes", status: 200, body: "", newConnections: 1 },
+      { request: "/head", status: 200, body: "", newConnections: 0 },
+      // The redirect is still followed; only the hop to /legit needs the new connection.
+      { request: "/303 + trailing bytes", status: 200, body: "legit!", newConnections: 1 },
+      { request: "/303", status: 200, body: "legit!", newConnections: 0 },
+    ]);
+  } finally {
+    for (const socket of sockets) socket.destroy();
+    server.close();
+  }
+});
+
 // https://github.com/oven-sh/bun/issues/16682
 it("an explicit numeric `timeout` extends the socket idle deadline past the default", async () => {
   // The child runs with a 1s idle default (BUN_CONFIG_HTTP_IDLE_TIMEOUT=1) and