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_;
