ensure that the inline cookie header will be folded correctly

source/common/http/header_map_impl.cc

@@ -285,6 +285,17 @@ template <> HeaderMapImpl::StaticLookupTable<ResponseTrailerMap>::StaticLookupTa
   finalizeTable();
 }
 
+absl::string_view HeaderMapImpl::delimiterByHeader(const LowerCaseString& key,
+                                                   bool correctly_coalesce_cookies) {
+  // TODO(wbpcode): 'correctly_coalesce_cookies' feature is enabled by default and is allowed to be
+  // disabled via runtime. But I doubt if any user will actually disable it. The comma separator is
+  // obviously problematic for cookies. Maybe we can consider removing it.
+  if (correctly_coalesce_cookies && key == Http::Headers::get().Cookie) {
+    return "; ";
+  }
+  return ",";
+}
+
 uint64_t HeaderMapImpl::appendToHeader(HeaderString& header, absl::string_view data,
                                        absl::string_view delimiter) {
   if (data.empty()) {
@@ -368,8 +379,10 @@ void HeaderMapImpl::insertByKey(HeaderString&& key, HeaderString&& value) {
     if (*lookup.value().entry_ == nullptr) {
       maybeCreateInline(lookup.value().entry_, *lookup.value().key_, std::move(value));
     } else {
+      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 +447,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);
+    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/header_map_impl.h

@@ -319,6 +319,8 @@ class HeaderMapImpl : NonCopyable {
   void insertByKey(HeaderString&& key, HeaderString&& value);
   static uint64_t appendToHeader(HeaderString& header, absl::string_view data,
                                  absl::string_view delimiter = ",");
+  static absl::string_view delimiterByHeader(const LowerCaseString& key,
+                                             bool correctly_coalesce_cookies);
   HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key);
   HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key,
                                      HeaderString&& 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