From 3ec5bd7e65370db1fdbb175e480a4a650c252c6f Mon Sep 17 00:00:00 2001 From: Andy Dixon Date: Fri, 2 Oct 2026 10:58:38 +0100 Subject: [PATCH] AI codebase review fix: pin parent directory and verify owner/type before trashing Trash renamed the item by path after the confirmation dialog, so a parent directory swapped for a symlink since the scan (e.g. by another user, while an administrator cleans their home) redirected the move to an unrelated file. The parent is now opened once and the item is checked (owner and file type as scanned) and renamed relative to that fd. macOS volume trash directories now get the same ownership/symlink checks as freedesktop.org ones on Linux. Co-Authored-By: Claude Opus 5.5 --- internal/platform/ops.go | 8 ++++ internal/platform/ops_darwin.go | 13 +++++-- internal/platform/ops_linux.go | 29 +------------- internal/platform/ops_linux_test.go | 57 ++++++++++++++++++++++++++-- internal/platform/ops_other.go | 2 +- internal/platform/ops_windows.go | 5 ++- internal/platform/trash_unix.go | 59 +++++++++++++++++++++++++++++ internal/tui/actions.go | 4 +- 8 files changed, 139 insertions(+), 38 deletions(-) create mode 100644 internal/platform/trash_unix.go diff --git a/internal/platform/ops.go b/internal/platform/ops.go index 1c8da8c..85bcb8f 100644 --- a/internal/platform/ops.go +++ b/internal/platform/ops.go @@ -2,6 +2,7 @@ package platform import ( "errors" + "io/fs" "os" "os/exec" "path/filepath" @@ -19,6 +20,13 @@ func startDetached(name string, args ...string) error { return nil } +// Expect is what the scan recorded for an item about to be trashed. Trash +// refuses an item whose owner or file type no longer matches. +type Expect struct { + UID uint32 + Mode fs.FileMode // only the type bits are compared +} + // checkAbs guards helper invocations: an absolute path can never be parsed // as an option by the helper program. func checkAbs(path string) error { diff --git a/internal/platform/ops_darwin.go b/internal/platform/ops_darwin.go index 6e4a481..b038a47 100644 --- a/internal/platform/ops_darwin.go +++ b/internal/platform/ops_darwin.go @@ -21,7 +21,7 @@ func Reveal(path string) error { // Trash moves path into ~/.Trash (same volume) or the volume's // .Trashes/ directory. It never copies and never deletes. -func Trash(path string) error { +func Trash(path string, want Expect) error { if err := checkAbs(path); err != nil { return err } @@ -34,8 +34,13 @@ func Trash(path string) error { dirs = append(dirs, filepath.Join(vol, ".Trashes", fmt.Sprint(os.Getuid()))) } base := filepath.Base(path) - for _, dir := range dirs { - if err := os.MkdirAll(dir, 0o700); err != nil { + for vi, dir := range dirs { + if err := os.MkdirAll(filepath.Dir(dir), 0o700); err != nil { + continue + } + // A volume trash is on a shared filesystem: another user could plant + // a symlink or an open directory there, so insist on a private one. + if err := ensurePrivateDir(dir, vi > 0); err != nil { continue } for i := 1; i < 10000; i++ { @@ -47,7 +52,7 @@ func Trash(path string) error { if _, err := os.Lstat(dst); err == nil { continue } - err := os.Rename(path, dst) + err := renameChecked(path, dst, want) if errors.Is(err, syscall.EXDEV) { break // try the next trash location } diff --git a/internal/platform/ops_linux.go b/internal/platform/ops_linux.go index 9f75f57..03eb026 100644 --- a/internal/platform/ops_linux.go +++ b/internal/platform/ops_linux.go @@ -29,7 +29,7 @@ func Reveal(path string) error { // Trash moves path to the freedesktop.org trash: the home trash when on the // same filesystem, otherwise $topdir/.Trash-$uid on the item's filesystem. // It never copies across filesystems and never deletes. -func Trash(path string) error { +func Trash(path string, want Expect) error { if err := checkAbs(path); err != nil { return err } @@ -99,7 +99,7 @@ func Trash(path string) error { os.Remove(ip) return errors.Join(werr, cerr) } - if err := os.Rename(path, dst); err != nil { + if err := renameChecked(path, dst, want); err != nil { os.Remove(ip) return err } @@ -108,31 +108,6 @@ func Trash(path string) error { return errors.New("trash: too many items with the same name") } -// ensurePrivateDir creates dir (mode 0700) if missing; an existing entry must -// be a real directory (not a symlink) owned by us. With strict (trash on a -// shared filesystem) it must also be inaccessible to others, as the -// freedesktop.org spec requires; home trash directories made by desktop -// environments are sometimes 0755, which is harmless there. -func ensurePrivateDir(dir string, strict bool) error { - if err := os.Mkdir(dir, 0o700); err != nil && !errors.Is(err, os.ErrExist) { - return err - } - fi, err := os.Lstat(dir) - if err != nil { - return err - } - st := fi.Sys().(*syscall.Stat_t) - switch { - case !fi.IsDir(): - return errors.New("not a directory (symlink?)") - case int(st.Uid) != os.Getuid(): - return errors.New("owned by another user") - case strict && fi.Mode().Perm()&0o077 != 0: - return errors.New("accessible to other users") - } - return nil -} - // mountTop returns the top directory of the filesystem containing path. func mountTop(path string, dev uint64) string { cur := filepath.Dir(path) diff --git a/internal/platform/ops_linux_test.go b/internal/platform/ops_linux_test.go index 3f32da0..9980d49 100644 --- a/internal/platform/ops_linux_test.go +++ b/internal/platform/ops_linux_test.go @@ -16,7 +16,7 @@ func TestTrashFreedesktop(t *testing.T) { if err := os.WriteFile(victim, []byte("x"), 0o644); err != nil { t.Fatal(err) } - if err := Trash(victim); err != nil { + if err := Trash(victim, want(t, victim)); err != nil { t.Fatal(err) } if _, err := os.Lstat(victim); !os.IsNotExist(err) { @@ -32,13 +32,13 @@ func TestTrashFreedesktop(t *testing.T) { } // A second item with the same name gets a distinct trash name. os.WriteFile(victim, []byte("y"), 0o644) - if err := Trash(victim); err != nil { + if err := Trash(victim, want(t, victim)); err != nil { t.Fatal(err) } if _, err := os.Lstat(trashed + ".2"); err != nil { t.Fatal("collision not handled") } - if err := Trash("relative/path"); err == nil { + if err := Trash("relative/path", Expect{}); err == nil { t.Fatal("relative paths must be refused") } } @@ -62,3 +62,54 @@ func TestEnsurePrivateDirRejectsSymlink(t *testing.T) { t.Fatal(err) } } + +func want(t *testing.T, path string) Expect { + t.Helper() + m, err := Lstat(path) + if err != nil { + t.Fatal(err) + } + return Expect{UID: m.UID, Mode: m.Mode} +} + +// A parent swapped for a symlink after the scan must not redirect the move +// to whatever the symlink points at. +func TestTrashRefusesSwappedItem(t *testing.T) { + data := t.TempDir() + t.Setenv("XDG_DATA_HOME", data) + dir := filepath.Join(data, "work") + os.Mkdir(dir, 0o755) + victim := filepath.Join(dir, "big.iso") + os.WriteFile(victim, []byte("x"), 0o644) + scanned := want(t, victim) + + // Item replaced by a symlink to a file elsewhere: type differs. + other := filepath.Join(data, "precious") + os.WriteFile(other, []byte("keep"), 0o644) + os.Remove(victim) + os.Symlink(other, victim) + scannedDir := Expect{UID: scanned.UID, Mode: os.ModeDir} + if err := Trash(victim, scanned); err == nil { + t.Fatal("symlink accepted where a regular file was scanned") + } + if err := Trash(victim, scannedDir); err == nil { + t.Fatal("type mismatch accepted") + } + if _, err := os.Lstat(victim); err != nil { + t.Fatal("refused item was moved") + } + + // Owner differs from the scan. + os.Remove(victim) + os.WriteFile(victim, []byte("x"), 0o644) + if err := Trash(victim, Expect{UID: scanned.UID + 1, Mode: scanned.Mode}); err == nil { + t.Fatal("owner mismatch accepted") + } + entries, _ := os.ReadDir(filepath.Join(data, "Trash", "info")) + if len(entries) != 0 { + t.Fatalf("refused trash left %d .trashinfo files", len(entries)) + } + if err := Trash(victim, scanned); err != nil { + t.Fatal(err) + } +} diff --git a/internal/platform/ops_other.go b/internal/platform/ops_other.go index 9794878..ee3d9d5 100644 --- a/internal/platform/ops_other.go +++ b/internal/platform/ops_other.go @@ -11,4 +11,4 @@ const TrashName = "trash" func Reveal(string) error { return errors.New("not supported on this platform") } // Trash is unsupported on this platform. -func Trash(string) error { return errors.New("trash not supported on this platform") } +func Trash(string, Expect) error { return errors.New("trash not supported on this platform") } diff --git a/internal/platform/ops_windows.go b/internal/platform/ops_windows.go index 028677d..e1c7214 100644 --- a/internal/platform/ops_windows.go +++ b/internal/platform/ops_windows.go @@ -45,8 +45,9 @@ const ( ) // Trash moves path to the Recycle Bin via SHFileOperationW with -// FOF_ALLOWUNDO. It never deletes permanently. -func Trash(path string) error { +// FOF_ALLOWUNDO. It never deletes permanently. want is not checked here: +// Windows exposes no owner or inode identity through the scan. +func Trash(path string, _ Expect) error { if err := checkAbs(path); err != nil { return err } diff --git a/internal/platform/trash_unix.go b/internal/platform/trash_unix.go new file mode 100644 index 0000000..078e815 --- /dev/null +++ b/internal/platform/trash_unix.go @@ -0,0 +1,59 @@ +//go:build linux || darwin + +package platform + +import ( + "errors" + "io/fs" + "os" + "path/filepath" + "syscall" + + "golang.org/x/sys/unix" +) + +// renameChecked moves path to dst if it still matches want. The parent +// directory is opened once and both the check and the rename are relative to +// that descriptor, so a parent swapped for a symlink since the scan (or +// between check and rename) cannot redirect the move to another user's file. +func renameChecked(path, dst string, want Expect) error { + dir, base := filepath.Dir(path), filepath.Base(path) + dfd, err := unix.Open(dir, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return &os.PathError{Op: "open", Path: dir, Err: err} + } + defer unix.Close(dfd) + var st unix.Stat_t + if err := unix.Fstatat(dfd, base, &st, unix.AT_SYMLINK_NOFOLLOW); err != nil { + return &os.PathError{Op: "stat", Path: path, Err: err} + } + if st.Uid != want.UID || modeFromUnix(uint32(st.Mode))&fs.ModeType != want.Mode&fs.ModeType { + return errors.New("item changed since the scan (owner or type differs); rescan first") + } + return unix.Renameat(dfd, base, unix.AT_FDCWD, dst) +} + +// ensurePrivateDir creates dir (mode 0700) if missing; an existing entry must +// be a real directory (not a symlink) owned by us. With strict (trash on a +// shared filesystem) it must also be inaccessible to others, as the +// freedesktop.org spec requires; home trash directories made by desktop +// environments are sometimes 0755, which is harmless there. +func ensurePrivateDir(dir string, strict bool) error { + if err := os.Mkdir(dir, 0o700); err != nil && !errors.Is(err, os.ErrExist) { + return err + } + fi, err := os.Lstat(dir) + if err != nil { + return err + } + st := fi.Sys().(*syscall.Stat_t) + switch { + case !fi.IsDir(): + return errors.New("not a directory (symlink?)") + case int(st.Uid) != os.Getuid(): + return errors.New("owned by another user") + case strict && fi.Mode().Perm()&0o077 != 0: + return errors.New("accessible to other users") + } + return nil +} diff --git a/internal/tui/actions.go b/internal/tui/actions.go index cf7af22..55fe3dc 100644 --- a/internal/tui/actions.go +++ b/internal/tui/actions.go @@ -3,6 +3,7 @@ package tui import ( "context" "fmt" + "io/fs" "time" tea "charm.land/bubbletea/v2" @@ -39,6 +40,7 @@ func (m *Model) requestTrash() tea.Cmd { } n := m.tree.Node(id) path := m.tree.Path(id) + want := platform.Expect{UID: n.UID, Mode: fs.FileMode(n.Mode)} what := "file" if n.IsDir() { what = fmt.Sprintf("directory with %s files", textutil.Count(int64(n.Files))) @@ -51,7 +53,7 @@ func (m *Model) requestTrash() tea.Cmd { yes: "move to " + platform.TrashName, onYes: func(m *Model) tea.Cmd { t := m.tree - return func() tea.Msg { return trashDoneMsg{t, id, platform.Trash(path)} } + return func() tea.Msg { return trashDoneMsg{t, id, platform.Trash(path, want)} } }, }) return nil