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.")
