From 1fdbd71f22e3c69a4ea12ff2534387a5463c7f98 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Mon, 19 Aug 2024 19:33:50 +0200 Subject: [PATCH 1/3] feat: add new tests that should fail --- lib/handler_test.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) 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") From c74ea19ec9df43430ac86929f66bc0f88798debb Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Mon, 19 Aug 2024 19:33:58 +0200 Subject: [PATCH 2/3] wip: fix --- lib/handler.go | 10 ++---- lib/permissions.go | 79 +++++++++++++++++++++++++++++++++++++--------- 2 files changed, 66 insertions(+), 23 deletions(-) diff --git a/lib/handler.go b/lib/handler.go index 979e8cd..800f976 100644 --- a/lib/handler.go +++ b/lib/handler.go @@ -2,7 +2,6 @@ package lib import ( "net/http" - "net/url" "os" "strings" @@ -108,13 +107,8 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } // Checks for user permissions relatively to this PATH. - allowed := user.Allowed(r, func(destination string) bool { - u, err := url.Parse(destination) - if err != nil { - return false - } - path := strings.TrimPrefix(u.Path, user.Prefix) - _, err = user.FileSystem.Stat(r.Context(), path) + allowed := user.Allowed(r, user.Prefix, func(filename string) bool { + _, err := user.FileSystem.Stat(r.Context(), filename) return !os.IsNotExist(err) }) diff --git a/lib/permissions.go b/lib/permissions.go index 72daae3..bbc2c16 100644 --- a/lib/permissions.go +++ b/lib/permissions.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "path/filepath" "regexp" "strings" @@ -39,17 +40,50 @@ 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, prefix string, 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" { + u, err := url.Parse(r.Header.Get("Destination")) + if err != nil { + return false + } + dst := strings.TrimPrefix(u.Path, prefix) + if !strings.HasPrefix(dst, "/") { + dst = "/" + dst + } + + fmt.Println(dst) + + for i := len(p.Rules) - 1; i >= 0; i-- { + if p.Rules[i].Matches(dst) { + if !p.Rules[i].Permissions.AllowedDestination(r.Method, dst, fileExists) { + fmt.Println("disallowed", p.Rules[i].Path) + return false + } + + // Only check the first rule that matches, similarly to the source rules. + break + } + } - if rule.Matches(r.URL.Path) { - return rule.Permissions.Allowed(r, destinationExists) + if !p.Permissions.AllowedDestination(r.Method, dst, fileExists) { + fmt.Println("disallowed") + 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.AllowedSource(r.Method, r.URL.Path, fileExists) + } + } + + return p.Permissions.AllowedSource(r.Method, r.URL.Path, fileExists) } func (p *UserPermissions) Validate() error { @@ -100,8 +134,10 @@ func (p *Permissions) UnmarshalText(data []byte) error { return nil } -func (p Permissions) Allowed(r *http.Request, destinationExists func(string) bool) bool { - switch r.Method { +// AllowedSource 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) AllowedSource(method, filename string, fileExists func(string) bool) bool { + switch method { case "GET", "HEAD", "OPTIONS", "POST", "PROPFIND": // Note: POST backend implementation just returns the same thing as GET. return p.Read @@ -110,21 +146,34 @@ 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(filename) { 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": + return p.Create || p.Read || p.Update || p.Delete + default: + 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(method, filename string, fileExists func(string) bool) bool { + switch method { case "COPY", "MOVE": - if destinationExists(r.Header.Get("Destination")) { + if fileExists(filename) { return p.Update } else { return p.Create } - case "DELETE": - return p.Delete - case "LOCK", "UNLOCK": - return p.Create || p.Read || p.Update || p.Delete default: return false } From a056e78477133630eef6a19e7ee8c9a7d1d1a0ad Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Wed, 21 Aug 2024 18:14:11 +0200 Subject: [PATCH 3/3] fix: make it non breaking --- lib/handler.go | 16 +++++++++++++++- lib/permissions.go | 39 +++++++++++++-------------------------- 2 files changed, 28 insertions(+), 27 deletions(-) diff --git a/lib/handler.go b/lib/handler.go index 800f976..aef0238 100644 --- a/lib/handler.go +++ b/lib/handler.go @@ -2,6 +2,7 @@ package lib import ( "net/http" + "net/url" "os" "strings" @@ -106,8 +107,21 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { zap.L().Info("user authorized", zap.String("username", username)) } + // 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 { + destination = strings.TrimPrefix(u.Path, user.Prefix) + if !strings.HasPrefix(destination, "/") { + destination = "/" + destination + } + r.Header.Set("Destination", destination) + } + } + // Checks for user permissions relatively to this PATH. - allowed := user.Allowed(r, user.Prefix, func(filename string) bool { + allowed := user.Allowed(r, func(filename string) bool { _, err := user.FileSystem.Stat(r.Context(), filename) return !os.IsNotExist(err) }) diff --git a/lib/permissions.go b/lib/permissions.go index bbc2c16..57e5e91 100644 --- a/lib/permissions.go +++ b/lib/permissions.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "net/http" - "net/url" "path/filepath" "regexp" "strings" @@ -40,27 +39,16 @@ type UserPermissions struct { } // Allowed checks if the user has permission to access a directory/file -func (p UserPermissions) Allowed(r *http.Request, prefix string, fileExists func(string) bool) bool { - +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" { - u, err := url.Parse(r.Header.Get("Destination")) - if err != nil { - return false - } - dst := strings.TrimPrefix(u.Path, prefix) - if !strings.HasPrefix(dst, "/") { - dst = "/" + dst - } - - fmt.Println(dst) + dst := r.Header.Get("Destination") for i := len(p.Rules) - 1; i >= 0; i-- { if p.Rules[i].Matches(dst) { - if !p.Rules[i].Permissions.AllowedDestination(r.Method, dst, fileExists) { - fmt.Println("disallowed", p.Rules[i].Path) + if !p.Rules[i].Permissions.AllowedDestination(r, fileExists) { return false } @@ -69,8 +57,7 @@ func (p UserPermissions) Allowed(r *http.Request, prefix string, fileExists func } } - if !p.Permissions.AllowedDestination(r.Method, dst, fileExists) { - fmt.Println("disallowed") + if !p.Permissions.AllowedDestination(r, fileExists) { return false } } @@ -79,11 +66,11 @@ func (p UserPermissions) Allowed(r *http.Request, prefix string, fileExists func // 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.AllowedSource(r.Method, r.URL.Path, fileExists) + return p.Rules[i].Permissions.Allowed(r, fileExists) } } - return p.Permissions.AllowedSource(r.Method, r.URL.Path, fileExists) + return p.Permissions.Allowed(r, fileExists) } func (p *UserPermissions) Validate() error { @@ -134,10 +121,10 @@ func (p *Permissions) UnmarshalText(data []byte) error { return nil } -// AllowedSource returns whether this permission set has permissions to execute this +// 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) AllowedSource(method, filename string, fileExists func(string) bool) bool { - switch method { +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. return p.Read @@ -146,7 +133,7 @@ func (p Permissions) AllowedSource(method, filename string, fileExists func(stri case "PROPPATCH": return p.Update case "PUT": - if fileExists(filename) { + if fileExists(r.URL.Path) { return p.Update } else { return p.Create @@ -166,10 +153,10 @@ func (p Permissions) AllowedSource(method, filename string, fileExists func(stri // 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(method, filename string, fileExists func(string) bool) bool { - switch method { +func (p Permissions) AllowedDestination(r *http.Request, fileExists func(string) bool) bool { + switch r.Method { case "COPY", "MOVE": - if fileExists(filename) { + if fileExists(r.Header.Get("Destination")) { return p.Update } else { return p.Create