diff --git a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto index f0c7b0f59e1e..3423cb2f0cfb 100644 --- a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto +++ b/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 `. -// [#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 ` 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 ` and is provided for convenience. + bool merge_slashes = 33; } message Rds { diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index db59eeb09e7d..76ff5613e44e 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -10,6 +10,7 @@ Version history * fault: added overrides for default runtime keys in :ref:`HTTPFault ` filter. * grpc-json: added support for :ref:`ignoring 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` in the path. * listeners: added :ref:`HTTP inspector listener filter `. * router: added :ref:`rq_retry_skipped_request_not_complete ` counter stat to router stats. * tls: added verification of IP address SAN fields in certificates against configured SANs in the diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index 78ebc7a19161..647aefaec9ee 100644 --- a/source/common/http/conn_manager_config.h +++ b/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 diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 6ec1bd5b9116..a8b282f6651a 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/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 diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index 56ce3204a468..49372fc50dbf 100644 --- a/source/common/http/path_utility.cc +++ b/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 diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index ad0d32c3ff7d..a588f39de46e 100644 --- a/source/common/http/path_utility.h +++ b/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 diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index f5596e83bfe0..3831d92c7c21 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/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()) { diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index eb97a9bf21db..71d9b5fa92d1 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -139,6 +139,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, 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, 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; diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 9d1da1e8700d..4964bb51ca0d 100644 --- a/source/server/http/admin.h +++ b/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(); diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index e15e88e97473..cd174f7c07a8 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/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 access_logs_; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 7f2abde9ba58..45e1f7b56ebd 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/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 upstream_conn_; // for websocket tests NiceMock conn_pool_; // for websocket tests diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 48fa93d14174..5174340992ea 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/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 internal_address_config_ = std::make_unique(); @@ -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(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(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(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("134.2.2.11"); diff --git a/test/common/http/path_utility_test.cc b/test/common/http/path_utility_test.cc index 2cc299465add..3ee558a2633e 100644 --- a/test/common/http/path_utility_test.cc +++ b/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 diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index 9afdaa5a6c2b..d197b9b7fa94 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/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