Remove semantically redundant closed_ checks in ping thread. (#2613)

A 303 response turns the follow-up request into a GET, but only the buffered body and headers were cleared. A content provider (sized or chunked) stayed on the request, so the original payload was sent again, and for a chunked provider the unframed chunks also broke the keep-alive connection.

The redirect also percent-decoded the Location path before sending it. That turned %23, %3F and %25 into a fragment, a query delimiter and a different octet, so the client requested a different resource than the one named, and made set_path_encode(false) fail on any Location containing %20. The path is now sent as given.

httplib.h

@@ -4377,7 +4377,7 @@ public:
 
 private:
   bool parse_sse_line(const std::string &line, SSEMessage &msg, int &retry_ms,
-                      bool &has_data);
+                      bool &has_data, bool &has_id);
   void run_event_loop();
   void dispatch_event(const SSEMessage &msg);
   bool should_reconnect(int count) const;
@@ -4887,7 +4887,8 @@ inline void SSEClient::stop() {
 }
 
 inline bool SSEClient::parse_sse_line(const std::string &line, SSEMessage &msg,
-                                      int &retry_ms, bool &has_data) {
+                                      int &retry_ms, bool &has_data,
+                                      bool &has_id) {
   // Blank line signals end of event
   if (line.empty()) { return true; }
 
@@ -4917,6 +4918,7 @@ inline bool SSEClient::parse_sse_line(const std::string &line, SSEMessage &msg,
   } else if (field == "id") {
     // Empty id is valid (clears the last event ID)
     msg.id = value;
+    has_id = true;
   } else if (field == "retry") {
     // Parse retry interval in milliseconds
     // Per the SSE spec, a value that is not all ASCII digits is ignored.
@@ -4987,7 +4989,8 @@ inline void SSEClient::run_event_loop() {
     // Event receiving loop
     std::string buffer;
     SSEMessage current_msg;
-    bool has_data = false;
+    auto has_data = false;
+    auto has_id = false;
 
     while (running_.load() && result.next()) {
       buffer.append(result.data(), result.size());
@@ -5006,13 +5009,13 @@ inline void SSEClient::run_event_loop() {
         if (!line.empty() && line.back() == '\r') { line.pop_back(); }
 
         // Parse the line and check if event is complete
-        auto event_complete =
+        auto event_complete = parse_sse_line(
-            parse_sse_line(line, current_msg, reconnect_interval_ms_, has_data);
+            line, current_msg, reconnect_interval_ms_, has_data, has_id);
 
         if (event_complete) {
           // Update last_event_id for reconnection, even for an event that
-          // has no data
+          // has no data. An empty id clears it.
-          if (!current_msg.id.empty()) { last_event_id_ = current_msg.id; }
+          if (has_id) { last_event_id_ = current_msg.id; }
 
           // An event without a data field is not dispatched
           if (has_data) { dispatch_event(current_msg); }
@@ -5020,6 +5023,7 @@ inline void SSEClient::run_event_loop() {
           // Reset the message for the next event either way
           current_msg.clear();
           has_data = false;
+          has_id = false;
         }
       }
 
@@ -8972,6 +8976,9 @@ inline bool redirect(T &cli, Request &req, Response &res,
     new_req.method = "GET";
     new_req.body.clear();
     new_req.headers.clear();
+    new_req.content_length_ = 0;
+    new_req.content_provider_ = nullptr;
+    new_req.is_chunked_content_provider_ = false;
   }
 
   Response new_res;
@@ -15734,7 +15741,7 @@ inline bool ClientImpl::redirect(Request &req, Response &res, Error &error) {
   if (next_host.empty()) { next_host = host_; }
   if (next_path.empty()) { next_path = "/"; }
 
-  auto path = decode_path_component(next_path) + next_query;
+  auto path = std::move(next_path) + next_query;
 
   // Same host redirect - use current client
   if (next_scheme == scheme && next_host == host_ && next_port == port_) {
@@ -15989,9 +15996,9 @@ inline bool ClientImpl::write_request(Stream &strm, Request &req,
     // Write request line and headers
     if (detail::write_request_line(bstrm, req.method, path_with_query) < 0) {
       // A rejected method (not a token, e.g. carrying CR/LF) or target (e.g.
-      // CR/LF smuggled in via a decoded redirect Location under
+      // CR/LF in a caller-supplied path under set_path_encode(false)) must
-      // set_path_encode(false)) must fail the request cleanly instead of
+      // fail the request cleanly instead of emitting a request-line-less,
-      // emitting a request-line-less, header-injecting request.
+      // header-injecting request.
       error = Error::Write;
       rejected_locally = true;
       output_error_log(error, &req);
@@ -22823,9 +22830,9 @@ inline void WebSocket::start_heartbeat() {
   if (ping_interval_sec_ == 0) { return; }
   ping_thread_ = std::thread([this]() {
     std::unique_lock<std::mutex> lock(ping_mutex_);
-    while (!closed_) {
+    // The predicate keeps a spurious wakeup from sending a ping early
-      ping_cv_.wait_for(lock, std::chrono::seconds(ping_interval_sec_));
+    while (!ping_cv_.wait_for(lock, std::chrono::seconds(ping_interval_sec_),
-      if (closed_) { break; }
+                              [this]() { return closed_.load(); })) {
       // If the peer has failed to respond to the previous pings, give up.
       // RFC 6455 does not define a pong-timeout mechanism; this is an
       // opt-in liveness check controlled by max_missed_pongs_.

test/test.cc

@@ -10209,8 +10209,7 @@ TEST(RequestLineInjectionTest, RejectsInvalidCharsInTarget) {
 
   // A target carrying CR/LF, SP or other control octets must be rejected
   // before anything reaches the wire, otherwise it splits the request line and
-  // injects a header or a whole request. This is what a decoded redirect
+  // injects a header or a whole request.
-  // Location ("%0D%0A") turns into when path encoding is disabled.
   const std::string evil_targets[] = {
       "/a\r\nInjected: pwned",
       "/a\rInjected",
@@ -10231,9 +10230,9 @@ TEST(RequestLineInjectionTest, RejectsInvalidCharsInTarget) {
 TEST(RequestLineInjectionTest, ClientRejectsCRLFTargetEndToEnd) {
   // End-to-end counterpart to RejectsInvalidCharsInTarget. With path encoding
   // disabled the client transmits the target verbatim, so a CR/LF-bearing
-  // target -- what a redirect Location "%0D%0A" decodes to -- reaches
+  // target reaches write_request. The client must fail cleanly with
-  // write_request. The client must fail cleanly with Error::Write instead of
+  // Error::Write instead of putting a request-line-less, header-injecting
-  // putting a request-line-less, header-injecting request on the wire.
+  // request on the wire.
   Server svr;
 
   svr.Get("/a", [](const Request &, Response &res) {
@@ -17496,6 +17495,138 @@ TEST(TaskQueueTest, MaxQueuedRequests) {
 #endif
 }
 
+TEST(RedirectTest, SeeOtherDoesNotResendContentProviderBody) {
+  Server svr;
+  std::string method;
+  std::string body;
+  auto has_content_length = false;
+  auto has_transfer_encoding = false;
+  std::atomic<int> bad_requests{0};
+
+  // Leftover body bytes get parsed as a malformed request and answered with
+  // 400
+  svr.set_logger([&](const Request & /*req*/, const Response &res) {
+    if (res.status == StatusCode::BadRequest_400) { bad_requests++; }
+  });
+
+  svr.Post("/up", [](const Request & /*req*/, Response &res) {
+    res.set_redirect("/down", StatusCode::SeeOther_303);
+  });
+  svr.Get("/down", [&](const Request &req, Response &res) {
+    method = req.method;
+    body = req.body;
+    has_content_length = req.has_header("Content-Length");
+    has_transfer_encoding = req.has_header("Transfer-Encoding");
+    res.set_content("ok", "text/plain");
+  });
+
+  auto port = svr.bind_to_any_port(HOST);
+  auto thread = std::thread([&]() { svr.listen_after_bind(); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    thread.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+  svr.wait_until_ready();
+
+  const std::string payload = "SECRET-BODY";
+
+  auto check = [&](Client &cli, const Result &res) {
+    ASSERT_TRUE(res) << "Error: " << to_string(res.error());
+    EXPECT_EQ(StatusCode::OK_200, res->status);
+    EXPECT_EQ("ok", res->body);
+    EXPECT_EQ("GET", method);
+    EXPECT_TRUE(body.empty());
+    EXPECT_FALSE(has_content_length);
+    EXPECT_FALSE(has_transfer_encoding);
+
+    // Nothing of the original body may be left on the connection
+    auto res2 = cli.Get("/down");
+    ASSERT_TRUE(res2) << "Error: " << to_string(res2.error());
+    EXPECT_EQ(StatusCode::OK_200, res2->status);
+    EXPECT_EQ("ok", res2->body);
+    EXPECT_EQ(0, bad_requests);
+  };
+
+  // With content length
+  {
+    Client cli(HOST, port);
+    cli.set_keep_alive(true);
+    cli.set_follow_location(true);
+
+    auto res = cli.Post(
+        "/up", payload.size(),
+        [&](size_t offset, size_t length, DataSink &sink) {
+          return sink.write(payload.data() + offset, length);
+        },
+        "text/plain");
+    check(cli, res);
+  }
+
+  // Without content length (chunked)
+  {
+    Client cli(HOST, port);
+    cli.set_keep_alive(true);
+    cli.set_follow_location(true);
+
+    auto res = cli.Post(
+        "/up",
+        [&](size_t /*offset*/, DataSink &sink) {
+          sink.write(payload.data(), payload.size());
+          sink.done();
+          return true;
+        },
+        "text/plain");
+    check(cli, res);
+  }
+}
+
+TEST(RedirectTest, LocationPathIsNotDecoded) {
+  Server svr;
+  std::string target;
+
+  svr.Get(R"(/start/.*)", [](const Request &req, Response &res) {
+    // Echo the still-encoded name back in the Location
+    const std::string prefix = "/start/";
+    res.status = StatusCode::Found_302;
+    res.set_header("Location", "/dest/" + req.target.substr(prefix.size()));
+  });
+  svr.Get(R"(/dest.*)", [&](const Request &req, Response &res) {
+    target = req.target;
+    res.set_content("ok", "text/plain");
+  });
+
+  auto port = svr.bind_to_any_port(HOST);
+  auto thread = std::thread([&]() { svr.listen_after_bind(); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    thread.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+  svr.wait_until_ready();
+
+  // Decoding the Location path would turn the first four into a path
+  // separator, a query delimiter, a fragment delimiter and a different
+  // percent-encoded octet. The rest must keep reaching the server as they are.
+  const std::vector<std::string> names = {
+      "a%2Fb", "x%3Fy", "x%23y", "a%2520b", "a%20b", "%C3%BC", "a-b_c.d~e",
+  };
+
+  for (auto path_encode : {true, false}) {
+    Client cli(HOST, port);
+    cli.set_follow_location(true);
+    cli.set_path_encode(path_encode);
+
+    for (const auto &name : names) {
+      target.clear();
+      auto res = cli.Get("/start/" + name);
+      ASSERT_TRUE(res) << "Error: " << to_string(res.error());
+      EXPECT_EQ(StatusCode::OK_200, res->status);
+      EXPECT_EQ("/dest/" + name, target);
+    }
+  }
+}
+
 TEST(RedirectTest, RedirectToUrlWithQueryParameters) {
   Server svr;
 
@@ -22470,6 +22601,30 @@ TEST_F(SSEParsingTest, EventWithoutDataUpdatesLastEventId) {
   EXPECT_EQ(msgs[0].data, "42");
 }
 
+TEST_F(SSEParsingTest, EmptyEventIdClearsLastEventId) {
+  // The first connection sets an id and then clears it with an empty one; the
+  // second reports whether it was reconnected with a Last-Event-ID
+  std::atomic<int> connection_count{0};
+  server_->Get("/id-clear", [&](const Request &req, Response &res) {
+    if (connection_count++ == 0) {
+      res.set_content("id: 1\ndata: first\n\nid:\ndata: second\n\n",
+                      "text/event-stream");
+    } else {
+      res.set_content(
+          std::string("data: ") +
+              (req.has_header("Last-Event-ID") ? "sent" : "not sent") +
+              "\n\nevent: end\ndata: end\n\n",
+          "text/event-stream");
+    }
+  });
+
+  auto msgs = collect("/id-clear");
+  ASSERT_EQ(msgs.size(), 3u);
+  EXPECT_EQ(msgs[0].id, "1");
+  EXPECT_EQ(msgs[1].id, "");
+  EXPECT_EQ(msgs[2].data, "not sent");
+}
+
 TEST_F(SSEParsingTest, CompleteEventParsing) {
   auto msgs = parse("event: notification\nid: evt-42\n"
                     "data: {\"type\":\"alert\"}\nretry: 1000\n\n");