diff --git a/tests/tools/test_terminal_heredoc_background_guard.py b/tests/tools/test_terminal_heredoc_background_guard.py index 72d9408d27..c20e53f18e 100644 --- a/tests/tools/test_terminal_heredoc_background_guard.py +++ b/tests/tools/test_terminal_heredoc_background_guard.py @@ -1,4 +1,4 @@ -"""Regression tests for heredoc-aware background-'&' detection. +"""Regression tests for conservative heredoc-aware background-'&' detection. Context: ``_foreground_background_guidance`` blocks a foreground command that looks like it backgrounds a process with ``&`` (so the agent is nudged toward @@ -6,20 +6,23 @@ looks like it backgrounds a process with ``&`` (so the agent is nudged toward blank out quoted content so an ``&`` *inside a string* isn't mistaken for the shell background operator. -Bug: ``_strip_quotes`` documented that it strips "heredoc-style inline text" -but only stripped single/double/backtick quotes — it had no heredoc handling. -So a foreground command carrying a heredoc whose BODY contains a spaced ``&`` -was wrongly rejected. Real-world triggers: +Bug (#63788): ``_strip_quotes`` documented that it strips "heredoc-style +inline text" but had no heredoc handling, so a foreground command carrying a +heredoc whose BODY contains a spaced ``&`` was wrongly rejected. Real-world +triggers: - ``osascript <<'EOF' ... set x to "a" & b ... EOF`` (AppleScript concat) - ``python3 <<'EOF' ... z = a & b ... EOF`` (Python bitwise-and) -- a heredoc body containing literal UI text like ``FaceTime & Privacy`` -The fix strips heredoc bodies (``</dev/null 2>" + AMP + "1 " + AMP + ")" + NL + + "EOF" + ) + assert guidance(cmd) is not None + + def test_shell_interpreter_payload_is_still_scanned(self): + cmd = ( + "bash <<'EOF'" + NL + + "nohup sleep 10 >/dev/null 2>" + AMP + "1 " + AMP + NL + + "EOF" + ) + assert guidance(cmd) is not None + + def test_python_elsewhere_does_not_authorize_bash_heredoc(self): + cmd = ( + "python3 -c 'pass'; bash <<'EOF'" + NL + + "nohup sleep 10 " + AMP + NL + + "EOF" + ) + assert guidance(cmd) is not None + + def test_pipeline_python_does_not_authorize_bash_heredoc(self): + cmd = ( + "bash <<'EOF' | python3" + NL + + "nohup sleep 10 " + AMP + NL + + "EOF" + ) + assert guidance(cmd) is not None + + def test_nested_substitution_does_not_authorize_heredoc(self): + cmd = ( + "python3 -c $(bash <<'SH'" + NL + + "nohup sleep 100 >/dev/null 2>" + AMP + "1 " + AMP + NL + + "printf pass" + NL + + "SH" + NL + + ")" + ) + assert guidance(cmd) is not None + + +class TestInactiveMarkersCannotHideShellTail: + """Fake '<<' markers must not swallow a later real background operator.""" + + def test_marker_in_comment_does_not_hide_background_command(self): + cmd = ": # </dev/null " + AMP + NL + + "print('ok')" + NL + + "PY" + ) + assert guidance(cmd) is not None + class TestRealBackgroundingStillBlocked: """A genuine shell-level '&' must still be caught after the fix.""" @@ -72,26 +181,88 @@ class TestRealBackgroundingStillBlocked: def test_inline_background(self): assert guidance("sleep 100 " + AMP + " echo done") is not None + def test_background_on_heredoc_opener(self): + cmd = "python3 - <<'PY' " + AMP + NL + "print('ok')" + NL + "PY" + assert guidance(cmd) is not None + def test_background_after_heredoc(self): - # A heredoc that itself is backgrounded — the trailing '&' after the - # closing delimiter is real backgrounding and must still be flagged. - cmd = "cat <<'EOF' > f.txt" + NL + "payload" + NL + "EOF" + NL + "long_running " + AMP + # A real backgrounding '&' AFTER the closing delimiter is still caught. + cmd = ( + "python3 - <<'PY'" + NL + + "print('ok')" + NL + + "PY" + NL + + "long_running " + AMP + ) + assert guidance(cmd) is not None + + def test_background_after_cat_heredoc_redirect(self): + cmd = ( + "cat <<'EOF' > f.txt" + NL + "payload" + NL + "EOF" + NL + + "long_running " + AMP + ) assert guidance(cmd) is not None -class TestStripQuotesHeredoc: - """Direct unit checks on the helper.""" +class TestStripHelpers: + """Direct unit checks on the masking helpers.""" - def test_heredoc_body_removed(self): - cmd = "osascript <<'EOF'" + NL + 'x ' + AMP + " y" + NL + "EOF" + def test_inert_body_removed_shell_tail_preserved(self): + cmd = ( + "python3 - <<'PY'" + NL + + "x = left " + AMP + " right" + NL + + "PY" + NL + + "sleep 10 " + AMP + ) stripped = _strip_quotes(cmd) - # The bare spaced ampersand from the body must not survive. - assert (" " + AMP + " ") not in stripped + assert "x = left " + AMP + " right" not in stripped + assert "sleep 10 " + AMP in stripped - def test_multiple_heredocs(self): + def test_masking_preserves_line_structure(self): + cmd = ( + "python3 - <<'PY'" + NL + + "a " + AMP + " b" + NL + + "c " + AMP + " d" + NL + + "PY" + NL + + "echo done" + ) + assert strip_inert_heredoc_bodies(cmd).count(NL) == cmd.count(NL) + + def test_multiple_sequential_heredocs(self): cmd = ( "cat <<'A'" + NL + "one " + AMP + " two" + NL + "A" + NL + "cat <<'B'" + NL + "three " + AMP + " four" + NL + "B" ) - stripped = _strip_quotes(cmd) + stripped = strip_inert_heredoc_bodies(cmd) assert (" " + AMP + " ") not in stripped + + def test_ambiguous_input_returned_unchanged(self): + # Unparseable '<<' token → fail closed, identical string back. + cmd = "cat <<" + NL + "text " + AMP + " more" + assert strip_inert_heredoc_bodies(cmd) == cmd + + def test_delimiter_requires_exact_terminator_line(self): + # Indented terminator doesn't close a normal heredoc; the heredoc is + # unterminated → everything stays visible. + cmd = ( + "python3 - <<'PY'" + NL + + "payload " + AMP + " text" + NL + + "\tPY" + ) + assert strip_inert_heredoc_bodies(cmd) == cmd + + def test_dash_heredoc_space_indented_line_is_body(self): + # For <<- only TABS are stripped before terminator comparison. A + # space-indented " PY" line does NOT close the heredoc (verified + # against real bash), so the following "sleep 7 &" line is still BODY + # text — inert data that cat prints — and masking it is correct. + cmd = ( + "cat <<-'PY'" + NL + + "payload" + NL + + " PY" + NL + + "sleep 7 " + AMP + NL + + "PY" + NL + + "echo after" + ) + stripped = strip_inert_heredoc_bodies(cmd) + assert "sleep 7 " + AMP not in stripped + assert "echo after" in stripped diff --git a/tools/shell_heredoc.py b/tools/shell_heredoc.py new file mode 100644 index 0000000000..53568783cf --- /dev/null +++ b/tools/shell_heredoc.py @@ -0,0 +1,346 @@ +"""Conservative heredoc masking for shell-command scanners. + +Several guards scan raw command text for dangerous shell syntax (the +foreground background-'&' guard in ``tools/terminal_tool.py``, the +blocked-command checks, the gateway lifecycle guard in +``cron/lifecycle_guard.py``). Heredoc *bodies* are usually inline data — +AppleScript concatenation, Python bitwise-and, literal UI text — and +scanning them produces false positives. + +Naively stripping every heredoc body is unsafe the other way: fake ``<<`` +markers inside quotes or comments can swallow a *real* background operator +that follows them, and some heredoc bodies genuinely execute (unquoted +delimiters allow ``$(...)`` expansion; ``bash <<'EOF'`` runs the body as +shell). This module therefore masks a body ONLY when all of the following +hold, and otherwise leaves the command untouched: + +- every heredoc delimiter on the opener is quoted (``<<'EOF'`` / ``<<"EOF"`` + / ``< str: + """Blank inert quoted spans without erasing shell-active substitutions. + + Single-quoted spans become ``''``. Double-quoted spans become ``""`` + UNLESS they contain ``$(`` or a backtick (still executable — keep them + visible). Backtick spans are always kept (executable). + """ + result = [] + cursor = 0 + while cursor < len(command): + char = command[cursor] + if char == "'": + closing = command.find("'", cursor + 1) + if closing == -1: + result.append(command[cursor:]) + break + result.append("''") + cursor = closing + 1 + continue + if char == '"': + end = cursor + 1 + while end < len(command): + if command[end] == "\\" and end + 1 < len(command): + end += 2 + continue + if command[end] == '"': + end += 1 + break + end += 1 + if not command[cursor:end].endswith('"'): + result.append(command[cursor:]) + break + segment = command[cursor:end] + result.append(segment if "$(" in segment or "`" in segment else '""') + cursor = end + continue + if char == "`": + end = cursor + 1 + while end < len(command): + if command[end] == "\\" and end + 1 < len(command): + end += 2 + continue + if command[end] == "`": + end += 1 + break + end += 1 + result.append(command[cursor:end]) + cursor = end + continue + result.append(char) + cursor += 1 + return "".join(result) + + +def _contains_nested_shell_scope(masked_opener: str) -> bool: + """Return whether a quote-masked opener contains nested executable syntax.""" + return any(marker in masked_opener for marker in ("$(", "`", "<(", ">(")) + + +def _parse_heredoc_operator(command: str, index: int): + """Parse one active ``<<`` redirection and return its shell delimiter. + + Returns ``(end_index, delimiter, strip_tabs, quoted)`` or ``None`` when + the token at ``index`` is not a well-formed heredoc opener (here-strings, + trailing ``<<`` at end of line, unterminated quote in the delimiter). + """ + if not command.startswith("<<", index) or command.startswith("<<<", index): + return None + + cursor = index + 2 + strip_tabs = False + if cursor < len(command) and command[cursor] == "-": + strip_tabs = True + cursor += 1 + while cursor < len(command) and command[cursor] in " \t": + cursor += 1 + if cursor >= len(command) or command[cursor] in "\r\n": + return None + + delimiter: list[str] = [] + quoted = False + while cursor < len(command): + char = command[cursor] + if char.isspace() or char in ";&|<>()": + break + if char == "\\": + if cursor + 1 >= len(command) or command[cursor + 1] in "\r\n": + return None + quoted = True + delimiter.append(command[cursor + 1]) + cursor += 2 + continue + if char in "'\"": + quoted = True + quote = char + cursor += 1 + while cursor < len(command) and command[cursor] != quote: + if quote == '"' and command[cursor] == "\\": + if cursor + 1 >= len(command): + return None + following = command[cursor + 1] + if following in {"$", "`", '"', "\\", "\n"}: + delimiter.append(following) + cursor += 2 + continue + # In double quotes, backslash is literal before all other + # characters. Preserve it so the terminator stays exact. + delimiter.append("\\") + cursor += 1 + continue + if command[cursor] in "\r\n": + return None + delimiter.append(command[cursor]) + cursor += 1 + if cursor >= len(command): + return None + cursor += 1 + continue + delimiter.append(char) + cursor += 1 + + if not delimiter and not quoted: + return None + return cursor, "".join(delimiter), strip_tabs, quoted + + +def _scan_heredoc_command_unit(command: str, start: int): + """Scan one logical shell command, ignoring markers in quotes/comments. + + Returns ``(end_index, heredoc_specs, unknown_operator, has_list_operator)`` + where each spec is ``(delimiter, strip_tabs, quoted)``. + ``unknown_operator`` reports a ``<<`` token that could not be parsed + soundly — the caller must fail closed and leave the command unmodified. + ``has_list_operator`` reports an unquoted ``;``, ``|`` or ``&`` on the + opener — the unit composes multiple shell commands. + """ + cursor = start + quote = None + comment = False + specs = [] + unknown_operator = False + has_list_operator = False + + while cursor < len(command): + char = command[cursor] + if comment: + if char == "\n": + return cursor, specs, unknown_operator, has_list_operator + cursor += 1 + continue + + if quote is not None: + if quote in {'"', "`"} and char == "\\" and cursor + 1 < len(command): + cursor += 2 + continue + if char == quote: + quote = None + cursor += 1 + continue + + if char == "\\" and cursor + 1 < len(command): + # Includes line continuations: the logical command keeps going on + # the next physical line, so a heredoc opener there still belongs + # to this unit. + cursor += 2 + continue + if char in "'\"`": + quote = char + cursor += 1 + continue + if char == "#": + previous = command[cursor - 1] if cursor > start else "" + if cursor == start or previous.isspace() or previous in ";&|()": + comment = True + cursor += 1 + continue + if char == "\n": + return cursor, specs, unknown_operator, has_list_operator + if command.startswith("<<<", cursor): + cursor += 3 + continue + if command.startswith("<<", cursor): + parsed = _parse_heredoc_operator(command, cursor) + if parsed is None: + unknown_operator = True + cursor += 2 + continue + cursor, delimiter, strip_tabs, quoted = parsed + specs.append((delimiter, strip_tabs, quoted)) + continue + if char in ";|&": + has_list_operator = True + cursor += 1 + + return len(command), specs, unknown_operator, has_list_operator + + +def _find_heredoc_close( + command: str, + body_start: int, + delimiter: str, + strip_tabs: bool, +) -> int | None: + """Return the position after an exact shell heredoc terminator line.""" + cursor = body_start + while True: + newline = command.find("\n", cursor) + if newline == -1: + line = command[cursor:] + after = len(command) + else: + line = command[cursor:newline] + after = newline + 1 + if line.endswith("\r"): + line = line[:-1] + candidate = line.lstrip("\t") if strip_tabs else line + if candidate == delimiter: + return after + if newline == -1: + return None + cursor = after + + +def strip_inert_heredoc_bodies(command: str) -> str: + """Mask heredoc bodies that are provably inert data; keep the rest. + + See the module docstring for the qualification rules. Masked bodies are + replaced with an equivalent number of newlines so positions of the + surrounding real command text keep their line structure. On ANY ambiguity + (unparseable ``<<`` token, unterminated heredoc, unquoted delimiter, + compound opener, nested shell scope, unknown consumer) the original + command is returned unchanged — a scanner false positive is acceptable, + hiding real shell syntax is not. + """ + ranges: list[tuple[int, int]] = [] + command_start = 0 + + # Fast path: no '<<' anywhere means no heredoc can exist — skip the state + # machine entirely. This function runs on every terminal tool call. + if "<<" not in command: + return command + + while command_start < len(command): + command_end, specs, unknown_operator, has_list_operator = ( + _scan_heredoc_command_unit(command, command_start) + ) + if unknown_operator: + return command + if not specs: + if command_end >= len(command): + break + command_start = command_end + 1 + continue + if command_end >= len(command): + # Opener with no following body line: nothing to mask, and the + # heredoc is unterminated — leave everything visible. + return command + + body_cursor = command_end + 1 + body_ranges: list[tuple[int, int]] = [] + unterminated = False + for delimiter, strip_tabs, _quoted in specs: + close_end = _find_heredoc_close( + command, + body_cursor, + delimiter, + strip_tabs, + ) + if close_end is None: + unterminated = True + break + body_ranges.append((body_cursor, close_end)) + body_cursor = close_end + if unterminated: + return command + + if all(quoted for _delimiter, _strip_tabs, quoted in specs) and not has_list_operator: + masked_opener = _mask_simple_quotes(command[command_start:command_end]) + if not _contains_nested_shell_scope(masked_opener) and ( + _INERT_HEREDOC_CONSUMER_RE.search(masked_opener) + ): + ranges.extend(body_ranges) + command_start = body_cursor + + result = command + for start, end in reversed(ranges): + replacement = "\n" * result[start:end].count("\n") + result = result[:start] + replacement + result[end:] + return result diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index 222e069318..0c1f742f39 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -68,6 +68,7 @@ def _redact_terminal_error_text(value: Any) -> str: # --------------------------------------------------------------------------- from tools.interrupt import is_interrupted, _interrupt_event # noqa: F401 — re-exported from tools.registry import tool_error +from tools.shell_heredoc import strip_inert_heredoc_bodies # display_hermes_home imported lazily at call site (stale-module safety during hermes update) @@ -2380,50 +2381,17 @@ def _strip_quotes(command: str) -> str: This prevents false positives when keywords like 'nohup' or 'setsid' appear in commit messages, Python -c code, echo arguments, or PR body text. - Also strips backtick-quoted content and heredoc body text. + Also strips backtick-quoted content and provably-inert heredoc body text. """ - # Remove heredoc bodies FIRST (before quote-stripping — a heredoc delimiter - # may be quoted, e.g. <<'EOF', and the body commonly contains characters like - # '&' that are literal payload, not shell operators). Matches < str: - heredoc_re = re.compile(r"<<-?\s*(['\"]?)([A-Za-z_][A-Za-z0-9_]*)\1") - out = text - # Iterate because a command may contain multiple heredocs. - while True: - m = heredoc_re.search(out) - if not m: - break - delim = m.group(2) - # The heredoc body starts on the NEXT line — anything between the - # `< file.txt`, - # additional args, or even a trailing `&`) is still real command - # text and must be preserved. Find the newline that ends the - # opener line. - nl = out.find("\n", m.end()) - if nl == -1: - # No body at all (opener with no following line) — nothing to - # strip; blank the delimiter so we don't loop, keep the rest. - out = out[: m.start()] + "<<" + out[m.end() :] - break - body_start = nl # keep the newline; body is what follows it - # Closing delimiter: on its own line, optional leading tabs for <<-. - close_re = re.compile(r"\n[ \t]*" + re.escape(delim) + r"[ \t]*(?=\n|$)") - cm = close_re.search(out, body_start) - # Blank the `<