From 0da881f8fecf589e3e3241f745e38af19dd95ffc Mon Sep 17 00:00:00 2001 From: wbpcode Date: Mon, 2 Aug 2021 13:55:04 +0800 Subject: [PATCH 1/4] inline cookie delimiter Signed-off-by: wbpcode --- source/common/http/header_map_impl.cc | 21 +++++-- source/common/http/header_map_impl.h | 2 + source/common/http/utility.cc | 77 +++++++++++++------------- test/common/http/BUILD | 9 +++ test/common/http/inline_cookie_test.cc | 62 +++++++++++++++++++++ 5 files changed, 129 insertions(+), 42 deletions(-) create mode 100644 test/common/http/inline_cookie_test.cc diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index ee6956d049d9b..350551404237f 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -285,6 +285,17 @@ template <> HeaderMapImpl::StaticLookupTable::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); diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 79dd2d54c6bb5..654d07e1bd61d 100644 --- a/source/common/http/header_map_impl.h +++ b/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); diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index cc472f8e1cecd..483b9bb1ebfdb 100644 --- a/source/common/http/utility.cc +++ b/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, diff --git a/test/common/http/BUILD b/test/common/http/BUILD index df4b5763d2490..edd7e75664edf 100644 --- a/test/common/http/BUILD +++ b/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"], diff --git a/test/common/http/inline_cookie_test.cc b/test/common/http/inline_cookie_test.cc new file mode 100644 index 0000000000000..a8d28cc27cc42 --- /dev/null +++ b/test/common/http/inline_cookie_test.cc @@ -0,0 +1,62 @@ +#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" +#include + +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::Headers::get().Cookie); + Http::CustomInlineHeaderRegistry::registerInlineHeader( + Http::LowerCaseString("header_for_compare")); + + auto mock_snapshot = std::make_shared>(); + testing::NiceMock 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 From 0b123943c90c3689f277988052b051f0218ba1ba Mon Sep 17 00:00:00 2001 From: wbpcode Date: Mon, 2 Aug 2021 16:44:32 +0800 Subject: [PATCH 2/4] remove header include added by clangd Signed-off-by: wbpcode --- test/common/http/inline_cookie_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/common/http/inline_cookie_test.cc b/test/common/http/inline_cookie_test.cc index a8d28cc27cc42..5d8269f81975e 100644 --- a/test/common/http/inline_cookie_test.cc +++ b/test/common/http/inline_cookie_test.cc @@ -5,7 +5,6 @@ #include "test/test_common/utility.h" #include "gtest/gtest.h" -#include namespace Envoy { namespace Http { From 58659ad95ef04beec4da377bb5df9f3e24840470 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 4 Aug 2021 11:17:42 +0800 Subject: [PATCH 3/4] move delimiterByHeader to anonymous namespace Signed-off-by: wbpcode --- source/common/http/header_map_impl.cc | 18 +++++++----------- source/common/http/header_map_impl.h | 2 -- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 350551404237f..697ef06799181 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -46,6 +46,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 "; "; + } + return ","; +} + } // namespace // Initialize as a Type::Inline @@ -285,17 +292,6 @@ template <> HeaderMapImpl::StaticLookupTable::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()) { diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 654d07e1bd61d..79dd2d54c6bb5 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -319,8 +319,6 @@ 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); From 8703819b1a3a60277d61e4bebc3749208fc077e3 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Thu, 5 Aug 2021 10:30:09 +0800 Subject: [PATCH 4/4] minor update Signed-off-by: wbpcode --- source/common/http/header_map_impl.cc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 697ef06799181..3e2bd3b599adc 100644 --- a/source/common/http/header_map_impl.cc +++ b/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) @@ -48,9 +51,9 @@ bool validatedLowerCaseString(absl::string_view str) { absl::string_view delimiterByHeader(const LowerCaseString& key, bool correctly_coalesce_cookies) { if (correctly_coalesce_cookies && key == Http::Headers::get().Cookie) { - return "; "; + return DelimiterForInlineCookies; } - return ","; + return DelimiterForInlineHeaders; } } // namespace @@ -375,7 +378,7 @@ 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 = + const auto delimiter = delimiterByHeader(*lookup.value().key_, header_map_correctly_coalesce_cookies_); const uint64_t added_size = appendToHeader((*lookup.value().entry_)->value(), value.getStringView(), delimiter); @@ -443,7 +446,7 @@ 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()) { - auto delimiter = delimiterByHeader(key, header_map_correctly_coalesce_cookies_); + 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 {