diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index ef7fc793797dd..5b28768ff14fd 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -575,27 +575,28 @@ RouteEntryImplBase::loadRuntimeData(const envoy::config::route::v3::RouteMatch& return runtime; } -const std::string& RouteEntryImplBase::getPathRewrite(const Http::RequestHeaderMap& headers, absl::optional &container) const { - // Just use the prefix rewrite if this isn't a redirect. - if (!isRedirect()) { - return prefix_rewrite_; - } - - // Return the regex rewrite substitution for redirects, if set. - if (regex_rewrite_redirect_ != nullptr) { - std::string path(headers.getPathValue()); +const std::string& +RouteEntryImplBase::getPathRewrite(const Http::RequestHeaderMap& headers, + absl::optional& container) const { + // Just use the prefix rewrite if this isn't a redirect. + if (!isRedirect()) { + return prefix_rewrite_; + } - // Replace the entire path, but preserve the query parameters - auto just_path(Http::PathUtil::removeQueryAndFragment(path)); + // Return the regex rewrite substitution for redirects, if set. + if (regex_rewrite_redirect_ != nullptr) { + // Copy just the path and rewrite it using the regex. + // + // Store the result in the output container, and return a reference to the underlying string. + auto just_path(Http::PathUtil::removeQueryAndFragment(headers.getPathValue())); + container = + regex_rewrite_redirect_->replaceAll(just_path, regex_rewrite_redirect_substitution_); - // Store the result in the output container, and return a reference to the underlying string. - container = std::move(path.replace(0, just_path.size(), - regex_rewrite_redirect_->replaceAll(just_path, regex_rewrite_redirect_substitution_))); - return container.value(); - } + return container.value(); + } - // Otherwise, return the prefix rewrite used for redirects. - return prefix_rewrite_redirect_; + // Otherwise, return the prefix rewrite used for redirects. + return prefix_rewrite_redirect_; } // finalizePathHeaders does the "standard" path rewriting, meaning that it diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 2a60bafeeae7b..36c58404da929 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -450,7 +450,8 @@ class RouteEntryImplBase : public RouteEntry, if (!isDirectResponse()) { return false; } - return !host_redirect_.empty() || !path_redirect_.empty() || !prefix_rewrite_redirect_.empty() || regex_rewrite_redirect_ != nullptr; + return !host_redirect_.empty() || !path_redirect_.empty() || + !prefix_rewrite_redirect_.empty() || regex_rewrite_redirect_ != nullptr; } bool matchRoute(const Http::RequestHeaderMap& headers, const StreamInfo::StreamInfo& stream_info, @@ -550,7 +551,8 @@ class RouteEntryImplBase : public RouteEntry, * to store memory that backs the returned path, and so the lifetime of the container must * outlife any use of the returned path. */ - const std::string& getPathRewrite(const Http::RequestHeaderMap& headers, absl::optional &container) const; + const std::string& getPathRewrite(const Http::RequestHeaderMap& headers, + absl::optional& container) const; void finalizePathHeader(Http::RequestHeaderMap& headers, absl::string_view matched_path, bool insert_envoy_original_path) const; diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index f116e51bee79c..c135f197cad17 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -6255,6 +6255,77 @@ name: AllRedirects } } +TEST_F(RouteConfigurationV2, RedirectRegexRewrite) { + std::string yaml = R"EOF( +virtual_hosts: + - name: redirect + domains: [redirect.lyft.com] + routes: + - match: + safe_regex: + google_re2: {} + regex: /foo/([0-9]{4})/(.*) + redirect: + regex_rewrite: + pattern: + google_re2: {} + regex: /foo/([0-9]{4})/(.*) + substitution: /\2/\1/baz + - match: + safe_regex: + google_re2: {} + regex: /strip-query/([0-9]{4})/(.*) + redirect: + strip_query: true + regex_rewrite: + pattern: + google_re2: {} + regex: /strip-query/([0-9]{4})/(.*) + substitution: /\2/\1/baz + )EOF"; + + TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); + + EXPECT_EQ(nullptr, config.route(genRedirectHeaders("www.foo.com", "/foo", true, true), 0)); + + // Regex rewrite with a query, no strip_query + { + Http::TestRequestHeaderMapImpl headers = + genRedirectHeaders("redirect.lyft.com", "/foo/9000/endpoint?lang=eng&con=US", false, false); + const DirectResponseEntry* redirect = config.route(headers, 0)->directResponseEntry(); + redirect->rewritePathHeader(headers, true); + EXPECT_EQ("http://redirect.lyft.com/endpoint/9000/baz?lang=eng&con=US", + redirect->newPath(headers)); + } + // Regex rewrite without a query, no strip_query + { + Http::TestRequestHeaderMapImpl headers = + genRedirectHeaders("redirect.lyft.com", "/foo/1984/bar/anything", false, false); + const DirectResponseEntry* redirect = config.route(headers, 0)->directResponseEntry(); + redirect->rewritePathHeader(headers, true); + EXPECT_EQ("http://redirect.lyft.com/bar/anything/1984/baz", + redirect->newPath(headers)); + } + // Regex rewrite with a query, with strip_query + { + Http::TestRequestHeaderMapImpl headers = + genRedirectHeaders("redirect.lyft.com", "/strip-query/9000/endpoint?lang=eng&con=US", false, false); + const DirectResponseEntry* redirect = config.route(headers, 0)->directResponseEntry(); + redirect->rewritePathHeader(headers, true); + EXPECT_EQ("http://redirect.lyft.com/endpoint/9000/baz", + redirect->newPath(headers)); + } + // Regex rewrite without a query, with strip_query + { + Http::TestRequestHeaderMapImpl headers = + genRedirectHeaders("redirect.lyft.com", "/strip-query/1984/bar/anything", false, false); + const DirectResponseEntry* redirect = config.route(headers, 0)->directResponseEntry(); + redirect->rewritePathHeader(headers, true); + EXPECT_EQ("http://redirect.lyft.com/bar/anything/1984/baz", + redirect->newPath(headers)); + } +} + TEST_F(RouteConfigurationV2, PathRedirectQueryNotPreserved) { TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues(