rewrite: Handle fragment before query (fix #4775)

2 files changed, 22 insertions, 4 deletions

diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go
index 690e752..da92269 100644
--- a/modules/caddyhttp/rewrite/rewrite.go
+++ b/modules/caddyhttp/rewrite/rewrite.go
@@ -135,13 +135,20 @@ func (rewr Rewrite) Rewrite(r *http.Request, repl *caddy.Replacer) bool {
 		// find the bounds of each part of the URI that exist
 		pathStart, qsStart, fragStart := -1, -1, -1
 		pathEnd, qsEnd := -1, -1
+	loop:
 		for i, ch := range uri {
 			switch {
 			case ch == '?' && qsStart < 0:
 				pathEnd, qsStart = i, i+1
-			case ch == '#' && fragStart < 0:
-				qsEnd, fragStart = i, i+1
-			case pathStart < 0 && qsStart < 0 && fragStart < 0:
+			case ch == '#' && fragStart < 0: // everything after fragment is fragment (very clear in RFC 3986 section 4.2)
+				if qsStart < 0 {
+					pathEnd = i
+				} else {
+					qsEnd = i
+				}
+				fragStart = i + 1
+				break loop
+			case pathStart < 0 && qsStart < 0:
 				pathStart = i
 			}
 		}
diff --git a/modules/caddyhttp/rewrite/rewrite_test.go b/modules/caddyhttp/rewrite/rewrite_test.go
index 3e2e13d..84dce95 100644
--- a/modules/caddyhttp/rewrite/rewrite_test.go
+++ b/modules/caddyhttp/rewrite/rewrite_test.go
@@ -204,6 +204,16 @@ func TestRewrite(t *testing.T) {
 			input:  newRequest(t, "GET", "/%C2%B7%E2%88%B5.png?a=b"),
 			expect: newRequest(t, "GET", "/i/%C2%B7%E2%88%B5.png?a=b"),
 		},
+		{
+			rule:   Rewrite{URI: "/bar#?"},
+			input:  newRequest(t, "GET", "/foo#fragFirst?c=d"), // not a valid query string (is part of fragment)
+			expect: newRequest(t, "GET", "/bar#?"),             // I think this is right? but who knows; std lib drops fragment when parsing
+		},
+		{
+			rule:   Rewrite{URI: "/bar"},
+			input:  newRequest(t, "GET", "/foo#fragFirst?c=d"),
+			expect: newRequest(t, "GET", "/bar#fragFirst?c=d"),
+		},
 
 		{
 			rule:   Rewrite{StripPathPrefix: "/prefix"},
@@ -271,10 +281,11 @@ func TestRewrite(t *testing.T) {
 	} {
 		// copy the original input just enough so that we can
 		// compare it after the rewrite to see if it changed
+		urlCopy := *tc.input.URL
 		originalInput := &http.Request{
 			Method:     tc.input.Method,
 			RequestURI: tc.input.RequestURI,
-			URL:        &*tc.input.URL,
+			URL:        &urlCopy,
 		}
 
 		// populate the replacer just enough for our tests