Rename QuicSpdyStream::AreHeadersValid() to ValidatedRequestHeaders() and retain error details in the stream if it returns false. PiperOrigin-RevId: 532920884
diff --git a/quiche/quic/core/http/quic_spdy_client_stream.cc b/quiche/quic/core/http/quic_spdy_client_stream.cc index 2f6eb8d..9ac5897 100644 --- a/quiche/quic/core/http/quic_spdy_client_stream.cc +++ b/quiche/quic/core/http/quic_spdy_client_stream.cc
@@ -6,6 +6,7 @@ #include <utility> +#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "quiche/quic/core/http/quic_client_promised_info.h" #include "quiche/quic/core/http/quic_spdy_client_session.h" @@ -195,9 +196,9 @@ return bytes_sent; } -bool QuicSpdyClientStream::AreHeadersValid( - const QuicHeaderList& header_list) const { - if (!QuicSpdyStream::AreHeadersValid(header_list)) { +bool QuicSpdyClientStream::ValidatedRequestHeaders( + const QuicHeaderList& header_list) { + if (!QuicSpdyStream::ValidatedRequestHeaders(header_list)) { return false; } // Verify the presence of :status header. @@ -206,10 +207,17 @@ if (pair.first == ":status") { saw_status = true; } else if (absl::StrContains(pair.first, ":")) { - QUIC_DLOG(ERROR) << "Unexpected ':' in header " << pair.first << "."; + set_invalid_request_details( + absl::StrCat("Unexpected ':' in header ", pair.first, ".")); + QUIC_DLOG(ERROR) << invalid_request_details(); return false; } } + if (!saw_status) { + set_invalid_request_details("Missing :status in response header."); + QUIC_DLOG(ERROR) << invalid_request_details(); + return false; + } return saw_status; }
diff --git a/quiche/quic/core/http/quic_spdy_client_stream.h b/quiche/quic/core/http/quic_spdy_client_stream.h index 02956c6..35446b0 100644 --- a/quiche/quic/core/http/quic_spdy_client_stream.h +++ b/quiche/quic/core/http/quic_spdy_client_stream.h
@@ -72,7 +72,7 @@ using QuicSpdyStream::SetPriority; protected: - bool AreHeadersValid(const QuicHeaderList& header_list) const override; + bool ValidatedRequestHeaders(const QuicHeaderList& header_list) override; // Called by OnInitialHeadersComplete to set response_header_. Returns false // on error.
diff --git a/quiche/quic/core/http/quic_spdy_server_stream_base.cc b/quiche/quic/core/http/quic_spdy_server_stream_base.cc index 9a23c26..6176032 100644 --- a/quiche/quic/core/http/quic_spdy_server_stream_base.cc +++ b/quiche/quic/core/http/quic_spdy_server_stream_base.cc
@@ -4,6 +4,7 @@ #include "quiche/quic/core/http/quic_spdy_server_stream_base.h" +#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "quiche/quic/core/http/quic_spdy_session.h" #include "quiche/quic/core/quic_error_codes.h" @@ -48,9 +49,9 @@ QuicSpdyStream::StopReading(); } -bool QuicSpdyServerStreamBase::AreHeadersValid( - const QuicHeaderList& header_list) const { - if (!QuicSpdyStream::AreHeadersValid(header_list)) { +bool QuicSpdyServerStreamBase::ValidatedRequestHeaders( + const QuicHeaderList& header_list) { + if (!QuicSpdyStream::ValidatedRequestHeaders(header_list)) { return false; } @@ -84,17 +85,22 @@ } else if (pair.first == ":authority") { saw_authority = true; } else if (absl::StrContains(pair.first, ":")) { - QUIC_DLOG(ERROR) << "Unexpected ':' in header " << pair.first << "."; + set_invalid_request_details( + absl::StrCat("Unexpected ':' in header ", pair.first, ".")); + QUIC_DLOG(ERROR) << invalid_request_details(); return false; } if (is_extended_connect) { if (!spdy_session()->allow_extended_connect()) { - QUIC_DLOG(ERROR) - << "Received extended-CONNECT request while it is disabled."; + set_invalid_request_details( + "Received extended-CONNECT request while it is disabled."); + QUIC_DLOG(ERROR) << invalid_request_details(); return false; } } else if (saw_method && !saw_connect) { if (saw_protocol) { + set_invalid_request_details( + "Received non-CONNECT request with :protocol header."); QUIC_DLOG(ERROR) << "Receive non-CONNECT request with :protocol."; return false; } @@ -106,15 +112,18 @@ // Saw all the required pseudo headers. return true; } - QUIC_DLOG(ERROR) << "Missing required pseudo headers for extended-CONNECT."; + set_invalid_request_details( + "Missing required pseudo headers for extended-CONNECT."); + QUIC_DLOG(ERROR) << invalid_request_details(); return false; } // This is a vanilla CONNECT or non-CONNECT request. if (saw_connect) { // Check vanilla CONNECT. if (saw_path || saw_scheme) { - QUIC_DLOG(ERROR) - << "Received invalid CONNECT request with disallowed pseudo header."; + set_invalid_request_details( + "Received invalid CONNECT request with disallowed pseudo header."); + QUIC_DLOG(ERROR) << invalid_request_details(); return false; } return true; @@ -123,7 +132,8 @@ if (saw_method && saw_authority && saw_path && saw_scheme) { return true; } - QUIC_LOG(ERROR) << "Missing required pseudo headers."; + set_invalid_request_details("Missing required pseudo headers."); + QUIC_LOG(ERROR) << invalid_request_details(); return false; }
diff --git a/quiche/quic/core/http/quic_spdy_server_stream_base.h b/quiche/quic/core/http/quic_spdy_server_stream_base.h index dd3423b..24133d5 100644 --- a/quiche/quic/core/http/quic_spdy_server_stream_base.h +++ b/quiche/quic/core/http/quic_spdy_server_stream_base.h
@@ -23,7 +23,7 @@ void StopReading() override; protected: - bool AreHeadersValid(const QuicHeaderList& header_list) const override; + bool ValidatedRequestHeaders(const QuicHeaderList& header_list) override; }; } // namespace quic
diff --git a/quiche/quic/core/http/quic_spdy_session_test.cc b/quiche/quic/core/http/quic_spdy_session_test.cc index 86c4712..e778864 100644 --- a/quiche/quic/core/http/quic_spdy_session_test.cc +++ b/quiche/quic/core/http/quic_spdy_session_test.cc
@@ -257,7 +257,7 @@ MOCK_METHOD(bool, HasPendingRetransmission, (), (const, override)); protected: - bool AreHeadersValid(const QuicHeaderList& /*header_list*/) const override { + bool ValidatedRequestHeaders(const QuicHeaderList& /*header_list*/) override { return true; } };
diff --git a/quiche/quic/core/http/quic_spdy_stream.cc b/quiche/quic/core/http/quic_spdy_stream.cc index 8f8a5b1..8727363 100644 --- a/quiche/quic/core/http/quic_spdy_stream.cc +++ b/quiche/quic/core/http/quic_spdy_stream.cc
@@ -622,8 +622,11 @@ } // Validate request headers if it did not exceed size limit. If it did, // OnHeadersTooLarge() should have already handled it previously. - if (!header_too_large && !AreHeadersValid(header_list)) { + if (!header_too_large && !ValidatedRequestHeaders(header_list)) { QUIC_CODE_COUNT_N(quic_validate_request_header, 1, 2); + QUICHE_DCHECK(!invalid_request_details().empty()) + << "ValidatedRequestHeaders() returns false without populating " + "invalid_request_details_"; if (GetQuicReloadableFlag(quic_act_upon_invalid_header)) { QUIC_RELOADABLE_FLAG_COUNT(quic_act_upon_invalid_header); OnInvalidHeaders(); @@ -1626,21 +1629,32 @@ } } // namespace -bool QuicSpdyStream::AreHeadersValid(const QuicHeaderList& header_list) const { +bool QuicSpdyStream::ValidatedRequestHeaders( + const QuicHeaderList& header_list) { for (const std::pair<std::string, std::string>& pair : header_list) { const std::string& name = pair.first; if (std::any_of(name.begin(), name.end(), isInvalidHeaderNameCharacter)) { - QUIC_DLOG(ERROR) << "Invalid request header " << name; + invalid_request_details_ = absl::StrCat("Invalid request header ", name); + QUIC_DLOG(ERROR) << invalid_request_details_; return false; } if (http2::GetInvalidHttp2HeaderSet().contains(name)) { - QUIC_DLOG(ERROR) << name << " header is not allowed"; + invalid_request_details_ = absl::StrCat(name, " header is not allowed"); + QUIC_DLOG(ERROR) << invalid_request_details_; return false; } } return true; } +void QuicSpdyStream::set_invalid_request_details( + std::string invalid_request_details) { + QUIC_BUG_IF( + empty invalid request detail, + !invalid_request_details_.empty() || invalid_request_details.empty()); + invalid_request_details_ = std::move(invalid_request_details); +} + bool QuicSpdyStream::AreHeaderFieldValuesValid( const QuicHeaderList& header_list) const { if (!VersionUsesHttp3(transport_version())) {
diff --git a/quiche/quic/core/http/quic_spdy_stream.h b/quiche/quic/core/http/quic_spdy_stream.h index 301df24..98b1a0e 100644 --- a/quiche/quic/core/http/quic_spdy_stream.h +++ b/quiche/quic/core/http/quic_spdy_stream.h
@@ -328,6 +328,10 @@ void WriteGreaseCapsule(); + const std::string& invalid_request_details() const { + return invalid_request_details_; + } + protected: // Called when the received headers are too large. By default this will // reset the stream. @@ -359,15 +363,18 @@ void OnWriteSideInDataRecvdState() override; - virtual bool AreHeadersValid(const QuicHeaderList& header_list) const; - // TODO(b/202433856) Merge AreHeaderFieldValueValid into AreHeadersValid once - // all flags guarding the behavior of AreHeadersValid has been rolled out. + virtual bool ValidatedRequestHeaders(const QuicHeaderList& header_list); + // TODO(b/202433856) Merge AreHeaderFieldValueValid into + // ValidatedRequestHeaders once all flags guarding the behavior of + // ValidatedRequestHeaders has been rolled out. virtual bool AreHeaderFieldValuesValid( const QuicHeaderList& header_list) const; // Reset stream upon invalid request headers. virtual void OnInvalidHeaders(); + void set_invalid_request_details(std::string invalid_request_details); + private: friend class test::QuicSpdyStreamPeer; friend class test::QuicStreamPeer; @@ -493,6 +500,9 @@ Http3DatagramVisitor* datagram_visitor_ = nullptr; // CONNECT-IP support. ConnectIpVisitor* connect_ip_visitor_ = nullptr; + + // Empty if the headers are valid. + std::string invalid_request_details_; }; } // namespace quic
diff --git a/quiche/quic/core/http/quic_spdy_stream_test.cc b/quiche/quic/core/http/quic_spdy_stream_test.cc index e7d68cd..400eeff 100644 --- a/quiche/quic/core/http/quic_spdy_stream_test.cc +++ b/quiche/quic/core/http/quic_spdy_stream_test.cc
@@ -276,10 +276,6 @@ size_t headers_payload_length() const { return headers_payload_length_; } - bool AreHeadersValid(const QuicHeaderList& header_list) const override { - return QuicSpdyStream::AreHeadersValid(header_list); - } - private: bool should_process_data_; spdy::Http2HeaderBlock saved_headers_;