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):