quiche: handle multiple cookies and multiple set-cookie headers

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_coalesce_cookie_headers_
+                                    ? 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

@@ -325,6 +325,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_coalesce_cookie_headers_ = Runtime::runtimeFeatureEnabled(
+      "envoy.reloadable_features.header_map_coalesce_cookie_headers");
 };
 
 /**

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_coalesce_cookie_headers",
 };
 
 // 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,17 @@ 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 (!Runtime::runtimeFeatureEnabled(
+            "envoy.reloadable_features.header_map_coalesce_cookie_headers") ||
+        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 +63,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_coalesce_cookie_headers")) {
+    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_coalesce_cookie_headers")) {
+          // 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

@@ -574,5 +574,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_coalesce_cookie_headers")) {
+    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