diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 334279a41785c..553e832bf5b38 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -39,6 +39,14 @@ Version history * http: added generic :ref:`Upgrade support `. * http: better handling of HEAD requests. Now sending transfer-encoding: chunked rather than content-length: 0. +* http: fixed missing support for appending to predefined inline headers, e.g. + *authorization*, in features that interact with request and response headers, + e.g. :ref:`request_headers_to_add + `. For example, a + request header *authorization: token1* will appear as *authorization: + token1,token2*, after having :ref:`request_headers_to_add + ` with *authorization: + token2* applied. * http: response filters not applied to early error paths such as http_parser generated 400s. * http: :ref:`hpack_table_size ` now controls dynamic table size of both: encoder and decoder. diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index efbf38638eb25..3e158dc608aa5 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -321,10 +321,12 @@ class HeaderMap { /** * Add a reference header to the map. Both key and value MUST point to data that will live beyond * the lifetime of any request/response using the string (since a codec may optimize for zero - * copy). Nothing will be copied. + * copy). The key will not be copied and a best effort will be made not to + * copy the value (but this may happen when comma concatenating, see below). * - * Calling addReference multiple times for the same header will result in multiple headers being - * present in the HeaderMap. + * Calling addReference multiple times for the same header will result in: + * - Comma concatenation for predefined inline headers. + * - Multiple headers being present in the HeaderMap for other headers. * * @param key specifies the name of the header to add; it WILL NOT be copied. * @param value specifies the value of the header to add; it WILL NOT be copied. @@ -336,8 +338,9 @@ class HeaderMap { * the lifetime of any request/response using the string (since a codec may optimize for zero * copy). The value will be copied. * - * Calling addReferenceKey multiple times for the same header will result in multiple headers - * being present in the HeaderMap. + * Calling addReference multiple times for the same header will result in: + * - Comma concatenation for predefined inline headers. + * - Multiple headers being present in the HeaderMap for other headers. * * @param key specifies the name of the header to add; it WILL NOT be copied. * @param value specifies the value of the header to add; it WILL be copied. @@ -349,8 +352,9 @@ class HeaderMap { * live beyond the lifetime of any request/response using the string (since a codec may optimize * for zero copy). The value will be copied. * - * Calling addReferenceKey multiple times for the same header will result in multiple headers - * being present in the HeaderMap. + * Calling addReference multiple times for the same header will result in: + * - Comma concatenation for predefined inline headers. + * - Multiple headers being present in the HeaderMap for other headers. * * @param key specifies the name of the header to add; it WILL NOT be copied. * @param value specifies the value of the header to add; it WILL be copied. @@ -360,8 +364,9 @@ class HeaderMap { /** * Add a header by copying both the header key and the value. * - * Calling addCopy multiple times for the same header will result in multiple headers being - * present in the HeaderMap. + * Calling addCopy multiple times for the same header will result in: + * - Comma concatenation for predefined inline headers. + * - Multiple headers being present in the HeaderMap for other headers. * * @param key specifies the name of the header to add; it WILL be copied. * @param value specifies the value of the header to add; it WILL be copied. @@ -371,8 +376,9 @@ class HeaderMap { /** * Add a header by copying both the header key and the value. * - * Calling addCopy multiple times for the same header will result in multiple headers being - * present in the HeaderMap. + * Calling addCopy multiple times for the same header will result in: + * - Comma concatenation for predefined inline headers. + * - Multiple headers being present in the HeaderMap for other headers. * * @param key specifies the name of the header to add; it WILL be copied. * @param value specifies the value of the header to add; it WILL be copied. diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 28728515ef12e..7f17f1e24b600 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -67,13 +67,15 @@ void HeaderString::freeDynamic() { void HeaderString::append(const char* data, uint32_t size) { switch (type_) { case Type::Reference: { - // Switch back to inline and fall through. We do not actually append to the static string - // currently which would require a copy. - type_ = Type::Inline; - buffer_.dynamic_ = inline_buffer_; - string_length_ = 0; - - FALLTHRU; + // Rather than be too clever and optimize this uncommon case, we dynamically + // allocate and copy. + type_ = Type::Dynamic; + dynamic_capacity_ = (string_length_ + size) * 2; + char* buf = static_cast(malloc(dynamic_capacity_)); + RELEASE_ASSERT(buf != nullptr, ""); + memcpy(buf, buffer_.ref_, string_length_); + buffer_.dynamic_ = buf; + break; } case Type::Inline: { @@ -94,6 +96,7 @@ void HeaderString::append(const char* data, uint32_t size) { RELEASE_ASSERT(new_capacity <= std::numeric_limits::max(), ""); buffer_.dynamic_ = static_cast(malloc(new_capacity)); memcpy(buffer_.dynamic_, inline_buffer_, string_length_); + RELEASE_ASSERT(buffer_.dynamic_ != nullptr, ""); dynamic_capacity_ = new_capacity; type_ = Type::Dynamic; } else { @@ -101,6 +104,7 @@ void HeaderString::append(const char* data, uint32_t size) { // Need to reallocate. dynamic_capacity_ = (string_length_ + size) * 2; buffer_.dynamic_ = static_cast(realloc(buffer_.dynamic_, dynamic_capacity_)); + RELEASE_ASSERT(buffer_.dynamic_ != nullptr, ""); } } } @@ -323,8 +327,12 @@ void HeaderMapImpl::insertByKey(HeaderString&& key, HeaderString&& value) { if (cb) { key.clear(); StaticLookupResponse ref_lookup_response = cb(*this); - ASSERT(*ref_lookup_response.entry_ == nullptr); // This function doesn't handle append. - maybeCreateInline(ref_lookup_response.entry_, *ref_lookup_response.key_, std::move(value)); + if (*ref_lookup_response.entry_ == nullptr) { + maybeCreateInline(ref_lookup_response.entry_, *ref_lookup_response.key_, std::move(value)); + } else { + appendToHeader((*ref_lookup_response.entry_)->value(), value.c_str()); + value.clear(); + } } else { std::list::iterator i = headers_.insert(std::move(key), std::move(value)); i->entry_ = i; diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 7d8bb9413ece3..326b954002c09 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -110,7 +110,7 @@ TEST(HeaderStringTest, All) { HeaderString string(static_string); EXPECT_EQ(HeaderString::Type::Reference, string.type()); string.append("a", 1); - EXPECT_STREQ("a", string.c_str()); + EXPECT_STREQ("HELLOa", string.c_str()); } // Copy inline @@ -425,11 +425,37 @@ TEST(HeaderMapImplTest, SetRemovesAllValues) { } TEST(HeaderMapImplTest, DoubleInlineAdd) { - HeaderMapImpl headers; - headers.addReferenceKey(Headers::get().ContentLength, 5); - EXPECT_DEBUG_DEATH(headers.addReferenceKey(Headers::get().ContentLength, 6), ""); - EXPECT_STREQ("5", headers.ContentLength()->value().c_str()); - EXPECT_EQ(1UL, headers.size()); + { + HeaderMapImpl headers; + const std::string foo("foo"); + const std::string bar("bar"); + headers.addReference(Headers::get().ContentLength, foo); + headers.addReference(Headers::get().ContentLength, bar); + EXPECT_STREQ("foo,bar", headers.ContentLength()->value().c_str()); + EXPECT_EQ(1UL, headers.size()); + } + { + HeaderMapImpl headers; + headers.addReferenceKey(Headers::get().ContentLength, "foo"); + headers.addReferenceKey(Headers::get().ContentLength, "bar"); + EXPECT_STREQ("foo,bar", headers.ContentLength()->value().c_str()); + EXPECT_EQ(1UL, headers.size()); + } + { + HeaderMapImpl headers; + headers.addReferenceKey(Headers::get().ContentLength, 5); + headers.addReferenceKey(Headers::get().ContentLength, 6); + EXPECT_STREQ("5,6", headers.ContentLength()->value().c_str()); + EXPECT_EQ(1UL, headers.size()); + } + { + HeaderMapImpl headers; + const std::string foo("foo"); + headers.addReference(Headers::get().ContentLength, foo); + headers.addReferenceKey(Headers::get().ContentLength, 6); + EXPECT_STREQ("foo,6", headers.ContentLength()->value().c_str()); + EXPECT_EQ(1UL, headers.size()); + } } TEST(HeaderMapImplTest, DoubleInlineSet) { diff --git a/test/common/router/header_parser_corpus/clusterfuzz-testcase-header_parser_fuzz_test-5107723602493440 b/test/common/router/header_parser_corpus/clusterfuzz-testcase-header_parser_fuzz_test-5107723602493440 new file mode 100644 index 0000000000000..1866c52aa3098 --- /dev/null +++ b/test/common/router/header_parser_corpus/clusterfuzz-testcase-header_parser_fuzz_test-5107723602493440 @@ -0,0 +1,14 @@ +headers_to_add { +} +headers_to_add { + header { + key: "te" + value: "l" + } +} +headers_to_add { + header { + key: "te" + value: "@" + } +} diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index e25e7b79a4f62..29eaf276d9716 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -120,6 +120,26 @@ stat_prefix: header_test - header: key: "x-real-ip" value: "%DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT%" + - name: append-same-headers + domains: ["append-same-headers.com"] + request_headers_to_add: + - header: + key: "x-foo" + value: "value1" + - header: + key: "authorization" + value: "token1" + routes: + - match: { prefix: "/test" } + route: + cluster: cluster_0 + request_headers_to_add: + - header: + key: "x-foo" + value: "value2" + - header: + key: "authorization" + value: "token2" )EOF"; } // namespace @@ -915,4 +935,39 @@ TEST_P(HeaderIntegrationTest, TestXFFParsing) { }); } +// Validates behavior around same header appending (both predefined headers and +// other). +TEST_P(HeaderIntegrationTest, TestAppendSameHeaders) { + initializeFilter(HeaderMode::Append, false); + performRequest( + Http::TestHeaderMapImpl{ + {":method", "GET"}, + {":path", "/test"}, + {":scheme", "http"}, + {":authority", "append-same-headers.com"}, + {"authorization", "token3"}, + {"x-foo", "value3"}, + }, + Http::TestHeaderMapImpl{ + {":authority", "append-same-headers.com"}, + {":path", "/test"}, + {":method", "GET"}, + {"authorization", "token3,token2,token1"}, + {"x-foo", "value3"}, + {"x-foo", "value2"}, + {"x-foo", "value1"}, + }, + Http::TestHeaderMapImpl{ + {"server", "envoy"}, + {"content-length", "0"}, + {":status", "200"}, + {"x-unmodified", "response"}, + }, + Http::TestHeaderMapImpl{ + {"server", "envoy"}, + {"x-unmodified", "response"}, + {":status", "200"}, + }); +} + } // namespace Envoy