🛡️ Sentinel: [CRITICAL/HIGH] Fix command injection

.jules/sentinel.md

@@ -0,0 +1,5 @@
+## 2024-11-20 - Defense-in-Depth against Command Injection
+
+**Vulnerability:** Use of `/bin/bash -c` string pipelines with dynamic substitution to probe developer cache paths and run category cleanup commands (like `docker system prune` and `xcrun simctl`).
+**Learning:** Even static shell commands in system utilities expose unnecessary risk surface because path logic and environments can be hijacked by downstream agents or environment configurations.
+**Prevention:** Eliminate all `/bin/bash` wrappers. Refactor shell pipelines (like `head -1 | sed`) directly into Swift `String` manipulations and structure all process commands using `[[String]]` step arrays routed via `URL(fileURLWithPath: "/usr/bin/env")`. Use identical `Pipe()` handles to replace `2>&1` securely.

Sources/Cacheout/Cleaner/CacheCleaner.swift

@@ -16,8 +16,8 @@
 /// ## Custom Clean Commands
 ///
 /// Categories with a `cleanCommand` (e.g., Simulator Devices) bypass file deletion
-/// entirely. The command runs via `/bin/bash -c` with a 30-second timeout and a
-/// restricted `PATH` environment. If the command times out, the process is terminated
+/// entirely. The steps run via direct execution bypassing the shell with a 30-second timeout
+/// per step and a restricted `PATH` environment. If the step times out, the process is terminated
 /// and an error is reported.
 ///
 /// ## Cleanup Logging
@@ -46,10 +46,10 @@ actor CacheCleaner {
         for result in results where result.isSelected && !result.isEmpty {
             var categoryFreed: Int64 = 0
 
-            // If the category has a custom clean command, run it instead of deleting files
-            if let command = result.category.cleanCommand {
+            // If the category has custom clean steps, run them instead of deleting files
+            if let steps = result.category.cleanSteps {
                 do {
-                    try runCleanCommand(command)
+                    try runCleanSteps(steps)
                     categoryFreed = result.sizeBytes
                 } catch {
                     errors.append((result.category.name, error.localizedDescription))
@@ -96,37 +96,41 @@ actor CacheCleaner {
         return CleanupReport(cleaned: cleaned, errors: errors)
     }
 
-    /// Run a custom clean command via /bin/bash with a 30-second timeout.
-    private func runCleanCommand(_ command: String) throws {
-        let process = Process()
-        process.executableURL = URL(fileURLWithPath: "/bin/bash")
-        process.arguments = ["-c", command]
-        process.standardOutput = FileHandle.nullDevice
-        process.standardError = FileHandle.nullDevice
-        process.environment = [
-            "PATH": "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin",
-            "HOME": FileManager.default.homeDirectoryForCurrentUser.path
-        ]
-
-        try process.run()
-
-        let deadline = DispatchTime.now() + .seconds(30)
-        let group = DispatchGroup()
-        group.enter()
-        DispatchQueue.global().async {
-            process.waitUntilExit()
-            group.leave()
-        }
+    /// Run custom clean steps via direct process execution with a 30-second timeout per step.
+    private func runCleanSteps(_ steps: [[String]]) throws {
+        for step in steps {
+            guard !step.isEmpty else { continue }
+
+            let process = Process()
+            process.executableURL = URL(fileURLWithPath: "/usr/bin/env")
+            process.arguments = step
+            process.standardOutput = FileHandle.nullDevice
+            process.standardError = FileHandle.nullDevice
+            process.environment = [
+                "PATH": "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin",
+                "HOME": FileManager.default.homeDirectoryForCurrentUser.path
+            ]
+
+            try process.run()
+
+            let deadline = DispatchTime.now() + .seconds(30)
+            let group = DispatchGroup()
+            group.enter()
+            DispatchQueue.global().async {
+                process.waitUntilExit()
+                group.leave()
+            }
 
-        if group.wait(timeout: deadline) == .timedOut {
-            process.terminate()
-            throw NSError(domain: "CacheCleaner", code: -1,
-                          userInfo: [NSLocalizedDescriptionKey: "Clean command timed out after 30s"])
-        }
+            if group.wait(timeout: deadline) == .timedOut {
+                process.terminate()
+                throw NSError(domain: "CacheCleaner", code: -1,
+                              userInfo: [NSLocalizedDescriptionKey: "Clean step timed out after 30s"])
+            }
 
-        guard process.terminationStatus == 0 else {
-            throw NSError(domain: "CacheCleaner", code: Int(process.terminationStatus),
-                          userInfo: [NSLocalizedDescriptionKey: "Clean command exited with status \(process.terminationStatus)"])
+            guard process.terminationStatus == 0 else {
+                throw NSError(domain: "CacheCleaner", code: Int(process.terminationStatus),
+                              userInfo: [NSLocalizedDescriptionKey: "Clean step exited with status \(process.terminationStatus)"])
+            }
         }
     }
 

Sources/Cacheout/Models/CacheCategory.swift

@@ -24,9 +24,9 @@
 ///
 /// ### Custom Clean Commands
 ///
-/// Some categories (like Simulator Devices) require a specialized cleanup command
-/// instead of simple file deletion. The `cleanCommand` property holds an optional
-/// shell command that the `CacheCleaner` runs via `/bin/bash -c` with a 30-second timeout.
+/// Some categories (like Simulator Devices) require specialized cleanup steps
+/// instead of simple file deletion. The `cleanSteps` property holds optional
+/// commands that the `CacheCleaner` runs directly with a 30-second timeout.
 
 import Foundation
 
@@ -59,10 +59,10 @@ enum PathDiscovery: Hashable {
     /// Always checked via FileManager.fileExists.
     case staticPath(String)
 
-    /// Run a shell command that outputs the cache path on stdout.
+    /// Run a direct process that outputs the cache path on stdout.
     /// Falls back to `fallbacks` if the command fails or path doesn't exist.
     /// The `requiresTool` is checked via `/usr/bin/which` before probing.
-    case probed(command: String, requiresTool: String?, fallbacks: [String])
+    case probed(executable: String, arguments: [String], requiresTool: String?, fallbacks: [String], transform: ((String) -> String?)? = nil)
 
     /// Absolute path (not relative to home). Used for system-level paths
     /// that live outside ~/
@@ -80,10 +80,10 @@ struct CacheCategory: Identifiable, Hashable {
     let rebuildNote: String
     let defaultSelected: Bool
 
-    /// Optional shell command to run for cleanup instead of deleting files.
-    /// When set, the cleaner runs this command instead of rm/trash.
-    /// The command runs via /bin/bash -c.
-    let cleanCommand: String?
+    /// Optional structured steps to run for cleanup instead of deleting files.
+    /// When set, the cleaner runs these steps sequentially instead of rm/trash.
+    /// Commands are run via direct execution, bypassing the shell.
+    let cleanSteps: [[String]]?
 
     func hash(into hasher: inout Hasher) { hasher.combine(id) }
     static func == (lhs: CacheCategory, rhs: CacheCategory) -> Bool { lhs.id == rhs.id }
@@ -104,14 +104,14 @@ struct CacheCategory: Identifiable, Hashable {
         self.riskLevel = riskLevel
         self.rebuildNote = rebuildNote
         self.defaultSelected = defaultSelected
-        self.cleanCommand = nil
+        self.cleanSteps = nil
     }
 
     /// Full init with discovery and optional clean command
     init(
         name: String, slug: String, description: String, icon: String,
         discovery: [PathDiscovery], riskLevel: RiskLevel, rebuildNote: String,
-        defaultSelected: Bool, cleanCommand: String? = nil
+        defaultSelected: Bool, cleanSteps: [[String]]? = nil
     ) {
         self.name = name
         self.slug = slug
@@ -121,7 +121,7 @@ struct CacheCategory: Identifiable, Hashable {
         self.riskLevel = riskLevel
         self.rebuildNote = rebuildNote
         self.defaultSelected = defaultSelected
-        self.cleanCommand = cleanCommand
+        self.cleanSteps = cleanSteps
     }
 
     // MARK: - Path Resolution
@@ -146,14 +146,15 @@ struct CacheCategory: Identifiable, Hashable {
                     results.append(url)
                 }
 
-            case .probed(let command, let requiresTool, let fallbacks):
+            case .probed(let executable, let arguments, let requiresTool, let fallbacks, let transform):
                 // Check if required tool is installed
                 if let tool = requiresTool, !toolExists(tool) {
                     continue
                 }
 
                 // Try the probe command
-                if let probedPath = runProbe(command),
+                if let probedOutput = runProbe(executable: executable, arguments: arguments),
+                   let probedPath = transform?(probedOutput) ?? defaultTransform(probedOutput),
                    directoryExists(at: URL(fileURLWithPath: probedPath)) {
                     results.append(URL(fileURLWithPath: probedPath))
                     continue
@@ -186,23 +187,26 @@ struct CacheCategory: Identifiable, Hashable {
     }
 
     private func toolExists(_ tool: String) -> Bool {
-        let result = shell("/usr/bin/which \(tool)")
+        let result = runCommand(executable: "which", arguments: [tool])
         return result != nil && !result!.isEmpty
     }
 
-    private func runProbe(_ command: String) -> String? {
-        guard let output = shell(command) else { return nil }
+    private func defaultTransform(_ output: String) -> String? {
         let trimmed = output.trimmingCharacters(in: .whitespacesAndNewlines)
         return trimmed.isEmpty ? nil : trimmed
     }
 
-    /// Run a shell command with a 2-second timeout. Returns stdout or nil.
-    private func shell(_ command: String) -> String? {
+    private func runProbe(executable: String, arguments: [String]) -> String? {
+        return runCommand(executable: executable, arguments: arguments)
+    }
+
+    /// Run a process directly with a 2-second timeout. Returns stdout or nil.
+    private func runCommand(executable: String, arguments: [String]) -> String? {
         let process = Process()
         let pipe = Pipe()
 
-        process.executableURL = URL(fileURLWithPath: "/bin/bash")
-        process.arguments = ["-c", command]
+        process.executableURL = URL(fileURLWithPath: "/usr/bin/env")
+        process.arguments = [executable] + arguments
         process.standardOutput = pipe
         process.standardError = FileHandle.nullDevice
         process.environment = [
@@ -236,3 +240,33 @@ struct CacheCategory: Identifiable, Hashable {
         return String(data: data, encoding: .utf8)
     }
 }
+
+// Equality and Hashable ignores closures, which is fine since they're only in PathDiscovery.probed
+extension PathDiscovery {
+    static func == (lhs: PathDiscovery, rhs: PathDiscovery) -> Bool {
+        switch (lhs, rhs) {
+        case let (.staticPath(l), .staticPath(r)): return l == r
+        case let (.absolutePath(l), .absolutePath(r)): return l == r
+        case let (.probed(le, la, lr, lf, _), .probed(re, ra, rr, rf, _)):
+            return le == re && la == ra && lr == rr && lf == rf
+        default: return false
+        }
+    }
+
+    func hash(into hasher: inout Hasher) {
+        switch self {
+        case .staticPath(let path):
+            hasher.combine(0)
+            hasher.combine(path)
+        case .absolutePath(let path):
+            hasher.combine(1)
+            hasher.combine(path)
+        case .probed(let executable, let arguments, let requiresTool, let fallbacks, _):
+            hasher.combine(2)
+            hasher.combine(executable)
+            hasher.combine(arguments)
+            hasher.combine(requiresTool)
+            hasher.combine(fallbacks)
+        }
+    }
+}

Sources/Cacheout/Scanner/Categories.swift

@@ -66,7 +66,11 @@ extension CacheCategory {
             riskLevel: .review,
             rebuildNote: "Recreated when you use Simulator. Run 'xcrun simctl delete unavailable' for targeted cleanup.",
             defaultSelected: false,
-            cleanCommand: "xcrun simctl shutdown all 2>/dev/null; xcrun simctl delete unavailable 2>/dev/null; xcrun simctl erase all 2>/dev/null"
+            cleanSteps: [
+                ["xcrun", "simctl", "shutdown", "all"],
+                ["xcrun", "simctl", "delete", "unavailable"],
+                ["xcrun", "simctl", "erase", "all"]
+            ]
         ),        CacheCategory(
             name: "Swift PM Cache",
             slug: "swift_pm_cache",
@@ -84,9 +88,19 @@ extension CacheCategory {
             icon: "leaf.fill",
             discovery: [
                 .probed(
-                    command: "pod cache list --short 2>/dev/null | head -1 | sed 's|/[^/]*$||'",
+                    executable: "pod",
+                    arguments: ["cache", "list", "--short"],
                     requiresTool: "pod",
-                    fallbacks: ["Library/Caches/CocoaPods"]
+                    fallbacks: ["Library/Caches/CocoaPods"],
+                    transform: { output in
+                        // Equivalent to `head -1 | sed 's|/[^/]*$||'`
+                        let firstLine = output.components(separatedBy: "\n").first ?? ""
+                        let components = firstLine.components(separatedBy: "/")
+                        if components.count > 1 {
+                            return components.dropLast().joined(separator: "/")
+                        }
+                        return firstLine.trimmingCharacters(in: .whitespacesAndNewlines)
+                    }
                 )
             ],
             riskLevel: .safe,
@@ -105,7 +119,8 @@ extension CacheCategory {
             icon: "mug.fill",
             discovery: [
                 .probed(
-                    command: "brew --cache 2>/dev/null",
+                    executable: "brew",
+                    arguments: ["--cache"],
                     requiresTool: "brew",
                     fallbacks: ["Library/Caches/Homebrew"]
                 )
@@ -126,7 +141,8 @@ extension CacheCategory {
             icon: "shippingbox.fill",
             discovery: [
                 .probed(
-                    command: "npm config get cache 2>/dev/null",
+                    executable: "npm",
+                    arguments: ["config", "get", "cache"],
                     requiresTool: "npm",
                     fallbacks: [".npm/_cacache", ".npm"]
                 )
@@ -141,7 +157,8 @@ extension CacheCategory {
             icon: "link",
             discovery: [
                 .probed(
-                    command: "yarn cache dir 2>/dev/null",
+                    executable: "yarn",
+                    arguments: ["cache", "dir"],
                     requiresTool: "yarn",
                     fallbacks: ["Library/Caches/Yarn"]
                 )
@@ -157,7 +174,8 @@ extension CacheCategory {
             icon: "archivebox.fill",
             discovery: [
                 .probed(
-                    command: "pnpm store path 2>/dev/null",
+                    executable: "pnpm",
+                    arguments: ["store", "path"],
                     requiresTool: "pnpm",
                     fallbacks: ["Library/pnpm/store", ".local/share/pnpm/store"]
                 )
@@ -173,9 +191,11 @@ extension CacheCategory {
             icon: "hare.fill",
             discovery: [
                 .probed(
-                    command: "echo dummy", // bun doesn't have a cache-dir command
+                    executable: "echo",
+                    arguments: ["dummy"], // bun doesn't have a cache-dir command
                     requiresTool: "bun",
-                    fallbacks: [".bun/install/cache"]
+                    fallbacks: [".bun/install/cache"],
+                    transform: { _ in return nil } // Force fallback behavior
                 )
             ],
             riskLevel: .safe,
@@ -189,9 +209,11 @@ extension CacheCategory {
             icon: "wrench.and.screwdriver.fill",
             discovery: [
                 .probed(
-                    command: "echo dummy",
+                    executable: "echo",
+                    arguments: ["dummy"],
                     requiresTool: "node",
-                    fallbacks: ["Library/Caches/node-gyp"]
+                    fallbacks: ["Library/Caches/node-gyp"],
+                    transform: { _ in return nil } // Force fallback behavior
                 )
             ],
             riskLevel: .safe,
@@ -219,7 +241,8 @@ extension CacheCategory {
             icon: "puzzlepiece.fill",
             discovery: [
                 .probed(
-                    command: "pip3 cache dir 2>/dev/null || python3 -m pip cache dir 2>/dev/null",
+                    executable: "python3",
+                    arguments: ["-m", "pip", "cache", "dir"],
                     requiresTool: nil, // python3 is always on macOS
                     fallbacks: ["Library/Caches/pip", "Library/Caches/pip-tools"]
                 )
@@ -235,7 +258,8 @@ extension CacheCategory {
             icon: "bolt.fill",
             discovery: [
                 .probed(
-                    command: "uv cache dir 2>/dev/null",
+                    executable: "uv",
+                    arguments: ["cache", "dir"],
                     requiresTool: "uv",
                     fallbacks: [".cache/uv"]
                 )
@@ -280,12 +304,14 @@ extension CacheCategory {
             icon: "cube.fill",
             discovery: [
                 .probed(
-                    command: "echo dummy",
+                    executable: "echo",
+                    arguments: ["dummy"],
                     requiresTool: "docker",
                     fallbacks: [
                         "Library/Containers/com.docker.docker/Data/vms/0/data",
                         "Library/Containers/com.docker.docker/Data"
-                    ]
+                    ],
+                    transform: { _ in return nil } // Force fallback behavior
                 )
             ],
             riskLevel: .caution,