fileserver: Use EscapedPath for browse (#5534)

* fileserver: Use EscapedPath for browse Fix #5143 * Fixes if filter element is not present * Remove extraneous line
caddyserver · May 15, 2023 · 52d7335 · 52d7335
1 parent 96919ac
commit 52d7335
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 7 deletions.
diff --git a/modules/caddyhttp/fileserver/browse.go b/modules/caddyhttp/fileserver/browse.go
@@ -82,8 +82,8 @@ func (fsrv *FileServer) serveBrowse(root, dirPath string, w http.ResponseWriter,
 
 	repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer)
 
-	// calling path.Clean here prevents weird breadcrumbs when URL paths are sketchy like /%2e%2e%2f
-	listing, err := fsrv.loadDirectoryContents(r.Context(), dir.(fs.ReadDirFile), root, path.Clean(r.URL.Path), repl)
+	// TODO: not entirely sure if path.Clean() is necessary here but seems like a safe plan (i.e. /%2e%2e%2f) - someone could verify this
+	listing, err := fsrv.loadDirectoryContents(r.Context(), dir.(fs.ReadDirFile), root, path.Clean(r.URL.EscapedPath()), repl)
 	switch {
 	case os.IsPermission(err):
 		return caddyhttp.Error(http.StatusForbidden, err)

diff --git a/modules/caddyhttp/fileserver/browse.html b/modules/caddyhttp/fileserver/browse.html
@@ -850,11 +850,11 @@ <h1>
 
 		<script>
 			const filterEl = document.getElementById('filter');
-			filterEl.focus({ preventScroll: true });
+			filterEl?.focus({ preventScroll: true });
 
 			function initPage() {
 				// populate and evaluate filter
-				if (!filterEl.value) {
+				if (!filterEl?.value) {
 					const filterParam = new URL(window.location.href).searchParams.get('filter');
 					if (filterParam) {
 						filterEl.value = filterParam;
@@ -874,6 +874,7 @@ <h1>
 			}
 
 			function filter() {
+				if (!filterEl) return;
 				const q = filterEl.value.trim().toLowerCase();
 				document.querySelectorAll('tr.file').forEach(function(el) {
 					if (!q) {

diff --git a/modules/caddyhttp/fileserver/browsetplcontext_test.go b/modules/caddyhttp/fileserver/browsetplcontext_test.go
@@ -25,6 +25,45 @@ func TestBreadcrumbs(t *testing.T) {
 	}{
 		{"", []crumb{}},
 		{"/", []crumb{{Text: "/"}}},
+		{"/foo/", []crumb{
+			{Link: "../", Text: "/"},
+			{Link: "", Text: "foo"},
+		}},
+		{"/foo/bar/", []crumb{
+			{Link: "../../", Text: "/"},
+			{Link: "../", Text: "foo"},
+			{Link: "", Text: "bar"},
+		}},
+		{"/foo bar/", []crumb{
+			{Link: "../", Text: "/"},
+			{Link: "", Text: "foo bar"},
+		}},
+		{"/foo bar/baz/", []crumb{
+			{Link: "../../", Text: "/"},
+			{Link: "../", Text: "foo bar"},
+			{Link: "", Text: "baz"},
+		}},
+		{"/100%25 test coverage/is a lie/", []crumb{
+			{Link: "../../", Text: "/"},
+			{Link: "../", Text: "100% test coverage"},
+			{Link: "", Text: "is a lie"},
+		}},
+		{"/AC%2FDC/", []crumb{
+			{Link: "../", Text: "/"},
+			{Link: "", Text: "AC/DC"},
+		}},
+		{"/foo/%2e%2e%2f/bar", []crumb{
+			{Link: "../../../", Text: "/"},
+			{Link: "../../", Text: "foo"},
+			{Link: "../", Text: "../"},
+			{Link: "", Text: "bar"},
+		}},
+		{"/foo/../bar", []crumb{
+			{Link: "../../../", Text: "/"},
+			{Link: "../../", Text: "foo"},
+			{Link: "../", Text: ".."},
+			{Link: "", Text: "bar"},
+		}},
 		{"foo/bar/baz", []crumb{
 			{Link: "../../", Text: "foo"},
 			{Link: "../", Text: "bar"},
@@ -51,16 +90,16 @@ func TestBreadcrumbs(t *testing.T) {
 		}},
 	}
 
-	for _, d := range testdata {
+	for testNum, d := range testdata {
 		l := browseTemplateContext{Path: d.path}
 		actual := l.Breadcrumbs()
 		if len(actual) != len(d.expected) {
-			t.Errorf("wrong size output, got %d elements but expected %d", len(actual), len(d.expected))
+			t.Errorf("Test %d: Got %d components but expected %d; got: %+v", testNum, len(actual), len(d.expected), actual)
 			continue
 		}
 		for i, c := range actual {
 			if c != d.expected[i] {
-				t.Errorf("got %#v but expected %#v at index %d", c, d.expected[i], i)
+				t.Errorf("Test %d crumb %d: got %#v but expected %#v at index %d", testNum, i, c, d.expected[i], i)
 			}
 		}
 	}