fix(approval): allow quoted shell metacharacters in allowlist matching (port of oh-my-pi#7553)

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"
+        )

tools/approval.py

@@ -2817,12 +2817,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: