reject ambiguously framed responses in client read paths (#2581)

* reject ambiguously framed responses in client read paths

* Accept non-chunked Transfer-Encoding responses in the client framing guard

RFC 9112 §6.3 treats requests and responses differently when the final
transfer coding is not chunked: a request's body length cannot be
determined and the server must answer 400, but a response's body simply
runs until the server closes the connection. read_content() and the
open_stream() body reader already do that, so such a response is not
ambiguous and rejecting it broke valid responses such as
"Transfer-Encoding: gzip" followed by a close.

Keep rejecting a Transfer-Encoding paired with a non-zero Content-Length,
which is the actual ambiguity, and drop the non-chunked clause from both
client read paths.

Tests: check that rejection surfaces as Error::Read, that a non-chunked
Transfer-Encoding response is read until close on both paths, and that
HEAD, 204 and 304 responses with both framing headers are not rejected.

Claude-Session: https://claude.ai/code/session_01JYPWKpbp4a881EdpEf2xSi

* Share the framing check and reuse existing test helpers

Factor "Transfer-Encoding with a non-zero Content-Length" into
detail::has_conflicting_content_length() next to
is_chunked_transfer_encoding(), and call it from the server request
guard and both client read paths so the rule and its RFC 9112 §6.3
rationale live in one place.

In the tests, drop the POSIX-only raw socket helper in favour of the
existing serve_single_response() and read_all(), which also lets the
tests run on Windows. Fold the stream-only test into the buffered one so
each case checks both Get() and open_stream(), and cover the HEAD/204/304
exclusion on the open_stream() path too.

Claude-Session: https://claude.ai/code/session_01JYPWKpbp4a881EdpEf2xSi

---------

Co-authored-by: yhirose <yuji.hirose.bug@gmail.com>

httplib.h

@@ -8365,6 +8365,17 @@ inline bool is_chunked_transfer_encoding(const Headers &headers) {
   return case_ignore::equal(last_coding, "chunked");
 }
 
+inline bool has_conflicting_content_length(const Headers &headers) {
+  // RFC 9112 §6.3: a message carrying both Transfer-Encoding and a non-zero
+  // Content-Length is framed ambiguously. The body readers here delimit it by
+  // the transfer coding and drop Content-Length, while an intermediary may do
+  // the reverse, so the two disagree on where the body ends and a reused
+  // connection is desynchronised (request/response smuggling). Content-Length:
+  // 0 is tolerated for compatibility with existing peers.
+  return has_header(headers, "Transfer-Encoding") &&
+         get_header_value_u64(headers, "Content-Length", 0, 0) > 0;
+}
+
 template <typename T, typename U>
 bool prepare_content_receiver(T &x, int &status,
                               ContentReceiverWithProgress receiver,
@@ -14275,8 +14286,8 @@ Server::process_request(Stream &strm, const std::string &remote_addr,
   // coding is not chunked, which leaves the body length undeterminable. The
   // latter must not fall through to the "no body" path, or the body bytes are
   // parsed as the next request on a persistent connection.
-  if (req.has_header("Transfer-Encoding") &&
-      (req.get_header_value_u64("Content-Length") > 0 ||
+  if (detail::has_conflicting_content_length(req.headers) ||
+      (req.has_header("Transfer-Encoding") &&
        !detail::is_chunked_transfer_encoding(req.headers))) {
     connection_closed = true;
     res.status = StatusCode::BadRequest_400;
@@ -15091,6 +15102,17 @@ ClientImpl::open_stream(const std::string &method, const std::string &path,
     return handle;
   }
 
+  // Same framing check as ClientImpl::process_request(). A HEAD or bodyless
+  // (204/304) response legitimately carries framing headers with no body.
+  if (method != "HEAD" &&
+      handle.response->status != StatusCode::NoContent_204 &&
+      handle.response->status != StatusCode::NotModified_304 &&
+      detail::has_conflicting_content_length(handle.response->headers)) {
+    handle.error = Error::Read;
+    handle.response.reset();
+    return handle;
+  }
+
   handle.body_reader_.stream = handle.stream_;
   handle.body_reader_.payload_max_length = payload_max_length_;
 
@@ -16035,6 +16057,17 @@ inline bool ClientImpl::process_request(Stream &strm, Request &req,
   // Body
   if ((res.status != StatusCode::NoContent_204) && req.method != "HEAD" &&
       req.method != "CONNECT") {
+    // Reject ambiguous framing (RFC 9112 §6.3). Unlike a request, a response
+    // whose final transfer coding is not chunked is not ambiguous: its body
+    // runs until the server closes the connection, so it is not rejected.
+    // HEAD/204 are excluded above and a 304 carries no body.
+    if (res.status != StatusCode::NotModified_304 &&
+        detail::has_conflicting_content_length(res.headers)) {
+      error = Error::Read;
+      output_error_log(error, &req);
+      return false;
+    }
+
     auto redirect = 300 < res.status && res.status < 400 &&
                     res.status != StatusCode::NotModified_304 &&
                     follow_location_;

test/test.cc

@@ -19863,6 +19863,105 @@ TEST(OpenStreamMalformedContentLength, OutOfRange) {
   server_thread.join();
 }
 
+// Serves `response` to the single request `fn` makes with a fresh client.
+template <typename Fn>
+static void with_single_response(const std::string &response, Fn fn) {
+#ifndef _WIN32
+  signal(SIGPIPE, SIG_IGN);
+#endif
+
+  std::promise<int> port_promise;
+  auto port_future = port_promise.get_future();
+  auto server_thread = serve_single_response(port_promise, response);
+  auto se = detail::scope_exit([&] { server_thread.join(); });
+
+  auto port = port_future.get();
+  ASSERT_GT(port, 0);
+  Client cli("127.0.0.1", port);
+  fn(cli);
+}
+
+// RFC 9112 §6.3: a response that pairs a non-zero Content-Length with
+// Transfer-Encoding is framed ambiguously. Both read paths delimit its body by
+// the chunked coding and drop Content-Length, so a front-end that trusts
+// Content-Length would disagree about where the body ends (response
+// smuggling). The client must reject such a response.
+TEST(ClientResponseSmugglingTest, ContentLengthAndTransferEncodingRejected) {
+  for (const char *te : {"chunked", "gzip, chunked"}) {
+    auto response = std::string("HTTP/1.1 200 OK\r\n") +
+                    "Content-Length: 5\r\n" + "Transfer-Encoding: " + te +
+                    "\r\n" + "Connection: close\r\n" + "\r\n" +
+                    "5\r\nhello\r\n0\r\n\r\n";
+
+    with_single_response(response, [&](Client &cli) {
+      auto res = cli.Get("/");
+      EXPECT_FALSE(static_cast<bool>(res)) << te;
+      EXPECT_EQ(Error::Read, res.error()) << te;
+    });
+
+    with_single_response(response, [&](Client &cli) {
+      auto handle = cli.open_stream("GET", "/");
+      EXPECT_FALSE(handle.is_valid()) << te;
+      EXPECT_EQ(Error::Read, handle.error) << te;
+    });
+  }
+}
+
+// Unambiguous framing stays readable on both paths: chunked alone, and a final
+// coding other than chunked, whose body RFC 9112 §6.3 delimits by the server
+// closing the connection (unlike a request, which must be rejected).
+TEST(ClientResponseSmugglingTest, UnambiguousFramingAccepted) {
+  for (const char *response : {"HTTP/1.1 200 OK\r\n"
+                               "Transfer-Encoding: chunked\r\n"
+                               "Connection: close\r\n"
+                               "\r\n"
+                               "5\r\nhello\r\n0\r\n\r\n",
+                               "HTTP/1.1 200 OK\r\n"
+                               "Transfer-Encoding: gzip\r\n"
+                               "Connection: close\r\n"
+                               "\r\n"
+                               "hello"}) {
+    with_single_response(response, [&](Client &cli) {
+      auto res = cli.Get("/");
+      ASSERT_TRUE(static_cast<bool>(res)) << response;
+      EXPECT_EQ("hello", res->body);
+    });
+
+    with_single_response(response, [&](Client &cli) {
+      auto handle = cli.open_stream("GET", "/");
+      ASSERT_TRUE(handle.is_valid()) << response;
+      EXPECT_EQ("hello", read_all(handle));
+    });
+  }
+}
+
+// A response to HEAD, and a 204 or 304 response, carries no body, so its
+// framing headers describe nothing to read and must not be rejected.
+TEST(ClientResponseSmugglingTest, BodylessResponseNotRejected) {
+  const std::string framing = "Content-Length: 5\r\n"
+                              "Transfer-Encoding: chunked\r\n"
+                              "Connection: close\r\n"
+                              "\r\n";
+
+  with_single_response("HTTP/1.1 200 OK\r\n" + framing, [](Client &cli) {
+    EXPECT_TRUE(static_cast<bool>(cli.Head("/")));
+  });
+  with_single_response("HTTP/1.1 200 OK\r\n" + framing, [](Client &cli) {
+    EXPECT_TRUE(cli.open_stream("HEAD", "/").is_valid());
+  });
+
+  for (const char *status : {"204 No Content", "304 Not Modified"}) {
+    auto response = std::string("HTTP/1.1 ") + status + "\r\n" + framing;
+
+    with_single_response(response, [&](Client &cli) {
+      EXPECT_TRUE(static_cast<bool>(cli.Get("/"))) << status;
+    });
+    with_single_response(response, [&](Client &cli) {
+      EXPECT_TRUE(cli.open_stream("GET", "/").is_valid()) << status;
+    });
+  }
+}
+
 #ifdef CPPHTTPLIB_ZLIB_SUPPORT
 TEST_F(OpenStreamTest, Gzip) {
   Client cli("127.0.0.1", port_);