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 <chris.lu@gmail.com> Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>
2026-10-20 04:54:24 +02:00 · 2026-10-18 12:57:10 -07:00
1 parent 8cb42cd69f
commit 863efb3d35
@@ -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)
+	}
+}