Redirect regex fix

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<std::string> &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<std::string>& 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

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<std::string> &container) const;
+  const std::string& getPathRewrite(const Http::RequestHeaderMap& headers,
+                                    absl::optional<std::string>& container) const;
 
   void finalizePathHeader(Http::RequestHeaderMap& headers, absl::string_view matched_path,
                           bool insert_envoy_original_path) const;

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(