From 0a4668fcfa9a08d3abe77b1b88f35f7708126b12 Mon Sep 17 00:00:00 2001 From: Jay Hemnani <193022578+jayhemnani9910@users.noreply.github.com> Date: Fri, 30 Oct 2026 10:54:43 +0530 Subject: [PATCH] webdav: overwrite on MOVE when the Overwrite header is missing (#12232) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * webdav: overwrite on MOVE when the Overwrite header is missing * webdav: defer a MOVE destination's delete into the rename x/net removes an existing destination before Rename, so a refused or failed move — a CanRename rejection, a vanished parent, a transient filer error — loses the target entirely. For an overwriting MOVE onto an existing destination, run the request against a filesystem that skips the destination RemoveAll and lets the filer's rename replace the target inside one transaction instead; a file vs. collection clash still takes the delete + rename path. Rename itself now tolerates an existing target, which the atomic filer rename already handles. * webdav: only default Overwrite when the header has no values An Overwrite header sent with an empty first value makes Header.Get return "", which the defaulting treated as absent and rewrote to T — overriding a client's explicit F in a later value. Check the value list instead so a present-but-empty header is left alone. * webdav: move a collection onto a non-empty one via delete + rename deferredDeleteFS skips the destination RemoveAll so the filer rename can replace the target in one transaction, but the filer only replaces an empty collection: moveSelfEntry deletes the target non-recursively and refuses ErrNonEmptyFolder. Detect a non-empty collection destination and take the depth-infinity delete + rename path moveFiles would have run. * webdav: park a clashing MOVE target aside until the rename lands A refused rename (cross-collection move, bucket target) after deleting the destination lost its contents; the target now renames to a hidden sibling first and is restored when the winning rename fails. Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * webdav: restore a parked MOVE target under a detached context A client disconnect cancels the request context mid-move, which also canceled the restore and stranded the destination under its hidden sibling name; the restore and parked-entry removal now run under context.WithoutCancel with a bounded deadline. Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * webdav: bound the parked MOVE target's restore after the move The detached restore deadline started before the source rename, so a long move could exhaust it; each detached context now starts where its work begins. A failed parked-entry removal retries briefly and its warning names the parked sibling for recovery. Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * webdav: restore a parked MOVE target without replacing a new claimant A client can create the destination name while the old target is parked, so the restore rename must fail instead of replacing that new entry. * webdav: propagate RemoveAll's context to the filer delete The parked-target cleanup runs under a bounded detached context, but the delete RPC discarded it; passing ctx through makes the deadline effective. --------- Co-authored-by: user.email Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- weed/server/webdav_server.go | 157 +++++++++++++-- weed/server/webdav_server_test.go | 321 ++++++++++++++++++++++++++++++ 2 files changed, 466 insertions(+), 12 deletions(-) diff --git a/weed/server/webdav_server.go b/weed/server/webdav_server.go index 2fc6fd321d..5ca6707298 100644 --- a/weed/server/webdav_server.go +++ b/weed/server/webdav_server.go @@ -96,12 +96,142 @@ type copyMoveGuardHandler struct { next http.Handler } +// deferredDeleteFS defers a destination's delete to the rename that follows +// it: x/net removes an existing MOVE destination before Rename, leaving it +// lost when the rename cannot complete. The filer's rename replaces the +// target in one transaction, so the delete is skipped here; a file vs. +// collection clash cannot merge in place and moves the target aside, which +// the refused rename then restores. +type deferredDeleteFS struct { + webdav.FileSystem + dst string +} + +func (f deferredDeleteFS) RemoveAll(ctx context.Context, name string) error { + if strings.TrimRight(name, "/") == strings.TrimRight(f.dst, "/") { + return nil + } + return f.FileSystem.RemoveAll(ctx, name) +} + +func (f deferredDeleteFS) Rename(ctx context.Context, oldName, newName string) error { + // a file vs. collection clash cannot merge in place, and a collection + // only replaces an empty one — those targets are moved aside rather + // than deleted outright, as moveFiles would have done, so a refused + // rename leaves the destination's contents to restore + src, srcErr := f.FileSystem.Stat(ctx, oldName) + dst, dstErr := f.FileSystem.Stat(ctx, newName) + if srcErr == nil && dstErr == nil { + clashing := src.IsDir() != dst.IsDir() + if !clashing && dst.IsDir() { + nonEmpty, err := nonEmptyDir(ctx, f.FileSystem, newName) + if err != nil { + return err + } + clashing = nonEmpty + } + if clashing { + return renameOverTarget(ctx, f.FileSystem, oldName, newName) + } + } + return f.FileSystem.Rename(ctx, oldName, newName) +} + +// renameOverTarget parks a clashing rename target under a hidden sibling +// name so a refused rename can put it back; the parked entry is only +// removed once the move has landed. +func renameOverTarget(ctx context.Context, fs webdav.FileSystem, oldName, newName string) error { + dir, base := path.Split(strings.TrimRight(newName, "/")) + spare := fmt.Sprintf("%s.%s.webdav-moved-%x", dir, base, uint32(util.RandomInt32())) + if err := fs.Rename(ctx, newName, spare); err != nil { + return err + } + if err := fs.Rename(ctx, oldName, newName); err != nil { + // a client disconnect cancels ctx mid-move; restoration gets its + // own deadline, started after the move so its duration is not eaten + restoreCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Minute) + defer cancel() + // newName may have been claimed while the target was parked; the + // restore must refuse to replace it rather than clobber new data + var backErr error + if nrf, ok := fs.(interface { + rename(context.Context, string, string, bool) error + }); ok { + backErr = nrf.rename(restoreCtx, spare, newName, true) + } else { + backErr = fs.Rename(restoreCtx, spare, newName) + } + if backErr != nil { + glog.WarningfCtx(restoreCtx, "restore moved-aside %s to %s: %v", spare, newName, backErr) + } + return err + } + cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Minute) + defer cancel() + var removeErr error + for attempt := 0; attempt < 3; attempt++ { + if attempt > 0 { + time.Sleep(2 * time.Second) + } + if removeErr = fs.RemoveAll(cleanupCtx, spare); removeErr == nil { + return nil + } + } + // the move is done, so it still reports success; the warning names the + // parked sibling for recovery instead of leaving it hidden + glog.WarningfCtx(cleanupCtx, "moved-aside %s remains parked after failed removes: %v", spare, removeErr) + return nil +} + +// nonEmptyDir reports whether a collection holds entries: the rename below +// replaces the target's own entry, which a non-empty collection refuses. +func nonEmptyDir(ctx context.Context, fs webdav.FileSystem, name string) (bool, error) { + dir, err := fs.OpenFile(ctx, name, os.O_RDONLY, 0) + if err != nil { + return false, err + } + defer dir.Close() + entries, err := dir.Readdir(1) + if err == io.EOF { + return false, nil + } + return len(entries) > 0, err +} + +// moveHandler runs an overwriting MOVE onto an existing destination against +// deferredDeleteFS, so the target survives a refused or failed rename. +func (h copyMoveGuardHandler) moveHandler(r *http.Request) http.Handler { + next, ok := h.next.(*webdav.Handler) + if !ok || r.Header.Get("Overwrite") == "F" { + return h.next + } + u, err := url.Parse(r.Header.Get("Destination")) + if err != nil || u.Host != "" && u.Host != r.Host { + return h.next + } + if _, err := h.fs.Stat(r.Context(), u.Path); err != nil { + return h.next + } + inner := *next + inner.FileSystem = deferredDeleteFS{FileSystem: inner.FileSystem, dst: u.Path} + return &inner +} + func (h copyMoveGuardHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + // RFC 4918 10.6: a missing Overwrite header means T, but webdav.Handler + // only overwrites on MOVE when it is set (golang/go#66059) + if r.Method == "MOVE" && len(r.Header.Values("Overwrite")) == 0 { + r.Header.Set("Overwrite", "T") + } + next := h.next + if r.Method == "MOVE" { + next = h.moveHandler(r) + } if r.Method == "MOVE" || r.Method == "COPY" { // a foreign-host Destination is another server's problem: the webdav // handler answers 502, so the local checks below do not apply if u, err := url.Parse(r.Header.Get("Destination")); err == nil && u.Host != "" && u.Host != r.Host { - h.next.ServeHTTP(w, r) + next.ServeHTTP(w, r) return } if u, err := url.Parse(r.Header.Get("Destination")); err == nil { @@ -155,7 +285,7 @@ func (h copyMoveGuardHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) } } } - h.next.ServeHTTP(w, r) + next.ServeHTTP(w, r) } // adapted from https://github.com/mattn/davfs/blob/master/plugin/mysql/mysql.go @@ -417,7 +547,7 @@ func (fs *WebDavFileSystem) removeAll(ctx context.Context, fullFilePath string, } dir, name := fullpath.DirAndName() - return filer_pb.Remove(context.Background(), fs, dir, name, true, recursive, false, false, []int32{fs.signature}) + return filer_pb.Remove(ctx, fs, dir, name, true, recursive, false, false, []int32{fs.signature}) } @@ -429,6 +559,10 @@ func (fs *WebDavFileSystem) RemoveAll(ctx context.Context, name string) error { } func (fs *WebDavFileSystem) Rename(ctx context.Context, oldName, newName string) error { + return fs.rename(ctx, oldName, newName, false) +} + +func (fs *WebDavFileSystem) rename(ctx context.Context, oldName, newName string, failIfExists bool) error { glog.V(2).Infof("WebDavFileSystem.Rename %v to %v", oldName, newName) @@ -449,11 +583,9 @@ func (fs *WebDavFileSystem) Rename(ctx context.Context, oldName, newName string) return os.ErrInvalid } - _, err = fs.stat(ctx, newName) - if err == nil { - return os.ErrExist - } - if err != os.ErrNotExist { + // an existing target is replaced inside the rename's transaction; a + // failed lookup aborts the move here + if _, err = fs.stat(ctx, newName); err != nil && err != os.ErrNotExist { return err } @@ -467,10 +599,11 @@ func (fs *WebDavFileSystem) Rename(ctx context.Context, oldName, newName string) return fs.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { request := &filer_pb.AtomicRenameEntryRequest{ - OldDirectory: oldDir, - OldName: oldBaseName, - NewDirectory: newDir, - NewName: newBaseName, + OldDirectory: oldDir, + OldName: oldBaseName, + NewDirectory: newDir, + NewName: newBaseName, + FailIfTargetExists: failIfExists, } _, err := client.AtomicRenameEntry(ctx, request) diff --git a/weed/server/webdav_server_test.go b/weed/server/webdav_server_test.go index b59bc18120..697bc15f8c 100644 --- a/weed/server/webdav_server_test.go +++ b/weed/server/webdav_server_test.go @@ -504,6 +504,327 @@ func (f *stubDavFs) Stat(_ context.Context, name string) (os.FileInfo, error) { } // A PUT onto a folder must not delete the folder to make room for the file. +// Without an Overwrite header a MOVE replaces an existing destination, as +// COPY already does. +func TestCopyMoveGuardHandler_MoveWithoutOverwriteHeader(t *testing.T) { + ctx := context.Background() + fs := webdav.NewMemFS() + for name, data := range map[string]string{"/a.txt": "a", "/b.txt": "b"} { + f, err := fs.OpenFile(ctx, name, os.O_RDWR|os.O_CREATE, 0644) + if err != nil { + t.Fatal(err) + } + if _, err := f.Write([]byte(data)); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + } + h := copyMoveGuardHandler{ + fs: fs, + next: &webdav.Handler{FileSystem: fs, LockSystem: webdav.NewMemLS()}, + } + req := httptest.NewRequest("MOVE", "http://x/a.txt", nil) + req.Header.Set("Destination", "http://x/b.txt") + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + + assert.Equal(t, http.StatusNoContent, w.Code) + r, err := fs.OpenFile(ctx, "/b.txt", os.O_RDONLY, 0) + if assert.NoError(t, err) { + data, err := io.ReadAll(r) + assert.NoError(t, err) + assert.Equal(t, "a", string(data)) + } + _, err = fs.Stat(ctx, "/a.txt") + assert.True(t, os.IsNotExist(err)) +} + +// An Overwrite header with an empty first value is present, not missing, so +// the default must not overwrite it — a later explicit F forbids the move. +func TestCopyMoveGuardHandler_MoveEmptyOverwriteValue(t *testing.T) { + ctx := context.Background() + fs := webdav.NewMemFS() + for name, data := range map[string]string{"/a.txt": "a", "/b.txt": "b"} { + f, err := fs.OpenFile(ctx, name, os.O_RDWR|os.O_CREATE, 0644) + if err != nil { + t.Fatal(err) + } + if _, err := f.Write([]byte(data)); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + } + h := copyMoveGuardHandler{ + fs: fs, + next: &webdav.Handler{FileSystem: fs, LockSystem: webdav.NewMemLS()}, + } + req := httptest.NewRequest("MOVE", "http://x/a.txt", nil) + req.Header.Set("Destination", "http://x/b.txt") + req.Header["Overwrite"] = []string{"", "F"} + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + + assert.Equal(t, http.StatusPreconditionFailed, w.Code) + r, err := fs.OpenFile(ctx, "/b.txt", os.O_RDONLY, 0) + if assert.NoError(t, err) { + data, err := io.ReadAll(r) + assert.NoError(t, err) + assert.Equal(t, "b", string(data)) + } +} + +// An overwriting MOVE of a collection onto a non-empty collection removes the +// destination: the deferred rename only merges over an empty one, so this +// case parks the target aside and deletes it once the move has landed. +func TestCopyMoveGuardHandler_MoveDirOntoNonEmptyDir(t *testing.T) { + ctx := context.Background() + fs := webdav.NewMemFS() + put := func(name, data string) { + f, err := fs.OpenFile(ctx, name, os.O_RDWR|os.O_CREATE, 0644) + if err != nil { + t.Fatal(err) + } + if _, err := f.Write([]byte(data)); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + } + if err := fs.Mkdir(ctx, "/src", 0755); err != nil { + t.Fatal(err) + } + put("/src/s.txt", "s") + if err := fs.Mkdir(ctx, "/dst", 0755); err != nil { + t.Fatal(err) + } + put("/dst/d.txt", "d") + + h := copyMoveGuardHandler{ + fs: fs, + next: &webdav.Handler{FileSystem: fs, LockSystem: webdav.NewMemLS()}, + } + req := httptest.NewRequest("MOVE", "http://x/src", nil) + req.Header.Set("Destination", "http://x/dst") + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + + assert.Equal(t, http.StatusNoContent, w.Code) + _, err := fs.Stat(ctx, "/dst/d.txt") + assert.True(t, os.IsNotExist(err)) + if r, err := fs.OpenFile(ctx, "/dst/s.txt", os.O_RDONLY, 0); assert.NoError(t, err) { + data, err := io.ReadAll(r) + assert.NoError(t, err) + assert.Equal(t, "s", string(data)) + } + _, err = fs.Stat(ctx, "/src") + assert.True(t, os.IsNotExist(err)) +} + +// An overwriting MOVE of a collection onto an empty collection keeps the +// deferred path: the destination survives if the rename itself fails. +func TestCopyMoveGuardHandler_MoveDirOntoEmptyDirKeepsDestinationOnFailure(t *testing.T) { + ctx := context.Background() + mem := webdav.NewMemFS() + if err := mem.Mkdir(ctx, "/src", 0755); err != nil { + t.Fatal(err) + } + if err := mem.Mkdir(ctx, "/dst", 0755); err != nil { + t.Fatal(err) + } + fs := failRenameFS{mem} + h := copyMoveGuardHandler{ + fs: fs, + next: &webdav.Handler{FileSystem: fs, LockSystem: webdav.NewMemLS()}, + } + req := httptest.NewRequest("MOVE", "http://x/src", nil) + req.Header.Set("Destination", "http://x/dst") + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + + assert.Equal(t, http.StatusForbidden, w.Code) + if fi, err := mem.Stat(ctx, "/dst"); assert.NoError(t, err) { + assert.True(t, fi.IsDir()) + } +} + +type failRenameFS struct { + webdav.FileSystem +} + +func (f failRenameFS) Rename(context.Context, string, string) error { + return fmt.Errorf("rename refused") +} + +type failPairRenameFS struct { + webdav.FileSystem + oldName, newName string +} + +func (f failPairRenameFS) Rename(ctx context.Context, oldName, newName string) error { + if oldName == f.oldName && newName == f.newName { + return fmt.Errorf("rename refused") + } + return f.FileSystem.Rename(ctx, oldName, newName) +} + +// A MOVE of a collection onto a non-empty collection whose rename is refused +// (cross-collection moves, bucket targets) must bring the destination back: +// it is only parked aside while the winning rename runs. +func TestCopyMoveGuardHandler_MoveDirOntoNonEmptyDirRestoresOnRenameFailure(t *testing.T) { + ctx := context.Background() + mem := webdav.NewMemFS() + put := func(name, data string) { + f, err := mem.OpenFile(ctx, name, os.O_RDWR|os.O_CREATE, 0644) + if err != nil { + t.Fatal(err) + } + if _, err := f.Write([]byte(data)); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + } + if err := mem.Mkdir(ctx, "/src", 0755); err != nil { + t.Fatal(err) + } + put("/src/s.txt", "s") + if err := mem.Mkdir(ctx, "/dst", 0755); err != nil { + t.Fatal(err) + } + put("/dst/d.txt", "d") + + fs := failPairRenameFS{mem, "/src", "/dst"} + h := copyMoveGuardHandler{ + fs: fs, + next: &webdav.Handler{FileSystem: fs, LockSystem: webdav.NewMemLS()}, + } + req := httptest.NewRequest("MOVE", "http://x/src", nil) + req.Header.Set("Destination", "http://x/dst") + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + + assert.Equal(t, http.StatusForbidden, w.Code) + if r, err := mem.OpenFile(ctx, "/dst/d.txt", os.O_RDONLY, 0); assert.NoError(t, err) { + data, err := io.ReadAll(r) + assert.NoError(t, err) + assert.Equal(t, "d", string(data)) + r.Close() + } + if r, err := mem.OpenFile(ctx, "/src/s.txt", os.O_RDONLY, 0); assert.NoError(t, err) { + r.Close() + } + root, err := mem.OpenFile(ctx, "/", os.O_RDONLY, 0) + if assert.NoError(t, err) { + entries, err := root.Readdir(-1) + assert.NoError(t, err) + names := make([]string, 0, len(entries)) + for _, e := range entries { + names = append(names, e.Name()) + } + assert.ElementsMatch(t, []string{"src", "dst"}, names) + } +} + +type cancelingRenameFS struct { + webdav.FileSystem + cancel context.CancelFunc + oldName, newName string +} + +func (f cancelingRenameFS) Rename(ctx context.Context, oldName, newName string) error { + if oldName == f.oldName && newName == f.newName { + f.cancel() + } + if err := ctx.Err(); err != nil { + return err + } + return f.FileSystem.Rename(ctx, oldName, newName) +} + +// A client disconnect cancels the request context mid-move; restoring the +// parked destination runs under a detached context, so the destination is +// not left under its hidden name. +func TestDeferredDeleteFS_RestoreSurvivesCanceledContext(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + mem := webdav.NewMemFS() + put := func(name, data string) { + f, err := mem.OpenFile(ctx, name, os.O_RDWR|os.O_CREATE, 0644) + if err != nil { + t.Fatal(err) + } + if _, err := f.Write([]byte(data)); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + } + if err := mem.Mkdir(ctx, "/src", 0755); err != nil { + t.Fatal(err) + } + put("/src/s.txt", "s") + if err := mem.Mkdir(ctx, "/dst", 0755); err != nil { + t.Fatal(err) + } + put("/dst/d.txt", "d") + + fs := deferredDeleteFS{ + FileSystem: cancelingRenameFS{mem, cancel, "/src", "/dst"}, + dst: "/dst", + } + err := fs.Rename(ctx, "/src", "/dst") + assert.ErrorIs(t, err, context.Canceled) + if r, err := mem.OpenFile(context.Background(), "/dst/d.txt", os.O_RDONLY, 0); assert.NoError(t, err) { + data, err := io.ReadAll(r) + assert.NoError(t, err) + assert.Equal(t, "d", string(data)) + r.Close() + } +} + +// A MOVE whose rename fails must leave the existing destination in place: +// the delete is deferred into the rename, which replaces atomically. +func TestCopyMoveGuardHandler_MoveKeepsDestinationOnRenameFailure(t *testing.T) { + ctx := context.Background() + mem := webdav.NewMemFS() + for name, data := range map[string]string{"/a.txt": "a", "/b.txt": "b"} { + f, err := mem.OpenFile(ctx, name, os.O_RDWR|os.O_CREATE, 0644) + if err != nil { + t.Fatal(err) + } + if _, err := f.Write([]byte(data)); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + } + fs := failRenameFS{mem} + h := copyMoveGuardHandler{ + fs: fs, + next: &webdav.Handler{FileSystem: fs, LockSystem: webdav.NewMemLS()}, + } + req := httptest.NewRequest("MOVE", "http://x/a.txt", nil) + req.Header.Set("Destination", "http://x/b.txt") + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + + assert.Equal(t, http.StatusForbidden, w.Code) + r, err := mem.OpenFile(ctx, "/b.txt", os.O_RDONLY, 0) + if assert.NoError(t, err) { + data, err := io.ReadAll(r) + assert.NoError(t, err) + assert.Equal(t, "b", string(data)) + } +} + func TestWebDavFileSystem_OpenFile_DoesNotReplaceFolder(t *testing.T) { filer := &deleteRecordingFiler{existing: &filer_pb.Entry{ Name: "sub",