http: support appending to predefined inline headers.

docs/root/intro/version_history.rst

@@ -38,6 +38,10 @@ 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>`.
 * 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

@@ -323,8 +323,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

@@ -427,8 +427,8 @@ 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());
+  headers.addReferenceKey(Headers::get().ContentLength, 6);
+  EXPECT_STREQ("5,6", headers.ContentLength()->value().c_str());
   EXPECT_EQ(1UL, headers.size());
 }
 

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