diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 2567a95b6935..d4c746a6990c 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -21,7 +21,7 @@ namespace Envoy { namespace Http { - +class ConnectionManagerUtility; // Used by ASSERTs to validate internal consistency. E.g. valid HTTP header keys/values should // never contain embedded NULLs. static inline bool validHeaderString(absl::string_view s) { @@ -821,6 +821,13 @@ class RequestHeaderMap public: INLINE_REQ_STRING_HEADERS(DEFINE_INLINE_STRING_HEADER) INLINE_REQ_NUMERIC_HEADERS(DEFINE_INLINE_NUMERIC_HEADER) + virtual absl::string_view getForwardingPath() PURE; + virtual absl::string_view getFilterPath() PURE; + +private: + friend class ConnectionManagerUtility; + virtual void setForwardingPath(absl::string_view path) PURE; + virtual void setFilterPath(absl::string_view path) PURE; }; using RequestHeaderMapPtr = std::unique_ptr; using RequestHeaderMapOptRef = OptRef; diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 33cb754e6aa5..3dccdc07bab1 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -453,8 +453,10 @@ envoy_cc_library( ":legacy_path_canonicalizer", "//include/envoy/http:header_map_interface", "//source/common/common:logger_lib", + "//source/common/protobuf:utility_lib", "//source/common/runtime:runtime_features_lib", "@com_googlesource_googleurl//url:envoy_url", + "@envoy_api//envoy/type/http/v3:pkg_cc_proto", ], ) diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index 1d77ff43474a..8f5b365894d9 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -10,6 +10,7 @@ #include "envoy/type/v3/percent.pb.h" #include "common/http/date_provider.h" +#include "common/http/path_utility.h" #include "common/local_reply/local_reply.h" #include "common/network/utility.h" #include "common/stats/symbol_table_impl.h" @@ -466,6 +467,10 @@ class ConnectionManagerConfig { * @return LocalReply configuration which supplies mapping for local reply generated by Envoy. */ virtual const LocalReply::LocalReply& localReply() const PURE; + + virtual const PathTransformer& forwardingPathTransformer() const PURE; + + virtual const PathTransformer& filterPathTransformer() 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 1d9d008a3bcc..750eff73a8ac 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -439,12 +439,21 @@ bool ConnectionManagerUtility::maybeNormalizePath(RequestHeaderMap& request_head return true; // It's as valid as it is going to get. } bool is_valid_path = true; - if (config.shouldNormalizePath()) { - is_valid_path = PathUtil::canonicalPath(request_headers); + const auto original_path = request_headers.getPathValue(); + absl::optional forwarding_path = + config.forwardingPathTransformer().transform(original_path); + absl::optional filter_path; + if (forwarding_path.has_value()) { + request_headers.setForwardingPath(forwarding_path.value()); + filter_path = config.filterPathTransformer().transform(forwarding_path.value()); + } else { + return false; } - // Merge slashes after path normalization to catch potential edge cases with percent encoding. - if (is_valid_path && config.shouldMergeSlashes()) { - PathUtil::mergeSlashes(request_headers); + if (filter_path.has_value()) { + request_headers.setPath(filter_path.value()); + request_headers.setFilterPath(filter_path.value()); + } else { + return false; } return is_valid_path; } diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 2f52e70c6669..51ce3f79bd0c 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -479,6 +479,8 @@ class RequestHeaderMapImpl final : public TypedHeaderMapImpl, INLINE_REQ_NUMERIC_HEADERS(DEFINE_INLINE_HEADER_NUMERIC_FUNCS) INLINE_REQ_RESP_STRING_HEADERS(DEFINE_INLINE_HEADER_STRING_FUNCS) INLINE_REQ_RESP_NUMERIC_HEADERS(DEFINE_INLINE_HEADER_NUMERIC_FUNCS) + absl::string_view getForwardingPath() override { return forwarding_path_; } + absl::string_view getFilterPath() override { return filter_path_; } protected: // NOTE: Because inline_headers_ is a variable size member, it must be the last member in the @@ -496,9 +498,11 @@ class RequestHeaderMapImpl final : public TypedHeaderMapImpl, INLINE_REQ_RESP_STRING_HEADERS(DEFINE_HEADER_HANDLE) INLINE_REQ_RESP_NUMERIC_HEADERS(DEFINE_HEADER_HANDLE) }; - + void setForwardingPath(absl::string_view path) override { forwarding_path_ = path; } + void setFilterPath(absl::string_view path) override { filter_path_ = path; } + std::string forwarding_path_; + std::string filter_path_; using HeaderHandles = ConstSingleton; - RequestHeaderMapImpl() { clearInline(); } HeaderEntryImpl* inline_headers_[]; diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index 988e82b9be82..c30f6b95c49a 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -1,5 +1,7 @@ #include "common/http/path_utility.h" +#include "envoy/common/exception.h" + #include "common/common/logger.h" #include "common/http/legacy_path_canonicalizer.h" #include "common/runtime/runtime_features.h" @@ -35,16 +37,56 @@ absl::optional canonicalizePath(absl::string_view original_path) { bool PathUtil::canonicalPath(RequestHeaderMap& headers) { ASSERT(headers.Path()); const auto original_path = headers.getPathValue(); - // canonicalPath is supposed to apply on path component in URL instead of :path header + const absl::optional normalized_path = PathTransformer::rfcNormalize(original_path); + if (!normalized_path.has_value()) { + return false; + } + headers.setPath(normalized_path.value()); + return true; +} + +void PathUtil::mergeSlashes(RequestHeaderMap& headers) { + ASSERT(headers.Path()); + const auto original_path = headers.getPathValue(); + const absl::optional normalized = + PathTransformer::mergeSlashes(original_path).value(); + if (normalized.has_value()) { + headers.setPath(normalized.value()); + } +} + +absl::string_view PathUtil::removeQueryAndFragment(const absl::string_view path) { + absl::string_view ret = path; + // Trim query parameters and/or fragment if present. + size_t offset = ret.find_first_of("?#"); + if (offset != absl::string_view::npos) { + ret.remove_suffix(ret.length() - offset); + } + return ret; +} + +absl::optional PathTransformer::mergeSlashes(absl::string_view original_path) { + 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 std::string(original_path); + } + const absl::string_view path_prefix = absl::StartsWith(path, "/") ? "/" : absl::string_view(); + const absl::string_view path_suffix = absl::EndsWith(path, "/") ? "/" : absl::string_view(); + return absl::StrCat(path_prefix, absl::StrJoin(absl::StrSplit(path, '/', absl::SkipEmpty()), "/"), + path_suffix, query); +} + +absl::optional PathTransformer::rfcNormalize(absl::string_view original_path) { const auto query_pos = original_path.find('?'); auto normalized_path_opt = canonicalizePath( query_pos == original_path.npos ? original_path : absl::string_view(original_path.data(), query_pos) // '?' is not included ); - if (!normalized_path_opt.has_value()) { - return false; + return {}; } auto& normalized_path = normalized_path_opt.value(); const absl::string_view query_suffix = @@ -54,35 +96,51 @@ bool PathUtil::canonicalPath(RequestHeaderMap& headers) { if (!query_suffix.empty()) { normalized_path.insert(normalized_path.end(), query_suffix.begin(), query_suffix.end()); } - headers.setPath(normalized_path); - return true; + return normalized_path; } -void PathUtil::mergeSlashes(RequestHeaderMap& headers) { - ASSERT(headers.Path()); - const auto original_path = headers.getPathValue(); - // 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; +PathTransformer::PathTransformer(const bool should_normalize_path, + const bool should_merge_slashes) { + if (should_normalize_path) { + transformations_.emplace_back(PathTransformer::rfcNormalize); + } + if (should_merge_slashes) { + transformations_.emplace_back(PathTransformer::mergeSlashes); } - const absl::string_view path_prefix = absl::StartsWith(path, "/") ? "/" : absl::string_view(); - const absl::string_view path_suffix = absl::EndsWith(path, "/") ? "/" : absl::string_view(); - headers.setPath(absl::StrCat(path_prefix, - absl::StrJoin(absl::StrSplit(path, '/', absl::SkipEmpty()), "/"), - path_suffix, query)); } -absl::string_view PathUtil::removeQueryAndFragment(const absl::string_view path) { - absl::string_view ret = path; - // Trim query parameters and/or fragment if present. - size_t offset = ret.find_first_of("?#"); - if (offset != absl::string_view::npos) { - ret.remove_suffix(ret.length() - offset); +PathTransformer::PathTransformer( + envoy::type::http::v3::PathTransformation const& path_transformation) { + const auto& operations = path_transformation.operations(); + std::vector operation_hashes; + for (auto const& operation : operations) { + uint64_t operation_hash = MessageUtil::hash(operation); + if (find(operation_hashes.begin(), operation_hashes.end(), operation_hash) != + operation_hashes.end()) { + // Currently we only have RFC normalization and merge slashes, don't expect duplicates for + // these transformations. + throw EnvoyException("Duplicate path transformation"); + } + if (operation.has_normalize_path_rfc_3986()) { + transformations_.emplace_back(PathTransformer::rfcNormalize); + } else if (operation.has_merge_slashes()) { + transformations_.emplace_back(PathTransformer::mergeSlashes); + } + operation_hashes.push_back(operation_hash); } - return ret; +} + +absl::optional PathTransformer::transform(absl::string_view original) const { + absl::optional path_string = std::string(original); + absl::string_view path_string_view = original; + for (Transformation const& transformation : transformations_) { + path_string = transformation(path_string_view); + if (!path_string.has_value()) { + return {}; + } + path_string_view = path_string.value(); + } + return path_string; } } // namespace Http diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index a6a99aaef78d..b4b1b17ab725 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -1,6 +1,12 @@ #pragma once +#include + #include "envoy/http/header_map.h" +#include "envoy/type/http/v3/path_transformation.pb.h" + +#include "common/protobuf/protobuf.h" +#include "common/protobuf/utility.h" #include "absl/strings/string_view.h" @@ -24,5 +30,26 @@ class PathUtil { static absl::string_view removeQueryAndFragment(const absl::string_view path); }; +class PathTransformer { +public: + PathTransformer() = default; + PathTransformer(const bool should_normalize_path, const bool should_merge_slashes); + PathTransformer(envoy::type::http::v3::PathTransformation const& path_transformation); + + // Take a string_view as argument and return an optional string. + // The optional will be null if the transformation fail. + absl::optional transform(const absl::string_view original_path) const; + + static absl::optional mergeSlashes(absl::string_view original_path); + + static absl::optional rfcNormalize(absl::string_view original_path); + +private: + using Transformation = std::function(absl::string_view)>; + // A sequence of transformations specified by path_transformation.operations() + // Transformations will be applied to a path string in order in transform(). + std::list transformations_; +}; + } // namespace Http } // namespace Envoy diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 33692173e474..c5327c9fbdc6 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -567,6 +567,11 @@ void RouteEntryImplBase::finalizeRequestHeaders(Http::RequestHeaderMap& headers, request_headers_parser_->evaluateHeaders(headers, stream_info); } + // Restore the forwarding path if none of the filter mutate the path. + if (headers.Path() && headers.getFilterPath() == headers.getPathValue()) { + headers.setPath(std::string(headers.getForwardingPath())); + } + if (!host_rewrite_.empty()) { headers.setHost(host_rewrite_); } else if (auto_host_rewrite_header_) { diff --git a/source/extensions/filters/network/http_connection_manager/BUILD b/source/extensions/filters/network/http_connection_manager/BUILD index 590a4a10a44a..5702a8dac1ea 100644 --- a/source/extensions/filters/network/http_connection_manager/BUILD +++ b/source/extensions/filters/network/http_connection_manager/BUILD @@ -39,6 +39,7 @@ envoy_cc_extension( "//source/common/filter/http:filter_config_discovery_lib", "//source/common/http:conn_manager_lib", "//source/common/http:default_server_string_lib", + "//source/common/http:path_utility_lib", "//source/common/http:request_id_extension_lib", "//source/common/http:utility_lib", "//source/common/http/http1:codec_lib", diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 03562a77568e..c64f2c4b2542 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -273,6 +273,16 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( idle_timeout_ = absl::nullopt; } + // If the path normalization options has been set. + if (config.has_path_normalization_options()) { + forwarding_path_transformer_ = + Http::PathTransformer(config.path_normalization_options().forwarding_transformation()); + filter_path_transformer_ = + Http::PathTransformer(config.path_normalization_options().http_filter_transformation()); + } else { + forwarding_path_transformer_ = Http::PathTransformer(normalize_path_, merge_slashes_); + } + if (config.strip_any_host_port() && config.strip_matching_host_port()) { throw EnvoyException(fmt::format( "Error: Only one of `strip_matching_host_port` or `strip_any_host_port` can be set.")); diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index d2c889a12aa9..f446b2ab0503 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -24,6 +24,7 @@ #include "common/http/http1/codec_stats.h" #include "common/http/http2/codec_stats.h" #include "common/http/http3/codec_stats.h" +#include "common/http/path_utility.h" #include "common/json/json_loader.h" #include "common/local_reply/local_reply.h" #include "common/router/rds_impl.h" @@ -180,6 +181,12 @@ class HttpConnectionManagerConfig : Logger::Loggable, } std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } const LocalReply::LocalReply& localReply() const override { return *local_reply_; } + const Http::PathTransformer& filterPathTransformer() const override { + return filter_path_transformer_; + } + const Http::PathTransformer& forwardingPathTransformer() const override { + return forwarding_path_transformer_; + } private: enum class CodecType { HTTP1, HTTP2, HTTP3, AUTO }; @@ -267,6 +274,8 @@ class HttpConnectionManagerConfig : Logger::Loggable, static const uint64_t RequestTimeoutMs = 0; // request header timeout is disabled by default static const uint64_t RequestHeaderTimeoutMs = 0; + Http::PathTransformer forwarding_path_transformer_; + Http::PathTransformer filter_path_transformer_; }; /** diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index 94ce28ba6466..525c3b2c6926 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -29,6 +29,7 @@ #include "common/http/default_server_string.h" #include "common/http/http1/codec_stats.h" #include "common/http/http2/codec_stats.h" +#include "common/http/path_utility.h" #include "common/http/request_id_extension_impl.h" #include "common/http/utility.h" #include "common/network/connection_balancer_impl.h" @@ -196,6 +197,14 @@ class AdminImpl : public Admin, }; } + const Http::PathTransformer& forwardingPathTransformer() const override { + return forwarding_path_transformer_; + } + + const Http::PathTransformer& filterPathTransformer() const override { + return filter_path_transformer_; + } + private: /** * Individual admin handler including prefix, help text, and callback. @@ -441,6 +450,8 @@ class AdminImpl : public Admin, AdminListenerPtr listener_; const AdminInternalAddressConfig internal_address_config_; const LocalReply::LocalReplyPtr local_reply_; + Http::PathTransformer forwarding_path_transformer_; + Http::PathTransformer filter_path_transformer_; }; } // namespace Server diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index ed9f08f86a16..57e0c7f8d384 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -20,6 +20,7 @@ #include "common/http/date_provider_impl.h" #include "common/http/exception.h" #include "common/http/header_utility.h" +#include "common/http/path_utility.h" #include "common/network/address_impl.h" #include "common/network/utility.h" @@ -204,6 +205,13 @@ class FuzzConfig : public ConnectionManagerConfig { return envoy::config::core::v3::HttpProtocolOptions::ALLOW; } const LocalReply::LocalReply& localReply() const override { return *local_reply_; } + const Http::PathTransformer& forwardingPathTransformer() const override { + return forwarding_path_transformer_; + } + + const Http::PathTransformer& filterPathTransformer() const override { + return filter_path_transformer_; + } const envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager config_; @@ -249,6 +257,8 @@ class FuzzConfig : public ConnectionManagerConfig { Http::DefaultInternalAddressConfig internal_address_config_; bool normalize_path_{true}; LocalReply::LocalReplyPtr local_reply_; + Http::PathTransformer forwarding_path_transformer_; + Http::PathTransformer filter_path_transformer_; }; // Internal representation of stream state. Encapsulates the stream state, mocks diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index d72bcd3f5f06..d92d90cae888 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -401,7 +401,8 @@ TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { InSequence s; setup(false, ""); // Enable path sanitizer - normalize_path_ = true; + forwarding_path_transformer_ = PathTransformer(true, false); + filter_path_transformer_ = PathTransformer(); EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { decoder_ = &conn_manager_->newStream(response_encoder_); @@ -442,7 +443,8 @@ TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { TEST_F(HttpConnectionManagerImplTest, FilterShouldUseSantizedPath) { setup(false, ""); // Enable path sanitizer - normalize_path_ = true; + forwarding_path_transformer_ = PathTransformer(true, false); + filter_path_transformer_ = PathTransformer(); const std::string original_path = "/x/%2E%2e/z"; const std::string normalized_path = "/z"; @@ -483,7 +485,8 @@ TEST_F(HttpConnectionManagerImplTest, FilterShouldUseSantizedPath) { TEST_F(HttpConnectionManagerImplTest, RouteShouldUseSantizedPath) { setup(false, ""); // Enable path sanitizer - normalize_path_ = true; + forwarding_path_transformer_ = PathTransformer(true, false); + filter_path_transformer_ = PathTransformer(); const std::string original_path = "/x/%2E%2e/z"; const std::string normalized_path = "/z"; diff --git a/test/common/http/conn_manager_impl_test_base.h b/test/common/http/conn_manager_impl_test_base.h index a9ed3344ffbb..8745e644e6ca 100644 --- a/test/common/http/conn_manager_impl_test_base.h +++ b/test/common/http/conn_manager_impl_test_base.h @@ -3,6 +3,7 @@ #include "common/http/conn_manager_impl.h" #include "common/http/context_impl.h" #include "common/http/date_provider_impl.h" +#include "common/http/path_utility.h" #include "common/network/address_impl.h" #include "extensions/access_loggers/common/file_access_log_impl.h" @@ -140,7 +141,10 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan return headers_with_underscores_action_; } const LocalReply::LocalReply& localReply() const override { return *local_reply_; } - + const PathTransformer& forwardingPathTransformer() const override { + return forwarding_path_transformer_; + } + const PathTransformer& filterPathTransformer() const override { return filter_path_transformer_; } Envoy::Event::SimulatedTimeSystem test_time_; NiceMock route_config_provider_; std::shared_ptr route_config_{new NiceMock()}; @@ -212,6 +216,8 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan std::vector decoder_filters_; std::vector encoder_filters_; std::shared_ptr log_handler_; + PathTransformer forwarding_path_transformer_; + PathTransformer filter_path_transformer_; }; } // namespace Http diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 6541303b9638..a621a3d5a001 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -1,3 +1,4 @@ +#include #include #include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" @@ -8,6 +9,7 @@ #include "common/http/conn_manager_utility.h" #include "common/http/header_utility.h" #include "common/http/headers.h" +#include "common/http/path_utility.h" #include "common/network/address_impl.h" #include "common/network/utility.h" #include "common/runtime/runtime_impl.h" @@ -1376,6 +1378,12 @@ TEST_F(ConnectionManagerUtilityTest, RemovesProxyResponseHeaders) { // maybeNormalizePath() returns true with an empty path. TEST_F(ConnectionManagerUtilityTest, SanitizeEmptyPath) { + std::unique_ptr forwarding_path_transformer = + std::make_unique(false, false); + std::unique_ptr filter_path_transformer = std::make_unique(); + ON_CALL(config_, forwardingPathTransformer()) + .WillByDefault(ReturnRef(*forwarding_path_transformer)); + ON_CALL(config_, filterPathTransformer()).WillByDefault(ReturnRef(*filter_path_transformer)); ON_CALL(config_, shouldNormalizePath()).WillByDefault(Return(false)); TestRequestHeaderMapImpl original_headers; @@ -1386,7 +1394,12 @@ TEST_F(ConnectionManagerUtilityTest, SanitizeEmptyPath) { // maybeNormalizePath() does nothing by default. TEST_F(ConnectionManagerUtilityTest, SanitizePathDefaultOff) { - ON_CALL(config_, shouldNormalizePath()).WillByDefault(Return(false)); + std::unique_ptr forwarding_path_transformer = + std::make_unique(false, false); + std::unique_ptr filter_path_transformer = std::make_unique(); + ON_CALL(config_, forwardingPathTransformer()) + .WillByDefault(ReturnRef(*forwarding_path_transformer)); + ON_CALL(config_, filterPathTransformer()).WillByDefault(ReturnRef(*filter_path_transformer)); TestRequestHeaderMapImpl original_headers; original_headers.setPath("/xyz/../a"); @@ -1397,7 +1410,12 @@ TEST_F(ConnectionManagerUtilityTest, SanitizePathDefaultOff) { // maybeNormalizePath() leaves already normal paths alone. TEST_F(ConnectionManagerUtilityTest, SanitizePathNormalPath) { - ON_CALL(config_, shouldNormalizePath()).WillByDefault(Return(true)); + std::unique_ptr forwarding_path_transformer = + std::make_unique(true, false); + std::unique_ptr filter_path_transformer = std::make_unique(); + ON_CALL(config_, forwardingPathTransformer()) + .WillByDefault(ReturnRef(*forwarding_path_transformer)); + ON_CALL(config_, filterPathTransformer()).WillByDefault(ReturnRef(*filter_path_transformer)); TestRequestHeaderMapImpl original_headers; original_headers.setPath("/xyz"); @@ -1408,7 +1426,12 @@ TEST_F(ConnectionManagerUtilityTest, SanitizePathNormalPath) { // maybeNormalizePath() normalizes relative paths. TEST_F(ConnectionManagerUtilityTest, SanitizePathRelativePAth) { - ON_CALL(config_, shouldNormalizePath()).WillByDefault(Return(true)); + std::unique_ptr forwarding_path_transformer = + std::make_unique(true, false); + std::unique_ptr filter_path_transformer = std::make_unique(); + ON_CALL(config_, forwardingPathTransformer()) + .WillByDefault(ReturnRef(*forwarding_path_transformer)); + ON_CALL(config_, filterPathTransformer()).WillByDefault(ReturnRef(*filter_path_transformer)); TestRequestHeaderMapImpl original_headers; original_headers.setPath("/xyz/../abc"); @@ -1419,8 +1442,12 @@ TEST_F(ConnectionManagerUtilityTest, SanitizePathRelativePAth) { // 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)); + std::unique_ptr forwarding_path_transformer = + std::make_unique(false, false); + std::unique_ptr filter_path_transformer = std::make_unique(); + ON_CALL(config_, forwardingPathTransformer()) + .WillByDefault(ReturnRef(*forwarding_path_transformer)); + ON_CALL(config_, filterPathTransformer()).WillByDefault(ReturnRef(*filter_path_transformer)); TestRequestHeaderMapImpl original_headers; original_headers.setPath("/xyz///abc"); @@ -1431,8 +1458,12 @@ TEST_F(ConnectionManagerUtilityTest, MergeSlashesDefaultOff) { // maybeNormalizePath() merges adjacent slashes. TEST_F(ConnectionManagerUtilityTest, MergeSlashes) { - ON_CALL(config_, shouldNormalizePath()).WillByDefault(Return(true)); - ON_CALL(config_, shouldMergeSlashes()).WillByDefault(Return(true)); + std::unique_ptr forwarding_path_transformer = + std::make_unique(false, true); + std::unique_ptr filter_path_transformer = std::make_unique(); + ON_CALL(config_, forwardingPathTransformer()) + .WillByDefault(ReturnRef(*forwarding_path_transformer)); + ON_CALL(config_, filterPathTransformer()).WillByDefault(ReturnRef(*filter_path_transformer)); TestRequestHeaderMapImpl original_headers; original_headers.setPath("/xyz///abc"); @@ -1443,8 +1474,12 @@ TEST_F(ConnectionManagerUtilityTest, MergeSlashes) { // 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)); + std::unique_ptr forwarding_path_transformer = + std::make_unique(false, true); + std::unique_ptr filter_path_transformer = std::make_unique(); + ON_CALL(config_, forwardingPathTransformer()) + .WillByDefault(ReturnRef(*forwarding_path_transformer)); + ON_CALL(config_, filterPathTransformer()).WillByDefault(ReturnRef(*filter_path_transformer)); TestRequestHeaderMapImpl original_headers; original_headers.setPath("/xyz/..//abc"); diff --git a/test/common/http/path_utility_test.cc b/test/common/http/path_utility_test.cc index d7c639934136..7e1eff1c959a 100644 --- a/test/common/http/path_utility_test.cc +++ b/test/common/http/path_utility_test.cc @@ -37,6 +37,15 @@ TEST_F(PathUtilityTest, AlreadyNormalPaths) { } } +// Already normalized path don't change. +TEST(PathTransformationTest, AlreadyNormalPaths) { + const std::vector normal_paths{"/xyz", "/x/y/z"}; + for (const auto& path : normal_paths) { + const auto result = PathTransformer::rfcNormalize(absl::string_view(path)); + EXPECT_EQ(path, result); + } +} + // Invalid paths are rejected. TEST_F(PathUtilityTest, InvalidPaths) { const std::vector invalid_paths{"/xyz/.%00../abc", "/xyz/%00.%00./abc", @@ -47,6 +56,15 @@ TEST_F(PathUtilityTest, InvalidPaths) { } } +// Invalid paths are rejected. +TEST(PathTransformationTest, InvalidPaths) { + const std::vector invalid_paths{"/xyz/.%00../abc", "/xyz/%00.%00./abc", + "/xyz/AAAAA%%0000/abc"}; + for (const auto& path : invalid_paths) { + EXPECT_FALSE(PathTransformer::rfcNormalize(path).has_value()) << "original path: " << path; + } +} + // Paths that are valid get normalized. TEST_F(PathUtilityTest, NormalizeValidPaths) { const std::vector> non_normal_pairs{ @@ -57,7 +75,9 @@ TEST_F(PathUtilityTest, NormalizeValidPaths) { {"/a/..\\c", "/c"}, // "..\\" canonicalization {"/%c0%af", "/%c0%af"}, // 2 bytes unicode reserved characters {"/%5c%25", "/%5c%25"}, // reserved characters - {"/a/b/%2E%2E/c", "/a/c"} // %2E escape + {"/a/b/%2E%2E/c", "/a/c"}, // %2E escape + {"/a/b/%2E./c", "/a/c"}, {"/a/b/%2E/c", "/a/b/c"}, {"..", "/"}, + {"/a/b/%2E/c", "/a/b/c"}, {"/../a/b", "/a/b"}, {"/./a/b", "/a/b"}, }; for (const auto& path_pair : non_normal_pairs) { @@ -69,6 +89,32 @@ TEST_F(PathUtilityTest, NormalizeValidPaths) { } } +// Already normalized path don't change. +TEST(PathTransformationTest, NormalizeValidPaths) { + + const std::vector> non_normal_pairs{ + {"/a/b/../c", "/a/c"}, // parent dir + {"/a/b/./c", "/a/b/c"}, // current dir + {"a/b/../c", "/a/c"}, // non / start + {"/a/b/../../../../c", "/c"}, // out number parent + {"/a/..\\c", "/c"}, // "..\\" canonicalization + {"/%c0%af", "/%c0%af"}, // 2 bytes unicode reserved characters + {"/%5c%25", "/%5c%25"}, // reserved characters + {"/a/b/%2E%2E/c", "/a/c"} // %2E escape + }; + + const std::vector normal_paths{"/xyz", "/x/y/z"}; + for (const auto& path : normal_paths) { + const auto result = PathTransformer::rfcNormalize(absl::string_view(path)).value(); + EXPECT_EQ(path, result); + } + for (const auto& path_pair : non_normal_pairs) { + const auto& path = path_pair.first; + const auto result = PathTransformer::rfcNormalize(absl::string_view(path)).value(); + EXPECT_EQ(result, path_pair.second); + } +} + // Paths that are valid get normalized. TEST_F(PathUtilityTest, NormalizeCasePath) { const std::vector> non_normal_pairs{ @@ -86,6 +132,23 @@ TEST_F(PathUtilityTest, NormalizeCasePath) { << "original path: " << path_pair.second; } } + +// Paths that are valid get normalized. +TEST(PathTransformationTest, NormalizeCasePath) { + const std::vector> non_normal_pairs{ + {"/A/B/C", "/A/B/C"}, // not normalize to lower case + {"/a/b/%2E%2E/c", "/a/c"}, // %2E can be normalized to . + {"/a/b/%2e%2e/c", "/a/c"}, // %2e can be normalized to . + {"/a/%2F%2f/c", "/a/%2F%2f/c"}, // %2F is not normalized to %2f + }; + + for (const auto& path_pair : non_normal_pairs) { + const auto& path = path_pair.first; + const auto result = PathTransformer::rfcNormalize(absl::string_view(path)).value(); + EXPECT_EQ(result, path_pair.second); + } +} + // These test cases are explicitly not covered above: // "/../c\r\n\" '\n' '\r' should be excluded by http parser // "/a/\0c", '\0' should be excluded by http parser @@ -113,6 +176,25 @@ TEST_F(PathUtilityTest, MergeSlashes) { EXPECT_EQ("/a/?b", mergeSlashes("//a/?b")); // ends with slash + query } +TEST(PathTransformationTest, MergeSlashes) { + EXPECT_EQ("", PathTransformer::mergeSlashes("").value()); // empty + EXPECT_EQ("a/b/c", PathTransformer::mergeSlashes("a//b/c").value()); // relative + EXPECT_EQ("/a/b/c/", PathTransformer::mergeSlashes("/a//b/c/").value()); // ends with slash + EXPECT_EQ("a/b/c/", PathTransformer::mergeSlashes("a//b/c/").value()); // relative ends with slash + EXPECT_EQ("/a", PathTransformer::mergeSlashes("/a").value()); // no-op + EXPECT_EQ("/a/b/c", PathTransformer::mergeSlashes("//a/b/c").value()); // double / start + EXPECT_EQ("/a/b/c", PathTransformer::mergeSlashes("/a//b/c").value()); // double / in the middle + EXPECT_EQ("/a/b/c/", PathTransformer::mergeSlashes("/a/b/c//").value()); // double / end + EXPECT_EQ("/a/b/c", PathTransformer::mergeSlashes("/a///b/c").value()); // triple / in the middle + EXPECT_EQ("/a/b/c", + PathTransformer::mergeSlashes("/a////b/c").value()); // quadruple / in the middle + EXPECT_EQ( + "/a/b?a=///c", + PathTransformer::mergeSlashes("/a//b?a=///c").value()); // slashes in the query are ignored + EXPECT_EQ("/a/b?", PathTransformer::mergeSlashes("/a//b?").value()); // empty query + EXPECT_EQ("/a/?b", PathTransformer::mergeSlashes("//a/?b").value()); // ends with slash + query +} + TEST_F(PathUtilityTest, RemoveQueryAndFragment) { EXPECT_EQ("", PathUtil::removeQueryAndFragment("")); EXPECT_EQ("/abc", PathUtil::removeQueryAndFragment("/abc")); @@ -133,5 +215,82 @@ TEST_F(PathUtilityTest, RemoveQueryAndFragment) { EXPECT_EQ("/abc", PathUtil::removeQueryAndFragment("/abc?param=value#fragment")); } +class PathTransformerTest : public testing::Test { +public: + void setPathTransformer(std::vector transformations_config) { + envoy::type::http::v3::PathTransformation path_transformation_config; + Protobuf::RepeatedPtrField* operations = + path_transformation_config.mutable_operations(); + for (std::string const& transformation : transformations_config) { + auto* operation = operations->Add(); + if (transformation == "NormalizePathRFC3986") { + operation->mutable_normalize_path_rfc_3986(); + } else if (transformation == "MergeSlashes") { + operation->mutable_merge_slashes(); + } + } + path_transformer_ = std::make_unique(path_transformation_config); + } + const PathTransformer& pathTransformer() { return *path_transformer_; } + + std::unique_ptr path_transformer_; +}; + +TEST_F(PathTransformerTest, MergeSlashes) { + setPathTransformer({"MergeSlashes"}); + PathTransformer const& path_transformer = pathTransformer(); + EXPECT_EQ("", path_transformer.transform("").value()); // empty + EXPECT_EQ("a/b/c", path_transformer.transform("a//b/c").value()); // relative + EXPECT_EQ("/a/b/c/", path_transformer.transform("/a//b/c/").value()); // ends with slash + EXPECT_EQ("a/b/c/", path_transformer.transform("a//b/c/").value()); // relative ends with slash + EXPECT_EQ("/a", path_transformer.transform("/a").value()); // no-op + EXPECT_EQ("/a/b/c", path_transformer.transform("//a/b/c").value()); // double / start + EXPECT_EQ("/a/b/c", path_transformer.transform("/a//b/c").value()); // double / in the middle + EXPECT_EQ("/a/b/c/", path_transformer.transform("/a/b/c//").value()); // double / end + EXPECT_EQ("/a/b/c", path_transformer.transform("/a///b/c").value()); // triple / in the middle + EXPECT_EQ("/a/b/c", + path_transformer.transform("/a////b/c").value()); // quadruple / in the middle + EXPECT_EQ("/a/b?a=///c", + path_transformer.transform("/a//b?a=///c").value()); // slashes in the query are ignored + EXPECT_EQ("/a/b?", path_transformer.transform("/a//b?").value()); // empty query + EXPECT_EQ("/a/?b", path_transformer.transform("//a/?b").value()); // ends with slash + query +} + +TEST_F(PathTransformerTest, RfcNormalize) { + setPathTransformer({"NormalizePathRFC3986"}); + PathTransformer const& path_transformer = pathTransformer(); + EXPECT_EQ("/x/y/z", + path_transformer.transform("/x/y/z").value()); // Already normalized path don't change. + + EXPECT_EQ("/a/c", path_transformer.transform("a/b/../c").value()); // parent dir + EXPECT_EQ("/a/b/c", path_transformer.transform("/a/b/./c").value()); // current dir + EXPECT_EQ("/a/c", path_transformer.transform("a/b/../c").value()); // non / start + EXPECT_EQ("/c", path_transformer.transform("/a/b/../../../../c").value()); // out number parent + EXPECT_EQ("/c", path_transformer.transform("/a/..\\c").value()); // "..\\" canonicalization + EXPECT_EQ("/%c0%af", + path_transformer.transform("/%c0%af").value()); // 2 bytes unicode reserved characters + EXPECT_EQ("/%5c%25", path_transformer.transform("/%5c%25").value()); // reserved characters + EXPECT_EQ("/a/c", path_transformer.transform("/a/b/%2E%2E/c").value()); // %2E escape + + EXPECT_EQ("/A/B/C", path_transformer.transform("/A/B/C").value()); // empty + EXPECT_EQ("/a/c", path_transformer.transform("/a/b/%2E%2E/c").value()); // relative + EXPECT_EQ("/a/c", path_transformer.transform("/a/b/%2e%2e/c").value()); // ends with slash + EXPECT_EQ("/a/%2F%2f/c", + path_transformer.transform("/a/%2F%2f/c").value()); // relative ends with slash + + EXPECT_FALSE(path_transformer.transform("/xyz/.%00../abc").has_value()); + EXPECT_FALSE(path_transformer.transform("/xyz/%00.%00./abc").has_value()); + EXPECT_FALSE(path_transformer.transform("/xyz/AAAAA%%0000/abc").has_value()); +} + +TEST_F(PathTransformerTest, DuplicateTransformation) { + EXPECT_THROW(setPathTransformer({"MergeSlashes", "MergeSlashes"}), EnvoyException); + EXPECT_THROW(setPathTransformer({"MergeSlashes", "NormalizePathRFC3986", "MergeSlashes"}), + EnvoyException); + EXPECT_THROW(setPathTransformer({"NormalizePathRFC3986", "MergeSlashes", "NormalizePathRFC3986"}), + EnvoyException); + EXPECT_NO_THROW(setPathTransformer({"MergeSlashes", "NormalizePathRFC3986"})); +} + } // namespace Http } // namespace Envoy diff --git a/test/extensions/filters/network/http_connection_manager/BUILD b/test/extensions/filters/network/http_connection_manager/BUILD index cf1a8ea1827c..b8e99971dee3 100644 --- a/test/extensions/filters/network/http_connection_manager/BUILD +++ b/test/extensions/filters/network/http_connection_manager/BUILD @@ -26,6 +26,7 @@ envoy_extension_cc_test_library( deps = [ ":config_cc_proto", "//source/common/filter/http:filter_config_discovery_lib", + "//source/common/http:conn_manager_lib", "//source/common/network:address_lib", "//source/extensions/filters/http/health_check:config", "//source/extensions/filters/http/router:config", 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 40ec2c38e4f9..2b40f975a0eb 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -1,3 +1,4 @@ +#include "envoy/common/exception.h" #include "envoy/config/core/v3/base.pb.h" #include "envoy/config/trace/v3/http_tracer.pb.h" #include "envoy/config/trace/v3/opencensus.pb.h" @@ -7,6 +8,7 @@ #include "envoy/server/request_id_extension_config.h" #include "envoy/type/v3/percent.pb.h" +#include "common/http/conn_manager_utility.h" #include "common/network/address_impl.h" #include "extensions/filters/network/http_connection_manager/config.h" @@ -996,6 +998,92 @@ TEST_F(HttpConnectionManagerConfigTest, MergeSlashesDefault) { EXPECT_FALSE(config.shouldMergeSlashes()); } +// Validated that forwarding transformation is applied to both forwarding path and filter path. +TEST_F(HttpConnectionManagerConfigTest, ForwadingTransformation) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + route_config: + name: local_route + path_normalization_options: + forwarding_transformation: + operations: + - normalize_path_rfc_3986: {} + - merge_slashes: {} + http_filter_transformation: + operations: + merge_slashes: true + normalize_path: true + http_filters: + - name: envoy.filters.http.router + )EOF"; + + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromYaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_, http_tracer_manager_, + filter_config_provider_manager_); + Http::TestRequestHeaderMapImpl header_map{ + {":method", "GET"}, {":path", "/foo//bar"}, {":authority", "host"}, {":scheme", "http"}}; + Http::ConnectionManagerUtility::maybeNormalizePath(header_map, config); + EXPECT_EQ(header_map.getForwardingPath(), "/foo/bar"); + EXPECT_EQ(header_map.getFilterPath(), "/foo/bar"); +} + +// Validated that http filter transformation is applied to only filter path. +TEST_F(HttpConnectionManagerConfigTest, FilterTransformation) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + route_config: + name: local_route + path_normalization_options: + forwarding_transformation: + operations: + http_filter_transformation: + operations: + - normalize_path_rfc_3986: {} + - merge_slashes: {} + merge_slashes: true + normalize_path: true + http_filters: + - name: envoy.filters.http.router + )EOF"; + + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromYaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_, http_tracer_manager_, + filter_config_provider_manager_); + Http::TestRequestHeaderMapImpl header_map{ + {":method", "GET"}, {":path", "/foo//bar"}, {":authority", "host"}, {":scheme", "http"}}; + Http::ConnectionManagerUtility::maybeNormalizePath(header_map, config); + EXPECT_EQ(header_map.getForwardingPath(), "/foo//bar"); + EXPECT_EQ(header_map.getFilterPath(), "/foo/bar"); +} + +// Validated duplicate operation inside a transformation throw exception. +TEST_F(HttpConnectionManagerConfigTest, DuplicatePathTransformation) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + route_config: + name: local_route + path_normalization_options: + forwarding_transformation: + operations: + - normalize_path_rfc_3986: {} + - normalize_path_rfc_3986: {} + http_filter_transformation: + operations: + merge_slashes: true + normalize_path: true + http_filters: + - name: envoy.filters.http.router + )EOF"; + + EXPECT_THROW(HttpConnectionManagerConfig(parseHttpConnectionManagerFromYaml(yaml_string), + context_, date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_, + http_tracer_manager_, filter_config_provider_manager_), + EnvoyException); +} + // Validated that when configured, we merge slashes. TEST_F(HttpConnectionManagerConfigTest, MergeSlashesTrue) { const std::string yaml_string = R"EOF( diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index d10b64b2b7df..8e002e8b6fe7 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -1,4 +1,5 @@ #include "envoy/api/v2/endpoint.pb.h" +#include "envoy/common/exception.h" #include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "envoy/config/cluster/v3/cluster.pb.h" #include "envoy/config/core/v3/base.pb.h" @@ -206,6 +207,7 @@ class HeaderIntegrationTest } void prepareEDS() { + config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { auto* static_resources = bootstrap.mutable_static_resources(); ASSERT(static_resources->clusters_size() == 1); @@ -283,6 +285,19 @@ class HeaderIntegrationTest hcm) { // Overwrite default config with our own. TestUtility::loadFromYaml(http_connection_mgr_config, hcm); + + if (forwarding_transformation_.has_value()) { + hcm.mutable_path_normalization_options()->mutable_forwarding_transformation()->CopyFrom( + TestUtility::parseYaml( + forwarding_transformation_.value())); + } + if (filter_transformation_.has_value()) { + hcm.mutable_path_normalization_options() + ->mutable_http_filter_transformation() + ->CopyFrom(TestUtility::parseYaml( + filter_transformation_.value())); + } + envoy::extensions::filters::http::router::v3::Router router_config; router_config.set_suppress_envoy_headers(routerSuppressEnvoyHeaders()); hcm.mutable_http_filters(0)->mutable_typed_config()->PackFrom(router_config); @@ -357,7 +372,6 @@ class HeaderIntegrationTest } } }); - initialize(); } @@ -451,7 +465,9 @@ class HeaderIntegrationTest bool normalize_path_{false}; FakeHttpConnectionPtr eds_connection_; FakeStreamPtr eds_stream_; -}; + absl::optional forwarding_transformation_; + absl::optional filter_transformation_; +}; // namespace Envoy INSTANTIATE_TEST_SUITE_P( IpVersionsSuppressEnvoyHeaders, HeaderIntegrationTest, @@ -1055,6 +1071,131 @@ TEST_P(HeaderIntegrationTest, TestPathAndRouteWhenNormalizePathOff) { }); } +TEST_P(HeaderIntegrationTest, TestForwardingPathNormalizationVisibleToUpstream) { + forwarding_transformation_ = std::string( + R"EOF( + operations: + - normalize_path_rfc_3986: {} + )EOF"); + filter_transformation_ = std::string( + R"EOF( + operations: + )EOF"); + initializeFilter(HeaderMode::Append, false); + performRequest( + Http::TestRequestHeaderMapImpl{ + {":method", "GET"}, + {":path", "/private/../public"}, + {":scheme", "http"}, + {":authority", "path-sanitization.com"}, + }, + Http::TestRequestHeaderMapImpl{{":authority", "path-sanitization.com"}, + {":path", "/public"}, + {":method", "GET"}, + {"x-site", "public"}}, + Http::TestResponseHeaderMapImpl{ + {"server", "envoy"}, + {"content-length", "0"}, + {":status", "200"}, + {"x-unmodified", "response"}, + }, + Http::TestResponseHeaderMapImpl{ + {"server", "envoy"}, + {"x-unmodified", "response"}, + {":status", "200"}, + }); +} + +// Validate that filter path normalization is not visible to upstream server. +TEST_P(HeaderIntegrationTest, TestFilterPathNormalizationInvisibleToUpstream) { + forwarding_transformation_ = std::string( + R"EOF( + operations: + )EOF"); + filter_transformation_ = std::string( + R"EOF( + operations: + - normalize_path_rfc_3986: {} + )EOF"); + initializeFilter(HeaderMode::Append, false); + performRequest( + Http::TestRequestHeaderMapImpl{ + {":method", "GET"}, + {":path", "/private/../public"}, + {":scheme", "http"}, + {":authority", "path-sanitization.com"}, + }, + Http::TestRequestHeaderMapImpl{{":authority", "path-sanitization.com"}, + {":path", "/private/../public"}, + {":method", "GET"}, + {"x-site", "public"}}, + Http::TestResponseHeaderMapImpl{ + {"server", "envoy"}, + {"content-length", "0"}, + {":status", "200"}, + {"x-unmodified", "response"}, + }, + Http::TestResponseHeaderMapImpl{ + {"server", "envoy"}, + {"x-unmodified", "response"}, + {":status", "200"}, + }); +} + +// Validate that new path normalization api take precedence. +// Old api set path normalization off, new api set path normalization on, should normalize. +TEST_P(HeaderIntegrationTest, TestOldPathNormalizationApiIgnoreWhenNewApiConfigured) { + normalize_path_ = false; + forwarding_transformation_ = std::string( + R"EOF( + operations: + - normalize_path_rfc_3986: {} + )EOF"); + filter_transformation_ = std::string( + R"EOF( + operations: + )EOF"); + initializeFilter(HeaderMode::Append, false); + performRequest( + Http::TestRequestHeaderMapImpl{ + {":method", "GET"}, + {":path", "/private/../public"}, + {":scheme", "http"}, + {":authority", "path-sanitization.com"}, + }, + Http::TestRequestHeaderMapImpl{{":authority", "path-sanitization.com"}, + {":path", "/public"}, + {":method", "GET"}, + {"x-site", "public"}}, + Http::TestResponseHeaderMapImpl{ + {"server", "envoy"}, + {"content-length", "0"}, + {":status", "200"}, + {"x-unmodified", "response"}, + }, + Http::TestResponseHeaderMapImpl{ + {"server", "envoy"}, + {"x-unmodified", "response"}, + {":status", "200"}, + }); +} + +// Validate that envoy reject configuration when operations are duplicate in the same +// transformation. +TEST_P(HeaderIntegrationTest, TestDuplicateOperationInPathTransformation) { + forwarding_transformation_ = std::string( + R"EOF( + operations: + - normalize_path_rfc_3986: {} + - normalize_path_rfc_3986: {} + )EOF"); + filter_transformation_ = std::string( + R"EOF( + operations: + )EOF"); + EXPECT_DEATH(initializeFilter(HeaderMode::Append, false), "Details: Lds update failed."); +} + // Validates behavior when normalize path is on. // Path to decide route and path to upstream are both // the normalized. diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index d41a90c9435d..fa73d15ae765 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -577,6 +577,8 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD(const Http::Http1Settings&, http1Settings, (), (const)); MOCK_METHOD(bool, shouldNormalizePath, (), (const)); MOCK_METHOD(bool, shouldMergeSlashes, (), (const)); + MOCK_METHOD(const Http::PathTransformer&, filterPathTransformer, (), (const)); + MOCK_METHOD(const Http::PathTransformer&, forwardingPathTransformer, (), (const)); MOCK_METHOD(Http::StripPortType, stripPortType, (), (const)); MOCK_METHOD(envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction, headersWithUnderscoresAction, (), (const)); diff --git a/test/test_common/utility.h b/test/test_common/utility.h index e0e8da053d99..8e3cadb23780 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -1039,6 +1039,15 @@ class TestRequestHeaderMapImpl INLINE_REQ_NUMERIC_HEADERS(DEFINE_TEST_INLINE_NUMERIC_HEADER_FUNCS) INLINE_REQ_RESP_STRING_HEADERS(DEFINE_TEST_INLINE_STRING_HEADER_FUNCS) INLINE_REQ_RESP_NUMERIC_HEADERS(DEFINE_TEST_INLINE_NUMERIC_HEADER_FUNCS) + absl::string_view getForwardingPath() override { return forwarding_path_; } + absl::string_view getFilterPath() override { return filter_path_; } + +private: + void setForwardingPath(absl::string_view path) override { forwarding_path_ = path; } + void setFilterPath(absl::string_view path) override { filter_path_ = path; } + + std::string forwarding_path_; + std::string filter_path_; }; using TestRequestTrailerMapImpl = TestHeaderMapImplBase;