Revert cl/535379163 and cl/538613643. These two CLs cause the following tests to fail in Envoy: //test/integration:quic_protocol_integration_test //test/integration:integration_admin_test --test_filter //Protocols/IntegrationAdminTest.Admin/IPv4_Http2Downstream_HttpUpstreamHttpParserOghttp2NoDeferredProcessing //test/integration:default_header_validator_integration_test --test_filter //Protocols/DownstreamUhvIntegrationTest.BackslashInUriPathConversionWithUhvOverride/IPv4_Http2Downstream_HttpUpstreamHttpParserOghttp2NoDeferredProcessing PiperOrigin-RevId: 542058237
diff --git a/quiche/http2/adapter/header_validator.cc b/quiche/http2/adapter/header_validator.cc index 272111d..fca0e8d 100644 --- a/quiche/http2/adapter/header_validator.cc +++ b/quiche/http2/adapter/header_validator.cc
@@ -33,14 +33,6 @@ "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-._~%!$&'()[" "]*+,;=:"; -const absl::string_view kValidPathChars = - "/abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-._~%!$&'()" - "*+,;=:@?"; - -const absl::string_view kValidPathCharsWithFragment = - "/abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-._~%!$&'()" - "*+,;=:@?#"; - using CharMap = std::array<bool, 256>; CharMap BuildValidCharMap(absl::string_view valid_chars) { @@ -199,7 +191,8 @@ } else if (key == ":authority" && !ValidateAndSetAuthority(value)) { return HEADER_FIELD_INVALID; } else if (key == ":path") { - if (value.empty() || !IsValidPath(value, allow_fragment_in_path_)) { + if (value.empty()) { + // For now, reject an empty path regardless of scheme. return HEADER_FIELD_INVALID; } path_ = std::string(value); @@ -273,17 +266,6 @@ return AllCharsInMap(authority, valid_chars); } -bool HeaderValidator::IsValidPath(absl::string_view path, bool allow_fragment) { - static const CharMap valid_chars = BuildValidCharMap(kValidPathChars); - static const CharMap valid_chars_with_fragment = - BuildValidCharMap(kValidPathCharsWithFragment); - if (allow_fragment) { - return AllCharsInMap(path, valid_chars_with_fragment); - } else { - return AllCharsInMap(path, valid_chars); - } -} - HeaderValidator::ContentLengthStatus HeaderValidator::HandleContentLength( absl::string_view value) { if (value.empty()) {
diff --git a/quiche/http2/adapter/header_validator.h b/quiche/http2/adapter/header_validator.h index aa87423..a6070db 100644 --- a/quiche/http2/adapter/header_validator.h +++ b/quiche/http2/adapter/header_validator.h
@@ -33,10 +33,6 @@ // Returns whether `authority` is valid according to RFC 3986 Section 3.2. static bool IsValidAuthority(absl::string_view authority); - // Returns whether `path` is valid according to RFC 3986 Section 3.3. May - // contain the query part of a URI. - static bool IsValidPath(absl::string_view path, bool allow_fragment); - private: enum ContentLengthStatus { CONTENT_LENGTH_OK,
diff --git a/quiche/http2/adapter/header_validator_base.h b/quiche/http2/adapter/header_validator_base.h index ce6c9a3..2b25afa 100644 --- a/quiche/http2/adapter/header_validator_base.h +++ b/quiche/http2/adapter/header_validator_base.h
@@ -55,7 +55,6 @@ void SetObsTextOption(ObsTextOption option) { obs_text_option_ = option; } // Allows the "extended CONNECT" syntax described in RFC 8441. void SetAllowExtendedConnect() { allow_extended_connect_ = true; } - void SetAllowFragmentInPath() { allow_fragment_in_path_ = true; } protected: std::string status_; @@ -63,7 +62,6 @@ absl::optional<size_t> content_length_; ObsTextOption obs_text_option_ = ObsTextOption::kDisallow; bool allow_extended_connect_ = false; - bool allow_fragment_in_path_ = false; }; } // namespace adapter
diff --git a/quiche/http2/adapter/header_validator_test.cc b/quiche/http2/adapter/header_validator_test.cc index e8baa32..f939c2b 100644 --- a/quiche/http2/adapter/header_validator_test.cc +++ b/quiche/http2/adapter/header_validator_test.cc
@@ -492,8 +492,7 @@ for (Header to_add : kSampleRequestPseudoheaders) { if (to_add.first == ":path") { EXPECT_EQ(HeaderValidator::HEADER_OK, - validator.ValidateSingleHeader(to_add.first, value)) - << "Problematic char: [" << c << "]"; + validator.ValidateSingleHeader(to_add.first, value)); } else { EXPECT_EQ(HeaderValidator::HEADER_OK, validator.ValidateSingleHeader(to_add.first, to_add.second)); @@ -502,38 +501,21 @@ EXPECT_TRUE(validator.FinishHeaderBlock(HeaderType::REQUEST)); } - // Various invalid path characters. - for (const absl::string_view c : {"[", "<", "}", "`", "\\", " ", "\t", "#"}) { + // BUG: Various invalid path characters. + for (const absl::string_view c : {"[", "<", "}", "`", "\\", " ", "\t"}) { const std::string value = absl::StrCat("/shawa", c, "rma"); HeaderValidator validator; validator.StartHeaderBlock(); for (Header to_add : kSampleRequestPseudoheaders) { if (to_add.first == ":path") { - EXPECT_EQ(HeaderValidator::HEADER_FIELD_INVALID, + EXPECT_EQ(HeaderValidator::HEADER_OK, validator.ValidateSingleHeader(to_add.first, value)); } else { EXPECT_EQ(HeaderValidator::HEADER_OK, validator.ValidateSingleHeader(to_add.first, to_add.second)); } } - EXPECT_FALSE(validator.FinishHeaderBlock(HeaderType::REQUEST)); - } - - // The fragment initial character can be explicitly allowed. - { - HeaderValidator validator; - validator.SetAllowFragmentInPath(); - validator.StartHeaderBlock(); - for (Header to_add : kSampleRequestPseudoheaders) { - if (to_add.first == ":path") { - EXPECT_EQ(HeaderValidator::HEADER_OK, - validator.ValidateSingleHeader(to_add.first, "/shawa#rma")); - } else { - EXPECT_EQ(HeaderValidator::HEADER_OK, - validator.ValidateSingleHeader(to_add.first, to_add.second)); - } - } EXPECT_TRUE(validator.FinishHeaderBlock(HeaderType::REQUEST)); } }
diff --git a/quiche/http2/adapter/oghttp2_adapter_test.cc b/quiche/http2/adapter/oghttp2_adapter_test.cc index c6f4d47..9b3c193 100644 --- a/quiche/http2/adapter/oghttp2_adapter_test.cc +++ b/quiche/http2/adapter/oghttp2_adapter_test.cc
@@ -171,19 +171,18 @@ EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "GET")); EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https")); EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com")); - EXPECT_CALL( - visitor, - OnInvalidFrame(1, Http2VisitorInterface::InvalidFrameError::kHttpHeader)); - + EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/ fragment")); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnEndStream(1)); // Stream 3 EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 5)); EXPECT_CALL(visitor, OnBeginHeadersForStream(3)); EXPECT_CALL(visitor, OnHeaderForStream(3, ":method", "GET")); EXPECT_CALL(visitor, OnHeaderForStream(3, ":scheme", "https")); EXPECT_CALL(visitor, OnHeaderForStream(3, ":authority", "example.com")); - EXPECT_CALL( - visitor, - OnInvalidFrame(3, Http2VisitorInterface::InvalidFrameError::kHttpHeader)); + EXPECT_CALL(visitor, OnHeaderForStream(3, ":path", "/\tfragment2")); + EXPECT_CALL(visitor, OnEndHeadersForStream(3)); + EXPECT_CALL(visitor, OnEndStream(3)); const int64_t result = adapter->ProcessBytes(frames); EXPECT_EQ(frames.size(), static_cast<size_t>(result));
diff --git a/quiche/http2/adapter/oghttp2_session.cc b/quiche/http2/adapter/oghttp2_session.cc index 8738958..e7cccdd 100644 --- a/quiche/http2/adapter/oghttp2_session.cc +++ b/quiche/http2/adapter/oghttp2_session.cc
@@ -199,9 +199,6 @@ if (session_.options_.validate_http_headers) { QUICHE_VLOG(2) << "instantiating regular header validator"; validator_ = std::make_unique<HeaderValidator>(); - if (session_.options_.allow_fragment_in_path) { - validator_->SetAllowFragmentInPath(); - } } else { QUICHE_VLOG(2) << "instantiating noop header validator"; validator_ = std::make_unique<NoopHeaderValidator>();
diff --git a/quiche/http2/adapter/oghttp2_session.h b/quiche/http2/adapter/oghttp2_session.h index ed3cccb..0954395 100644 --- a/quiche/http2/adapter/oghttp2_session.h +++ b/quiche/http2/adapter/oghttp2_session.h
@@ -75,10 +75,6 @@ // If true, validates header field names and values according to RFC 7230 // and RFC 7540. bool validate_http_headers = true; - // If true, allows the '#' character in request paths, even though this - // contradicts RFC 3986 Section 3.3. - // TODO(birenroy): Flip the default value to false. - bool allow_fragment_in_path = true; }; OgHttp2Session(Http2VisitorInterface& visitor, Options options);