fix(security): expand git argument sanitization (#523)

* fix(security): expand git argument sanitization

Expand sanitize_git_args() blocklist to also reject --pager=, --editor=,
-c (config injection), --no-verify, and > in arguments. Apply validation
to git_add() paths and git_diff() files argument (previously only called
from git_checkout()). The -c check uses exact match to avoid
false-positives on --cached.

Closes #516

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* style: apply rustfmt to providers/mod.rs

Fix pre-existing formatting issue from main.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
fettpl 2026-02-17 13:51:08 +01:00 committed by GitHub
parent d2ed5113e9
commit 87dcd7a7a0
No known key found for this signature in database
GPG key ID: B5690EEEBB952194

View file

@ -28,13 +28,22 @@ impl GitOperationsTool {
if arg_lower.starts_with("--exec=") if arg_lower.starts_with("--exec=")
|| arg_lower.starts_with("--upload-pack=") || arg_lower.starts_with("--upload-pack=")
|| arg_lower.starts_with("--receive-pack=") || arg_lower.starts_with("--receive-pack=")
|| arg_lower.starts_with("--pager=")
|| arg_lower.starts_with("--editor=")
|| arg_lower == "--no-verify"
|| arg_lower.contains("$(") || arg_lower.contains("$(")
|| arg_lower.contains('`') || arg_lower.contains('`')
|| arg.contains('|') || arg.contains('|')
|| arg.contains(';') || arg.contains(';')
|| arg.contains('>')
{ {
anyhow::bail!("Blocked potentially dangerous git argument: {arg}"); anyhow::bail!("Blocked potentially dangerous git argument: {arg}");
} }
// Block `-c` config injection (exact match or `-c=...` prefix).
// This must not false-positive on `--cached` or `-cached`.
if arg_lower == "-c" || arg_lower.starts_with("-c=") {
anyhow::bail!("Blocked potentially dangerous git argument: {arg}");
}
result.push(arg.to_string()); result.push(arg.to_string());
} }
Ok(result) Ok(result)
@ -129,6 +138,9 @@ impl GitOperationsTool {
.and_then(|v| v.as_bool()) .and_then(|v| v.as_bool())
.unwrap_or(false); .unwrap_or(false);
// Validate files argument against injection patterns
self.sanitize_git_args(files)?;
let mut git_args = vec!["diff", "--unified=3"]; let mut git_args = vec!["diff", "--unified=3"];
if cached { if cached {
git_args.push("--cached"); git_args.push("--cached");
@ -314,6 +326,9 @@ impl GitOperationsTool {
.and_then(|v| v.as_str()) .and_then(|v| v.as_str())
.ok_or_else(|| anyhow::anyhow!("Missing 'paths' parameter"))?; .ok_or_else(|| anyhow::anyhow!("Missing 'paths' parameter"))?;
// Validate paths against injection patterns
self.sanitize_git_args(paths)?;
let output = self.run_git_command(&["add", "--", paths]).await; let output = self.run_git_command(&["add", "--", paths]).await;
match output { match output {
@ -574,6 +589,52 @@ mod tests {
assert!(tool.sanitize_git_args("arg; rm file").is_err()); assert!(tool.sanitize_git_args("arg; rm file").is_err());
} }
#[test]
fn sanitize_git_blocks_pager_editor_injection() {
let tmp = TempDir::new().unwrap();
let tool = test_tool(tmp.path());
assert!(tool.sanitize_git_args("--pager=less").is_err());
assert!(tool.sanitize_git_args("--editor=vim").is_err());
}
#[test]
fn sanitize_git_blocks_config_injection() {
let tmp = TempDir::new().unwrap();
let tool = test_tool(tmp.path());
// Exact `-c` flag (config injection)
assert!(tool.sanitize_git_args("-c core.sshCommand=evil").is_err());
assert!(tool.sanitize_git_args("-c=core.pager=less").is_err());
}
#[test]
fn sanitize_git_blocks_no_verify() {
let tmp = TempDir::new().unwrap();
let tool = test_tool(tmp.path());
assert!(tool.sanitize_git_args("--no-verify").is_err());
}
#[test]
fn sanitize_git_blocks_redirect_in_args() {
let tmp = TempDir::new().unwrap();
let tool = test_tool(tmp.path());
assert!(tool.sanitize_git_args("file.txt > /tmp/out").is_err());
}
#[test]
fn sanitize_git_cached_not_blocked() {
let tmp = TempDir::new().unwrap();
let tool = test_tool(tmp.path());
// --cached must NOT be blocked by the `-c` check
assert!(tool.sanitize_git_args("--cached").is_ok());
// Other safe flags starting with -c prefix
assert!(tool.sanitize_git_args("-cached").is_ok());
}
#[test] #[test]
fn sanitize_git_allows_safe() { fn sanitize_git_allows_safe() {
let tmp = TempDir::new().unwrap(); let tmp = TempDir::new().unwrap();
@ -583,6 +644,8 @@ mod tests {
assert!(tool.sanitize_git_args("main").is_ok()); assert!(tool.sanitize_git_args("main").is_ok());
assert!(tool.sanitize_git_args("feature/test-branch").is_ok()); assert!(tool.sanitize_git_args("feature/test-branch").is_ok());
assert!(tool.sanitize_git_args("--cached").is_ok()); assert!(tool.sanitize_git_args("--cached").is_ok());
assert!(tool.sanitize_git_args("src/main.rs").is_ok());
assert!(tool.sanitize_git_args(".").is_ok());
} }
#[test] #[test]