From 5863ab424dbc970f90d9c511e480cbf1060f50ea Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Jan 2026 05:53:23 +0000 Subject: [PATCH] security: fix minor security observations and add regression tests - Add logging when timeout parsing falls back to default (runner.rs) - Add path canonicalization in config discovery to prevent symlink attacks (config/mod.rs) - Add argument validation in pre-commit command construction to prevent injection (precommit.rs) - Add comprehensive regression tests for all security fixes - Fix clippy warnings in test code (unwrap_err -> expect_err, manual_string_new) --- benches/benchmarks.rs | 7 ++- src/checks/precommit.rs | 111 +++++++++++++++++++++++++++++++++-- src/config/mod.rs | 126 ++++++++++++++++++++++++++++++++++++++-- src/core/detector.rs | 12 ++-- src/core/runner.rs | 41 ++++++++++++- 5 files changed, 282 insertions(+), 15 deletions(-) diff --git a/benches/benchmarks.rs b/benches/benchmarks.rs index 2d5358e..dea4dcb 100644 --- a/benches/benchmarks.rs +++ b/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) }); }); } diff --git a/src/checks/precommit.rs b/src/checks/precommit.rs index 52873f9..d040994 100644 --- a/src/checks/precommit.rs +++ b/src/checks/precommit.rs @@ -30,6 +30,13 @@ pub async fn run_all(repo_root: &Path) -> Result { } /// 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 { if !is_installed() { return Err(Error::PreCommitNotFound); @@ -41,6 +48,16 @@ async fn run_with_args(repo_root: &Path, args: &[&str]) -> Result { }); } + // 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" + ); + } + } } diff --git a/src/config/mod.rs b/src/config/mod.rs index 0346449..d3d13e2 100644 --- a/src/config/mod.rs +++ b/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 { 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)); + } } diff --git a/src/core/detector.rs b/src/core/detector.rs index 0a56f1b..fe60234 100644 --- a/src/core/detector.rs +++ b/src/core/detector.rs @@ -341,7 +341,7 @@ mod tests { #[test] fn test_mode_parse_error_message() { - let err = "invalid".parse::().unwrap_err(); + let err = "invalid".parse::().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] diff --git a/src/core/runner.rs b/src/core/runner.rs index 6f8ac8a..6b473f1 100644 --- a/src/core/runner.rs +++ b/src/core/runner.rs @@ -314,7 +314,14 @@ async fn run_check_async( Mode::Agent | Mode::Ci => &config.agent.timeout, }; - let timeout = parse_duration(timeout_str).unwrap_or(Duration::from_secs(300)); + let timeout = parse_duration(timeout_str).unwrap_or_else(|| { + tracing::warn!( + timeout_str = %timeout_str, + default_secs = 300, + "Invalid timeout format, using default" + ); + Duration::from_secs(300) + }); let mut options = ExecuteOptions::default().timeout(timeout); @@ -743,4 +750,36 @@ mod tests { let parallelism = concurrency::available_parallelism(); assert!(parallelism >= 1); } + + // ========================================================================= + // Security tests - timeout parsing + // ========================================================================= + + #[test] + fn test_parse_duration_valid_formats() { + // Verify that valid duration formats are parsed correctly + assert!(parse_duration("30s").is_some()); + assert!(parse_duration("5m").is_some()); + assert!(parse_duration("1h").is_some()); + assert!(parse_duration("1h30m").is_some()); + assert!(parse_duration("15m30s").is_some()); + } + + #[test] + fn test_parse_duration_invalid_returns_none() { + // Invalid formats should return None (triggering the warning log) + assert!(parse_duration("invalid").is_none()); + assert!(parse_duration("").is_none()); + assert!(parse_duration("abc").is_none()); + assert!(parse_duration("-5s").is_none()); + // Note: These now return None, which triggers the warning log in run_check_async + } + + #[test] + fn test_parse_duration_boundary_values() { + // Test boundary values + assert!(parse_duration("0s").is_some()); + assert!(parse_duration("1s").is_some()); + assert!(parse_duration("999999s").is_some()); + } }