caddyhttp: Test cases for %2F and %252F

[francislavoie]

As per the discussion in https://caddy.community/t/serving-static-files-with-encoding/22382, I just wanted to add a test case as a sanity check that %252F behaves correctly, i.e. it would result in %2F after being joined.

[mohammed90]

I love tests 💯

[francislavoie]

We still have more to discuss about this topic. I think we need to do something like this:

diff --git a/modules/caddyhttp/fileserver/staticfiles.go b/modules/caddyhttp/fileserver/staticfiles.go
index 1f0b6a5e..adc09d21 100644
--- a/modules/caddyhttp/fileserver/staticfiles.go
+++ b/modules/caddyhttp/fileserver/staticfiles.go
@@ -258,13 +258,20 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
                return caddyhttp.Error(http.StatusNotFound, fmt.Errorf("filesystem not found"))
        }
 
+       // we want the raw path without URL decoding, because
+       // target filenames may contain %2F and such
+       rawPath := r.URL.Path
+       if r.URL.RawPath != "" {
+               rawPath = r.URL.RawPath
+       }
+
        // remove any trailing `/` as it breaks fs.ValidPath() in the stdlib
-       filename := strings.TrimSuffix(caddyhttp.SanitizedPathJoin(root, r.URL.Path), "/")
+       filename := strings.TrimSuffix(caddyhttp.SanitizedPathJoin(root, rawPath), "/")
 
        fsrv.logger.Debug("sanitized path join",
                zap.String("site_root", root),
                zap.String("fs", fsName),
-               zap.String("request_path", r.URL.Path),
+               zap.String("request_path", rawPath),
                zap.String("result", filename))
 
        // get information about the file

But if we do this, we also kinda need to take it a step further to adjust how the file matcher does it... problem with that though is the Caddy ecosystem has already "standardized" on using {path} everywhere which uses the "wrong" path for checking for existence of files.

The thought would be to make a new {rawpath} placeholder, but then we have to use {rawpath} instead of {path} everywhere... or we can change {path} to return the raw path instead, but that breaks usecases where the path isn't used for doing filesystem lookups.

I don't see a clean way to deal with it.

Some prior art #5278 and #5504, we merged in a hack in rewrite that bypasses the placeholder if {path} is given and instead calls r.URL.EscapedPath(). That approach was needed because sometimes %2E aka ? could be in the path and we need to handle that in a special way.

[mholt]

We just need to be very careful when dealing with encoding-related patches...

{rawpath} might be an option worth exploring.

[francislavoie]

Yeah, I'm always 11/10 nervous about making changes to that stuff.

The trouble IMO is try_files is used both for rewrites and for file matching. Rewrites may be proxied to a backend, file matching typically won't. But we haven't clearly separated those in how they're used, so it's really messy. We can't easily just change all {path} to {rawpath} etc.

[mholt]

True, that directive is overloaded. So we'll need to use discretion when replacing {path} with {rawpath}.