🛡️ Sentinel: [CRITICAL] Fix argument injection in command allowlist

.jules/sentinel.md

@@ -2,3 +2,7 @@
 **Vulnerability:** The `OperationQueue` worker in `mill-server` executed file operations (create, write, delete, rename) using raw paths from the operation object without validating they were within the project root.
 **Learning:** Background workers that process serialized operations are a common bypass for security checks enforced at the API layer. The API layer might validate the request, but if the worker is "dumb" and blindly executes the queued operation, an internal attacker or a buggy component can exploit it.
 **Prevention:** Validation must happen at the *execution point* (in the worker), not just at the ingestion point. We introduced `validate_path` in the worker loop to enforce project root containment using `canonicalize` (handling non-existent files correctly).
+## 2025-06-09 - Argument Injection Mitigation
+**Vulnerability:** The command allowlist in `run_validation` used permissive string matching (`starts_with`) to validate user commands, allowing argument injection (e.g., `cargo test --malicious-arg`).
+**Learning:** Permissive string matching on command strings is insufficient for security validations and can be bypassed by appending unexpected arguments or shell characters.
+**Prevention:** Parse the command string into a program and arguments first, then strictly match them against the exact parsed elements of the allowed prefixes.

crates/mill-services/src/services/filesystem/file_service/utils.rs

@@ -23,6 +23,18 @@ impl FileService {
             "Running post-operation validation"
         );
 
+        // Run validation command in the project root
+        // SECURITY: Parse command string to avoid shell injection
+        let (program, args) = match parse_command_line(&self.validation_config.command) {
+            Some(res) => res,
+            None => {
+                return Some(json!({
+                    "validation_status": "error",
+                    "validation_error": "Empty validation command"
+                }));
+            }
+        };
+
         // SECURITY: Validate the command before execution
         // For now, we implement a simple allowlist of safe prefixes/commands
         // This prevents completely arbitrary code execution from a malicious config
@@ -56,9 +68,19 @@ impl FileService {
             "make check",
         ];
 
-        let is_safe = safe_prefixes
-            .iter()
-            .any(|prefix| self.validation_config.command.trim().starts_with(prefix));
+        // SECURITY: Avoid permissive string checks like `starts_with` which are vulnerable to argument injection.
+        // Instead, parse the command string into a program and its arguments first, then strictly match them
+        // against the exact parsed elements of the allowed prefixes.
+        let is_safe = safe_prefixes.iter().any(|prefix| {
+            if let Some((prefix_prog, prefix_args)) = parse_command_line(prefix) {
+                if program != prefix_prog || args.len() < prefix_args.len() {
+                    return false;
+                }
+                args.iter().zip(prefix_args.iter()).all(|(a, b)| a == b)
+            } else {
+                false
+            }
+        });
 
         if !is_safe {
             error!(
@@ -71,18 +93,6 @@ impl FileService {
             }));
         }
 
-        // Run validation command in the project root
-        // SECURITY: Parse command string to avoid shell injection
-        let (program, args) = match parse_command_line(&self.validation_config.command) {
-            Some(res) => res,
-            None => {
-                return Some(json!({
-                    "validation_status": "error",
-                    "validation_error": "Empty validation command"
-                }));
-            }
-        };
-
         let output = match Command::new(&program)
             .args(&args)
             .current_dir(&self.project_root)