diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 9c65dfa1bc9b0..7a48b841f2a08 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -129,6 +129,12 @@ class HeaderString { */ char* buffer() { return buffer_.dynamic_; } + /** + * Trim trailing whitespaces from the HeaderString. + * v1.13 supports both Inline and Dynamic, but not Reference type. + */ + void rtrim(); + /** * Get an absl::string_view. It will NOT be NUL terminated! * diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index ecc98017b8419..bcbc4f91aad8c 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -150,6 +150,15 @@ void HeaderString::append(const char* data, uint32_t size) { string_length_ += size; } +void HeaderString::rtrim() { + ASSERT(type() == Type::Inline || type() == Type::Dynamic); + absl::string_view original = getStringView(); + absl::string_view rtrimmed = StringUtil::rtrim(original); + if (original.size() != rtrimmed.size()) { + string_length_ = rtrimmed.size(); + } +} + void HeaderString::clear() { switch (type_) { case Type::Reference: { diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 32bcb70db812a..358b2dd074762 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -461,6 +461,10 @@ void ConnectionImpl::completeLastHeader() { checkHeaderNameForUnderscores(); if (!current_header_field_.empty()) { toLowerTable().toLowerCase(current_header_field_.buffer(), current_header_field_.size()); + // Strip trailing whitespace of the current header value if any. Leading whitespace was trimmed + // in ConnectionImpl::onHeaderValue. http_parser does not strip leading or trailing whitespace + // as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4 + current_header_value_.rtrim(); current_header_map_->addViaMove(std::move(current_header_field_), std::move(current_header_value_)); } @@ -584,9 +588,8 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { if (!current_header_map_) { current_header_map_ = std::make_unique(); } - // Work around a bug in http_parser where trailing whitespace is not trimmed - // as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4 - const absl::string_view header_value = StringUtil::trim(absl::string_view(data, length)); + + absl::string_view header_value{data, length}; if (strict_header_validation_) { if (!Http::HeaderUtility::headerValueIsValid(header_value)) { @@ -604,6 +607,13 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { } header_parsing_state_ = HeaderParsingState::Value; + if (current_header_value_.empty()) { + // Strip leading whitespace if the current header value input contains the first bytes of the + // encoded header value. Trailing whitespace is stripped once the full header value is known in + // ConnectionImpl::completeLastHeader. http_parser does not strip leading or trailing whitespace + // as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4 . + header_value = StringUtil::ltrim(header_value); + } current_header_value_.append(header_value.data(), header_value.length()); checkMaxHeadersSize(); diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 74f2d225e308d..f3c60252e1f0b 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -115,6 +115,37 @@ TEST(HeaderStringTest, All) { EXPECT_EQ("HELLO", string.getStringView()); } + // Inline rtrim removes trailing whitespace only. + { + // This header string is short enough to fit into Inline type. + const std::string data_with_leading_lws = " \t\f\v data"; + const std::string data_with_leading_and_trailing_lws = data_with_leading_lws + " \t\f\v"; + HeaderString string; + string.append(data_with_leading_and_trailing_lws.data(), + data_with_leading_and_trailing_lws.size()); + EXPECT_EQ(data_with_leading_and_trailing_lws, string.getStringView()); + EXPECT_EQ(string.type(), HeaderString::Type::Inline); + string.rtrim(); + EXPECT_NE(data_with_leading_and_trailing_lws, string.getStringView()); + EXPECT_EQ(data_with_leading_lws, string.getStringView()); + } + + // Dynamic rtrim removes trailing whitespace only. + { + // Making this string longer than Inline can fit. + const std::string padding_data_with_leading_lws = " \t\f\v data" + std::string(128, 'a'); + const std::string data_with_leading_and_trailing_lws = + padding_data_with_leading_lws + " \t\f\v"; + HeaderString string; + string.append(data_with_leading_and_trailing_lws.data(), + data_with_leading_and_trailing_lws.size()); + EXPECT_EQ(data_with_leading_and_trailing_lws, string.getStringView()); + EXPECT_EQ(string.type(), HeaderString::Type::Dynamic); + string.rtrim(); + EXPECT_NE(data_with_leading_and_trailing_lws, string.getStringView()); + EXPECT_EQ(padding_data_with_leading_lws, string.getStringView()); + } + // Static clear() does nothing. { std::string static_string("HELLO"); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 8ea74a0337a38..d6f23bec85611 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -43,6 +43,16 @@ std::string createHeaderFragment(int num_headers) { } return headers; } + +Buffer::OwnedImpl createBufferWithNByteSlices(absl::string_view input, size_t max_slice_size) { + Buffer::OwnedImpl buffer; + for (size_t offset = 0; offset < input.size(); offset += max_slice_size) { + buffer.appendSliceForTest(input.substr(offset, max_slice_size)); + } + // Verify that the buffer contains the right number of slices. + ASSERT(buffer.getRawSlices(nullptr, 0) == (input.size() + max_slice_size - 1) / max_slice_size); + return buffer; +} } // namespace class Http1ServerConnectionImplTest : public testing::Test { @@ -72,7 +82,13 @@ class Http1ServerConnectionImplTest : public testing::Test { // Then send a response just to clean up. void sendAndValidateRequestAndSendResponse(absl::string_view raw_request, const TestHeaderMapImpl& expected_request_headers) { - NiceMock decoder; + Buffer::OwnedImpl buffer(raw_request); + sendAndValidateRequestAndSendResponse(buffer, expected_request_headers); + } + + void sendAndValidateRequestAndSendResponse(Buffer::Instance& buffer, + const TestHeaderMapImpl& expected_request_headers) { + NiceMock decoder; Http::StreamEncoder* response_encoder = nullptr; EXPECT_CALL(callbacks_, newStream(_, _)) .Times(1) @@ -80,8 +96,7 @@ class Http1ServerConnectionImplTest : public testing::Test { response_encoder = &encoder; return decoder; })); - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_request_headers), true)).Times(1); - Buffer::OwnedImpl buffer(raw_request); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_request_headers), true)); codec_->dispatch(buffer); EXPECT_EQ(0U, buffer.length()); response_encoder->encodeHeaders(TestHeaderMapImpl{{":status", "200"}}, true); @@ -353,6 +368,42 @@ TEST_F(Http1ServerConnectionImplTest, HostWithLWS) { "GET / HTTP/1.1\r\nHost: host \r\n\r\n", expected_headers); } +// Regression test for https://github.com/envoyproxy/envoy/issues/10270. Linear whitespace at the +// beginning and end of a header value should be stripped. Whitespace in the middle should be +// preserved. +TEST_F(Http1ServerConnectionImplTest, InnerLWSIsPreserved) { + initialize(); + + // Header with many spaces surrounded by non-whitespace characters to ensure that dispatching is + // split across multiple dispatch calls. The threshold used here comes from Envoy preferring 16KB + // reads, but the important part is that the header value is split such that the pieces have + // leading and trailing whitespace characters. + const std::string header_value_with_inner_lws = "v" + std::string(32 * 1024, ' ') + "v"; + TestHeaderMapImpl expected_headers{{":authority", "host"}, + {":path", "/"}, + {":method", "GET"}, + {"header_field", header_value_with_inner_lws}}; + + { + // Regression test spaces in the middle are preserved + Buffer::OwnedImpl header_buffer = createBufferWithNByteSlices( + "GET / HTTP/1.1\r\nHost: host\r\nheader_field: " + header_value_with_inner_lws + "\r\n\r\n", + 16 * 1024); + EXPECT_EQ(3, header_buffer.getRawSlices(nullptr, 0)); + sendAndValidateRequestAndSendResponse(header_buffer, expected_headers); + } + + { + // Regression test spaces before and after are removed + Buffer::OwnedImpl header_buffer = createBufferWithNByteSlices( + "GET / HTTP/1.1\r\nHost: host\r\nheader_field: " + header_value_with_inner_lws + + " \r\n\r\n", + 16 * 1024); + EXPECT_EQ(3, header_buffer.getRawSlices(nullptr, 0)); + sendAndValidateRequestAndSendResponse(header_buffer, expected_headers); + } +} + TEST_F(Http1ServerConnectionImplTest, Http10) { initialize(); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index e0d69f3774bc7..e28d6b9a07bb0 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -241,6 +241,39 @@ TEST_P(ProtocolIntegrationTest, DrainClose) { test_server_->drainManager().draining_ = false; } +// Regression test for https://github.com/envoyproxy/envoy/issues/10270 +TEST_P(ProtocolIntegrationTest, LongHeaderValueWithSpaces) { + // Header with at least 20kb of spaces surrounded by non-whitespace characters to ensure that + // dispatching is split across 2 dispatch calls. This threshold comes from Envoy preferring 16KB + // reads, which the buffer rounds up to about 20KB when allocating slices in + // Buffer::OwnedImpl::reserve(). + const std::string long_header_value_with_inner_lws = "v" + std::string(32 * 1024, ' ') + "v"; + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"longrequestvalue", long_header_value_with_inner_lws}}); + waitForNextUpstreamRequest(); + EXPECT_EQ(long_header_value_with_inner_lws, upstream_request_->headers() + .get(Http::LowerCaseString("longrequestvalue")) + ->value() + .getStringView()); + upstream_request_->encodeHeaders( + Http::TestHeaderMapImpl{{":status", "200"}, + {"longresponsevalue", long_header_value_with_inner_lws}}, + true); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + EXPECT_EQ( + long_header_value_with_inner_lws, + response->headers().get(Http::LowerCaseString("longresponsevalue"))->value().getStringView()); +} + TEST_P(ProtocolIntegrationTest, Retry) { initialize(); codec_client_ = makeHttpConnection(lookupPort("http"));