Fix shell quote bypass in terminal permission system (#48436)

Richard Feldman created

## Problem

Zed's terminal permission system validates commands using regex patterns
against string representations of parsed commands. The
`SimpleCommand::to_string()` method in brush-parser preserves the raw
lexical form including shell quotes, causing pattern matching to fail on
obfuscated commands.

For example, all of these are semantically equivalent to `rm -rf /` in
POSIX shells, but produced different strings from `to_string()` that
bypassed the hardcoded deny pattern `rm\s+(-[rf]+\s+)*/\s*$`:

| Input | `extract_commands()` returned | Matched deny pattern? |
|---|---|---|
| `rm -rf /` | `"rm -rf /"` | ✅ Yes |
| `rm -rf '/'` | `"rm -rf '/'"` | ❌ No |
| `'rm' -rf /` | `"'rm' -rf /"` | ❌ No |
| `r'm' -rf /` | `"r'm' -rf /"` | ❌ No |
| `rm -r'f' /` | `"rm -r'f' /"` | ❌ No |
| `rm -rf \/` | `"rm -rf \\/"` | ❌ No |

Both hardcoded deny patterns (Phase 2, which operates on extracted
sub-commands) and user-configured `always_deny` patterns were affected.

## Fix

Replace the `simple_command.to_string()` call in
`extract_commands_from_simple_command` with manual construction that:

1. Iterates the command name (`word_or_name`) and word arguments from
the suffix
2. Normalizes each word by parsing it into `WordPiece`s via
`brush_parser::word::parse`
3. Reconstructs the semantic (unquoted) value from each piece:
   - `Text` — used as-is
- `SingleQuotedText` — used as-is (brush-parser already strips the
wrapping quotes)
- `EscapeSequence` — leading `\` stripped to produce the unescaped
character
- `AnsiCQuotedText` — used as-is (brush-parser already evaluates
`$'...'`)
- `DoubleQuotedSequence` / `GettextDoubleQuotedSequence` — inner pieces
recursively normalized
   - `TildePrefix` — preserved as `~` + suffix
- `ParameterExpansion` / `CommandSubstitution` /
`BackquotedCommandSubstitution` / `ArithmeticExpression` — original
source text preserved so patterns like `\$HOME` and `\${HOME}` continue
to match
4. Joins normalized words with spaces

Falls back to the raw `word.value` if word parsing fails.

## Cross-shell safety

The normalization operates on POSIX-parsed ASTs (brush-parser uses POSIX
grammar), so it applies POSIX quoting rules. For non-POSIX shells
(Nushell, PowerShell, Xonsh, Elvish, etc.) where quoting semantics
differ, any mismatch results in false positives (overly cautious
denials), never false negatives. This is the correct error direction for
a security feature. When brush-parser fails to parse a non-POSIX
command, `extract_commands` returns `None` and the existing fallback
(raw-string matching with `allow_enabled=false`) handles it.

(No release notes because granular tool permissions are still
feautre-flagged.)

Release Notes:

- N/A

Change summary

crates/agent/src/shell_parser.rs | 290 +++++++++++++++++++++++++++------
1 file changed, 238 insertions(+), 52 deletions(-)

Detailed changes

crates/agent/src/shell_parser.rs 🔗

@@ -12,100 +12,201 @@ pub fn extract_commands(command: &str) -> Option<Vec<String>> {
     let program = parser.parse_program().ok()?;
 
     let mut commands = Vec::new();
-    extract_commands_from_program(&program, &mut commands);
+    extract_commands_from_program(&program, &mut commands)?;
 
     Some(commands)
 }
 
-fn extract_commands_from_program(program: &ast::Program, commands: &mut Vec<String>) {
+fn extract_commands_from_program(program: &ast::Program, commands: &mut Vec<String>) -> Option<()> {
     for complete_command in &program.complete_commands {
-        extract_commands_from_compound_list(complete_command, commands);
+        extract_commands_from_compound_list(complete_command, commands)?;
     }
+    Some(())
 }
 
 fn extract_commands_from_compound_list(
     compound_list: &ast::CompoundList,
     commands: &mut Vec<String>,
-) {
+) -> Option<()> {
     for item in &compound_list.0 {
-        extract_commands_from_and_or_list(&item.0, commands);
+        extract_commands_from_and_or_list(&item.0, commands)?;
     }
+    Some(())
 }
 
-fn extract_commands_from_and_or_list(and_or_list: &ast::AndOrList, commands: &mut Vec<String>) {
-    extract_commands_from_pipeline(&and_or_list.first, commands);
+fn extract_commands_from_and_or_list(
+    and_or_list: &ast::AndOrList,
+    commands: &mut Vec<String>,
+) -> Option<()> {
+    extract_commands_from_pipeline(&and_or_list.first, commands)?;
 
     for and_or in &and_or_list.additional {
         match and_or {
             ast::AndOr::And(pipeline) | ast::AndOr::Or(pipeline) => {
-                extract_commands_from_pipeline(pipeline, commands);
+                extract_commands_from_pipeline(pipeline, commands)?;
             }
         }
     }
+    Some(())
 }
 
-fn extract_commands_from_pipeline(pipeline: &ast::Pipeline, commands: &mut Vec<String>) {
+fn extract_commands_from_pipeline(
+    pipeline: &ast::Pipeline,
+    commands: &mut Vec<String>,
+) -> Option<()> {
     for command in &pipeline.seq {
-        extract_commands_from_command(command, commands);
+        extract_commands_from_command(command, commands)?;
     }
+    Some(())
 }
 
-fn extract_commands_from_command(command: &ast::Command, commands: &mut Vec<String>) {
+fn extract_commands_from_command(command: &ast::Command, commands: &mut Vec<String>) -> Option<()> {
     match command {
         ast::Command::Simple(simple_command) => {
-            extract_commands_from_simple_command(simple_command, commands);
+            extract_commands_from_simple_command(simple_command, commands)?;
         }
         ast::Command::Compound(compound_command, _redirect_list) => {
-            extract_commands_from_compound_command(compound_command, commands);
+            extract_commands_from_compound_command(compound_command, commands)?;
         }
         ast::Command::Function(func_def) => {
-            extract_commands_from_function_body(&func_def.body, commands);
+            extract_commands_from_function_body(&func_def.body, commands)?;
         }
         ast::Command::ExtendedTest(test_expr) => {
-            extract_commands_from_extended_test_expr(test_expr, commands);
+            extract_commands_from_extended_test_expr(test_expr, commands)?;
         }
     }
+    Some(())
 }
 
 fn extract_commands_from_simple_command(
     simple_command: &ast::SimpleCommand,
     commands: &mut Vec<String>,
-) {
-    let command_str = simple_command.to_string();
-    if !command_str.trim().is_empty() {
+) -> Option<()> {
+    // Build a normalized command string from individual words, stripping shell
+    // quotes so that security patterns match regardless of quoting style.
+    // For example, both `rm -rf '/'` and `rm -rf /` normalize to "rm -rf /".
+    //
+    // If any word fails to normalize, we return None so that `extract_commands`
+    // returns None — the same as a shell parse failure. The caller then falls
+    // back to raw-input matching with always_allow disabled.
+    let mut words = Vec::new();
+    if let Some(word) = &simple_command.word_or_name {
+        words.push(normalize_word(word)?);
+    }
+    if let Some(suffix) = &simple_command.suffix {
+        for item in &suffix.0 {
+            if let ast::CommandPrefixOrSuffixItem::Word(word) = item {
+                words.push(normalize_word(word)?);
+            }
+        }
+    }
+    let command_str = words.join(" ");
+    if !command_str.is_empty() {
         commands.push(command_str);
     }
 
+    // Extract nested commands from command substitutions, process substitutions, etc.
     if let Some(prefix) = &simple_command.prefix {
-        extract_commands_from_command_prefix(prefix, commands);
+        extract_commands_from_command_prefix(prefix, commands)?;
     }
     if let Some(word) = &simple_command.word_or_name {
         extract_commands_from_word(word, commands);
     }
     if let Some(suffix) = &simple_command.suffix {
-        extract_commands_from_command_suffix(suffix, commands);
+        extract_commands_from_command_suffix(suffix, commands)?;
+    }
+    Some(())
+}
+
+/// Normalizes a shell word by stripping quoting syntax and returning the
+/// semantic (unquoted) value. Returns `None` if word parsing fails.
+fn normalize_word(word: &ast::Word) -> Option<String> {
+    let options = ParserOptions::default();
+    let pieces = brush_parser::word::parse(&word.value, &options).ok()?;
+    let mut result = String::new();
+    for piece_with_source in &pieces {
+        normalize_word_piece_into(
+            &piece_with_source.piece,
+            &word.value,
+            piece_with_source.start_index,
+            piece_with_source.end_index,
+            &mut result,
+        );
+    }
+    Some(result)
+}
+
+fn normalize_word_piece_into(
+    piece: &WordPiece,
+    raw_value: &str,
+    start_index: usize,
+    end_index: usize,
+    result: &mut String,
+) {
+    match piece {
+        WordPiece::Text(text) => result.push_str(text),
+        WordPiece::SingleQuotedText(text) => result.push_str(text),
+        WordPiece::AnsiCQuotedText(text) => result.push_str(text),
+        WordPiece::EscapeSequence(text) => {
+            result.push_str(text.strip_prefix('\\').unwrap_or(text));
+        }
+        WordPiece::DoubleQuotedSequence(pieces)
+        | WordPiece::GettextDoubleQuotedSequence(pieces) => {
+            for inner in pieces {
+                normalize_word_piece_into(
+                    &inner.piece,
+                    raw_value,
+                    inner.start_index,
+                    inner.end_index,
+                    result,
+                );
+            }
+        }
+        WordPiece::TildePrefix(prefix) => {
+            result.push('~');
+            result.push_str(prefix);
+        }
+        // For parameter expansions, command substitutions, and arithmetic expressions,
+        // preserve the original source text so that patterns like `\$HOME` continue
+        // to match.
+        WordPiece::ParameterExpansion(_)
+        | WordPiece::CommandSubstitution(_)
+        | WordPiece::BackquotedCommandSubstitution(_)
+        | WordPiece::ArithmeticExpression(_) => {
+            if let Some(source) = raw_value.get(start_index..end_index) {
+                result.push_str(source);
+            }
+        }
     }
 }
 
-fn extract_commands_from_command_prefix(prefix: &ast::CommandPrefix, commands: &mut Vec<String>) {
+fn extract_commands_from_command_prefix(
+    prefix: &ast::CommandPrefix,
+    commands: &mut Vec<String>,
+) -> Option<()> {
     for item in &prefix.0 {
-        extract_commands_from_prefix_or_suffix_item(item, commands);
+        extract_commands_from_prefix_or_suffix_item(item, commands)?;
     }
+    Some(())
 }
 
-fn extract_commands_from_command_suffix(suffix: &ast::CommandSuffix, commands: &mut Vec<String>) {
+fn extract_commands_from_command_suffix(
+    suffix: &ast::CommandSuffix,
+    commands: &mut Vec<String>,
+) -> Option<()> {
     for item in &suffix.0 {
-        extract_commands_from_prefix_or_suffix_item(item, commands);
+        extract_commands_from_prefix_or_suffix_item(item, commands)?;
     }
+    Some(())
 }
 
 fn extract_commands_from_prefix_or_suffix_item(
     item: &ast::CommandPrefixOrSuffixItem,
     commands: &mut Vec<String>,
-) {
+) -> Option<()> {
     match item {
         ast::CommandPrefixOrSuffixItem::IoRedirect(redirect) => {
-            extract_commands_from_io_redirect(redirect, commands);
+            extract_commands_from_io_redirect(redirect, commands)?;
         }
         ast::CommandPrefixOrSuffixItem::AssignmentWord(assignment, _word) => {
             extract_commands_from_assignment(assignment, commands);
@@ -114,16 +215,20 @@ fn extract_commands_from_prefix_or_suffix_item(
             extract_commands_from_word(word, commands);
         }
         ast::CommandPrefixOrSuffixItem::ProcessSubstitution(_kind, subshell) => {
-            extract_commands_from_compound_list(&subshell.list, commands);
+            extract_commands_from_compound_list(&subshell.list, commands)?;
         }
     }
+    Some(())
 }
 
-fn extract_commands_from_io_redirect(redirect: &ast::IoRedirect, commands: &mut Vec<String>) {
+fn extract_commands_from_io_redirect(
+    redirect: &ast::IoRedirect,
+    commands: &mut Vec<String>,
+) -> Option<()> {
     match redirect {
         ast::IoRedirect::File(_fd, _kind, target) => {
             if let ast::IoFileRedirectTarget::ProcessSubstitution(_kind, subshell) = target {
-                extract_commands_from_compound_list(&subshell.list, commands);
+                extract_commands_from_compound_list(&subshell.list, commands)?;
             }
         }
         ast::IoRedirect::HereDocument(_fd, _here_doc) => {}
@@ -134,6 +239,7 @@ fn extract_commands_from_io_redirect(redirect: &ast::IoRedirect, commands: &mut
             extract_commands_from_word(word, commands);
         }
     }
+    Some(())
 }
 
 fn extract_commands_from_assignment(assignment: &ast::Assignment, commands: &mut Vec<String>) {
@@ -188,13 +294,13 @@ fn extract_commands_from_word_piece(piece: &WordPiece, commands: &mut Vec<String
 fn extract_commands_from_compound_command(
     compound_command: &ast::CompoundCommand,
     commands: &mut Vec<String>,
-) {
+) -> Option<()> {
     match compound_command {
         ast::CompoundCommand::BraceGroup(brace_group) => {
-            extract_commands_from_compound_list(&brace_group.list, commands);
+            extract_commands_from_compound_list(&brace_group.list, commands)?;
         }
         ast::CompoundCommand::Subshell(subshell) => {
-            extract_commands_from_compound_list(&subshell.list, commands);
+            extract_commands_from_compound_list(&subshell.list, commands)?;
         }
         ast::CompoundCommand::ForClause(for_clause) => {
             if let Some(words) = &for_clause.values {
@@ -202,69 +308,76 @@ fn extract_commands_from_compound_command(
                     extract_commands_from_word(word, commands);
                 }
             }
-            extract_commands_from_do_group(&for_clause.body, commands);
+            extract_commands_from_do_group(&for_clause.body, commands)?;
         }
         ast::CompoundCommand::CaseClause(case_clause) => {
             extract_commands_from_word(&case_clause.value, commands);
             for item in &case_clause.cases {
                 if let Some(body) = &item.cmd {
-                    extract_commands_from_compound_list(body, commands);
+                    extract_commands_from_compound_list(body, commands)?;
                 }
             }
         }
         ast::CompoundCommand::IfClause(if_clause) => {
-            extract_commands_from_compound_list(&if_clause.condition, commands);
-            extract_commands_from_compound_list(&if_clause.then, commands);
+            extract_commands_from_compound_list(&if_clause.condition, commands)?;
+            extract_commands_from_compound_list(&if_clause.then, commands)?;
             if let Some(elses) = &if_clause.elses {
                 for else_item in elses {
                     if let Some(condition) = &else_item.condition {
-                        extract_commands_from_compound_list(condition, commands);
+                        extract_commands_from_compound_list(condition, commands)?;
                     }
-                    extract_commands_from_compound_list(&else_item.body, commands);
+                    extract_commands_from_compound_list(&else_item.body, commands)?;
                 }
             }
         }
         ast::CompoundCommand::WhileClause(while_clause)
         | ast::CompoundCommand::UntilClause(while_clause) => {
-            extract_commands_from_compound_list(&while_clause.0, commands);
-            extract_commands_from_do_group(&while_clause.1, commands);
+            extract_commands_from_compound_list(&while_clause.0, commands)?;
+            extract_commands_from_do_group(&while_clause.1, commands)?;
         }
         ast::CompoundCommand::ArithmeticForClause(arith_for) => {
-            extract_commands_from_do_group(&arith_for.body, commands);
+            extract_commands_from_do_group(&arith_for.body, commands)?;
         }
         ast::CompoundCommand::Arithmetic(_arith_cmd) => {}
     }
+    Some(())
 }
 
-fn extract_commands_from_do_group(do_group: &ast::DoGroupCommand, commands: &mut Vec<String>) {
-    extract_commands_from_compound_list(&do_group.list, commands);
+fn extract_commands_from_do_group(
+    do_group: &ast::DoGroupCommand,
+    commands: &mut Vec<String>,
+) -> Option<()> {
+    extract_commands_from_compound_list(&do_group.list, commands)
 }
 
-fn extract_commands_from_function_body(func_body: &ast::FunctionBody, commands: &mut Vec<String>) {
-    extract_commands_from_compound_command(&func_body.0, commands);
+fn extract_commands_from_function_body(
+    func_body: &ast::FunctionBody,
+    commands: &mut Vec<String>,
+) -> Option<()> {
+    extract_commands_from_compound_command(&func_body.0, commands)
 }
 
 fn extract_commands_from_extended_test_expr(
     test_expr: &ast::ExtendedTestExprCommand,
     commands: &mut Vec<String>,
-) {
-    extract_commands_from_extended_test_expr_inner(&test_expr.expr, commands);
+) -> Option<()> {
+    extract_commands_from_extended_test_expr_inner(&test_expr.expr, commands)
 }
 
 fn extract_commands_from_extended_test_expr_inner(
     expr: &ast::ExtendedTestExpr,
     commands: &mut Vec<String>,
-) {
+) -> Option<()> {
     match expr {
         ast::ExtendedTestExpr::Not(inner) => {
-            extract_commands_from_extended_test_expr_inner(inner, commands);
+            extract_commands_from_extended_test_expr_inner(inner, commands)?;
         }
         ast::ExtendedTestExpr::And(left, right) | ast::ExtendedTestExpr::Or(left, right) => {
-            extract_commands_from_extended_test_expr_inner(left, commands);
-            extract_commands_from_extended_test_expr_inner(right, commands);
+            extract_commands_from_extended_test_expr_inner(left, commands)?;
+            extract_commands_from_extended_test_expr_inner(right, commands)?;
         }
         ast::ExtendedTestExpr::Parenthesized(inner) => {
-            extract_commands_from_extended_test_expr_inner(inner, commands);
+            extract_commands_from_extended_test_expr_inner(inner, commands)?;
         }
         ast::ExtendedTestExpr::UnaryTest(_, word) => {
             extract_commands_from_word(word, commands);
@@ -274,6 +387,7 @@ fn extract_commands_from_extended_test_expr_inner(
             extract_commands_from_word(word2, commands);
         }
     }
+    Some(())
 }
 
 #[cfg(test)]
@@ -292,6 +406,78 @@ mod tests {
         assert_eq!(commands, vec!["ls -la /tmp"]);
     }
 
+    #[test]
+    fn test_single_quoted_argument_is_normalized() {
+        let commands = extract_commands("rm -rf '/'").expect("parse failed");
+        assert_eq!(commands, vec!["rm -rf /"]);
+    }
+
+    #[test]
+    fn test_single_quoted_command_name_is_normalized() {
+        let commands = extract_commands("'rm' -rf /").expect("parse failed");
+        assert_eq!(commands, vec!["rm -rf /"]);
+    }
+
+    #[test]
+    fn test_double_quoted_argument_is_normalized() {
+        let commands = extract_commands("rm -rf \"/\"").expect("parse failed");
+        assert_eq!(commands, vec!["rm -rf /"]);
+    }
+
+    #[test]
+    fn test_double_quoted_command_name_is_normalized() {
+        let commands = extract_commands("\"rm\" -rf /").expect("parse failed");
+        assert_eq!(commands, vec!["rm -rf /"]);
+    }
+
+    #[test]
+    fn test_escaped_argument_is_normalized() {
+        let commands = extract_commands("rm -rf \\/").expect("parse failed");
+        assert_eq!(commands, vec!["rm -rf /"]);
+    }
+
+    #[test]
+    fn test_partial_quoting_command_name_is_normalized() {
+        let commands = extract_commands("r'm' -rf /").expect("parse failed");
+        assert_eq!(commands, vec!["rm -rf /"]);
+    }
+
+    #[test]
+    fn test_partial_quoting_flag_is_normalized() {
+        let commands = extract_commands("rm -r'f' /").expect("parse failed");
+        assert_eq!(commands, vec!["rm -rf /"]);
+    }
+
+    #[test]
+    fn test_quoted_bypass_in_chained_command() {
+        let commands = extract_commands("ls && 'rm' -rf '/'").expect("parse failed");
+        assert_eq!(commands, vec!["ls", "rm -rf /"]);
+    }
+
+    #[test]
+    fn test_tilde_preserved_after_normalization() {
+        let commands = extract_commands("rm -rf ~").expect("parse failed");
+        assert_eq!(commands, vec!["rm -rf ~"]);
+    }
+
+    #[test]
+    fn test_quoted_tilde_normalized() {
+        let commands = extract_commands("rm -rf '~'").expect("parse failed");
+        assert_eq!(commands, vec!["rm -rf ~"]);
+    }
+
+    #[test]
+    fn test_parameter_expansion_preserved() {
+        let commands = extract_commands("rm -rf $HOME").expect("parse failed");
+        assert_eq!(commands, vec!["rm -rf $HOME"]);
+    }
+
+    #[test]
+    fn test_braced_parameter_expansion_preserved() {
+        let commands = extract_commands("rm -rf ${HOME}").expect("parse failed");
+        assert_eq!(commands, vec!["rm -rf ${HOME}"]);
+    }
+
     #[test]
     fn test_and_operator() {
         let commands = extract_commands("ls && rm -rf /").expect("parse failed");