caddyhttp: Refactor and export SanitizedPathJoin for use in fastcgi (#4207)

5 files changed, 6 insertions, 126 deletions

diff --git a/modules/caddyhttp/fileserver/browse.go b/modules/caddyhttp/fileserver/browse.go
index 3122f12..cd9bcbc 100644
--- a/modules/caddyhttp/fileserver/browse.go
+++ b/modules/caddyhttp/fileserver/browse.go
@@ -207,7 +207,7 @@ func isSymlinkTargetDir(f os.FileInfo, root, urlPath string) bool {
 	if !isSymlink(f) {
 		return false
 	}
-	target := sanitizedPathJoin(root, path.Join(urlPath, f.Name()))
+	target := caddyhttp.SanitizedPathJoin(root, path.Join(urlPath, f.Name()))
 	targetInfo, err := os.Stat(target)
 	if err != nil {
 		return false
diff --git a/modules/caddyhttp/fileserver/matcher.go b/modules/caddyhttp/fileserver/matcher.go
index 064398c..739b9e0 100644
--- a/modules/caddyhttp/fileserver/matcher.go
+++ b/modules/caddyhttp/fileserver/matcher.go
@@ -185,7 +185,7 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) {
 		if strings.HasSuffix(file, "/") {
 			suffix += "/"
 		}
-		fullpath = sanitizedPathJoin(root, suffix)
+		fullpath = caddyhttp.SanitizedPathJoin(root, suffix)
 		return
 	}
 
diff --git a/modules/caddyhttp/fileserver/matcher_test.go b/modules/caddyhttp/fileserver/matcher_test.go
index e319907..746a5d7 100644
--- a/modules/caddyhttp/fileserver/matcher_test.go
+++ b/modules/caddyhttp/fileserver/matcher_test.go
@@ -94,7 +94,7 @@ func TestFileMatcher(t *testing.T) {
 			t.Fatalf("Test %d: actual path: %v, expected: %v", i, rel, tc.expectedPath)
 		}
 
-		fileType, ok := repl.Get("http.matchers.file.type")
+		fileType, _ := repl.Get("http.matchers.file.type")
 		if fileType != tc.expectedType {
 			t.Fatalf("Test %d: actual file type: %v, expected: %v", i, fileType, tc.expectedType)
 		}
@@ -197,7 +197,7 @@ func TestPHPFileMatcher(t *testing.T) {
 			t.Fatalf("Test %d: actual path: %v, expected: %v", i, rel, tc.expectedPath)
 		}
 
-		fileType, ok := repl.Get("http.matchers.file.type")
+		fileType, _ := repl.Get("http.matchers.file.type")
 		if fileType != tc.expectedType {
 			t.Fatalf("Test %d: actual file type: %v, expected: %v", i, fileType, tc.expectedType)
 		}
diff --git a/modules/caddyhttp/fileserver/staticfiles.go b/modules/caddyhttp/fileserver/staticfiles.go
index 9266332..45f8610 100644
--- a/modules/caddyhttp/fileserver/staticfiles.go
+++ b/modules/caddyhttp/fileserver/staticfiles.go
@@ -161,7 +161,7 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
 	filesToHide := fsrv.transformHidePaths(repl)
 
 	root := repl.ReplaceAll(fsrv.Root, ".")
-	filename := sanitizedPathJoin(root, r.URL.Path)
+	filename := caddyhttp.SanitizedPathJoin(root, r.URL.Path)
 
 	fsrv.logger.Debug("sanitized path join",
 		zap.String("site_root", root),
@@ -185,7 +185,7 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
 	var implicitIndexFile bool
 	if info.IsDir() && len(fsrv.IndexNames) > 0 {
 		for _, indexPage := range fsrv.IndexNames {
-			indexPath := sanitizedPathJoin(filename, indexPage)
+			indexPath := caddyhttp.SanitizedPathJoin(filename, indexPage)
 			if fileHidden(indexPath, filesToHide) {
 				// pretend this file doesn't exist
 				fsrv.logger.Debug("hiding index file",
@@ -435,42 +435,6 @@ func (fsrv *FileServer) transformHidePaths(repl *caddy.Replacer) []string {
 	return hide
 }
 
-// sanitizedPathJoin performs filepath.Join(root, reqPath) that
-// is safe against directory traversal attacks. It uses logic
-// similar to that in the Go standard library, specifically
-// in the implementation of http.Dir. The root is assumed to
-// be a trusted path, but reqPath is not.
-func sanitizedPathJoin(root, reqPath string) string {
-	// TODO: Caddy 1 uses this:
-	// prevent absolute path access on Windows, e.g. http://localhost:5000/C:\Windows\notepad.exe
-	// if runtime.GOOS == "windows" && len(reqPath) > 0 && filepath.IsAbs(reqPath[1:]) {
-	// TODO.
-	// }
-
-	// TODO: whereas std lib's http.Dir.Open() uses this:
-	// if filepath.Separator != '/' && strings.ContainsRune(name, filepath.Separator) {
-	// 	return nil, errors.New("http: invalid character in file path")
-	// }
-
-	// TODO: see https://play.golang.org/p/oh77BiVQFti for another thing to consider
-
-	if root == "" {
-		root = "."
-	}
-
-	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 += separator
-	}
-
-	return path
-}
-
 // 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
diff --git a/modules/caddyhttp/fileserver/staticfiles_test.go b/modules/caddyhttp/fileserver/staticfiles_test.go
index 690f46a..5d6133c 100644
--- a/modules/caddyhttp/fileserver/staticfiles_test.go
+++ b/modules/caddyhttp/fileserver/staticfiles_test.go
@@ -15,96 +15,12 @@
 package fileserver
 
 import (
-	"net/url"
 	"path/filepath"
 	"runtime"
 	"strings"
 	"testing"
 )
 
-func TestSanitizedPathJoin(t *testing.T) {
-	// For easy reference:
-	// %2e = .
-	// %2f = /
-	// %5c = \
-	for i, tc := range []struct {
-		inputRoot string
-		inputPath string
-		expect    string
-	}{
-		{
-			inputPath: "",
-			expect:    ".",
-		},
-		{
-			inputPath: "/",
-			expect:    ".",
-		},
-		{
-			inputPath: "/foo",
-			expect:    "foo",
-		},
-		{
-			inputPath: "/foo/",
-			expect:    "foo" + separator,
-		},
-		{
-			inputPath: "/foo/bar",
-			expect:    filepath.Join("foo", "bar"),
-		},
-		{
-			inputRoot: "/a",
-			inputPath: "/foo/bar",
-			expect:    filepath.Join("/", "a", "foo", "bar"),
-		},
-		{
-			inputPath: "/foo/../bar",
-			expect:    "bar",
-		},
-		{
-			inputRoot: "/a/b",
-			inputPath: "/foo/../bar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
-		},
-		{
-			inputRoot: "/a/b",
-			inputPath: "/..%2fbar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
-		},
-		{
-			inputRoot: "/a/b",
-			inputPath: "/%2e%2e%2fbar",
-			expect:    filepath.Join("/", "a", "b", "bar"),
-		},
-		{
-			inputRoot: "/a/b",
-			inputPath: "/%2e%2e%2f%2e%2e%2f",
-			expect:    filepath.Join("/", "a", "b") + separator,
-		},
-		{
-			inputRoot: "C:\\www",
-			inputPath: "/foo/bar",
-			expect:    filepath.Join("C:\\www", "foo", "bar"),
-		},
-		// TODO: test more windows paths... on windows... sigh.
-	} {
-		// we don't *need* to use an actual parsed URL, but it
-		// adds some authenticity to the tests since real-world
-		// values will be coming in from URLs; thus, the test
-		// corpus can contain paths as encoded by clients, which
-		// more closely emulates the actual attack vector
-		u, err := url.Parse("http://test:9999" + tc.inputPath)
-		if err != nil {
-			t.Fatalf("Test %d: invalid URL: %v", i, err)
-		}
-		actual := sanitizedPathJoin(tc.inputRoot, u.Path)
-		if actual != tc.expect {
-			t.Errorf("Test %d: [%s %s] => %s (expected %s)",
-				i, tc.inputRoot, tc.inputPath, actual, tc.expect)
-		}
-	}
-}
-
 func TestFileHidden(t *testing.T) {
 	for i, tc := range []struct {
 		inputHide []string