Skip to content

Commit 5863ab4

Browse files
claudeclaude
committed
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)
1 parent 15120a6 commit 5863ab4

5 files changed

Lines changed: 282 additions & 15 deletions

File tree

‎benches/benchmarks.rs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
//! Benchmarks for agent-precommit.
22
3+
#![allow(missing_docs)]
4+
#![allow(let_underscore_drop)]
5+
36
use criterion::{black_box, criterion_group, criterion_main, Criterion};
47

58
fn benchmark_mode_detection(c: &mut Criterion) {
@@ -25,7 +28,9 @@ timeout = "15m"
2528

2629
c.bench_function("config_parsing", |b| {
2730
b.iter(|| {
28-
let _: toml::Value = toml::from_str(black_box(toml_content)).unwrap();
31+
let result: toml::Value =
32+
toml::from_str(black_box(toml_content)).expect("parse config");
33+
black_box(result)
2934
});
3035
});
3136
}

‎src/checks/precommit.rs‎

Lines changed: 107 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,13 @@ pub async fn run_all(repo_root: &Path) -> Result<bool> {
3030
}
3131

3232
/// Runs pre-commit with custom arguments.
33+
///
34+
/// # Security
35+
///
36+
/// This function only accepts arguments from hardcoded sources within this crate
37+
/// (e.g., `&["--all-files"]`). Arguments are validated to ensure they start with
38+
/// `-` to prevent command injection if this function is ever called with
39+
/// user-controlled input in the future.
3340
async fn run_with_args(repo_root: &Path, args: &[&str]) -> Result<bool> {
3441
if !is_installed() {
3542
return Err(Error::PreCommitNotFound);
@@ -41,6 +48,16 @@ async fn run_with_args(repo_root: &Path, args: &[&str]) -> Result<bool> {
4148
});
4249
}
4350

51+
// Security: Validate that all arguments look like flags (start with -)
52+
// This prevents potential command injection if args were ever user-controlled
53+
for arg in args {
54+
if !arg.starts_with('-') {
55+
return Err(Error::Internal {
56+
message: format!("Invalid pre-commit argument: {arg}"),
57+
});
58+
}
59+
}
60+
4461
let cmd = if args.is_empty() {
4562
"pre-commit run".to_string()
4663
} else {
@@ -142,7 +159,7 @@ repos:
142159

143160
let result = run_staged(temp.path()).await;
144161
assert!(result.is_err());
145-
let err = result.unwrap_err();
162+
let err = result.expect_err("should return PreCommitNotFound");
146163
assert!(matches!(err, Error::PreCommitNotFound));
147164
}
148165

@@ -158,7 +175,7 @@ repos:
158175

159176
let result = run_all(temp.path()).await;
160177
assert!(result.is_err());
161-
let err = result.unwrap_err();
178+
let err = result.expect_err("should return PreCommitNotFound");
162179
assert!(matches!(err, Error::PreCommitNotFound));
163180
}
164181

@@ -174,7 +191,7 @@ repos:
174191

175192
let result = run_staged(temp.path()).await;
176193
assert!(result.is_err());
177-
let err = result.unwrap_err();
194+
let err = result.expect_err("should return PreCommitConfigNotFound");
178195
assert!(matches!(err, Error::PreCommitConfigNotFound { .. }));
179196
}
180197

@@ -190,7 +207,7 @@ repos:
190207

191208
let result = run_all(temp.path()).await;
192209
assert!(result.is_err());
193-
let err = result.unwrap_err();
210+
let err = result.expect_err("should return PreCommitConfigNotFound");
194211
assert!(matches!(err, Error::PreCommitConfigNotFound { .. }));
195212
}
196213

@@ -218,4 +235,90 @@ repos:
218235
let expected_path = temp.path().join(PRE_COMMIT_CONFIG);
219236
assert!(expected_path.ends_with(".pre-commit-config.yaml"));
220237
}
238+
239+
// =========================================================================
240+
// Security tests - argument validation
241+
// =========================================================================
242+
243+
#[tokio::test]
244+
async fn test_run_with_args_rejects_non_flag_arguments() {
245+
// This test verifies that non-flag arguments are rejected
246+
// to prevent potential command injection
247+
let temp = TempDir::new().expect("create temp dir");
248+
std::fs::write(temp.path().join(PRE_COMMIT_CONFIG), "repos: []").expect("write config");
249+
250+
// Skip if pre-commit is not installed - the validation happens before execution
251+
if !is_installed() {
252+
return;
253+
}
254+
255+
// Test with a potentially malicious argument that doesn't start with -
256+
let result = run_with_args(temp.path(), &["--all-files", "; echo pwned"]).await;
257+
assert!(result.is_err());
258+
let err = result.expect_err("should reject non-flag argument");
259+
assert!(matches!(err, Error::Internal { .. }));
260+
}
261+
262+
#[tokio::test]
263+
async fn test_run_with_args_accepts_valid_flags() {
264+
// This test verifies that valid flag arguments are accepted
265+
let temp = TempDir::new().expect("create temp dir");
266+
std::fs::write(temp.path().join(PRE_COMMIT_CONFIG), "repos: []").expect("write config");
267+
268+
// Skip if pre-commit is not installed
269+
if !is_installed() {
270+
return;
271+
}
272+
273+
// Valid flags should pass validation (execution may still fail, but validation passes)
274+
// We can't fully test this without pre-commit being properly configured,
275+
// but the validation step should pass for valid flags
276+
let result = run_with_args(temp.path(), &["--all-files"]).await;
277+
// Result could be Ok or Err depending on pre-commit execution,
278+
// but it should NOT be an Internal error from validation
279+
if let Err(ref e) = result {
280+
assert!(
281+
!matches!(e, Error::Internal { .. }),
282+
"Valid flags should not cause validation error"
283+
);
284+
}
285+
}
286+
287+
#[tokio::test]
288+
async fn test_run_with_args_rejects_path_injection() {
289+
// Test that path-like arguments are rejected
290+
let temp = TempDir::new().expect("create temp dir");
291+
std::fs::write(temp.path().join(PRE_COMMIT_CONFIG), "repos: []").expect("write config");
292+
293+
if !is_installed() {
294+
return;
295+
}
296+
297+
let result = run_with_args(temp.path(), &["/etc/passwd"]).await;
298+
assert!(result.is_err());
299+
let err = result.expect_err("should reject path injection");
300+
assert!(matches!(err, Error::Internal { .. }));
301+
}
302+
303+
#[tokio::test]
304+
async fn test_run_with_args_empty_is_valid() {
305+
// Empty args should be valid
306+
let temp = TempDir::new().expect("create temp dir");
307+
std::fs::write(temp.path().join(PRE_COMMIT_CONFIG), "repos: []").expect("write config");
308+
309+
if !is_installed() {
310+
return;
311+
}
312+
313+
// Empty args should pass validation
314+
let result = run_with_args(temp.path(), &[]).await;
315+
// Result could be Ok or Err depending on pre-commit execution,
316+
// but it should NOT be an Internal error from validation
317+
if let Err(ref e) = result {
318+
assert!(
319+
!matches!(e, Error::Internal { .. }),
320+
"Empty args should not cause validation error"
321+
);
322+
}
323+
}
221324
}

‎src/config/mod.rs‎

Lines changed: 122 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,14 +69,28 @@ impl Config {
6969
}
7070

7171
/// Finds the configuration file by searching up the directory tree.
72+
///
73+
/// # Security
74+
///
75+
/// This function canonicalizes paths to prevent symlink attacks where
76+
/// a malicious symlink could redirect config loading to an unexpected location.
7277
pub fn find_config_file() -> Result<PathBuf> {
7378
let cwd = std::env::current_dir().map_err(|e| Error::io("get current dir", e))?;
7479

80+
// Canonicalize the starting directory to resolve symlinks
81+
let cwd = cwd
82+
.canonicalize()
83+
.map_err(|e| Error::io("canonicalize current dir", e))?;
84+
7585
let mut current = cwd.as_path();
7686
loop {
7787
let config_path = current.join(CONFIG_FILE_NAME);
7888
if config_path.exists() {
79-
return Ok(config_path);
89+
// Canonicalize the config path to ensure it resolves to a real location
90+
let canonical_path = config_path
91+
.canonicalize()
92+
.map_err(|e| Error::io("canonicalize config path", e))?;
93+
return Ok(canonical_path);
8094
}
8195

8296
match current.parent() {
@@ -644,7 +658,7 @@ mod tests {
644658
config.human.timeout = "invalid".to_string();
645659
let result = config.validate();
646660
assert!(result.is_err());
647-
let err_msg = result.unwrap_err().to_string();
661+
let err_msg = result.expect_err("should fail for invalid timeout").to_string();
648662
assert!(err_msg.contains("Invalid duration"));
649663
}
650664

@@ -671,7 +685,7 @@ mod tests {
671685
config.checks.insert(
672686
"placeholder-check".to_string(),
673687
CheckConfig {
674-
run: "".to_string(),
688+
run: String::new(),
675689
description: "Test".to_string(),
676690
enabled_if: None,
677691
env: HashMap::new(),
@@ -861,7 +875,7 @@ mod tests {
861875
env: HashMap::new(),
862876
};
863877
assert!(check.enabled_if.is_some());
864-
let condition = check.enabled_if.as_ref().unwrap();
878+
let condition = check.enabled_if.as_ref().expect("enabled_if should be Some");
865879
assert_eq!(condition.file_exists, Some("Cargo.toml".to_string()));
866880
}
867881

@@ -1009,4 +1023,108 @@ description = "Test"
10091023
let debug_str = format!("{:?}", config);
10101024
assert!(debug_str.contains("Config"));
10111025
}
1026+
1027+
// =========================================================================
1028+
// Security tests - path canonicalization
1029+
// =========================================================================
1030+
1031+
#[test]
1032+
fn test_find_config_file_returns_canonical_path() {
1033+
use tempfile::TempDir;
1034+
1035+
let temp = TempDir::new().expect("create temp dir");
1036+
let config_path = temp.path().join(CONFIG_FILE_NAME);
1037+
1038+
// Write a valid config
1039+
let config = Config::default();
1040+
let toml_str = toml::to_string_pretty(&config).expect("serialize");
1041+
std::fs::write(&config_path, toml_str).expect("write config");
1042+
1043+
// Change to temp directory and find config
1044+
let original_dir = std::env::current_dir().expect("get cwd");
1045+
std::env::set_current_dir(temp.path()).expect("change to temp dir");
1046+
1047+
let result = Config::find_config_file();
1048+
std::env::set_current_dir(original_dir).expect("restore cwd");
1049+
1050+
assert!(result.is_ok());
1051+
let found_path = result.expect("find config");
1052+
1053+
// The path should be absolute (canonicalized)
1054+
assert!(found_path.is_absolute());
1055+
// The path should exist
1056+
assert!(found_path.exists());
1057+
}
1058+
1059+
#[test]
1060+
#[cfg(unix)]
1061+
fn test_find_config_file_resolves_symlinks() {
1062+
use std::os::unix::fs::symlink;
1063+
use tempfile::TempDir;
1064+
1065+
let temp = TempDir::new().expect("create temp dir");
1066+
let real_dir = temp.path().join("real");
1067+
let link_dir = temp.path().join("link");
1068+
1069+
std::fs::create_dir(&real_dir).expect("create real dir");
1070+
1071+
// Create config in real directory
1072+
let config = Config::default();
1073+
let toml_str = toml::to_string_pretty(&config).expect("serialize");
1074+
std::fs::write(real_dir.join(CONFIG_FILE_NAME), toml_str).expect("write config");
1075+
1076+
// Create symlink to real directory
1077+
symlink(&real_dir, &link_dir).expect("create symlink");
1078+
1079+
// Change to symlinked directory and find config
1080+
let original_dir = std::env::current_dir().expect("get cwd");
1081+
std::env::set_current_dir(&link_dir).expect("change to link dir");
1082+
1083+
let result = Config::find_config_file();
1084+
std::env::set_current_dir(original_dir).expect("restore cwd");
1085+
1086+
assert!(result.is_ok());
1087+
let found_path = result.expect("find config");
1088+
1089+
// The path should be resolved to the real location (not through symlink)
1090+
// After canonicalization, the path should not contain "link"
1091+
let path_str = found_path.to_string_lossy();
1092+
assert!(
1093+
!path_str.contains("link"),
1094+
"Path should be canonicalized: {path_str}"
1095+
);
1096+
assert!(
1097+
path_str.contains("real"),
1098+
"Path should resolve to real dir: {path_str}"
1099+
);
1100+
}
1101+
1102+
#[test]
1103+
fn test_find_config_file_walks_up_canonicalized_tree() {
1104+
use tempfile::TempDir;
1105+
1106+
let temp = TempDir::new().expect("create temp dir");
1107+
let nested = temp.path().join("src/lib/utils");
1108+
std::fs::create_dir_all(&nested).expect("create nested dirs");
1109+
1110+
// Create config at root
1111+
let config = Config::default();
1112+
let toml_str = toml::to_string_pretty(&config).expect("serialize");
1113+
std::fs::write(temp.path().join(CONFIG_FILE_NAME), toml_str).expect("write config");
1114+
1115+
// Change to nested directory and find config
1116+
let original_dir = std::env::current_dir().expect("get cwd");
1117+
std::env::set_current_dir(&nested).expect("change to nested dir");
1118+
1119+
let result = Config::find_config_file();
1120+
std::env::set_current_dir(original_dir).expect("restore cwd");
1121+
1122+
assert!(result.is_ok());
1123+
let found_path = result.expect("find config");
1124+
1125+
// Should find the config in the parent directory
1126+
assert!(found_path.is_absolute());
1127+
assert!(found_path.exists());
1128+
assert!(found_path.ends_with(CONFIG_FILE_NAME));
1129+
}
10121130
}

‎src/core/detector.rs‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ mod tests {
341341

342342
#[test]
343343
fn test_mode_parse_error_message() {
344-
let err = "invalid".parse::<Mode>().unwrap_err();
344+
let err = "invalid".parse::<Mode>().expect_err("should fail to parse invalid");
345345
assert!(err.contains("Invalid mode"));
346346
assert!(err.contains("human, agent, or ci"));
347347
}
@@ -495,8 +495,9 @@ mod tests {
495495
// =========================================================================
496496

497497
#[test]
498-
fn test_known_agent_env_vars_not_empty() {
499-
assert!(!KNOWN_AGENT_ENV_VARS.is_empty());
498+
fn test_known_agent_env_vars_has_expected_count() {
499+
// Verify we have a reasonable number of known agent env vars
500+
assert!(KNOWN_AGENT_ENV_VARS.len() >= 10);
500501
}
501502

502503
#[test]
@@ -515,8 +516,9 @@ mod tests {
515516
}
516517

517518
#[test]
518-
fn test_known_ci_env_vars_not_empty() {
519-
assert!(!KNOWN_CI_ENV_VARS.is_empty());
519+
fn test_known_ci_env_vars_has_expected_count() {
520+
// Verify we have a reasonable number of known CI env vars
521+
assert!(KNOWN_CI_ENV_VARS.len() >= 10);
520522
}
521523

522524
#[test]

0 commit comments

Comments
 (0)