ensure that the inline cookie header will be folded correctly (envoyp…

…roxy#17560)

Commit Message: ensure that the inline cookie header will be folded correctly
Additional Description:

This PR is a supplement to envoyproxy#17330 and envoyproxy#14969 and will finally close envoyproxy#17234. This PR mainly did following works:

update insertByKey to choose suitable delimiter for inline header.
update parseCookie to avoid unnecessary iteration for parsing cookie value.
Risk Level: Low.
Testing: Add.
Docs Changes: N/A.
Release Notes: N/A.
Signed-off-by: wbpcode <[email protected]>

source/common/http/header_map_impl.cc

@@ -22,6 +22,9 @@ namespace {
 // This includes the NULL (StringUtil::itoa technically only needs 21).
 constexpr size_t MaxIntegerLength{32};
 
+constexpr absl::string_view DelimiterForInlineHeaders{","};
+constexpr absl::string_view DelimiterForInlineCookies{"; "};
+
 void validateCapacity(uint64_t new_capacity) {
   // If the resizing will cause buffer overflow due to hitting uint32_t::max, an OOM is likely
   // imminent. Fast-fail rather than allow a buffer overflow attack (issue #1421)
@@ -46,6 +49,13 @@ bool validatedLowerCaseString(absl::string_view str) {
   return lower_case_str == str;
 }
 
+absl::string_view delimiterByHeader(const LowerCaseString& key, bool correctly_coalesce_cookies) {
+  if (correctly_coalesce_cookies && key == Http::Headers::get().Cookie) {
+    return DelimiterForInlineCookies;
+  }
+  return DelimiterForInlineHeaders;
+}
+
 } // namespace
 
 // Initialize as a Type::Inline
@@ -368,8 +378,10 @@ void HeaderMapImpl::insertByKey(HeaderString&& key, HeaderString&& value) {
     if (*lookup.value().entry_ == nullptr) {
       maybeCreateInline(lookup.value().entry_, *lookup.value().key_, std::move(value));
     } else {
+      const auto delimiter =
+          delimiterByHeader(*lookup.value().key_, header_map_correctly_coalesce_cookies_);
       const uint64_t added_size =
-          appendToHeader((*lookup.value().entry_)->value(), value.getStringView());
+          appendToHeader((*lookup.value().entry_)->value(), value.getStringView(), delimiter);
       addSize(added_size);
       value.clear();
     }
@@ -434,10 +446,8 @@ void HeaderMapImpl::appendCopy(const LowerCaseString& key, absl::string_view val
   // TODO(#9221): converge on and document a policy for coalescing multiple headers.
   auto entry = getExisting(key);
   if (!entry.empty()) {
-    const std::string delimiter = (key == Http::Headers::get().Cookie ? "; " : ",");
-    const uint64_t added_size = header_map_correctly_coalesce_cookies_
-                                    ? appendToHeader(entry[0]->value(), value, delimiter)
-                                    : appendToHeader(entry[0]->value(), value);
+    const auto delimiter = delimiterByHeader(key, header_map_correctly_coalesce_cookies_);
+    const uint64_t added_size = appendToHeader(entry[0]->value(), value, delimiter);
     addSize(added_size);
   } else {
     addCopy(key, value);

source/common/http/utility.cc

@@ -254,44 +254,46 @@ bool maybeAdjustForIpv6(absl::string_view absolute_url, uint64_t& offset, uint64
   return true;
 }
 
-std::string parseCookie(const HeaderMap& headers, const std::string& key,
-                        const std::string& cookie) {
-
-  std::string ret;
-
-  headers.iterateReverse([&key, &ret, &cookie](const HeaderEntry& header) -> HeaderMap::Iterate {
-    // Find the cookie headers in the request (typically, there's only one).
-    if (header.key() == cookie) {
-
-      // Split the cookie header into individual cookies.
-      for (const auto& s : StringUtil::splitToken(header.value().getStringView(), ";")) {
-        // Find the key part of the cookie (i.e. the name of the cookie).
-        size_t first_non_space = s.find_first_not_of(' ');
-        size_t equals_index = s.find('=');
-        if (equals_index == absl::string_view::npos) {
-          // The cookie is malformed if it does not have an `=`. Continue
-          // checking other cookies in this header.
-          continue;
-        }
-        const absl::string_view k = s.substr(first_non_space, equals_index - first_non_space);
-        // If the key matches, parse the value from the rest of the cookie string.
-        if (k == key) {
-          absl::string_view v = s.substr(equals_index + 1, s.size() - 1);
-
-          // Cookie values may be wrapped in double quotes.
-          // https://tools.ietf.org/html/rfc6265#section-4.1.1
-          if (v.size() >= 2 && v.back() == '"' && v[0] == '"') {
-            v = v.substr(1, v.size() - 2);
-          }
-          ret = std::string{v};
-          return HeaderMap::Iterate::Break;
-        }
+absl::string_view parseCookie(absl::string_view cookie_value, absl::string_view key) {
+  // Split the cookie header into individual cookies.
+  for (const auto& s : StringUtil::splitToken(cookie_value, ";")) {
+    // Find the key part of the cookie (i.e. the name of the cookie).
+    size_t first_non_space = s.find_first_not_of(' ');
+    size_t equals_index = s.find('=');
+    if (equals_index == absl::string_view::npos) {
+      // The cookie is malformed if it does not have an `=`. Continue
+      // checking other cookies in this header.
+      continue;
+    }
+    absl::string_view k = s.substr(first_non_space, equals_index - first_non_space);
+    // If the key matches, parse the value from the rest of the cookie string.
+    if (k == key) {
+      absl::string_view v = s.substr(equals_index + 1, s.size() - 1);
+
+      // Cookie values may be wrapped in double quotes.
+      // https://tools.ietf.org/html/rfc6265#section-4.1.1
+      if (v.size() >= 2 && v.back() == '"' && v[0] == '"') {
+        v = v.substr(1, v.size() - 2);
       }
+      return v;
     }
-    return HeaderMap::Iterate::Continue;
-  });
+  }
+  return EMPTY_STRING;
+}
 
-  return ret;
+std::string parseCookie(const HeaderMap& headers, const std::string& key,
+                        const LowerCaseString& cookie) {
+  const Http::HeaderMap::GetResult cookie_headers = headers.get(cookie);
+
+  for (size_t index = 0; index < cookie_headers.size(); index++) {
+    auto cookie_header_value = cookie_headers[index]->value().getStringView();
+    absl::string_view result = parseCookie(cookie_header_value, key);
+    if (!result.empty()) {
+      return std::string{result};
+    }
+  }
+
+  return EMPTY_STRING;
 }
 
 bool Utility::Url::initialize(absl::string_view absolute_url, bool is_connect) {
@@ -429,11 +431,12 @@ std::string Utility::stripQueryString(const HeaderString& path) {
 }
 
 std::string Utility::parseCookieValue(const HeaderMap& headers, const std::string& key) {
-  return parseCookie(headers, key, Http::Headers::get().Cookie.get());
+  // TODO(wbpcode): Modify the headers parameter type to 'RequestHeaderMap'.
+  return parseCookie(headers, key, Http::Headers::get().Cookie);
 }
 
 std::string Utility::parseSetCookieValue(const Http::HeaderMap& headers, const std::string& key) {
-  return parseCookie(headers, key, Http::Headers::get().SetCookie.get());
+  return parseCookie(headers, key, Http::Headers::get().SetCookie);
 }
 
 std::string Utility::makeSetCookieValue(const std::string& key, const std::string& value,

test/common/http/BUILD

@@ -319,6 +319,15 @@ envoy_cc_fuzz_test(
     ],
 )
 
+envoy_cc_test(
+    name = "inline_cookie_test",
+    srcs = ["inline_cookie_test.cc"],
+    deps = [
+        "//source/common/http:header_map_lib",
+        "//test/mocks/runtime:runtime_mocks",
+    ],
+)
+
 envoy_cc_test(
     name = "header_utility_test",
     srcs = ["header_utility_test.cc"],

test/common/http/inline_cookie_test.cc

@@ -0,0 +1,61 @@
+#include "source/common/http/header_map_impl.h"
+#include "source/common/http/header_utility.h"
+
+#include "test/mocks/runtime/mocks.h"
+#include "test/test_common/utility.h"
+
+#include "gtest/gtest.h"
+
+namespace Envoy {
+namespace Http {
+namespace {
+
+// Test that the cookie header can work correctly after being registered as an inline header. The
+// test will register the cookie as an inline header. In order to avoid affecting other tests, the
+// test is placed in this separate source file.
+TEST(InlineCookieTest, InlineCookieTest) {
+  Http::CustomInlineHeaderRegistry::registerInlineHeader<Http::RequestHeaderMap::header_map_type>(
+      Http::Headers::get().Cookie);
+  Http::CustomInlineHeaderRegistry::registerInlineHeader<Http::RequestHeaderMap::header_map_type>(
+      Http::LowerCaseString("header_for_compare"));
+
+  auto mock_snapshot = std::make_shared<testing::NiceMock<Runtime::MockSnapshot>>();
+  testing::NiceMock<Runtime::MockLoader> mock_loader;
+  Runtime::LoaderSingleton::initialize(&mock_loader);
+
+  {
+    // Enable 'envoy.reloadable_features.header_map_correctly_coalesce_cookies' feature.
+    ON_CALL(mock_loader, threadsafeSnapshot()).WillByDefault(testing::Return(mock_snapshot));
+    ON_CALL(*mock_snapshot, runtimeFeatureEnabled(_)).WillByDefault(testing::Return(true));
+
+    Http::TestRequestHeaderMapImpl headers{{"cookie", "key1:value1"},
+                                           {"cookie", "key2:value2"},
+                                           {"header_for_compare", "value1"},
+                                           {"header_for_compare", "value2"}};
+
+    // Delimiter for inline 'cookie' header is specialized '; '.
+    EXPECT_EQ("key1:value1; key2:value2", headers.get_("cookie"));
+    // Delimiter for inline 'header_for_compare' header is default ','.
+    EXPECT_EQ("value1,value2", headers.get_("header_for_compare"));
+  }
+
+  {
+    // Disable 'envoy.reloadable_features.header_map_correctly_coalesce_cookies' feature.
+    ON_CALL(mock_loader, threadsafeSnapshot()).WillByDefault(testing::Return(mock_snapshot));
+    ON_CALL(*mock_snapshot, runtimeFeatureEnabled(_)).WillByDefault(testing::Return(false));
+
+    Http::TestRequestHeaderMapImpl headers{{"cookie", "key1:value1"},
+                                           {"cookie", "key2:value2"},
+                                           {"header_for_compare", "value1"},
+                                           {"header_for_compare", "value2"}};
+
+    // 'envoy.reloadable_features.header_map_correctly_coalesce_cookies' is disabled then default
+    // ',' will be used as delimiter.
+    EXPECT_EQ("key1:value1,key2:value2", headers.get_("cookie"));
+    EXPECT_EQ("value1,value2", headers.get_("header_for_compare"));
+  }
+}
+
+} // namespace
+} // namespace Http
+} // namespace Envoy