sftp: refuse mkdir and uploads under a missing folder (#12156)

* sftp: refuse mkdir and uploads under a missing folder

* sftp: re-check the parent directory when a buffered upload commits

The open-time check cannot catch a parent removed while the upload
buffered; the filer would recreate it when the entry lands.

* sftp: cover mkdir and upload commit against a missing parent

Extend the test filer with CreateEntry so makeDir can be exercised end
to end.

* sftp: refuse a rename into a missing folder

The filer's rename path creates the target entry through CreateEntry,
which recreates absent parents the same way uploads did.

---------

Co-authored-by: user.email <chrislusf@users.noreply.github.com>

weed/sftpd/sftp_copy_existing_test.go

@@ -6,6 +6,7 @@ import (
 	"net"
 	"net/http"
 	"net/http/httptest"
+	"os"
 	"testing"
 
 	"github.com/seaweedfs/seaweedfs/weed/pb"
@@ -208,3 +209,21 @@ func TestCopyExistingContentTrailingGap(t *testing.T) {
 		t.Fatalf("copied %d bytes, want %d with a zero-filled tail", dst.Len(), len(want))
 	}
 }
+
+// mkdir and uploads must not create a missing parent folder on the way.
+func TestRequireParentDir(t *testing.T) {
+	fs := newCopyTestServer(t, "", map[string]*filer_pb.Entry{
+		"/home/d":       {Name: "d", IsDirectory: true},
+		"/home/d/a.txt": {Name: "a.txt"},
+	})
+
+	if err := fs.requireParentDir("/home/d/new"); err != nil {
+		t.Fatalf("existing parent: %v", err)
+	}
+	if err := fs.requireParentDir("/home/missing/new"); err != os.ErrNotExist {
+		t.Fatalf("missing parent: got %v, want os.ErrNotExist", err)
+	}
+	if err := fs.requireParentDir("/home/d/a.txt/new"); err != os.ErrNotExist {
+		t.Fatalf("file as parent: got %v, want os.ErrNotExist", err)
+	}
+}

weed/sftpd/sftp_file_writer.go

@@ -84,6 +84,11 @@ func (w *SeaweedSftpFileWriter) Close() error {
 		return err
 	}
 
+	// The parent may have been removed while the upload buffered.
+	if err := w.fs.requireParentDir(w.absPath); err != nil {
+		return err
+	}
+
 	// Stream the file to the absolute path (after HomeDir translation)
 	return w.fs.putFile(w.absPath, w.tmpFile, w.fs.user)
 }

weed/sftpd/sftp_filer.go

@@ -157,6 +157,9 @@ func (fs *SftpServer) newFileWriter(r *sftp.Request) (io.WriterAt, error) {
 		glog.Errorf("Permission denied for %s", absPath)
 		return nil, err
 	}
+	if err := fs.requireParentDir(absPath); err != nil {
+		return nil, err
+	}
 	// Create a temporary file to buffer writes
 	tmpFile, err := os.CreateTemp("", "sftp-upload-*")
 	if err != nil {
@@ -275,6 +278,9 @@ func (fs *SftpServer) renameEntry(absPath, absTarget string) error {
 	if err := fs.checkRenameKeepsAcl(absPath, absTarget); err != nil {
 		return err
 	}
+	if err := fs.requireParentDir(absTarget); err != nil {
+		return err
+	}
 	oldDir, oldName := util.FullPath(absPath).DirAndName()
 	newDir, newName := util.FullPath(absTarget).DirAndName()
 	return fs.callWithClient(false, func(ctx context.Context, client filer_pb.SeaweedFilerClient) error {
@@ -513,6 +519,9 @@ func (fs *SftpServer) makeDir(absPath string) error {
 	if err := fs.checkFilePermission(dir, "write"); err != nil {
 		return err
 	}
+	if err := fs.requireParentDir(absPath); err != nil {
+		return err
+	}
 	// default mode and ownership
 	err := filer_pb.Mkdir(context.Background(), fs, string(dir), name, func(entry *filer_pb.Entry) {
 		mode := uint32(0755 | os.ModeDir)
@@ -534,6 +543,23 @@ func (fs *SftpServer) makeDir(absPath string) error {
 	return err
 }
 
+// requireParentDir returns os.ErrNotExist unless the parent of absPath is a directory,
+// so mkdir and uploads don't create missing folders on the way.
+func (fs *SftpServer) requireParentDir(absPath string) error {
+	dir, _ := util.FullPath(absPath).DirAndName()
+	if dir == "/" {
+		return nil
+	}
+	entry, err := fs.getEntry(dir)
+	if err != nil {
+		return err
+	}
+	if !entry.IsDirectory {
+		return os.ErrNotExist
+	}
+	return nil
+}
+
 // removeDir deletes a directory.
 func (fs *SftpServer) removeDir(absPath string) error {
 	return fs.deleteEntry(absPath, false)

weed/sftpd/sftp_permissions_test.go

@@ -132,9 +132,82 @@ func TestNewFileWriterChecksTargetFile(t *testing.T) {
 	}
 	delete(filer.entries, "/home/alice/nested")
 	delete(filer.entries, "/home/alice/nested/ok.f")
-	if err := write("/gone/deeper.txt"); err != nil {
+	if err := fs.checkFilePermission("/home/alice/gone/deeper.txt", "write"); err != nil {
 		t.Errorf("expected write when ancestors resolve to the writable dir, got %v", err)
 	}
+	// The missing folder itself is not created on the way.
+	if err := write("/gone/deeper.txt"); err != os.ErrNotExist {
+		t.Errorf("expected os.ErrNotExist under a missing folder, got %v", err)
+	}
+}
+
+// makeDir and a committed upload must refuse a parent that is missing, and a
+// parent removed while an upload buffered must fail the close too.
+func TestRequireParentDirEnforced(t *testing.T) {
+	lis, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+	filer := &permissionTestFiler{entries: map[string]*filer_pb.Entry{
+		"/":               {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{FileMode: 0755}},
+		"/home":           {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{FileMode: 0755}},
+		"/home/alice":     {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}},
+		"/home/alice/d":   {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}},
+		"/home/alice/a.t": {Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0644}},
+	}}
+	grpcServer := grpc.NewServer()
+	filer_pb.RegisterSeaweedFilerServer(grpcServer, filer)
+	go func() { _ = grpcServer.Serve(lis) }()
+	t.Cleanup(grpcServer.Stop)
+
+	_, port, _ := net.SplitHostPort(lis.Addr().String())
+	fs := &SftpServer{
+		filerAddr:      pb.ServerAddress("127.0.0.1:0." + port),
+		grpcDialOption: grpc.WithTransportCredentials(insecure.NewCredentials()),
+		user: &user.User{
+			Username: "alice", Uid: 1000, Gid: 1000, HomeDir: "/home/alice",
+			Permissions: map[string][]string{
+				"/home/alice": {"*"},
+			},
+		},
+	}
+
+	if err := fs.makeDir("/home/alice/missing/new"); err != os.ErrNotExist {
+		t.Fatalf("mkdir under a missing parent: got %v, want os.ErrNotExist", err)
+	}
+	if _, ok := filer.entries["/home/alice/missing"]; ok {
+		t.Error("mkdir created the missing parent")
+	}
+	if _, ok := filer.entries["/home/alice/missing/new"]; ok {
+		t.Error("mkdir created a directory under a missing parent")
+	}
+
+	if err := fs.renameEntry("/home/alice/a.t", "/home/alice/missing/b.t"); err != os.ErrNotExist {
+		t.Fatalf("rename into a missing folder: got %v, want os.ErrNotExist", err)
+	}
+	if _, ok := filer.entries["/home/alice/missing/b.t"]; ok {
+		t.Error("rename moved a file into a missing folder")
+	}
+
+	if err := fs.makeDir("/home/alice/d/new"); err != nil {
+		t.Fatalf("mkdir under an existing parent: %v", err)
+	}
+	if _, ok := filer.entries["/home/alice/d/new"]; !ok {
+		t.Error("mkdir under an existing parent did not create the entry")
+	}
+
+	w, err := fs.newFileWriter(&sftp.Request{Filepath: "/d/f.txt"})
+	if err != nil {
+		t.Fatalf("open upload under an existing parent: %v", err)
+	}
+	writer, ok := w.(*SeaweedSftpFileWriter)
+	if !ok {
+		t.Fatalf("unexpected writer type %T", w)
+	}
+	delete(filer.entries, "/home/alice/d")
+	if err := writer.Close(); err != os.ErrNotExist {
+		t.Errorf("upload closing after its parent was removed: got %v, want os.ErrNotExist", err)
+	}
 }
 
 // renameEntry must honor a more specific ACL on an existing target: renaming
@@ -289,6 +362,11 @@ func (f *permissionTestFiler) AtomicRenameEntry(_ context.Context, req *filer_pb
 	return &filer_pb.AtomicRenameEntryResponse{}, nil
 }
 
+func (f *permissionTestFiler) CreateEntry(_ context.Context, req *filer_pb.CreateEntryRequest) (*filer_pb.CreateEntryResponse, error) {
+	f.entries[req.Directory+"/"+req.Entry.Name] = req.Entry
+	return &filer_pb.CreateEntryResponse{}, nil
+}
+
 // checkRenameKeepsAcl also compares the moved root (inherited restrictions) and
 // ACL paths configured under the destination (deeper grants), while configured
 // paths without a real entry do not block the rename.