Repository: caddyserver/caddy (language: go)

Issue:
try_files possibly double-escaping URI-encoded characters (%3F)
[Moving this one over from [the community forums](https://caddy.community/t/handling-3f-in-uri-with-try-files/18301/4).]

While testing out Caddy to replace nginx I noticed that the `try_files` directive might be double-escaping URI-encoded strings (in particular, `%3F` (`?`). I ran into this because I have files in my filesystem with `?` in their names, from mirroring an old Wordpress site that I access via a `try_files` config.

I'm running Caddy version `v2.6.2 h1:wKoFIxpmOJLGl3QXoo6PNbYvGW4xLEgo32GPBEjWL8o=`, accessing it directly.

### Steps to reproduce

To eliminate the rest of my config for my live site, I duplicated the problem with a tiny test scenario. I set up this config:

```
mati.ca, www.mati.ca {
        root * /home/rich/sites/mati.ca
        log
        file_server browse {
                index index.html index.htm
        }
        try_files {path}
}
```

and created three files in the site root directory, named as follows. The content of each file is the same as the file’s filename, so I could tell what file caddy was returning.

```
file.txt
file.txt?honk
file.txt%3Fhonk
```

### Expected behavior

When I `curl https://www.mati.ca/file.txt%3Fhonk`, it should serve the file named `file.txt?honk`.

```
$ curl https://www.mati.ca/file.txt?honk
file.txt
$ curl https://www.mati.ca/file.txt%3Fhonk
file.txt?honk
```

### Observed behavior

```
$ curl https://www.mati.ca/file.txt?honk
file.txt
$ curl https://www.mati.ca/file.txt%3Fhonk
file.txt
```

(*Without* `try_files {path}` I get the expected behavior.)


Discussion / hints:
Did some digging around on this hoping that I could provide a fix, but I didn't get _quite_ that far.

That said, the problem seems to be in https://github.com/caddyserver/caddy/blob/master/modules/caddyhttp/rewrite/rewrite.go#L213-L223: 

```go
		// before continuing, we need to check if a query string
		// snuck into the path component during replacements
		if before, after, found := strings.Cut(newPath, "?"); found {
			// recompute; new path contains a query string
			var injectedQuery string
			newPath, injectedQuery = before, after
			// don't overwrite explicitly-configured query string
			if query == "" {
				query = injectedQuery
			}
		}
```
which was part of the fix for https://github.com/caddyserver/caddy/issues/3209. 

Prior to that block, `newPath` is correct in my use case. 

I think the underlying problem is that before the `repl.ReplaceAll` immediately before that block -- with URIs decoded and query separated -- we know that any `?` in path is a literal `?`; but after the `repl.ReplaceAll` there's no way to know if a `?` in the path was intended to introduce query parameters.

Where I got stuck is what to do instead. I considered adding a check to see if `path` also contained a `?`, but that's very focused on _this_ bug and not the general case. But I think it's not enough to only check if `newPath` contains `?` there.

(This all makes me realize there's a really ugly edge case here too, where the request contains a `%3F` _and_ the rewrite introduces a query string.)


Hey, sorry for the silence on this. Got backlogged on notifications and then they disappeared from my GH account for a while, but now they're back, so, yay.

Thank you for the report and for looking into a fix.

> I think the underlying problem is that before the repl.ReplaceAll immediately before that block -- with URIs decoded and query separated -- we know that any ? in path is a literal ?; but after the repl.ReplaceAll there's no way to know if a ? in the path was intended to introduce query parameters.

It's been a while since I looked at that code, but... yes, exactly.

> Where I got stuck is what to do instead.

Reasonable. I would also like to rework our logic there.

I'm not sure what the right answer is right now either. If I can find some time I'll try to look into it; but further discussion or ideas are welcomed!

Fix patch:
diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go
index 31c97788679..499d2173fb0 100644
--- a/modules/caddyhttp/rewrite/rewrite.go
+++ b/modules/caddyhttp/rewrite/rewrite.go
@@ -195,16 +195,10 @@ func (rewr Rewrite) Rewrite(r *http.Request, repl *caddy.Replacer) bool {
 		var newPath, newQuery, newFrag string
 
 		if path != "" {
-			// Since the 'uri' placeholder performs a URL-encode,
-			// we need to intercept it so that it doesn't, because
-			// otherwise we risk a double-encode of the path.
-			uriPlaceholder := "{http.request.uri}"
-			if strings.Contains(path, uriPlaceholder) {
-				tmpUri := r.URL.Path
-				if r.URL.RawQuery != "" {
-					tmpUri += "?" + r.URL.RawQuery
-				}
-				path = strings.ReplaceAll(path, uriPlaceholder, tmpUri)
+			// replace the `path` placeholder to escaped path
+			pathPlaceholder := "{http.request.uri.path}"
+			if strings.Contains(path, pathPlaceholder) {
+				path = strings.ReplaceAll(path, pathPlaceholder, r.URL.EscapedPath())
 			}
 
 			newPath = repl.ReplaceAll(path, "")
@@ -232,7 +226,11 @@ func (rewr Rewrite) Rewrite(r *http.Request, repl *caddy.Replacer) bool {
 		// update the URI with the new components
 		// only after building them
 		if pathStart >= 0 {
-			r.URL.Path = newPath
+			if path, err := url.PathUnescape(newPath); err != nil {
+				r.URL.Path = newPath
+			} else {
+				r.URL.Path = path
+			}
 		}
 		if qsStart >= 0 {
 			r.URL.RawQuery = newQuery


Test patch:
diff --git a/modules/caddyhttp/rewrite/rewrite_test.go b/modules/caddyhttp/rewrite/rewrite_test.go
index 5875983b6da..bb937ec5bad 100644
--- a/modules/caddyhttp/rewrite/rewrite_test.go
+++ b/modules/caddyhttp/rewrite/rewrite_test.go
@@ -59,6 +59,16 @@ func TestRewrite(t *testing.T) {
 			input:  newRequest(t, "GET", "/"),
 			expect: newRequest(t, "GET", "foo"),
 		},
+		{
+			rule:   Rewrite{URI: "{http.request.uri}"},
+			input:  newRequest(t, "GET", "/bar%3Fbaz?c=d"),
+			expect: newRequest(t, "GET", "/bar%3Fbaz?c=d"),
+		},
+		{
+			rule:   Rewrite{URI: "{http.request.uri.path}"},
+			input:  newRequest(t, "GET", "/bar%3Fbaz"),
+			expect: newRequest(t, "GET", "/bar%3Fbaz"),
+		},
 		{
 			rule:   Rewrite{URI: "/foo{http.request.uri.path}"},
 			input:  newRequest(t, "GET", "/bar"),
@@ -323,7 +333,7 @@ func TestRewrite(t *testing.T) {
 			input:  newRequest(t, "GET", "/foo/findme%2Fbar"),
 			expect: newRequest(t, "GET", "/foo/replaced%2Fbar"),
 		},
-
+		
 		{
 			rule:   Rewrite{PathRegexp: []*regexReplacer{{Find: "/{2,}", Replace: "/"}}},
 			input:  newRequest(t, "GET", "/foo//bar///baz?a=b//c"),