diff --git a/weed/sftpd/sftp_copy_existing_test.go b/weed/sftpd/sftp_copy_existing_test.go index 5f603107a7..1597d5d8b4 100644 --- a/weed/sftpd/sftp_copy_existing_test.go +++ b/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) + } +} diff --git a/weed/sftpd/sftp_file_writer.go b/weed/sftpd/sftp_file_writer.go index 3f6d915b0d..4687598eed 100644 --- a/weed/sftpd/sftp_file_writer.go +++ b/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) } diff --git a/weed/sftpd/sftp_filer.go b/weed/sftpd/sftp_filer.go index 70ff8669bf..f173eddb0a 100644 --- a/weed/sftpd/sftp_filer.go +++ b/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) diff --git a/weed/sftpd/sftp_permissions_test.go b/weed/sftpd/sftp_permissions_test.go index e9dbc70132..8dff396299 100644 --- a/weed/sftpd/sftp_permissions_test.go +++ b/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.