From 39af0aec318cd2d07c6e920055565f238c7f4830 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Tue, 25 Aug 2026 14:06:09 -0400 Subject: [PATCH] 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) * 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) * 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) --------- Co-authored-by: Claude Opus 5 (1M context) --- modules/caddyhttp/rewrite/rewrite.go | 26 +++++- modules/caddyhttp/rewrite/rewrite_test.go | 96 +++++++++++++++++++++++ 2 files changed, 121 insertions(+), 1 deletion(-) diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go index 8437ad790..324c531d7 100644 --- a/modules/caddyhttp/rewrite/rewrite.go +++ b/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 { diff --git a/modules/caddyhttp/rewrite/rewrite_test.go b/modules/caddyhttp/rewrite/rewrite_test.go index 0b80e45b6..6a43ea5b2 100644 --- a/modules/caddyhttp/rewrite/rewrite_test.go +++ b/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