rewrite: fix URI splitting when a query or fragment arrives via a placeholder (#7947)

* rewrite: don't drop a trailing '=' from the query string

buildQueryString scanned for '=' unconditionally when looking for the
end of a component, but '=' only delimits a key from its value once;
any further '=' bytes are literal data. When the query ended with '=',
that byte was consumed as a delimiter with nothing following it to
re-emit, so it was silently lost:

	?x=1&sig=YWJjZA==  =>  ?x=1&sig=YWJjZA=

This corrupts base64 padding in the last query parameter, which is the
shape of an S3 presigned URL (X-Amz-Signature), turning a valid
signature into a 403. Only the final '=' of the query was affected;
?sig=YWJjZA==&x=1 came through intact.

Disable the '=' search while consuming a value so that only '&' ends it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* rewrite: honor a query string injected by a replacement value

Which URI components get written back was decided from the literal
config string, before placeholders were expanded, but an injected query
is only detected after expansion. The two were never reconciled: for
`rewrite * {rp.header.X-Accel-Redirect}` there is no literal '?', so
qsStart stayed -1, and the correctly-built query string was computed and
then discarded by the `if qsStart >= 0` guard.

Only half the split was applied. The path was still truncated at the
injected '?', so the query was not preserved either -- it was dropped,
and any query already on the request survived in its place:

	GET /orig?keep=me, X-Accel-Redirect: /hello?some=param
	=> /hello?keep=me

Track whether a query was actually injected and include that in the
write-back condition. Appending a literal '?' to the rewrite value was
the known workaround precisely because it set qsStart; that keeps
working and is now unnecessary.

The flag is only set where the injected query is adopted, so an
explicitly configured query still wins, and a value with no '?' still
leaves the query untouched -- which is what the implicit rewrites of
try_files and php_fastcgi rely on. Those stay safe regardless, since
escapePathPlaceholders already escapes the two placeholders they use, so
a client-supplied %3F cannot split the URI.

Fixes #5208

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* rewrite: drop a fragment injected by a replacement value

The scan that separates path, query and fragment runs on the literal
config string, so a '#' arriving later via a replacement value was never
treated as a delimiter. It leaked into whichever component it landed in:

	X-Accel-Redirect: /hello?p=x#frag  =>  RawQuery = "p=x#frag"

Everything after '#' is fragment (RFC 3986 section 4.2) and a fragment is
never sent to the server, so drop it before the path is split, mirroring
how the scan already handles a literal '#'. An escaped %23 is unaffected,
so a real '#' in a path or query is still expressible, and a configured
fragment still wins over an injected one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

modules/caddyhttp/rewrite/rewrite.go

@@ -215,8 +215,19 @@ func (rewr Rewrite) Rewrite(r *http.Request, repl *caddy.Replacer) bool {
 			newPath = repl.ReplaceAll(path, "")
 		}
 
+		// a fragment may have snuck into the path component during
+		// replacements; everything after the first '#' is fragment
+		// (RFC 3986 section 4.2), which is never sent to the server,
+		// so drop it. this mirrors how a literal '#' in the configured
+		// URI is handled by the scan above, and prevents the fragment
+		// from being mistaken for part of the path or query below.
+		if before, _, found := strings.Cut(newPath, "#"); found {
+			newPath = before
+		}
+
 		// before continuing, we need to check if a query string
 		// snuck into the path component during replacements
+		queryInjected := false
 		if before, after, found := strings.Cut(newPath, "?"); found {
 			// recompute; new path contains a query string
 			var injectedQuery string
@@ -233,6 +244,13 @@ func (rewr Rewrite) Rewrite(r *http.Request, repl *caddy.Replacer) bool {
 				injectedQuery = strings.ReplaceAll(injectedQuery, "{", "%7B")
 				injectedQuery = strings.ReplaceAll(injectedQuery, "}", "%7D")
 				query = injectedQuery
+
+				// the replacement value spoke about the query string, so the
+				// query must be written back even though the configured URI
+				// had no literal '?' to set qsStart. an injected query that
+				// is empty (a value ending in '?') clears the query, which is
+				// consistent with configuring a bare '?'.
+				queryInjected = true
 			}
 		}
 
@@ -253,7 +271,7 @@ func (rewr Rewrite) Rewrite(r *http.Request, repl *caddy.Replacer) bool {
 			}
 			r.URL.RawPath = "" // force recomputing when EscapedPath() is called
 		}
-		if qsStart >= 0 {
+		if qsStart >= 0 || queryInjected {
 			r.URL.RawQuery = newQuery
 		}
 		if fragStart >= 0 {
@@ -344,6 +362,12 @@ func buildQueryString(qs string, repl *caddy.Replacer) string {
 		// determine the end of this component, which will be at
 		// the next equal sign or ampersand, whichever comes first
 		nextEq, nextAmp := strings.Index(qs, "="), strings.Index(qs, "&")
+		if !wroteVal {
+			// we are consuming a value, and '=' only delimits a key from
+			// its value; any further '=' bytes are literal data, such as
+			// base64 padding in a signature. only '&' ends a value.
+			nextEq = -1
+		}
 		ampIsNext := nextAmp >= 0 && (nextAmp < nextEq || nextEq < 0)
 		end := len(qs) // assume no delimiter remains...
 		if ampIsNext {

modules/caddyhttp/rewrite/rewrite_test.go

@@ -396,6 +396,102 @@ func TestRewrite(t *testing.T) {
 			input:  newRequestWithHeader(t, "GET", "/anything", "X-Fwd", "ok?path={file./etc/passwd}"),
 			expect: newRequest(t, "GET", "/serve/ok?path=%7Bfile./etc/passwd%7D"),
 		},
+
+		// '=' delimits a key from its value only once; any further '=' bytes
+		// are literal data, such as base64 padding in a signature.
+		{
+			rule:   Rewrite{URI: "?sig=YWJjZA=="},
+			input:  newRequest(t, "GET", "/hello"),
+			expect: newRequest(t, "GET", "/hello?sig=YWJjZA=="),
+		},
+		{
+			rule:   Rewrite{URI: "?x=1&sig=YWJjZA=="},
+			input:  newRequest(t, "GET", "/hello"),
+			expect: newRequest(t, "GET", "/hello?x=1&sig=YWJjZA=="),
+		},
+
+		// a query string that arrives via a replacement value is honored even
+		// though the configured URI has no literal '?' (see issue #5208).
+		{
+			rule:   Rewrite{URI: "{http.request.header.X-Accel-Redirect}"},
+			input:  newRequestWithHeader(t, "GET", "/orig", "X-Accel-Redirect", "/hello?some=param&some_other=param"),
+			expect: newRequest(t, "GET", "/hello?some=param&some_other=param"),
+		},
+		{
+			// an injected query replaces the original one, as a configured query would
+			rule:   Rewrite{URI: "{http.request.header.X-Accel-Redirect}"},
+			input:  newRequestWithHeader(t, "GET", "/orig?keep=me", "X-Accel-Redirect", "/hello?some=param"),
+			expect: newRequest(t, "GET", "/hello?some=param"),
+		},
+		{
+			// a value ending in '?' clears the query, same as configuring a bare '?'
+			rule:   Rewrite{URI: "{http.request.header.X-Accel-Redirect}"},
+			input:  newRequestWithHeader(t, "GET", "/orig?keep=me", "X-Accel-Redirect", "/hello?"),
+			expect: newRequest(t, "GET", "/hello"),
+		},
+		{
+			// no '?' in the value means the query is not touched
+			rule:   Rewrite{URI: "{http.request.header.X-Accel-Redirect}"},
+			input:  newRequestWithHeader(t, "GET", "/orig?keep=me", "X-Accel-Redirect", "/hello"),
+			expect: newRequest(t, "GET", "/hello?keep=me"),
+		},
+		{
+			// an explicitly configured query still wins over an injected one
+			rule:   Rewrite{URI: "{http.request.header.X-Accel-Redirect}?a=b"},
+			input:  newRequestWithHeader(t, "GET", "/orig", "X-Accel-Redirect", "/hello?some=param"),
+			expect: newRequest(t, "GET", "/hello?a=b"),
+		},
+		{
+			// an escaped '?' in the request path does not split the URI, so the
+			// implicit rewrites of try_files and php_fastcgi stay safe
+			rule:   Rewrite{URI: "{http.request.uri.path}"},
+			input:  newRequest(t, "GET", "/bar%3Fbaz?keep=me"),
+			expect: newRequest(t, "GET", "/bar%3Fbaz?keep=me"),
+		},
+		{
+			// an S3 presigned URL arriving via a replacement value survives intact
+			rule:   Rewrite{URI: "{http.request.header.X-Ph}"},
+			input:  newRequestWithHeader(t, "GET", "/orig", "X-Ph", "/f.jpg?X-Amz-Credential=AK%2Ffr-par%2Fs3&X-Amz-Signature=YWJjZA=="),
+			expect: newRequest(t, "GET", "/f.jpg?X-Amz-Credential=AK%2Ffr-par%2Fs3&X-Amz-Signature=YWJjZA=="),
+		},
+
+		// a fragment that arrives via a replacement value is dropped: everything
+		// after '#' is fragment (RFC 3986 section 4.2) and is never sent to the
+		// server, so it must not leak into the path or query.
+		{
+			rule:   Rewrite{URI: "{http.request.header.X-Ph}"},
+			input:  newRequestWithHeader(t, "GET", "/orig", "X-Ph", "/hello#frag"),
+			expect: newRequest(t, "GET", "/hello"),
+		},
+		{
+			rule:   Rewrite{URI: "{http.request.header.X-Ph}"},
+			input:  newRequestWithHeader(t, "GET", "/orig", "X-Ph", "/hello?a=b#frag"),
+			expect: newRequest(t, "GET", "/hello?a=b"),
+		},
+		{
+			// a '?' after the injected '#' is fragment, not a query delimiter
+			rule:   Rewrite{URI: "{http.request.header.X-Ph}"},
+			input:  newRequestWithHeader(t, "GET", "/orig", "X-Ph", "/hello#frag?notquery"),
+			expect: newRequest(t, "GET", "/hello"),
+		},
+		{
+			// a configured fragment still wins over an injected one
+			rule:   Rewrite{URI: "{http.request.header.X-Ph}#cfgfrag"},
+			input:  newRequestWithHeader(t, "GET", "/orig", "X-Ph", "/hello?a=b#frag"),
+			expect: newRequest(t, "GET", "/hello?a=b#cfgfrag"),
+		},
+		{
+			// an escaped '#' is not a fragment delimiter and is preserved
+			rule:   Rewrite{URI: "{http.request.header.X-Ph}"},
+			input:  newRequestWithHeader(t, "GET", "/orig", "X-Ph", "/hello%23frag"),
+			expect: newRequest(t, "GET", "/hello%23frag"),
+		},
+		{
+			// a fragment-only value leaves the original query alone
+			rule:   Rewrite{URI: "{http.request.header.X-Ph}"},
+			input:  newRequestWithHeader(t, "GET", "/orig?keep=me", "X-Ph", "/hello#frag"),
+			expect: newRequest(t, "GET", "/hello?keep=me"),
+		},
 	} {
 		// copy the original input just enough so that we can
 		// compare it after the rewrite to see if it changed