From d8e1b04ed520286878912b9dab62f2cfc6a56d0f Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Tue, 9 Mar 2021 05:59:06 +0000 Subject: [PATCH 01/28] path transformer interface Signed-off-by: chaoqin-li1123 --- source/common/http/BUILD | 1 + source/common/http/path_utility.cc | 21 +++++++++++++++++++++ source/common/http/path_utility.h | 9 +++++++++ 3 files changed, 31 insertions(+) diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 10de05782ae5..f88ad2184506 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -443,6 +443,7 @@ envoy_cc_library( "//source/common/common:logger_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/path_utility.cc b/source/common/http/path_utility.cc index 988e82b9be82..b270351d85f1 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 "common/common/logger.h" #include "common/http/legacy_path_canonicalizer.h" #include "common/runtime/runtime_features.h" @@ -85,5 +87,24 @@ absl::string_view PathUtil::removeQueryAndFragment(const absl::string_view path) return ret; } + + +PathTransformer::PathTransformer(envoy::type::http::v3::PathTransformation path_transformation) { + const google::protobuf::RepeatedPtrField operations = path_transformation.operations(); + for (auto const& operation: operations) { + if (operation.has_normalize_path_rfc_3986()) { + + } + else if (operation.has_merge_slashes()) { + + } + } +} + + +std::string PathTransformer::transform(absl::string_view original) { + return std::string(original[0], 3); +} + } // namespace Http } // namespace Envoy diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index a6a99aaef78d..e15efaab648f 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -4,6 +4,8 @@ #include "absl/strings/string_view.h" +#include "envoy/type/http/v3/path_transformation.pb.h" + namespace Envoy { namespace Http { @@ -24,5 +26,12 @@ class PathUtil { static absl::string_view removeQueryAndFragment(const absl::string_view path); }; +class PathTransformer { +public: + PathTransformer(envoy::type::http::v3::PathTransformation operations); + + std::string transform(const absl::string_view original); +}; + } // namespace Http } // namespace Envoy From effbb999100fb05a4fdd9a8eba3559b57e2d9b9c Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 10 Mar 2021 00:16:20 +0000 Subject: [PATCH 02/28] add normalization helper function Signed-off-by: chaoqin-li1123 --- source/common/http/path_utility.cc | 41 +++++++++++++++++++++++++++--- source/common/http/path_utility.h | 7 +++++ 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index b270351d85f1..7da2c364a605 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -87,23 +87,58 @@ absl::string_view PathUtil::removeQueryAndFragment(const absl::string_view path) return ret; } +std::string 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 original_path.data(); + } + 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); +} + +std::string 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 + ); + auto& normalized_path = normalized_path_opt.value(); + const absl::string_view query_suffix = + query_pos == original_path.npos + ? absl::string_view{} + : absl::string_view{original_path.data() + query_pos, original_path.size() - query_pos}; + if (!query_suffix.empty()) { + normalized_path.insert(normalized_path.end(), query_suffix.begin(), query_suffix.end()); + } + return normalized_path; +} PathTransformer::PathTransformer(envoy::type::http::v3::PathTransformation path_transformation) { const google::protobuf::RepeatedPtrField operations = path_transformation.operations(); for (auto const& operation: operations) { if (operation.has_normalize_path_rfc_3986()) { - + transformations.emplace_back(rfcNormalize); } else if (operation.has_merge_slashes()) { - + transformations.emplace_back(mergeSlashes); } } } std::string PathTransformer::transform(absl::string_view original) { - return std::string(original[0], 3); + std::string transformed = original.data(); + for (Transformation transformation: transformations) { + transformed = transformation(transformed); + } + return transformed; } } // namespace Http diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index e15efaab648f..7d0ac6bdb312 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -6,6 +6,8 @@ #include "envoy/type/http/v3/path_transformation.pb.h" +#include + namespace Envoy { namespace Http { @@ -26,11 +28,16 @@ class PathUtil { static absl::string_view removeQueryAndFragment(const absl::string_view path); }; +using Transformation = std::function; + class PathTransformer { public: PathTransformer(envoy::type::http::v3::PathTransformation operations); std::string transform(const absl::string_view original); + +private: + std::list transformations; }; } // namespace Http From 579c5b15f5ca74ce0040f034a605d23ee81bba89 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 10 Mar 2021 00:31:13 +0000 Subject: [PATCH 03/28] fix format Signed-off-by: chaoqin-li1123 --- source/common/http/path_utility.cc | 20 ++++++++------------ source/common/http/path_utility.h | 7 +++---- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index 7da2c364a605..a5c75d8249d1 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -1,7 +1,5 @@ #include "common/http/path_utility.h" - - #include "common/common/logger.h" #include "common/http/legacy_path_canonicalizer.h" #include "common/runtime/runtime_features.h" @@ -91,14 +89,13 @@ std::string 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) { + if (path.find("//") == absl::string_view::npos) { return original_path.data(); } 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); + return absl::StrCat(path_prefix, absl::StrJoin(absl::StrSplit(path, '/', absl::SkipEmpty()), "/"), + path_suffix, query); } std::string rfcNormalize(absl::string_view original_path) { @@ -121,21 +118,20 @@ std::string rfcNormalize(absl::string_view original_path) { } PathTransformer::PathTransformer(envoy::type::http::v3::PathTransformation path_transformation) { - const google::protobuf::RepeatedPtrField operations = path_transformation.operations(); - for (auto const& operation: operations) { + const Protobuf::RepeatedPtrField operations = + path_transformation.operations(); + for (auto const& operation : operations) { if (operation.has_normalize_path_rfc_3986()) { transformations.emplace_back(rfcNormalize); - } - else if (operation.has_merge_slashes()) { + } else if (operation.has_merge_slashes()) { transformations.emplace_back(mergeSlashes); } } } - std::string PathTransformer::transform(absl::string_view original) { std::string transformed = original.data(); - for (Transformation transformation: transformations) { + for (Transformation transformation : transformations) { transformed = transformation(transformed); } return transformed; diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index 7d0ac6bdb312..910a8b57783f 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -1,12 +1,11 @@ #pragma once -#include "envoy/http/header_map.h" - -#include "absl/strings/string_view.h" +#include +#include "envoy/http/header_map.h" #include "envoy/type/http/v3/path_transformation.pb.h" -#include +#include "absl/strings/string_view.h" namespace Envoy { namespace Http { From 12cb83a0572ad8b9080bdbccd7787ebaf584b91f Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 10 Mar 2021 01:26:09 +0000 Subject: [PATCH 04/28] add unit test Signed-off-by: chaoqin-li1123 --- source/common/http/path_utility.cc | 8 ++++---- source/common/http/path_utility.h | 8 +++++++- test/common/http/path_utility_test.cc | 9 +++++++++ 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index a5c75d8249d1..9605adb9fc6a 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -85,7 +85,7 @@ absl::string_view PathUtil::removeQueryAndFragment(const absl::string_view path) return ret; } -std::string mergeSlashes(absl::string_view original_path) { +std::string 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); @@ -98,7 +98,7 @@ std::string mergeSlashes(absl::string_view original_path) { path_suffix, query); } -std::string rfcNormalize(absl::string_view original_path) { +std::string PathTransformer::rfcNormalize(absl::string_view original_path) { const auto query_pos = original_path.find('?'); auto normalized_path_opt = canonicalizePath( query_pos == original_path.npos @@ -122,9 +122,9 @@ PathTransformer::PathTransformer(envoy::type::http::v3::PathTransformation path_ path_transformation.operations(); for (auto const& operation : operations) { if (operation.has_normalize_path_rfc_3986()) { - transformations.emplace_back(rfcNormalize); + transformations.emplace_back(PathTransformer::rfcNormalize); } else if (operation.has_merge_slashes()) { - transformations.emplace_back(mergeSlashes); + transformations.emplace_back(PathTransformer::mergeSlashes); } } } diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index 910a8b57783f..568957055722 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -5,6 +5,8 @@ #include "envoy/http/header_map.h" #include "envoy/type/http/v3/path_transformation.pb.h" +#include "common/protobuf/protobuf.h" + #include "absl/strings/string_view.h" namespace Envoy { @@ -33,7 +35,11 @@ class PathTransformer { public: PathTransformer(envoy::type::http::v3::PathTransformation operations); - std::string transform(const absl::string_view original); + std::string transform(const absl::string_view original_path); + + static std::string mergeSlashes(absl::string_view original_path); + + static std::string rfcNormalize(absl::string_view original_path); private: std::list transformations; diff --git a/test/common/http/path_utility_test.cc b/test/common/http/path_utility_test.cc index d7c639934136..1848ee31b3aa 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", From 33e999f07f8592c0d4d6280cbdcdac42b4da6f04 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 10 Mar 2021 01:48:19 +0000 Subject: [PATCH 05/28] more unit tests Signed-off-by: chaoqin-li1123 --- test/common/http/path_utility_test.cc | 60 +++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/test/common/http/path_utility_test.cc b/test/common/http/path_utility_test.cc index 1848ee31b3aa..0519a28f7a38 100644 --- a/test/common/http/path_utility_test.cc +++ b/test/common/http/path_utility_test.cc @@ -78,6 +78,32 @@ TEST_F(PathUtilityTest, NormalizeValidPaths) { } } +// Already normalized path don't change. +TEST(PathTransformerTest, 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)); + 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)); + EXPECT_EQ(result, path_pair.second); + } +} + // Paths that are valid get normalized. TEST_F(PathUtilityTest, NormalizeCasePath) { const std::vector> non_normal_pairs{ @@ -95,6 +121,23 @@ TEST_F(PathUtilityTest, NormalizeCasePath) { << "original path: " << path_pair.second; } } + +// Paths that are valid get normalized. +TEST(PathTransformerTest, 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)); + 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 @@ -122,6 +165,23 @@ TEST_F(PathUtilityTest, MergeSlashes) { EXPECT_EQ("/a/?b", mergeSlashes("//a/?b")); // ends with slash + query } +TEST(PathTransformerTest, MergeSlashes) { + EXPECT_EQ("", PathTransformer::mergeSlashes("")); // empty + EXPECT_EQ("a/b/c", PathTransformer::mergeSlashes("a//b/c")); // relative + EXPECT_EQ("/a/b/c/", PathTransformer::mergeSlashes("/a//b/c/")); // ends with slash + EXPECT_EQ("a/b/c/", PathTransformer::mergeSlashes("a//b/c/")); // relative ends with slash + EXPECT_EQ("/a", PathTransformer::mergeSlashes("/a")); // no-op + EXPECT_EQ("/a/b/c", PathTransformer::mergeSlashes("//a/b/c")); // double / start + EXPECT_EQ("/a/b/c", PathTransformer::mergeSlashes("/a//b/c")); // double / in the middle + EXPECT_EQ("/a/b/c/", PathTransformer::mergeSlashes("/a/b/c//")); // double / end + EXPECT_EQ("/a/b/c", PathTransformer::mergeSlashes("/a///b/c")); // triple / in the middle + EXPECT_EQ("/a/b/c", PathTransformer::mergeSlashes("/a////b/c")); // quadruple / in the middle + EXPECT_EQ("/a/b?a=///c", + PathTransformer::mergeSlashes("/a//b?a=///c")); // slashes in the query are ignored + EXPECT_EQ("/a/b?", PathTransformer::mergeSlashes("/a//b?")); // empty query + EXPECT_EQ("/a/?b", PathTransformer::mergeSlashes("//a/?b")); // ends with slash + query +} + TEST_F(PathUtilityTest, RemoveQueryAndFragment) { EXPECT_EQ("", PathUtil::removeQueryAndFragment("")); EXPECT_EQ("/abc", PathUtil::removeQueryAndFragment("/abc")); From d4330f306428731ca1f9ff52e55d31a28944fb8b Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 10 Mar 2021 21:04:08 +0000 Subject: [PATCH 06/28] transformer return optional string Signed-off-by: chaoqin-li1123 --- source/common/http/path_utility.cc | 14 +++++++----- source/common/http/path_utility.h | 8 +++---- test/common/http/path_utility_test.cc | 32 +++++++++++++-------------- 3 files changed, 28 insertions(+), 26 deletions(-) diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index 9605adb9fc6a..fff43684b528 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -85,7 +85,7 @@ absl::string_view PathUtil::removeQueryAndFragment(const absl::string_view path) return ret; } -std::string PathTransformer::mergeSlashes(absl::string_view original_path) { +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); @@ -98,7 +98,7 @@ std::string PathTransformer::mergeSlashes(absl::string_view original_path) { path_suffix, query); } -std::string PathTransformer::rfcNormalize(absl::string_view original_path) { +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 @@ -129,12 +129,14 @@ PathTransformer::PathTransformer(envoy::type::http::v3::PathTransformation path_ } } -std::string PathTransformer::transform(absl::string_view original) { - std::string transformed = original.data(); +absl::optional PathTransformer::transform(absl::string_view original) { + std::string path = original.data(); for (Transformation transformation : transformations) { - transformed = transformation(transformed); + absl::optional transformed = transformation(path).value(); + if (!transformed.has_value()) return {}; + path = transformed.value(); } - return transformed; + return path; } } // namespace Http diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index 568957055722..d7e963f84cfa 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -29,17 +29,17 @@ class PathUtil { static absl::string_view removeQueryAndFragment(const absl::string_view path); }; -using Transformation = std::function; +using Transformation = std::function(absl::string_view)>; class PathTransformer { public: PathTransformer(envoy::type::http::v3::PathTransformation operations); - std::string transform(const absl::string_view original_path); + absl::optional transform(const absl::string_view original_path); - static std::string mergeSlashes(absl::string_view original_path); + static absl::optional mergeSlashes(absl::string_view original_path); - static std::string rfcNormalize(absl::string_view original_path); + static absl::optional rfcNormalize(absl::string_view original_path); private: std::list transformations; diff --git a/test/common/http/path_utility_test.cc b/test/common/http/path_utility_test.cc index 0519a28f7a38..92a63ab3ca24 100644 --- a/test/common/http/path_utility_test.cc +++ b/test/common/http/path_utility_test.cc @@ -94,12 +94,12 @@ TEST(PathTransformerTest, NormalizeValidPaths) { const std::vector normal_paths{"/xyz", "/x/y/z"}; for (const auto& path : normal_paths) { - const auto result = PathTransformer::rfcNormalize(absl::string_view(path)); + 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)); + const auto result = PathTransformer::rfcNormalize(absl::string_view(path)).value(); EXPECT_EQ(result, path_pair.second); } } @@ -133,7 +133,7 @@ TEST(PathTransformerTest, NormalizeCasePath) { for (const auto& path_pair : non_normal_pairs) { const auto& path = path_pair.first; - const auto result = PathTransformer::rfcNormalize(absl::string_view(path)); + const auto result = PathTransformer::rfcNormalize(absl::string_view(path)).value(); EXPECT_EQ(result, path_pair.second); } } @@ -166,20 +166,20 @@ TEST_F(PathUtilityTest, MergeSlashes) { } TEST(PathTransformerTest, MergeSlashes) { - EXPECT_EQ("", PathTransformer::mergeSlashes("")); // empty - EXPECT_EQ("a/b/c", PathTransformer::mergeSlashes("a//b/c")); // relative - EXPECT_EQ("/a/b/c/", PathTransformer::mergeSlashes("/a//b/c/")); // ends with slash - EXPECT_EQ("a/b/c/", PathTransformer::mergeSlashes("a//b/c/")); // relative ends with slash - EXPECT_EQ("/a", PathTransformer::mergeSlashes("/a")); // no-op - EXPECT_EQ("/a/b/c", PathTransformer::mergeSlashes("//a/b/c")); // double / start - EXPECT_EQ("/a/b/c", PathTransformer::mergeSlashes("/a//b/c")); // double / in the middle - EXPECT_EQ("/a/b/c/", PathTransformer::mergeSlashes("/a/b/c//")); // double / end - EXPECT_EQ("/a/b/c", PathTransformer::mergeSlashes("/a///b/c")); // triple / in the middle - EXPECT_EQ("/a/b/c", PathTransformer::mergeSlashes("/a////b/c")); // quadruple / in the middle + 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")); // slashes in the query are ignored - EXPECT_EQ("/a/b?", PathTransformer::mergeSlashes("/a//b?")); // empty query - EXPECT_EQ("/a/?b", PathTransformer::mergeSlashes("//a/?b")); // ends with slash + query + 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) { From 2de9ab6060398cd0fc635caa6be488228de2d64c Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 11 Mar 2021 23:15:06 +0000 Subject: [PATCH 07/28] build path transformer in hcm config Signed-off-by: chaoqin-li1123 --- source/common/http/path_utility.cc | 4 +++- .../filters/network/http_connection_manager/BUILD | 1 + .../filters/network/http_connection_manager/config.cc | 5 +++-- .../filters/network/http_connection_manager/config.h | 5 +++++ test/common/http/path_utility_test.cc | 9 +++++++++ 5 files changed, 21 insertions(+), 3 deletions(-) diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index fff43684b528..7d32645fc65f 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -105,7 +105,9 @@ absl::optional PathTransformer::rfcNormalize(absl::string_view orig ? original_path : absl::string_view(original_path.data(), query_pos) // '?' is not included ); - + if (!normalized_path_opt.has_value()) { + return {}; + } auto& normalized_path = normalized_path_opt.value(); const absl::string_view query_suffix = query_pos == original_path.npos diff --git a/source/extensions/filters/network/http_connection_manager/BUILD b/source/extensions/filters/network/http_connection_manager/BUILD index 78e2dead4762..871537b4044c 100644 --- a/source/extensions/filters/network/http_connection_manager/BUILD +++ b/source/extensions/filters/network/http_connection_manager/BUILD @@ -32,6 +32,7 @@ envoy_cc_extension( "//include/envoy/server:admin_interface", "//include/envoy/server:options_interface", "//include/envoy/stats:stats_interface", + "//source/common/http:path_utility_lib", "//source/common/access_log:access_log_lib", "//source/common/common:minimal_logger_lib", "//source/common/config:utility_lib", diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 2db39453d10f..d9ed7be59812 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -193,7 +193,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( Config::ConfigProviderManager& scoped_routes_config_provider_manager, Tracing::HttpTracerManager& http_tracer_manager, Filter::Http::FilterConfigProviderManager& filter_config_provider_manager) - : context_(context), stats_prefix_(fmt::format("http.{}.", config.stat_prefix())), + : context_(context), stats_prefix_(fmt::format("http.{}.", config.stat_prefix())), stats_(Http::ConnectionManagerImpl::generateStats(stats_prefix_, context_.scope())), tracing_stats_( Http::ConnectionManagerImpl::generateTracingStats(stats_prefix_, context_.scope())), @@ -253,7 +253,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( merge_slashes_(config.merge_slashes()), headers_with_underscores_action_( config.common_http_protocol_options().headers_with_underscores_action()), - local_reply_(LocalReply::Factory::create(config.local_reply_config(), context)) { + local_reply_(LocalReply::Factory::create(config.local_reply_config(), context)), forwarding_path_transformer_(config.path_normalization_options().forwarding_transformation()), + filter_path_transformer_(config.path_normalization_options().http_filter_transformation()) { // If idle_timeout_ was not configured in common_http_protocol_options, use value in deprecated // idle_timeout field. // TODO(asraa): Remove when idle_timeout is removed. diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 639e3f6ac9a5..4357661473f6 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -28,6 +28,9 @@ #include "common/router/rds_impl.h" #include "common/router/scoped_rds.h" #include "common/tracing/http_tracer_impl.h" +#include "common/http/path_utility.h" + + #include "extensions/filters/network/common/factory_base.h" #include "extensions/filters/network/well_known_names.h" @@ -264,6 +267,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/test/common/http/path_utility_test.cc b/test/common/http/path_utility_test.cc index 92a63ab3ca24..50aa5233ed60 100644 --- a/test/common/http/path_utility_test.cc +++ b/test/common/http/path_utility_test.cc @@ -56,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{ From 1540cdb46d0ff3a56600d6d95543ad5e6a6203a0 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 12 Mar 2021 00:01:38 +0000 Subject: [PATCH 08/28] fix format Signed-off-by: chaoqin-li1123 --- source/common/http/path_utility.cc | 3 ++- .../network/http_connection_manager/BUILD | 2 +- .../network/http_connection_manager/config.cc | 5 +++-- .../network/http_connection_manager/config.h | 4 +--- test/common/http/path_utility_test.cc | 16 +++++++++------- 5 files changed, 16 insertions(+), 14 deletions(-) diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index 7d32645fc65f..c3d8a4d5b14e 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -135,7 +135,8 @@ absl::optional PathTransformer::transform(absl::string_view origina std::string path = original.data(); for (Transformation transformation : transformations) { absl::optional transformed = transformation(path).value(); - if (!transformed.has_value()) return {}; + if (!transformed.has_value()) + return {}; path = transformed.value(); } return path; diff --git a/source/extensions/filters/network/http_connection_manager/BUILD b/source/extensions/filters/network/http_connection_manager/BUILD index 871537b4044c..be590e3a6d54 100644 --- a/source/extensions/filters/network/http_connection_manager/BUILD +++ b/source/extensions/filters/network/http_connection_manager/BUILD @@ -32,13 +32,13 @@ envoy_cc_extension( "//include/envoy/server:admin_interface", "//include/envoy/server:options_interface", "//include/envoy/stats:stats_interface", - "//source/common/http:path_utility_lib", "//source/common/access_log:access_log_lib", "//source/common/common:minimal_logger_lib", "//source/common/config:utility_lib", "//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 d9ed7be59812..cfa5aad0d191 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -193,7 +193,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( Config::ConfigProviderManager& scoped_routes_config_provider_manager, Tracing::HttpTracerManager& http_tracer_manager, Filter::Http::FilterConfigProviderManager& filter_config_provider_manager) - : context_(context), stats_prefix_(fmt::format("http.{}.", config.stat_prefix())), + : context_(context), stats_prefix_(fmt::format("http.{}.", config.stat_prefix())), stats_(Http::ConnectionManagerImpl::generateStats(stats_prefix_, context_.scope())), tracing_stats_( Http::ConnectionManagerImpl::generateTracingStats(stats_prefix_, context_.scope())), @@ -253,7 +253,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( merge_slashes_(config.merge_slashes()), headers_with_underscores_action_( config.common_http_protocol_options().headers_with_underscores_action()), - local_reply_(LocalReply::Factory::create(config.local_reply_config(), context)), forwarding_path_transformer_(config.path_normalization_options().forwarding_transformation()), + local_reply_(LocalReply::Factory::create(config.local_reply_config(), context)), + forwarding_path_transformer_(config.path_normalization_options().forwarding_transformation()), filter_path_transformer_(config.path_normalization_options().http_filter_transformation()) { // If idle_timeout_ was not configured in common_http_protocol_options, use value in deprecated // idle_timeout field. diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 4357661473f6..834a446e4386 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -23,14 +23,12 @@ #include "common/http/date_provider_impl.h" #include "common/http/http1/codec_stats.h" #include "common/http/http2/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" #include "common/router/scoped_rds.h" #include "common/tracing/http_tracer_impl.h" -#include "common/http/path_utility.h" - - #include "extensions/filters/network/common/factory_base.h" #include "extensions/filters/network/well_known_names.h" diff --git a/test/common/http/path_utility_test.cc b/test/common/http/path_utility_test.cc index 50aa5233ed60..0b5a79bee7c8 100644 --- a/test/common/http/path_utility_test.cc +++ b/test/common/http/path_utility_test.cc @@ -178,15 +178,17 @@ TEST(PathTransformerTest, 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()); // 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/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 } From b45e089b827db6149ee62fdc70943aa7726921a8 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 12 Mar 2021 02:39:38 +0000 Subject: [PATCH 09/28] fix clang tidy and add test cases Signed-off-by: chaoqin-li1123 --- source/common/http/path_utility.cc | 9 +++++---- test/common/http/path_utility_test.cc | 4 +++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index c3d8a4d5b14e..78846565e8a2 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -120,8 +120,8 @@ absl::optional PathTransformer::rfcNormalize(absl::string_view orig } PathTransformer::PathTransformer(envoy::type::http::v3::PathTransformation path_transformation) { - const Protobuf::RepeatedPtrField operations = - path_transformation.operations(); + const Protobuf::RepeatedPtrField& + operations = path_transformation.operations(); for (auto const& operation : operations) { if (operation.has_normalize_path_rfc_3986()) { transformations.emplace_back(PathTransformer::rfcNormalize); @@ -133,10 +133,11 @@ PathTransformer::PathTransformer(envoy::type::http::v3::PathTransformation path_ absl::optional PathTransformer::transform(absl::string_view original) { std::string path = original.data(); - for (Transformation transformation : transformations) { + for (Transformation const& transformation : transformations) { absl::optional transformed = transformation(path).value(); - if (!transformed.has_value()) + if (!transformed.has_value()) { return {}; + } path = transformed.value(); } return path; diff --git a/test/common/http/path_utility_test.cc b/test/common/http/path_utility_test.cc index 0b5a79bee7c8..1d4b5222ba07 100644 --- a/test/common/http/path_utility_test.cc +++ b/test/common/http/path_utility_test.cc @@ -75,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) { From 93d74e151192f0be1fccdca1cfc31839738280b4 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 12 Mar 2021 05:40:13 +0000 Subject: [PATCH 10/28] avoid string copy Signed-off-by: chaoqin-li1123 --- source/common/http/path_utility.cc | 20 ++++++++++---------- source/common/http/path_utility.h | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index 78846565e8a2..858a0f538105 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -120,27 +120,27 @@ absl::optional PathTransformer::rfcNormalize(absl::string_view orig } PathTransformer::PathTransformer(envoy::type::http::v3::PathTransformation path_transformation) { - const Protobuf::RepeatedPtrField& - operations = path_transformation.operations(); + const auto& operations = path_transformation.operations(); for (auto const& operation : operations) { if (operation.has_normalize_path_rfc_3986()) { - transformations.emplace_back(PathTransformer::rfcNormalize); + transformations_.emplace_back(PathTransformer::rfcNormalize); } else if (operation.has_merge_slashes()) { - transformations.emplace_back(PathTransformer::mergeSlashes); + transformations_.emplace_back(PathTransformer::mergeSlashes); } } } absl::optional PathTransformer::transform(absl::string_view original) { - std::string path = original.data(); - for (Transformation const& transformation : transformations) { - absl::optional transformed = transformation(path).value(); - if (!transformed.has_value()) { + absl::optional path_string; + 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 = transformed.value(); + path_string_view = path_string.value(); } - return path; + return path_string; } } // namespace Http diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index d7e963f84cfa..c4856aa21d38 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -42,7 +42,7 @@ class PathTransformer { static absl::optional rfcNormalize(absl::string_view original_path); private: - std::list transformations; + std::list transformations_; }; } // namespace Http From 761424758f2170084240777693d05c285c9c4726 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 12 Mar 2021 15:55:30 +0000 Subject: [PATCH 11/28] refactor merge slash Signed-off-by: chaoqin-li1123 --- source/common/http/path_utility.cc | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index 858a0f538105..65d26a1ea764 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -61,18 +61,10 @@ bool PathUtil::canonicalPath(RequestHeaderMap& headers) { 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; + absl::optional normalized = PathTransformer::mergeSlashes(original_path).value(); + if (normalized.has_value()) { + headers.setPath(normalized.value()); } - 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) { @@ -90,7 +82,7 @@ absl::optional PathTransformer::mergeSlashes(absl::string_view orig 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 original_path.data(); + 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(); From 0e08d96ce0882f33ef26e30a5988994cd2f1f7ec Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 12 Mar 2021 17:55:21 +0000 Subject: [PATCH 12/28] reject duplicate transformation Signed-off-by: chaoqin-li1123 --- source/common/http/BUILD | 1 + source/common/http/path_utility.cc | 6 ++++++ source/common/http/path_utility.h | 1 + 3 files changed, 8 insertions(+) diff --git a/source/common/http/BUILD b/source/common/http/BUILD index f88ad2184506..3062a3c37725 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -442,6 +442,7 @@ envoy_cc_library( "//include/envoy/http:header_map_interface", "//source/common/common:logger_lib", "//source/common/runtime:runtime_features_lib", + "//source/common/protobuf:utility_lib", "@com_googlesource_googleurl//url:envoy_url", "@envoy_api//envoy/type/http/v3:pkg_cc_proto", ], diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index 65d26a1ea764..a639a68172b4 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -7,6 +7,7 @@ #include "absl/strings/str_join.h" #include "absl/strings/str_split.h" #include "absl/types/optional.h" +#include "envoy/common/exception.h" #include "url/url_canon.h" #include "url/url_canon_stdstring.h" @@ -113,7 +114,12 @@ absl::optional PathTransformer::rfcNormalize(absl::string_view orig PathTransformer::PathTransformer(envoy::type::http::v3::PathTransformation 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()) { + throw EnvoyException("Duplicate path transformation"); + } if (operation.has_normalize_path_rfc_3986()) { transformations_.emplace_back(PathTransformer::rfcNormalize); } else if (operation.has_merge_slashes()) { diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index c4856aa21d38..9dd63642a982 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -6,6 +6,7 @@ #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" From 0e6bca715a6a6f81ad5dba7332bad52eb768ee4e Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 12 Mar 2021 17:57:38 +0000 Subject: [PATCH 13/28] reject duplicate transformation Signed-off-by: chaoqin-li1123 --- source/common/http/BUILD | 2 +- source/common/http/path_utility.cc | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 3062a3c37725..b26067b7a7b0 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -441,8 +441,8 @@ envoy_cc_library( ":legacy_path_canonicalizer", "//include/envoy/http:header_map_interface", "//source/common/common:logger_lib", - "//source/common/runtime:runtime_features_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/path_utility.cc b/source/common/http/path_utility.cc index a639a68172b4..4fc5d36303ae 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" @@ -7,7 +9,6 @@ #include "absl/strings/str_join.h" #include "absl/strings/str_split.h" #include "absl/types/optional.h" -#include "envoy/common/exception.h" #include "url/url_canon.h" #include "url/url_canon_stdstring.h" @@ -114,10 +115,11 @@ absl::optional PathTransformer::rfcNormalize(absl::string_view orig PathTransformer::PathTransformer(envoy::type::http::v3::PathTransformation path_transformation) { const auto& operations = path_transformation.operations(); - std::vector operation_hashes; + 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()) { + if (find(operation_hashes.begin(), operation_hashes.end(), operation_hash) != + operation_hashes.end()) { throw EnvoyException("Duplicate path transformation"); } if (operation.has_normalize_path_rfc_3986()) { From e9bbff20e35ca5bf61bdd5cfe9994b7573a4a0c6 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 12 Mar 2021 23:10:03 +0000 Subject: [PATCH 14/28] more refactor Signed-off-by: chaoqin-li1123 --- source/common/http/path_utility.cc | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index 4fc5d36303ae..809629ad456f 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -37,26 +37,11 @@ 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 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()) { + absl::optional normalized_path = PathTransformer::rfcNormalize(original_path); + if (!normalized_path.has_value()) { return false; } - auto& normalized_path = normalized_path_opt.value(); - const absl::string_view query_suffix = - query_pos == original_path.npos - ? absl::string_view{} - : absl::string_view{original_path.data() + query_pos, original_path.size() - query_pos}; - if (!query_suffix.empty()) { - normalized_path.insert(normalized_path.end(), query_suffix.begin(), query_suffix.end()); - } - headers.setPath(normalized_path); + headers.setPath(normalized_path.value()); return true; } From 91aff85d59c728205b43bd61a26b3654d97c917f Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 15 Mar 2021 04:44:07 +0000 Subject: [PATCH 15/28] test for path transformer Signed-off-by: chaoqin-li1123 --- source/common/http/path_utility.cc | 3 +- source/common/http/path_utility.h | 2 +- test/common/http/path_utility_test.cc | 40 +++++++++++++++++++++++++-- 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index 809629ad456f..42c407cc01eb 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -112,10 +112,11 @@ PathTransformer::PathTransformer(envoy::type::http::v3::PathTransformation path_ } else if (operation.has_merge_slashes()) { transformations_.emplace_back(PathTransformer::mergeSlashes); } + operation_hashes.push_back(operation_hash); } } -absl::optional PathTransformer::transform(absl::string_view original) { +absl::optional PathTransformer::transform(absl::string_view original) const { absl::optional path_string; absl::string_view path_string_view = original; for (Transformation const& transformation : transformations_) { diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index 9dd63642a982..d6452624a71b 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -36,7 +36,7 @@ class PathTransformer { public: PathTransformer(envoy::type::http::v3::PathTransformation operations); - absl::optional transform(const absl::string_view original_path); + absl::optional transform(const absl::string_view original_path) const; static absl::optional mergeSlashes(absl::string_view original_path); diff --git a/test/common/http/path_utility_test.cc b/test/common/http/path_utility_test.cc index 1d4b5222ba07..684610016fea 100644 --- a/test/common/http/path_utility_test.cc +++ b/test/common/http/path_utility_test.cc @@ -90,7 +90,7 @@ TEST_F(PathUtilityTest, NormalizeValidPaths) { } // Already normalized path don't change. -TEST(PathTransformerTest, NormalizeValidPaths) { +TEST(PathTransformationTest, NormalizeValidPaths) { const std::vector> non_normal_pairs{ {"/a/b/../c", "/a/c"}, // parent dir @@ -134,7 +134,7 @@ TEST_F(PathUtilityTest, NormalizeCasePath) { } // Paths that are valid get normalized. -TEST(PathTransformerTest, NormalizeCasePath) { +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 . @@ -176,7 +176,7 @@ TEST_F(PathUtilityTest, MergeSlashes) { EXPECT_EQ("/a/?b", mergeSlashes("//a/?b")); // ends with slash + query } -TEST(PathTransformerTest, MergeSlashes) { +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 @@ -215,5 +215,39 @@ 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_.reset(new PathTransformer(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 +} + +TEST_F(PathTransformerTest, DuplicateTransformation) { + EXPECT_THROW(setPathTransformer({"MergeSlashes", "MergeSlashes"}), EnvoyException); +} + } // namespace Http } // namespace Envoy From 044d8204fd5d3b6908450a33582245dcef3c1bc0 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 15 Mar 2021 16:42:50 +0000 Subject: [PATCH 16/28] store paths in header map, more test Signed-off-by: chaoqin-li1123 --- include/envoy/http/header_map.h | 4 ++ source/common/http/conn_manager_config.h | 2 + source/common/http/conn_manager_utility.cc | 1 + source/common/http/header_map_impl.h | 8 +++- source/common/http/path_utility.cc | 5 ++- .../network/http_connection_manager/config.h | 15 +++++++ test/common/http/path_utility_test.cc | 41 ++++++++++++++++++- test/test_common/utility.h | 9 ++++ 8 files changed, 81 insertions(+), 4 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 6e21463416a6..13a55f7376e9 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -782,6 +782,10 @@ class RequestHeaderMap public: INLINE_REQ_STRING_HEADERS(DEFINE_INLINE_STRING_HEADER) INLINE_REQ_NUMERIC_HEADERS(DEFINE_INLINE_NUMERIC_HEADER) + virtual void setForwaringPath(absl::string_view path) PURE; + virtual absl::string_view getForwardingPath() PURE; + virtual void setFilterPath(absl::string_view path) PURE; + virtual absl::string_view getFilterPath() PURE; }; using RequestHeaderMapPtr = std::unique_ptr; using RequestHeaderMapOptRef = OptRef; diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index 1d77ff43474a..09538e7b68ea 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -466,6 +466,8 @@ class ConnectionManagerConfig { * @return LocalReply configuration which supplies mapping for local reply generated by Envoy. */ virtual const LocalReply::LocalReply& localReply() const PURE; + + virtual void normalizePath(Http::RequestHeaderMap&) const {} }; } // namespace Http } // namespace Envoy diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 0b2b8a82149f..0b600f15528b 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -440,6 +440,7 @@ bool ConnectionManagerUtility::maybeNormalizePath(RequestHeaderMap& request_head return true; // It's as valid as it is going to get. } bool is_valid_path = true; + config.normalizePath(request_headers); if (config.shouldNormalizePath()) { is_valid_path = PathUtil::canonicalPath(request_headers); } diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 1a87b2c4df71..6e00446a27af 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -462,6 +462,11 @@ class RequestHeaderMapImpl final : public TypedHeaderMapImpl, INLINE_REQ_RESP_STRING_HEADERS(DEFINE_INLINE_HEADER_STRING_FUNCS) INLINE_REQ_RESP_NUMERIC_HEADERS(DEFINE_INLINE_HEADER_NUMERIC_FUNCS) + void setForwaringPath(absl::string_view path) override { forwarding_path_ = path; } + absl::string_view getForwardingPath() override { return forwarding_path_; } + void setFilterPath(absl::string_view path) override { filter_path_ = 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 // most derived class. This forces the definition of the following three functions to also be @@ -480,7 +485,8 @@ class RequestHeaderMapImpl final : public TypedHeaderMapImpl, }; using HeaderHandles = ConstSingleton; - + std::string forwarding_path_; + std::string filter_path_; RequestHeaderMapImpl() { clearInline(); } HeaderEntryImpl* inline_headers_[]; diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index 42c407cc01eb..954bee93db97 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -37,7 +37,7 @@ absl::optional canonicalizePath(absl::string_view original_path) { bool PathUtil::canonicalPath(RequestHeaderMap& headers) { ASSERT(headers.Path()); const auto original_path = headers.getPathValue(); - absl::optional normalized_path = PathTransformer::rfcNormalize(original_path); + const absl::optional normalized_path = PathTransformer::rfcNormalize(original_path); if (!normalized_path.has_value()) { return false; } @@ -48,7 +48,8 @@ bool PathUtil::canonicalPath(RequestHeaderMap& headers) { void PathUtil::mergeSlashes(RequestHeaderMap& headers) { ASSERT(headers.Path()); const auto original_path = headers.getPathValue(); - absl::optional normalized = PathTransformer::mergeSlashes(original_path).value(); + const absl::optional normalized = + PathTransformer::mergeSlashes(original_path).value(); if (normalized.has_value()) { headers.setPath(normalized.value()); } diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 834a446e4386..c828a8f3ce7d 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -180,6 +180,21 @@ class HttpConnectionManagerConfig : Logger::Loggable, } std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } const LocalReply::LocalReply& localReply() const override { return *local_reply_; } + void normalizePath(Http::RequestHeaderMap& request_headers) const override { + const auto original_path = request_headers.getPathValue(); + absl::optional forwarding_path = + forwarding_path_transformer_.transform(original_path); + absl::optional filter_path; + if (forwarding_path.has_value()) { + filter_path = filter_path_transformer_.transform(forwarding_path.value()); + } + if (forwarding_path.has_value()) { + request_headers.setForwaringPath(forwarding_path.value()); + } + if (filter_path.has_value()) { + request_headers.setFilterPath(filter_path.value()); + } + } private: enum class CodecType { HTTP1, HTTP2, HTTP3, AUTO }; diff --git a/test/common/http/path_utility_test.cc b/test/common/http/path_utility_test.cc index 684610016fea..794e718284cc 100644 --- a/test/common/http/path_utility_test.cc +++ b/test/common/http/path_utility_test.cc @@ -229,7 +229,7 @@ class PathTransformerTest : public testing::Test { operation->mutable_merge_slashes(); } } - path_transformer_.reset(new PathTransformer(path_transformation_config)); + path_transformer_ = std::make_unique(path_transformation_config); } const PathTransformer& pathTransformer() { return *path_transformer_; } @@ -243,10 +243,49 @@ TEST_F(PathTransformerTest, MergeSlashes) { 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 } 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 diff --git a/test/test_common/utility.h b/test/test_common/utility.h index fd8f25ce68d9..a849c63b0c96 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -1029,6 +1029,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) + + void setForwaringPath(absl::string_view path) override { forwarding_path_ = path; } + absl::string_view getForwardingPath() override { return forwarding_path_; } + void setFilterPath(absl::string_view path) override { filter_path_ = path; } + absl::string_view getFilterPath() override { return filter_path_; } + +private: + std::string forwarding_path_; + std::string filter_path_; }; using TestRequestTrailerMapImpl = TestHeaderMapImplBase; From 996b786bde6937def46b502edc29a3a28ebec4ec Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Tue, 16 Mar 2021 16:09:47 +0000 Subject: [PATCH 17/28] add path restoration Signed-off-by: chaoqin-li1123 --- source/common/http/conn_manager_config.h | 2 +- source/common/http/path_utility.h | 3 +-- source/common/router/config_impl.cc | 5 +++++ source/server/admin/admin.h | 1 + test/common/http/conn_manager_impl_fuzz_test.cc | 1 + test/common/http/conn_manager_impl_test_base.h | 1 + test/common/http/path_utility_test.cc | 4 ++++ test/mocks/http/mocks.h | 1 + 8 files changed, 15 insertions(+), 3 deletions(-) diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index 09538e7b68ea..9db47fd1f381 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -467,7 +467,7 @@ class ConnectionManagerConfig { */ virtual const LocalReply::LocalReply& localReply() const PURE; - virtual void normalizePath(Http::RequestHeaderMap&) const {} + virtual void normalizePath(Http::RequestHeaderMap&) const PURE; }; } // namespace Http } // namespace Envoy diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index d6452624a71b..7cd28cc01c6f 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -30,8 +30,6 @@ class PathUtil { static absl::string_view removeQueryAndFragment(const absl::string_view path); }; -using Transformation = std::function(absl::string_view)>; - class PathTransformer { public: PathTransformer(envoy::type::http::v3::PathTransformation operations); @@ -43,6 +41,7 @@ class PathTransformer { static absl::optional rfcNormalize(absl::string_view original_path); private: + using Transformation = std::function(absl::string_view)>; std::list transformations_; }; diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 0c9e896ee740..57d817af57da 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -557,6 +557,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.getPathValue())); + } + if (!host_rewrite_.empty()) { headers.setHost(host_rewrite_); } else if (auto_host_rewrite_header_) { diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index 69d5942b1a41..f1647f6f06ff 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -194,6 +194,7 @@ class AdminImpl : public Admin, return runCallback(path_and_query, response_headers, response, filter); }; } + void normalizePath(Http::RequestHeaderMap&) const override {} private: /** diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index ed9f08f86a16..200c114f31ca 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -198,6 +198,7 @@ class FuzzConfig : public ConnectionManagerConfig { const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return false; } bool shouldMergeSlashes() const override { return false; } + void normalizePath(Http::RequestHeaderMap&) const override {} Http::StripPortType stripPortType() const override { return Http::StripPortType::None; } envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headersWithUnderscoresAction() const override { diff --git a/test/common/http/conn_manager_impl_test_base.h b/test/common/http/conn_manager_impl_test_base.h index 725415183f94..42300c551048 100644 --- a/test/common/http/conn_manager_impl_test_base.h +++ b/test/common/http/conn_manager_impl_test_base.h @@ -133,6 +133,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return normalize_path_; } bool shouldMergeSlashes() const override { return merge_slashes_; } + void normalizePath(Http::RequestHeaderMap&) const override {} Http::StripPortType stripPortType() const override { return strip_port_type_; } const RequestIDExtensionSharedPtr& requestIDExtension() override { return request_id_extension_; } envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction diff --git a/test/common/http/path_utility_test.cc b/test/common/http/path_utility_test.cc index 794e718284cc..7e1eff1c959a 100644 --- a/test/common/http/path_utility_test.cc +++ b/test/common/http/path_utility_test.cc @@ -277,6 +277,10 @@ TEST_F(PathTransformerTest, RfcNormalize) { 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) { diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 92bf33b678fd..d311bf53aa36 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -566,6 +566,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD(const Http::Http1Settings&, http1Settings, (), (const)); MOCK_METHOD(bool, shouldNormalizePath, (), (const)); MOCK_METHOD(bool, shouldMergeSlashes, (), (const)); + MOCK_METHOD(void, normalizePath, (Http::RequestHeaderMap&), (const)); MOCK_METHOD(Http::StripPortType, stripPortType, (), (const)); MOCK_METHOD(envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction, headersWithUnderscoresAction, (), (const)); From 95137dafed646ddef6e971de501b2c5914d11346 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 17 Mar 2021 00:29:23 +0000 Subject: [PATCH 18/28] diaable old api when new api exist Signed-off-by: chaoqin-li1123 --- source/common/http/conn_manager_utility.cc | 3 +++ .../network/http_connection_manager/config.cc | 16 +++++++++++++--- .../network/http_connection_manager/config.h | 15 +++++++++------ 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 04d0b243acbc..f446b47f3c91 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -440,6 +440,9 @@ bool ConnectionManagerUtility::maybeNormalizePath(RequestHeaderMap& request_head } bool is_valid_path = true; config.normalizePath(request_headers); + if (!config.shouldNormalizePath() && !config.shouldMergeSlashes()) { + return true; + } if (config.shouldNormalizePath()) { is_valid_path = PathUtil::canonicalPath(request_headers); } diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index cfa5aad0d191..ad6c37e28d34 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -27,6 +27,7 @@ #include "common/http/http2/codec_impl.h" #include "common/http/http3/quic_codec_factory.h" #include "common/http/http3/well_known_names.h" +#include "common/http/path_utility.h" #include "common/http/request_id_extension_impl.h" #include "common/http/utility.h" #include "common/local_reply/local_reply.h" @@ -253,9 +254,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( merge_slashes_(config.merge_slashes()), headers_with_underscores_action_( config.common_http_protocol_options().headers_with_underscores_action()), - local_reply_(LocalReply::Factory::create(config.local_reply_config(), context)), - forwarding_path_transformer_(config.path_normalization_options().forwarding_transformation()), - filter_path_transformer_(config.path_normalization_options().http_filter_transformation()) { + local_reply_(LocalReply::Factory::create(config.local_reply_config(), context)) { // If idle_timeout_ was not configured in common_http_protocol_options, use value in deprecated // idle_timeout field. // TODO(asraa): Remove when idle_timeout is removed. @@ -268,6 +267,17 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( idle_timeout_ = absl::nullopt; } + // If the path normalization options has been set. + if (config.has_path_normalization_options()) { + ASSERT(false); + normalize_path_ = false; + merge_slashes_ = false; + forwarding_path_transformer_ = std::make_unique( + config.path_normalization_options().forwarding_transformation()); + filter_path_transformer_ = std::make_unique( + config.path_normalization_options().http_filter_transformation()); + } + 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 c828a8f3ce7d..55c9ffa78495 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -181,12 +181,15 @@ class HttpConnectionManagerConfig : Logger::Loggable, std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } const LocalReply::LocalReply& localReply() const override { return *local_reply_; } void normalizePath(Http::RequestHeaderMap& request_headers) const override { + if (forwarding_path_transformer_ == nullptr) { + return; + } const auto original_path = request_headers.getPathValue(); absl::optional forwarding_path = - forwarding_path_transformer_.transform(original_path); + forwarding_path_transformer_->transform(original_path); absl::optional filter_path; if (forwarding_path.has_value()) { - filter_path = filter_path_transformer_.transform(forwarding_path.value()); + filter_path = filter_path_transformer_->transform(forwarding_path.value()); } if (forwarding_path.has_value()) { request_headers.setForwaringPath(forwarding_path.value()); @@ -267,8 +270,8 @@ class HttpConnectionManagerConfig : Logger::Loggable, const bool proxy_100_continue_; const bool stream_error_on_invalid_http_messaging_; std::chrono::milliseconds delayed_close_timeout_; - const bool normalize_path_; - const bool merge_slashes_; + bool normalize_path_; + bool merge_slashes_; Http::StripPortType strip_port_type_; const envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action_; @@ -280,8 +283,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_; + std::unique_ptr forwarding_path_transformer_; + std::unique_ptr filter_path_transformer_; }; /** From 184359c2806e4d845dbf6cb4f85a14421802b333 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 17 Mar 2021 15:53:25 +0000 Subject: [PATCH 19/28] comments for path transformer class Signed-off-by: chaoqin-li1123 --- source/common/http/path_utility.cc | 3 ++- source/common/http/path_utility.h | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index 954bee93db97..04993561a910 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -99,7 +99,8 @@ absl::optional PathTransformer::rfcNormalize(absl::string_view orig return normalized_path; } -PathTransformer::PathTransformer(envoy::type::http::v3::PathTransformation path_transformation) { +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) { diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index 7cd28cc01c6f..68e6c279609c 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -32,8 +32,10 @@ class PathUtil { class PathTransformer { public: - PathTransformer(envoy::type::http::v3::PathTransformation operations); + 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); @@ -42,6 +44,8 @@ class PathTransformer { 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_; }; From c2810d94ade393e224c09a77a4a18554d728e18e Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 18 Mar 2021 00:57:46 +0000 Subject: [PATCH 20/28] fix typo Signed-off-by: chaoqin-li1123 --- include/envoy/http/header_map.h | 2 +- source/common/http/header_map_impl.h | 2 +- .../extensions/filters/network/http_connection_manager/config.h | 2 +- test/test_common/utility.h | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 8a3409d754ba..c6574f54cfcd 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -807,7 +807,7 @@ class RequestHeaderMap public: INLINE_REQ_STRING_HEADERS(DEFINE_INLINE_STRING_HEADER) INLINE_REQ_NUMERIC_HEADERS(DEFINE_INLINE_NUMERIC_HEADER) - virtual void setForwaringPath(absl::string_view path) PURE; + virtual void setForwardingPath(absl::string_view path) PURE; virtual absl::string_view getForwardingPath() PURE; virtual void setFilterPath(absl::string_view path) PURE; virtual absl::string_view getFilterPath() PURE; diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 6e00446a27af..7e672411bf40 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -462,7 +462,7 @@ class RequestHeaderMapImpl final : public TypedHeaderMapImpl, INLINE_REQ_RESP_STRING_HEADERS(DEFINE_INLINE_HEADER_STRING_FUNCS) INLINE_REQ_RESP_NUMERIC_HEADERS(DEFINE_INLINE_HEADER_NUMERIC_FUNCS) - void setForwaringPath(absl::string_view path) override { forwarding_path_ = path; } + void setForwardingPath(absl::string_view path) override { forwarding_path_ = path; } absl::string_view getForwardingPath() override { return forwarding_path_; } void setFilterPath(absl::string_view path) override { filter_path_ = path; } absl::string_view getFilterPath() override { return filter_path_; } diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 55c9ffa78495..b5a99e14cb2c 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -192,7 +192,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, filter_path = filter_path_transformer_->transform(forwarding_path.value()); } if (forwarding_path.has_value()) { - request_headers.setForwaringPath(forwarding_path.value()); + request_headers.setForwardingPath(forwarding_path.value()); } if (filter_path.has_value()) { request_headers.setFilterPath(filter_path.value()); diff --git a/test/test_common/utility.h b/test/test_common/utility.h index a849c63b0c96..e4caa0c83e9b 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -1030,7 +1030,7 @@ class TestRequestHeaderMapImpl INLINE_REQ_RESP_STRING_HEADERS(DEFINE_TEST_INLINE_STRING_HEADER_FUNCS) INLINE_REQ_RESP_NUMERIC_HEADERS(DEFINE_TEST_INLINE_NUMERIC_HEADER_FUNCS) - void setForwaringPath(absl::string_view path) override { forwarding_path_ = path; } + void setForwardingPath(absl::string_view path) override { forwarding_path_ = path; } absl::string_view getForwardingPath() override { return forwarding_path_; } void setFilterPath(absl::string_view path) override { filter_path_ = path; } absl::string_view getFilterPath() override { return filter_path_; } From 3d37a0fec40f20497ca0bcff8c7eaceb8b1760f1 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 18 Mar 2021 16:44:12 +0000 Subject: [PATCH 21/28] change set and get filter path method to be private Signed-off-by: chaoqin-li1123 --- include/envoy/http/header_map.h | 1 + source/common/http/header_map_impl.h | 13 ++++++------- source/common/http/path_utility.cc | 2 ++ .../network/http_connection_manager/config.h | 5 ++--- test/test_common/utility.h | 3 +-- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index c6574f54cfcd..ff4e728faca3 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -807,6 +807,7 @@ class RequestHeaderMap public: INLINE_REQ_STRING_HEADERS(DEFINE_INLINE_STRING_HEADER) INLINE_REQ_NUMERIC_HEADERS(DEFINE_INLINE_NUMERIC_HEADER) +private: virtual void setForwardingPath(absl::string_view path) PURE; virtual absl::string_view getForwardingPath() PURE; virtual void setFilterPath(absl::string_view path) PURE; diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 7e672411bf40..35cadd25ded1 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -462,11 +462,6 @@ class RequestHeaderMapImpl final : public TypedHeaderMapImpl, INLINE_REQ_RESP_STRING_HEADERS(DEFINE_INLINE_HEADER_STRING_FUNCS) INLINE_REQ_RESP_NUMERIC_HEADERS(DEFINE_INLINE_HEADER_NUMERIC_FUNCS) - void setForwardingPath(absl::string_view path) override { forwarding_path_ = path; } - absl::string_view getForwardingPath() override { return forwarding_path_; } - void setFilterPath(absl::string_view path) override { filter_path_ = 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 // most derived class. This forces the definition of the following three functions to also be @@ -483,10 +478,14 @@ class RequestHeaderMapImpl final : public TypedHeaderMapImpl, INLINE_REQ_RESP_STRING_HEADERS(DEFINE_HEADER_HANDLE) INLINE_REQ_RESP_NUMERIC_HEADERS(DEFINE_HEADER_HANDLE) }; - - using HeaderHandles = ConstSingleton; + void setForwardingPath(absl::string_view path) override { forwarding_path_ = path; } + absl::string_view getForwardingPath() override { return forwarding_path_; } + void setFilterPath(absl::string_view path) override { filter_path_ = path; } + absl::string_view getFilterPath() override { return filter_path_; } std::string forwarding_path_; std::string filter_path_; + friend class HttpConnectionManagerConfig; + 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 04993561a910..f09fc49f369e 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -107,6 +107,8 @@ PathTransformer::PathTransformer( 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()) { diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index b5a99e14cb2c..4e392f82ecfb 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -188,13 +188,12 @@ class HttpConnectionManagerConfig : Logger::Loggable, absl::optional forwarding_path = forwarding_path_transformer_->transform(original_path); absl::optional filter_path; - if (forwarding_path.has_value()) { - filter_path = filter_path_transformer_->transform(forwarding_path.value()); - } if (forwarding_path.has_value()) { request_headers.setForwardingPath(forwarding_path.value()); + filter_path = filter_path_transformer_->transform(forwarding_path.value()); } if (filter_path.has_value()) { + request_headers.setPath(filter_path.value()); request_headers.setFilterPath(filter_path.value()); } } diff --git a/test/test_common/utility.h b/test/test_common/utility.h index e4caa0c83e9b..4c775a2cc4a4 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -1029,13 +1029,12 @@ 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) - +private: void setForwardingPath(absl::string_view path) override { forwarding_path_ = path; } absl::string_view getForwardingPath() override { return forwarding_path_; } void setFilterPath(absl::string_view path) override { filter_path_ = path; } absl::string_view getFilterPath() override { return filter_path_; } -private: std::string forwarding_path_; std::string filter_path_; }; From e24a7e7b77dd53c4fc5dca023057ee88e2cf4557 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 19 Mar 2021 18:41:02 +0000 Subject: [PATCH 22/28] make set filter path and set forwarding path private Signed-off-by: chaoqin-li1123 --- include/envoy/http/header_map.h | 16 +++++++++++++--- source/common/http/header_map_impl.h | 5 ++--- test/test_common/utility.h | 5 +++-- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index ff4e728faca3..d0b1e5daf7f9 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -19,8 +19,15 @@ #include "absl/strings/string_view.h" namespace Envoy { -namespace Http { +namespace Extensions { +namespace NetworkFilters { +namespace HttpConnectionManager { +class HttpConnectionManagerConfig; +} +} // namespace NetworkFilters +} // namespace Extensions +namespace Http { // 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) { @@ -807,11 +814,14 @@ 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 Envoy::Extensions::NetworkFilters::HttpConnectionManager:: + HttpConnectionManagerConfig; virtual void setForwardingPath(absl::string_view path) PURE; - virtual absl::string_view getForwardingPath() PURE; virtual void setFilterPath(absl::string_view path) PURE; - virtual absl::string_view getFilterPath() PURE; }; using RequestHeaderMapPtr = std::unique_ptr; using RequestHeaderMapOptRef = OptRef; diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 35cadd25ded1..437d73acabe9 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -461,6 +461,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 @@ -479,12 +481,9 @@ class RequestHeaderMapImpl final : public TypedHeaderMapImpl, INLINE_REQ_RESP_NUMERIC_HEADERS(DEFINE_HEADER_HANDLE) }; void setForwardingPath(absl::string_view path) override { forwarding_path_ = path; } - absl::string_view getForwardingPath() override { return forwarding_path_; } void setFilterPath(absl::string_view path) override { filter_path_ = path; } - absl::string_view getFilterPath() override { return filter_path_; } std::string forwarding_path_; std::string filter_path_; - friend class HttpConnectionManagerConfig; using HeaderHandles = ConstSingleton; RequestHeaderMapImpl() { clearInline(); } diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 4c775a2cc4a4..86a28ec3112d 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -1029,11 +1029,12 @@ 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; } - absl::string_view getForwardingPath() override { return forwarding_path_; } void setFilterPath(absl::string_view path) override { filter_path_ = path; } - absl::string_view getFilterPath() override { return filter_path_; } std::string forwarding_path_; std::string filter_path_; From 50a227e1b8036020cae2e5b2f290d9b75d267e33 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Sun, 21 Mar 2021 05:09:52 +0000 Subject: [PATCH 23/28] add test for hcm config Signed-off-by: chaoqin-li1123 --- source/common/http/path_utility.cc | 2 +- .../network/http_connection_manager/config.cc | 1 - .../http_connection_manager/config_test.cc | 117 ++++++++++++++++++ 3 files changed, 118 insertions(+), 2 deletions(-) diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index f09fc49f369e..577eb18f2c71 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -121,7 +121,7 @@ PathTransformer::PathTransformer( } absl::optional PathTransformer::transform(absl::string_view original) const { - absl::optional path_string; + 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); diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index ad6c37e28d34..2a3172a6d636 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -269,7 +269,6 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( // If the path normalization options has been set. if (config.has_path_normalization_options()) { - ASSERT(false); normalize_path_ = false; merge_slashes_ = false; forwarding_path_transformer_ = std::make_unique( 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 7db4e20d4e8c..9135ad9275e4 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" @@ -962,6 +963,122 @@ TEST_F(HttpConnectionManagerConfigTest, MergeSlashesDefault) { EXPECT_FALSE(config.shouldMergeSlashes()); } +// Validated that old api is ignored when new api is configured. +TEST_F(HttpConnectionManagerConfigTest, IgnoreOldApiWhenNewApiConfigured) { + 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: {} + 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_); + EXPECT_FALSE(config.shouldMergeSlashes()); + EXPECT_FALSE(config.shouldNormalizePath()); +} + +// 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_); + EXPECT_FALSE(config.shouldMergeSlashes()); + EXPECT_FALSE(config.shouldNormalizePath()); + Http::TestRequestHeaderMapImpl header_map{ + {":method", "GET"}, {":path", "/foo//bar"}, {":authority", "host"}, {":scheme", "http"}}; + config.normalizePath(header_map); + 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_); + EXPECT_FALSE(config.shouldMergeSlashes()); + EXPECT_FALSE(config.shouldNormalizePath()); + Http::TestRequestHeaderMapImpl header_map{ + {":method", "GET"}, {":path", "/foo//bar"}, {":authority", "host"}, {":scheme", "http"}}; + config.normalizePath(header_map); + 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( From e9a483dcc698f83a614453a41b3016cc24e52797 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Tue, 23 Mar 2021 16:50:51 +0000 Subject: [PATCH 24/28] add header integration tests Signed-off-by: chaoqin-li1123 --- source/common/router/config_impl.cc | 2 +- test/integration/header_integration_test.cc | 147 +++++++++++++++++++- 2 files changed, 146 insertions(+), 3 deletions(-) diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 0aa260441306..c5327c9fbdc6 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -569,7 +569,7 @@ void RouteEntryImplBase::finalizeRequestHeaders(Http::RequestHeaderMap& headers, // Restore the forwarding path if none of the filter mutate the path. if (headers.Path() && headers.getFilterPath() == headers.getPathValue()) { - headers.setPath(std::string(headers.getPathValue())); + headers.setPath(std::string(headers.getForwardingPath())); } if (!host_rewrite_.empty()) { diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index d10b64b2b7df..2a502593360c 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,133 @@ 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), + "rejected_counter == nullptr || rejected_counter->value() == 0. Details: Lds update failed."); +} + // Validates behavior when normalize path is on. // Path to decide route and path to upstream are both // the normalized. From b40bebb202d0b6b93ab5fdcf67823c357e6e975f Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Tue, 23 Mar 2021 19:22:04 +0000 Subject: [PATCH 25/28] fix header integration tests Signed-off-by: chaoqin-li1123 --- test/integration/header_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index 2a502593360c..d233cec640f6 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -1195,7 +1195,7 @@ TEST_P(HeaderIntegrationTest, TestDuplicateOperationInPathTransformation) { )EOF"); EXPECT_DEATH( initializeFilter(HeaderMode::Append, false), - "rejected_counter == nullptr || rejected_counter->value() == 0. Details: Lds update failed."); + "Details: Lds update failed."); } // Validates behavior when normalize path is on. From 0d7f899950e82333d5a27c4949ed7c2ab3cb8c80 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Tue, 23 Mar 2021 19:24:49 +0000 Subject: [PATCH 26/28] fix format Signed-off-by: chaoqin-li1123 --- test/integration/header_integration_test.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index d233cec640f6..8e002e8b6fe7 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -1193,9 +1193,7 @@ TEST_P(HeaderIntegrationTest, TestDuplicateOperationInPathTransformation) { R"EOF( operations: )EOF"); - EXPECT_DEATH( - initializeFilter(HeaderMode::Append, false), - "Details: Lds update failed."); + EXPECT_DEATH(initializeFilter(HeaderMode::Append, false), "Details: Lds update failed."); } // Validates behavior when normalize path is on. From ee8b9a33bf1f22b28614332cd8a33e66b69fc8ac Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 26 Mar 2021 04:09:21 +0000 Subject: [PATCH 27/28] clean up code Signed-off-by: chaoqin-li1123 --- include/envoy/http/header_map.h | 12 +---- source/common/http/conn_manager_config.h | 5 +- source/common/http/conn_manager_utility.cc | 25 +++++---- source/common/http/path_utility.cc | 10 ++++ source/common/http/path_utility.h | 2 + .../network/http_connection_manager/config.cc | 6 ++- .../network/http_connection_manager/config.h | 25 +++------ source/server/admin/admin.h | 1 - .../http/conn_manager_impl_fuzz_test.cc | 1 - test/common/http/conn_manager_impl_test.cc | 9 ++-- .../common/http/conn_manager_impl_test_base.h | 10 +++- test/common/http/conn_manager_utility_test.cc | 53 +++++++++++++++---- .../network/http_connection_manager/BUILD | 1 + .../http_connection_manager/config_test.cc | 37 ++----------- test/mocks/http/mocks.h | 3 +- 15 files changed, 109 insertions(+), 91 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index d0b1e5daf7f9..2be06f9ced80 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -19,15 +19,8 @@ #include "absl/strings/string_view.h" namespace Envoy { -namespace Extensions { -namespace NetworkFilters { -namespace HttpConnectionManager { - -class HttpConnectionManagerConfig; -} -} // namespace NetworkFilters -} // namespace Extensions 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) { @@ -818,8 +811,7 @@ class RequestHeaderMap virtual absl::string_view getFilterPath() PURE; private: - friend class Envoy::Extensions::NetworkFilters::HttpConnectionManager:: - HttpConnectionManagerConfig; + friend class ConnectionManagerUtility; virtual void setForwardingPath(absl::string_view path) PURE; virtual void setFilterPath(absl::string_view path) PURE; }; diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index 9db47fd1f381..9d89eb37a78d 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" @@ -467,7 +468,9 @@ class ConnectionManagerConfig { */ virtual const LocalReply::LocalReply& localReply() const PURE; - virtual void normalizePath(Http::RequestHeaderMap&) const PURE; + virtual PathTransformer* forwardingPathTransformer() const { return nullptr; } + + virtual PathTransformer* filterPathTransformer() const { return nullptr; } }; } // namespace Http } // namespace Envoy diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index f446b47f3c91..8f09dc77ed5d 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -435,20 +435,25 @@ void ConnectionManagerUtility::mutateResponseHeaders(ResponseHeaderMap& response bool ConnectionManagerUtility::maybeNormalizePath(RequestHeaderMap& request_headers, const ConnectionManagerConfig& config) { - if (!request_headers.Path()) { + if (!request_headers.Path() || !config.forwardingPathTransformer()) { return true; // It's as valid as it is going to get. } bool is_valid_path = true; - config.normalizePath(request_headers); - if (!config.shouldNormalizePath() && !config.shouldMergeSlashes()) { - return 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/path_utility.cc b/source/common/http/path_utility.cc index 577eb18f2c71..c30f6b95c49a 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -99,6 +99,16 @@ absl::optional PathTransformer::rfcNormalize(absl::string_view orig return normalized_path; } +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); + } +} + PathTransformer::PathTransformer( envoy::type::http::v3::PathTransformation const& path_transformation) { const auto& operations = path_transformation.operations(); diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index 68e6c279609c..b4b1b17ab725 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -32,6 +32,8 @@ class PathUtil { 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. diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index aaf7132515ff..ed19bc3d6ff7 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -269,12 +269,14 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( // If the path normalization options has been set. if (config.has_path_normalization_options()) { - normalize_path_ = false; - merge_slashes_ = false; forwarding_path_transformer_ = std::make_unique( config.path_normalization_options().forwarding_transformation()); filter_path_transformer_ = std::make_unique( config.path_normalization_options().http_filter_transformation()); + } else { + forwarding_path_transformer_ = + std::make_unique(normalize_path_, merge_slashes_); + filter_path_transformer_ = std::make_unique(); } if (config.strip_any_host_port() && config.strip_matching_host_port()) { diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 4e392f82ecfb..e146c5d1a479 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -180,22 +180,11 @@ class HttpConnectionManagerConfig : Logger::Loggable, } std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } const LocalReply::LocalReply& localReply() const override { return *local_reply_; } - void normalizePath(Http::RequestHeaderMap& request_headers) const override { - if (forwarding_path_transformer_ == nullptr) { - return; - } - const auto original_path = request_headers.getPathValue(); - absl::optional forwarding_path = - forwarding_path_transformer_->transform(original_path); - absl::optional filter_path; - if (forwarding_path.has_value()) { - request_headers.setForwardingPath(forwarding_path.value()); - filter_path = filter_path_transformer_->transform(forwarding_path.value()); - } - if (filter_path.has_value()) { - request_headers.setPath(filter_path.value()); - request_headers.setFilterPath(filter_path.value()); - } + Http::PathTransformer* filterPathTransformer() const override { + return filter_path_transformer_.get(); + } + Http::PathTransformer* forwardingPathTransformer() const override { + return forwarding_path_transformer_.get(); } private: @@ -269,8 +258,8 @@ class HttpConnectionManagerConfig : Logger::Loggable, const bool proxy_100_continue_; const bool stream_error_on_invalid_http_messaging_; std::chrono::milliseconds delayed_close_timeout_; - bool normalize_path_; - bool merge_slashes_; + const bool normalize_path_; + const bool merge_slashes_; Http::StripPortType strip_port_type_; const envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action_; diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index 23c5d9cb9426..afe0fcf68d3c 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -194,7 +194,6 @@ class AdminImpl : public Admin, return runCallback(path_and_query, response_headers, response, filter); }; } - void normalizePath(Http::RequestHeaderMap&) const override {} private: /** diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 200c114f31ca..ed9f08f86a16 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -198,7 +198,6 @@ class FuzzConfig : public ConnectionManagerConfig { const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return false; } bool shouldMergeSlashes() const override { return false; } - void normalizePath(Http::RequestHeaderMap&) const override {} Http::StripPortType stripPortType() const override { return Http::StripPortType::None; } envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headersWithUnderscoresAction() const override { diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 5ebc6505f078..f54d9749c662 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_ = std::make_unique(true, false); + filter_path_transformer_ = std::make_unique(); 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_ = std::make_unique(true, false); + filter_path_transformer_ = std::make_unique(); 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_ = std::make_unique(true, false); + filter_path_transformer_ = std::make_unique(); 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 8b4248e71fa7..312d27245901 100644 --- a/test/common/http/conn_manager_impl_test_base.h +++ b/test/common/http/conn_manager_impl_test_base.h @@ -5,6 +5,8 @@ #include "common/http/date_provider_impl.h" #include "common/network/address_impl.h" +#include "source/common/http/_virtual_includes/path_utility_lib/common/http/path_utility.h" + #include "extensions/access_loggers/common/file_access_log_impl.h" #include "test/mocks/access_log/mocks.h" @@ -133,7 +135,6 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan const Http::Http1Settings& http1Settings() const override { return http1_settings_; } bool shouldNormalizePath() const override { return normalize_path_; } bool shouldMergeSlashes() const override { return merge_slashes_; } - void normalizePath(Http::RequestHeaderMap&) const override {} Http::StripPortType stripPortType() const override { return strip_port_type_; } const RequestIDExtensionSharedPtr& requestIDExtension() override { return request_id_extension_; } envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction @@ -141,7 +142,10 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan return headers_with_underscores_action_; } const LocalReply::LocalReply& localReply() const override { return *local_reply_; } - + PathTransformer* forwardingPathTransformer() const override { + return forwarding_path_transformer_.get(); + } + PathTransformer* filterPathTransformer() const override { return filter_path_transformer_.get(); } Envoy::Event::SimulatedTimeSystem test_time_; NiceMock route_config_provider_; std::shared_ptr route_config_{new NiceMock()}; @@ -213,6 +217,8 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan std::vector decoder_filters_; std::vector encoder_filters_; std::shared_ptr log_handler_; + std::unique_ptr forwarding_path_transformer_; + std::unique_ptr 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..bc9249314812 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(Return(forwarding_path_transformer.get())); + ON_CALL(config_, filterPathTransformer()).WillByDefault(Return(filter_path_transformer.get())); 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(Return(forwarding_path_transformer.get())); + ON_CALL(config_, filterPathTransformer()).WillByDefault(Return(filter_path_transformer.get())); 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(Return(forwarding_path_transformer.get())); + ON_CALL(config_, filterPathTransformer()).WillByDefault(Return(filter_path_transformer.get())); 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(Return(forwarding_path_transformer.get())); + ON_CALL(config_, filterPathTransformer()).WillByDefault(Return(filter_path_transformer.get())); 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(Return(forwarding_path_transformer.get())); + ON_CALL(config_, filterPathTransformer()).WillByDefault(Return(filter_path_transformer.get())); 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(Return(forwarding_path_transformer.get())); + ON_CALL(config_, filterPathTransformer()).WillByDefault(Return(filter_path_transformer.get())); 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(Return(forwarding_path_transformer.get())); + ON_CALL(config_, filterPathTransformer()).WillByDefault(Return(filter_path_transformer.get())); TestRequestHeaderMapImpl original_headers; original_headers.setPath("/xyz/..//abc"); 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 9135ad9275e4..5fc4f20c83ca 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -8,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" @@ -963,32 +964,6 @@ TEST_F(HttpConnectionManagerConfigTest, MergeSlashesDefault) { EXPECT_FALSE(config.shouldMergeSlashes()); } -// Validated that old api is ignored when new api is configured. -TEST_F(HttpConnectionManagerConfigTest, IgnoreOldApiWhenNewApiConfigured) { - 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: {} - 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_); - EXPECT_FALSE(config.shouldMergeSlashes()); - EXPECT_FALSE(config.shouldNormalizePath()); -} - // Validated that forwarding transformation is applied to both forwarding path and filter path. TEST_F(HttpConnectionManagerConfigTest, ForwadingTransformation) { const std::string yaml_string = R"EOF( @@ -1012,11 +987,9 @@ TEST_F(HttpConnectionManagerConfigTest, ForwadingTransformation) { date_provider_, route_config_provider_manager_, scoped_routes_config_provider_manager_, http_tracer_manager_, filter_config_provider_manager_); - EXPECT_FALSE(config.shouldMergeSlashes()); - EXPECT_FALSE(config.shouldNormalizePath()); Http::TestRequestHeaderMapImpl header_map{ {":method", "GET"}, {":path", "/foo//bar"}, {":authority", "host"}, {":scheme", "http"}}; - config.normalizePath(header_map); + Http::ConnectionManagerUtility::maybeNormalizePath(header_map, config); EXPECT_EQ(header_map.getForwardingPath(), "/foo/bar"); EXPECT_EQ(header_map.getFilterPath(), "/foo/bar"); } @@ -1044,11 +1017,9 @@ TEST_F(HttpConnectionManagerConfigTest, FilterTransformation) { date_provider_, route_config_provider_manager_, scoped_routes_config_provider_manager_, http_tracer_manager_, filter_config_provider_manager_); - EXPECT_FALSE(config.shouldMergeSlashes()); - EXPECT_FALSE(config.shouldNormalizePath()); Http::TestRequestHeaderMapImpl header_map{ {":method", "GET"}, {":path", "/foo//bar"}, {":authority", "host"}, {":scheme", "http"}}; - config.normalizePath(header_map); + Http::ConnectionManagerUtility::maybeNormalizePath(header_map, config); EXPECT_EQ(header_map.getForwardingPath(), "/foo//bar"); EXPECT_EQ(header_map.getFilterPath(), "/foo/bar"); } @@ -1062,7 +1033,7 @@ TEST_F(HttpConnectionManagerConfigTest, DuplicatePathTransformation) { path_normalization_options: forwarding_transformation: operations: - - normalize_path_rfc_3986: {}\ + - normalize_path_rfc_3986: {} - normalize_path_rfc_3986: {} http_filter_transformation: operations: diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index ae1d70deab55..ba17c30c0258 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -576,7 +576,8 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD(const Http::Http1Settings&, http1Settings, (), (const)); MOCK_METHOD(bool, shouldNormalizePath, (), (const)); MOCK_METHOD(bool, shouldMergeSlashes, (), (const)); - MOCK_METHOD(void, normalizePath, (Http::RequestHeaderMap&), (const)); + MOCK_METHOD(Http::PathTransformer*, filterPathTransformer, (), (const)); + MOCK_METHOD(Http::PathTransformer*, forwardingPathTransformer, (), (const)); MOCK_METHOD(Http::StripPortType, stripPortType, (), (const)); MOCK_METHOD(envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction, headersWithUnderscoresAction, (), (const)); From 4786f15aa6de50c063a9f8f5445c2a94e0330f3f Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 26 Mar 2021 05:03:15 +0000 Subject: [PATCH 28/28] format Signed-off-by: chaoqin-li1123 --- test/common/http/conn_manager_impl_test_base.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/common/http/conn_manager_impl_test_base.h b/test/common/http/conn_manager_impl_test_base.h index 312d27245901..96ebb408102b 100644 --- a/test/common/http/conn_manager_impl_test_base.h +++ b/test/common/http/conn_manager_impl_test_base.h @@ -3,10 +3,9 @@ #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 "source/common/http/_virtual_includes/path_utility_lib/common/http/path_utility.h" - #include "extensions/access_loggers/common/file_access_log_impl.h" #include "test/mocks/access_log/mocks.h"