diff --git a/tests/tools/test_hardline_escaped_quote_state.py b/tests/tools/test_hardline_escaped_quote_state.py new file mode 100644 index 0000000000..e9b721a163 --- /dev/null +++ b/tests/tools/test_hardline_escaped_quote_state.py @@ -0,0 +1,42 @@ +"""The hardline floor judges shell quoting on the AUTHOR's text, not on normalized text. + +``_normalize_command_for_detection`` strips backslash escapes so ``r\\m`` cannot hide ``rm``. That +is right for pattern matching but wrong for quote tracking: ``\\"`` becomes ``"`` and flips quote +parity. Two consumers paid for it in opposite directions: + +* false positive — a shell-valid ``grep -o "[^\\"]*" f`` lexed as unterminated and hit the + unconditional block (118 of 125 hardline blocks in one week of real agent use, all benign greps); +* bypass — ``cat "f\\"n.txt"; rm -rf --no-preserve-root /`` put the ``; rm`` start "inside" a + phantom quote, so no command start was marked and the floor let it through. + +Direction from #85922 (@Soju06): read quote state from the raw command (only quoted newlines masked). +""" +import pytest + +from tools.approval_detection import detect_dangerous_command, detect_hardline_command + + +@pytest.mark.parametrize("command", [ + 'grep -o "[^\\"]*" f', + r'grep -n "^from\|^__all__\|^ \"" tools/environments/__init__.py | head -15', + r'egrep -n "alpha\"beta|gamma" input.txt', + r'grep -v "^./tests/\|def \|\"\"\"" x.py | grep -v "read_only=True"', + 'grep -n "; reboot" f.txt', + 'echo "value is ${HOME}/x"', +]) +def test_escaped_quotes_in_a_valid_grep_pattern_are_not_malformed(command): + assert detect_hardline_command(command) == (False, None) + assert detect_dangerous_command(command) == (False, None, None) + + +@pytest.mark.parametrize(("command", "description"), [ + (r'cat "f\"n.txt"; rm -rf --no-preserve-root /', "recursive delete of root filesystem"), + (r'echo "a\"b"; reboot', "system shutdown/reboot"), + (r'echo "a\"b" && rm -rf ~', "recursive delete of home directory"), + (r'echo "a\"b"; rm${IFS}-rf${IFS}/', "recursive delete of root filesystem"), + (r'grep -n "prefix \"quoted\" suffix" input.txt; reboot', "system shutdown/reboot"), + ("printf \\\\\nreboot", "system shutdown/reboot"), + ("grep 'unterminated", "command parser limit or malformed executable payload"), +]) +def test_escaped_quote_before_a_hardline_command_does_not_hide_it(command, description): + assert detect_hardline_command(command) == (True, description) diff --git a/tools/approval_detection.py b/tools/approval_detection.py index 617fb8de88..0d7732f24a 100644 --- a/tools/approval_detection.py +++ b/tools/approval_detection.py @@ -175,8 +175,11 @@ def detect_hardline_command(command: str) -> tuple: """Check hardline patterns (NEVER bypassable, even in YOLO) -> (is_hardline, description).""" if _command_parser_limit_exceeded(command): return (True, _PARSER_LIMIT_DESCRIPTION) - normalized = _normalize_command_for_detection(command) - _, malformed_grep = _grep_safe_detection_variant(normalized) + # The malformed-quoting verdict needs the author's quote state. Normalization strips escapes + # (`\"` -> `"`), so a shell-valid pattern like `grep -o "[^\"]*"` lexed as unterminated and was + # reported as a hardline block (118 of 125 hardline blocks in one week of real use, every one a + # benign grep). Only quoted newlines are masked: they are data, and masking keeps quoting intact. + _, malformed_grep = _grep_safe_detection_variant(_mask_quoted_newlines(command)) if malformed_grep: return (True, _MALFORMED_EXEC_DESCRIPTION) for command_variant in _command_detection_variants(command): @@ -1087,7 +1090,9 @@ def _iter_shell_command_starts(command: str): starts.append(inner) scan(inner, end if j is None else j - 1) elif kind == "char" and quote is None and i != skip: - if command[i] in "({;\n": + # `${` opens a parameter expansion, not a brace group: a start marked inside it would + # split `${IFS}` and defeat the IFS collapse in normalization. + if command[i] in "({;\n" and not (command[i] == "{" and i > 0 and command[i - 1] == "$"): starts.append(i + 1) elif command[i] in "&|": repeated = i + 1 < end and command[i + 1] == command[i] @@ -1107,14 +1112,14 @@ def _iter_shell_command_starts(command: str): starts.append(end) -def _mark_command_starts(command: str) -> str: - """Insert a newline before each real (quote-aware) command start. +def _mark_command_starts(command: str, marker: str = "\n") -> str: + """Insert *marker* (a newline) before each real (quote-aware) command start. ``\\n`` is already a ``_CMDPOS`` separator, so this exposes subshell ``(cmd)`` and brace-group ``{ cmd; }`` openers — which the flat pattern class omits — to the anchored patterns WITHOUT the quoted-prose false positives that adding ``(`` / ``{`` to ``_CMDPOS`` would cause: starts inside quotes are never produced, so ``--title "block (reboot)"`` is left as-is.""" offsets = sorted(o for o in _iter_shell_command_starts(command) if o > 0) - return _splice(command, [(o, o, "\n") for o in offsets]) if offsets else command + return _splice(command, [(o, o, marker) for o in offsets]) if offsets else command def _mask_quoted_newlines(command: str) -> str: @@ -1376,6 +1381,14 @@ def _command_detection_variants(command: str): marked = _mark_command_starts(grep_safe) if marked != grep_safe and fresh(marked): yield marked + # Every variant above tracks quotes on NORMALIZED text, where `\"` has already become `"`. That + # flips quote parity, so in `cat "f\"n.txt"; rm -rf /` the `; rm` start sat "inside" a phantom + # quote, no start was marked, and the hardline floor let it through. Mark starts on the RAW + # command (only quoted newlines masked), then normalize; the leading space keeps the marker + # from being eaten as a `\` continuation when the preceding text ends in a backslash. + faithful = _normalize_command_for_detection(_mark_command_starts(_mask_quoted_newlines(command), marker=" \n")) + if fresh(faithful): + yield faithful # Quoting/escaping can spell an executable in pieces (r\m, r''m). Keep that deobfuscation scoped # to command words so arguments don't false-positive. for word_start, word_end, word in _iter_shell_command_word_spans(normalized): diff --git a/website/docs/user-guide/security.md b/website/docs/user-guide/security.md index a3927fba12..8a69a1684b 100644 --- a/website/docs/user-guide/security.md +++ b/website/docs/user-guide/security.md @@ -140,6 +140,8 @@ The blocklist is the floor below `--yolo`. It trips **before** the approval laye If you hit the blocklist, the tool call returns an explanatory error to the agent and nothing runs. If a legitimate workflow needs one of these commands (you're the operator of a wipe-and-reinstall pipeline, for example), run it outside the agent. +The floor also fails closed on a command whose shell quoting cannot be parsed (`grep 'unterminated`): the error says `malformed executable payload`. Quoting is judged on the command exactly as written, so shell-valid escapes inside a quoted pattern (`grep -o "[^\"]*" file`) are not malformed, and an escaped quote before a separator (`echo "a\"b"; reboot`) does not hide the command that follows it. + ### User-Defined Deny Rules (`approvals.deny`) The hardline blocklist is fixed and code-shipped. `approvals.deny` is its user-editable counterpart: a list of glob patterns that block matching terminal commands unconditionally — **before** `--yolo`, `/yolo`, and `approvals.mode: off` are consulted. Use it to run yolo-with-exceptions: "let the agent do everything, except these specific things, ever."