fix(tool): reject mutating find commands as read-only#2004
Conversation
There was a problem hiding this comment.
@VectorPeak, thanks for the fix. The mutating predicates list and test coverage look correct.
However, _is_mutating_find_command uses shlex.split + manual token iteration, while the rest of this file uses tree-sitter AST for command analysis (extract_file_paths, check_injection_risk, split_compound_command, etc.). Using shlex here introduces a second parsing paradigm and requires maintaining FIND_PREDICATES_WITH_ARGUMENTS just to skip arguments correctly — which is fragile (e.g., -fprintf takes 2 args but the code skips 1).
Suggest using the existing tree-sitter parser instead. This eliminates the argument-counting list entirely and aligns with the file's established pattern:
def _is_mutating_find_command(self, cmd: str) -> bool:
"""Check if a find command contains mutating predicates via AST."""
try:
tree = self.parser.parse(bytes(cmd, "utf8"))
except Exception:
return False
root = tree.root_node
cmd_node = self._find_first_simple_command(root)
if cmd_node is None:
return False
name_node = cmd_node.child_by_field_name("name")
if name_node is None or name_node.text.decode("utf8") != "find":
return False
for child in cmd_node.children:
if child.type == "word":
text = child.text.decode("utf8")
if text in FIND_MUTATING_PREDICATES:
return True
return FalseThis way only FIND_MUTATING_PREDICATES is needed; FIND_PREDICATES_WITH_ARGUMENTS can be removed.
|
@DavdGao Updated as suggested: _is_mutating_find_command now uses the existing tree-sitter AST parser, and FIND_PREDICATES_WITH_ARGUMENTS has been removed. |
2 participants
AgentScope Version
Current branch based on
mainat922e673b. No released version bump is involved.Description
Fixes #2003 by preventing mutating GNU
findactions from being classified as read-only beforeBash.check_permissions()reaches the normal permission path.What Problem This Solves
Bash.check_permissions()has an early allow path for commands thatBashCommandParser.is_read_only_command()marks as safe. This is the right behavior for inspection-only commands such asls,cat,grep, and ordinaryfindsearches, because those commands should not require extra confirmation just to read or list files.The bug was that
findwas treated as read-only at the command-prefix level. In_is_single_command_read_only(), the generic check accepted any command wherecmd == readonly_cmdorcmd.startswith(readonly_cmd + " "). Sincefindwas present inREAD_ONLY_COMMANDS, the parser could classify a command as read-only immediately after seeing the leadingfindtext, without checking the rest of thefindexpression.That distinction matters because
findis not only a search command. Afindexpression can contain actions: some delete matched files, some execute arbitrary commands for each match, and some write matching paths to a named output file. These examples all start with the same read-only-looking prefix, but they cross different permission boundaries:Before this fix, those commands could be classified through the same prefix path as a harmless search like:
The intended boundary is more precise:
findshould be auto-allowable only when the expression is still read-only. Once the expression contains mutating actions such as-delete,-exec,-execdir,-ok,-okdir,-fls,-fprint,-fprint0, or-fprintf, it should fall through to the normal permission decision instead of being treated like a file listing.The fix also avoids the opposite mistake: pattern values that merely look like action names should remain read-only. For example, searching for a filename literally called
-deleteshould not be treated as deletion:Changes
This PR adds a small
find-specific guard before the generic read-only prefix match runs. The new logic tokenizes a single command, confirms the executable isfind, and checks only expression positions for mutating actions.The mutating actions covered by this PR are:
The parser also skips arguments for common one-argument
findpredicates so that pattern values which merely look like actions are not misclassified. These remain read-only:The change only narrows internal Bash permission classification. It does not change Bash command execution, dangerous path lists, dedicated file tools, command parsing outside this read-only check, user-facing APIs, or documentation examples. Documentation was not updated for that reason.
Evidence
Direct parser verification covered both sides of the boundary:
Direct
Bash.check_permissions()verification confirmed the same runtime permission behavior for the tested cases: read-onlyfindsearches still return automaticALLOW, while mutatingfindactions no longer return automaticALLOW.Additional validation performed:
The targeted Bash pytest cases were invoked on Windows and reported
4 skippedbecause these Bash parser test classes already have an existingwin32class-level skip. Parser behavior andBash.check_permissions()behavior were therefore verified directly in addition to syntax and whitespace checks.Possible call chain / impact
Before this fix, the affected path was:
After this fix, mutating
findactions are detected before the generic prefix allow path, so they are classified as non-read-only and continue through the normal permission flow.Sibling surfaces checked:
tests/permission_bash_parser_test.pytests/builtin_bash_test.pyfindsearches with action-like pattern values (-name,-path,-regex)findtests followed by mutating actions (-daystart,-nogroup,-nouser)Checklist
Please check the following items before code is ready to be reviewed.