Always reset content length status when removing the Content-Length header in BalsaHeaders. Currently, when removing the "Content-Length" header, BalsaHeaders does not reset its internal content length status if "Transfer-Encoding: chunked" is also present. This can lead to inconsistencies where `content_length_valid()` remains true even after the "Content-Length" header is gone. This change introduces a new reloadable flag, `gfe2_reloadable_flag_reset_content_length_status_when_removing_content_length_header`, which, when true, ensures that the content length status is always reset when "Content-Length" is removed, regardless of the presence of "Transfer-Encoding: chunked". Tests are added to cover a scenario where both headers are initially present in a request. I manually e2e tested, and confirmed that * When gfe2_reloadable_flag_reset_content_length_status_when_removing_content_length_header is false, we saw `REQUIRED_BODY_BUT_NO_CONTENT_LENGTH` in the test backend http://screen/C7QyLfJxaEwCtsA. * When the feature flag is true, the response code is 200, and the request headers sent by GFE has `transfer-encoding`: http://screen/9yDWwpGSFhnYVZi I am now writing the nice automated e2e test that requires changes in the implementation. Given the tight timeline of the product launch, I don't think the automated test is blocking this change. Protected by gfe2_reloadable_flag_reset_content_length_status_when_removing_content_length_header. PiperOrigin-RevId: 836699619
diff --git a/quiche/balsa/balsa_headers.cc b/quiche/balsa/balsa_headers.cc index ae4ce40..dcb3aa2 100644 --- a/quiche/balsa/balsa_headers.cc +++ b/quiche/balsa/balsa_headers.cc
@@ -22,8 +22,10 @@ #include "absl/strings/string_view.h" #include "quiche/balsa/balsa_enums.h" #include "quiche/balsa/header_properties.h" +#include "quiche/common/platform/api/quiche_flags.h" #include "quiche/common/platform/api/quiche_header_policy.h" #include "quiche/common/platform/api/quiche_logging.h" +#include "quiche/common/quiche_feature_flags_list.h" namespace { @@ -323,7 +325,9 @@ // header we're removing is one of those headers. void BalsaHeaders::MaybeClearSpecialHeaderValues(absl::string_view key) { if (absl::EqualsIgnoreCase(key, kContentLength)) { - if (transfer_encoding_is_chunked_) { + if (transfer_encoding_is_chunked_ && + !GetQuicheReloadableFlag( + reset_content_length_status_when_removing_content_length_header)) { return; }
diff --git a/quiche/balsa/balsa_headers_test.cc b/quiche/balsa/balsa_headers_test.cc index b00ca2b..f67a078 100644 --- a/quiche/balsa/balsa_headers_test.cc +++ b/quiche/balsa/balsa_headers_test.cc
@@ -27,6 +27,7 @@ #include "quiche/balsa/http_validation_policy.h" #include "quiche/balsa/simple_buffer.h" #include "quiche/common/platform/api/quiche_expect_bug.h" +#include "quiche/common/platform/api/quiche_flags.h" #include "quiche/common/platform/api/quiche_logging.h" #include "quiche/common/platform/api/quiche_test.h" @@ -47,6 +48,11 @@ size_t size) { headers->WriteFromFramer(ptr, size); } + + static void set_transfer_encoding_is_chunked(BalsaHeaders* headers, + bool value) { + headers->transfer_encoding_is_chunked_ = value; + } }; namespace { @@ -3164,7 +3170,24 @@ EXPECT_FALSE(headers.transfer_encoding_is_chunked()); } -TEST(BalsaHeaders, ClearContentLength) { +class BalsaHeadersClearContentLengthTest : public QuicheTestWithParam<bool> { + public: + BalsaHeadersClearContentLengthTest() + : flag_reset_content_length_status_when_removing_content_length_header_( + GetParam()) { + SetQuicheReloadableFlag( + reset_content_length_status_when_removing_content_length_header, + flag_reset_content_length_status_when_removing_content_length_header_); + } + + protected: + bool flag_reset_content_length_status_when_removing_content_length_header_; +}; + +INSTANTIATE_TEST_SUITE_P(BalsaHeadersClearContentLengthTest, + BalsaHeadersClearContentLengthTest, testing::Bool()); + +TEST_P(BalsaHeadersClearContentLengthTest, ClearContentLength) { // Test that ClearContentLength() removes the content-length header and // resets content_length_status(). BalsaHeaders headers; @@ -3198,9 +3221,34 @@ EXPECT_EQ(BalsaHeadersEnums::NO_CONTENT_LENGTH, headers.content_length_status()); EXPECT_FALSE(headers.content_length_valid()); + + // Populate the content-length header and set chunked encoding, to simulate + // the case where both content-length and chunked encoding are set in the + // header. Then test that ClearContentLength() removes the content-length + // header and resets content_length_status(). + headers.SetContentLength(10); + // We cannot use SetTransferEncodingToChunkedAndClearContentLength() here + // because it will clear the content-length header internally. + headers.ReplaceOrAppendHeader("Transfer-Encoding", "chunked"); + BalsaHeadersTestPeer::set_transfer_encoding_is_chunked(&headers, true); + headers.ClearContentLength(); + EXPECT_EQ("chunked", headers.GetAllOfHeaderAsString("Transfer-Encoding")); + EXPECT_TRUE(headers.transfer_encoding_is_chunked()); + EXPECT_FALSE(headers.HasHeader("Content-length")); + if (flag_reset_content_length_status_when_removing_content_length_header_) { + EXPECT_EQ(BalsaHeadersEnums::NO_CONTENT_LENGTH, + headers.content_length_status()); + EXPECT_FALSE(headers.content_length_valid()); + EXPECT_EQ(0u, headers.content_length()); + } else { + EXPECT_EQ(BalsaHeadersEnums::VALID_CONTENT_LENGTH, + headers.content_length_status()); + EXPECT_TRUE(headers.content_length_valid()); + EXPECT_EQ(10u, headers.content_length()); + } } -TEST(BalsaHeaders, ClearContentLengthByRemoveHeader) { +TEST_P(BalsaHeadersClearContentLengthTest, ClearContentLengthByRemoveHeader) { // Test that calling Remove() methods to clear the content-length header // correctly resets internal content length fields. BalsaHeaders headers; @@ -3228,8 +3276,67 @@ EXPECT_FALSE(headers.content_length_valid()); } +// Test that calling Remove() methods to clear the content-length header +// correctly resets internal content length fields when the Transfer-Encoding +// header is present. +TEST_P(BalsaHeadersClearContentLengthTest, + ClearContentLengthByRemoveHeaderWithTransferEncodingHeader) { + BalsaHeaders headers; + headers.SetContentLength(10); + // We cannot use SetTransferEncodingToChunkedAndClearContentLength() here + // because it will clear the content-length header internally. + headers.ReplaceOrAppendHeader("Transfer-Encoding", "chunked"); + BalsaHeadersTestPeer::set_transfer_encoding_is_chunked(&headers, true); + headers.RemoveAllOfHeader("Content-Length"); + if (flag_reset_content_length_status_when_removing_content_length_header_) { + EXPECT_EQ(BalsaHeadersEnums::NO_CONTENT_LENGTH, + headers.content_length_status()); + EXPECT_EQ(0u, headers.content_length()); + EXPECT_FALSE(headers.content_length_valid()); + } else { + EXPECT_EQ(BalsaHeadersEnums::VALID_CONTENT_LENGTH, + headers.content_length_status()); + EXPECT_EQ(10u, headers.content_length()); + EXPECT_TRUE(headers.content_length_valid()); + } + + headers.SetContentLength(11); + headers.ReplaceOrAppendHeader("Transfer-Encoding", "chunked"); + BalsaHeadersTestPeer::set_transfer_encoding_is_chunked(&headers, true); + std::vector<absl::string_view> headers_to_remove; + headers_to_remove.emplace_back("Content-Length"); + headers.RemoveAllOfHeaderInList(headers_to_remove); + if (flag_reset_content_length_status_when_removing_content_length_header_) { + EXPECT_EQ(BalsaHeadersEnums::NO_CONTENT_LENGTH, + headers.content_length_status()); + EXPECT_EQ(0u, headers.content_length()); + EXPECT_FALSE(headers.content_length_valid()); + } else { + EXPECT_EQ(BalsaHeadersEnums::VALID_CONTENT_LENGTH, + headers.content_length_status()); + EXPECT_EQ(11u, headers.content_length()); + EXPECT_TRUE(headers.content_length_valid()); + } + + headers.SetContentLength(12); + headers.ReplaceOrAppendHeader("Transfer-Encoding", "chunked"); + BalsaHeadersTestPeer::set_transfer_encoding_is_chunked(&headers, true); + headers.RemoveAllHeadersWithPrefix("Content"); + if (flag_reset_content_length_status_when_removing_content_length_header_) { + EXPECT_EQ(BalsaHeadersEnums::NO_CONTENT_LENGTH, + headers.content_length_status()); + EXPECT_EQ(0u, headers.content_length()); + EXPECT_FALSE(headers.content_length_valid()); + } else { + EXPECT_EQ(BalsaHeadersEnums::VALID_CONTENT_LENGTH, + headers.content_length_status()); + EXPECT_EQ(12u, headers.content_length()); + EXPECT_TRUE(headers.content_length_valid()); + } +} + // Chunk-encoding an identity-coded BalsaHeaders removes the identity-coding. -TEST(BalsaHeaders, IdentityCodingToChunked) { +TEST_P(BalsaHeadersClearContentLengthTest, IdentityCodingToChunked) { std::string message = "HTTP/1.1 200 OK\r\n" "Transfer-Encoding: identity\r\n\r\n"; @@ -3253,7 +3360,7 @@ ElementsAre("chunked")); } -TEST(BalsaHeaders, SwitchContentLengthToChunk) { +TEST_P(BalsaHeadersClearContentLengthTest, SwitchContentLengthToChunk) { // Test that a header originally with content length header is correctly // switched to using chunk encoding. BalsaHeaders headers; @@ -3272,7 +3379,7 @@ EXPECT_FALSE(headers.content_length_valid()); } -TEST(BalsaHeaders, SwitchChunkedToContentLength) { +TEST_P(BalsaHeadersClearContentLengthTest, SwitchChunkedToContentLength) { // Test that a header originally with chunk encoding is correctly // switched to using content length. BalsaHeaders headers;
diff --git a/quiche/common/quiche_feature_flags_list.h b/quiche/common/quiche_feature_flags_list.h index aec984a..cabd955 100755 --- a/quiche/common/quiche_feature_flags_list.h +++ b/quiche/common/quiche_feature_flags_list.h
@@ -61,6 +61,7 @@ QUICHE_FLAG(bool, quiche_reloadable_flag_quic_testonly_default_true, true, true, "A testonly reloadable flag that will always default to true.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_use_proof_source_get_cert_chains, false, true, "When true, quic::TlsServerHandshaker will use ProofSource::GetCertChains() instead of ProofSource::GetCertChain()") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_use_received_client_addresses_cache, true, true, "If true, use a LRU cache to record client addresses of packets received on server's original address.") +QUICHE_FLAG(bool, quiche_reloadable_flag_reset_content_length_status_when_removing_content_length_header, false, false, "If true, the content_length_status field in balsa_headers will always be reset when the Content-Length header is removed, no matter if the Transfer-Encoding header is chuncked or not.") QUICHE_FLAG(bool, quiche_restart_flag_quic_dispatcher_close_connection_on_invalid_ack, false, false, "An invalid ack is an ack that the peer sent for a packet that was not sent by the dispatcher. If true, the dispatcher will close the connection if it receives an invalid ack.") QUICHE_FLAG(bool, quiche_restart_flag_quic_shed_tls_handshake_config, false, false, "If true, QUIC connections will call SSL_set_shed_handshake_config to drop BoringSSL handshake state after the handshake finishes in order to save memory.") QUICHE_FLAG(bool, quiche_restart_flag_quic_stop_parsing_legacy_version_info, false, false, "If true, disable parsing the legacy version information transport parameter.")