diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index b22784f194d9..61f4ab33ec5c 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -68,6 +68,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. * jwt_authn: reject requests with a proper error if JWT has the wrong issuer when allow_missing is used. Before this change, the requests are accepted. diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 6a4c00e5663e..4dd22ee29b56 100644 --- a/source/common/http/header_map_impl.cc +++ b/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); diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 442cddd79b24..1a87b2c4df71 100644 --- a/source/common/http/header_map_impl.h +++ b/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"); }; /** diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 4d7d0ac79369..d7200a84ad13 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -91,6 +91,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 diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_utils.cc b/source/extensions/quic_listeners/quiche/envoy_quic_utils.cc index 611a7a28d6f3..abb70d9093f2 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_utils.cc +++ b/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; diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_utils.h b/source/extensions/quic_listeners/quiche/envoy_quic_utils.h index 3c29b28ef21f..2e9c247d4722 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_utils.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_utils.h @@ -42,8 +42,15 @@ template std::unique_ptr 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 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; } diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 40ccb33d7bf7..1605ee6f4634 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/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"); diff --git a/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc b/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc index 15e986255dbb..eb0125b25fc3 100644 --- a/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc +++ b/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,6 +195,13 @@ 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)); @@ -199,6 +209,8 @@ TEST_P(EnvoyQuicServerStreamTest, GetRequestAndResponse) { 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); diff --git a/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc b/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc index 7c97378d009c..b6061c71602d 100644 --- a/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc +++ b/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