quiche: handle multiple cookies and multiple set-cookie headers

docs/root/version_history/current.rst

@@ -56,6 +56,7 @@ Bug Fixes
 * filter_chain: fix filter chain matching with the server name as the case-insensitive way.
 * grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling` to false.
 * grpc_http_bridge: the downstream HTTP status is now correctly set for trailers-only responses from the upstream.
+* header map: pick the right delimiter to append multiple header values to the same key. Previouly header with multiple values are coalesced with ",", after this fix cookie headers should be coalesced with " ;". This doesn't affect Http1 or Http2 requests because these 2 codecs coalesce cookie headers before adding it to header map. To revert to the old behavior, set the runtime feature `envoy.reloadable_features.header_map_correctly_coalesce_cookies` to false.
 * http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false.
 * http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false.
 * listener: prevent crashing when an unknown listener config proto is received and debug logging is enabled.

source/common/http/header_map_impl.cc

@@ -428,7 +428,10 @@ 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 uint64_t added_size = appendToHeader(entry[0]->value(), value);
+    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);
     addSize(added_size);
   } else {
     addCopy(key, value);

source/common/http/header_map_impl.h

@@ -329,6 +329,8 @@ class HeaderMapImpl : NonCopyable {
   HeaderList headers_;
   // This holds the internal byte size of the HeaderMap.
   uint64_t cached_byte_size_ = 0;
+  const bool header_map_correctly_coalesce_cookies_ = Runtime::runtimeFeatureEnabled(
+      "envoy.reloadable_features.header_map_correctly_coalesce_cookies");
 };
 
 /**

source/common/runtime/runtime_features.cc

@@ -89,6 +89,7 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.unify_grpc_handling",
     "envoy.reloadable_features.upstream_http2_flood_checks",
     "envoy.restart_features.use_apple_api_for_dns_lookups",
+    "envoy.reloadable_features.header_map_correctly_coalesce_cookies",
 };
 
 // This is a section for officially sanctioned runtime features which are too

source/extensions/quic_listeners/quiche/envoy_quic_utils.cc

@@ -54,7 +54,8 @@ spdy::SpdyHeaderBlock envoyHeadersToSpdyHeaderBlock(const Http::HeaderMap& heade
   spdy::SpdyHeaderBlock header_block;
   headers.iterate([&header_block](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate {
     // The key-value pairs are copied.
-    header_block.insert({header.key().getStringView(), header.value().getStringView()});
+    header_block.AppendValueOrAddHeader(header.key().getStringView(),
+                                        header.value().getStringView());
     return Http::HeaderMap::Iterate::Continue;
   });
   return header_block;

source/extensions/quic_listeners/quiche/envoy_quic_utils.h

@@ -42,8 +42,15 @@ template <class T>
 std::unique_ptr<T> quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list) {
   auto headers = T::create();
   for (const auto& entry : header_list) {
-    // TODO(danzh): Avoid copy by referencing entry as header_list is already validated by QUIC.
-    headers->addCopy(Http::LowerCaseString(entry.first), entry.second);
+    auto key = Http::LowerCaseString(entry.first);
+    if (key != Http::Headers::get().Cookie) {
+      // TODO(danzh): Avoid copy by referencing entry as header_list is already validated by QUIC.
+      headers->addCopy(key, entry.second);
+    } else {
+      // QUICHE breaks "cookie" header into crumbs. Coalesce them by appending current one to
+      // existing one if there is any.
+      headers->appendCopy(key, entry.second);
+    }
   }
   return headers;
 }
@@ -54,8 +61,7 @@ std::unique_ptr<T> spdyHeaderBlockToEnvoyHeaders(const spdy::SpdyHeaderBlock& he
   for (auto entry : header_block) {
     // TODO(danzh): Avoid temporary strings and addCopy() with string_view.
     std::string key(entry.first);
-    std::string value(entry.second);
-    headers->addCopy(Http::LowerCaseString(key), value);
+    headers->addCopy(Http::LowerCaseString(key), entry.second);
   }
   return headers;
 }

test/common/http/header_map_impl_test.cc

@@ -741,6 +741,24 @@ TEST_P(HeaderMapImplTest, DoubleCookieAdd) {
   ASSERT_EQ(set_cookie_value[1]->value().getStringView(), "bar");
 }
 
+TEST_P(HeaderMapImplTest, AppendCookieHeadersWithSemicolon) {
+  if (!Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.header_map_correctly_coalesce_cookies")) {
+    return;
+  }
+  TestRequestHeaderMapImpl headers;
+  const std::string foo("foo=1");
+  const std::string bar("bar=2");
+  const LowerCaseString& cookie = Http::Headers::get().Cookie;
+  headers.addReference(cookie, foo);
+  headers.appendCopy(cookie, bar);
+  EXPECT_EQ(1UL, headers.size());
+
+  const auto cookie_value = headers.get(LowerCaseString("cookie"));
+  ASSERT_EQ(cookie_value.size(), 1);
+  ASSERT_EQ(cookie_value[0]->value().getStringView(), "foo=1; bar=2");
+}
+
 TEST_P(HeaderMapImplTest, DoubleInlineSet) {
   TestRequestHeaderMapImpl headers;
   headers.setReferenceKey(Headers::get().ContentType, "blah");

test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc

@@ -183,6 +183,9 @@ TEST_P(EnvoyQuicServerStreamTest, GetRequestAndResponse) {
   request_headers.OnHeader(":authority", host_);
   request_headers.OnHeader(":method", "GET");
   request_headers.OnHeader(":path", "/");
+  // QUICHE stack doesn't coalesce Cookie headers for header compression optimization.
+  request_headers.OnHeader("cookie", "a=b");
+  request_headers.OnHeader("cookie", "c=d");
   request_headers.OnHeaderBlockEnd(/*uncompressed_header_bytes=*/0,
                                    /*compressed_header_bytes=*/0);
 
@@ -192,13 +195,22 @@ TEST_P(EnvoyQuicServerStreamTest, GetRequestAndResponse) {
         EXPECT_EQ(host_, headers->getHostValue());
         EXPECT_EQ("/", headers->getPathValue());
         EXPECT_EQ(Http::Headers::get().MethodValues.Get, headers->getMethodValue());
+        if (Runtime::runtimeFeatureEnabled(
+                "envoy.reloadable_features.header_map_correctly_coalesce_cookies")) {
+          // Verify that the duplicated headers are handled correctly before passing to stream
+          // decoder.
+          EXPECT_EQ("a=b; c=d",
+                    headers->get(Http::Headers::get().Cookie)[0]->value().getStringView());
+        }
       }));
   if (quic::VersionUsesHttp3(quic_version_.transport_version)) {
     EXPECT_CALL(stream_decoder_, decodeData(BufferStringEqual(""), /*end_stream=*/true));
     spdy::SpdyHeaderBlock spdy_headers;
     spdy_headers[":authority"] = host_;
     spdy_headers[":method"] = "GET";
     spdy_headers[":path"] = "/";
+    spdy_headers.AppendValueOrAddHeader("cookie", "a=b");
+    spdy_headers.AppendValueOrAddHeader("cookie", "c=d");
     std::string payload = spdyHeaderToHttp3StreamPayload(spdy_headers);
     quic::QuicStreamFrame frame(stream_id_, true, 0, payload);
     quic_stream_->OnStreamFrame(frame);

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc

@@ -571,5 +571,38 @@ TEST_P(QuicHttpIntegrationTest, Reset101SwitchProtocolResponse) {
   EXPECT_FALSE(response->complete());
 }
 
+TEST_P(QuicHttpIntegrationTest, MultipleSetCookieAndCookieHeaders) {
+  initialize();
+
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto encoder_decoder =
+      codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "GET"},
+                                                                 {":path", "/dynamo/url"},
+                                                                 {":scheme", "http"},
+                                                                 {":authority", "host"},
+                                                                 {"cookie", "a=b"},
+                                                                 {"cookie", "c=d"}});
+  request_encoder_ = &encoder_decoder.first;
+  auto response = std::move(encoder_decoder.second);
+  codec_client_->sendData(*request_encoder_, 0, true);
+  waitForNextUpstreamRequest();
+  if (Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.header_map_correctly_coalesce_cookies")) {
+    EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Cookie)[0]->value(),
+              "a=b; c=d");
+  }
+
+  upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"},
+                                                                   {"set-cookie", "foo"},
+                                                                   {"set-cookie", "bar"}},
+                                   true);
+  response->waitForEndStream();
+  EXPECT_TRUE(response->complete());
+  const auto out = response->headers().get(Http::LowerCaseString("set-cookie"));
+  ASSERT_EQ(out.size(), 2);
+  ASSERT_EQ(out[0]->value().getStringView(), "foo");
+  ASSERT_EQ(out[1]->value().getStringView(), "bar");
+}
+
 } // namespace Quic
 } // namespace Envoy