rewrite: Attempt query string fix (#2891)

2 files changed, 26 insertions, 6 deletions

diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go
index adb90f4..3a644ef 100644
--- a/modules/caddyhttp/rewrite/rewrite.go
+++ b/modules/caddyhttp/rewrite/rewrite.go
@@ -126,9 +126,24 @@ func (rewr Rewrite) rewrite(r *http.Request, repl caddy.Replacer, logger *zap.Lo
 		if newU.Path != "" {
 			r.URL.Path = newU.Path
 		}
-		if newU.RawQuery != "" {
-			newU.RawQuery = strings.TrimPrefix(newU.RawQuery, "&")
-			r.URL.RawQuery = newU.RawQuery
+		if strings.Contains(newURI, "?") {
+			// you'll notice we check for existence of a question mark
+			// instead of RawQuery != "". We do this because if the user
+			// wants to remove an existing query string, they do that by
+			// appending "?" to the path: "/foo?" -- in this case, then,
+			// RawQuery is "" but we still want to set it to that; hence,
+			// we check for a "?", which always starts a query string
+			inputQuery := newU.Query()
+			outputQuery := make(url.Values)
+			for k := range inputQuery {
+				// overwrite existing values; we don't simply keep
+				// appending because it can cause rewrite rules like
+				// "{path}{query}&a=b" with rehandling enabled to go
+				// on forever: "/foo.html?a=b&a=b&a=b..."
+				outputQuery.Set(k, inputQuery.Get(k))
+			}
+			// this sorts the keys, oh well
+			r.URL.RawQuery = outputQuery.Encode()
 		}
 		if newU.Fragment != "" {
 			r.URL.Fragment = newU.Fragment
diff --git a/modules/caddyhttp/rewrite/rewrite_test.go b/modules/caddyhttp/rewrite/rewrite_test.go
index f496f0a..2fb5c66 100644
--- a/modules/caddyhttp/rewrite/rewrite_test.go
+++ b/modules/caddyhttp/rewrite/rewrite_test.go
@@ -126,12 +126,17 @@ func TestRewrite(t *testing.T) {
 		{
 			rule:   Rewrite{URI: "/index.php?c=d&{http.request.uri.query}"},
 			input:  newRequest(t, "GET", "/?a=b"),
-			expect: newRequest(t, "GET", "/index.php?c=d&a=b"),
+			expect: newRequest(t, "GET", "/index.php?a=b&c=d"),
 		},
 		{
 			rule:   Rewrite{URI: "/index.php?{http.request.uri.query}&p={http.request.uri.path}"},
 			input:  newRequest(t, "GET", "/foo/bar?a=b"),
-			expect: newRequest(t, "GET", "/index.php?a=b&p=/foo/bar"),
+			expect: newRequest(t, "GET", "/index.php?a=b&p=%2Ffoo%2Fbar"),
+		},
+		{
+			rule:   Rewrite{URI: "{http.request.uri.path}?"},
+			input:  newRequest(t, "GET", "/foo/bar?a=b&c=d"),
+			expect: newRequest(t, "GET", "/foo/bar"),
 		},
 
 		{
@@ -188,7 +193,7 @@ func TestRewrite(t *testing.T) {
 		// populate the replacer just enough for our tests
 		repl.Set("http.request.uri.path", tc.input.URL.Path)
 		repl.Set("http.request.uri.query", tc.input.URL.RawQuery)
-		repl.Set("http.request.uri.query_string", "?"+tc.input.URL.Query().Encode())
+		repl.Set("http.request.uri.query_string", "?"+tc.input.URL.RawQuery)
 
 		changed := tc.rule.rewrite(tc.input, repl, nil)