diff --git a/crates/agent/src/tool_permissions.rs b/crates/agent/src/tool_permissions.rs index 7af196b295e8aa9fb3293c513818203c67aa2054..79564bbddea7063d00e18d97c8eab89533b20da5 100644 --- a/crates/agent/src/tool_permissions.rs +++ b/crates/agent/src/tool_permissions.rs @@ -1150,6 +1150,32 @@ mod tests { t("ls && echo hello").allow(&["^ls", "^echo"]).is_allow(); } + #[test] + fn dev_null_redirect_does_not_cause_false_negative() { + // Redirects to /dev/null are known-safe and should be skipped during + // command extraction, so they don't prevent auto-allow from matching. + t(r#"git log --oneline -20 2>/dev/null || echo "not a git repo or no commits""#) + .allow(&[r"^git\s+(status|diff|log|show)\b", "^echo"]) + .is_allow(); + } + + #[test] + fn redirect_to_real_file_still_causes_confirm() { + // Redirects to real files (not /dev/null) should still be included in + // the extracted commands, so they prevent auto-allow when unmatched. + t("echo hello > /etc/passwd").allow(&["^echo"]).is_confirm(); + } + + #[test] + fn pipe_does_not_cause_false_negative_when_all_commands_match() { + // A piped command like `echo "y\ny" | git add -p file` produces two commands: + // "echo y\ny" and "git add -p file". Both should match their respective allow + // patterns, so the overall command should be auto-allowed. + t(r#"echo "y\ny" | git add -p crates/acp_thread/src/acp_thread.rs"#) + .allow(&[r"^git\s+(--no-pager\s+)?(fetch|status|diff|log|show|add|commit|push|checkout\s+-b)\b", "^echo"]) + .is_allow(); + } + #[test] fn deny_triggers_on_any_matching_command() { t("ls && rm file").allow(&["^ls"]).deny(&["^rm"]).is_deny(); diff --git a/crates/shell_command_parser/src/shell_command_parser.rs b/crates/shell_command_parser/src/shell_command_parser.rs index e120799355b905ebf3cddc962132d0f7941faa1b..acfd656787c301d9f7ad61e6a14a052b3bc2924c 100644 --- a/crates/shell_command_parser/src/shell_command_parser.rs +++ b/crates/shell_command_parser/src/shell_command_parser.rs @@ -232,6 +232,10 @@ fn normalize_word_piece_into( Some(()) } +fn is_known_safe_redirect_target(normalized_target: &str) -> bool { + normalized_target == "/dev/null" +} + fn normalize_io_redirect(redirect: &ast::IoRedirect) -> Option { match redirect { ast::IoRedirect::File(fd, kind, target) => { @@ -257,6 +261,9 @@ fn normalize_io_redirect(redirect: &ast::IoRedirect) -> Option String::new(), }; let normalized = normalize_word(target_word)?; + if is_known_safe_redirect_target(&normalized) { + return Some(RedirectNormalization::Skip); + } Some(RedirectNormalization::Normalized(format!( "{}{} {}", fd_prefix, operator, normalized @@ -265,6 +272,9 @@ fn normalize_io_redirect(redirect: &ast::IoRedirect) -> Option { let operator = if *append { "&>>" } else { "&>" }; let normalized = normalize_word(word)?; + if is_known_safe_redirect_target(&normalized) { + return Some(RedirectNormalization::Skip); + } Some(RedirectNormalization::Normalized(format!( "{} {}", operator, normalized @@ -896,7 +906,7 @@ mod tests { #[test] fn test_pipe_with_stderr_redirect_on_first_command() { let commands = extract_commands("ls 2>/dev/null | grep foo").expect("parse failed"); - assert_eq!(commands, vec!["ls", "2> /dev/null", "grep foo"]); + assert_eq!(commands, vec!["ls", "grep foo"]); } #[test] @@ -990,4 +1000,65 @@ mod tests { assert!(commands.contains(&"cat".to_string())); assert!(commands.contains(&"tee /tmp/log".to_string())); } + + #[test] + fn test_redirect_to_dev_null_skipped() { + let commands = extract_commands("cmd > /dev/null").expect("parse failed"); + assert_eq!(commands, vec!["cmd"]); + } + + #[test] + fn test_stderr_redirect_to_dev_null_skipped() { + let commands = extract_commands("cmd 2>/dev/null").expect("parse failed"); + assert_eq!(commands, vec!["cmd"]); + } + + #[test] + fn test_stderr_redirect_to_dev_null_with_space_skipped() { + let commands = extract_commands("cmd 2> /dev/null").expect("parse failed"); + assert_eq!(commands, vec!["cmd"]); + } + + #[test] + fn test_append_redirect_to_dev_null_skipped() { + let commands = extract_commands("cmd >> /dev/null").expect("parse failed"); + assert_eq!(commands, vec!["cmd"]); + } + + #[test] + fn test_output_and_error_redirect_to_dev_null_skipped() { + let commands = extract_commands("cmd &>/dev/null").expect("parse failed"); + assert_eq!(commands, vec!["cmd"]); + } + + #[test] + fn test_append_output_and_error_redirect_to_dev_null_skipped() { + let commands = extract_commands("cmd &>>/dev/null").expect("parse failed"); + assert_eq!(commands, vec!["cmd"]); + } + + #[test] + fn test_quoted_dev_null_redirect_skipped() { + let commands = extract_commands("cmd 2>'/dev/null'").expect("parse failed"); + assert_eq!(commands, vec!["cmd"]); + } + + #[test] + fn test_redirect_to_real_file_still_included() { + let commands = extract_commands("echo hello > /etc/passwd").expect("parse failed"); + assert_eq!(commands, vec!["echo hello", "> /etc/passwd"]); + } + + #[test] + fn test_dev_null_redirect_in_chained_command() { + let commands = + extract_commands("git log 2>/dev/null || echo fallback").expect("parse failed"); + assert_eq!(commands, vec!["git log", "echo fallback"]); + } + + #[test] + fn test_mixed_safe_and_unsafe_redirects() { + let commands = extract_commands("cmd > /tmp/out 2>/dev/null").expect("parse failed"); + assert_eq!(commands, vec!["cmd", "> /tmp/out"]); + } }