Fix URI rewrite

Previously, if a rewrite was configured in a VS/VSR route, and the route had a split action, match action or a regex path, NGINX would mess up the arguments in the resulting rewrite. For example, the result would be /test%3Fhello=world?hello=world instead of the correct /test?hello=world Fixes #1993
nginx · Dec 15, 2021 · e2d5f3d · e2d5f3d
1 parent c3c52c9
commit e2d5f3d
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 9 deletions.
diff --git a/internal/configs/version1/nginx-plus.tmpl b/internal/configs/version1/nginx-plus.tmpl
@@ -89,6 +89,10 @@ http {
     variables_hash_bucket_size {{.VariablesHashBucketSize}};
     variables_hash_max_size {{.VariablesHashMaxSize}};
 
+    map $request_uri $request_uri_no_args {
+        "~^(?P<path>[^?]*)(\?.*)?$" $path;
+    }
+
     map $http_upgrade $connection_upgrade {
         default upgrade;
         ''      close;

diff --git a/internal/configs/version1/nginx.tmpl b/internal/configs/version1/nginx.tmpl
@@ -74,6 +74,10 @@ http {
     variables_hash_bucket_size {{.VariablesHashBucketSize}};
     variables_hash_max_size {{.VariablesHashMaxSize}};
 
+    map $request_uri $request_uri_no_args {
+        "~^(?P<path>[^?]*)(\?.*)?$" $path;
+    }
+
     map $http_upgrade $connection_upgrade {
         default upgrade;
         ''      close;

diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go
@@ -1395,8 +1395,11 @@ func generateRewrites(path string, proxy *conf_v1.ActionProxy, internal bool, or
 	var rewrites []string
 
 	if internal {
-		// For internal locations (splits locations) only, recover the original request_uri.
-		rewrites = append(rewrites, "^ $request_uri")
+		// For internal locations only, recover the original request_uri without (!) the arguments.
+		// This is necessary, because if we just use $request_uri (which includes the arguments),
+		// the rewrite that follows will result in an URI with duplicated arguments:
+		// for example, /test%3Fhello=world?hello=world instead of /test?hello=world
+		rewrites = append(rewrites, "^ $request_uri_no_args")
 	}
 
 	if isRegex {

diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go
@@ -5040,7 +5040,7 @@ func TestGenerateSplits(t *testing.T) {
 			Path:      "/internal_location_splits_1_split_0",
 			ProxyPass: "http://vs_default_cafe_coffee-v1",
 			Rewrites: []string{
-				"^ $request_uri",
+				"^ $request_uri_no_args",
 				fmt.Sprintf(`"^%v(.*)$" "/rewrite$1" break`, originalPath),
 			},
 			ProxyNextUpstream:        "error timeout",
@@ -7282,23 +7282,27 @@ func TestGenerateRewrites(t *testing.T) {
 		originalPath string
 		grpcEnabled  bool
 		expected     []string
+		msg          string
 	}{
 		{
 			proxy:    nil,
 			expected: nil,
+			msg:      "action isn't proxy",
 		},
 		{
 			proxy: &conf_v1.ActionProxy{
 				RewritePath: "",
 			},
 			expected: nil,
+			msg:      "no rewrite is configured",
 		},
 		{
 			path: "/path",
 			proxy: &conf_v1.ActionProxy{
 				RewritePath: "/rewrite",
 			},
 			expected: nil,
+			msg:      "non-regex rewrite for non-internal location is not needed",
 		},
 		{
 			path:     "/_internal_path",
@@ -7307,7 +7311,8 @@ func TestGenerateRewrites(t *testing.T) {
 				RewritePath: "/rewrite",
 			},
 			originalPath: "/path",
-			expected:     []string{`^ $request_uri`, `"^/path(.*)$" "/rewrite$1" break`},
+			expected:     []string{`^ $request_uri_no_args`, `"^/path(.*)$" "/rewrite$1" break`},
+			msg:          "non-regex rewrite for internal location",
 		},
 		{
 			path:     "~/regex",
@@ -7316,7 +7321,8 @@ func TestGenerateRewrites(t *testing.T) {
 				RewritePath: "/rewrite",
 			},
 			originalPath: "/path",
-			expected:     []string{`^ $request_uri`, `"^/path(.*)$" "/rewrite$1" break`},
+			expected:     []string{`^ $request_uri_no_args`, `"^/path(.*)$" "/rewrite$1" break`},
+			msg:          "regex rewrite for internal location",
 		},
 		{
 			path:     "~/regex",
@@ -7325,6 +7331,7 @@ func TestGenerateRewrites(t *testing.T) {
 				RewritePath: "/rewrite",
 			},
 			expected: []string{`"^/regex" "/rewrite" break`},
+			msg:      "regex rewrite for non-internal location",
 		},
 		{
 			path:     "/_internal_path",
@@ -7334,22 +7341,23 @@ func TestGenerateRewrites(t *testing.T) {
 			},
 			originalPath: "/path",
 			grpcEnabled:  true,
-			expected:     []string{`^ $request_uri`, `"^/path(.*)$" "/rewrite$1" break`},
+			expected:     []string{`^ $request_uri_no_args`, `"^/path(.*)$" "/rewrite$1" break`},
+			msg:          "non-regex rewrite for internal location with grpc enabled",
 		},
 		{
 			path:         "/_internal_path",
 			internal:     true,
 			originalPath: "/path",
 			grpcEnabled:  true,
 			expected:     []string{`^ $request_uri break`},
+			msg:          "empty rewrite for internal location with grpc enabled",
 		},
 	}
 
 	for _, test := range tests {
 		result := generateRewrites(test.path, test.proxy, test.internal, test.originalPath, test.grpcEnabled)
-		if !reflect.DeepEqual(result, test.expected) {
-			t.Errorf("generateRewrites(%v, %v, %v, %v) returned \n %v but expected \n %v",
-				test.path, test.proxy, test.internal, test.originalPath, result, test.expected)
+		if diff := cmp.Diff(test.expected, result); diff != "" {
+			t.Errorf("generateRewrites() '%v' mismatch (-want +got):\n%s", test.msg, diff)
 		}
 	}
 }