From 63449f163647a5f5bf96755ccb6191a764095415 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Wed, 21 Aug 2024 18:15:32 +0200 Subject: [PATCH] fix: check permissions at copy/move source and destination (#181) --- lib/handler.go | 20 +++++++++----- lib/handler_test.go | 19 ++++++++++--- lib/permissions.go | 66 ++++++++++++++++++++++++++++++++++----------- 3 files changed, 81 insertions(+), 24 deletions(-) diff --git a/lib/handler.go b/lib/handler.go index 979e8cd..aef0238 100644 --- a/lib/handler.go +++ b/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. - allowed := user.Allowed(r, func(destination string) bool { + // Cleanup destination header if it's present by stripping out the prefix + // and only keeping the path. + if destination := r.Header.Get("Destination"); destination != "" { u, err := url.Parse(destination) - if err != nil { - return false + if err == nil { + 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) }) diff --git a/lib/handler_test.go b/lib/handler_test.go index c256916..1c8b133 100644 --- a/lib/handler_test.go +++ b/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") diff --git a/lib/permissions.go b/lib/permissions.go index 72daae3..57e5e91 100644 --- a/lib/permissions.go +++ b/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 { - // Go through rules beginning from the last one. - for i := len(p.Rules) - 1; i >= 0; i-- { - rule := p.Rules[i] +func (p UserPermissions) Allowed(r *http.Request, fileExists func(string) bool) bool { + // For COPY and MOVE requests, we first check the permissions for the destination + // 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.Header.Get("Destination") - if rule.Matches(r.URL.Path) { - return rule.Permissions.Allowed(r, destinationExists) + for i := len(p.Rules) - 1; i >= 0; i-- { + 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) { - return p.Update - } else { - return p.Create - } - case "COPY", "MOVE": - if destinationExists(r.Header.Get("Destination")) { + if fileExists(r.URL.Path) { 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 + } +}