Skip to content

[Bug]: check_dangerous_command matches fdisk/format as bare substrings, so ordinary commands get a bypass-immune ASK #2809

Description

@sxh313

Prerequisites

  • I have searched the existing issues and discussions, and this is not a duplicate.
  • This is a bug, not a usage question. (For questions, please use Discussions instead.)

Background / Description

BashCommandParser.check_dangerous_command (tool/_builtin/_bash_parser.py:647) documents
itself as precise about this:

Uses word-boundary aware matching to avoid false positives like 'git add' matching 'dd'
pattern.

The branch that decides how to match, however, keys on the pattern's length
(_bash_parser.py:666-677):

for pattern in DANGEROUS_COMMANDS:
    if " " not in pattern and len(pattern) <= 4:
        regex = r"\b" + re.escape(pattern) + r"\b"     # word boundary
        if re.search(regex, normalized):
            return pattern
    else:
        if pattern in normalized:                       # bare substring
            return pattern

Against DANGEROUS_COMMANDS (tool/_constants.py:56-68) the len(pattern) <= 4 test
selects only dd and mkfs. The other nine patterns are multi-word, so the only thing
the length gate accomplishes is to throw fdisk (5) and format (6) into the bare
substring branch — the two patterns in the list that are also ordinary English words and
ordinary CLI vocabulary.

That substring match runs against the whole command line, so the pattern is reported
wherever it appears: as an npm/yarn/make target, as an option name, as part of a filename.
check_dangerous_command is step 2 of Bash.check_permissions (_bash.py:278) and a hit
returns ASK with bypass_immune=True (_bash.py:286), which means an allow rule cannot
silence it, and under PermissionMode.DONT_ASK a bypass-immune ASK is converted to DENY
(permission/_decision.py:41-42, :55) because no user is available. So a false positive
here does not just add a prompt — it can hard-block a command in exactly the unattended
mode this repo is built for.

Step 1 does not rescue every case, but it does narrow the set: is_read_only_command
(_bash.py:270) returns ALLOW first, and it matches read-only programs by prefix, so a
command starting with git, ls, cat, docker ps never reaches step 2. The reachable
false positives are therefore the ones whose own program is not read-only — npm run format, tar --format=ustar, cp fdisk.log /tmp — and those are all still blocked today.

Reproduction

No network, no API key, no shell execution — every call below is pure string work.

import re

from agentscope.tool._constants import DANGEROUS_COMMANDS
from agentscope.tool._builtin._bash_parser import BashCommandParser

parser = BashCommandParser()

print("patterns matched as bare substrings:")
for pattern in DANGEROUS_COMMANDS:
    if " " in pattern or len(pattern) > 4:
        print(f"   {pattern!r} (len={len(pattern)})")

CASES = [
    "npm run format",
    "yarn format",
    "make format",
    "eslint --format stylish src/",
    "tar --format=ustar -cf out.tar src",
    "python format_report.py",
    "cp fdisk.log /tmp",
    "node fdisk-parser.js",
    "mv format_names.py format_tools.py",
]
print("\ncheck_dangerous_command on each:")
for cmd in CASES:
    hit = parser.check_dangerous_command(cmd)
    read_only = parser.is_read_only_command(cmd)
    print(f"   {cmd!r:45} -> {hit!r}  (read-only: {read_only})")

Output on main (083cbd19), Python 3.11 — the first block shows only fdisk/format
are single-word patterns in the substring branch, and the second shows all nine reachable
cases flagged while none of them is read-only (so none is saved by step 1):

patterns matched as bare substrings:
   'rm -rf' (len=6)
   'sudo rm' (len=7)
   'fdisk' (len=5)
   'format' (len=6)
   'chmod 777' (len=9)
   'chmod -R 777' (len=12)
   'chown -R' (len=8)
   'kill -9' (len=7)
   '> /dev/' (len=7)

check_dangerous_command on each:
   'npm run format'                              -> 'format'  (read-only: False)
   'yarn format'                                 -> 'format'  (read-only: False)
   'make format'                                 -> 'format'  (read-only: False)
   'eslint --format stylish src/'                -> 'format'  (read-only: False)
   'tar --format=ustar -cf out.tar src'          -> 'format'  (read-only: False)
   'python format_report.py'                     -> 'format'  (read-only: False)
   'cp fdisk.log /tmp'                           -> 'fdisk'  (read-only: False)
   'node fdisk-parser.js'                        -> 'fdisk'  (read-only: False)
   'mv format_names.py format_tools.py'          -> 'format'  (read-only: False)
flagged: 9 / 9

tests/permission_bash_parser_test.py::BashParserDangerousCommandTest::test_safe_commands
already asserts that a safe command must return None, so "a false positive here is a bug"
is this file's own standard, not a new one.

Expected behaviour

A single-word pattern should be judged only where bash actually runs a program, i.e. the
name node of each parsed command — the same mechanism this class already uses elsewhere:
_is_mutating_find_command (_bash_parser.py:239) parses the command, walks to
_find_first_simple_command (:626) and reads child_by_field_name("name") (:251).
npm run format should return None, and fdisk /dev/sda, /sbin/fdisk -l,
\fdisk -l and sudo -u root dd … should keep returning their pattern.

Scope

Two points this issue deliberately keeps out of a "one-line regex" fix:

  • Dropping and len(pattern) <= 4 is not a fix. -, ? and = are non-word
    characters, so format inside --format= or ?format=json still has word boundaries:
    \bformat\b on 'npm run --format=ustar x': True
    \bformat\b on 'curl -s "https://api?format=json"': True
    The distinction has to be positional, not lexical.
  • Multi-word patterns (rm -rf, chmod 777, …) stay substring-matched. A quoted
    echo "rm -rf cache" asking for confirmation is conservative and reasonable, and
    changing it would widen this diff for no safety gain.

Widening detection (e.g. adding sfdisk/cfdisk, or covering other wrappers) is a separate
concern; see the follow-up note in the PR. #2501 does the same kind of "resolve the real
command name" work for the dangerous-removal path in tool/_builtin/_bash.py, which is a
different function in a different file — this issue is the narrowing half for
check_dangerous_command.

No activity

Activity on this issue will appear here.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    triage/confirmedVerified: the reported defect exists

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions