Fix #1908

yhirose · Sep 3, 2024 · 975cf0d · 975cf0d
1 parent 4854a69
commit 975cf0d
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 27 deletions.
diff --git a/httplib.h b/httplib.h
@@ -4021,6 +4021,18 @@ inline bool parse_header(const char *beg, const char *end, T fn) {
     auto val = compare_case_ignore(key, "Location")
                    ? std::string(p, end)
                    : decode_url(std::string(p, end), false);
+
+    // NOTE: From RFC 9110:
+    // Field values containing CR, LF, or NUL characters are
+    // invalid and dangerous, due to the varying ways that
+    // implementations might parse and interpret those
+    // characters; a recipient of CR, LF, or NUL within a field
+    // value MUST either reject the message or replace each of
+    // those characters with SP before further processing or
+    // forwarding of that message.
+    static const std::string CR_LF_NUL("\r\n\0", 3);
+    if (val.find_first_of(CR_LF_NUL) != std::string::npos) { return false; }
+
     fn(key, val);
     return true;
   }
@@ -4058,25 +4070,12 @@ inline bool read_headers(Stream &strm, Headers &headers) {
     // Exclude line terminator
     auto end = line_reader.ptr() + line_reader.size() - line_terminator_len;
 
-    parse_header(line_reader.ptr(), end,
-                 [&](const std::string &key, std::string &val) {
-                   // NOTE: From RFC 9110:
-                   // Field values containing CR, LF, or NUL characters are
-                   // invalid and dangerous, due to the varying ways that
-                   // implementations might parse and interpret those
-                   // characters; a recipient of CR, LF, or NUL within a field
-                   // value MUST either reject the message or replace each of
-                   // those characters with SP before further processing or
-                   // forwarding of that message.
-                   for (auto &c : val) {
-                     switch (c) {
-                     case '\0':
-                     case '\n':
-                     case '\r': c = ' '; break;
-                     }
-                   }
-                   headers.emplace(key, val);
-                 });
+    if (!parse_header(line_reader.ptr(), end,
+                      [&](const std::string &key, std::string &val) {
+                        headers.emplace(key, val);
+                      })) {
+      return false;
+    }
   }
 
   return true;

diff --git a/test/test.cc b/test/test.cc
@@ -4718,9 +4718,7 @@ static void test_raw_request(const std::string &req,
   svr.Put("/put_hi", [&](const Request & /*req*/, Response &res) {
     res.set_content("ok", "text/plain");
   });
-  svr.Get("/header_field_value_check", [&](const Request &req, Response &res) {
-    auto val = req.get_header_value("Test");
-    EXPECT_EQ("[   ]", val);
+  svr.Get("/header_field_value_check", [&](const Request &/*req*/, Response &res) {
     res.set_content("ok", "text/plain");
   });
 
@@ -4857,9 +4855,11 @@ TEST(ServerRequestParsingTest, InvalidSpaceInURL) {
 }
 
 TEST(ServerRequestParsingTest, InvalidFieldValueContains_CR_LF_NUL) {
+  std::string out;
   std::string request(
       "GET /header_field_value_check HTTP/1.1\r\nTest: [\r\x00\n]\r\n\r\n", 55);
-  test_raw_request(request);
+  test_raw_request(request, &out);
+  EXPECT_EQ("HTTP/1.1 400 Bad Request", out.substr(0, 24));
 }
 
 TEST(ServerStopTest, StopServerWithChunkedTransmission) {
@@ -7587,10 +7587,8 @@ TEST(FileSystemTest, FileAndDirExistenceCheck) {
 TEST(DirtyDataRequestTest, HeadFieldValueContains_CR_LF_NUL) {
   Server svr;
 
-  svr.Get("/test", [&](const Request &req, Response &) {
-    auto val = req.get_header_value("Test");
-    EXPECT_EQ(val.size(), 7u);
-    EXPECT_EQ(val, "_  _  _");
+  svr.Get("/test", [&](const Request &/*req*/, Response &res) {
+    EXPECT_EQ(res.status, 400);
   });
 
   auto thread = std::thread([&]() { svr.listen(HOST, PORT); });