fileserver: Fix try_files for directories; windows fix (#3684)

* fileserver: Fix try_files for directories, windows fix

* fileserver: Add new file type placeholder, refactoring, tests

* fileserver: Review cleanup

* fileserver: Flip the return args order

modules/caddyhttp/fileserver/matcher.go

@@ -19,6 +19,7 @@ import (
 	"net/http"
 	"os"
 	"path"
+	"path/filepath"
 	"strings"
 	"time"
 
@@ -34,7 +35,7 @@ func init() {
 // MatchFile is an HTTP request matcher that can match
 // requests based upon file existence.
 //
-// Upon matching, two new placeholders will be made
+// Upon matching, three new placeholders will be made
 // available:
 //
 // - `{http.matchers.file.relative}` The root-relative
@@ -42,6 +43,8 @@ func init() {
 // requests.
 // - `{http.matchers.file.absolute}` The absolute path
 // of the matched file.
+// - `{http.matchers.file.type}` Set to "directory" if
+// the matched file is a directory, "file" otherwise.
 type MatchFile struct {
 	// The root directory, used for creating absolute
 	// file paths, and required when working with
@@ -153,25 +156,18 @@ func (m MatchFile) Validate() error {
 }
 
 // Match returns true if r matches m. Returns true
-// if a file was matched. If so, two placeholders
+// if a file was matched. If so, three placeholders
 // will be available:
 //    - http.matchers.file.relative
 //    - http.matchers.file.absolute
+//    - http.matchers.file.type
 func (m MatchFile) Match(r *http.Request) bool {
-	repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer)
+	return m.selectFile(r)
-	rel, abs, matched := m.selectFile(r)
-	if matched {
-		repl.Set("http.matchers.file.relative", rel)
-		repl.Set("http.matchers.file.absolute", abs)
-	}
-	return matched
 }
 
 // selectFile chooses a file according to m.TryPolicy by appending
 // the paths in m.TryFiles to m.Root, with placeholder replacements.
-// It returns the root-relative path to the matched file, the full
+func (m MatchFile) selectFile(r *http.Request) (matched bool) {
-// or absolute path, and whether a match was made.
-func (m MatchFile) selectFile(r *http.Request) (rel, abs string, matched bool) {
 	repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer)
 
 	root := repl.ReplaceAll(m.Root, ".")
@@ -183,13 +179,35 @@ func (m MatchFile) selectFile(r *http.Request) (rel, abs string, matched bool) {
 		m.TryFiles = []string{r.URL.Path}
 	}
 
+	// common preparation of the file into parts
+	prepareFilePath := func(file string) (string, string) {
+		suffix := m.firstSplit(path.Clean(repl.ReplaceAll(file, "")))
+		if strings.HasSuffix(file, "/") {
+			suffix += "/"
+		}
+		fullpath := sanitizedPathJoin(root, suffix)
+		return suffix, fullpath
+	}
+
+	// sets up the placeholders for the matched file
+	setPlaceholders := func(info os.FileInfo, rel string, abs string) {
+		repl.Set("http.matchers.file.relative", rel)
+		repl.Set("http.matchers.file.absolute", abs)
+
+		fileType := "file"
+		if info.IsDir() {
+			fileType = "directory"
+		}
+		repl.Set("http.matchers.file.type", fileType)
+	}
+
 	switch m.TryPolicy {
 	case "", tryPolicyFirstExist:
 		for _, f := range m.TryFiles {
-			suffix := m.firstSplit(path.Clean(repl.ReplaceAll(f, "")))
+			suffix, fullpath := prepareFilePath(f)
-			fullpath := sanitizedPathJoin(root, suffix)
+			if info, exists := strictFileExists(fullpath); exists {
-			if strictFileExists(fullpath) {
+				setPlaceholders(info, suffix, fullpath)
-				return suffix, fullpath, true
+				return true
 			}
 		}
 
@@ -197,9 +215,9 @@ func (m MatchFile) selectFile(r *http.Request) (rel, abs string, matched bool) {
 		var largestSize int64
 		var largestFilename string
 		var largestSuffix string
+		var info os.FileInfo
 		for _, f := range m.TryFiles {
-			suffix := m.firstSplit(path.Clean(repl.ReplaceAll(f, "")))
+			suffix, fullpath := prepareFilePath(f)
-			fullpath := sanitizedPathJoin(root, suffix)
 			info, err := os.Stat(fullpath)
 			if err == nil && info.Size() > largestSize {
 				largestSize = info.Size()
@@ -207,15 +225,16 @@ func (m MatchFile) selectFile(r *http.Request) (rel, abs string, matched bool) {
 				largestSuffix = suffix
 			}
 		}
-		return largestSuffix, largestFilename, true
+		setPlaceholders(info, largestSuffix, largestFilename)
+		return true
 
 	case tryPolicySmallestSize:
 		var smallestSize int64
 		var smallestFilename string
 		var smallestSuffix string
+		var info os.FileInfo
 		for _, f := range m.TryFiles {
-			suffix := m.firstSplit(path.Clean(repl.ReplaceAll(f, "")))
+			suffix, fullpath := prepareFilePath(f)
-			fullpath := sanitizedPathJoin(root, suffix)
 			info, err := os.Stat(fullpath)
 			if err == nil && (smallestSize == 0 || info.Size() < smallestSize) {
 				smallestSize = info.Size()
@@ -223,15 +242,16 @@ func (m MatchFile) selectFile(r *http.Request) (rel, abs string, matched bool) {
 				smallestSuffix = suffix
 			}
 		}
-		return smallestSuffix, smallestFilename, true
+		setPlaceholders(info, smallestSuffix, smallestFilename)
+		return true
 
 	case tryPolicyMostRecentlyMod:
 		var recentDate time.Time
 		var recentFilename string
 		var recentSuffix string
+		var info os.FileInfo
 		for _, f := range m.TryFiles {
-			suffix := m.firstSplit(path.Clean(repl.ReplaceAll(f, "")))
+			suffix, fullpath := prepareFilePath(f)
-			fullpath := sanitizedPathJoin(root, suffix)
 			info, err := os.Stat(fullpath)
 			if err == nil &&
 				(recentDate.IsZero() || info.ModTime().After(recentDate)) {
@@ -240,7 +260,8 @@ func (m MatchFile) selectFile(r *http.Request) (rel, abs string, matched bool) {
 				recentSuffix = suffix
 			}
 		}
-		return recentSuffix, recentFilename, true
+		setPlaceholders(info, recentSuffix, recentFilename)
+		return true
 	}
 
 	return
@@ -252,7 +273,7 @@ func (m MatchFile) selectFile(r *http.Request) (rel, abs string, matched bool) {
 // the file must also be a directory; if it does
 // NOT end in a forward slash, the file must NOT
 // be a directory.
-func strictFileExists(file string) bool {
+func strictFileExists(file string) (os.FileInfo, bool) {
 	stat, err := os.Stat(file)
 	if err != nil {
 		// in reality, this can be any error
@@ -263,16 +284,16 @@ func strictFileExists(file string) bool {
 		// the file exists, so we just treat any
 		// error as if it does not exist; see
 		// https://stackoverflow.com/a/12518877/1048862
-		return false
+		return nil, false
 	}
-	if strings.HasSuffix(file, "/") {
+	if strings.HasSuffix(file, string(filepath.Separator)) {
 		// by convention, file paths ending
-		// in a slash must be a directory
+		// in a path separator must be a directory
-		return stat.IsDir()
+		return stat, stat.IsDir()
 	}
 	// by convention, file paths NOT ending
-	// in a slash must NOT be a directory
+	// in a path separator must NOT be a directory
-	return !stat.IsDir()
+	return stat, !stat.IsDir()
 }
 
 // firstSplit returns the first result where the path

modules/caddyhttp/fileserver/matcher_test.go

@@ -22,30 +22,114 @@ import (
 	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
 )
 
+func TestFileMatcher(t *testing.T) {
+	for i, tc := range []struct {
+		path         string
+		expectedPath string
+		expectedType string
+		matched      bool
+	}{
+		{
+			path:         "/foo.txt",
+			expectedPath: "/foo.txt",
+			expectedType: "file",
+			matched:      true,
+		},
+		{
+			path:         "/foo.txt/",
+			expectedPath: "/foo.txt",
+			expectedType: "file",
+			matched:      true,
+		},
+		{
+			path:         "/foodir",
+			expectedPath: "/foodir/",
+			expectedType: "directory",
+			matched:      true,
+		},
+		{
+			path:         "/foodir/",
+			expectedPath: "/foodir/",
+			expectedType: "directory",
+			matched:      true,
+		},
+		{
+			path:         "/foodir/foo.txt",
+			expectedPath: "/foodir/foo.txt",
+			expectedType: "file",
+			matched:      true,
+		},
+		{
+			path:    "/missingfile.php",
+			matched: false,
+		},
+	} {
+		m := &MatchFile{
+			Root:     "./testdata",
+			TryFiles: []string{"{http.request.uri.path}", "{http.request.uri.path}/"},
+		}
+
+		u, err := url.Parse(tc.path)
+		if err != nil {
+			t.Fatalf("Test %d: parsing path: %v", i, err)
+		}
+
+		req := &http.Request{URL: u}
+		repl := caddyhttp.NewTestReplacer(req)
+
+		result := m.Match(req)
+		if result != tc.matched {
+			t.Fatalf("Test %d: expected match=%t, got %t", i, tc.matched, result)
+		}
+
+		rel, ok := repl.Get("http.matchers.file.relative")
+		if !ok && result {
+			t.Fatalf("Test %d: expected replacer value", i)
+		}
+		if !result {
+			continue
+		}
+
+		if rel != tc.expectedPath {
+			t.Fatalf("Test %d: actual path: %v, expected: %v", i, rel, tc.expectedPath)
+		}
+
+		fileType, ok := repl.Get("http.matchers.file.type")
+		if fileType != tc.expectedType {
+			t.Fatalf("Test %d: actual file type: %v, expected: %v", i, fileType, tc.expectedType)
+		}
+	}
+}
+
 func TestPHPFileMatcher(t *testing.T) {
 	for i, tc := range []struct {
 		path         string
 		expectedPath string
+		expectedType string
 		matched      bool
 	}{
 		{
 			path:         "/index.php",
 			expectedPath: "/index.php",
+			expectedType: "file",
 			matched:      true,
 		},
 		{
 			path:         "/index.php/somewhere",
 			expectedPath: "/index.php",
+			expectedType: "file",
 			matched:      true,
 		},
 		{
 			path:         "/remote.php",
 			expectedPath: "/remote.php",
+			expectedType: "file",
 			matched:      true,
 		},
 		{
 			path:         "/remote.php/somewhere",
 			expectedPath: "/remote.php",
+			expectedType: "file",
 			matched:      true,
 		},
 		{
@@ -55,11 +139,13 @@ func TestPHPFileMatcher(t *testing.T) {
 		{
 			path:         "/notphp.php.txt",
 			expectedPath: "/notphp.php.txt",
+			expectedType: "file",
 			matched:      true,
 		},
 		{
 			path:         "/notphp.php.txt/",
 			expectedPath: "/notphp.php.txt",
+			expectedType: "file",
 			matched:      true,
 		},
 		{
@@ -69,12 +155,14 @@ func TestPHPFileMatcher(t *testing.T) {
 		{
 			path:         "/foo.php.php/index.php",
 			expectedPath: "/foo.php.php/index.php",
+			expectedType: "file",
 			matched:      true,
 		},
 		{
 			// See https://github.com/caddyserver/caddy/issues/3623
 			path:         "/%E2%C3",
 			expectedPath: "/%E2%C3",
+			expectedType: "file",
 			matched:      false,
 		},
 	} {
@@ -108,6 +196,11 @@ func TestPHPFileMatcher(t *testing.T) {
 		if rel != tc.expectedPath {
 			t.Fatalf("Test %d: actual path: %v, expected: %v", i, rel, tc.expectedPath)
 		}
+
+		fileType, ok := repl.Get("http.matchers.file.type")
+		if fileType != tc.expectedType {
+			t.Fatalf("Test %d: actual file type: %v, expected: %v", i, fileType, tc.expectedType)
+		}
 	}
 }
 

modules/caddyhttp/fileserver/staticfiles.go

@@ -23,7 +23,6 @@ import (
 	"mime"
 	"net/http"
 	"os"
-	"path"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -323,7 +322,18 @@ func sanitizedPathJoin(root, reqPath string) string {
 	if root == "" {
 		root = "."
 	}
-	return filepath.Join(root, filepath.FromSlash(path.Clean("/"+reqPath)))
+
+	path := filepath.Join(root, filepath.Clean("/"+reqPath))
+
+	// filepath.Join also cleans the path, and cleaning strips
+	// the trailing slash, so we need to re-add it afterwards.
+	// 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)
+	}
+
+	return path
 }
 
 // fileHidden returns true if filename is hidden

modules/caddyhttp/fileserver/staticfiles_test.go

@@ -42,6 +42,10 @@ func TestSanitizedPathJoin(t *testing.T) {
 			inputPath: "/foo",
 			expect:    "foo",
 		},
+		{
+			inputPath: "/foo/",
+			expect:    "foo" + string(filepath.Separator),
+		},
 		{
 			inputPath: "/foo/bar",
 			expect:    filepath.Join("foo", "bar"),
@@ -73,7 +77,7 @@ func TestSanitizedPathJoin(t *testing.T) {
 		{
 			inputRoot: "/a/b",
 			inputPath: "/%2e%2e%2f%2e%2e%2f",
-			expect:    filepath.Join("/", "a", "b"),
+			expect:    filepath.Join("/", "a", "b") + string(filepath.Separator),
 		},
 		{
 			inputRoot: "C:\\www",

modules/caddyhttp/fileserver/testdata/foo.txt

@@ -0,0 +1 @@
+foo.txt

modules/caddyhttp/fileserver/testdata/foodir/foo.txt

@@ -0,0 +1 @@
+foodir/foo.txt