rewrite: Make URI modifications more transactional (#2891)

Before, modifying the path might have affected how a new query string
was built if the query string relied on the path. Now, we build each
component in isolation and only change the URI on the request later.

Also, prevent trailing & in query string.

2 files changed, 31 insertions, 5 deletions

diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go
index abcaa84..e2d57c4 100644
--- a/modules/caddyhttp/rewrite/rewrite.go
+++ b/modules/caddyhttp/rewrite/rewrite.go
@@ -112,7 +112,7 @@ func (rewr Rewrite) rewrite(r *http.Request, repl *caddy.Replacer, logger *zap.L
 		r.Method = strings.ToUpper(repl.ReplaceAll(rewr.Method, ""))
 	}
 
-	// uri (path, query string, and fragment just because)
+	// uri (path, query string, and fragment... because why not)
 	if uri := rewr.URI; uri != "" {
 		// find the bounds of each part of the URI that exist
 		pathStart, qsStart, fragStart := -1, -1, -1
@@ -134,14 +134,30 @@ func (rewr Rewrite) rewrite(r *http.Request, repl *caddy.Replacer, logger *zap.L
 			qsEnd = len(uri)
 		}
 
+		// build components which are specified, and store them
+		// in a temporary variable so that they all read the
+		// same version of the URI
+		var newPath, newQuery, newFrag string
 		if pathStart >= 0 {
-			r.URL.Path = repl.ReplaceAll(uri[pathStart:pathEnd], "")
+			newPath = repl.ReplaceAll(uri[pathStart:pathEnd], "")
 		}
 		if qsStart >= 0 {
-			r.URL.RawQuery = buildQueryString(uri[qsStart:qsEnd], repl)
+			newQuery = buildQueryString(uri[qsStart:qsEnd], repl)
 		}
 		if fragStart >= 0 {
-			r.URL.Fragment = repl.ReplaceAll(uri[fragStart:], "")
+			newFrag = repl.ReplaceAll(uri[fragStart:], "")
+		}
+
+		// update the URI with the new components
+		// only after building them
+		if pathStart >= 0 {
+			r.URL.Path = newPath
+		}
+		if qsStart >= 0 {
+			r.URL.RawQuery = newQuery
+		}
+		if fragStart >= 0 {
+			r.URL.Fragment = newFrag
 		}
 	}
 
@@ -206,7 +222,7 @@ func buildQueryString(qs string, repl *caddy.Replacer) string {
 		// if previous iteration wrote a value,
 		// that means we are writing a key
 		if wroteVal {
-			if sb.Len() > 0 {
+			if sb.Len() > 0 && len(comp) > 0 {
 				sb.WriteRune('&')
 			}
 		} else {
diff --git a/modules/caddyhttp/rewrite/rewrite_test.go b/modules/caddyhttp/rewrite/rewrite_test.go
index 3dbc2d6..34a0cdb 100644
--- a/modules/caddyhttp/rewrite/rewrite_test.go
+++ b/modules/caddyhttp/rewrite/rewrite_test.go
@@ -64,6 +64,16 @@ func TestRewrite(t *testing.T) {
 			expect: newRequest(t, "GET", "/foo/bar"),
 		},
 		{
+			rule:   Rewrite{URI: "/index.php?p={http.request.uri.path}"},
+			input:  newRequest(t, "GET", "/foo/bar"),
+			expect: newRequest(t, "GET", "/index.php?p=%2Ffoo%2Fbar"),
+		},
+		{
+			rule:   Rewrite{URI: "?a=b&{http.request.uri.query}"},
+			input:  newRequest(t, "GET", "/"),
+			expect: newRequest(t, "GET", "/?a=b"),
+		},
+		{
 			rule:   Rewrite{URI: "/?c=d"},
 			input:  newRequest(t, "GET", "/"),
 			expect: newRequest(t, "GET", "/?c=d"),