From fb02929c3a95ca2ad025ccc1b83803f3249df6f6 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Sun, 20 Sep 2026 16:25:19 +0200 Subject: [PATCH] fix: authorize the destination collection a copy or move replaces A COPY or MOVE onto an existing destination replaces it, which destroys everything the destination collection held. Only the source subtree was authorized per descendant, and the destination itself was authorized as an update, so a rule restricting a subtree held against DELETE but not against an overwrite of the collection above it. Removing a collection is now authorized the way DELETE on it would be, on the collection and on every path beneath it. Writing over a file stays an update. --- README.md | 5 ++ lib/handler.go | 49 ++++++++++++- lib/handler_test.go | 173 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 225 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index ed45652..ea3d348 100644 --- a/README.md +++ b/README.md @@ -121,6 +121,9 @@ directory: /data # permissions. For example, to allow to read and create, set "RC". Default is "R". # LOCK counts as a write: it needs U on a path that exists and C on one that does # not, since locking a path that does not exist creates it. +# Being overwritten counts as well: a COPY or MOVE onto an existing file replaces +# it and needs U, while one onto an existing collection removes everything it +# holds and needs D, on the collection and on every path under it. permissions: R # The default permissions rules for users. Default is none. Rules are applied @@ -244,6 +247,8 @@ A `regex` rule is matched literally against the path, and gets none of the above Rules apply to every path an operation touches, not only the one it names. Collection listings leave out entries the rules deny, copying a collection leaves those entries behind, and a `MOVE` or `DELETE` that would act on a denied descendant is refused outright. +Overwriting a destination is authorized for what it destroys. A `COPY` or `MOVE` onto an existing destination replaces it: RFC4918 has `MOVE` perform a `DELETE` with `Depth: infinity` on the destination first, and requires an overwritten collection to end up with exactly the membership the source had, so either way whatever was there is gone. Replacing a file needs `U` on it, the same permission `PUT` needs. Replacing a collection removes everything it holds, so it needs `D` on that collection and on every path beneath it: a rule withholding `D` anywhere under a destination refuses the overwrite outright, even where it grants `C` and `U`. + Rules follow the case sensitivity of the file system, which each served directory is probed for at startup. Where names are case-insensitive, as on APFS and NTFS, `path: /secret/` also covers `/SECRET/`, a `regex` is matched against the folded path as well as the path as written, and Unicode normal forms count as one name. Elsewhere rules are matched exactly, since `/secret` and `/SECRET` are then different directories. ### CORS diff --git a/lib/handler.go b/lib/handler.go index fecdba7..dd9d32e 100644 --- a/lib/handler.go +++ b/lib/handler.go @@ -169,8 +169,9 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - // MOVE and DELETE act on a whole subtree in one call, so every descendant - // needs authorizing here. COPY and PROPFIND go through permFS instead. + // MOVE and DELETE act on a whole source subtree in one call, so every + // descendant needs authorizing here. Reading COPY and PROPFIND out of the + // source goes through permFS instead. if r.Method == "MOVE" || r.Method == "DELETE" { ok, err := user.fs.allowedThroughout(r.Context(), req.path, func(p Permissions) bool { return p.Allowed(req, fileExists) @@ -188,6 +189,50 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } + // Excerpt from RFC4918, section 9.9.3, on MOVE: + // + // If a resource exists at the destination and the Overwrite header is + // "T", then prior to performing the move, the server MUST perform a + // DELETE with "Depth: infinity" on the destination resource. + // + // And from section 9.8.4, on COPY: + // + // When a collection is overwritten, the membership of the destination + // collection after the successful COPY request MUST be the same + // membership as the source collection immediately before the COPY. + // + // Either way whatever the destination collection held is gone, which + // golang.org/x/net/webdav carries out as a RemoveAll before the rename or + // the copy. Writing over a file is an update, already authorized by Allowed, + // but removing a collection takes everything under it. Nothing writes to + // those descendants, they are only destroyed, so authorize the destination + // the way DELETE on that collection would be. + if (r.Method == "MOVE" || r.Method == "COPY") && req.destination != "" { + info, err := user.fs.Stat(r.Context(), req.destination) + if err == nil && info.IsDir() { + deletable := func(p Permissions) bool { return p.Delete } + + // allowedThroughout reaches descendants only, and the destination + // check in Allowed covers the collection itself as an update rather + // than a delete, so that is checked here. + ok := user.allowedAt(req.destination, deletable) + if ok { + ok, err = user.fs.allowedThroughout(r.Context(), req.destination, deletable) + if err != nil { + lZap.Error("could not authorize destination subtree", zap.String("destination", req.destination), zap.Error(err)) + w.WriteHeader(http.StatusForbidden) + return + } + } + + if !ok { + lZap.Info("denied by a rule on the destination collection", zap.String("method", r.Method), zap.String("destination", req.destination)) + w.WriteHeader(http.StatusForbidden) + return + } + } + } + if r.Method == "HEAD" { w = responseWriterNoBody{w} } diff --git a/lib/handler_test.go b/lib/handler_test.go index 6a2c0b0..b2847af 100644 --- a/lib/handler_test.go +++ b/lib/handler_test.go @@ -1462,6 +1462,179 @@ rules: require.FileExists(t, filepath.Join(dirA, "secret", "flag.txt")) } +// TestServerRulesDestinationOverwriteDescendants covers a COPY or MOVE onto an +// existing collection, which replaces it: the server deletes the destination +// with "Depth: infinity" first, reaching descendants that authorizing only the +// destination itself let it destroy. +func TestServerRulesDestinationOverwriteDescendants(t *testing.T) { + t.Parallel() + + dir := makeTestDirectory(t, map[string][]byte{ + "shared/protected/confidential.txt": []byte("top secret"), + "empty/.keep": []byte(""), + }) + + srv := makeTestServer(t, fmt.Sprintf(` +directory: %s +permissions: CRUD +rules: + - path: "/shared/protected/" + permissions: R +`, dir)) + defer srv.Close() + + confidential := filepath.Join(dir, "shared", "protected", "confidential.txt") + + // Controls: the rule holds when the request names the denied subtree. + code, _ := doRequest(t, "DELETE", srv.URL+"/shared/protected/confidential.txt", "", "", nil, "") + require.Equal(t, http.StatusForbidden, code) + + code, _ = doRequest(t, "DELETE", srv.URL+"/shared/", "", "", nil, "") + require.Equal(t, http.StatusForbidden, code) + + // Overwriting the collection above the rule destroys the denied subtree, so + // it is refused for the same reason DELETE is. + code, _ = doRequest(t, "MOVE", srv.URL+"/empty/", "", "", map[string]string{ + "Destination": srv.URL + "/shared/", + "Overwrite": "T", + }, "") + require.Equal(t, http.StatusForbidden, code) + + // COPY overwrites unless the header says otherwise, so it needs no Overwrite. + code, _ = doRequest(t, "COPY", srv.URL+"/empty/", "", "", map[string]string{ + "Destination": srv.URL + "/shared/", + "Depth": "infinity", + }, "") + require.Equal(t, http.StatusForbidden, code) + + require.FileExists(t, confidential) +} + +// TestServerRulesDestinationOverwriteDescendantsMultiDir is the same defect +// across mounts, where the destination collection lives under another mount. +func TestServerRulesDestinationOverwriteDescendantsMultiDir(t *testing.T) { + t.Parallel() + + dirA := makeTestDirectory(t, map[string][]byte{"shared/protected/flag.txt": []byte("mount secret")}) + dirB := makeTestDirectory(t, map[string][]byte{"empty/.keep": []byte("")}) + + srv := makeTestServer(t, fmt.Sprintf(` +permissions: CRUD +directories: + - name: alpha + path: %s + - name: beta + path: %s +rules: + - path: "/alpha/shared/protected/" + permissions: R +`, dirA, dirB)) + defer srv.Close() + + code, _ := doRequest(t, "MOVE", srv.URL+"/beta/empty/", "", "", map[string]string{ + "Destination": srv.URL + "/alpha/shared/", + "Overwrite": "T", + }, "") + require.Equal(t, http.StatusForbidden, code) + + code, _ = doRequest(t, "COPY", srv.URL+"/beta/empty/", "", "", map[string]string{ + "Destination": srv.URL + "/alpha/shared/", + "Depth": "infinity", + }, "") + require.Equal(t, http.StatusForbidden, code) + + require.FileExists(t, filepath.Join(dirA, "shared", "protected", "flag.txt")) +} + +// TestServerRulesDestinationOverwriteRequiresDelete covers the permission class +// the overwrite is authorized under. Removing a collection is delete-class, so a +// rule that grants writes but withholds D refuses the overwrite, whether it +// governs the destination itself or something beneath it. +func TestServerRulesDestinationOverwriteRequiresDelete(t *testing.T) { + t.Parallel() + + dir := makeTestDirectory(t, map[string][]byte{ + "empty/.keep": []byte(""), + "empty2/.keep": []byte(""), + "nodelete/note.txt": []byte("kept by the rule on the collection"), + "shared/keep/n.txt": []byte("kept by the rule on a descendant"), + }) + + srv := makeTestServer(t, fmt.Sprintf(` +directory: %s +permissions: CRUD +rules: + - path: "/nodelete/" + permissions: CRU + - path: "/shared/keep/" + permissions: CRU +`, dir)) + defer srv.Close() + + // The rule governs the destination collection itself. + code, _ := doRequest(t, "MOVE", srv.URL+"/empty/", "", "", map[string]string{ + "Destination": srv.URL + "/nodelete/", + "Overwrite": "T", + }, "") + require.Equal(t, http.StatusForbidden, code) + require.FileExists(t, filepath.Join(dir, "nodelete", "note.txt")) + + // The rule governs a descendant of the destination collection. + code, _ = doRequest(t, "MOVE", srv.URL+"/empty2/", "", "", map[string]string{ + "Destination": srv.URL + "/shared/", + "Overwrite": "T", + }, "") + require.Equal(t, http.StatusForbidden, code) + require.FileExists(t, filepath.Join(dir, "shared", "keep", "n.txt")) +} + +// TestServerDestinationOverwriteAllowed pins what overwriting a destination is +// still allowed to do, so the delete-class check on collections does not spread +// to writing over a file. Replacing a file is update-class, the same class PUT +// over an existing file needs, which clients rely on when they save by writing a +// temporary file and moving it over the target. +func TestServerDestinationOverwriteAllowed(t *testing.T) { + t.Parallel() + + dir := makeTestDirectory(t, map[string][]byte{ + "empty/.keep": []byte(""), + "empty2/.keep": []byte(""), + "plain/note.txt": []byte("plain"), + "updatable/doc.txt": []byte("updatable"), + "source.txt": []byte("source"), + }) + + srv := makeTestServer(t, fmt.Sprintf(` +directory: %s +permissions: CRUD +rules: + - path: "/updatable/doc.txt" + permissions: RU +`, dir)) + defer srv.Close() + + // No rule withholds D anywhere under the destination. + code, _ := doRequest(t, "MOVE", srv.URL+"/empty/", "", "", map[string]string{ + "Destination": srv.URL + "/plain/", + "Overwrite": "T", + }, "") + require.Equal(t, http.StatusNoContent, code) + + // A destination that does not exist is created, not overwritten. + code, _ = doRequest(t, "MOVE", srv.URL+"/empty2/", "", "", map[string]string{ + "Destination": srv.URL + "/fresh/", + "Overwrite": "T", + }, "") + require.Equal(t, http.StatusCreated, code) + + // A file destination needs only the U its rule grants, not D. + code, _ = doRequest(t, "MOVE", srv.URL+"/source.txt", "", "", map[string]string{ + "Destination": srv.URL + "/updatable/doc.txt", + "Overwrite": "T", + }, "") + require.Equal(t, http.StatusNoContent, code) +} + // TestServerLockRequiresWritePermission covers LOCK being authorized by any // permission at all, which let a read-only user create a file by locking a // missing path, and hold a write lock that blocks legitimate writers.