Address PR Comments

Signed-off-by: Alvin Baptiste <[email protected]>
envoyproxy · Nov 12, 2019 · e8cd453 · e8cd453
1 parent 26bfd63
commit e8cd453
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 6 deletions.
diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst
@@ -3,6 +3,7 @@ Version history
 
 1.13.0 (pending)
 ================
+* http: added the ability to sanitize headers nominated by the Connection header.  This new behavior is guarded by envoy.reloadable_features.connection_header_sanitization which defaults to true.
 * access log: added FILTER_STATE :ref:`access log formatters <config_access_log_format>` and gRPC access logger.
 * api: remove all support for v1
 * build: official released binary is now built against libc++.

diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc
@@ -399,8 +399,8 @@ bool Utility::sanitizeConnectionHeader(Http::HeaderMap& headers) {
   const auto& connection_header_value = headers.Connection()->value();
 
   StringUtil::CaseUnorderedSet headers_to_remove{};
-  std::vector<std::string> connection_header_tokens =
-      absl::StrSplit(connection_header_value.getStringView(), ',');
+  std::vector<absl::string_view> connection_header_tokens =
+      StringUtil::splitToken(connection_header_value.getStringView(), ",", false);
 
   // If we have 10 or more nominated headers, fail this request
   if (connection_header_tokens.size() >= MAX_ALLOWED_NOMINATED_HEADERS) {
@@ -444,6 +444,7 @@ bool Utility::sanitizeConnectionHeader(Http::HeaderMap& headers) {
                !token_sv.find(':')) {
       // If pseudo headers or X-Forwarded* headers are nominated, this could be
       // an invalid request. Reject the request
+      ENVOY_LOG_MISC(trace, "Invalid nomination of {} header", token_sv);
       return false;
     } else {
       // Examine the value of all other nominated headers
@@ -462,7 +463,8 @@ bool Utility::sanitizeConnectionHeader(Http::HeaderMap& headers) {
       }
 
       if (is_te_header) {
-        for (const auto& header_value : absl::StrSplit(nominated_header_value_sv, ',')) {
+        for (const auto& header_value :
+             StringUtil::splitToken(nominated_header_value_sv, ",", false)) {
 
           const absl::string_view header_sv = StringUtil::trim(header_value);
 
@@ -476,7 +478,6 @@ bool Utility::sanitizeConnectionHeader(Http::HeaderMap& headers) {
         }
 
         if (keep_header) {
-          nominated_header->value().clear();
           nominated_header->value().setCopy(Http::Headers::get().TEValues.Trailers.data(),
                                             Http::Headers::get().TEValues.Trailers.size());
         }

diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc
@@ -1279,6 +1279,39 @@ TEST_P(HeaderIntegrationTest, TestNominatedPseudoHeader) {
       });
 }
 
+// Validate that we can sanitize the headers when splitting
+// the Connection header results in empty tokens
+TEST_P(HeaderIntegrationTest, TestSanitizeEmptyTokensFromHeaders) {
+  initializeFilter(HeaderMode::Append, false);
+  performRequest(
+      Http::TestHeaderMapImpl{
+          {":method", "GET"},
+          {":path", "/"},
+          {":scheme", "http"},
+          {":authority", "no-headers.com"},
+          {"x-request-foo", "downstram"},
+          {"connection", "te, foo,, bar, close"},
+          {"te", "trailers"},
+          {"foo", "monday"},
+          {"bar", "friday"},
+      },
+      Http::TestHeaderMapImpl{
+          {":authority", "no-headers.com"},
+          {":path", "/"},
+          {":method", "GET"},
+          {"x-request-foo", "downstram"},
+          {"te", "trailers"},
+      },
+      Http::TestHeaderMapImpl{
+          {":status", "400"},
+          {"server", "envoy"},
+      },
+      Http::TestHeaderMapImpl{
+          {":status", "400"},
+          {"server", "envoy"},
+      });
+}
+
 // Validate that we fail the request if there are too many
 // nominated headers
 TEST_P(HeaderIntegrationTest, TestTooManyNominatedHeaders) {

diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc
@@ -337,9 +337,7 @@ void HttpIntegrationTest::verifyResponse(IntegrationStreamDecoderPtr response,
 absl::optional<uint64_t>
 HttpIntegrationTest::waitForNextUpstreamRequest(const std::vector<uint64_t>& upstream_indices,
                                                 std::chrono::milliseconds connection_wait_timeout) {
-
   absl::optional<uint64_t> upstream_with_request;
-
   // If there is no upstream connection, wait for it to be established.
   if (!fake_upstream_connection_) {