Preserve the order of header fields with the same name (#2520)

RFC 9110 5.3 makes the order of header fields sharing a field name
significant, but Headers was a std::unordered_multimap, which gives no
ordering guarantee for equivalent keys. libstdc++ hands duplicates back
in reverse insertion order while libc++ uses insertion order, so
get_header_value() returned a different field depending on the platform,
and code that picks a value out of an accidentally or maliciously
duplicated field name had no way to say which one it wanted.

Replace Headers with a small container that keeps the fields in the
order they were received or set. Storage is a flat vector and lookup is
a linear scan, which beats hashing for the at most
CPPHTTPLIB_HEADER_MAX_COUNT fields a message carries. begin()/end() walk
every field, while find() and equal_range() hand back the same iterator
type restricted to one field name; equality compares only the position,
so a restricted iterator still compares equal to end(). Erasing an
equal_range() therefore removes only the fields with that name and
leaves interleaved fields alone.

std::multimap was the smaller change but sorts by field name, which
would stop control data such as Host from leading the message. Instead
Host is now prepended via emplace_front() so it keeps its place at the
front of a request.

Two side effects worth noting: incrementing past the last field of a
name now saturates at end(), so an out-of-range id passed to
get_header_value() returns the default instead of running off the
container as it did before; and iterators follow std::vector rules, so
they are invalidated by insertion.

Fixes #2509

httplib.h

@@ -321,6 +321,7 @@ using socket_t = int;
 #include <functional>
 #include <iomanip>
 #include <iostream>
+#include <iterator>
 #include <list>
 #include <map>
 #include <memory>
@@ -333,9 +334,11 @@ using socket_t = int;
 #include <sys/stat.h>
 #include <system_error>
 #include <thread>
+#include <type_traits>
 #include <unordered_map>
 #include <unordered_set>
 #include <utility>
+#include <vector>
 
 // On macOS with a TLS backend, enable Keychain root certificates by default
 // unless the user explicitly opts out. Not enabled on iOS/tvOS/watchOS since
@@ -968,9 +971,263 @@ enum StatusCode {
   NetworkAuthenticationRequired_511 = 511,
 };
 
-using Headers =
+// RFC 9110 5.3: the order in which header fields with the same field name are
-    std::unordered_multimap<std::string, std::string, detail::case_ignore::hash,
+// received is significant, and a proxy must not reorder them. Neither
-                            detail::case_ignore::equal_to>;
+// std::unordered_multimap (no ordering guarantee at all for equivalent keys:
+// libstdc++ yields reverse insertion order, libc++ insertion order) nor
+// std::multimap (sorts by field name, so control data such as Host no longer
+// leads the message) can express that, so header fields are kept in a flat
+// vector in the order they were received or set. Lookup is a linear scan,
+// which beats hashing for the at most CPPHTTPLIB_HEADER_MAX_COUNT fields a
+// message carries.
+class Headers {
+public:
+  using key_type = std::string;
+  using mapped_type = std::string;
+  using value_type = std::pair<std::string, std::string>;
+  using size_type = std::size_t;
+  using difference_type = std::ptrdiff_t;
+  using reference = value_type &;
+  using const_reference = const value_type &;
+
+private:
+  static size_type npos() { return static_cast<size_type>(-1); }
+
+  // Iterating a Headers yields every field in insertion order, but
+  // equal_range() and find() have to walk only the fields sharing one name,
+  // which are not adjacent. Both are the same iterator type: key_idx_ selects
+  // between the two traversals, and since equality compares only the position,
+  // an iterator restricted to one name still compares equal to end().
+  template <typename V> class iterator_t {
+  public:
+    using iterator_category = std::bidirectional_iterator_tag;
+    using value_type = Headers::value_type;
+    using difference_type = Headers::difference_type;
+    using pointer = V *;
+    using reference = V &;
+
+    iterator_t() : data_(nullptr), idx_(0), size_(0), key_idx_(npos()) {}
+
+    template <typename U,
+              typename std::enable_if<std::is_convertible<U *, V *>::value,
+                                      int>::type = 0>
+    iterator_t(const iterator_t<U> &rhs)
+        : data_(rhs.data_), idx_(rhs.idx_), size_(rhs.size_),
+          key_idx_(rhs.key_idx_) {}
+
+    reference operator*() const { return data_[idx_]; }
+    pointer operator->() const { return data_ + idx_; }
+
+    iterator_t &operator++() {
+      // Saturating, so that advancing past the last field of a name (which
+      // get_header_value() does when asked for an out-of-range id) stays at
+      // end() instead of running off the container.
+      if (idx_ >= size_) { return *this; }
+      ++idx_;
+      if (key_idx_ != npos()) {
+        while (idx_ < size_ && !matches(idx_)) {
+          ++idx_;
+        }
+      }
+      return *this;
+    }
+
+    iterator_t operator++(int) {
+      auto tmp = *this;
+      ++*this;
+      return tmp;
+    }
+
+    iterator_t &operator--() {
+      if (idx_ == 0) { return *this; }
+      --idx_;
+      if (key_idx_ != npos()) {
+        while (idx_ > 0 && !matches(idx_)) {
+          --idx_;
+        }
+      }
+      return *this;
+    }
+
+    iterator_t operator--(int) {
+      auto tmp = *this;
+      --*this;
+      return tmp;
+    }
+
+    template <typename U> bool operator==(const iterator_t<U> &rhs) const {
+      return idx_ == rhs.idx_;
+    }
+
+    template <typename U> bool operator!=(const iterator_t<U> &rhs) const {
+      return idx_ != rhs.idx_;
+    }
+
+  private:
+    friend class Headers;
+    template <typename> friend class iterator_t;
+
+    iterator_t(V *data, size_type idx, size_type size, size_type key_idx)
+        : data_(data), idx_(idx), size_(size), key_idx_(key_idx) {}
+
+    bool matches(size_type i) const {
+      return detail::case_ignore::equal(data_[i].first, data_[key_idx_].first);
+    }
+
+    V *data_;
+    size_type idx_;
+    size_type size_;
+    size_type key_idx_;
+  };
+
+public:
+  using iterator = iterator_t<value_type>;
+  using const_iterator = iterator_t<const value_type>;
+
+  Headers() = default;
+  Headers(std::initializer_list<value_type> il) : entries_(il) {}
+  template <typename InputIt>
+  Headers(InputIt first, InputIt last) : entries_(first, last) {}
+
+  iterator begin() { return make_iter(0, npos()); }
+  iterator end() { return make_iter(entries_.size(), npos()); }
+  const_iterator begin() const { return make_citer(0, npos()); }
+  const_iterator end() const { return make_citer(entries_.size(), npos()); }
+  const_iterator cbegin() const { return begin(); }
+  const_iterator cend() const { return end(); }
+
+  bool empty() const { return entries_.empty(); }
+  size_type size() const { return entries_.size(); }
+  void clear() { entries_.clear(); }
+  void swap(Headers &rhs) { entries_.swap(rhs.entries_); }
+
+  iterator insert(const value_type &val) {
+    entries_.push_back(val);
+    return make_iter(entries_.size() - 1, npos());
+  }
+
+  iterator insert(value_type &&val) {
+    entries_.push_back(std::move(val));
+    return make_iter(entries_.size() - 1, npos());
+  }
+
+  template <typename... Args> iterator emplace(Args &&...args) {
+    entries_.emplace_back(std::forward<Args>(args)...);
+    return make_iter(entries_.size() - 1, npos());
+  }
+
+  // RFC 9110 5.3 recommends sending control data such as Host first.
+  template <typename... Args> iterator emplace_front(Args &&...args) {
+    entries_.emplace(entries_.begin(), std::forward<Args>(args)...);
+    return make_iter(0, npos());
+  }
+
+  iterator find(const std::string &key) {
+    auto i = index_of(key);
+    return i == npos() ? end() : make_iter(i, i);
+  }
+
+  const_iterator find(const std::string &key) const {
+    auto i = index_of(key);
+    return i == npos() ? end() : make_citer(i, i);
+  }
+
+  size_type count(const std::string &key) const {
+    size_type n = 0;
+    for (const auto &entry : entries_) {
+      if (detail::case_ignore::equal(entry.first, key)) { n++; }
+    }
+    return n;
+  }
+
+  std::pair<iterator, iterator> equal_range(const std::string &key) {
+    auto i = index_of(key);
+    return i == npos() ? std::make_pair(end(), end())
+                       : std::make_pair(make_iter(i, i), end());
+  }
+
+  std::pair<const_iterator, const_iterator>
+  equal_range(const std::string &key) const {
+    auto i = index_of(key);
+    return i == npos() ? std::make_pair(end(), end())
+                       : std::make_pair(make_citer(i, i), end());
+  }
+
+  size_type erase(const std::string &key) {
+    auto before = entries_.size();
+    entries_.erase(std::remove_if(entries_.begin(), entries_.end(),
+                                  [&](const value_type &entry) {
+                                    return detail::case_ignore::equal(
+                                        entry.first, key);
+                                  }),
+                   entries_.end());
+    return before - entries_.size();
+  }
+
+  iterator erase(const_iterator pos) {
+    entries_.erase(entries_.begin() + static_cast<difference_type>(pos.idx_));
+    return make_iter(pos.idx_, npos());
+  }
+
+  // Erases what iterating [first, last) would actually visit, so erasing an
+  // equal_range() removes only the fields with that name, not everything
+  // positioned between them.
+  iterator erase(const_iterator first, const_iterator last) {
+    auto from = first.idx_;
+    auto to = last.idx_;
+    if (from >= to) { return make_iter(from, npos()); }
+
+    auto begin_it = entries_.begin();
+    auto from_it = begin_it + static_cast<difference_type>(from);
+    auto to_it = begin_it + static_cast<difference_type>(to);
+
+    if (first.key_idx_ == npos()) {
+      entries_.erase(from_it, to_it);
+    } else {
+      auto key = entries_[first.key_idx_].first;
+      auto keep = from_it;
+      for (auto it = from_it; it != to_it; ++it) {
+        if (!detail::case_ignore::equal(it->first, key)) {
+          if (keep != it) { *keep = std::move(*it); }
+          ++keep;
+        }
+      }
+      if (keep != to_it) {
+        keep = std::move(to_it, entries_.end(), keep);
+      } else {
+        keep = entries_.end();
+      }
+      entries_.erase(keep, entries_.end());
+    }
+    return make_iter(from, npos());
+  }
+
+  friend bool operator==(const Headers &lhs, const Headers &rhs) {
+    return lhs.entries_ == rhs.entries_;
+  }
+
+  friend bool operator!=(const Headers &lhs, const Headers &rhs) {
+    return !(lhs == rhs);
+  }
+
+private:
+  size_type index_of(const std::string &key) const {
+    for (size_type i = 0; i < entries_.size(); i++) {
+      if (detail::case_ignore::equal(entries_[i].first, key)) { return i; }
+    }
+    return npos();
+  }
+
+  iterator make_iter(size_type idx, size_type key_idx) {
+    return iterator(entries_.data(), idx, entries_.size(), key_idx);
+  }
+
+  const_iterator make_citer(size_type idx, size_type key_idx) const {
+    return const_iterator(entries_.data(), idx, entries_.size(), key_idx);
+  }
+
+  std::vector<value_type> entries_;
+};
 
 using Params = std::multimap<std::string, std::string>;
 using Match = std::smatch;
@@ -7541,10 +7798,10 @@ inline bool is_chunked_transfer_encoding(const Headers &headers) {
   //
   // Security: reading a chunked message as unframed leaves its body in the
   // socket, where a keep-alive connection parses it as a smuggled request.
-  // Headers is an unordered_multimap whose iteration order for duplicate keys
+  // Headers now preserves the order the lines were received in, so the final
-  // is not portable, so when there is more than one Transfer-Encoding line we
+  // coding could be identified even when it is split across lines, but we
-  // cannot tell which coding is truly final. In that ambiguous case we fail
+  // keep failing safe and treat any message carrying more than one
-  // safe by treating the message as chunked (a mis-parse just closes the
+  // Transfer-Encoding line as chunked (a mis-parse just closes the
   // connection, whereas the opposite error enables smuggling).
   auto rng = headers.equal_range("Transfer-Encoding");
 
@@ -12773,11 +13030,17 @@ Server::process_request(Stream &strm, const std::string &remote_addr,
     return write_response(strm, close_connection, req, res);
   }
 
-  // RFC 9112 §6.3: Reject requests with both a non-zero Content-Length and
+  // RFC 9112 §6.3: Reject requests whose framing is ambiguous, which would
-  // any Transfer-Encoding to prevent request smuggling. Content-Length: 0 is
+  // otherwise let an intermediary and this parser disagree on where the body
-  // tolerated for compatibility with existing clients.
+  // ends and enable request smuggling. Two cases: a non-zero Content-Length
-  if (req.get_header_value_u64("Content-Length") > 0 &&
+  // alongside any Transfer-Encoding (Content-Length: 0 is tolerated for
-      req.has_header("Transfer-Encoding")) {
+  // compatibility with existing clients), and a Transfer-Encoding whose final
+  // 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 ||
+       !detail::is_chunked_transfer_encoding(req.headers))) {
     connection_closed = true;
     res.status = StatusCode::BadRequest_400;
     return write_response(strm, close_connection, req, res);
@@ -13423,11 +13686,13 @@ inline void ClientImpl::prepare_default_headers(Request &r, bool for_stream,
     if (!r.has_header(header.first)) { r.headers.insert(header); }
   }
 
+  // RFC 9110 5.3 recommends sending control data such as Host first, so
+  // prepend it rather than appending it after the caller's own fields.
   if (!r.has_header("Host")) {
     if (address_family_ == AF_UNIX) {
-      r.headers.emplace("Host", "localhost");
+      r.headers.emplace_front("Host", "localhost");
     } else {
-      r.headers.emplace(
+      r.headers.emplace_front(
           "Host", detail::make_host_and_port_string(host_, port_, is_ssl()));
     }
   }

test/test.cc

@@ -1366,6 +1366,93 @@ TEST(GetHeaderValueTest, Range) {
   }
 }
 
+// Joins every field of a Headers into "name=value " so a whole traversal can
+// be compared in one assertion.
+static std::string headers_to_string(const Headers &headers) {
+  std::string s;
+  for (const auto &header : headers) {
+    s += header.first + "=" + header.second + " ";
+  }
+  return s;
+}
+
+TEST(HeadersOrderTest, DuplicateFieldsKeepInsertionOrder) {
+  // RFC 9110 5.3: the order of fields sharing a name is significant. This used
+  // to depend on the standard library (libstdc++ handed back duplicates in
+  // reverse insertion order, libc++ in insertion order).
+  Request req;
+  req.set_header("Accept-Encoding", "gzip");
+  req.set_header("Accept-Encoding", "deflate");
+  req.set_header("Accept-Encoding", "br");
+
+  EXPECT_EQ(3U, req.get_header_value_count("Accept-Encoding"));
+  EXPECT_EQ("gzip", req.get_header_value("Accept-Encoding"));
+  EXPECT_EQ("gzip", req.get_header_value("Accept-Encoding", "", 0));
+  EXPECT_EQ("deflate", req.get_header_value("Accept-Encoding", "", 1));
+  EXPECT_EQ("br", req.get_header_value("Accept-Encoding", "", 2));
+}
+
+TEST(HeadersOrderTest, IdBeyondTheLastDuplicateYieldsDefault) {
+  Request req;
+  req.set_header("X-Test", "only");
+
+  EXPECT_EQ("only", req.get_header_value("X-Test", "def", 0));
+  EXPECT_EQ("def", req.get_header_value("X-Test", "def", 1));
+  EXPECT_EQ("def", req.get_header_value("X-Test", "def", 99));
+  EXPECT_EQ("def", req.get_header_value("X-Missing", "def", 3));
+}
+
+TEST(HeadersOrderTest, TraversalFollowsInsertionOrder) {
+  Headers headers;
+  headers.emplace("Host", "example.com");
+  headers.emplace("Accept-Encoding", "gzip");
+  headers.emplace("User-Agent", "test");
+  headers.emplace("Accept-Encoding", "deflate");
+
+  EXPECT_EQ("Host=example.com Accept-Encoding=gzip User-Agent=test "
+            "Accept-Encoding=deflate ",
+            headers_to_string(headers));
+}
+
+TEST(HeadersOrderTest, LookupIsCaseInsensitiveAndPicksTheFirstField) {
+  Headers headers = {{"Content-Type", "text/html"},
+                     {"CONTENT-TYPE", "text/xml"}};
+
+  EXPECT_EQ(2U, headers.count("content-type"));
+  auto it = headers.find("content-type");
+  ASSERT_TRUE(it != headers.end());
+  EXPECT_EQ("text/html", it->second);
+}
+
+TEST(HeadersOrderTest, ErasingAnEqualRangeSparesInterleavedFields) {
+  // The fields sharing a name are not adjacent, so an equal_range() erase must
+  // drop only those fields and leave everything positioned between them.
+  Headers headers = {{"A", "1"}, {"X", "x"}, {"A", "2"},
+                     {"Y", "y"}, {"A", "3"}, {"Z", "z"}};
+
+  auto rng = headers.equal_range("a");
+  headers.erase(rng.first, rng.second);
+
+  EXPECT_EQ("X=x Y=y Z=z ", headers_to_string(headers));
+}
+
+TEST(HeadersOrderTest, ErasingByNameKeepsTheRemainingOrder) {
+  Headers headers = {
+      {"A", "1"}, {"B", "2"}, {"a", "3"}, {"C", "4"}, {"A", "5"}};
+
+  EXPECT_EQ(3U, headers.erase("A"));
+  EXPECT_EQ(0U, headers.count("A"));
+  EXPECT_EQ("B=2 C=4 ", headers_to_string(headers));
+}
+
+TEST(HeadersOrderTest, EmplaceFrontPrepends) {
+  Headers headers = {{"Accept", "*/*"}, {"User-Agent", "test"}};
+  headers.emplace_front("Host", "example.com");
+
+  EXPECT_EQ("Host=example.com Accept=*/* User-Agent=test ",
+            headers_to_string(headers));
+}
+
 TEST(ParseHeaderValueTest, Range) {
   {
     Ranges ranges;
@@ -8295,6 +8382,63 @@ TEST(ServerRequestParsingTest, TrimWhitespaceFromHeaderValues) {
   EXPECT_EQ("HTTP/1.1 400 Bad Request", res.substr(0, 24));
 }
 
+TEST(HeadersOrderTest, ReceivedFieldsKeepTheirOrder) {
+  Server svr;
+  std::string received;
+  svr.Get("/order", [&](const Request &req, Response &res) {
+    received = headers_to_string(req.headers);
+    res.set_content("ok", "text/plain");
+  });
+
+  thread t = thread([&] { svr.listen(HOST, PORT); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    t.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+
+  svr.wait_until_ready();
+
+  const std::string req = "GET /order HTTP/1.1\r\n"
+                          "X-First: 1\r\n"
+                          "X-Dup: a\r\n"
+                          "X-Second: 2\r\n"
+                          "X-Dup: b\r\n"
+                          "Connection: close\r\n"
+                          "\r\n";
+
+  std::string res;
+  ASSERT_TRUE(send_request(5, req, &res));
+  EXPECT_EQ("HTTP/1.1 200 OK", res.substr(0, 15));
+  EXPECT_EQ("X-First=1 X-Dup=a X-Second=2 X-Dup=b Connection=close ", received);
+}
+
+TEST(HeadersOrderTest, SentFieldsKeepTheirOrder) {
+  Server svr;
+  svr.Get("/cookies", [](const Request & /*req*/, Response &res) {
+    res.set_header("Set-Cookie", "first=1");
+    res.set_header("X-Between", "y");
+    res.set_header("Set-Cookie", "second=2");
+    res.set_content("ok", "text/plain");
+  });
+
+  thread t = thread([&] { svr.listen(HOST, PORT); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    t.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+
+  svr.wait_until_ready();
+
+  Client cli(HOST, PORT);
+  auto res = cli.Get("/cookies");
+  ASSERT_TRUE(res);
+  EXPECT_EQ(2U, res->get_header_value_count("Set-Cookie"));
+  EXPECT_EQ("first=1", res->get_header_value("Set-Cookie", "", 0));
+  EXPECT_EQ("second=2", res->get_header_value("Set-Cookie", "", 1));
+}
+
 TEST(ServerResponseSplittingTest, ChunkedTrailerCRLFInjection) {
   Server svr;
   svr.Get("/injected-trailer", [&](const Request & /*req*/, Response &res) {
@@ -20778,6 +20922,48 @@ TEST(RequestSmugglingTest, ContentLengthAndTransferEncodingRejected) {
   }
 }
 
+TEST(RequestSmugglingTest, NonFinalChunkedTransferEncodingRejected) {
+  Server svr;
+  svr.Post("/test", [&](const Request &, Response &res) {
+    res.set_content("ok", "text/plain");
+  });
+
+  thread t = thread([&] { svr.listen(HOST, PORT); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    t.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+  svr.wait_until_ready();
+
+  // RFC 9112 6.3: when chunked is not the final transfer coding the body
+  // length cannot be determined, so the server must answer 400 and close
+  // rather than treat the request as bodyless and leave the body in the
+  // socket for the next request to pick up.
+  for (const auto &transfer_encoding : {"gzip", "chunked, gzip"}) {
+    auto req = std::string("POST /test HTTP/1.1\r\n") + "Host: localhost\r\n" +
+               "Transfer-Encoding: " + transfer_encoding + "\r\n" + "\r\n";
+
+    std::string response;
+    ASSERT_TRUE(send_request(1, req, &response));
+    EXPECT_EQ("HTTP/1.1 400 Bad Request",
+              response.substr(0, response.find("\r\n")))
+        << transfer_encoding;
+  }
+
+  // A sequence ending in chunked stays valid.
+  auto req = "POST /test HTTP/1.1\r\n"
+             "Host: localhost\r\n"
+             "Transfer-Encoding: gzip, chunked\r\n"
+             "Connection: close\r\n"
+             "\r\n"
+             "0\r\n\r\n";
+
+  std::string response;
+  ASSERT_TRUE(send_request(1, req, &response));
+  EXPECT_EQ("HTTP/1.1 200 OK", response.substr(0, response.find("\r\n")));
+}
+
 // Regression for issue #2450: a DELETE without Content-Length on a
 // keep-alive connection must not let the post-response drain consume the
 // next request's bytes.