From 863efb3d35b552e0ad44df2cfd9d2bcfb0adb5b8 Mon Sep 17 00:00:00 2001 From: Jay Hemnani <193022578+jayhemnani9910@users.noreply.github.com> Date: Mon, 19 Oct 2026 01:27:10 +0530 Subject: [PATCH] webdav: delete a folder with its contents (#11811) * webdav: delete a folder with its contents * webdav: guard the recursive delete against stale and confined paths - propagate stat lookup errors instead of returning an error-bearing FileInfo: OpenFile and DELETE would otherwise take the recursive removeAll on a path whose type was never confirmed, wiping a directory on a transient filer error - refuse wrappedFs.Rename from or to the served root, matching the RemoveAll guard and webdav.Dir - normalize trailing slashes in removeAll so the delete names the entry itself, and refuse the root for every caller * webdav: keep file-only deletes shallow and refuse ancestor destinations - removeAll used by OpenFile and failed-upload cleanup now deletes non-recursively, so a folder swapped in after the stat check is not emptied by a pending file delete - MOVE and COPY whose destination contains the source are refused before the handler removes the destination, which would otherwise take the source down with it * webdav: also refuse a destination inside the source a MOVE or COPY into its own subtree recurses into the destination it just created; refuse it with the same guard instead of burning the recursion budget for a 500 * webdav: allow a shallow copy into the source subtree a COPY with Depth: 0 creates only the destination collection and never reads children, so it is safe inside the source; only deep copies and moves are refused there * webdav: refuse a copy/move whose source is unreadable * webdav: cover the missing-source refusal in the guard test * webdav: 500 on a source lookup failure, 404 only when absent --------- Co-authored-by: Chris Lu Co-authored-by: Chris Lu --- weed/server/webdav_server.go | 95 ++++++++++----- weed/server/webdav_server_test.go | 194 +++++++++++++++++++++++++++++- weed/server/wrapped_webdav_fs.go | 6 + 3 files changed, 263 insertions(+), 32 deletions(-) diff --git a/weed/server/webdav_server.go b/weed/server/webdav_server.go index 389a3d5fb..508a95d12 100644 --- a/weed/server/webdav_server.go +++ b/weed/server/webdav_server.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "net/http" + "net/url" "os" "path" "strings" @@ -74,9 +75,12 @@ func NewWebDavServer(option *WebDavOption) (ws *WebDavServer, err error) { option: option, grpcDialOption: security.LoadClientTLS(util.GetViper(), "grpc.filer"), Handler: listedEntriesHandler{ - next: &webdav.Handler{ - FileSystem: fs, - LockSystem: webdav.NewMemLS(), + next: copyMoveGuardHandler{ + fs: fs, + next: &webdav.Handler{ + FileSystem: fs, + LockSystem: webdav.NewMemLS(), + }, }, }, } @@ -84,6 +88,40 @@ func NewWebDavServer(option *WebDavOption) (ws *WebDavServer, err error) { return ws, nil } +// an overwriting MOVE or COPY removes the destination before reading the +// source, so a destination that contains the source removes it too; a +// destination under the source copies itself unless the copy is shallow +type copyMoveGuardHandler struct { + fs webdav.FileSystem + next http.Handler +} + +func (h copyMoveGuardHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if r.Method == "MOVE" || r.Method == "COPY" { + if u, err := url.Parse(r.Header.Get("Destination")); err == nil { + src := strings.Trim(path.Clean(r.URL.Path), "/") + dst := strings.Trim(path.Clean(u.Path), "/") + above := dst == "" || src == dst || strings.HasPrefix(src, dst+"/") + below := src == "" || strings.HasPrefix(dst, src+"/") + if above || below && (r.Method == "MOVE" || r.Header.Get("Depth") != "0") { + w.WriteHeader(http.StatusForbidden) + return + } + } + // a source that cannot be read makes the operation fail after the + // destination is already gone — reject it before that + if _, err := h.fs.Stat(r.Context(), r.URL.Path); err != nil { + if os.IsNotExist(err) { + w.WriteHeader(http.StatusNotFound) + } else { + w.WriteHeader(http.StatusInternalServerError) + } + return + } + } + h.next.ServeHTTP(w, r) +} + // adapted from https://github.com/mattn/davfs/blob/master/plugin/mysql/mysql.go type WebDavFileSystem struct { @@ -101,7 +139,6 @@ type FileInfo struct { modifiedTime time.Time etag string isDirectory bool - err error } func (fi *FileInfo) Name() string { return fi.name } @@ -112,12 +149,8 @@ func (fi *FileInfo) IsDir() bool { return fi.isDirectory } func (fi *FileInfo) Sys() interface{} { return nil } func (fi *FileInfo) ETag(ctx context.Context) (string, error) { - if fi.err != nil { - return "", fi.err - } if fi.etag == "" { - // nothing hashed this entry; let webdav derive one rather than - // publish an empty DAV:getetag, which is not a valid entity-tag + // an empty DAV:getetag is not a valid entity-tag; let webdav derive one return "", webdav.ErrNotImplemented } return fi.etag, nil @@ -203,6 +236,9 @@ func (fs *WebDavFileSystem) Mkdir(ctx context.Context, fullDirPath string, perm if err == nil { return os.ErrExist } + if err != os.ErrNotExist { + return err + } return fs.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { dir, name := util.NormalizePath(fullDirPath).DirAndName() @@ -244,12 +280,14 @@ func (fs *WebDavFileSystem) OpenFile(ctx context.Context, fullFilePath string, f if strings.HasSuffix(fullFilePath, "/") { return nil, os.ErrInvalid } - _, err = fs.stat(ctx, fullFilePath) + fi, err := fs.stat(ctx, fullFilePath) if err == nil { - if flag&os.O_EXCL != 0 { + if flag&os.O_EXCL != 0 || fi.IsDir() { return nil, os.ErrExist } - fs.removeAll(ctx, fullFilePath) + fs.removeAll(ctx, fullFilePath, false) + } else if err != os.ErrNotExist { + return nil, err } dir, name := util.FullPath(fullFilePath).DirAndName() @@ -307,15 +345,19 @@ func (fs *WebDavFileSystem) OpenFile(ctx context.Context, fullFilePath string, f } -func (fs *WebDavFileSystem) removeAll(ctx context.Context, fullFilePath string) error { +func (fs *WebDavFileSystem) removeAll(ctx context.Context, fullFilePath string, recursive bool) error { var err error if fullFilePath, err = clearName(fullFilePath); err != nil { return err } - dir, name := util.FullPath(fullFilePath).DirAndName() + fullpath := util.NormalizePath(fullFilePath) + if fullpath == "/" { + return os.ErrInvalid + } + dir, name := fullpath.DirAndName() - return filer_pb.Remove(context.Background(), fs, dir, name, true, false, false, false, []int32{fs.signature}) + return filer_pb.Remove(context.Background(), fs, dir, name, true, recursive, false, false, []int32{fs.signature}) } @@ -323,7 +365,7 @@ func (fs *WebDavFileSystem) RemoveAll(ctx context.Context, name string) error { glog.V(2).Infof("WebDavFileSystem.RemoveAll %v", name) - return fs.removeAll(ctx, name) + return fs.removeAll(ctx, name, true) } func (fs *WebDavFileSystem) Rename(ctx context.Context, oldName, newName string) error { @@ -338,23 +380,22 @@ func (fs *WebDavFileSystem) Rename(ctx context.Context, oldName, newName string) return err } - of, err := fs.stat(ctx, oldName) - if err != nil { + if _, err = fs.stat(ctx, oldName); err != nil { return os.ErrExist } - if of.IsDir() { - if strings.HasSuffix(oldName, "/") { - oldName = strings.TrimRight(oldName, "/") - } - if strings.HasSuffix(newName, "/") { - newName = strings.TrimRight(newName, "/") - } + oldName = string(util.NormalizePath(oldName)) + newName = string(util.NormalizePath(newName)) + if oldName == "/" || newName == "/" { + return os.ErrInvalid } _, err = fs.stat(ctx, newName) if err == nil { return os.ErrExist } + if err != os.ErrNotExist { + return err + } oldDir, oldBaseName := util.FullPath(oldName).DirAndName() newDir, newBaseName := util.FullPath(newName).DirAndName() @@ -395,7 +436,7 @@ func (fs *WebDavFileSystem) stat(ctx context.Context, fullFilePath string) (os.F if err == filer_pb.ErrNotFound { return nil, os.ErrNotExist } - return &FileInfo{err: err}, nil + return nil, err } if entry == nil { return nil, os.ErrNotExist @@ -481,7 +522,7 @@ func (f *WebDavFile) Write(buf []byte) (int, error) { if flushErr != nil { if f.entry.Attributes.Mtime == 0 { - if err := f.fs.removeAll(ctx, f.name); err != nil { + if err := f.fs.removeAll(ctx, f.name, false); err != nil { glog.Errorf("bufWriter.Flush remove file error: %+v", f.name) } } diff --git a/weed/server/webdav_server_test.go b/weed/server/webdav_server_test.go index 3be052ec8..e74774520 100644 --- a/weed/server/webdav_server_test.go +++ b/weed/server/webdav_server_test.go @@ -5,7 +5,10 @@ import ( "fmt" "io" "net" + "net/http" + "net/http/httptest" "os" + "path" "testing" "golang.org/x/net/webdav" @@ -56,11 +59,6 @@ func TestFileInfoETag(t *testing.T) { if etag, err := (&FileInfo{etag: "abc"}).ETag(ctx); err != nil || etag != "abc" { t.Errorf("ETag() = %q, %v, want \"abc\", nil", etag, err) } - - failed := &FileInfo{err: os.ErrInvalid} - if _, err := failed.ETag(ctx); err != os.ErrInvalid { - t.Errorf("ETag() = %v, want the stat error", err) - } } type noVolumeFiler struct { @@ -85,6 +83,24 @@ func (f *createRecordingFiler) CreateEntry(_ context.Context, req *filer_pb.Crea return &filer_pb.CreateEntryResponse{}, nil } +type deleteRecordingFiler struct { + filer_pb.UnimplementedSeaweedFilerServer + req *filer_pb.DeleteEntryRequest + existing *filer_pb.Entry +} + +func (f *deleteRecordingFiler) LookupDirectoryEntry(_ context.Context, _ *filer_pb.LookupDirectoryEntryRequest) (*filer_pb.LookupDirectoryEntryResponse, error) { + if f.existing == nil { + return nil, filer_pb.ErrNotFound + } + return &filer_pb.LookupDirectoryEntryResponse{Entry: f.existing}, nil +} + +func (f *deleteRecordingFiler) DeleteEntry(_ context.Context, req *filer_pb.DeleteEntryRequest) (*filer_pb.DeleteEntryResponse, error) { + f.req = req + return &filer_pb.DeleteEntryResponse{}, nil +} + func startFakeWebDavFiler(t *testing.T, impl filer_pb.SeaweedFilerServer) pb.ServerAddress { t.Helper() lis, err := net.Listen("tcp", "127.0.0.1:0") @@ -161,3 +177,171 @@ func TestWebDavFileSystem_Mkdir_EntryName(t *testing.T) { assert.True(t, filer.req.Entry.IsDirectory) } } + +// DELETE, and MOVE or COPY with Overwrite: T, remove a collection through RemoveAll, which must take its contents too. +func TestWebDavFileSystem_RemoveAll_Recursive(t *testing.T) { + filer := &deleteRecordingFiler{} + filerAddr := startFakeWebDavFiler(t, filer) + fs := &WebDavFileSystem{option: &WebDavOption{ + Filer: filerAddr, + GrpcDialOption: grpc.WithTransportCredentials(insecure.NewCredentials()), + }} + + assert.NoError(t, fs.RemoveAll(context.Background(), "/dir/sub")) + if assert.NotNil(t, filer.req) { + assert.Equal(t, "/dir", filer.req.Directory) + assert.Equal(t, "sub", filer.req.Name) + assert.True(t, filer.req.IsRecursive) + } + + filer.req = nil + assert.NoError(t, fs.RemoveAll(context.Background(), "/dir/sub/")) + if assert.NotNil(t, filer.req) { + assert.Equal(t, "/dir", filer.req.Directory) + assert.Equal(t, "sub", filer.req.Name) + } + + filer.req = nil + wrapped := NewWrappedFs(fs, "/served") + assert.Equal(t, os.ErrInvalid, fs.RemoveAll(context.Background(), "/")) + assert.Equal(t, os.ErrInvalid, wrapped.RemoveAll(context.Background(), "/../")) + assert.Equal(t, os.ErrInvalid, wrapped.Rename(context.Background(), "/", "/x")) + assert.Nil(t, filer.req) +} + +// MOVE or COPY into an ancestor destination is refused before the +// destination is removed: removing it first would delete the source too. +func TestCopyMoveGuardHandler(t *testing.T) { + called := false + h := copyMoveGuardHandler{ + fs: &stubDavFs{present: map[string]bool{"/dir": true, "/dir/sub": true, "/dirs": true, "/dir/other": true, "/anywhere": true}}, + next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + }), + } + + for _, tc := range []struct{ method, src, dst string }{ + {"MOVE", "/dir/sub", "/dir"}, + {"MOVE", "/dir/sub", "/dir/"}, + {"MOVE", "/dir", "/dir"}, + {"MOVE", "/dir", "/dir/sub"}, + {"MOVE", "/", "/anywhere"}, + {"COPY", "/dir/sub", "/dir"}, + {"COPY", "/dir", "/dir/sub"}, + } { + called = false + req := httptest.NewRequest(tc.method, "http://x"+tc.src, nil) + req.Header.Set("Destination", "http://x"+tc.dst) + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + assert.Equal(t, http.StatusForbidden, w.Code, "%s %s -> %s", tc.method, tc.src, tc.dst) + assert.False(t, called) + } + + for _, tc := range []struct{ method, src, dst, depth string }{ + {"COPY", "/dir/sub", "/dir/other", ""}, + {"COPY", "/dirs", "/dir", ""}, + {"COPY", "/dir", "/dir/sub", "0"}, + } { + called = false + req := httptest.NewRequest(tc.method, "http://x"+tc.src, nil) + req.Header.Set("Destination", "http://x"+tc.dst) + if tc.depth != "" { + req.Header.Set("Depth", tc.depth) + } + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + assert.True(t, called, "%s %s -> %s depth=%q", tc.method, tc.src, tc.dst, tc.depth) + } + + // a source that fails to stat must not reach the handler either — the + // destination would be deleted before the operation fails anyway; only + // confirmed absence is a 404 + errFs := copyMoveGuardHandler{ + fs: &stubDavFs{errPaths: map[string]bool{"/broken": true}}, + next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { called = true }), + } + for _, tc := range []struct { + method, src, dst string + want int + }{ + {"MOVE", "/missing", "/dir", http.StatusNotFound}, + {"COPY", "/missing", "/dir", http.StatusNotFound}, + {"MOVE", "/broken", "/dir", http.StatusInternalServerError}, + } { + called = false + req := httptest.NewRequest(tc.method, "http://x"+tc.src, nil) + req.Header.Set("Destination", "http://x"+tc.dst) + w := httptest.NewRecorder() + errFs.ServeHTTP(w, req) + assert.Equal(t, tc.want, w.Code, "%s %s -> %s", tc.method, tc.src, tc.dst) + assert.False(t, called) + } +} + +type stubDavFs struct { + webdav.FileSystem + present map[string]bool + errPaths map[string]bool +} + +func (f *stubDavFs) Stat(_ context.Context, name string) (os.FileInfo, error) { + if f.errPaths[path.Clean(name)] { + return nil, fmt.Errorf("filer unavailable") + } + if f.present[path.Clean(name)] { + return &FileInfo{name: path.Base(name)}, nil + } + return nil, os.ErrNotExist +} + +// A PUT onto a folder must not delete the folder to make room for the file. +func TestWebDavFileSystem_OpenFile_DoesNotReplaceFolder(t *testing.T) { + filer := &deleteRecordingFiler{existing: &filer_pb.Entry{ + Name: "sub", + IsDirectory: true, + Attributes: &filer_pb.FuseAttributes{FileMode: uint32(os.ModeDir | 0755)}, + }} + filerAddr := startFakeWebDavFiler(t, filer) + fs := &WebDavFileSystem{option: &WebDavOption{ + Filer: filerAddr, + GrpcDialOption: grpc.WithTransportCredentials(insecure.NewCredentials()), + }} + + _, err := fs.OpenFile(context.Background(), "/dir/sub", os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644) + assert.Equal(t, os.ErrExist, err) + assert.Nil(t, filer.req) +} + +type renameRecordingFiler struct { + filer_pb.UnimplementedSeaweedFilerServer + req *filer_pb.AtomicRenameEntryRequest +} + +func (f *renameRecordingFiler) LookupDirectoryEntry(_ context.Context, req *filer_pb.LookupDirectoryEntryRequest) (*filer_pb.LookupDirectoryEntryResponse, error) { + if req.Name != "file.txt" { + return nil, filer_pb.ErrNotFound + } + return &filer_pb.LookupDirectoryEntryResponse{Entry: &filer_pb.Entry{Name: "file.txt", Attributes: &filer_pb.FuseAttributes{}}}, nil +} + +func (f *renameRecordingFiler) AtomicRenameEntry(_ context.Context, req *filer_pb.AtomicRenameEntryRequest) (*filer_pb.AtomicRenameEntryResponse, error) { + f.req = req + return &filer_pb.AtomicRenameEntryResponse{}, nil +} + +// MOVE of a file onto an existing folder removes the folder, then renames onto "/dir/coll/". +func TestWebDavFileSystem_Rename_FileOntoSlashedPath(t *testing.T) { + filer := &renameRecordingFiler{} + filerAddr := startFakeWebDavFiler(t, filer) + fs := &WebDavFileSystem{option: &WebDavOption{ + Filer: filerAddr, + GrpcDialOption: grpc.WithTransportCredentials(insecure.NewCredentials()), + }} + + assert.NoError(t, fs.Rename(context.Background(), "/dir/file.txt", "/dir/coll/")) + if assert.NotNil(t, filer.req) { + assert.Equal(t, "/dir", filer.req.NewDirectory) + assert.Equal(t, "coll", filer.req.NewName) + } +} diff --git a/weed/server/wrapped_webdav_fs.go b/weed/server/wrapped_webdav_fs.go index a2fa1c2bc..16e40c188 100644 --- a/weed/server/wrapped_webdav_fs.go +++ b/weed/server/wrapped_webdav_fs.go @@ -41,11 +41,17 @@ func (w wrappedFs) OpenFile(ctx context.Context, name string, flag int, perm os. } func (w wrappedFs) RemoveAll(ctx context.Context, name string) error { + if path.Clean("/"+name) == "/" { + return os.ErrInvalid + } name = w.confine(name) return w.FileSystem.RemoveAll(ctx, name) } func (w wrappedFs) Rename(ctx context.Context, oldName, newName string) error { + if path.Clean("/"+oldName) == "/" || path.Clean("/"+newName) == "/" { + return os.ErrInvalid + } oldName = w.confine(oldName) newName = w.confine(newName) return w.FileSystem.Rename(ctx, oldName, newName)