http: support appending to predefined inline headers. (#4211)

Previously we just ASSERTed when performing an addReferenceKey() with a
predefined header. This PR adds support for comma concatenation and
adjusts docs to make clear the contract for value lifetime across the
API surface.

Fixes #3919. Also resolves ClusterFuzz issue
https://oss-fuzz.com/v2/testcase-detail/5107723602493440?noredirect=1.

Risk level: High. This affects header processing and wire representation
of predefined headers when appending.
Testing: Unit/integration tests, corpus entry added.

Signed-off-by: Harvey Tuch <htuch@google.com>

docs/root/intro/version_history.rst

@@ -39,6 +39,14 @@ Version history
 * http: added generic :ref:`Upgrade support 
   <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.upgrade_configs>`.
 * 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
+  <envoy_api_field_route.Route.request_headers_to_add>`. For example, a
+  request header *authorization: token1* will appear as *authorization:
+  token1,token2*, after having :ref:`request_headers_to_add
+  <envoy_api_field_route.Route.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 <envoy_api_field_core.Http2ProtocolOptions.hpack_table_size>` now controls
   dynamic table size of both: encoder and decoder.

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.

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<char*>(malloc(dynamic_capacity_));
+    RELEASE_ASSERT(buf != nullptr, "");
+    memcpy(buf, buffer_.ref_, string_length_);
+    buffer_.dynamic_ = buf;
+    break;
   }
 
   case Type::Inline: {
@@ -94,13 +96,15 @@ void HeaderString::append(const char* data, uint32_t size) {
       RELEASE_ASSERT(new_capacity <= std::numeric_limits<uint32_t>::max(), "");
       buffer_.dynamic_ = static_cast<char*>(malloc(new_capacity));
       memcpy(buffer_.dynamic_, inline_buffer_, string_length_);
+      RELEASE_ASSERT(buffer_.dynamic_ != nullptr, "");
       dynamic_capacity_ = new_capacity;
       type_ = Type::Dynamic;
     } else {
       if (size + 1 + string_length_ > dynamic_capacity_) {
         // Need to reallocate.
         dynamic_capacity_ = (string_length_ + size) * 2;
         buffer_.dynamic_ = static_cast<char*>(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<HeaderEntryImpl>::iterator i = headers_.insert(std::move(key), std::move(value));
     i->entry_ = i;

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) {

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: "@"
+  }
+}

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