security: fix minor security observations and add regression tests

benches/benchmarks.rs

@@ -1,5 +1,8 @@
 //! Benchmarks for agent-precommit.
 
+#![allow(missing_docs)]
+#![allow(let_underscore_drop)]
+
 use criterion::{black_box, criterion_group, criterion_main, Criterion};
 
 fn benchmark_mode_detection(c: &mut Criterion) {
@@ -25,7 +28,9 @@ timeout = "15m"
 
     c.bench_function("config_parsing", |b| {
         b.iter(|| {
-            let _: toml::Value = toml::from_str(black_box(toml_content)).unwrap();
+            let result: toml::Value =
+                toml::from_str(black_box(toml_content)).expect("parse config");
+            black_box(result)
         });
     });
 }

src/checks/precommit.rs

@@ -30,6 +30,13 @@ pub async fn run_all(repo_root: &Path) -> Result<bool> {
 }
 
 /// Runs pre-commit with custom arguments.
+///
+/// # Security
+///
+/// This function only accepts arguments from hardcoded sources within this crate
+/// (e.g., `&["--all-files"]`). Arguments are validated to ensure they start with
+/// `-` to prevent command injection if this function is ever called with
+/// user-controlled input in the future.
 async fn run_with_args(repo_root: &Path, args: &[&str]) -> Result<bool> {
     if !is_installed() {
         return Err(Error::PreCommitNotFound);
@@ -41,6 +48,16 @@ async fn run_with_args(repo_root: &Path, args: &[&str]) -> Result<bool> {
         });
     }
 
+    // Security: Validate that all arguments look like flags (start with -)
+    // This prevents potential command injection if args were ever user-controlled
+    for arg in args {
+        if !arg.starts_with('-') {
+            return Err(Error::Internal {
+                message: format!("Invalid pre-commit argument: {arg}"),
+            });
+        }
+    }
+
     let cmd = if args.is_empty() {
         "pre-commit run".to_string()
     } else {
@@ -142,7 +159,7 @@ repos:
 
         let result = run_staged(temp.path()).await;
         assert!(result.is_err());
-        let err = result.unwrap_err();
+        let err = result.expect_err("should return PreCommitNotFound");
         assert!(matches!(err, Error::PreCommitNotFound));
     }
 
@@ -158,7 +175,7 @@ repos:
 
         let result = run_all(temp.path()).await;
         assert!(result.is_err());
-        let err = result.unwrap_err();
+        let err = result.expect_err("should return PreCommitNotFound");
         assert!(matches!(err, Error::PreCommitNotFound));
     }
 
@@ -174,7 +191,7 @@ repos:
 
         let result = run_staged(temp.path()).await;
         assert!(result.is_err());
-        let err = result.unwrap_err();
+        let err = result.expect_err("should return PreCommitConfigNotFound");
         assert!(matches!(err, Error::PreCommitConfigNotFound { .. }));
     }
 
@@ -190,7 +207,7 @@ repos:
 
         let result = run_all(temp.path()).await;
         assert!(result.is_err());
-        let err = result.unwrap_err();
+        let err = result.expect_err("should return PreCommitConfigNotFound");
         assert!(matches!(err, Error::PreCommitConfigNotFound { .. }));
     }
 
@@ -218,4 +235,90 @@ repos:
         let expected_path = temp.path().join(PRE_COMMIT_CONFIG);
         assert!(expected_path.ends_with(".pre-commit-config.yaml"));
     }
+
+    // =========================================================================
+    // Security tests - argument validation
+    // =========================================================================
+
+    #[tokio::test]
+    async fn test_run_with_args_rejects_non_flag_arguments() {
+        // This test verifies that non-flag arguments are rejected
+        // to prevent potential command injection
+        let temp = TempDir::new().expect("create temp dir");
+        std::fs::write(temp.path().join(PRE_COMMIT_CONFIG), "repos: []").expect("write config");
+
+        // Skip if pre-commit is not installed - the validation happens before execution
+        if !is_installed() {
+            return;
+        }
+
+        // Test with a potentially malicious argument that doesn't start with -
+        let result = run_with_args(temp.path(), &["--all-files", "; echo pwned"]).await;
+        assert!(result.is_err());
+        let err = result.expect_err("should reject non-flag argument");
+        assert!(matches!(err, Error::Internal { .. }));
+    }
+
+    #[tokio::test]
+    async fn test_run_with_args_accepts_valid_flags() {
+        // This test verifies that valid flag arguments are accepted
+        let temp = TempDir::new().expect("create temp dir");
+        std::fs::write(temp.path().join(PRE_COMMIT_CONFIG), "repos: []").expect("write config");
+
+        // Skip if pre-commit is not installed
+        if !is_installed() {
+            return;
+        }
+
+        // Valid flags should pass validation (execution may still fail, but validation passes)
+        // We can't fully test this without pre-commit being properly configured,
+        // but the validation step should pass for valid flags
+        let result = run_with_args(temp.path(), &["--all-files"]).await;
+        // Result could be Ok or Err depending on pre-commit execution,
+        // but it should NOT be an Internal error from validation
+        if let Err(ref e) = result {
+            assert!(
+                !matches!(e, Error::Internal { .. }),
+                "Valid flags should not cause validation error"
+            );
+        }
+    }
+
+    #[tokio::test]
+    async fn test_run_with_args_rejects_path_injection() {
+        // Test that path-like arguments are rejected
+        let temp = TempDir::new().expect("create temp dir");
+        std::fs::write(temp.path().join(PRE_COMMIT_CONFIG), "repos: []").expect("write config");
+
+        if !is_installed() {
+            return;
+        }
+
+        let result = run_with_args(temp.path(), &["/etc/passwd"]).await;
+        assert!(result.is_err());
+        let err = result.expect_err("should reject path injection");
+        assert!(matches!(err, Error::Internal { .. }));
+    }
+
+    #[tokio::test]
+    async fn test_run_with_args_empty_is_valid() {
+        // Empty args should be valid
+        let temp = TempDir::new().expect("create temp dir");
+        std::fs::write(temp.path().join(PRE_COMMIT_CONFIG), "repos: []").expect("write config");
+
+        if !is_installed() {
+            return;
+        }
+
+        // Empty args should pass validation
+        let result = run_with_args(temp.path(), &[]).await;
+        // Result could be Ok or Err depending on pre-commit execution,
+        // but it should NOT be an Internal error from validation
+        if let Err(ref e) = result {
+            assert!(
+                !matches!(e, Error::Internal { .. }),
+                "Empty args should not cause validation error"
+            );
+        }
+    }
 }

src/config/mod.rs

@@ -69,14 +69,28 @@ impl Config {
     }
 
     /// Finds the configuration file by searching up the directory tree.
+    ///
+    /// # Security
+    ///
+    /// This function canonicalizes paths to prevent symlink attacks where
+    /// a malicious symlink could redirect config loading to an unexpected location.
     pub fn find_config_file() -> Result<PathBuf> {
         let cwd = std::env::current_dir().map_err(|e| Error::io("get current dir", e))?;
 
+        // Canonicalize the starting directory to resolve symlinks
+        let cwd = cwd
+            .canonicalize()
+            .map_err(|e| Error::io("canonicalize current dir", e))?;
+
         let mut current = cwd.as_path();
         loop {
             let config_path = current.join(CONFIG_FILE_NAME);
             if config_path.exists() {
-                return Ok(config_path);
+                // Canonicalize the config path to ensure it resolves to a real location
+                let canonical_path = config_path
+                    .canonicalize()
+                    .map_err(|e| Error::io("canonicalize config path", e))?;
+                return Ok(canonical_path);
             }
 
             match current.parent() {
@@ -644,7 +658,7 @@ mod tests {
         config.human.timeout = "invalid".to_string();
         let result = config.validate();
         assert!(result.is_err());
-        let err_msg = result.unwrap_err().to_string();
+        let err_msg = result.expect_err("should fail for invalid timeout").to_string();
         assert!(err_msg.contains("Invalid duration"));
     }
 
@@ -671,7 +685,7 @@ mod tests {
         config.checks.insert(
             "placeholder-check".to_string(),
             CheckConfig {
-                run: "".to_string(),
+                run: String::new(),
                 description: "Test".to_string(),
                 enabled_if: None,
                 env: HashMap::new(),
@@ -861,7 +875,7 @@ mod tests {
             env: HashMap::new(),
         };
         assert!(check.enabled_if.is_some());
-        let condition = check.enabled_if.as_ref().unwrap();
+        let condition = check.enabled_if.as_ref().expect("enabled_if should be Some");
         assert_eq!(condition.file_exists, Some("Cargo.toml".to_string()));
     }
 
@@ -1009,4 +1023,108 @@ description = "Test"
         let debug_str = format!("{:?}", config);
         assert!(debug_str.contains("Config"));
     }
+
+    // =========================================================================
+    // Security tests - path canonicalization
+    // =========================================================================
+
+    #[test]
+    fn test_find_config_file_returns_canonical_path() {
+        use tempfile::TempDir;
+
+        let temp = TempDir::new().expect("create temp dir");
+        let config_path = temp.path().join(CONFIG_FILE_NAME);
+
+        // Write a valid config
+        let config = Config::default();
+        let toml_str = toml::to_string_pretty(&config).expect("serialize");
+        std::fs::write(&config_path, toml_str).expect("write config");
+
+        // Change to temp directory and find config
+        let original_dir = std::env::current_dir().expect("get cwd");
+        std::env::set_current_dir(temp.path()).expect("change to temp dir");
+
+        let result = Config::find_config_file();
+        std::env::set_current_dir(original_dir).expect("restore cwd");
+
+        assert!(result.is_ok());
+        let found_path = result.expect("find config");
+
+        // The path should be absolute (canonicalized)
+        assert!(found_path.is_absolute());
+        // The path should exist
+        assert!(found_path.exists());
+    }
+
+    #[test]
+    #[cfg(unix)]
+    fn test_find_config_file_resolves_symlinks() {
+        use std::os::unix::fs::symlink;
+        use tempfile::TempDir;
+
+        let temp = TempDir::new().expect("create temp dir");
+        let real_dir = temp.path().join("real");
+        let link_dir = temp.path().join("link");
+
+        std::fs::create_dir(&real_dir).expect("create real dir");
+
+        // Create config in real directory
+        let config = Config::default();
+        let toml_str = toml::to_string_pretty(&config).expect("serialize");
+        std::fs::write(real_dir.join(CONFIG_FILE_NAME), toml_str).expect("write config");
+
+        // Create symlink to real directory
+        symlink(&real_dir, &link_dir).expect("create symlink");
+
+        // Change to symlinked directory and find config
+        let original_dir = std::env::current_dir().expect("get cwd");
+        std::env::set_current_dir(&link_dir).expect("change to link dir");
+
+        let result = Config::find_config_file();
+        std::env::set_current_dir(original_dir).expect("restore cwd");
+
+        assert!(result.is_ok());
+        let found_path = result.expect("find config");
+
+        // The path should be resolved to the real location (not through symlink)
+        // After canonicalization, the path should not contain "link"
+        let path_str = found_path.to_string_lossy();
+        assert!(
+            !path_str.contains("link"),
+            "Path should be canonicalized: {path_str}"
+        );
+        assert!(
+            path_str.contains("real"),
+            "Path should resolve to real dir: {path_str}"
+        );
+    }
+
+    #[test]
+    fn test_find_config_file_walks_up_canonicalized_tree() {
+        use tempfile::TempDir;
+
+        let temp = TempDir::new().expect("create temp dir");
+        let nested = temp.path().join("src/lib/utils");
+        std::fs::create_dir_all(&nested).expect("create nested dirs");
+
+        // Create config at root
+        let config = Config::default();
+        let toml_str = toml::to_string_pretty(&config).expect("serialize");
+        std::fs::write(temp.path().join(CONFIG_FILE_NAME), toml_str).expect("write config");
+
+        // Change to nested directory and find config
+        let original_dir = std::env::current_dir().expect("get cwd");
+        std::env::set_current_dir(&nested).expect("change to nested dir");
+
+        let result = Config::find_config_file();
+        std::env::set_current_dir(original_dir).expect("restore cwd");
+
+        assert!(result.is_ok());
+        let found_path = result.expect("find config");
+
+        // Should find the config in the parent directory
+        assert!(found_path.is_absolute());
+        assert!(found_path.exists());
+        assert!(found_path.ends_with(CONFIG_FILE_NAME));
+    }
 }

src/core/detector.rs

@@ -341,7 +341,7 @@ mod tests {
 
     #[test]
     fn test_mode_parse_error_message() {
-        let err = "invalid".parse::<Mode>().unwrap_err();
+        let err = "invalid".parse::<Mode>().expect_err("should fail to parse invalid");
         assert!(err.contains("Invalid mode"));
         assert!(err.contains("human, agent, or ci"));
     }
@@ -495,8 +495,9 @@ mod tests {
     // =========================================================================
 
     #[test]
-    fn test_known_agent_env_vars_not_empty() {
-        assert!(!KNOWN_AGENT_ENV_VARS.is_empty());
+    fn test_known_agent_env_vars_has_expected_count() {
+        // Verify we have a reasonable number of known agent env vars
+        assert!(KNOWN_AGENT_ENV_VARS.len() >= 10);
     }
 
     #[test]
@@ -515,8 +516,9 @@ mod tests {
     }
 
     #[test]
-    fn test_known_ci_env_vars_not_empty() {
-        assert!(!KNOWN_CI_ENV_VARS.is_empty());
+    fn test_known_ci_env_vars_has_expected_count() {
+        // Verify we have a reasonable number of known CI env vars
+        assert!(KNOWN_CI_ENV_VARS.len() >= 10);
     }
 
     #[test]