Repository: caddyserver/caddy (language: go)

Issue:
the change to SanitizedPathJoin in v2.8.x can cause routing to break in certain configurations 
in previous versions, this would work to route an SPA. i got this through google result showing matt's post here:

https://caddy.community/t/how-to-serve-a-single-page-app-spa-with-caddy-2/12770

```
:3838 {
        root * dir
	try_files {path} /
	file_server
}
```

however, in v2.8.0, this breaks. and must be replaced with

```
:3838 {
        root * dir
	try_files {path} /index.html
	file_server
}
```


after a git bisect, i find the commit where the behavior changed to be 
```
6d97d8d87beb788d19a4084d07ec9157e5705b13 is the first bad commit
commit 6d97d8d87beb788d19a4084d07ec9157e5705b13
Author: Matt Holt <mholt@users.noreply.github.com>
Date:   Tue Apr 23 20:05:57 2024 -0400

    caddyhttp: Address some Go 1.20 features (#6252)

    Co-authored-by: Francis Lavoie <lavofr@gmail.com>

 cmd/x509rootsfallback.go                     | 33 ++++++++++++++++++++++++++++
 go.mod                                       |  1 +
 go.sum                                       |  2 ++
 modules/caddyhttp/caddyhttp.go               | 13 +++++++++--
 modules/caddyhttp/caddyhttp_test.go          | 26 ++++++++++++++++------
 modules/caddyhttp/requestbody/caddyfile.go   | 26 +++++++++++++++++++++-
 modules/caddyhttp/requestbody/requestbody.go | 30 +++++++++++++++++++++++++
 modules/caddyhttp/responsewriter_test.go     | 10 ++++-----
 8 files changed, 125 insertions(+), 16 deletions(-)
 create mode 100644 cmd/x509rootsfallback.go
```
doing some more digging, but i thought i would raise this asap since perhaps other people were surprised by this change. 


Discussion / hints:
my guess is it has something to do with the changes to SanitizedPathJoin

```
// with the local file system. If root is empty, the current
// directory is assumed. If the cleaned request path is deemed
// not local according to lexical processing (i.e. ignoring links),
// it will be rejected as unsafe and only the root will be returned.
```

after adding some debug logging, it seems that a different 'r.URL.Path' is being used in staticfiles.go:270
it seems the origin of the different r.URL.Path is in fileserver/matcher.go:365.

pre this commit, if i query for `:3838/dir/notexist`, i would get `dir/notexist` and `dir/`

after this change, i get `dir/notexist` and `dir`

this causes the different r.URL.Path down the line, as the fullpath in the matchcandidate is now different. 
@mholt we ran into this during smoke tests before production. 

is this change intended? we are changing all our configs to be explicitly `/index.html`, but for some reason i feel like this is a bug. i feel like putting `/` as a file to try should make it try all the index files in that dir. 
I think @elee1766 is definitely on the right track here.  The lack of trailing slash causes [this strictFileExists check](https://github.com/caddyserver/caddy/blob/40c582ce8273bf12b08e8e77df6498bca5f626a6/modules/caddyhttp/fileserver/matcher.go#L422) to fail since requests must have a trailing slash [if and only if](https://github.com/caddyserver/caddy/blob/40c582ce8273bf12b08e8e77df6498bca5f626a6/modules/caddyhttp/fileserver/matcher.go#L528-L535) they map to a directory.

The change that fixes this for me is to update SanitizePathJoin to consider a path unsafe only if the cleaned path is non-empty.  So update [this block](https://github.com/caddyserver/caddy/blob/40c582ce8273bf12b08e8e77df6498bca5f626a6/modules/caddyhttp/caddyhttp.go#L242-L245) to be:

``` go
	if relPath != "" && !filepath.IsLocal(relPath) {
		// path is unsafe (see https://github.com/golang/go/issues/56336#issuecomment-1416214885)
		return root
	}
```

I'm still walking through the code to verify that this makes sense and is the best place to fix it.  I'm not 100% convinced yet, but am getting closer.

Fix patch:
diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go
index f42b166cf5e..e1e71f4a0dc 100644
--- a/modules/caddyhttp/caddyhttp.go
+++ b/modules/caddyhttp/caddyhttp.go
@@ -239,7 +239,7 @@ func SanitizedPathJoin(root, reqPath string) string {
 	}
 
 	relPath := path.Clean("/" + reqPath)[1:] // clean path and trim the leading /
-	if !filepath.IsLocal(relPath) {
+	if relPath != "" && !filepath.IsLocal(relPath) {
 		// path is unsafe (see https://github.com/golang/go/issues/56336#issuecomment-1416214885)
 		return root
 	}


Test patch:
diff --git a/modules/caddyhttp/caddyhttp_test.go b/modules/caddyhttp/caddyhttp_test.go
index 4763c383e74..0062c6711ed 100644
--- a/modules/caddyhttp/caddyhttp_test.go
+++ b/modules/caddyhttp/caddyhttp_test.go
@@ -26,22 +26,28 @@ func TestSanitizedPathJoin(t *testing.T) {
 			inputPath: "/",
 			expect:    ".",
 		},
+		{
+			// fileserver.MatchFile passes an inputPath of "//" for some try_files values.
+			// See https://github.com/caddyserver/caddy/issues/6352
+			inputPath: "//",
+			expect:    filepath.FromSlash("./"),
+		},
 		{
 			inputPath: "/foo",
 			expect:    "foo",
 		},
 		{
 			inputPath: "/foo/",
-			expect:    "foo" + separator,
+			expect:    filepath.FromSlash("foo/"),
 		},
 		{
 			inputPath: "/foo/bar",
-			expect:    filepath.Join("foo", "bar"),
+			expect:    filepath.FromSlash("foo/bar"),
 		},
 		{
 			inputRoot: "/a",
 			inputPath: "/foo/bar",
-			expect:    filepath.Join("/", "a", "foo", "bar"),
+			expect:    filepath.FromSlash("/a/foo/bar"),
 		},
 		{
 			inputPath: "/foo/../bar",
@@ -50,32 +56,34 @@ func TestSanitizedPathJoin(t *testing.T) {
 		{
 			inputRoot: "/a/b",
 			inputPath: "/foo/../bar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
+			expect:    filepath.FromSlash("/a/b/bar"),
 		},
 		{
 			inputRoot: "/a/b",
 			inputPath: "/..%2fbar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
+			expect:    filepath.FromSlash("/a/b/bar"),
 		},
 		{
 			inputRoot: "/a/b",
 			inputPath: "/%2e%2e%2fbar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
+			expect:    filepath.FromSlash("/a/b/bar"),
 		},
 		{
+			// inputPath fails the IsLocal test so only the root is returned,
+			// but with a trailing slash since one was included in inputPath
 			inputRoot: "/a/b",
 			inputPath: "/%2e%2e%2f%2e%2e%2f",
-			expect:    "/a/b", // inputPath fails the IsLocal test so only the root is returned
+			expect:    filepath.FromSlash("/a/b/"),
 		},
 		{
 			inputRoot: "/a/b",
 			inputPath: "/foo%2fbar",
-			expect:    filepath.Join("/", "a", "b", "foo", "bar"),
+			expect:    filepath.FromSlash("/a/b/foo/bar"),
 		},
 		{
 			inputRoot: "/a/b",
 			inputPath: "/foo%252fbar",
-			expect:    filepath.Join("/", "a", "b", "foo%2fbar"),
+			expect:    filepath.FromSlash("/a/b/foo%2fbar"),
 		},
 		{
 			inputRoot: "C:\\www",
@@ -92,7 +100,7 @@ func TestSanitizedPathJoin(t *testing.T) {
 			// https://github.com/golang/go/issues/56336#issuecomment-1416214885
 			inputRoot: "root",
 			inputPath: "/a/b/../../c",
-			expect:    filepath.Join("root", "c"),
+			expect:    filepath.FromSlash("root/c"),
 		},
 	} {
 		// we don't *need* to use an actual parsed URL, but it