From fc0e8230ba829408c9a232ebbf3d22362b3c4870 Mon Sep 17 00:00:00 2001 From: yhirose Date: Fri, 16 Oct 2026 05:32:29 -0400 Subject: [PATCH] Close the connection after a partly read request body When a content receiver returned false, or threw, partway through a body, the drain after the response read a whole Content-Length again from where it stopped. It swallowed the start of the next request, so the rest was parsed as a request of its own once the handler answered below 400. A handler that called the content reader a second time read into the next request the same way. Mark the body once reading starts, and if it does not finish, answer with Connection: close and never drain it from the middle. Let each request's body be read only once. --- httplib.h | 19 ++++++++++++--- test/test.cc | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/httplib.h b/httplib.h index eeba6d51..6e6b3812 100644 --- a/httplib.h +++ b/httplib.h @@ -1811,6 +1811,7 @@ struct Request { bool is_multipart_form_data() const; // private members... + bool body_read_started_ = false; bool body_consumed_ = false; bool expect_100_continue_pending_ = false; size_t redirect_count_ = CPPHTTPLIB_REDIRECT_MAX_COUNT; @@ -13586,7 +13587,10 @@ inline bool Server::write_response_core(Stream &strm, bool close_connection, 400 <= res.status || // Don't leave connections open after errors // The client withholds the body until `100 Continue`, which was never // sent, so whether and when the body follows is unknown. - (req.expect_100_continue_pending_ && detail::has_framed_body(req))) { + (req.expect_100_continue_pending_ && detail::has_framed_body(req)) || + // The body was left partway, e.g. by a content receiver that returned + // false, so the rest of it cannot be told from the next request. + (req.body_read_started_ && !req.body_consumed_)) { res.set_header("Connection", "close"); } else { std::string s = "timeout="; @@ -13858,6 +13862,10 @@ inline bool Server::read_content_core( } #endif + // A second read would start where the first one stopped, or in the next + // request. + if (req.body_read_started_) { return false; } + // The client is waiting for this before it sends the body. if (req.expect_100_continue_pending_) { req.expect_100_continue_pending_ = false; @@ -13865,6 +13873,9 @@ inline bool Server::read_content_core( strm.write("\r\n"); } + // Set before reading, since a receiver may also stop by throwing. + req.body_read_started_ = true; + if (!detail::read_content(strm, req, payload_max_length_, res.status, nullptr, out, true)) { return false; @@ -14938,11 +14949,13 @@ Server::process_request(Stream &strm, const std::string &remote_addr, // Drain any unconsumed framed body to prevent request smuggling on // keep-alive. Without framing there is no body to drain — reading would // consume the next request (issue #2450). If the response has committed the - // connection to close, there is no next request to protect. + // connection to close, there is no next request to protect. A body read in + // part cannot be drained from where it stopped, so the connection closes. if (!req.body_consumed_ && detail::has_framed_body(req) && !detail::has_header_token(res.headers, "Connection", "close")) { int dummy_status; - if (!detail::read_content( + if (req.body_read_started_ || + !detail::read_content( strm, req, payload_max_length_, dummy_status, nullptr, [](const char *, size_t, size_t, size_t) { return true; }, false)) { connection_closed = true; diff --git a/test/test.cc b/test/test.cc index 749f73c3..0cb52a89 100644 --- a/test/test.cc +++ b/test/test.cc @@ -26689,6 +26689,74 @@ TEST(RequestSmugglingTest, ErrorResponseClosesConnection) { EXPECT_NE(std::string::npos, first_response.find("Connection: close")); } +TEST(RequestSmugglingTest, AbortedContentReaderClosesConnection) { + // A content reader that stops early leaves the stream inside the body. + // Draining the whole Content-Length from there, or reading the body again, + // reads into the next request, and whatever follows is parsed as a request + // of its own. + Server svr; + std::atomic smuggled_count(0); + svr.Get("/smuggled", [&](const Request &, Response &) { smuggled_count++; }); + svr.Post("/upload", [](const Request &req, Response &res, + const ContentReader &content_reader) { + // Stop at the last chunk, so the whole body has been read however it + // arrives. + auto length = req.get_header_value_u64("Content-Length"); + size_t total = 0; + auto consumed = content_reader([&](const char *, size_t n) { + total += n; + return total < length; + }); + EXPECT_FALSE(consumed); + if (req.has_param("retry")) { + EXPECT_FALSE(content_reader([](const char *, size_t) { return true; })); + } + // Replace the 400 the aborted read set, which would close the connection. + res.set_redirect("/done"); + }); + + auto port = svr.bind_to_any_port(HOST); + thread t = thread([&] { svr.listen_after_bind(); }); + auto se = detail::scope_exit([&] { + svr.stop(); + t.join(); + }); + svr.wait_until_ready(); + + // A front end sees the second request as a POST whose body is the GET. The + // first body is as long as the POST's headers, so draining it again reads + // exactly those and leaves the GET to be parsed next. + std::string inner = "GET /smuggled HTTP/1.1\r\n" + "Host: localhost\r\n" + "\r\n"; + std::string second_head = "POST /post HTTP/1.1\r\n" + "Host: localhost\r\n" + "Content-Length: " + + std::to_string(inner.size()) + + "\r\n" + "\r\n"; + for (auto target : {"/upload", "/upload?retry"}) { + smuggled_count = 0; + std::string request = "POST " + std::string(target) + + " HTTP/1.1\r\n" + "Host: localhost\r\n" + "Content-Length: " + + std::to_string(second_head.size()) + + "\r\n" + "\r\n" + + std::string(second_head.size(), 'A') + second_head + + inner; + + std::string response; + ASSERT_TRUE(send_request(2, request, &response, port)) << target; + + EXPECT_EQ(0u, response.find("HTTP/1.1 302")) << target; + EXPECT_NE(std::string::npos, response.find("Connection: close")) << target; + EXPECT_EQ(std::string::npos, response.find("HTTP/1.1", 1)) << target; + EXPECT_EQ(0, smuggled_count.load()) << target; + } +} + TEST(RequestSmugglingTest, ContentLengthAndTransferEncodingRejected) { // RFC 9112 §6.3: A request with both Content-Length and Transfer-Encoding // must be rejected with 400 Bad Request.