diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 9ea314e888214..c7e6a9a211a25 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1074,19 +1074,13 @@ bool ConnectionManagerImpl::ActiveStream::validateTrailers() { failure_details = std::string(transformation_result.details()); } - Code response_code = Code::BadRequest; absl::optional grpc_status; if (Grpc::Common::hasGrpcContentType(*request_headers_)) { grpc_status = Grpc::Status::WellKnownGrpcStatus::Internal; } - // TODO(#24735): Harmonize H/2 and H/3 behavior with H/1 - if (connection_manager_.codec_->protocol() < Protocol::Http2) { - sendLocalReply(response_code, "", nullptr, grpc_status, failure_details); - } else { - filter_manager_.streamInfo().setResponseCodeDetails(failure_details); - resetStream(); - } + sendLocalReply(Code::BadRequest, "", nullptr, grpc_status, failure_details); + if (!response_encoder_->streamErrorOnInvalidHttpMessage()) { connection_manager_.handleCodecError(failure_details); } diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index a325dff0dfb21..e8c2c98d4e378 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -405,10 +405,8 @@ class ConnectionManagerImpl : Logger::Loggable, // Note: this method is a noop unless ENVOY_ENABLE_UHV is defined // Call header validator extension to validate the request trailer map after it was // deserialized. If the trailer map failed validation, this method does the following: - // 1. For H/1 it sends 400 response and returns false. - // 2. For H/2 and H/3 it resets the stream (without error response). Issue #24735 is filed to - // harmonize this behavior with H/1. - // 3. If the `stream_error_on_invalid_http_message` is set to `false` (it is by default) in the + // 1. For H/1, H2, and H3, it sends 400 response and returns false. + // 2. If the `stream_error_on_invalid_http_message` is set to `false` (it is by default) in the // HTTP connection manager configuration, then the entire connection is closed. bool validateTrailers(); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index b31dd3037b19c..1fca99b17e7b3 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1470,7 +1470,7 @@ TEST_P(Http1ServerConnectionImplTest, HeaderNameWithUnderscoreAreDropped) { // Ensures that request with header names containing the underscore character are rejected // when the option is set to reject request. -TEST_P(Http1ServerConnectionImplTest, HeaderNameWithUnderscoreAreRejected) { +TEST_P(Http1ServerConnectionImplTest, HeaderNameWithUnderscoreCauseRequestRejected) { headers_with_underscores_action_ = envoy::config::core::v3::HttpProtocolOptions::REJECT_REQUEST; initialize(); diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 77e3c1ebf6861..c290baedb46ef 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -2865,8 +2865,7 @@ TEST_P(Http2CodecImplTest, HeaderNameWithUnderscoreAreRejected) { TestRequestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); request_headers.addCopy("bad_header", "something"); - EXPECT_CALL(request_decoder_, sendLocalReply(Http::Code::BadRequest, _, _, _, _)); - EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(0); + EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)); EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); driveToCompletion(); EXPECT_EQ( diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index c28df237301d8..c4bec105c97f3 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1828,7 +1828,7 @@ TEST_P(DownstreamProtocolIntegrationTest, HeadersWithUnderscoresCauseRequestReje ASSERT_TRUE(response->reset()); EXPECT_EQ((downstream_protocol_ == Http::CodecType::HTTP3 ? Http::StreamResetReason::ProtocolError - : Http::StreamResetReason::ConnectionTermination), + : Http::StreamResetReason::RemoteReset), response->resetReason()); } EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("unexpected_underscore")); @@ -1859,19 +1859,9 @@ TEST_P(DownstreamProtocolIntegrationTest, TrailerWithUnderscoresCauseRequestReje *request_encoder_, Http::TestRequestTrailerMapImpl{{"trailer1", "value1"}, {"trailer_2", "value2"}}); - if (downstream_protocol_ == Http::CodecType::HTTP1) { - ASSERT_TRUE(codec_client_->waitForDisconnect()); - ASSERT_TRUE(response->complete()); - EXPECT_EQ("400", response->headers().getStatusValue()); - } else { - ASSERT_TRUE(response->waitForReset()); - codec_client_->close(); - ASSERT_TRUE(response->reset()); - EXPECT_EQ((downstream_protocol_ == Http::CodecType::HTTP3 - ? Http::StreamResetReason::ProtocolError - : Http::StreamResetReason::ConnectionTermination), - response->resetReason()); - } + ASSERT_TRUE(codec_client_->waitForDisconnect()); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("400", response->headers().getStatusValue()); EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("unexpected_underscore")); }