Skip to content

Commit 951e395

Browse files
committed
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)
1 parent f6acfa7 commit 951e395

File tree

3 files changed

+67
-24
lines changed

3 files changed

+67
-24
lines changed

cmd/analyze/delete.go

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,11 @@ func moveToTrash(path string) error {
126126
return fmt.Errorf("failed to resolve path: %w", err)
127127
}
128128

129+
// Validate path to prevent path traversal attacks.
130+
if err := validatePath(absPath); err != nil {
131+
return err
132+
}
133+
129134
// Escape path for AppleScript (handle quotes and backslashes).
130135
escapedPath := strings.ReplaceAll(absPath, "\\", "\\\\")
131136
escapedPath = strings.ReplaceAll(escapedPath, "\"", "\\\"")
@@ -146,3 +151,23 @@ func moveToTrash(path string) error {
146151

147152
return nil
148153
}
154+
155+
// validatePath checks path safety for external commands.
156+
// Returns error if path is empty, relative, contains null bytes, or escapes root.
157+
func validatePath(path string) error {
158+
if path == "" {
159+
return fmt.Errorf("path is empty")
160+
}
161+
if !filepath.IsAbs(path) {
162+
return fmt.Errorf("path must be absolute: %s", path)
163+
}
164+
if strings.Contains(path, "\x00") {
165+
return fmt.Errorf("path contains null bytes")
166+
}
167+
// Ensure Clean doesn't radically alter the path (path traversal check).
168+
clean := filepath.Clean(path)
169+
if !strings.HasPrefix(clean, "/") {
170+
return fmt.Errorf("path escapes root: %s", path)
171+
}
172+
return nil
173+
}

cmd/analyze/main.go

Lines changed: 22 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -775,18 +775,14 @@ func (m model) updateKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
775775
}
776776
for path := range m.largeMultiSelected {
777777
go func(p string) {
778-
ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout)
779-
defer cancel()
780-
_ = exec.CommandContext(ctx, "open", p).Run()
778+
_ = safeOpen(p, false)
781779
}(path)
782780
}
783781
m.status = fmt.Sprintf("Opening %d items...", count)
784782
} else {
785783
selected := m.largeFiles[m.largeSelected]
786784
go func(path string) {
787-
ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout)
788-
defer cancel()
789-
_ = exec.CommandContext(ctx, "open", path).Run()
785+
_ = safeOpen(path, false)
790786
}(selected.Path)
791787
m.status = fmt.Sprintf("Opening %s...", selected.Name)
792788
}
@@ -800,18 +796,14 @@ func (m model) updateKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
800796
}
801797
for path := range m.multiSelected {
802798
go func(p string) {
803-
ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout)
804-
defer cancel()
805-
_ = exec.CommandContext(ctx, "open", p).Run()
799+
_ = safeOpen(p, false)
806800
}(path)
807801
}
808802
m.status = fmt.Sprintf("Opening %d items...", count)
809803
} else {
810804
selected := m.entries[m.selected]
811805
go func(path string) {
812-
ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout)
813-
defer cancel()
814-
_ = exec.CommandContext(ctx, "open", path).Run()
806+
_ = safeOpen(path, false)
815807
}(selected.Path)
816808
m.status = fmt.Sprintf("Opening %s...", selected.Name)
817809
}
@@ -829,18 +821,14 @@ func (m model) updateKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
829821
}
830822
for path := range m.largeMultiSelected {
831823
go func(p string) {
832-
ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout)
833-
defer cancel()
834-
_ = exec.CommandContext(ctx, "open", "-R", p).Run()
824+
_ = safeOpen(p, true)
835825
}(path)
836826
}
837827
m.status = fmt.Sprintf("Showing %d items in Finder...", count)
838828
} else {
839829
selected := m.largeFiles[m.largeSelected]
840830
go func(path string) {
841-
ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout)
842-
defer cancel()
843-
_ = exec.CommandContext(ctx, "open", "-R", path).Run()
831+
_ = safeOpen(path, true)
844832
}(selected.Path)
845833
m.status = fmt.Sprintf("Showing %s in Finder...", selected.Name)
846834
}
@@ -854,18 +842,14 @@ func (m model) updateKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
854842
}
855843
for path := range m.multiSelected {
856844
go func(p string) {
857-
ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout)
858-
defer cancel()
859-
_ = exec.CommandContext(ctx, "open", "-R", p).Run()
845+
_ = safeOpen(p, true)
860846
}(path)
861847
}
862848
m.status = fmt.Sprintf("Showing %d items in Finder...", count)
863849
} else {
864850
selected := m.entries[m.selected]
865851
go func(path string) {
866-
ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout)
867-
defer cancel()
868-
_ = exec.CommandContext(ctx, "open", "-R", path).Run()
852+
_ = safeOpen(path, true)
869853
}(selected.Path)
870854
m.status = fmt.Sprintf("Showing %s in Finder...", selected.Name)
871855
}
@@ -1172,3 +1156,17 @@ func scanOverviewPathCmd(path string, index int) tea.Cmd {
11721156
}
11731157
}
11741158
}
1159+
1160+
// safeOpen executes 'open' command with path validation.
1161+
func safeOpen(path string, reveal bool) error {
1162+
if err := validatePath(path); err != nil {
1163+
return err
1164+
}
1165+
ctx, cancel := context.WithTimeout(context.Background(), openCommandTimeout)
1166+
defer cancel()
1167+
args := []string{path}
1168+
if reveal {
1169+
args = []string{"-R", path}
1170+
}
1171+
return exec.CommandContext(ctx, "open", args...).Run()
1172+
}

cmd/analyze/scanner.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,16 @@ func calculateDirSizeFast(root string, filesScanned, dirsScanned, bytesScanned *
409409

410410
// Use Spotlight (mdfind) to quickly find large files.
411411
func findLargeFilesWithSpotlight(root string, minSize int64) []fileEntry {
412+
// Validate root path.
413+
if err := validatePath(root); err != nil {
414+
return nil
415+
}
416+
417+
// Validate minSize is reasonable (non-negative and not excessively large).
418+
if minSize < 0 || minSize > 1<<50 { // 1 PB max
419+
return nil
420+
}
421+
412422
query := fmt.Sprintf("kMDItemFSSize >= %d", minSize)
413423

414424
ctx, cancel := context.WithTimeout(context.Background(), mdlsTimeout)
@@ -635,6 +645,16 @@ func getDirectorySizeFromDu(path string) (int64, error) {
635645
}
636646

637647
func getDirectorySizeFromDuWithExclude(path string, excludePath string) (int64, error) {
648+
// Validate paths.
649+
if err := validatePath(path); err != nil {
650+
return 0, err
651+
}
652+
if excludePath != "" {
653+
if err := validatePath(excludePath); err != nil {
654+
return 0, err
655+
}
656+
}
657+
638658
runDuSize := func(target string) (int64, error) {
639659
if _, err := os.Stat(target); err != nil {
640660
return 0, err

0 commit comments

Comments
 (0)