Ver código fonte

Ignore empty list elements in the Accept header (Fix #2567)

parse_accept_header() rejected any Accept value with a leading, trailing
or doubled comma, and Server::process_request() validates Accept before
routing, so "Accept: text/html," was answered 400 Bad Request on every
route.

RFC 9110 Section 5.6.1.2 requires a recipient to parse and ignore empty
list elements in a #rule list, so those values are legal. split() already
trims each element and skips the empty ones, which made the guard inside
the callback unreachable as well; drop both and let the empty elements
fall away. The header length limit bounds how many a sender can send, so
ignoring all of them cannot be used as a denial-of-service vector.

get_combined_header_value() keeps skipping empty field lines, but that
skip is no longer observable through a request now that a stray comma
parses cleanly, so it gets its own test.
yhirose 2 dias atrás
pai
commit
e96a52e9dd
2 arquivos alterados com 57 adições e 24 exclusões
  1. 7 14
      httplib.h
  2. 50 10
      test/test.cc

+ 7 - 14
httplib.h

@@ -7858,8 +7858,7 @@ inline std::string get_combined_header_value(const Headers &headers,
   for (auto it = rng.first; it != rng.second; ++it) {
     // RFC 9110 Section 5.6.1.2: a recipient has to parse and ignore empty list
     // elements, so an empty field line must not contribute a bare comma to the
-    // combined value. parse_accept_header() rejects a leading comma outright,
-    // which would turn a legal request into 400 Bad Request.
+    // combined value.
     if (it->second.empty()) { continue; }
     if (!combined.empty()) { combined += ", "; }
     combined += it->second;
@@ -8836,12 +8835,6 @@ inline bool parse_accept_header(const std::string &s,
   // Empty string is considered valid (no preference)
   if (s.empty()) { return true; }
 
-  // Check for invalid patterns: leading/trailing commas or consecutive commas
-  if (s.front() == ',' || s.back() == ',' ||
-      s.find(",,") != std::string::npos) {
-    return false;
-  }
-
   struct AcceptEntry {
     std::string media_type;
     double quality;
@@ -8852,16 +8845,16 @@ inline bool parse_accept_header(const std::string &s,
   int order = 0;
   bool has_invalid_entry = false;
 
-  // Split by comma and parse each entry
+  // Split by comma and parse each entry. RFC 9110 Section 5.6.1.2: a recipient
+  // has to parse and ignore empty list elements, so a leading, trailing or
+  // doubled comma must not turn a legal Accept value into 400 Bad Request.
+  // split() skips them, and the header length limit bounds how many a sender
+  // can send, so ignoring all of them cannot be used as a denial-of-service
+  // vector.
   split(s.data(), s.data() + s.size(), ',', [&](const char *b, const char *e) {
     std::string entry(b, e);
     entry = trim_copy(entry);
 
-    if (entry.empty()) {
-      has_invalid_entry = true;
-      return;
-    }
-
     AcceptEntry accept_entry;
     accept_entry.order = order++;
 

+ 50 - 10
test/test.cc

@@ -1067,14 +1067,6 @@ TEST(ParseAcceptHeaderTest, InvalidCases) {
   EXPECT_FALSE(
       detail::parse_accept_header("invalidtype,application/json", result));
 
-  // Empty media type
-  result.clear();
-  EXPECT_FALSE(detail::parse_accept_header(",application/json", result));
-
-  // Only commas
-  result.clear();
-  EXPECT_FALSE(detail::parse_accept_header(",,,", result));
-
   // Valid cases should still work
   EXPECT_TRUE(detail::parse_accept_header("*/*", result));
   EXPECT_EQ(result.size(), 1U);
@@ -1089,6 +1081,38 @@ TEST(ParseAcceptHeaderTest, InvalidCases) {
   EXPECT_EQ(result[0], "text/*");
 }
 
+// RFC 9110 Section 5.6.1.2: a recipient has to parse and ignore empty list
+// elements, so a leading, trailing or doubled comma is a legal Accept value
+// and must not be answered with 400 Bad Request.
+TEST(ParseAcceptHeaderTest, EmptyListElementsAreIgnored) {
+  struct {
+    const char *value;
+    std::vector<std::string> expected;
+  } cases[] = {
+      {",application/json", {"application/json"}},
+      {"text/html,", {"text/html"}},
+      {"text/html,,*/*", {"text/html", "*/*"}},
+      // An empty element may be spelled with whitespace in it
+      {"text/html, , application/json", {"text/html", "application/json"}},
+      // Nothing but empty elements is an empty list, which means the same
+      // thing as no Accept header at all
+      {",,,", {}},
+      // Quality values still apply across ignored empty elements
+      {",text/html;q=0.5,,application/json", {"application/json", "text/html"}},
+  };
+
+  for (const auto &c : cases) {
+    std::vector<std::string> result;
+    EXPECT_TRUE(detail::parse_accept_header(c.value, result))
+        << "value: " << c.value;
+    EXPECT_EQ(c.expected, result) << "value: " << c.value;
+  }
+
+  // An invalid entry is still rejected when empty elements surround it
+  std::vector<std::string> result;
+  EXPECT_FALSE(detail::parse_accept_header(",invalidtype,", result));
+}
+
 TEST(ParseAcceptHeaderTest, ContentTypesPopulatedAndInvalidHeaderHandling) {
   Server svr;
 
@@ -18519,8 +18543,6 @@ TEST(RepeatedFieldLinesTest, AcceptCombinesEveryFieldLine) {
 
 // RFC 9110 Section 5.6.1.2: empty list elements are parsed and ignored, so an
 // empty field line must not contribute a bare comma to the combined value.
-// parse_accept_header() rejects a leading comma outright, so a stray one would
-// turn a legal request into 400 Bad Request.
 TEST(RepeatedFieldLinesTest, EmptyFieldLineDoesNotInjectComma) {
   Server svr;
 
@@ -18557,6 +18579,24 @@ TEST(RepeatedFieldLinesTest, EmptyFieldLineDoesNotInjectComma) {
   EXPECT_EQ("application/json", observed_types[0]);
 }
 
+// The skip in get_combined_header_value() is what keeps an empty field line
+// from contributing a bare comma. Only a trailing empty field line exercises
+// it: a leading one is already covered by the "combined is still empty" check,
+// and parse_accept_header() now ignores the empty element either way, so the
+// request-level test above can no longer tell the two apart.
+TEST(RepeatedFieldLinesTest, EmptyFieldLineIsNotCombined) {
+  Headers headers;
+  headers.emplace("Accept", "text/html");
+  headers.emplace("Accept", "");
+  EXPECT_EQ("text/html", detail::get_combined_header_value(headers, "Accept"));
+
+  headers.clear();
+  headers.emplace("Accept", "");
+  headers.emplace("Accept", "application/json");
+  EXPECT_EQ("application/json",
+            detail::get_combined_header_value(headers, "Accept"));
+}
+
 #ifdef CPPHTTPLIB_ZLIB_SUPPORT
 // An encoding offered on a later Accept-Encoding field line is still offered.
 TEST(RepeatedFieldLinesTest, AcceptEncodingCombinesEveryFieldLine) {