From ebb4f7a1e91bd27481d86ea86902aa2432b966a7 Mon Sep 17 00:00:00 2001 From: Tw93 Date: Sat, 10 Jan 2026 07:24:58 +0800 Subject: [PATCH] feat(analyze): safer deletion with Trash and two-key confirm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Change delete confirmation from double-delete to Delete→Enter - Move files to macOS Trash instead of permanent deletion - Allow file recovery from Trash if accidentally deleted - Update UI prompts to show 'Press Enter to confirm' - Skip Finder-dependent tests in CI environments - Update SECURITY_AUDIT.md with new safety mechanisms Closes #288 --- SECURITY_AUDIT.md | 3 +- cmd/analyze/analyze_test.go | 16 +++--- cmd/analyze/delete.go | 105 ++++++++++++++++++++++-------------- cmd/analyze/delete_test.go | 54 +++++++++++++++++-- cmd/analyze/main.go | 2 +- cmd/analyze/view.go | 4 +- 6 files changed, 129 insertions(+), 55 deletions(-) diff --git a/SECURITY_AUDIT.md b/SECURITY_AUDIT.md index 04b87a0..60e0c2c 100644 --- a/SECURITY_AUDIT.md +++ b/SECURITY_AUDIT.md @@ -123,7 +123,8 @@ The analyzer (`mo analyze`) uses a distinct security model: - Runs with standard user permissions only. - Respects macOS System Integrity Protection (SIP). -- Requires explicit user confirmation for all deletions. +- **Two-Key Confirmation:** Deletion requires ⌫ (Delete) to enter confirmation mode, then Enter to confirm. Prevents accidental double-press of the same key. +- **Trash Instead of Delete:** Files are moved to macOS Trash using Finder's native API, allowing easy recovery if needed. - OS-level enforcement (cannot delete `/System` due to Read-Only Volume). **Code:** `cmd/analyze/*.go` diff --git a/cmd/analyze/analyze_test.go b/cmd/analyze/analyze_test.go index 213f5c2..6ae8d2e 100644 --- a/cmd/analyze/analyze_test.go +++ b/cmd/analyze/analyze_test.go @@ -90,6 +90,11 @@ func TestScanPathConcurrentBasic(t *testing.T) { } func TestDeletePathWithProgress(t *testing.T) { + // Skip in CI environments where Finder may not be available. + if os.Getenv("CI") != "" { + t.Skip("Skipping Finder-dependent test in CI") + } + parent := t.TempDir() target := filepath.Join(parent, "target") if err := os.MkdirAll(target, 0o755); err != nil { @@ -107,18 +112,15 @@ func TestDeletePathWithProgress(t *testing.T) { } var counter int64 - count, err := deletePathWithProgress(target, &counter) + count, err := trashPathWithProgress(target, &counter) if err != nil { - t.Fatalf("deletePathWithProgress returned error: %v", err) + t.Fatalf("trashPathWithProgress returned error: %v", err) } if count != int64(len(files)) { - t.Fatalf("expected %d files removed, got %d", len(files), count) - } - if got := atomic.LoadInt64(&counter); got != count { - t.Fatalf("counter mismatch: want %d, got %d", count, got) + t.Fatalf("expected %d files trashed, got %d", len(files), count) } if _, err := os.Stat(target); !os.IsNotExist(err) { - t.Fatalf("expected target to be removed, stat err=%v", err) + t.Fatalf("expected target to be moved to Trash, stat err=%v", err) } } diff --git a/cmd/analyze/delete.go b/cmd/analyze/delete.go index 3204241..04a7fa2 100644 --- a/cmd/analyze/delete.go +++ b/cmd/analyze/delete.go @@ -1,19 +1,24 @@ package main import ( - "io/fs" + "context" + "fmt" "os" + "os/exec" "path/filepath" "sort" "strings" "sync/atomic" + "time" tea "github.com/charmbracelet/bubbletea" ) +const trashTimeout = 30 * time.Second + func deletePathCmd(path string, counter *int64) tea.Cmd { return func() tea.Msg { - count, err := deletePathWithProgress(path, counter) + count, err := trashPathWithProgress(path, counter) return deleteProgressMsg{ done: true, err: err, @@ -23,20 +28,20 @@ func deletePathCmd(path string, counter *int64) tea.Cmd { } } -// deleteMultiplePathsCmd deletes paths and aggregates results. +// deleteMultiplePathsCmd moves paths to Trash and aggregates results. func deleteMultiplePathsCmd(paths []string, counter *int64) tea.Cmd { return func() tea.Msg { var totalCount int64 var errors []string - // Delete deeper paths first to avoid parent/child conflicts. + // Process deeper paths first to avoid parent/child conflicts. pathsToDelete := append([]string(nil), paths...) sort.Slice(pathsToDelete, func(i, j int) bool { return strings.Count(pathsToDelete[i], string(filepath.Separator)) > strings.Count(pathsToDelete[j], string(filepath.Separator)) }) for _, path := range pathsToDelete { - count, err := deletePathWithProgress(path, counter) + count, err := trashPathWithProgress(path, counter) totalCount += count if err != nil { if os.IsNotExist(err) { @@ -72,48 +77,70 @@ func (e *multiDeleteError) Error() string { return strings.Join(e.errors[:min(3, len(e.errors))], "; ") } -func deletePathWithProgress(root string, counter *int64) (int64, error) { +// trashPathWithProgress moves a path to Trash using Finder. +// This allows users to recover accidentally deleted files. +func trashPathWithProgress(root string, counter *int64) (int64, error) { + // Verify path exists. + info, err := os.Stat(root) + if err != nil { + return 0, err + } + + // Count items for progress reporting. var count int64 - var firstErr error - - err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { - if err != nil { - // Skip permission errors but continue. - if os.IsPermission(err) { - if firstErr == nil { - firstErr = err - } - return filepath.SkipDir + if info.IsDir() { + _ = filepath.WalkDir(root, func(_ string, d os.DirEntry, err error) error { + if err != nil { + return nil } - if firstErr == nil { - firstErr = err - } - return nil - } - - if !d.IsDir() { - if removeErr := os.Remove(path); removeErr == nil { + if !d.IsDir() { count++ if counter != nil { atomic.StoreInt64(counter, count) } - } else if firstErr == nil { - firstErr = removeErr } - } - - return nil - }) - - if err != nil && firstErr == nil { - firstErr = err - } - - if removeErr := os.RemoveAll(root); removeErr != nil { - if firstErr == nil { - firstErr = removeErr + return nil + }) + } else { + count = 1 + if counter != nil { + atomic.StoreInt64(counter, 1) } } - return count, firstErr + // Move to Trash using Finder AppleScript. + if err := moveToTrash(root); err != nil { + return 0, err + } + + return count, nil +} + +// moveToTrash uses macOS Finder to move a file/directory to Trash. +// This is the safest method as it uses the system's native trash mechanism. +func moveToTrash(path string) error { + absPath, err := filepath.Abs(path) + if err != nil { + return fmt.Errorf("failed to resolve path: %w", err) + } + + // Escape path for AppleScript (handle quotes and backslashes). + escapedPath := strings.ReplaceAll(absPath, "\\", "\\\\") + escapedPath = strings.ReplaceAll(escapedPath, "\"", "\\\"") + + script := fmt.Sprintf(`tell application "Finder" to delete POSIX file "%s"`, escapedPath) + + ctx, cancel := context.WithTimeout(context.Background(), trashTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, "osascript", "-e", script) + output, err := cmd.CombinedOutput() + if err != nil { + if ctx.Err() == context.DeadlineExceeded { + return fmt.Errorf("timeout moving to Trash") + } + return fmt.Errorf("failed to move to Trash: %s", strings.TrimSpace(string(output))) + } + + return nil } diff --git a/cmd/analyze/delete_test.go b/cmd/analyze/delete_test.go index 6039367..9e48b22 100644 --- a/cmd/analyze/delete_test.go +++ b/cmd/analyze/delete_test.go @@ -6,7 +6,47 @@ import ( "testing" ) +func TestTrashPathWithProgress(t *testing.T) { + // Skip in CI environments where Finder may not be available. + if os.Getenv("CI") != "" { + t.Skip("Skipping Finder-dependent test in CI") + } + + parent := t.TempDir() + target := filepath.Join(parent, "target") + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatalf("create target: %v", err) + } + + files := []string{ + filepath.Join(target, "one.txt"), + filepath.Join(target, "two.txt"), + } + for _, f := range files { + if err := os.WriteFile(f, []byte("content"), 0o644); err != nil { + t.Fatalf("write %s: %v", f, err) + } + } + + var counter int64 + count, err := trashPathWithProgress(target, &counter) + if err != nil { + t.Fatalf("trashPathWithProgress returned error: %v", err) + } + if count != int64(len(files)) { + t.Fatalf("expected %d files trashed, got %d", len(files), count) + } + if _, err := os.Stat(target); !os.IsNotExist(err) { + t.Fatalf("expected target to be moved to Trash, stat err=%v", err) + } +} + func TestDeleteMultiplePathsCmdHandlesParentChild(t *testing.T) { + // Skip in CI environments where Finder may not be available. + if os.Getenv("CI") != "" { + t.Skip("Skipping Finder-dependent test in CI") + } + base := t.TempDir() parent := filepath.Join(base, "parent") child := filepath.Join(parent, "child") @@ -32,12 +72,16 @@ func TestDeleteMultiplePathsCmdHandlesParentChild(t *testing.T) { t.Fatalf("unexpected error: %v", progress.err) } if progress.count != 2 { - t.Fatalf("expected 2 files deleted, got %d", progress.count) + t.Fatalf("expected 2 files trashed, got %d", progress.count) } if _, err := os.Stat(parent); !os.IsNotExist(err) { - t.Fatalf("expected parent to be removed, err=%v", err) - } - if _, err := os.Stat(child); !os.IsNotExist(err) { - t.Fatalf("expected child to be removed, err=%v", err) + t.Fatalf("expected parent to be moved to Trash, err=%v", err) + } +} + +func TestMoveToTrashNonExistent(t *testing.T) { + err := moveToTrash("/nonexistent/path/that/does/not/exist") + if err == nil { + t.Fatal("expected error for non-existent path") } } diff --git a/cmd/analyze/main.go b/cmd/analyze/main.go index bba4647..712d70b 100644 --- a/cmd/analyze/main.go +++ b/cmd/analyze/main.go @@ -530,7 +530,7 @@ func (m model) updateKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { // Delete confirm flow. if m.deleteConfirm { switch msg.String() { - case "delete", "backspace": + case "enter": m.deleteConfirm = false m.deleting = true var deleteCount int64 diff --git a/cmd/analyze/view.go b/cmd/analyze/view.go index 67ae52b..f2845a9 100644 --- a/cmd/analyze/view.go +++ b/cmd/analyze/view.go @@ -390,12 +390,12 @@ func (m model) View() string { } if deleteCount > 1 { - fmt.Fprintf(&b, "%sDelete:%s %d items (%s) %sPress ⌫ again | ESC cancel%s\n", + fmt.Fprintf(&b, "%sDelete:%s %d items (%s) %sPress Enter to confirm | ESC cancel%s\n", colorRed, colorReset, deleteCount, humanizeBytes(totalDeleteSize), colorGray, colorReset) } else { - fmt.Fprintf(&b, "%sDelete:%s %s (%s) %sPress ⌫ again | ESC cancel%s\n", + fmt.Fprintf(&b, "%sDelete:%s %s (%s) %sPress Enter to confirm | ESC cancel%s\n", colorRed, colorReset, m.deleteTarget.Name, humanizeBytes(m.deleteTarget.Size), colorGray, colorReset)