Repository navigation
fix(tool): reject mutating find commands as read-only - #2004
DavdGao merged 4 commits into
Conversation
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
@DavdGao Updated as suggested: _is_mutating_find_command now uses the existing tree-sitter AST parser, and FIND_PREDICATES_WITH_ARGUMENTS has been removed. |
Sorry, something went wrong.
DavdGao
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
AgentScope Version
Current branch based on main at 922e673b. No released version bump is involved.
Description
Fixes #2003 by preventing mutating GNU find actions from being classified as read-only before Bash.check_permissions() reaches the normal permission path.
What Problem This Solves
Bash.check_permissions() has an early allow path for commands that BashCommandParser.is_read_only_command() marks as safe. This is the right behavior for inspection-only commands such as ls, cat, grep, and ordinary find searches, because those commands should not require extra confirmation just to read or list files.
The bug was that find was treated as read-only at the command-prefix level. In _is_single_command_read_only(), the generic check accepted any command where cmd == readonly_cmd or cmd.startswith(readonly_cmd + " "). Since find was present in READ_ONLY_COMMANDS, the parser could classify a command as read-only immediately after seeing the leading find text, without checking the rest of the find expression.
That distinction matters because find is not only a search command. A find expression 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:
find . -delete # deletes matched files find . -exec rm {} \; # executes rm for each match find . -fprint results.txt # writes results to a named fileBefore this fix, those commands could be classified through the same prefix path as a harmless search like:
The intended boundary is more precise: find should 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 -delete should 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 is find, 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 find predicates 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:
find . -name '-delete' -> read_only=True find . -path './-fprint' -> read_only=True find . -regex '.*-exec.*' -> read_only=True find . -daystart -delete -> read_only=False find . -nogroup -delete -> read_only=False find . -delete -> read_only=False find . -exec rm {} \; -> read_only=False find . -fprint results.txt -> read_only=FalseDirect Bash.check_permissions() verification confirmed the same runtime permission behavior for the tested cases: read-only find searches still return automatic ALLOW, while mutating find actions no longer return automatic ALLOW.
Additional validation performed:
The targeted Bash pytest cases were invoked on Windows and reported 4 skipped because these Bash parser test classes already have an existing win32 class-level skip. Parser behavior and Bash.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 find actions 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:
Checklist
Please check the following items before code is ready to be reviewed.