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

.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-06-04 - Argument Injection in Command Allowlists
+**Vulnerability:** The `run_validation` command allowlist in `mill-services` verified if user-provided commands were safe by checking if the command string started with allowed prefixes using `.starts_with()`. This allowed bypassing the restriction by appending malicious strings without spaces (e.g., `cargo test-malicious`).
+**Learning:** Using simple string operations like `.starts_with()` for validating commands against a prefix allowlist is insufficient and prone to argument injection bypasses. The underlying shell or command runner parses arguments based on spaces and quotes, not arbitrary character positions.
+**Prevention:** Always parse the command string into its component parts (program and arguments) *before* validating against an allowlist. Ensure exact matching of the program name and each individual allowed argument.

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

@@ -56,21 +56,6 @@ impl FileService {
             "make check",
         ];
 
-        let is_safe = safe_prefixes
-            .iter()
-            .any(|prefix| self.validation_config.command.trim().starts_with(prefix));
-
-        if !is_safe {
-            error!(
-                command = %self.validation_config.command,
-                "Validation command blocked by security policy. Command must start with a known safe prefix (e.g., 'cargo', 'npm', 'go', 'make')."
-            );
-            return Some(json!({
-                "validation_status": "error",
-                "validation_error": format!("Security Error: Command '{}' is not in the allowed list.", self.validation_config.command)
-            }));
-        }
-
         // 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) {
@@ -83,6 +68,42 @@ impl FileService {
             }
         };
 
+        // SECURITY: Validate the parsed command against the allowlist.
+        // We parse both the target command and the allowed prefixes, then ensure
+        // exact matching of the program name and each prefix argument.
+        // This prevents argument injection bypasses (e.g., "cargo test-malicious")
+        // that could occur if we merely checked if the command string started
+        // with the prefix using `.starts_with()`.
+        let is_safe = safe_prefixes.iter().any(|prefix| {
+            if let Some((safe_prog, safe_args)) = parse_command_line(prefix) {
+                if program != safe_prog {
+                    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!(
+                command = %self.validation_config.command,
+                "Validation command blocked by security policy. Command must start with a known safe prefix (e.g., 'cargo', 'npm', 'go', 'make')."
+            );
+            return Some(json!({
+                "validation_status": "error",
+                "validation_error": format!("Security Error: Command '{}' is not in the allowed list.", self.validation_config.command)
+            }));
+        }
+
         let output = match Command::new(&program)
             .args(&args)
             .current_dir(&self.project_root)