From 12dd7d8f10822b61c3bb73a58ffe88916d012ce0 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Thu, 6 Aug 2026 22:47:04 -0700 Subject: [PATCH] Port from can1357/oh-my-pi#7553: allow quoted shell metacharacters in allowlist matching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit command_allowlist glob rules (e.g. 'cargo *') rejected any command whose quoted arguments contained shell metacharacters — a cargo benchmark regex filter like '^layer3/write/(a|b)$' disqualified the whole command even though those characters are literal to the shell. _has_allowlist_shell_operator is now quote-aware: - metacharacters inside single/double quotes or behind a backslash are treated as literal arguments; - $ and backtick inside DOUBLE quotes still disqualify (expansion is active there); - quoted/escaped control characters still disqualify when the command carries a -c/-e/--command/--eval-style option that hands the payload to another interpreter (sh -c '...', git -c alias.x='!...' x); - unterminated quotes disqualify (shape can't be reasoned about). Compound commands (unquoted ; & | < > backtick $( newline) are rejected exactly as before. hermes_cli/approvals_suggest.derive_glob picks up the same semantics via its existing import. --- .../tools/test_allowlist_quoted_metachars.py | 121 ++++++++++++++++++ tools/approval.py | 75 ++++++++++- 2 files changed, 193 insertions(+), 3 deletions(-) create mode 100644 tests/tools/test_allowlist_quoted_metachars.py diff --git a/tests/tools/test_allowlist_quoted_metachars.py b/tests/tools/test_allowlist_quoted_metachars.py new file mode 100644 index 0000000000000..c4332bf2f14fb --- /dev/null +++ b/tests/tools/test_allowlist_quoted_metachars.py @@ -0,0 +1,121 @@ +"""Tests for the quote-aware allowlist shell-operator check. + +Port of can1357/oh-my-pi#7553: `command_allowlist` glob rules (e.g. +``cargo *``) used to reject any command whose *quoted arguments* contained +shell metacharacters — a cargo benchmark regex filter like +``'^layer3/write/(a|b)$'`` disqualified the whole command even though the +metacharacters are literal to the shell. The matcher is now quote-aware, +while still rejecting genuinely compound commands and quoted payloads that +a ``-c``/``-e``-style option would hand to another interpreter. +""" + +import pytest + +from tools.approval import ( + _command_matches_permanent_allowlist, + _has_allowlist_shell_operator, +) + + +class TestHasAllowlistShellOperator: + # ------------------------------------------------------------------ + # Simple commands stay simple + # ------------------------------------------------------------------ + + def test_plain_command(self): + assert not _has_allowlist_shell_operator("git status") + + def test_quoted_metacharacters_are_literal(self): + # The motivating case: cargo bench regex filter (omp issue #7552). + cmd = ( + "cargo bench --manifest-path layers/layer3/Cargo.toml " + "--bench standardized_criterion -- " + "'^layer3/write/file-wal/batch-(10|1000|10000)$'" + ) + assert not _has_allowlist_shell_operator(cmd) + + def test_double_quoted_literal_metachars(self): + assert not _has_allowlist_shell_operator('grep -r "a|b;c" src') + + def test_escaped_metachar_is_literal(self): + assert not _has_allowlist_shell_operator("grep foo\\;bar file.txt") + + def test_unquoted_dollar_variable_is_simple(self): + # Historical behavior: only `$(` was compound, bare $VAR was not. + assert not _has_allowlist_shell_operator("echo $HOME") + + def test_unquoted_parens_alone_are_not_compound(self): + # Parens without $ were never matched by the old regex either. + assert not _has_allowlist_shell_operator("pytest -k (a and b)") + + # ------------------------------------------------------------------ + # Genuinely compound commands still rejected + # ------------------------------------------------------------------ + + @pytest.mark.parametrize("cmd", [ + "git status; rm -rf /tmp/x", + "git status && make", + "git status || make", + "cat foo | grep bar", + "echo hi > /etc/passwd", + "cat < seed", + "echo `rm x`", + "echo $(rm x)", + "git status\nrm x", + "git status & disown", + ]) + def test_unquoted_operators_compound(self, cmd): + assert _has_allowlist_shell_operator(cmd) + + def test_dollar_inside_double_quotes_is_active(self): + # Expansion still happens inside double quotes. + assert _has_allowlist_shell_operator('echo "$(rm x)"') + assert _has_allowlist_shell_operator('echo "`rm x`"') + assert _has_allowlist_shell_operator('echo "$HOME"') + + def test_unterminated_quote_is_compound(self): + assert _has_allowlist_shell_operator("echo 'unterminated") + + # ------------------------------------------------------------------ + # Reinterpreted-argument options: quoted payloads become executable + # ------------------------------------------------------------------ + + @pytest.mark.parametrize("cmd", [ + "sh -c 'rm -rf /tmp/x; echo done'", + 'bash -c "make | tee log"', + "git -c alias.x='!touch /tmp/pwn; printf ok' x", + 'git -c alias.x="!touch /tmp/pwn; printf ok" x', + "node --eval 'require(\"child_process\").exec(\"id\")>1'", + "perl -e 'system(\"id\");'", + ]) + def test_quoted_payload_with_interpreter_option(self, cmd): + assert _has_allowlist_shell_operator(cmd) + + def test_interpreter_option_without_quoted_metachars_ok(self): + # -c with a payload containing control chars (parens) is flagged... + assert _has_allowlist_shell_operator("python -c 'print(1)'") + # ...but a clean payload with no control characters at all is fine. + assert not _has_allowlist_shell_operator("python -c 'import sys'") + + +class TestAllowlistGlobWithQuotedArgs: + def test_cargo_glob_matches_quoted_regex_filter(self, monkeypatch): + import tools.approval as mod + monkeypatch.setattr(mod, "_permanent_approved", {"cargo *"}) + cmd = ( + "cargo bench --bench standardized_criterion -- " + "'^layer3/write/file-wal/batch-(10|1000|10000)$'" + ) + assert _command_matches_permanent_allowlist(cmd) + + def test_glob_still_refuses_compound(self, monkeypatch): + import tools.approval as mod + monkeypatch.setattr(mod, "_permanent_approved", {"cargo *"}) + assert not _command_matches_permanent_allowlist("cargo build && rm -rf /tmp/x") + + def test_glob_refuses_git_alias_payload(self, monkeypatch): + import tools.approval as mod + monkeypatch.setattr(mod, "_permanent_approved", {"git *"}) + assert not _command_matches_permanent_allowlist( + "git -c alias.x='!touch /tmp/pwn; printf ok' x" + ) diff --git a/tools/approval.py b/tools/approval.py index f7338cb65df66..5c846c934a8ef 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -2635,12 +2635,81 @@ def load_permanent(patterns: set): _permanent_approved.update(patterns) -_ALLOWLIST_SHELL_OPERATOR_RE = re.compile(r"(?:\n|&&|\|\||[;&|<>`]|\$\()") +# Shell control characters that make a command compound when they appear +# OUTSIDE quotes. Inside quotes they are literal to the outer shell — but +# they become executable again if an option like `-c`/`-e`/`--eval` (or a +# git `-c alias.x=!...`) hands the quoted argument to another interpreter, +# so quoted control chars only disqualify a command when such an option is +# present. Port of can1357/oh-my-pi#7553. +_SHELL_CONTROL_CHARS = frozenset("\n\r;&|<>`$()") +_REINTERPRETED_ARGUMENT_RE = re.compile( + r"(?:^|[ \t])(?:-[^-\s]*[ce]|--(?:command|eval))(?:[= \t]|$)" +) def _has_allowlist_shell_operator(command: str) -> bool: - """Return True when a command is too compound for the allowlist shortcut.""" - return bool(_ALLOWLIST_SHELL_OPERATOR_RE.search(command or "")) + """Return True when a command is too compound for the allowlist shortcut. + + Quote-aware: shell metacharacters inside single/double quotes or behind + a backslash are literal arguments (``cargo bench -- '^a(b|c)$'``), not + shell syntax, so they don't disqualify an otherwise-simple command from + matching a ``cargo *`` allowlist glob. Exceptions that still disqualify: + + - ``$`` or backtick inside DOUBLE quotes (expansion stays active there); + - any quoted/escaped control character when the command also carries a + ``-c``/``-e``/``--command``/``--eval``-style option that would hand + the quoted text to another interpreter (``sh -c '...'``, + ``git -c alias.x='!...' x``). + """ + command = command or "" + quote = None # None | "'" | '"' + has_reinterpretable = False + i = 0 + n = len(command) + while i < n: + ch = command[i] + if quote == "'": + if ch == "'": + quote = None + elif ch in _SHELL_CONTROL_CHARS: + has_reinterpretable = True + i += 1 + continue + if ch == "\\": + nxt = command[i + 1] if i + 1 < n else "" + if nxt in _SHELL_CONTROL_CHARS: + has_reinterpretable = True + i += 2 + continue + if quote == '"': + if ch == '"': + quote = None + elif ch in ("`", "$"): + # Expansion is active inside double quotes. + return True + elif ch in _SHELL_CONTROL_CHARS: + has_reinterpretable = True + i += 1 + continue + if ch in ("'", '"'): + quote = ch + i += 1 + continue + if ch == "$": + # Unquoted $ is only compound when it opens a substitution — + # matches the historical `\$\(` behavior ("$HOME" stays simple). + if i + 1 < n and command[i + 1] == "(": + return True + i += 1 + continue + if ch in _SHELL_CONTROL_CHARS and ch not in "()": + return True + i += 1 + continue + # An unterminated quote means we can't reason about the command shape. + if quote is not None: + return True + return has_reinterpretable and bool(_REINTERPRETED_ARGUMENT_RE.search(command)) def _command_matches_permanent_allowlist(command: str) -> bool: