rewrite: Use RawPath instead of Path (fix #3596) (#3918)

Prevent information loss, i.e. the encoded form that was sent by the
client, when using URL strip/replace.

2 files changed, 37 insertions, 11 deletions

diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go
index d47c388..ad1c470 100644
--- a/modules/caddyhttp/rewrite/rewrite.go
+++ b/modules/caddyhttp/rewrite/rewrite.go
@@ -191,11 +191,21 @@ func (rewr Rewrite) rewrite(r *http.Request, repl *caddy.Replacer, logger *zap.L
 	// strip path prefix or suffix
 	if rewr.StripPathPrefix != "" {
 		prefix := repl.ReplaceAll(rewr.StripPathPrefix, "")
-		r.URL.Path = strings.TrimPrefix(r.URL.Path, prefix)
+		r.URL.RawPath = strings.TrimPrefix(r.URL.RawPath, prefix)
+		if p, err := url.PathUnescape(r.URL.RawPath); err == nil && p != "" {
+			r.URL.Path = p
+		} else {
+			r.URL.Path = strings.TrimPrefix(r.URL.Path, prefix)
+		}
 	}
 	if rewr.StripPathSuffix != "" {
 		suffix := repl.ReplaceAll(rewr.StripPathSuffix, "")
-		r.URL.Path = strings.TrimSuffix(r.URL.Path, suffix)
+		r.URL.RawPath = strings.TrimSuffix(r.URL.RawPath, suffix)
+		if p, err := url.PathUnescape(r.URL.RawPath); err == nil && p != "" {
+			r.URL.Path = p
+		} else {
+			r.URL.Path = strings.TrimSuffix(r.URL.Path, suffix)
+		}
 	}
 
 	// substring replacements in URI
@@ -289,10 +299,10 @@ type replacer struct {
 	Limit int `json:"limit,omitempty"`
 }
 
-// do performs the replacement on r and returns true if any changes were made.
-func (rep replacer) do(r *http.Request, repl *caddy.Replacer) bool {
+// do performs the replacement on r.
+func (rep replacer) do(r *http.Request, repl *caddy.Replacer) {
 	if rep.Find == "" || rep.Replace == "" {
-		return false
+		return
 	}
 
 	lim := rep.Limit
@@ -303,13 +313,14 @@ func (rep replacer) do(r *http.Request, repl *caddy.Replacer) bool {
 	find := repl.ReplaceAll(rep.Find, "")
 	replace := repl.ReplaceAll(rep.Replace, "")
 
-	oldPath := r.URL.Path
-	oldQuery := r.URL.RawQuery
-
-	r.URL.Path = strings.Replace(oldPath, find, replace, lim)
-	r.URL.RawQuery = strings.Replace(oldQuery, find, replace, lim)
+	r.URL.RawPath = strings.Replace(r.URL.RawPath, find, replace, lim)
+	if p, err := url.PathUnescape(r.URL.RawPath); err == nil && p != "" {
+		r.URL.Path = p
+	} else {
+		r.URL.Path = strings.Replace(r.URL.Path, find, replace, lim)
+	}
 
-	return r.URL.Path != oldPath && r.URL.RawQuery != oldQuery
+	r.URL.RawQuery = strings.Replace(r.URL.RawQuery, find, replace, lim)
 }
 
 // Interface guard
diff --git a/modules/caddyhttp/rewrite/rewrite_test.go b/modules/caddyhttp/rewrite/rewrite_test.go
index fb4931b..9329a04 100644
--- a/modules/caddyhttp/rewrite/rewrite_test.go
+++ b/modules/caddyhttp/rewrite/rewrite_test.go
@@ -201,6 +201,11 @@ func TestRewrite(t *testing.T) {
 		},
 		{
 			rule:   Rewrite{StripPathPrefix: "/prefix"},
+			input:  newRequest(t, "GET", "/prefix/foo%2Fbar"),
+			expect: newRequest(t, "GET", "/foo%2Fbar"),
+		},
+		{
+			rule:   Rewrite{StripPathPrefix: "/prefix"},
 			input:  newRequest(t, "GET", "/foo/prefix/bar"),
 			expect: newRequest(t, "GET", "/foo/prefix/bar"),
 		},
@@ -216,6 +221,11 @@ func TestRewrite(t *testing.T) {
 			expect: newRequest(t, "GET", "/foo/bar/"),
 		},
 		{
+			rule:   Rewrite{StripPathSuffix: "suffix"},
+			input:  newRequest(t, "GET", "/foo%2Fbar/suffix"),
+			expect: newRequest(t, "GET", "/foo%2Fbar/"),
+		},
+		{
 			rule:   Rewrite{StripPathSuffix: "/suffix"},
 			input:  newRequest(t, "GET", "/foo/suffix/bar"),
 			expect: newRequest(t, "GET", "/foo/suffix/bar"),
@@ -231,6 +241,11 @@ func TestRewrite(t *testing.T) {
 			input:  newRequest(t, "GET", "/foo/findme/bar"),
 			expect: newRequest(t, "GET", "/foo/replaced/bar"),
 		},
+		{
+			rule:   Rewrite{URISubstring: []replacer{{Find: "findme", Replace: "replaced"}}},
+			input:  newRequest(t, "GET", "/foo/findme%2Fbar"),
+			expect: newRequest(t, "GET", "/foo/replaced%2Fbar"),
+		},
 	} {
 		// copy the original input just enough so that we can
 		// compare it after the rewrite to see if it changed