the change to SanitizedPathJoin in v2.8.x can cause routing to break in certain configurations

[elee1766]

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.

[elee1766]

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

[elee1766]

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.

[elee1766]

@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.

[willnorris]

I think @elee1766 is definitely on the right track here. The lack of trailing slash causes this strictFileExists check to fail since requests must have a trailing slash if and only if 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 to be:

	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.

[mholt]

Will's patch is elegant and simple. I'll tag a release momentarily.