From d41f62107148b630986521bc1286f575598a3aa6 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 5 Sep 2026 00:19:03 -0700 Subject: [PATCH 1/2] fix(approval): a grep inside "$(...)" no longer trips the hardline "malformed payload" block The quoted-grep scanner tokenizes from the grep's position to decide which quoted operand is inert data. _shell_tokens_with_spans lexed from there to the END of the top-level segment, so for a grep nested in a substitution such as sed -n "$(grep -n X f | cut -d: -f1),+3p" f it read past the closing ")" into the enclosing command, met the outer closing double quote as an opener, ended in quote state, returned None ("malformed"), and detect_hardline_command reported the whole command as being on the unconditional blocklist. Nothing about the command was dangerous. In the 1,393-agent refactor run this fired 546 times across the workers (every one a false positive; 318 of 323 retries succeeded by rewording), and three times on the maintainer's own session in one day. The lexer now stops at the end of the simple command it was asked to lex: an unquoted list separator or pipe or newline, or the ")" / backtick that closes the substitution it sits inside (tracking $(...) depth opened after start so a nested substitution's closer is not mistaken for the outer one). Genuinely unbalanced quoting still returns None and still fails closed; every hardline pattern is unchanged. Tests: six substitution shapes lex clean and are not hardline; the lexer yields exactly the grep's own words; unterminated quoting still blocks; rm -rf / , $(rm -rf /) and shutdown still block. --- .../test_approval_grep_in_substitution.py | 41 +++++++++++++++++++ tools/approval_detection.py | 41 ++++++++++++++----- 2 files changed, 72 insertions(+), 10 deletions(-) create mode 100644 tests/tools/test_approval_grep_in_substitution.py diff --git a/tests/tools/test_approval_grep_in_substitution.py b/tests/tools/test_approval_grep_in_substitution.py new file mode 100644 index 0000000000..abfff1359b --- /dev/null +++ b/tests/tools/test_approval_grep_in_substitution.py @@ -0,0 +1,41 @@ +"""A grep nested inside a command substitution must lex as its own simple command. + +The quoted-grep scanner tokenizes from the grep's position to decide which quoted operand is inert data. +It used to read to the end of the segment, so for ``"$(grep … | cut …)"`` it swallowed the enclosing +command's closing quote, called the quoting unbalanced, and the whole command was reported as a hardline +block. Every such report was a false positive (546 in one run); the lexer now stops at the end of the +simple command: an unquoted ``;`` ``|`` ``&`` newline, or the ``)`` / backtick closing its substitution. +""" +import pytest + +from tools.approval_detection import _quoted_grep_pattern_spans, _shell_tokens_with_spans, detect_hardline_command + + +@pytest.mark.parametrize("command", [ + 'sed -n "$(grep -n X f | cut -d: -f1),+3p" f', + 'sed -n "$(grep -n \'^### 1.\' d.md | cut -d: -f1),$(grep -n \'^### 3.\' d.md | cut -d: -f1)p" d.md', + 'echo "$(grep -c x f)"', + 'x="$(grep -n foo bar | head -1)"; echo $x', + 'for f in a b; do echo "$f sig=$(grep -cE \'^x\' $f)"; done', + 'echo `grep -c x f`', +]) +def test_grep_inside_a_substitution_is_not_malformed(command): + _spans, malformed = _quoted_grep_pattern_spans(command) + assert malformed is False + assert detect_hardline_command(command) == (False, None) + + +def test_lexer_stops_at_the_end_of_the_simple_command(): + seg = 'sed -n "$(grep -n X f | cut -d: -f1),+3p" f' + toks = _shell_tokens_with_spans(seg, seg.index("grep")) + assert [t[0] for t in toks] == ["grep", "-n", "X", "f"] + + +def test_genuinely_unbalanced_quoting_still_fails_closed(): + assert _shell_tokens_with_spans("grep 'unterminated", 0) is None + assert detect_hardline_command("grep 'unterminated")[0] is True + + +def test_hardline_patterns_unchanged(): + for command in ("rm -rf /", 'echo "$(rm -rf /)"', "sudo shutdown -h now"): + assert detect_hardline_command(command)[0] is True, command diff --git a/tools/approval_detection.py b/tools/approval_detection.py index 1ef703670a..23b7613abd 100644 --- a/tools/approval_detection.py +++ b/tools/approval_detection.py @@ -581,8 +581,16 @@ def _command_parser_limit_exceeded(command: str) -> bool: def _shell_tokens_with_spans(segment: str, start: int): """Return shell words as ``(value, start, end, quoted)`` or ``None`` on malformed quoting. Deliberately small lexer that never expands shell syntax; it exists to keep source spans (which - ``shlex`` does not expose) for deciding which quoted grep operand is data, not another command.""" + ``shlex`` does not expose) for deciding which quoted grep operand is data, not another command. + + Lexing stops at the end of the simple command that begins at *start*: an unquoted ``;``, ``|``, + ``&`` or newline, or the ``)`` / backtick that closes the substitution the command sits inside. + Without that, a grep nested as ``"$(grep … | cut …)"`` was lexed together with the enclosing + command's closing quote, read as unbalanced quoting, and reported as a hardline block (546 + blocked turns in one run, every one a false positive; ``sed -n "$(grep -n X f | cut -d: -f1),+3p" f`` + is the canonical shape).""" tokens, value, token_start, quote = [], [], None, None + depth = 0 # $(...) nesting opened AFTER start; a closer at depth 0 ends the enclosing substitution def flush(end: int) -> None: raw = segment[token_start:end] @@ -591,26 +599,39 @@ def _shell_tokens_with_spans(segment: str, start: int): inert = (raw.startswith("'") and raw.endswith("'")) or ("='" in raw and raw.endswith("'")) tokens.append(("".join(value), token_start, end, inert)) + end_at = len(segment) for kind, i, _, _ in _scan_shell(segment, start): - if kind == "char" and not quote and segment[i].isspace(): - if token_start is not None: - flush(i) - value, token_start = [], None - continue + ch = segment[i] + if kind == "char" and not quote: + if ch.isspace() and ch != "\n": + if token_start is not None: + flush(i) + value, token_start = [], None + continue + if segment.startswith("$(", i): + depth += 1 + elif ch == ")" or ch == "`": + if depth == 0: + end_at = i + break + depth -= 1 + elif ch in ";|&\n": + end_at = i + break if token_start is None: token_start = i if kind == "quote": - quote = None if quote else segment[i] + quote = None if quote else ch elif kind == "esc": value.append(segment[i + 1]) - elif segment[i] == "\\" and not quote: + elif ch == "\\" and not quote: return None # dangling backslash else: - value.append(segment[i]) + value.append(ch) if quote: return None if token_start is not None: - flush(len(segment)) + flush(end_at) return tokens From 98bf8b20734ae6372047a800eb4e4d3dc5862406 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 5 Sep 2026 06:07:21 -0700 Subject: [PATCH 2/2] fix(approval): a quoted $(...)/backtick body keeps its command boundaries when newlines are masked; a backtick operand is not a closer Independent review of the first fix found two defects. Bounding the grep lexer removed the old "malformed payload" floor, and that floor had been the ONLY thing blocking a hardline command hidden after a newline inside a double-quoted $(grep ...) substitution: _mask_quoted_newlines treated the newline as quoted DATA and replaced it with a space, so the later command-start pass saw `/dev/null reboot` as an operand and the public guards approved a hardline command with no callback. A substitution body inside double quotes is executable; the masker now recurses into $(...) and backtick bodies with a fresh quote state so a newline there stays a command boundary. The witness now blocks as "system shutdown/reboot" (the correct verdict), as do the ; / && / | / nested / backtick variants. Second: the lexer stopped at ANY backtick as if it closed an enclosing substitution, so a grep with a backtick OPERAND lost its operands and was reported malformed (a new false positive). A backtick opened after the grep is an operand substitution; only an unmatched one closes the enclosing command. Tests: the six hardline-inside-substitution shapes block as themselves; the public guard blocks with zero approval callbacks; the backtick operand, a data pattern and a quoted-newline commit message stay allowed. Approval/hardline suites: 637 passed; the one failure (real-binary sort payload probe) fails identically on main. --- .../test_approval_grep_in_substitution.py | 34 +++++++++++++ tools/approval_detection.py | 48 +++++++++++++++++-- 2 files changed, 77 insertions(+), 5 deletions(-) diff --git a/tests/tools/test_approval_grep_in_substitution.py b/tests/tools/test_approval_grep_in_substitution.py index abfff1359b..5cd85b3d1b 100644 --- a/tests/tools/test_approval_grep_in_substitution.py +++ b/tests/tools/test_approval_grep_in_substitution.py @@ -39,3 +39,37 @@ def test_genuinely_unbalanced_quoting_still_fails_closed(): def test_hardline_patterns_unchanged(): for command in ("rm -rf /", 'echo "$(rm -rf /)"', "sudo shutdown -h now"): assert detect_hardline_command(command)[0] is True, command + + +class TestExecutableSubstitutionBodiesStayExecutable: + """Bounding the grep lexer removed the old 'malformed' floor; the newline masker must then treat a + substitution body inside double quotes as CODE, or a newline-separated hardline command hides as an + operand (independent review witness: blocked on main as 'malformed', approved on the first fix).""" + + @pytest.mark.parametrize("cmd", [ + 'echo "$(grep -P \'safe\' /dev/null\nreboot)"', + 'echo "$(grep x f; reboot)"', + 'echo "$(grep x f && reboot)"', + 'echo "$(grep x f | shutdown -h now)"', + 'echo "$(echo $(grep x f)\nreboot)"', + 'echo "`grep x f\nreboot`"', + ]) + def test_hardline_command_inside_a_quoted_substitution_blocks_as_itself(self, cmd): + blocked, reason = detect_hardline_command(cmd) + assert blocked and reason == "system shutdown/reboot" + + def test_public_guard_blocks_without_offering_approval(self, tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hh")) + from tools.approval import check_dangerous_command + calls = [] + result = check_dangerous_command('echo "$(grep -P \'safe\' /dev/null\nreboot)"', "local", + approval_callback=lambda *a, **k: calls.append(a) or False) + assert result["approved"] is False and "hardline" in result["message"].lower() and calls == [] + + @pytest.mark.parametrize("cmd", [ + "grep -e `echo needle` file", # backtick OPERAND (opened after the grep) is not a closer + "grep -F 'sudo reboot' notes.md", # data pattern + 'git commit -m "fix\nsudo reboot handling"', # quoted data newline + ]) + def test_benign_shapes_stay_allowed(self, cmd): + assert detect_hardline_command(cmd) == (False, None) diff --git a/tools/approval_detection.py b/tools/approval_detection.py index 23b7613abd..9790b30a15 100644 --- a/tools/approval_detection.py +++ b/tools/approval_detection.py @@ -578,6 +578,14 @@ def _command_parser_limit_exceeded(command: str) -> bool: return sum(command.count(char) for char in ";&|\n") >= _MAX_DETECTION_SEGMENTS +def _backtick_end_from(segment: str, i: int) -> int | None: + """Index of the backtick closing the one opened at ``i``, or None when none follows.""" + j = segment.find("`", i + 1) + while j != -1 and segment[j - 1] == "\\": + j = segment.find("`", j + 1) + return None if j == -1 else j + + def _shell_tokens_with_spans(segment: str, start: int): """Return shell words as ``(value, start, end, quoted)`` or ``None`` on malformed quoting. Deliberately small lexer that never expands shell syntax; it exists to keep source spans (which @@ -591,6 +599,9 @@ def _shell_tokens_with_spans(segment: str, start: int): is the canonical shape).""" tokens, value, token_start, quote = [], [], None, None depth = 0 # $(...) nesting opened AFTER start; a closer at depth 0 ends the enclosing substitution + # A backtick opened AFTER start is an operand substitution (``grep -e `cmd` f``); the matching + # closer belongs to it, not to an enclosing backtick the command might sit inside. + in_backtick = False def flush(end: int) -> None: raw = segment[token_start:end] @@ -610,7 +621,15 @@ def _shell_tokens_with_spans(segment: str, start: int): continue if segment.startswith("$(", i): depth += 1 - elif ch == ")" or ch == "`": + elif ch == "`": + if in_backtick: + in_backtick = False + elif depth == 0 and _backtick_end_from(segment, i) is None: + end_at = i # unmatched: it closes the substitution this command sits inside + break + else: + in_backtick = True + elif ch == ")": if depth == 0: end_at = i break @@ -1042,10 +1061,29 @@ def _mask_quoted_newlines(command: str) -> str: exactly as the shell would, so masking them cannot hide a runnable command.""" if "\n" not in command: return command - return "".join( - " " if quote and kind == "char" and command[i] == "\n" else command[i:j] - for kind, i, j, quote in _scan_shell(command) - ) + return _mask_quoted_newlines_span(command, 0, len(command)) + + +def _mask_quoted_newlines_span(command: str, start: int, end: int) -> str: + """``_mask_quoted_newlines`` over ``command[start:end]``. A ``$(...)`` / backtick substitution + inside double quotes is EXECUTABLE, not data: its body is re-scanned with a fresh quote state so a + newline that separates commands inside it survives as a command boundary. Masking it as quoted + data turned ``"$(grep x f\nreboot)"`` into ``... f reboot)``, which no later stage can tell from an + operand; the pre-fix scanner only caught it by refusing the whole command as malformed.""" + out: list[str] = [] + for kind, i, j, quote in _scan_shell(command, start, end, subst="q"): + if kind == "subst": + # j is the index just past the closer; keep the opener and closer, recurse into the body. + body_start = i + (2 if command.startswith("$(", i) else 1) + body_end = j - 1 + out.append(command[i:body_start]) + out.append(_mask_quoted_newlines_span(command, body_start, body_end)) + out.append(command[body_end:j]) + elif quote and kind == "char" and command[i] == "\n": + out.append(" ") + else: + out.append(command[i:j]) + return "".join(out) def _iter_shell_command_word_spans(command: str):