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..5cd85b3d1b --- /dev/null +++ b/tests/tools/test_approval_grep_in_substitution.py @@ -0,0 +1,75 @@ +"""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 + + +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 1ef703670a..9790b30a15 100644 --- a/tools/approval_detection.py +++ b/tools/approval_detection.py @@ -578,11 +578,30 @@ 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 - ``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 + # 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] @@ -591,26 +610,47 @@ 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 == "`": + 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 + 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 @@ -1021,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):