fix: check permissions at copy/move source and destination (#181)

lib/handler.go

@@ -107,14 +107,22 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		zap.L().Info("user authorized", zap.String("username", username))
 	}
 
-	// Checks for user permissions relatively to this PATH.
+	// Cleanup destination header if it's present by stripping out the prefix
-	allowed := user.Allowed(r, func(destination string) bool {
+	// and only keeping the path.
+	if destination := r.Header.Get("Destination"); destination != "" {
 		u, err := url.Parse(destination)
-		if err != nil {
+		if err == nil {
-			return false
+			destination = strings.TrimPrefix(u.Path, user.Prefix)
+			if !strings.HasPrefix(destination, "/") {
+				destination = "/" + destination
+			}
+			r.Header.Set("Destination", destination)
 		}
-		path := strings.TrimPrefix(u.Path, user.Prefix)
+	}
-		_, err = user.FileSystem.Stat(r.Context(), path)
+
+	// Checks for user permissions relatively to this PATH.
+	allowed := user.Allowed(r, func(filename string) bool {
+		_, err := user.FileSystem.Stat(r.Context(), filename)
 		return !os.IsNotExist(err)
 	})
 

lib/handler_test.go

@@ -222,6 +222,7 @@ func TestServerRules(t *testing.T) {
 
 	dir := makeTestDirectory(t, map[string][]byte{
 		"foo.txt":   []byte("foo"),
+		"bar.js":    []byte("foo js"),
 		"a/foo.js":  []byte("foo js"),
 		"a/foo.txt": []byte("foo txt"),
 		"b/foo.txt": []byte("foo b"),
@@ -240,11 +241,11 @@ users:
     rules:
     - regex: "^.+.js$"
       permissions: R
-    - path: "/b"
+    - path: "/b/"
       permissions: R
     - path: "/a/foo.txt"
       permissions: none
-    - path: "/c"
+    - path: "/c/"
       permissions: none
 `, dir))
 
@@ -252,7 +253,7 @@ users:
 
 	files, err := client.ReadDir("/")
 	require.NoError(t, err)
-	require.Len(t, files, 4)
+	require.Len(t, files, 5)
 
 	err = client.Write("/foo.txt", []byte("new"), 0666)
 	require.NoError(t, err)
@@ -260,6 +261,18 @@ users:
 	err = client.Write("/new.txt", []byte("new"), 0666)
 	require.NoError(t, err)
 
+	err = client.Copy("/bar.js", "/b/bar.js", false)
+	require.ErrorContains(t, err, "403")
+
+	err = client.Copy("/bar.js", "/bar.jsx", false)
+	require.NoError(t, err)
+
+	err = client.Copy("/b/foo.txt", "/foo1.txt", false)
+	require.NoError(t, err)
+
+	err = client.Rename("/b/foo.txt", "/foo2.txt", false)
+	require.ErrorContains(t, err, "403")
+
 	_, err = client.Read("/a/foo.txt")
 	require.ErrorContains(t, err, "403")
 

lib/permissions.go

@@ -39,17 +39,38 @@ type UserPermissions struct {
 }
 
 // Allowed checks if the user has permission to access a directory/file
-func (p UserPermissions) Allowed(r *http.Request, destinationExists func(string) bool) bool {
+func (p UserPermissions) Allowed(r *http.Request, fileExists func(string) bool) bool {
-	// Go through rules beginning from the last one.
+	// For COPY and MOVE requests, we first check the permissions for the destination
-	for i := len(p.Rules) - 1; i >= 0; i-- {
+	// path. As soon as a rule matches and does not allow the operation at the destination,
-		rule := p.Rules[i]
+	// we fail immediately. If no rule matches, we check the global permissions.
+	if r.Method == "COPY" || r.Method == "MOVE" {
+		dst := r.Header.Get("Destination")
 
-		if rule.Matches(r.URL.Path) {
+		for i := len(p.Rules) - 1; i >= 0; i-- {
-			return rule.Permissions.Allowed(r, destinationExists)
+			if p.Rules[i].Matches(dst) {
+				if !p.Rules[i].Permissions.AllowedDestination(r, fileExists) {
+					return false
+				}
+
+				// Only check the first rule that matches, similarly to the source rules.
+				break
+			}
+		}
+
+		if !p.Permissions.AllowedDestination(r, fileExists) {
+			return false
 		}
 	}
 
-	return p.Permissions.Allowed(r, destinationExists)
+	// Go through rules beginning from the last one, and check the permissions at
+	// the source. The first matched rule returns.
+	for i := len(p.Rules) - 1; i >= 0; i-- {
+		if p.Rules[i].Matches(r.URL.Path) {
+			return p.Rules[i].Permissions.Allowed(r, fileExists)
+		}
+	}
+
+	return p.Permissions.Allowed(r, fileExists)
 }
 
 func (p *UserPermissions) Validate() error {
@@ -100,7 +121,9 @@ func (p *Permissions) UnmarshalText(data []byte) error {
 	return nil
 }
 
-func (p Permissions) Allowed(r *http.Request, destinationExists func(string) bool) bool {
+// Allowed returns whether this permission set has permissions to execute this
+// request in the source directory. This applies to all requests with all methods.
+func (p Permissions) Allowed(r *http.Request, fileExists func(string) bool) bool {
 	switch r.Method {
 	case "GET", "HEAD", "OPTIONS", "POST", "PROPFIND":
 		// Note: POST backend implementation just returns the same thing as GET.
@@ -110,17 +133,15 @@ func (p Permissions) Allowed(r *http.Request, destinationExists func(string) boo
 	case "PROPPATCH":
 		return p.Update
 	case "PUT":
-		if destinationExists(r.URL.Path) {
+		if fileExists(r.URL.Path) {
-			return p.Update
-		} else {
-			return p.Create
-		}
-	case "COPY", "MOVE":
-		if destinationExists(r.Header.Get("Destination")) {
 			return p.Update
 		} else {
 			return p.Create
 		}
+	case "COPY":
+		return p.Read
+	case "MOVE":
+		return p.Read && p.Delete
 	case "DELETE":
 		return p.Delete
 	case "LOCK", "UNLOCK":
@@ -129,3 +150,18 @@ func (p Permissions) Allowed(r *http.Request, destinationExists func(string) boo
 		return false
 	}
 }
+
+// AllowedDestination returns whether this permissions set has permissions to execute this
+// request in the destination directory. This only applies for COPY and MOVE requests.
+func (p Permissions) AllowedDestination(r *http.Request, fileExists func(string) bool) bool {
+	switch r.Method {
+	case "COPY", "MOVE":
+		if fileExists(r.Header.Get("Destination")) {
+			return p.Update
+		} else {
+			return p.Create
+		}
+	default:
+		return false
+	}
+}