fix(reviewer): close forbidden-git detection gaps via allowlist (#243)

Replace blocklist-based git mutation detection with fail-closed allowlist
semantics: any git command not matching the readonly set is forbidden.
Catches checkout HEAD --, checkout ., git switch variants, and uncommon
stash subcommands that bypassed the old regex.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
2026-07-06 11:42:14 -04:00
co-authored by Claude Opus 4.8
parent bab803ff3d
commit ec8f6abf5b
2 changed files with 36 additions and 28 deletions
+19 -28
View File
@@ -4,6 +4,11 @@ Reviewer sessions must never stash, reset, or otherwise manipulate unrelated
local changes from another session. When the active worktree has dirty tracked
files outside the PR scope, the workflow must stop or switch to a disposable
scratch worktree (``scripts/worktree-review``).
Git command policy (#243): reviewers use an allowlist, not a blocklist.
Any ``git`` invocation that does not match ``_READONLY_REVIEWER_GIT`` is
forbidden — including ``checkout HEAD --``, ``checkout .``, ``switch``,
and uncommon ``stash`` subcommands that older blocklists missed.
"""
from __future__ import annotations
@@ -11,25 +16,7 @@ from __future__ import annotations
import re
import shlex
# Subcommands that mutate unrelated local state — forbidden for reviewers.
_FORBIDDEN_REVIEWER_GIT = re.compile(
r"\bgit\b(?:\s+(?:-C\s+\S+\s+)?)?"
r"(?:stash(?:\s+(?:push|pop|drop|apply|clear|list))?|"
r"checkout\s+--|"
r"restore\s+|"
r"reset(?:\s+(?:--hard|--soft|--mixed|--merge))?|"
r"clean(?:\s+(?:-f|-fd|-fdx|-x|-d|-n))*|"
r"cherry-pick|"
r"rebase|"
r"merge|"
r"commit(?:\s+(?:--amend|-a|-am))?|"
r"push(?:\s+(?:--force|--force-with-lease))?|"
r"branch\s+-[dD]|"
r"worktree\s+remove)",
re.IGNORECASE,
)
# Read-only git operations reviewers may use for validation.
# Read-only git operations reviewers may use for validation (#243 allowlist).
_READONLY_REVIEWER_GIT = re.compile(
r"\bgit\b(?:\s+(?:-C\s+\S+\s+)?)?"
r"(?:fetch|status|diff|log|show|rev-parse|branch(?:\s+--show-current)?|"
@@ -37,6 +24,8 @@ _READONLY_REVIEWER_GIT = re.compile(
re.IGNORECASE,
)
_GIT_INVOCATION = re.compile(r"\bgit\b", re.IGNORECASE)
def parse_dirty_tracked_files(porcelain: str) -> list[str]:
"""Return tracked paths with local modifications from ``git status --porcelain``.
@@ -73,24 +62,26 @@ def files_outside_pr_scope(
return [path for path in dirty if path not in scope]
def is_forbidden_reviewer_git_command(command: str) -> bool:
"""True when a shell command would mutate unrelated local/remote git state."""
text = (command or "").strip()
if not text:
return False
return bool(_FORBIDDEN_REVIEWER_GIT.search(text))
def _is_git_command(command: str) -> bool:
return bool(_GIT_INVOCATION.search((command or "").strip()))
def is_readonly_reviewer_git_command(command: str) -> bool:
"""True when the command is an explicitly allowed read-only git operation."""
text = (command or "").strip()
if not text:
return False
if is_forbidden_reviewer_git_command(text):
if not text or not _is_git_command(text):
return False
return bool(_READONLY_REVIEWER_GIT.search(text))
def is_forbidden_reviewer_git_command(command: str) -> bool:
"""True when a git command is not on the reviewer readonly allowlist."""
text = (command or "").strip()
if not text or not _is_git_command(text):
return False
return not is_readonly_reviewer_git_command(text)
def assess_reviewer_git_command_log(commands: list[str] | None) -> dict:
"""Fail closed when reviewer shell history includes forbidden git mutations."""
forbidden = [
+17
View File
@@ -65,6 +65,23 @@ class TestForbiddenGitCommands(unittest.TestCase):
self.assertTrue(is_forbidden_reviewer_git_command("git reset --hard"))
self.assertTrue(is_forbidden_reviewer_git_command("git clean -fd"))
def test_blocks_checkout_restore_and_switch_bypasses(self):
"""Issue #243: blocklist gaps closed via readonly allowlist model."""
blocked = (
"git checkout HEAD -- review_proofs.py",
"git checkout prgs/master -- review_proofs.py",
"git checkout .",
"git switch -",
"git switch -C feat/other-branch",
"git switch master",
"git stash store",
"git stash branch wip-stash",
)
for cmd in blocked:
with self.subTest(cmd=cmd):
self.assertTrue(is_forbidden_reviewer_git_command(cmd))
self.assertFalse(is_readonly_reviewer_git_command(cmd))
def test_allows_readonly_commands(self):
for cmd in (
"git fetch prgs master",