Automated g4 rollback of changelist 587014516. *** Reason for rollback *** Causes problems for open source Envoy. *** Original change description *** Re-adds some minimal validation to NoopHeaderValidator. The minimal set of checks necessary for Envoy fuzz tests to succeed are: * header names and values may not contain \0, \r or \n * requests and responses must contain the minimal required set of pseudo-headers *** PiperOrigin-RevId: 588076959
diff --git a/quiche/http2/adapter/noop_header_validator.cc b/quiche/http2/adapter/noop_header_validator.cc index b687df5..f39342d 100644 --- a/quiche/http2/adapter/noop_header_validator.cc +++ b/quiche/http2/adapter/noop_header_validator.cc
@@ -1,56 +1,20 @@ #include "quiche/http2/adapter/noop_header_validator.h" #include "absl/strings/escaping.h" -#include "absl/strings/string_view.h" -#include "quiche/http2/adapter/header_validator_base.h" #include "quiche/common/platform/api/quiche_logging.h" namespace http2 { namespace adapter { -namespace { - -constexpr absl::string_view kInvalidChars("\0\r\n", 3); - -} // namespace - -void NoopHeaderValidator::StartHeaderBlock() { - HeaderValidatorBase::StartHeaderBlock(); - has_method_ = false; - has_scheme_ = false; - has_path_ = false; -} - HeaderValidatorBase::HeaderStatus NoopHeaderValidator::ValidateSingleHeader( absl::string_view key, absl::string_view value) { - if (key.empty()) { - return HEADER_FIELD_INVALID; - } - if (key.find_first_of(kInvalidChars) != absl::string_view::npos || - value.find_first_of(kInvalidChars) != absl::string_view::npos) { - return HEADER_FIELD_INVALID; - } - if (key[0] != ':') { - return HEADER_OK; - } if (key == ":status") { status_ = std::string(value); - } else if (key == ":method") { - has_method_ = true; - } else if (key == ":scheme") { - has_scheme_ = true; - } else if (key == ":path") { - has_path_ = true; } return HEADER_OK; } -bool NoopHeaderValidator::FinishHeaderBlock(HeaderType type) { - if (type == HeaderType::REQUEST) { - return has_method_ && has_scheme_ && has_path_; - } else if (type == HeaderType::RESPONSE || type == HeaderType::RESPONSE_100) { - return !status_.empty(); - } +bool NoopHeaderValidator::FinishHeaderBlock(HeaderType /* type */) { return true; }
diff --git a/quiche/http2/adapter/noop_header_validator.h b/quiche/http2/adapter/noop_header_validator.h index ef78027..f6b95e9 100644 --- a/quiche/http2/adapter/noop_header_validator.h +++ b/quiche/http2/adapter/noop_header_validator.h
@@ -13,16 +13,10 @@ public: NoopHeaderValidator() = default; - void StartHeaderBlock() override; HeaderStatus ValidateSingleHeader(absl::string_view key, absl::string_view value) override; bool FinishHeaderBlock(HeaderType type) override; - - private: - bool has_method_ = false; - bool has_scheme_ = false; - bool has_path_ = false; }; } // namespace adapter
diff --git a/quiche/http2/adapter/noop_header_validator_test.cc b/quiche/http2/adapter/noop_header_validator_test.cc index a6dc041..65cc763 100644 --- a/quiche/http2/adapter/noop_header_validator_test.cc +++ b/quiche/http2/adapter/noop_header_validator_test.cc
@@ -5,7 +5,6 @@ #include <vector> #include "absl/strings/str_cat.h" -#include "quiche/http2/adapter/header_validator_base.h" #include "quiche/common/platform/api/quiche_test.h" namespace http2 { @@ -20,20 +19,11 @@ {":path", "/foo"}, {":scheme", "https"}}; -TEST(NoopHeaderValidatorTest, EmptyHeaderBlock) { - NoopHeaderValidator v; - v.StartHeaderBlock(); - EXPECT_FALSE(v.FinishHeaderBlock(HeaderType::REQUEST)); - - v.StartHeaderBlock(); - EXPECT_FALSE(v.FinishHeaderBlock(HeaderType::RESPONSE)); -} - TEST(NoopHeaderValidatorTest, HeaderNameEmpty) { NoopHeaderValidator v; NoopHeaderValidator::HeaderStatus status = v.ValidateSingleHeader("", "value"); - EXPECT_EQ(NoopHeaderValidator::HEADER_FIELD_INVALID, status); + EXPECT_EQ(NoopHeaderValidator::HEADER_OK, status); } TEST(NoopHeaderValidatorTest, HeaderValueEmpty) { @@ -54,29 +44,27 @@ EXPECT_EQ(NoopHeaderValidator::HEADER_OK, status); } -TEST(NoopHeaderValidatorTest, FewInvalidNameChars) { +TEST(NoopHeaderValidatorTest, AnyNameCharIsValid) { NoopHeaderValidator v; char pseudo_name[] = ":met hod"; char name[] = "na me"; for (int i = std::numeric_limits<char>::min(); i < std::numeric_limits<char>::max(); ++i) { char c = static_cast<char>(i); - const HeaderValidatorBase::HeaderStatus expected_status = - (c == '\0' || c == '\r' || c == '\n') - ? HeaderValidatorBase::HEADER_FIELD_INVALID - : HeaderValidatorBase::HEADER_OK; // Test a pseudo-header name with this char. pseudo_name[3] = c; auto sv = absl::string_view(pseudo_name, 8); - EXPECT_EQ(expected_status, v.ValidateSingleHeader(sv, "value")); + EXPECT_EQ(NoopHeaderValidator::HEADER_OK, + v.ValidateSingleHeader(sv, "value")); // Test a regular header name with this char. name[2] = c; sv = absl::string_view(name, 5); - EXPECT_EQ(expected_status, v.ValidateSingleHeader(sv, "value")); + EXPECT_EQ(NoopHeaderValidator::HEADER_OK, + v.ValidateSingleHeader(sv, "value")); } } -TEST(NoopHeaderValidatorTest, FewInvalidValueChars) { +TEST(NoopHeaderValidatorTest, AnyValueCharIsValid) { NoopHeaderValidator v; char value[] = "val ue"; for (int i = std::numeric_limits<char>::min(); @@ -84,11 +72,8 @@ char c = static_cast<char>(i); value[3] = c; auto sv = absl::string_view(value, 6); - const HeaderValidatorBase::HeaderStatus expected_status = - (c == '\0' || c == '\r' || c == '\n') - ? HeaderValidatorBase::HEADER_FIELD_INVALID - : HeaderValidatorBase::HEADER_OK; - EXPECT_EQ(expected_status, v.ValidateSingleHeader("name", sv)); + EXPECT_EQ(NoopHeaderValidator::HEADER_OK, + v.ValidateSingleHeader("name", sv)); } } @@ -118,21 +103,18 @@ } } -TEST(NoopHeaderValidatorTest, FewInvalidAuthorityChars) { +TEST(NoopHeaderValidatorTest, AnyAuthorityCharIsValid) { char value[] = "ho st.example.com"; for (int i = std::numeric_limits<char>::min(); i < std::numeric_limits<char>::max(); ++i) { char c = static_cast<char>(i); value[2] = c; auto sv = absl::string_view(value, 17); - const HeaderValidatorBase::HeaderStatus expected_status = - (c == '\0' || c == '\r' || c == '\n') - ? HeaderValidatorBase::HEADER_FIELD_INVALID - : HeaderValidatorBase::HEADER_OK; for (absl::string_view key : {":authority", "host"}) { NoopHeaderValidator v; v.StartHeaderBlock(); - EXPECT_EQ(expected_status, v.ValidateSingleHeader(key, sv)); + EXPECT_EQ(NoopHeaderValidator::HEADER_OK, + v.ValidateSingleHeader(key, sv)); } } } @@ -170,13 +152,8 @@ v.ValidateSingleHeader(to_add.first, to_add.second)); } } - // If the missing pseudo-header is :authority, final validation will - // succeed. Otherwise, it will fail. - if (to_skip.first == ":authority") { - EXPECT_TRUE(v.FinishHeaderBlock(HeaderType::REQUEST)); - } else { - EXPECT_FALSE(v.FinishHeaderBlock(HeaderType::REQUEST)); - } + // Even if a pseudo-header is missing, final validation will succeed. + EXPECT_TRUE(v.FinishHeaderBlock(HeaderType::REQUEST)); } // When all pseudo-headers are present, final validation will succeed. @@ -320,11 +297,11 @@ NoopHeaderValidator v; for (HeaderType type : {HeaderType::RESPONSE, HeaderType::RESPONSE_100}) { - // When `:status` is missing, validation fails. + // When `:status` is missing, validation succeeds. v.StartHeaderBlock(); EXPECT_EQ(NoopHeaderValidator::HEADER_OK, v.ValidateSingleHeader("foo", "bar")); - EXPECT_FALSE(v.FinishHeaderBlock(type)); + EXPECT_TRUE(v.FinishHeaderBlock(type)); // When all pseudo-headers are present, final validation succeeds. v.StartHeaderBlock();