From 809e72792c501ceeadac4a3b9b327dfc575b9dfd Mon Sep 17 00:00:00 2001
From: Matthew Holt <mholt@users.noreply.github.com>
Date: Wed, 1 Apr 2020 00:43:40 -0600
Subject: rewrite: Fix for rewrites with URI placeholders (#3209)

If a placeholder in the path component injects a query string such as
the {http.request.uri} placeholder is wont to do, we need to separate it
out from the path.
---
 modules/caddyhttp/rewrite/rewrite.go      | 39 +++++++++++++++++++++++++------
 modules/caddyhttp/rewrite/rewrite_test.go | 31 ++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 7 deletions(-)

(limited to 'modules/caddyhttp/rewrite')

diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go
index 3ba63c4..d47c388 100644
--- a/modules/caddyhttp/rewrite/rewrite.go
+++ b/modules/caddyhttp/rewrite/rewrite.go
@@ -114,7 +114,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... because why not)
+	// 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
@@ -136,18 +136,43 @@ func (rewr Rewrite) rewrite(r *http.Request, repl *caddy.Replacer, logger *zap.L
 			qsEnd = len(uri)
 		}
 
+		// isolate the three main components of the URI
+		var path, query, frag string
+		if pathStart > -1 {
+			path = uri[pathStart:pathEnd]
+		}
+		if qsStart > -1 {
+			query = uri[qsStart:qsEnd]
+		}
+		if fragStart > -1 {
+			frag = uri[fragStart:]
+		}
+
 		// 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 {
-			newPath = repl.ReplaceAll(uri[pathStart:pathEnd], "")
+		if path != "" {
+			newPath = repl.ReplaceAll(path, "")
 		}
-		if qsStart >= 0 {
-			newQuery = buildQueryString(uri[qsStart:qsEnd], repl)
+
+		// before continuing, we need to check if a query string
+		// snuck into the path component during replacements
+		if quPos := strings.Index(newPath, "?"); quPos > -1 {
+			// recompute; new path contains a query string
+			var injectedQuery string
+			newPath, injectedQuery = newPath[:quPos], newPath[quPos+1:]
+			// don't overwrite explicitly-configured query string
+			if query == "" {
+				query = injectedQuery
+			}
 		}
-		if fragStart >= 0 {
-			newFrag = repl.ReplaceAll(uri[fragStart:], "")
+
+		if query != "" {
+			newQuery = buildQueryString(query, repl)
+		}
+		if frag != "" {
+			newFrag = repl.ReplaceAll(frag, "")
 		}
 
 		// update the URI with the new components
diff --git a/modules/caddyhttp/rewrite/rewrite_test.go b/modules/caddyhttp/rewrite/rewrite_test.go
index 34a0cdb..fb4931b 100644
--- a/modules/caddyhttp/rewrite/rewrite_test.go
+++ b/modules/caddyhttp/rewrite/rewrite_test.go
@@ -158,6 +158,36 @@ func TestRewrite(t *testing.T) {
 			input:  newRequest(t, "GET", "/foo/bar?a=b"),
 			expect: newRequest(t, "GET", "/foo?a=b#frag"),
 		},
+		{
+			rule:   Rewrite{URI: "/foo{http.request.uri}"},
+			input:  newRequest(t, "GET", "/bar?a=b"),
+			expect: newRequest(t, "GET", "/foo/bar?a=b"),
+		},
+		{
+			rule:   Rewrite{URI: "/foo{http.request.uri}"},
+			input:  newRequest(t, "GET", "/bar"),
+			expect: newRequest(t, "GET", "/foo/bar"),
+		},
+		{
+			rule:   Rewrite{URI: "/foo{http.request.uri}?c=d"},
+			input:  newRequest(t, "GET", "/bar?a=b"),
+			expect: newRequest(t, "GET", "/foo/bar?c=d"),
+		},
+		{
+			rule:   Rewrite{URI: "/foo{http.request.uri}?{http.request.uri.query}&c=d"},
+			input:  newRequest(t, "GET", "/bar?a=b"),
+			expect: newRequest(t, "GET", "/foo/bar?a=b&c=d"),
+		},
+		{
+			rule:   Rewrite{URI: "{http.request.uri}"},
+			input:  newRequest(t, "GET", "/bar?a=b"),
+			expect: newRequest(t, "GET", "/bar?a=b"),
+		},
+		{
+			rule:   Rewrite{URI: "{http.request.uri.path}bar?c=d"},
+			input:  newRequest(t, "GET", "/foo/?a=b"),
+			expect: newRequest(t, "GET", "/foo/bar?c=d"),
+		},
 
 		{
 			rule:   Rewrite{StripPathPrefix: "/prefix"},
@@ -211,6 +241,7 @@ func TestRewrite(t *testing.T) {
 		}
 
 		// populate the replacer just enough for our tests
+		repl.Set("http.request.uri", tc.input.RequestURI)
 		repl.Set("http.request.uri.path", tc.input.URL.Path)
 		repl.Set("http.request.uri.query", tc.input.URL.RawQuery)
 
-- 
cgit v1.2.3