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 <noreply@anthropic.com>

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 {

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/<uid> 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
 			}

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)

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)
+	}
+}

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") }

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
 	}

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
+}

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