From 4cde00da2e7c989d9ac64e7bd2d758973d36450b Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Tue, 2 Jul 2024 23:20:42 -0400 Subject: [PATCH] =?UTF-8?q?Revert=20"[balsa]=20Disallow=20CR=20not=20follo?= =?UTF-8?q?wed=20by=20LF=20in=20header=20values.=20(#34=E2=80=A6=20(#35025?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …829)" This reverts commit f15ec821d6a70a1d132f53f50970595efd1b84ee. Signed-off-by: Alyssa Wilk Signed-off-by: Fernando Cainelli --- source/common/http/http1/balsa_parser.cc | 1 - test/common/http/http1/codec_impl_test.cc | 61 ++++++++++++++--------- 2 files changed, 37 insertions(+), 25 deletions(-) diff --git a/source/common/http/http1/balsa_parser.cc b/source/common/http/http1/balsa_parser.cc index 000d14c61794c..804e16d8fd509 100644 --- a/source/common/http/http1/balsa_parser.cc +++ b/source/common/http/http1/balsa_parser.cc @@ -160,7 +160,6 @@ BalsaParser::BalsaParser(MessageType type, ParserCallbacks* connection, size_t m http_validation_policy.validate_transfer_encoding = false; http_validation_policy.require_content_length_if_body_required = false; http_validation_policy.disallow_invalid_header_characters_in_response = true; - http_validation_policy.disallow_lone_cr_in_request_headers = true; http_validation_policy.disallow_lone_cr_in_chunk_extension = true; framer_.set_http_validation_policy(http_validation_policy); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 284c324361676..be6918b963d84 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -4968,8 +4968,8 @@ TEST_P(Http1ServerConnectionImplTest, ValueStartsWithCR) { const absl::string_view value = "\r value starts with carriage return"; if (parser_impl_ == Http1ParserImpl::BalsaParser) { - testRequestWithValueExpectFailure(value, "http1.invalid_characters", - "header value contains invalid chars"); + const absl::string_view expected_value = "value starts with carriage return"; + testRequestWithValueExpectSuccess(value, expected_value); } else { #ifdef ENVOY_ENABLE_UHV testRequestWithValueExpectFailure(value, "http1.codec_error", "HPE_INVALID_HEADER_TOKEN"); @@ -4983,8 +4983,8 @@ TEST_P(Http1ServerConnectionImplTest, ValueWithCRInTheMiddle) { const absl::string_view value = "value has \r carriage return in the middle"; if (parser_impl_ == Http1ParserImpl::BalsaParser) { - testRequestWithValueExpectFailure(value, "http1.invalid_characters", - "header value contains invalid chars"); + const absl::string_view expected_value = "value has carriage return in the middle"; + testRequestWithValueExpectSuccess(value, expected_value); } else { testRequestWithValueExpectFailure(value, "http1.codec_error", "HPE_LF_EXPECTED"); } @@ -4994,8 +4994,8 @@ TEST_P(Http1ServerConnectionImplTest, ValueEndsWithCR) { const absl::string_view value = "value ends in carriage return \r"; if (parser_impl_ == Http1ParserImpl::BalsaParser) { - testRequestWithValueExpectFailure(value, "http1.invalid_characters", - "header value contains invalid chars"); + const absl::string_view expected_value = "value ends in carriage return"; + testRequestWithValueExpectSuccess(value, expected_value); } else { testRequestWithValueExpectFailure(value, "http1.codec_error", "HPE_LF_EXPECTED"); } @@ -5093,7 +5093,8 @@ TEST_P(Http1ClientConnectionImplTest, ValueStartsWithCR) { const absl::string_view value = "\r value starts with carriage return"; if (parser_impl_ == Http1ParserImpl::BalsaParser) { - testRequestWithValueExpectFailure(value, "header value contains invalid chars"); + const absl::string_view expected_value = "value starts with carriage return"; + testRequestWithValueExpectSuccess(value, expected_value); } else { #ifdef ENVOY_ENABLE_UHV testRequestWithValueExpectFailure(value, "HPE_INVALID_HEADER_TOKEN"); @@ -5107,7 +5108,8 @@ TEST_P(Http1ClientConnectionImplTest, ValueWithCRInTheMiddle) { const absl::string_view value = "value has \r carriage return in the middle"; if (parser_impl_ == Http1ParserImpl::BalsaParser) { - testRequestWithValueExpectFailure(value, "header value contains invalid chars"); + const absl::string_view expected_value = "value has carriage return in the middle"; + testRequestWithValueExpectSuccess(value, expected_value); } else { testRequestWithValueExpectFailure(value, "HPE_LF_EXPECTED"); } @@ -5117,7 +5119,8 @@ TEST_P(Http1ClientConnectionImplTest, ValueEndsWithCR) { const absl::string_view value = "value ends in carriage return \r"; if (parser_impl_ == Http1ParserImpl::BalsaParser) { - testRequestWithValueExpectFailure(value, "header value contains invalid chars"); + const absl::string_view expected_value = "value ends in carriage return"; + testRequestWithValueExpectSuccess(value, expected_value); } else { testRequestWithValueExpectFailure(value, "HPE_LF_EXPECTED"); } @@ -5151,9 +5154,12 @@ TEST_P(Http1ServerConnectionImplTest, FirstLineInvalidCR) { MockRequestDecoder decoder; EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + TestRequestHeaderMapImpl expected_headers{ + {":path", "/"}, + {":method", "GET"}, + }; if (parser_impl_ == Http1ParserImpl::BalsaParser) { - EXPECT_CALL(decoder, sendLocalReply(Http::Code::BadRequest, "Bad Request", _, _, - "http1.invalid_characters")); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)); } else { EXPECT_CALL(decoder, sendLocalReply(Http::Code::BadRequest, "Bad Request", _, _, "http1.codec_error")); @@ -5161,10 +5167,11 @@ TEST_P(Http1ServerConnectionImplTest, FirstLineInvalidCR) { Buffer::OwnedImpl buffer("GET /\rHTTP/1.1\r\n\r\n"); auto status = codec_->dispatch(buffer); - EXPECT_TRUE(isCodecProtocolError(status)); if (parser_impl_ == Http1ParserImpl::BalsaParser) { - EXPECT_EQ(status.message(), "http/1.1 protocol error: header value contains invalid chars"); + EXPECT_TRUE(status.ok()); + EXPECT_EQ(0u, buffer.length()); } else { + EXPECT_TRUE(isCodecProtocolError(status)); EXPECT_EQ(status.message(), "http/1.1 protocol error: HPE_LF_EXPECTED"); } } @@ -5183,13 +5190,24 @@ TEST_P(Http1ClientConnectionImplTest, FirstLineInvalidCR) { }; EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); + TestResponseHeaderMapImpl expected_headers{ + {":status", "200"}, + {"content-length", "5"}, + }; + if (parser_impl_ == Http1ParserImpl::BalsaParser) { + EXPECT_CALL(response_decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); + EXPECT_CALL(response_decoder, decodeData(BufferStringEqual("hello"), false)); + EXPECT_CALL(response_decoder, decodeData(BufferStringEqual(""), true)); + } + Buffer::OwnedImpl buffer("HTTP/1.1 200\rOK\r\ncontent-length: 5\r\n\r\n" "hello"); auto status = codec_->dispatch(buffer); - EXPECT_TRUE(isCodecProtocolError(status)); if (parser_impl_ == Http1ParserImpl::BalsaParser) { - EXPECT_EQ(status.message(), "http/1.1 protocol error: header value contains invalid chars"); + EXPECT_TRUE(status.ok()); + EXPECT_EQ(0u, buffer.length()); } else { + EXPECT_TRUE(isCodecProtocolError(status)); #ifdef ENVOY_ENABLE_UHV EXPECT_EQ(status.message(), "http/1.1 protocol error: HPE_INVALID_HEADER_TOKEN"); #else @@ -5207,13 +5225,8 @@ TEST_P(Http1ServerConnectionImplTest, HeaderNameInvalidCR) { MockRequestDecoder decoder; EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); - if (parser_impl_ == Http1ParserImpl::BalsaParser) { - EXPECT_CALL(decoder, sendLocalReply(Http::Code::BadRequest, "Bad Request", _, _, - "http1.invalid_characters")); - } else { - EXPECT_CALL(decoder, - sendLocalReply(Http::Code::BadRequest, "Bad Request", _, _, "http1.codec_error")); - } + EXPECT_CALL(decoder, + sendLocalReply(Http::Code::BadRequest, "Bad Request", _, _, "http1.codec_error")); // SPELLCHECKER(off) Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\nfo\ro: bar\r\n\r\n"); @@ -5221,7 +5234,7 @@ TEST_P(Http1ServerConnectionImplTest, HeaderNameInvalidCR) { auto status = codec_->dispatch(buffer); EXPECT_TRUE(isCodecProtocolError(status)); if (parser_impl_ == Http1ParserImpl::BalsaParser) { - EXPECT_EQ(status.message(), "http/1.1 protocol error: header value contains invalid chars"); + EXPECT_EQ(status.message(), "http/1.1 protocol error: INVALID_HEADER_NAME_CHARACTER"); } else { EXPECT_EQ(status.message(), "http/1.1 protocol error: HPE_INVALID_HEADER_TOKEN"); } @@ -5247,7 +5260,7 @@ TEST_P(Http1ClientConnectionImplTest, HeaderNameInvalidCR) { auto status = codec_->dispatch(buffer); EXPECT_TRUE(isCodecProtocolError(status)); if (parser_impl_ == Http1ParserImpl::BalsaParser) { - EXPECT_EQ(status.message(), "http/1.1 protocol error: header value contains invalid chars"); + EXPECT_EQ(status.message(), "http/1.1 protocol error: INVALID_HEADER_NAME_CHARACTER"); } else { EXPECT_EQ(status.message(), "http/1.1 protocol error: HPE_INVALID_HEADER_TOKEN"); }