diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go index 324c531d7..23da4f9b2 100644 --- a/modules/caddyhttp/rewrite/rewrite.go +++ b/modules/caddyhttp/rewrite/rewrite.go @@ -296,7 +296,7 @@ func (rewr Rewrite) Rewrite(r *http.Request, repl *caddy.Replacer) bool { mergeSlashes := !strings.Contains(suffix, "//") changePath(r, func(escapedPath string) string { escapedPath = caddyhttp.CleanPath(escapedPath, mergeSlashes) - return reverse(trimPathPrefix(reverse(escapedPath), reverse(suffix))) + return trimPathSuffix(escapedPath, suffix) }) } @@ -464,12 +464,58 @@ func trimPathPrefix(escapedPath, prefix string) string { return escapedPath } -func reverse(s string) string { - r := []rune(s) - for i, j := 0, len(r)-1; i < len(r)/2; i, j = i+1, j-1 { - r[i], r[j] = r[j], r[i] +// trimPathSuffix is the suffix counterpart of trimPathPrefix: it trims suffix +// from the end of escapedPath using the same escape-aware, case-insensitive +// comparison semantics. Both strings are iterated in lock-step from their ends, +// and if escapedPath has a '%' encoding at a particular position where the +// suffix pattern uses the decoded character, escapedPath's escape is decoded so +// the comparison happens in normalized/unescaped space. Conversely, if the +// suffix pattern itself uses an escape (`%xx`), escapedPath must literally use +// the same escape at that position (the escapes are then compared byte-for-byte). +// +// A naive reverse-then-trimPathPrefix approach cannot be used here: reversing +// the strings moves the '%' to the end of each escape sequence, which defeats +// trimPathPrefix's escape detection (it expects '%' to precede the two hex +// digits) and makes escaped path bytes compare unequal to their decoded form. +func trimPathSuffix(escapedPath, suffix string) string { + iPath, iSuffix := len(escapedPath), len(suffix) + for iPath > 0 && iSuffix > 0 { + suffixCh := suffix[iSuffix-1] + ch := string(escapedPath[iPath-1]) + step := 1 + + // if escapedPath uses a percent-encoding that ends at this position but + // the suffix pattern does not encode this position, decode escapedPath's + // escape so the comparison happens in normalized/unescaped space + pathHasEscape := iPath >= 3 && escapedPath[iPath-3] == '%' + suffixHasEscape := iSuffix >= 3 && suffix[iSuffix-3] == '%' + if pathHasEscape && !suffixHasEscape { + decoded, err := url.PathUnescape(escapedPath[iPath-3 : iPath]) + if err != nil { + // should be impossible unless EscapedPath() is returning invalid values! + return escapedPath + } + ch = decoded + step = 3 + } + + // suffix comparisons are case-insensitive for consistency with + // trimPathPrefix, which is case-insensitive for good reasons + if !strings.EqualFold(ch, string(suffixCh)) { + return escapedPath + } + + iPath -= step + iSuffix-- } - return string(r) + + // if we iterated through the entire suffix, we found it, so trim it + if iSuffix <= 0 { + return escapedPath[:iPath] + } + + // otherwise we did not find the suffix + return escapedPath } // substrReplacer describes either a simple and fast substring replacement. diff --git a/modules/caddyhttp/rewrite/rewrite_test.go b/modules/caddyhttp/rewrite/rewrite_test.go index 6a43ea5b2..0261acd35 100644 --- a/modules/caddyhttp/rewrite/rewrite_test.go +++ b/modules/caddyhttp/rewrite/rewrite_test.go @@ -335,6 +335,27 @@ func TestRewrite(t *testing.T) { input: newRequest(t, "GET", "/foo/suffix/bar"), expect: newRequest(t, "GET", "/foo/suffix/bar"), }, + { + // a decoded suffix pattern must match a percent-encoded path in + // normalized space, mirroring StripPathPrefix (see the "/a/b/c" + // vs "/a%2Fb/c/d" case above) + rule: Rewrite{StripPathSuffix: "/b/c"}, + input: newRequest(t, "GET", "/a/b%2Fc"), + expect: newRequest(t, "GET", "/a"), + }, + { + // decoded suffix char matches its percent-encoded form in the path + rule: Rewrite{StripPathSuffix: "bc"}, + input: newRequest(t, "GET", "/a%62c"), // %62 == 'b' + expect: newRequest(t, "GET", "/a"), + }, + { + // an escaped suffix pattern requires the path to use the same + // escape at that position, so a decoded path must NOT be stripped + rule: Rewrite{StripPathSuffix: "%2fsuffix"}, + input: newRequest(t, "GET", "/foo/bar/suffix"), + expect: newRequest(t, "GET", "/foo/bar/suffix"), + }, { rule: Rewrite{URISubstring: []substrReplacer{{Find: "findme", Replace: "replaced"}}},