🛡️ Sentinel: [CRITICAL] Fix command injection vulnerability in validation command

.jules/sentinel.md

@@ -2,3 +2,8 @@
 **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-05-24 - Argument Injection in Command Validation
+**Vulnerability:** The `run_validation` method relied on `starts_with` to check if a user-supplied command started with a safe prefix. This allowed attackers to bypass the allowlist with commands like `"cargo checkmate"` or `"cargo check; echo pwned"`.
+**Learning:** Permissive string matching (`starts_with`) on command strings is insufficient for security when the command is later parsed and executed.
+**Prevention:** Always parse the command string into its program and arguments *first*, then strictly validate against the parsed elements of the allowed prefixes.

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

@@ -56,11 +56,6 @@ async fn test_valid_command_parsing() {
 
     let status = val["validation_status"].as_str().unwrap();
 
-    // "error" usually means security error or execution error (command not found).
-    // If cargo is installed, it should be "failed" (exit code != 0) or "passed".
-    // If cargo is NOT installed, it returns "error" with "Failed to execute command: No such file or directory".
-    // We can check if it's NOT a Security Error.
-
     if status == "error" {
         let err_msg = val["validation_error"].as_str().unwrap();
         assert!(

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

@@ -23,8 +23,21 @@ impl FileService {
             "Running post-operation validation"
         );
 
-        // SECURITY: Validate the command before execution
-        // For now, we implement a simple allowlist of safe prefixes/commands
+        // Run validation command in the project root
+        // SECURITY: Parse command string first to prevent argument injection attacks where
+        // an attacker provides a command that starts with a safe prefix but appends malicious
+        // commands or arguments (e.g., "cargo check; echo pwned").
+        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 parsed command against an allowlist
         // This prevents completely arbitrary code execution from a malicious config
         // TODO: Move this policy to a robust configuration file or security policy
         let safe_prefixes = [
@@ -56,9 +69,25 @@ impl FileService {
             "make check",
         ];
 
-        let is_safe = safe_prefixes
-            .iter()
-            .any(|prefix| self.validation_config.command.trim().starts_with(prefix));
+        // Ensure the parsed program and its initial arguments exactly match a safe prefix
+        let is_safe = safe_prefixes.iter().any(|&prefix| {
+            if let Some((safe_program, safe_args)) = parse_command_line(prefix) {
+                if program != safe_program {
+                    return false;
+                }
+                if args.len() < safe_args.len() {
+                    return false;
+                }
+                for (i, safe_arg) in safe_args.iter().enumerate() {
+                    if args[i] != *safe_arg {
+                        return false;
+                    }
+                }
+                true
+            } else {
+                false
+            }
+        });
 
         if !is_safe {
             error!(
@@ -71,18 +100,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)