History

Skip /dev/null redirects from terminal auto-allow command extraction (#49503)

Redirects to `/dev/null` (e.g. `2>/dev/null`, `&>/dev/null`) are known-safe I/O routing, not commands. Previously, `extract_commands` emitted normalized redirect strings like `"2> /dev/null"` as separate entries in the command list checked against auto-allow regexes. Since `check_commands` requires **all** extracted entries to match an allow pattern, the unmatched redirect caused false-negatives — e.g. `git log --oneline -20 2>/dev/null || echo ...` would not be auto-allowed despite matching `^git` and `^echo` patterns. Rather than removing all redirects from extraction (which would hide dangerous redirects like `> /etc/passwd` from deny/confirm pattern checking), this fix surgically skips only `/dev/null` targets during redirect normalization. Redirects to real files are still emitted and still require a matching pattern for auto-allow, preserving the defense-in-depth property. Closes AI-41 Release Notes: - Fixed terminal auto-allow patterns incorrectly prompting for confirmation on commands containing `/dev/null` redirects (e.g. `2>/dev/null`).

Richard Feldman committed 8 months ago

Commit bf1bb521 parent 27dffc12 files · +98 −1Browse files

Showing the changes to crates/shell_command_parser/src/shell_command_parser.rs (1 of 2 files). Show all

Unified · Split

crates/shell_command_parser/src/shell_command_parser.rs+72−1
@@ -232,6 +232,10 @@ fn normalize_word_piece_into( · 231 unmodified lines
232232 Some(())
233233 }
234234
235+fn is_known_safe_redirect_target(normalized_target: &str) -> bool {
236+ normalized_target == "/dev/null"
237+}
238+
235239 fn normalize_io_redirect(redirect: &ast::IoRedirect) -> Option<RedirectNormalization> {
236240 match redirect {
237241 ast::IoRedirect::File(fd, kind, target) => {
@@ -257,6 +261,9 @@ fn normalize_io_redirect(redirect: &ast::IoRedirect) -> Option<RedirectNormaliza · 19 unmodified lines
257261 None => String::new(),
258262 };
259263 let normalized = normalize_word(target_word)?;
264+ if is_known_safe_redirect_target(&normalized) {
265+ return Some(RedirectNormalization::Skip);
266+ }
260267 Some(RedirectNormalization::Normalized(format!(
261268 "{}{} {}",
262269 fd_prefix, operator, normalized
@@ -265,6 +272,9 @@ fn normalize_io_redirect(redirect: &ast::IoRedirect) -> Option<RedirectNormaliza · 2 unmodified lines
265272 ast::IoRedirect::OutputAndError(word, append) => {
266273 let operator = if *append { "&>>" } else { "&>" };
267274 let normalized = normalize_word(word)?;
275+ if is_known_safe_redirect_target(&normalized) {
276+ return Some(RedirectNormalization::Skip);
277+ }
268278 Some(RedirectNormalization::Normalized(format!(
269279 "{} {}",
270280 operator, normalized
@@ -896,7 +906,7 @@ mod tests { · 625 unmodified lines
896906 #[test]
897907 fn test_pipe_with_stderr_redirect_on_first_command() {
898908 let commands = extract_commands("ls 2>/dev/null | grep foo").expect("parse failed");
899− assert_eq!(commands, vec!["ls", "2> /dev/null", "grep foo"]);
909+ assert_eq!(commands, vec!["ls", "grep foo"]);
900910 }
901911
902912 #[test]
@@ -990,4 +1000,65 @@ mod tests { · 87 unmodified lines
9901000 assert!(commands.contains(&"cat".to_string()));
9911001 assert!(commands.contains(&"tee /tmp/log".to_string()));
9921002 }
1003+
1004+ #[test]
1005+ fn test_redirect_to_dev_null_skipped() {
1006+ let commands = extract_commands("cmd > /dev/null").expect("parse failed");
1007+ assert_eq!(commands, vec!["cmd"]);
1008+ }
1009+
1010+ #[test]
1011+ fn test_stderr_redirect_to_dev_null_skipped() {
1012+ let commands = extract_commands("cmd 2>/dev/null").expect("parse failed");
1013+ assert_eq!(commands, vec!["cmd"]);
1014+ }
1015+
1016+ #[test]
1017+ fn test_stderr_redirect_to_dev_null_with_space_skipped() {
1018+ let commands = extract_commands("cmd 2> /dev/null").expect("parse failed");
1019+ assert_eq!(commands, vec!["cmd"]);
1020+ }
1021+
1022+ #[test]
1023+ fn test_append_redirect_to_dev_null_skipped() {
1024+ let commands = extract_commands("cmd >> /dev/null").expect("parse failed");
1025+ assert_eq!(commands, vec!["cmd"]);
1026+ }
1027+
1028+ #[test]
1029+ fn test_output_and_error_redirect_to_dev_null_skipped() {
1030+ let commands = extract_commands("cmd &>/dev/null").expect("parse failed");
1031+ assert_eq!(commands, vec!["cmd"]);
1032+ }
1033+
1034+ #[test]
1035+ fn test_append_output_and_error_redirect_to_dev_null_skipped() {
1036+ let commands = extract_commands("cmd &>>/dev/null").expect("parse failed");
1037+ assert_eq!(commands, vec!["cmd"]);
1038+ }
1039+
1040+ #[test]
1041+ fn test_quoted_dev_null_redirect_skipped() {
1042+ let commands = extract_commands("cmd 2>'/dev/null'").expect("parse failed");
1043+ assert_eq!(commands, vec!["cmd"]);
1044+ }
1045+
1046+ #[test]
1047+ fn test_redirect_to_real_file_still_included() {
1048+ let commands = extract_commands("echo hello > /etc/passwd").expect("parse failed");
1049+ assert_eq!(commands, vec!["echo hello", "> /etc/passwd"]);
1050+ }
1051+
1052+ #[test]
1053+ fn test_dev_null_redirect_in_chained_command() {
1054+ let commands =
1055+ extract_commands("git log 2>/dev/null || echo fallback").expect("parse failed");
1056+ assert_eq!(commands, vec!["git log", "echo fallback"]);
1057+ }
1058+
1059+ #[test]
1060+ fn test_mixed_safe_and_unsafe_redirects() {
1061+ let commands = extract_commands("cmd > /tmp/out 2>/dev/null").expect("parse failed");
1062+ assert_eq!(commands, vec!["cmd", "> /tmp/out"]);
1063+ }
9931064 }