http: Add ability to merge slashes

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

@@ -25,7 +25,7 @@ import "gogoproto/gogo.proto";
 // [#protodoc-title: HTTP connection manager]
 // HTTP connection manager :ref:`configuration overview <config_http_conn_man>`.
 
-// [#comment:next free field: 33]
+// [#comment:next free field: 34]
 message HttpConnectionManager {
   enum CodecType {
     option (gogoproto.goproto_enum_prefix) = false;
@@ -424,6 +424,13 @@ message HttpConnectionManager {
   // Note that Envoy does not perform
   // `case normalization <https://tools.ietf.org/html/rfc3986#section-6.2.2.1>`
   google.protobuf.BoolValue normalize_path = 30;
+
+  // Determines if adjacent slashes in the path are merged into one before any processing of
+  // requests by HTTP filters or routing. This affects the upstream *:path* header as well. Without
+  // setting this option, incoming requests with path `//dir///file` will not match against route
+  // with `prefix` match set to `/dir`. Defaults to `false`. Note that slash merging is not part of
+  // `HTTP spec <https://tools.ietf.org/html/rfc3986>` and is provided for convenience.
+  bool merge_slashes = 33;
 }
 
 message Rds {

docs/root/intro/version_history.rst

@@ -10,6 +10,7 @@ Version history
 * fault: added overrides for default runtime keys in :ref:`HTTPFault <envoy_api_msg_config.filter.http.fault.v2.HTTPFault>` filter.
 * grpc-json: added support for :ref:`ignoring unknown query parameters<envoy_api_field_config.filter.http.transcoder.v2.GrpcJsonTranscoder.ignore_unknown_query_parameters>`.
 * http: added the ability to reject HTTP/1.1 requests with invalid HTTP header values, using the runtime feature `envoy.reloadable_features.strict_header_validation`.
+* http: added the ability to :ref:`merge adjacent slashes<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.merge_slashes>` in the path.
 * listeners: added :ref:`HTTP inspector listener filter <config_listener_filters_http_inspector>`.
 * router: added :ref:`rq_retry_skipped_request_not_complete <config_http_filters_router_stats>` counter stat to router stats.
 * tls: added verification of IP address SAN fields in certificates against configured SANs in the

source/common/http/conn_manager_config.h

@@ -357,6 +357,12 @@ class ConnectionManagerConfig {
    * @return if the HttpConnectionManager should normalize url following RFC3986
    */
   virtual bool shouldNormalizePath() const PURE;
+
+  /**
+   * @return if the HttpConnectionManager should merge two or more adjacent slashes in the path into
+   * one.
+   */
+  virtual bool shouldMergeSlashes() const PURE;
 };
 } // namespace Http
 } // namespace Envoy

source/common/http/conn_manager_utility.cc

@@ -392,10 +392,15 @@ void ConnectionManagerUtility::mutateResponseHeaders(HeaderMap& response_headers
 bool ConnectionManagerUtility::maybeNormalizePath(HeaderMap& request_headers,
                                                   const ConnectionManagerConfig& config) {
   ASSERT(request_headers.Path());
+  bool is_valid_path = true;
   if (config.shouldNormalizePath()) {
-    return PathUtil::canonicalPath(*request_headers.Path());
+    is_valid_path = PathUtil::canonicalPath(*request_headers.Path());
   }
-  return true;
+  // Merge slashes after path normalization to catch potential edge cases with percent encoding.
+  if (is_valid_path && config.shouldMergeSlashes()) {
+    PathUtil::mergeSlashes(*request_headers.Path());
+  }
+  return is_valid_path;
 }
 
 } // namespace Http

source/common/http/path_utility.cc

@@ -4,6 +4,8 @@
 #include "common/chromium_url/url_canon_stdstring.h"
 #include "common/common/logger.h"
 
+#include "absl/strings/str_join.h"
+#include "absl/strings/str_split.h"
 #include "absl/strings/string_view.h"
 #include "absl/types/optional.h"
 
@@ -52,5 +54,20 @@ bool PathUtil::canonicalPath(HeaderEntry& path_header) {
   return true;
 }
 
+void PathUtil::mergeSlashes(HeaderEntry& path_header) {
+  const auto original_path = path_header.value().getStringView();
+  // Only operate on path component in URL.
+  const absl::string_view::size_type query_start = original_path.find('?');
+  const absl::string_view path = original_path.substr(0, query_start);
+  const absl::string_view query = absl::ClippedSubstr(original_path, query_start);
+  if (path.find("//") == absl::string_view::npos) {
+    return;
+  }
+  const absl::string_view prefix = absl::StartsWith(path, "/") ? "/" : absl::string_view();
+  const absl::string_view suffix = absl::EndsWith(path, "/") ? "/" : absl::string_view();
+  path_header.value(absl::StrCat(
+      prefix, absl::StrJoin(absl::StrSplit(path, '/', absl::SkipEmpty()), "/"), query, suffix));
+}
+
 } // namespace Http
 } // namespace Envoy

source/common/http/path_utility.h

@@ -13,6 +13,8 @@ class PathUtil {
   // Returns if the normalization succeeds.
   // If it is successful, the param will be updated with the normalized path.
   static bool canonicalPath(HeaderEntry& path_header);
+  // Merges two or more adjacent slashes in path part of URI into one.
+  static void mergeSlashes(HeaderEntry& path_header);
 };
 
 } // namespace Http

source/extensions/filters/network/http_connection_manager/config.cc

@@ -170,7 +170,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
 #else
                                                       0
 #endif
-                                                      ))) {
+                                                      ))),
+      merge_slashes_(config.merge_slashes()) {
   // If scoped RDS is enabled, avoid creating a route config provider. Route config providers will
   // be managed by the scoped routing logic instead.
   switch (config.route_specifier_case()) {

source/extensions/filters/network/http_connection_manager/config.h

@@ -139,6 +139,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   bool proxy100Continue() const override { return proxy_100_continue_; }
   const Http::Http1Settings& http1Settings() const override { return http1_settings_; }
   bool shouldNormalizePath() const override { return normalize_path_; }
+  bool shouldMergeSlashes() const override { return merge_slashes_; }
   std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; }
 
 private:
@@ -183,6 +184,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   const bool proxy_100_continue_;
   std::chrono::milliseconds delayed_close_timeout_;
   const bool normalize_path_;
+  const bool merge_slashes_;
 
   // Default idle timeout is 5 minutes if nothing is specified in the HCM config.
   static const uint64_t StreamIdleTimeoutMs = 5 * 60 * 1000;

source/server/http/admin.h

@@ -144,6 +144,7 @@ class AdminImpl : public Admin,
   bool proxy100Continue() const override { return false; }
   const Http::Http1Settings& http1Settings() const override { return http1_settings_; }
   bool shouldNormalizePath() const override { return true; }
+  bool shouldMergeSlashes() const override { return true; }
   Http::Code request(absl::string_view path_and_query, absl::string_view method,
                      Http::HeaderMap& response_headers, std::string& body) override;
   void closeSocket();

test/common/http/conn_manager_impl_fuzz_test.cc

@@ -110,6 +110,7 @@ class FuzzConfig : public ConnectionManagerConfig {
   bool proxy100Continue() const override { return proxy_100_continue_; }
   const Http::Http1Settings& http1Settings() const override { return http1_settings_; }
   bool shouldNormalizePath() const override { return false; }
+  bool shouldMergeSlashes() const override { return false; }
 
   const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager config_;
   std::list<AccessLog::InstanceSharedPtr> access_logs_;

test/common/http/conn_manager_impl_test.cc

@@ -279,6 +279,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan
   bool proxy100Continue() const override { return proxy_100_continue_; }
   const Http::Http1Settings& http1Settings() const override { return http1_settings_; }
   bool shouldNormalizePath() const override { return normalize_path_; }
+  bool shouldMergeSlashes() const override { return merge_slashes_; }
 
   DangerousDeprecatedTestTime test_time_;
   ConnectionManagerImplHelper::RouteConfigProvider route_config_provider_;
@@ -327,6 +328,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan
   bool preserve_external_request_id_ = false;
   Http::Http1Settings http1_settings_;
   bool normalize_path_ = false;
+  bool merge_slashes_ = false;
   NiceMock<Network::MockClientConnection> upstream_conn_; // for websocket tests
   NiceMock<Tcp::ConnectionPool::MockInstance> conn_pool_; // for websocket tests
 

test/common/http/conn_manager_utility_test.cc

@@ -85,6 +85,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig {
   MOCK_CONST_METHOD0(proxy100Continue, bool());
   MOCK_CONST_METHOD0(http1Settings, const Http::Http1Settings&());
   MOCK_CONST_METHOD0(shouldNormalizePath, bool());
+  MOCK_CONST_METHOD0(shouldMergeSlashes, bool());
 
   std::unique_ptr<Http::InternalAddressConfig> internal_address_config_ =
       std::make_unique<DefaultInternalAddressConfig>();
@@ -1226,6 +1227,42 @@ TEST_F(ConnectionManagerUtilityTest, SanitizePathRelativePAth) {
   EXPECT_EQ(header_map.Path()->value().getStringView(), "/abc");
 }
 
+// maybeNormalizePath() does not touch adjacent slashes by default.
+TEST_F(ConnectionManagerUtilityTest, MergeSlashesDefaultOff) {
+  ON_CALL(config_, shouldNormalizePath()).WillByDefault(Return(true));
+  ON_CALL(config_, shouldMergeSlashes()).WillByDefault(Return(false));
+  HeaderMapImpl original_headers;
+  original_headers.insertPath().value(std::string("/xyz///abc"));
+
+  HeaderMapImpl header_map(static_cast<HeaderMap&>(original_headers));
+  ConnectionManagerUtility::maybeNormalizePath(header_map, config_);
+  EXPECT_EQ(header_map.Path()->value().getStringView(), "/xyz///abc");
+}
+
+// maybeNormalizePath() merges adjacent slashes.
+TEST_F(ConnectionManagerUtilityTest, MergeSlashes) {
+  ON_CALL(config_, shouldNormalizePath()).WillByDefault(Return(true));
+  ON_CALL(config_, shouldMergeSlashes()).WillByDefault(Return(true));
+  HeaderMapImpl original_headers;
+  original_headers.insertPath().value(std::string("/xyz///abc"));
+
+  HeaderMapImpl header_map(static_cast<HeaderMap&>(original_headers));
+  ConnectionManagerUtility::maybeNormalizePath(header_map, config_);
+  EXPECT_EQ(header_map.Path()->value().getStringView(), "/xyz/abc");
+}
+
+// maybeNormalizePath() merges adjacent slashes if normalization if off.
+TEST_F(ConnectionManagerUtilityTest, MergeSlashesWithoutNormalization) {
+  ON_CALL(config_, shouldNormalizePath()).WillByDefault(Return(false));
+  ON_CALL(config_, shouldMergeSlashes()).WillByDefault(Return(true));
+  HeaderMapImpl original_headers;
+  original_headers.insertPath().value(std::string("/xyz/..//abc"));
+
+  HeaderMapImpl header_map(static_cast<HeaderMap&>(original_headers));
+  ConnectionManagerUtility::maybeNormalizePath(header_map, config_);
+  EXPECT_EQ(header_map.Path()->value().getStringView(), "/xyz/../abc");
+}
+
 // test preserve_external_request_id true does not reset the passed requestId if passed
 TEST_F(ConnectionManagerUtilityTest, PreserveExternalRequestId) {
   connection_.remote_address_ = std::make_shared<Network::Address::Ipv4Instance>("134.2.2.11");

test/common/http/path_utility_test.cc

@@ -85,5 +85,27 @@ TEST_F(PathUtilityTest, NormalizeCasePath) {
 // "/../c\r\n\"  '\n' '\r' should be excluded by http parser
 // "/a/\0c",     '\0' should be excluded by http parser
 
+// Paths that are valid get normalized.
+TEST_F(PathUtilityTest, MergeSlashes) {
+  auto mergeSlashes = [this](const std::string& path_value) {
+    auto& path_header = pathHeaderEntry(path_value);
+    PathUtil::mergeSlashes(path_header);
+    auto sanitized_path_value = path_header.value().getStringView();
+    return std::string(sanitized_path_value);
+  };
+  EXPECT_EQ("", mergeSlashes(""));                        // empty
+  EXPECT_EQ("a/b/c", mergeSlashes("a//b/c"));             // relative
+  EXPECT_EQ("/a/b/c/", mergeSlashes("/a//b/c/"));         // ends with slash
+  EXPECT_EQ("a/b/c/", mergeSlashes("a//b/c/"));           // relative ends with slash
+  EXPECT_EQ("/a", mergeSlashes("/a"));                    // no-op
+  EXPECT_EQ("/a/b/c", mergeSlashes("//a/b/c"));           // double / start
+  EXPECT_EQ("/a/b/c", mergeSlashes("/a//b/c"));           // double / in the middle
+  EXPECT_EQ("/a/b/c/", mergeSlashes("/a/b/c//"));         // double / end
+  EXPECT_EQ("/a/b/c", mergeSlashes("/a///b/c"));          // triple / in the middle
+  EXPECT_EQ("/a/b/c", mergeSlashes("/a////b/c"));         // quadruple / in the middle
+  EXPECT_EQ("/a/b?a=///c", mergeSlashes("/a//b?a=///c")); // slashes in the query are ignored
+  EXPECT_EQ("/a/b?", mergeSlashes("/a//b?"));             // empty query
+}
+
 } // namespace Http
 } // namespace Envoy

test/extensions/filters/network/http_connection_manager/config_test.cc

@@ -412,6 +412,56 @@ TEST_F(HttpConnectionManagerConfigTest, NormalizePathFalse) {
   EXPECT_FALSE(config.shouldNormalizePath());
 }
 
+// Validated that by default we don't merge slashes.
+TEST_F(HttpConnectionManagerConfigTest, MergeSlashesDefault) {
+  const std::string yaml_string = R"EOF(
+  stat_prefix: ingress_http
+  route_config:
+    name: local_route
+  http_filters:
+  - name: envoy.router
+  )EOF";
+
+  HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
+                                     date_provider_, route_config_provider_manager_,
+                                     scoped_routes_config_provider_manager_);
+  EXPECT_FALSE(config.shouldMergeSlashes());
+}
+
+// Validated that when configured, we merge slashes.
+TEST_F(HttpConnectionManagerConfigTest, MergeSlashesTrue) {
+  const std::string yaml_string = R"EOF(
+  stat_prefix: ingress_http
+  route_config:
+    name: local_route
+  merge_slashes: true
+  http_filters:
+  - name: envoy.router
+  )EOF";
+
+  HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
+                                     date_provider_, route_config_provider_manager_,
+                                     scoped_routes_config_provider_manager_);
+  EXPECT_TRUE(config.shouldMergeSlashes());
+}
+
+// Validated that when explicitly set false, we don't merge slashes.
+TEST_F(HttpConnectionManagerConfigTest, MergeSlashesFalse) {
+  const std::string yaml_string = R"EOF(
+  stat_prefix: ingress_http
+  route_config:
+    name: local_route
+  merge_slashes: false
+  http_filters:
+  - name: envoy.router
+  )EOF";
+
+  HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
+                                     date_provider_, route_config_provider_manager_,
+                                     scoped_routes_config_provider_manager_);
+  EXPECT_FALSE(config.shouldMergeSlashes());
+}
+
 TEST_F(HttpConnectionManagerConfigTest, ConfiguredRequestTimeout) {
   const std::string yaml_string = R"EOF(
   stat_prefix: ingress_http