Reject CR/LF in the request target and fail the request cleanly (#2501)

* reject crlf in request target in write_request_line

* declare write_request_line in test.cc for split builds

split.py strips `inline` and moves the definition into httplib.cc, so
detail::write_request_line is not visible from the split httplib.h and
test_split failed to compile. Re-declare it in test.cc, matching what the
base64_encode and getaddrinfo_with_timeout tests already do.

* Fail the request when write_request_line rejects the target

The CR/LF guard in write_request_line returns -1, but ClientImpl::write_request
ignored that return value. A rejected target therefore produced a request-line-
less request (headers only) that the client silently reported as a successful
send. This is the primary path reachable via a decoded redirect Location under
set_path_encode(false), and it also carries the CONNECT target.

Check the return value like ClientImpl::open_stream already does and fail with
Error::Write. Add an end-to-end test asserting the client refuses a CR/LF target
instead of putting it on the wire.

---------

Co-authored-by: Sayed Kaif <metsw24@gmail.com>

httplib.h

@@ -7542,6 +7542,13 @@ bool read_content(Stream &strm, T &x, size_t payload_max_length, int &status,
 
 inline ssize_t write_request_line(Stream &strm, const std::string &method,
                                   const std::string &path) {
+  // A request target must not carry CR/LF (or other control octets); otherwise
+  // a value smuggled into it splits the request line and injects headers or a
+  // whole request. The same field-value check already guards header values in
+  // check_and_write_headers and the request target in
+  // perform_websocket_handshake; apply it here too.
+  if (!fields::is_field_value(path)) { return -1; }
+
   std::string s = method;
   s += ' ';
   s += path;
@@ -13849,7 +13856,14 @@ inline bool ClientImpl::write_request(Stream &strm, Request &req,
     }
 
     // Write request line and headers
-    detail::write_request_line(bstrm, req.method, path_with_query);
+    if (detail::write_request_line(bstrm, req.method, path_with_query) < 0) {
+      // A rejected target (e.g. CR/LF smuggled in via a decoded redirect
+      // Location under set_path_encode(false)) must fail the request cleanly
+      // instead of emitting a request-line-less, header-injecting request.
+      error = Error::Write;
+      output_error_log(error, &req);
+      return false;
+    }
     if (!detail::check_and_write_headers(bstrm, req.headers, header_writer_,
                                          error)) {
       output_error_log(error, &req);

test/test.cc

@@ -7938,6 +7938,80 @@ TEST(ServerResponseSplittingTest, ChunkedTrailerCRLFInjection) {
   EXPECT_NE(std::string::npos, res.find("X-Safe: safe"));
 }
 
+// Forward declaration: in split builds split.py strips `inline` and moves the
+// definition into httplib.cc, so detail::write_request_line is not visible from
+// the public httplib.h. Re-declaring it here lets the tests link against the
+// symbol in both header-only and split builds.
+namespace httplib {
+namespace detail {
+ssize_t write_request_line(Stream &strm, const std::string &method,
+                           const std::string &path);
+} // namespace detail
+} // namespace httplib
+
+TEST(RequestLineInjectionTest, RejectsCRLFInTarget) {
+  // A well-formed target is written verbatim.
+  {
+    detail::BufferStream strm;
+    auto n = detail::write_request_line(strm, "GET", "/path?a=b");
+    EXPECT_GT(n, 0);
+    EXPECT_EQ("GET /path?a=b HTTP/1.1\r\n", strm.get_buffer());
+  }
+
+  // A target carrying CR/LF 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 Location ("%0D%0A") turns into
+  // when path encoding is disabled.
+  const std::string evil_targets[] = {
+      "/a\r\nInjected: pwned",
+      "/a\rInjected",
+      "/a\nInjected",
+      "/a\r\n\r\nGET /evil HTTP/1.1\r\nHost: victim\r\n\r\n",
+  };
+  for (const auto &evil : evil_targets) {
+    detail::BufferStream strm;
+    auto n = detail::write_request_line(strm, "GET", evil);
+    EXPECT_LT(n, 0);
+    EXPECT_TRUE(strm.get_buffer().empty());
+  }
+}
+
+TEST(RequestLineInjectionTest, ClientRejectsCRLFTargetEndToEnd) {
+  // End-to-end counterpart to RejectsCRLFInTarget. 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 write_request. The
+  // client must fail cleanly with Error::Write instead of putting a
+  // request-line-less, header-injecting request on the wire.
+  Server svr;
+
+  svr.Get("/a", [](const Request &, Response &res) {
+    res.set_content("ok", "text/plain");
+  });
+
+  auto thread = std::thread([&]() { svr.listen(HOST, PORT); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    thread.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+
+  svr.wait_until_ready();
+
+  {
+    Client cli(HOST, PORT);
+    cli.set_path_encode(false);
+    // The request is rejected before anything is written, so the connection
+    // stays silent and the subsequent response read would otherwise block for
+    // the full default read timeout. Shorten it -- the assertion is on the
+    // error, not the timeout.
+    cli.set_read_timeout(1, 0);
+
+    auto res = cli.Get("/a\r\nInjected: pwned");
+    EXPECT_FALSE(res);
+    EXPECT_EQ(Error::Write, res.error());
+  }
+}
+
 // Sends a raw request and verifies that there isn't a crash or exception.
 static void test_raw_request(const std::string &req,
                              std::string *out = nullptr) {