Merge commit from fork

* fix: resolve dot segments before checking path rules (GHSA-chxv-mvjv-f92j) * fix: match trailing-slash path rules against the bare collection * fix: match destination rules against the URL path when no prefix is set * fix: restrict collections named by a rule without granting access to them * docs: cleanup
2026-09-22 03:20:41 +08:00 · 2026-08-05 10:18:13 +02:00
parent c04649bf40
commit f869dd6276
5 changed files with 323 additions and 34 deletions
@@ -3,7 +3,6 @@ package lib
 import (
 	"net/http"
 	"os"
-	"strings"

 	"github.com/rs/cors"
 	"go.uber.org/zap"
@@ -167,8 +166,8 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	// 		collection resources.
 	//
 	// GET (or HEAD), when applied to collection, will return the same as PROPFIND method.
-	if (r.Method == "GET" || r.Method == "HEAD") && strings.HasPrefix(r.URL.Path, user.Prefix) {
-		info, err := user.FileSystem.Stat(r.Context(), strings.TrimPrefix(r.URL.Path, user.Prefix))
+	if r.Method == "GET" || r.Method == "HEAD" {
+		info, err := user.FileSystem.Stat(r.Context(), req.path)
 		if err == nil && info.IsDir() {
 			r.Method = "PROPFIND"

@@ -1012,3 +1012,243 @@ users:
 		require.ErrorContains(t, err, "403")
 	})
 }
+
+func TestServerRulesDotSegments(t *testing.T) {
+	t.Parallel()
+
+	// Sends a request with an unmodified request-target, since a WebDAV client
+	// would normalize the dot segments away before they reach the server.
+	do := func(t *testing.T, method, url string, header map[string]string) int {
+		t.Helper()
+
+		req, err := http.NewRequest(method, url, nil)
+		require.NoError(t, err)
+		for k, v := range header {
+			req.Header.Set(k, v)
+		}
+
+		resp, err := http.DefaultClient.Do(req)
+		require.NoError(t, err)
+		require.NoError(t, resp.Body.Close())
+		return resp.StatusCode
+	}
+
+	makeServer := func(t *testing.T) (*httptest.Server, string) {
+		dir := makeTestDirectory(t, map[string][]byte{
+			"public/pub.txt":  []byte("public"),
+			"secret/flag.txt": []byte("secret"),
+			"secret/keep.txt": []byte("keep"),
+		})
+
+		srv := makeTestServer(t, fmt.Sprintf(`
+directory: %s
+permissions: CRUD
+rules:
+  - path: "/secret/"
+    permissions: none
+`, dir))
+		t.Cleanup(srv.Close)
+		return srv, dir
+	}
+
+	t.Run("Source Path", func(t *testing.T) {
+		t.Parallel()
+		srv, _ := makeServer(t)
+
+		for _, path := range []string{
+			"/secret/flag.txt",
+			"/public/../secret/flag.txt",
+			"/public/%2e%2e/secret/flag.txt",
+			"/public/../secret/",
+			"/secret/.",
+			"/secret/%2e",
+		} {
+			require.Equal(t, http.StatusForbidden, do(t, "GET", srv.URL+path, nil), path)
+		}
+
+		require.Equal(t, http.StatusForbidden, do(t, "PUT", srv.URL+"/public/%2e%2e/secret/new.txt", nil))
+		require.Equal(t, http.StatusForbidden, do(t, "DELETE", srv.URL+"/public/%2e%2e/secret/keep.txt", nil))
+
+		// A request that resolves outside a rule must still succeed.
+		require.Equal(t, http.StatusOK, do(t, "GET", srv.URL+"/secret/../public/pub.txt", nil))
+	})
+
+	t.Run("Destination Header", func(t *testing.T) {
+		t.Parallel()
+		srv, _ := makeServer(t)
+
+		for _, destination := range []string{
+			srv.URL + "/public/%2e%2e/secret/moved.txt",
+			srv.URL + "/public/../secret/moved.txt",
+			"/public/%2e%2e/secret/moved.txt",
+		} {
+			code := do(t, "MOVE", srv.URL+"/public/pub.txt", map[string]string{
+				"Destination": destination,
+				"Overwrite":   "T",
+			})
+			require.Equal(t, http.StatusForbidden, code, destination)
+		}
+	})
+
+	t.Run("Regex Rule", func(t *testing.T) {
+		t.Parallel()
+
+		dir := makeTestDirectory(t, map[string][]byte{
+			"public/pub.txt":  []byte("public"),
+			"secret/flag.txt": []byte("secret"),
+		})
+
+		srv := makeTestServer(t, fmt.Sprintf(`
+directory: %s
+permissions: CRUD
+rules:
+  - regex: "^/secret/"
+    permissions: none
+`, dir))
+		defer srv.Close()
+
+		require.Equal(t, http.StatusForbidden, do(t, "GET", srv.URL+"/secret/flag.txt", nil))
+		require.Equal(t, http.StatusForbidden, do(t, "GET", srv.URL+"/public/%2e%2e/secret/flag.txt", nil))
+	})
+
+	t.Run("Directory Mounts", func(t *testing.T) {
+		t.Parallel()
+
+		alpha := makeTestDirectory(t, map[string][]byte{"a.txt": []byte("a")})
+		beta := makeTestDirectory(t, map[string][]byte{"b.txt": []byte("b")})
+
+		srv := makeTestServer(t, fmt.Sprintf(`
+permissions: CRUD
+directories:
+  - name: alpha
+    path: %s
+  - name: beta
+    path: %s
+rules:
+  - path: "/beta/"
+    permissions: none
+`, alpha, beta))
+		defer srv.Close()
+
+		require.Equal(t, http.StatusForbidden, do(t, "GET", srv.URL+"/beta/b.txt", nil))
+		require.Equal(t, http.StatusForbidden, do(t, "GET", srv.URL+"/alpha/%2e%2e/beta/b.txt", nil))
+		require.Equal(t, http.StatusOK, do(t, "GET", srv.URL+"/alpha/a.txt", nil))
+	})
+
+	t.Run("No Users", func(t *testing.T) {
+		t.Parallel()
+		srv, _ := makeServer(t)
+
+		// Without a users block no authentication runs, so the rule is the only
+		// access control there is.
+		require.Equal(t, http.StatusForbidden, do(t, "GET", srv.URL+"/public/%2e%2e/secret/flag.txt", nil))
+	})
+}
+
+func TestServerRulesBareCollection(t *testing.T) {
+	t.Parallel()
+
+	do := func(t *testing.T, method, url string) int {
+		t.Helper()
+
+		req, err := http.NewRequest(method, url, nil)
+		require.NoError(t, err)
+		req.Header.Set("Depth", "1")
+
+		resp, err := http.DefaultClient.Do(req)
+		require.NoError(t, err)
+		require.NoError(t, resp.Body.Close())
+		return resp.StatusCode
+	}
+
+	dir := makeTestDirectory(t, map[string][]byte{
+		"c/secret.txt": []byte("secret"),
+		"cd/open.txt":  []byte("open"),
+	})
+
+	srv := makeTestServer(t, fmt.Sprintf(`
+directory: %s
+permissions: CRUD
+rules:
+  - path: "/c/"
+    permissions: none
+`, dir))
+	defer srv.Close()
+
+	// A rule written "/c/" must also cover the collection named without the
+	// trailing slash, otherwise the denied directory can be listed or deleted.
+	for _, path := range []string{"/c/", "/c", "/c/secret.txt"} {
+		require.Equal(t, http.StatusForbidden, do(t, "PROPFIND", srv.URL+path), path)
+		require.Equal(t, http.StatusForbidden, do(t, "DELETE", srv.URL+path), path)
+	}
+
+	// A sibling whose name merely starts with the same characters is unaffected.
+	require.Equal(t, http.StatusMultiStatus, do(t, "PROPFIND", srv.URL+"/cd"))
+	require.Equal(t, http.StatusOK, do(t, "GET", srv.URL+"/cd/open.txt"))
+
+	// A rule governs the collection it names, but must not grant access to it
+	// that would not otherwise exist: removing "/pub" acts on the root, which
+	// the global permissions still deny.
+	grantDir := makeTestDirectory(t, map[string][]byte{"pub/x.txt": []byte("x")})
+	grantSrv := makeTestServer(t, fmt.Sprintf(`
+directory: %s
+permissions: none
+rules:
+  - path: "/pub/"
+    permissions: CRUD
+`, grantDir))
+	defer grantSrv.Close()
+
+	require.Equal(t, http.StatusNoContent, do(t, "DELETE", grantSrv.URL+"/pub/x.txt"))
+	require.Equal(t, http.StatusForbidden, do(t, "DELETE", grantSrv.URL+"/pub"))
+	require.Equal(t, http.StatusForbidden, do(t, "PROPFIND", grantSrv.URL+"/pub"))
+}
+
+func TestServerRulesEmptyPrefixDestination(t *testing.T) {
+	t.Parallel()
+
+	dir := makeTestDirectory(t, map[string][]byte{
+		"public/a.txt": []byte("a"),
+		"public/b.txt": []byte("b"),
+		"secret/x.txt": []byte("secret"),
+	})
+
+	srv := makeTestServer(t, fmt.Sprintf(`
+directory: %s
+prefix: ""
+permissions: CRUD
+rules:
+  - path: "/secret/"
+    permissions: none
+`, dir))
+	defer srv.Close()
+
+	// RFC 4918 has Destination as an absolute URI, so the host must not end up
+	// in the value the rules are matched against.
+	for name, destination := range map[string]string{
+		"absolute":  srv.URL + "/secret/moved.txt",
+		"bare path": "/secret/moved.txt",
+		"dot":       srv.URL + "/public/%2e%2e/secret/moved.txt",
+	} {
+		req, err := http.NewRequest("MOVE", srv.URL+"/public/a.txt", nil)
+		require.NoError(t, err)
+		req.Header.Set("Destination", destination)
+		req.Header.Set("Overwrite", "T")
+
+		resp, err := http.DefaultClient.Do(req)
+		require.NoError(t, err)
+		require.NoError(t, resp.Body.Close())
+		require.Equal(t, http.StatusForbidden, resp.StatusCode, name)
+	}
+
+	// A destination outside any rule must still work.
+	req, err := http.NewRequest("MOVE", srv.URL+"/public/b.txt", nil)
+	require.NoError(t, err)
+	req.Header.Set("Destination", srv.URL+"/public/moved.txt")
+	req.Header.Set("Overwrite", "T")
+
+	resp, err := http.DefaultClient.Do(req)
+	require.NoError(t, err)
+	require.NoError(t, resp.Body.Close())
+	require.Equal(t, http.StatusCreated, resp.StatusCode)
+}
@@ -35,6 +35,17 @@ func (r *Rule) Matches(path string) bool {
 	return strings.HasPrefix(path, r.Path)
 }

+// matchesCollection checks if [Rule] names path as the collection it governs,
+// such as a rule for "/c/" and a request for "/c". Regex rules are matched
+// literally and are not considered here.
+func (r *Rule) matchesCollection(path string) bool {
+	if r.Regex != nil || !strings.HasSuffix(r.Path, "/") {
+		return false
+	}
+
+	return path == strings.TrimSuffix(r.Path, "/")
+}
+
 type RulesBehavior string

 const (
@@ -67,35 +78,39 @@ func (p UserPermissions) Allowed(r *request, fileExists func(string) bool) bool
 	// path. As soon as a rule matches and does not allow the operation at the destination,
 	// we fail immediately. If no rule matches, we check the global permissions.
 	if r.method == "COPY" || r.method == "MOVE" {
-		dst := r.destination
-		ruleMatched := false
-
-		for i := len(p.Rules) - 1; i >= 0; i-- {
-			if p.Rules[i].Matches(dst) {
-				ruleMatched = true
-				if !p.Rules[i].Permissions.AllowedDestination(r, fileExists) {
-					return false
-				}
-
-				// Only check the first rule that matches, similarly to the source rules.
-				break
-			}
-		}
-
-		if !ruleMatched && !p.Permissions.AllowedDestination(r, fileExists) {
+		if !p.allowedAt(r.destination, func(perms Permissions) bool {
+			return perms.AllowedDestination(r, fileExists)
+		}) {
 			return false
 		}
 	}

-	// Go through rules beginning from the last one, and check the permissions at
-	// the source. The first matched rule returns.
+	return p.allowedAt(r.path, func(perms Permissions) bool {
+		return perms.Allowed(r, fileExists)
+	})
+}
+
+// allowedAt resolves the permissions that govern path and applies check to them.
+func (p UserPermissions) allowedAt(path string, check func(Permissions) bool) bool {
+	// Go through rules beginning from the last one. The first matched rule returns.
 	for i := len(p.Rules) - 1; i >= 0; i-- {
-		if p.Rules[i].Matches(r.path) {
-			return p.Rules[i].Permissions.Allowed(r, fileExists)
+		if p.Rules[i].Matches(path) {
+			return check(p.Rules[i].Permissions)
 		}
 	}

-	return p.Permissions.Allowed(r, fileExists)
+	// A rule written with a trailing slash also governs the collection it names,
+	// so that a rule for "/c/" cannot be evaded by asking for "/c". Such a request
+	// acts on an entry of the parent collection, so it needs the permissions that
+	// apply there too. Requiring both means the rule can restrict the collection
+	// without granting access that would otherwise not exist.
+	for i := len(p.Rules) - 1; i >= 0; i-- {
+		if p.Rules[i].matchesCollection(path) {
+			return check(p.Rules[i].Permissions) && check(p.Permissions)
+		}
+	}
+
+	return check(p.Permissions)
 }

 func (p *UserPermissions) Validate() error {
@@ -4,9 +4,39 @@ import (
 	"errors"
 	"net/http"
 	"net/url"
+	"path"
 	"strings"
 )

+// cleanPath resolves dot segments so that the permission checks see the same
+// path that the backing file system will ultimately open. The file systems in
+// golang.org/x/net/webdav apply path.Clean before joining the backing
+// directory, so without this the two layers disagree on which file a request
+// names and a rule can be bypassed with e.g. "/public/../secret/file.txt".
+func cleanPath(p string) string {
+	if !strings.HasPrefix(p, "/") {
+		p = "/" + p
+	}
+
+	cleaned := path.Clean(p)
+
+	// path.Clean drops the trailing slash, but rules are prefix matches and are
+	// commonly written with one, such as "/c/". Dropping it would stop a request
+	// for the collection itself from matching the rule that names it.
+	if cleaned != "/" && isCollectionPath(p) {
+		cleaned += "/"
+	}
+
+	return cleaned
+}
+
+// isCollectionPath reports whether p names a collection rather than a resource
+// within it. Besides an explicit trailing slash, a trailing "." or ".." segment
+// also resolves to the collection itself.
+func isCollectionPath(p string) bool {
+	return strings.HasSuffix(p, "/") || strings.HasSuffix(p, "/.") || strings.HasSuffix(p, "/..")
+}
+
 type request struct {
 	method      string
 	path        string
@@ -24,6 +54,11 @@ func newRequest(r *http.Request, prefix string) (*request, error) {
 			return nil, errors.New("invalid destination header")
 		}

+		// RFC 4918, section 10.3, has Destination as an absolute URI, which is
+		// what clients send in practice. Only the path is relevant here, and
+		// taking it unconditionally keeps the host out of the matched value.
+		destination = u.Path
+
 		if prefix != "" {
 			destination = strings.TrimPrefix(u.Path, prefix)
 			if len(destination) >= len(u.Path) {
@@ -31,11 +66,7 @@ func newRequest(r *http.Request, prefix string) (*request, error) {
 			}
 		}

-		if !strings.HasPrefix(destination, "/") {
-			destination = "/" + destination
-		}
-
-		ctx.destination = destination
+		ctx.destination = cleanPath(destination)
 	}

 	path := r.URL.Path
@@ -47,11 +78,7 @@ func newRequest(r *http.Request, prefix string) (*request, error) {
 		}
 	}

-	if !strings.HasPrefix(path, "/") {
-		path = "/" + path
-	}
-
-	ctx.path = path
+	ctx.path = cleanPath(path)

 	return ctx, nil
 }