From 724c72867835f9287b3df3ee5a0d75327c0780cf Mon Sep 17 00:00:00 2001
From: Matthew Holt <mholt@users.noreply.github.com>
Date: Tue, 17 Dec 2019 16:30:26 -0700
Subject: [PATCH] rewrite: Attempt query string fix (#2891)

---
 modules/caddyhttp/fileserver/caddyfile.go |  3 ++-
 modules/caddyhttp/rewrite/rewrite.go      | 21 ++++++++++++++++++---
 modules/caddyhttp/rewrite/rewrite_test.go | 11 ++++++++---
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/modules/caddyhttp/fileserver/caddyfile.go b/modules/caddyhttp/fileserver/caddyfile.go
index 8013fa24..3bd0ae47 100644
--- a/modules/caddyhttp/fileserver/caddyfile.go
+++ b/modules/caddyhttp/fileserver/caddyfile.go
@@ -127,7 +127,8 @@ func parseTryFiles(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error)
 	// to the end of the query string.
 	makeRoute := func(try []string, writeURIAppend string) []httpcaddyfile.ConfigValue {
 		handler := rewrite.Rewrite{
-			URI: "{http.matchers.file.relative}{http.request.uri.query_string}" + writeURIAppend,
+			Rehandle: true,
+			URI:      "{http.matchers.file.relative}{http.request.uri.query_string}" + writeURIAppend,
 		}
 		matcherSet := caddy.ModuleMap{
 			"file": h.JSON(MatchFile{
diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go
index adb90f48..3a644ef4 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 f496f0ae..2fb5c662 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)