Skip to content

Commit

Permalink
Revert "[balsa] Disallow CR not followed by LF in header values. (env…
Browse files Browse the repository at this point in the history 
…oyproxy#34… (envoyproxy#35025)

…829)"

This reverts commit f15ec82.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
  • Loading branch information
@alyssawilk @cainelli
alyssawilk authored and cainelli committed Jul 5, 2024
1 parent 1007ae0 commit 4cde00d
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 25 deletions.
1 change: 0 additions & 1 deletion source/common/http/http1/balsa_parser.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
61 changes: 37 additions & 24 deletions test/common/http/http1/codec_impl_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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");
}
Expand All @@ -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");
}
Expand Down Expand Up @@ -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");
Expand All @@ -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");
}
Expand All @@ -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");
}
Expand Down Expand Up @@ -5151,20 +5154,24 @@ 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"));
}

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");
}
}
Expand All @@ -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
Expand All @@ -5207,21 +5225,16 @@ 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");
// SPELLCHECKER(on)
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");
}
Expand All @@ -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");
}
Expand Down

0 comments on commit 4cde00d

Please sign in to comment.