Take care of remaining TODOs in the browse responder

5 files changed, 31 insertions, 41 deletions

diff --git a/modules/caddyhttp/fileserver/browse.go b/modules/caddyhttp/fileserver/browse.go
index ed5ad42..9869801 100644
--- a/modules/caddyhttp/fileserver/browse.go
+++ b/modules/caddyhttp/fileserver/browse.go
@@ -21,6 +21,16 @@ type Browse struct {
 }
 
 func (fsrv *FileServer) serveBrowse(dirPath string, w http.ResponseWriter, r *http.Request) error {
+	// navigation on the client-side gets messed up if the
+	// URL doesn't end in a trailing slash because hrefs like
+	// "/b/c" on a path like "/a" end up going to "/b/c" instead
+	// of "/a/b/c" - so we have to redirect in this case
+	if !strings.HasSuffix(r.URL.Path, "/") {
+		r.URL.Path += "/"
+		http.Redirect(w, r, r.URL.String(), http.StatusMovedPermanently)
+		return nil
+	}
+
 	dir, err := fsrv.openFile(dirPath, w)
 	if err != nil {
 		return err
@@ -29,7 +39,8 @@ func (fsrv *FileServer) serveBrowse(dirPath string, w http.ResponseWriter, r *ht
 
 	repl := r.Context().Value(caddy2.ReplacerCtxKey).(caddy2.Replacer)
 
-	listing, err := fsrv.loadDirectoryContents(dir, r.URL.Path, repl)
+	// calling path.Clean here prevents weird breadcrumbs when URL paths are sketchy like /%2e%2e%2f
+	listing, err := fsrv.loadDirectoryContents(dir, path.Clean(r.URL.Path), repl)
 	switch {
 	case os.IsPermission(err):
 		return caddyhttp.Error(http.StatusForbidden, err)
@@ -58,21 +69,6 @@ func (fsrv *FileServer) serveBrowse(dirPath string, w http.ResponseWriter, r *ht
 	buf.WriteTo(w)
 
 	return nil
-
-	// TODO: Sigh... do we have to put this here?
-	// // Browsing navigation gets messed up if browsing a directory
-	// // that doesn't end in "/" (which it should, anyway)
-	// u := *r.URL
-	// if u.Path == "" {
-	// 	u.Path = "/"
-	// }
-	// if u.Path[len(u.Path)-1] != '/' {
-	// 	u.Path += "/"
-	// 	http.Redirect(w, r, u.String(), http.StatusMovedPermanently)
-	// 	return http.StatusMovedPermanently, nil
-	// }
-
-	// return b.ServeListing(w, r, requestedFilepath, bc)
 }
 
 func (fsrv *FileServer) loadDirectoryContents(dir *os.File, urlPath string, repl caddy2.Replacer) (browseListing, error) {
@@ -138,20 +134,16 @@ func isSymlink(f os.FileInfo) bool {
 	return f.Mode()&os.ModeSymlink != 0
 }
 
-// isSymlinkTargetDir return true if f's symbolic link target
-// is a directory. Return false if not a symbolic link.
-// TODO: Re-implement
-func isSymlinkTargetDir(f os.FileInfo, urlPath string) bool {
-	// if !isSymlink(f) {
-	// 	return false
-	// }
-
-	// // TODO: Ensure path is sanitized
-	// target:= path.Join(root, urlPath, f.Name()))
-	// targetInfo, err := os.Stat(target)
-	// if err != nil {
-	// 	return false
-	// }
-	// return targetInfo.IsDir()
-	return false
+// isSymlinkTargetDir returns true if f's symbolic link target
+// is a directory.
+func isSymlinkTargetDir(f os.FileInfo, root, urlPath string) bool {
+	if !isSymlink(f) {
+		return false
+	}
+	target := sanitizedPathJoin(root, path.Join(urlPath, f.Name()))
+	targetInfo, err := os.Stat(target)
+	if err != nil {
+		return false
+	}
+	return targetInfo.IsDir()
 }
diff --git a/modules/caddyhttp/fileserver/browselisting.go b/modules/caddyhttp/fileserver/browselisting.go
index e17b7ae..94705b3 100644
--- a/modules/caddyhttp/fileserver/browselisting.go
+++ b/modules/caddyhttp/fileserver/browselisting.go
@@ -28,7 +28,7 @@ func (fsrv *FileServer) directoryListing(files []os.FileInfo, canGoUp bool, urlP
 			continue
 		}
 
-		isDir := f.IsDir() || isSymlinkTargetDir(f, urlPath)
+		isDir := f.IsDir() || isSymlinkTargetDir(f, fsrv.Root, urlPath)
 
 		if isDir {
 			name += "/"
diff --git a/modules/caddyhttp/fileserver/matcher.go b/modules/caddyhttp/fileserver/matcher.go
index fd994d0..29805a2 100644
--- a/modules/caddyhttp/fileserver/matcher.go
+++ b/modules/caddyhttp/fileserver/matcher.go
@@ -3,7 +3,6 @@ package fileserver
 import (
 	"net/http"
 	"os"
-	"path/filepath"
 
 	"bitbucket.org/lightcodelabs/caddy2"
 	"bitbucket.org/lightcodelabs/caddy2/modules/caddyhttp"
@@ -33,8 +32,7 @@ type FileMatcher struct {
 
 // Match matches the request r against m.
 func (m FileMatcher) Match(r *http.Request) bool {
-	// TODO: sanitize path
-	fullPath := filepath.Join(m.Root, m.Path)
+	fullPath := sanitizedPathJoin(m.Root, m.Path)
 	var match bool
 	if len(m.Flags) > 0 {
 		match = true
diff --git a/modules/caddyhttp/fileserver/staticfiles.go b/modules/caddyhttp/fileserver/staticfiles.go
index 873c5fb..19f5f68 100644
--- a/modules/caddyhttp/fileserver/staticfiles.go
+++ b/modules/caddyhttp/fileserver/staticfiles.go
@@ -30,9 +30,9 @@ type FileServer struct {
 	Root            string              `json:"root"` // default is current directory
 	Hide            []string            `json:"hide"`
 	IndexNames      []string            `json:"index_names"`
-	Files           []string            `json:"files"`    // all relative to the root; default is request URI path
-	Rehandle        bool                `json:"rehandle"` // issue a rehandle (internal redirect) if request is rewritten
+	Files           []string            `json:"files"` // all relative to the root; default is request URI path
 	SelectionPolicy string              `json:"selection_policy"`
+	Rehandle        bool                `json:"rehandle"` // issue a rehandle (internal redirect) if request is rewritten
 	Fallback        caddyhttp.RouteList `json:"fallback"`
 	Browse          *Browse             `json:"browse"`
 	// TODO: Etag
diff --git a/modules/caddyhttp/matchers.go b/modules/caddyhttp/matchers.go
index 5eb0837..13e122a 100644
--- a/modules/caddyhttp/matchers.go
+++ b/modules/caddyhttp/matchers.go
@@ -26,7 +26,7 @@ type (
 	matchHeaderRE     map[string]*matchRegexp
 	matchProtocol     string
 	matchStarlarkExpr string
-	matchTable        string
+	matchTable        string // TODO: finish implementing
 )
 
 func init() {
@@ -245,14 +245,14 @@ func (mre *matchRegexp) match(input string, repl caddy2.Replacer, scope string)
 
 	// save all capture groups, first by index
 	for i, match := range matches {
-		key := fmt.Sprintf("matchers.%s.%s.%d", scope, mre.Name, i)
+		key := fmt.Sprintf("http.matchers.%s.%s.%d", scope, mre.Name, i)
 		repl.Set(key, match)
 	}
 
 	// then by name
 	for i, name := range mre.compiled.SubexpNames() {
 		if i != 0 && name != "" {
-			key := fmt.Sprintf("matchers.%s.%s.%s", scope, mre.Name, name)
+			key := fmt.Sprintf("http.matchers.%s.%s.%s", scope, mre.Name, name)
 			repl.Set(key, matches[i])
 		}
 	}