From 8d038ca515ffaa4d37dca05a95181203b2db64e7 Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Mon, 2 Nov 2020 14:20:12 -0700 Subject: [PATCH] fileserver: Improve and clarify file hiding logic (#3844) * fileserver: Improve and clarify file hiding logic * Oops, forgot to run integration tests * Make this one integration test OS-agnostic * See if this appeases the Windows gods * D'oh --- ...sort_directives_with_any_matcher_first.txt | 16 ++-- modules/caddyhttp/fileserver/caddyfile.go | 8 ++ modules/caddyhttp/fileserver/matcher.go | 3 +- modules/caddyhttp/fileserver/staticfiles.go | 81 ++++++++++++++----- .../caddyhttp/fileserver/staticfiles_test.go | 50 ++++++++++-- 5 files changed, 119 insertions(+), 39 deletions(-) diff --git a/caddytest/integration/caddyfile_adapt/sort_directives_with_any_matcher_first.txt b/caddytest/integration/caddyfile_adapt/sort_directives_with_any_matcher_first.txt index d15f56bf..6203a89c 100644 --- a/caddytest/integration/caddyfile_adapt/sort_directives_with_any_matcher_first.txt +++ b/caddytest/integration/caddyfile_adapt/sort_directives_with_any_matcher_first.txt @@ -1,9 +1,9 @@ :80 -file_server +respond 200 @untrusted not remote_ip 10.1.1.0/24 -file_server @untrusted +respond @untrusted 401 ---------- { "apps": { @@ -30,20 +30,16 @@ file_server @untrusted ], "handle": [ { - "handler": "file_server", - "hide": [ - "Caddyfile" - ] + "handler": "static_response", + "status_code": 401 } ] }, { "handle": [ { - "handler": "file_server", - "hide": [ - "Caddyfile" - ] + "handler": "static_response", + "status_code": 200 } ] } diff --git a/modules/caddyhttp/fileserver/caddyfile.go b/modules/caddyhttp/fileserver/caddyfile.go index 9b458b2f..3acbfa94 100644 --- a/modules/caddyhttp/fileserver/caddyfile.go +++ b/modules/caddyhttp/fileserver/caddyfile.go @@ -15,6 +15,7 @@ package fileserver import ( + "path/filepath" "strings" "github.com/caddyserver/caddy/v2" @@ -85,7 +86,14 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error) // hide the Caddyfile (and any imported Caddyfiles) if configFiles := h.Caddyfiles(); len(configFiles) > 0 { for _, file := range configFiles { + file = filepath.Clean(file) if !fileHidden(file, fsrv.Hide) { + // if there's no path separator, the file server module will hide all + // files by that name, rather than a specific one; but we want to hide + // only this specific file, so ensure there's always a path separator + if !strings.Contains(file, separator) { + file = "." + separator + file + } fsrv.Hide = append(fsrv.Hide, file) } } diff --git a/modules/caddyhttp/fileserver/matcher.go b/modules/caddyhttp/fileserver/matcher.go index 475a9ee1..c103b033 100644 --- a/modules/caddyhttp/fileserver/matcher.go +++ b/modules/caddyhttp/fileserver/matcher.go @@ -19,7 +19,6 @@ import ( "net/http" "os" "path" - "path/filepath" "strings" "time" @@ -286,7 +285,7 @@ func strictFileExists(file string) (os.FileInfo, bool) { // https://stackoverflow.com/a/12518877/1048862 return nil, false } - if strings.HasSuffix(file, string(filepath.Separator)) { + if strings.HasSuffix(file, separator) { // by convention, file paths ending // in a path separator must be a directory return stat, stat.IsDir() diff --git a/modules/caddyhttp/fileserver/staticfiles.go b/modules/caddyhttp/fileserver/staticfiles.go index 2a7e0138..b9f2b23d 100644 --- a/modules/caddyhttp/fileserver/staticfiles.go +++ b/modules/caddyhttp/fileserver/staticfiles.go @@ -46,7 +46,20 @@ type FileServer struct { Root string `json:"root,omitempty"` // A list of files or folders to hide; the file server will pretend as if - // they don't exist. Accepts globular patterns like "*.hidden" or "/foo/*/bar". + // they don't exist. Accepts globular patterns like "*.ext" or "/foo/*/bar" + // as well as placeholders. Because site roots can be dynamic, this list + // uses file system paths, not request paths. To clarify, the base of + // relative paths is the current working directory, NOT the site root. + // + // Entries without a path separator (`/` or `\` depending on OS) will match + // any file or directory of that name regardless of its path. To hide only a + // specific file with a name that may not be unique, always use a path + // separator. For example, to hide all files or folder trees named "hidden", + // put "hidden" in the list. To hide only ./hidden, put "./hidden" in the list. + // + // When possible, all paths are resolved to their absolute form before + // comparisons are made. For maximum clarity and explictness, use complete, + // absolute paths; or, for greater portability, use relative paths instead. Hide []string `json:"hide,omitempty"` // The names of files to try as index files if a folder is requested. @@ -101,6 +114,16 @@ func (fsrv *FileServer) Provision(ctx caddy.Context) error { fsrv.Browse.template = tpl } + // for hide paths that are static (i.e. no placeholders), we can transform them into + // absolute paths before the server starts for very slight performance improvement + for i, h := range fsrv.Hide { + if !strings.Contains(h, "{") && strings.Contains(h, separator) { + if abs, err := filepath.Abs(h); err == nil { + fsrv.Hide[i] = abs + } + } + } + return nil } @@ -225,7 +248,7 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c } } w.WriteHeader(statusCode) - if r.Method != "HEAD" { + if r.Method != http.MethodHead { io.Copy(w, file) } return nil @@ -273,12 +296,12 @@ func mapDirOpenError(originalErr error, name string) error { return originalErr } - parts := strings.Split(name, string(filepath.Separator)) + parts := strings.Split(name, separator) for i := range parts { if parts[i] == "" { continue } - fi, err := os.Stat(strings.Join(parts[:i+1], string(filepath.Separator))) + fi, err := os.Stat(strings.Join(parts[:i+1], separator)) if err != nil { return originalErr } @@ -290,12 +313,19 @@ func mapDirOpenError(originalErr error, name string) error { return originalErr } -// transformHidePaths performs replacements for all the elements of -// fsrv.Hide and returns a new list of the transformed values. +// transformHidePaths performs replacements for all the elements of fsrv.Hide and +// makes them absolute paths (if they contain a path separator), then returns a +// new list of the transformed values. func (fsrv *FileServer) transformHidePaths(repl *caddy.Replacer) []string { hide := make([]string, len(fsrv.Hide)) for i := range fsrv.Hide { hide[i] = repl.ReplaceAll(fsrv.Hide[i], "") + if strings.Contains(hide[i], separator) { + abs, err := filepath.Abs(hide[i]) + if err == nil { + hide[i] = abs + } + } } return hide } @@ -330,40 +360,50 @@ func sanitizedPathJoin(root, reqPath string) string { // if the length is 1, then it's a path to the root, // and that should return ".", so we don't append the separator. if strings.HasSuffix(reqPath, "/") && len(reqPath) > 1 { - path += string(filepath.Separator) + path += separator } return path } -// fileHidden returns true if filename is hidden -// according to the hide list. +// fileHidden returns true if filename is hidden according to the hide list. +// filename must be a relative or absolute file system path, not a request +// URI path. It is expected that all the paths in the hide list are absolute +// paths or are singular filenames (without a path separator). func fileHidden(filename string, hide []string) bool { - sep := string(filepath.Separator) + if len(hide) == 0 { + return false + } + + // all path comparisons use the complete absolute path if possible + filenameAbs, err := filepath.Abs(filename) + if err == nil { + filename = filenameAbs + } + var components []string for _, h := range hide { - if !strings.Contains(h, sep) { + if !strings.Contains(h, separator) { // if there is no separator in h, then we assume the user // wants to hide any files or folders that match that // name; thus we have to compare against each component // of the filename, e.g. hiding "bar" would hide "/bar" // as well as "/foo/bar/baz" but not "/barstool". if len(components) == 0 { - components = strings.Split(filename, sep) + components = strings.Split(filename, separator) } for _, c := range components { - if c == h { + if hidden, _ := filepath.Match(h, c); hidden { return true } } } else if strings.HasPrefix(filename, h) { - // otherwise, if there is a separator in h, and - // filename is exactly prefixed with h, then we - // can do a prefix match so that "/foo" matches - // "/foo/bar" but not "/foobar". + // if there is a separator in h, and filename is exactly + // prefixed with h, then we can do a prefix match so that + // "/foo" matches "/foo/bar" but not "/foobar". withoutPrefix := strings.TrimPrefix(filename, h) - if strings.HasPrefix(withoutPrefix, sep) { + if strings.HasPrefix(withoutPrefix, separator) { return true } } @@ -414,7 +454,10 @@ var bufPool = sync.Pool{ }, } -const minBackoff, maxBackoff = 2, 5 +const ( + minBackoff, maxBackoff = 2, 5 + separator = string(filepath.Separator) +) // Interface guards var ( diff --git a/modules/caddyhttp/fileserver/staticfiles_test.go b/modules/caddyhttp/fileserver/staticfiles_test.go index 1bbd77b2..690f46ac 100644 --- a/modules/caddyhttp/fileserver/staticfiles_test.go +++ b/modules/caddyhttp/fileserver/staticfiles_test.go @@ -17,6 +17,8 @@ package fileserver import ( "net/url" "path/filepath" + "runtime" + "strings" "testing" ) @@ -44,7 +46,7 @@ func TestSanitizedPathJoin(t *testing.T) { }, { inputPath: "/foo/", - expect: "foo" + string(filepath.Separator), + expect: "foo" + separator, }, { inputPath: "/foo/bar", @@ -77,7 +79,7 @@ func TestSanitizedPathJoin(t *testing.T) { { inputRoot: "/a/b", inputPath: "/%2e%2e%2f%2e%2e%2f", - expect: filepath.Join("/", "a", "b") + string(filepath.Separator), + expect: filepath.Join("/", "a", "b") + separator, }, { inputRoot: "C:\\www", @@ -154,6 +156,26 @@ func TestFileHidden(t *testing.T) { inputPath: "/foo/asdf/bar", expect: true, }, + { + inputHide: []string{"*.txt"}, + inputPath: "/foo/bar.txt", + expect: true, + }, + { + inputHide: []string{"/foo/bar/*.txt"}, + inputPath: "/foo/bar/baz.txt", + expect: true, + }, + { + inputHide: []string{"/foo/bar/*.txt"}, + inputPath: "/foo/bar.txt", + expect: false, + }, + { + inputHide: []string{"/foo/bar/*.txt"}, + inputPath: "/foo/bar/index.html", + expect: false, + }, { inputHide: []string{"/foo"}, inputPath: "/foo", @@ -164,17 +186,29 @@ func TestFileHidden(t *testing.T) { inputPath: "/foobar", expect: false, }, + { + inputHide: []string{"first", "second"}, + inputPath: "/second", + expect: true, + }, } { - // for Windows' sake - tc.inputPath = filepath.FromSlash(tc.inputPath) - for i := range tc.inputHide { - tc.inputHide[i] = filepath.FromSlash(tc.inputHide[i]) + if runtime.GOOS == "windows" { + if strings.HasPrefix(tc.inputPath, "/") { + tc.inputPath, _ = filepath.Abs(tc.inputPath) + } + tc.inputPath = filepath.FromSlash(tc.inputPath) + for i := range tc.inputHide { + if strings.HasPrefix(tc.inputHide[i], "/") { + tc.inputHide[i], _ = filepath.Abs(tc.inputHide[i]) + } + tc.inputHide[i] = filepath.FromSlash(tc.inputHide[i]) + } } actual := fileHidden(tc.inputPath, tc.inputHide) if actual != tc.expect { - t.Errorf("Test %d: Is %s hidden in %v? Got %t but expected %t", - i, tc.inputPath, tc.inputHide, actual, tc.expect) + t.Errorf("Test %d: Does %v hide %s? Got %t but expected %t", + i, tc.inputHide, tc.inputPath, actual, tc.expect) } } }