From 951e395ab70e7b1016434bb34d813e618642c1c8 Mon Sep 17 00:00:00 2001 From: Tw93 Date: Sat, 14 Mar 2026 08:24:08 +0800 Subject: [PATCH] security: fix CodeQL command injection and path traversal alerts - Add validatePath() helper to check path safety before external commands - Validate paths in delete.go (moveToTrash), scanner.go (mdfind, du), and main.go (open command) - Remove overly restrictive character whitelist that rejected valid macOS paths (Chinese, emoji, $, ;, etc.) - Unify path validation logic across all three files Fixes CodeQL alerts: - Command injection in osascript (delete.go) - Command injection in mdfind/du (scanner.go) - Path traversal in open command (main.go) --- cmd/analyze/delete.go | 25 +++++++++++++++++++++++ cmd/analyze/main.go | 46 ++++++++++++++++++++---------------------- cmd/analyze/scanner.go | 20 ++++++++++++++++++ 3 files changed, 67 insertions(+), 24 deletions(-) diff --git a/cmd/analyze/delete.go b/cmd/analyze/delete.go index 196d751..a26a4d9 100644 --- a/cmd/analyze/delete.go +++ b/cmd/analyze/delete.go @@ -126,6 +126,11 @@ func moveToTrash(path string) error { return fmt.Errorf("failed to resolve path: %w", err) } + // Validate path to prevent path traversal attacks. + if err := validatePath(absPath); err != nil { + return err + } + // Escape path for AppleScript (handle quotes and backslashes). escapedPath := strings.ReplaceAll(absPath, "\\", "\\\\") escapedPath = strings.ReplaceAll(escapedPath, "\"", "\\\"") @@ -146,3 +151,23 @@ func moveToTrash(path string) error { return nil } + +// validatePath checks path safety for external commands. +// Returns error if path is empty, relative, contains null bytes, or escapes root. +func validatePath(path string) error { + if path == "" { + return fmt.Errorf("path is empty") + } + if !filepath.IsAbs(path) { + return fmt.Errorf("path must be absolute: %s", path) + } + if strings.Contains(path, "\x00") { + return fmt.Errorf("path contains null bytes") + } + // Ensure Clean doesn't radically alter the path (path traversal check). + clean := filepath.Clean(path) + if !strings.HasPrefix(clean, "/") { + return fmt.Errorf("path escapes root: %s", path) + } + return nil +} diff --git a/cmd/analyze/main.go b/cmd/analyze/main.go index c8ed030..9a6af40 100644 --- a/cmd/analyze/main.go +++ b/cmd/analyze/main.go @@ -775,18 +775,14 @@ func (m model) updateKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { } for path := range m.largeMultiSelected { go func(p string) { - ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout) - defer cancel() - _ = exec.CommandContext(ctx, "open", p).Run() + _ = safeOpen(p, false) }(path) } m.status = fmt.Sprintf("Opening %d items...", count) } else { selected := m.largeFiles[m.largeSelected] go func(path string) { - ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout) - defer cancel() - _ = exec.CommandContext(ctx, "open", path).Run() + _ = safeOpen(path, false) }(selected.Path) m.status = fmt.Sprintf("Opening %s...", selected.Name) } @@ -800,18 +796,14 @@ func (m model) updateKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { } for path := range m.multiSelected { go func(p string) { - ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout) - defer cancel() - _ = exec.CommandContext(ctx, "open", p).Run() + _ = safeOpen(p, false) }(path) } m.status = fmt.Sprintf("Opening %d items...", count) } else { selected := m.entries[m.selected] go func(path string) { - ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout) - defer cancel() - _ = exec.CommandContext(ctx, "open", path).Run() + _ = safeOpen(path, false) }(selected.Path) m.status = fmt.Sprintf("Opening %s...", selected.Name) } @@ -829,18 +821,14 @@ func (m model) updateKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { } for path := range m.largeMultiSelected { go func(p string) { - ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout) - defer cancel() - _ = exec.CommandContext(ctx, "open", "-R", p).Run() + _ = safeOpen(p, true) }(path) } m.status = fmt.Sprintf("Showing %d items in Finder...", count) } else { selected := m.largeFiles[m.largeSelected] go func(path string) { - ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout) - defer cancel() - _ = exec.CommandContext(ctx, "open", "-R", path).Run() + _ = safeOpen(path, true) }(selected.Path) m.status = fmt.Sprintf("Showing %s in Finder...", selected.Name) } @@ -854,18 +842,14 @@ func (m model) updateKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { } for path := range m.multiSelected { go func(p string) { - ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout) - defer cancel() - _ = exec.CommandContext(ctx, "open", "-R", p).Run() + _ = safeOpen(p, true) }(path) } m.status = fmt.Sprintf("Showing %d items in Finder...", count) } else { selected := m.entries[m.selected] go func(path string) { - ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout) - defer cancel() - _ = exec.CommandContext(ctx, "open", "-R", path).Run() + _ = safeOpen(path, true) }(selected.Path) m.status = fmt.Sprintf("Showing %s in Finder...", selected.Name) } @@ -1172,3 +1156,17 @@ func scanOverviewPathCmd(path string, index int) tea.Cmd { } } } + +// safeOpen executes 'open' command with path validation. +func safeOpen(path string, reveal bool) error { + if err := validatePath(path); err != nil { + return err + } + ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout) + defer cancel() + args := []string{path} + if reveal { + args = []string{"-R", path} + } + return exec.CommandContext(ctx, "open", args...).Run() +} diff --git a/cmd/analyze/scanner.go b/cmd/analyze/scanner.go index eac8fda..960de83 100644 --- a/cmd/analyze/scanner.go +++ b/cmd/analyze/scanner.go @@ -409,6 +409,16 @@ func calculateDirSizeFast(root string, filesScanned, dirsScanned, bytesScanned * // Use Spotlight (mdfind) to quickly find large files. func findLargeFilesWithSpotlight(root string, minSize int64) []fileEntry { + // Validate root path. + if err := validatePath(root); err != nil { + return nil + } + + // Validate minSize is reasonable (non-negative and not excessively large). + if minSize < 0 || minSize > 1<<50 { // 1 PB max + return nil + } + query := fmt.Sprintf("kMDItemFSSize >= %d", minSize) ctx, cancel := context.WithTimeout(context.Background(), mdlsTimeout) @@ -635,6 +645,16 @@ func getDirectorySizeFromDu(path string) (int64, error) { } func getDirectorySizeFromDuWithExclude(path string, excludePath string) (int64, error) { + // Validate paths. + if err := validatePath(path); err != nil { + return 0, err + } + if excludePath != "" { + if err := validatePath(excludePath); err != nil { + return 0, err + } + } + runDuSize := func(target string) (int64, error) { if _, err := os.Stat(target); err != nil { return 0, err