Unverified 提交 42531151 authored 作者: Will Chen's avatar Will Chen 提交者: GitHub

Relax shell injection checks in permission hooks (#2653)

## Summary - Add `$(cat ...)` as a safe command substitution pattern in the gh permission hook, allowing commands like `gh api graphql -f query="$(cat /tmp/query.graphql)" > /tmp/output.json` to passthrough instead of being blocked - Add safe pipe and redirect handling to the python permission hook, allowing pytest commands with `2>&1 | tail` output formatting patterns - Move `| cat` from blocked to allowed in python hook tests since `cat` is a safe read-only command #skip-bugbot ## Test plan - [x] All gh permission hook tests pass (572 good commands, 406 bad commands) - [x] All python permission hook tests pass (good, bad, passthrough, security-blocked) - [x] npm test passes (803 tests) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/dyad-sh/dyad/pull/2653" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end --> <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Relaxed shell injection checks in GH and Python permission hooks to allow common read-only patterns like $(cat ...), pipes to safe tools, and simple redirects. This reduces false blocks while keeping unsafe substitutions and pipelines guarded. - **Bug Fixes** - GH hook: treat $(cat ...) as a safe command substitution and neutralize it before checks. - Python hook: allow pipes to common text tools (e.g., tail, grep, cat) and redirects like 2>&1 and >/dev/null. - Tests: move python `| cat` to allowed; add cases for gh `$(cat ...)` and pytest `2>&1 | tail`. <sup>Written for commit b1695c6e09bdc58288197ae2ab79745947fcca59. Summary will update on new commits.</sup> <!-- End of auto-generated description by cubic. --> Co-authored-by: 's avatarClaude Opus 4.6 <noreply@anthropic.com>
上级 a852505d
...@@ -166,6 +166,14 @@ SAFE_GH_SUBCOMMAND_PATTERN = re.compile( ...@@ -166,6 +166,14 @@ SAFE_GH_SUBCOMMAND_PATTERN = re.compile(
r'[ \t]+[^)$`;&|<>\n\r]*\)' r'[ \t]+[^)$`;&|<>\n\r]*\)'
) )
# Safe $(cat ...) pattern - commonly used to load file contents into arguments
# e.g., gh api graphql -f query="$(cat /tmp/query.graphql)"
# cat is a read-only command that just outputs file contents.
# The file path must not contain shell metacharacters to prevent nested injection.
SAFE_CAT_SUBCOMMAND_PATTERN = re.compile(
r'\$\(cat\s+[^)$`;&|<>\n\r]+\)'
)
def extract_gh_command(command: str) -> Optional[str]: def extract_gh_command(command: str) -> Optional[str]:
""" """
...@@ -280,6 +288,10 @@ def contains_shell_injection(cmd: str) -> bool: ...@@ -280,6 +288,10 @@ def contains_shell_injection(cmd: str) -> bool:
# This allows patterns like: gh api repos/$(gh repo view --json nameWithOwner -q .nameWithOwner)/... # This allows patterns like: gh api repos/$(gh repo view --json nameWithOwner -q .nameWithOwner)/...
cmd_to_check = SAFE_GH_SUBCOMMAND_PATTERN.sub('SAFE_GH_SUB', cmd_without_safe_doubles) cmd_to_check = SAFE_GH_SUBCOMMAND_PATTERN.sub('SAFE_GH_SUB', cmd_without_safe_doubles)
# Replace safe $(cat ...) subcommands with a placeholder before checking
# This allows patterns like: gh api graphql -f query="$(cat /tmp/query.graphql)"
cmd_to_check = SAFE_CAT_SUBCOMMAND_PATTERN.sub('SAFE_CAT_SUB', cmd_to_check)
# Replace safe pipe destinations with a placeholder before checking # Replace safe pipe destinations with a placeholder before checking
# This allows patterns like: gh api graphql ... | jq '...' # This allows patterns like: gh api graphql ... | jq '...'
cmd_to_check = SAFE_PIPE_PATTERN.sub(' SAFE_PIPE ', cmd_to_check) cmd_to_check = SAFE_PIPE_PATTERN.sub(' SAFE_PIPE ', cmd_to_check)
......
...@@ -65,6 +65,19 @@ SINGLE_QUOTED_PATTERN = re.compile(r"'[^']*'") ...@@ -65,6 +65,19 @@ SINGLE_QUOTED_PATTERN = re.compile(r"'[^']*'")
# Pattern to match double-quoted strings without command substitution # Pattern to match double-quoted strings without command substitution
SAFE_DOUBLE_QUOTED_PATTERN = re.compile(r'"[^"$`]*"') SAFE_DOUBLE_QUOTED_PATTERN = re.compile(r'"[^"$`]*"')
# Safe pipe destinations - common text-processing commands (same as gh hook)
SAFE_PIPE_PATTERN = re.compile(
r'\|\s*('
r'jq|head|tail|grep|egrep|fgrep|wc|sort|uniq|cut|tr'
r'|base64|cat|column|fmt|fold|paste'
r'|expand|unexpand|rev|tac|nl|od|xxd|hexdump|strings'
r'|md5sum|sha256sum|sha1sum|shasum|cksum'
r')\b'
)
# Safe redirect patterns (same as gh hook)
SAFE_REDIRECT_PATTERN = re.compile(r'\d*>&\d+|\d*>/dev/null')
def contains_shell_injection(cmd: str) -> bool: def contains_shell_injection(cmd: str) -> bool:
""" """
...@@ -75,9 +88,15 @@ def contains_shell_injection(cmd: str) -> bool: ...@@ -75,9 +88,15 @@ def contains_shell_injection(cmd: str) -> bool:
cmd_without_single_quotes = SINGLE_QUOTED_PATTERN.sub("''", cmd) cmd_without_single_quotes = SINGLE_QUOTED_PATTERN.sub("''", cmd)
# Strip double-quoted strings that don't contain $( or backticks # Strip double-quoted strings that don't contain $( or backticks
cmd_without_safe_doubles = SAFE_DOUBLE_QUOTED_PATTERN.sub('""', cmd_without_single_quotes) cmd_to_check = SAFE_DOUBLE_QUOTED_PATTERN.sub('""', cmd_without_single_quotes)
# Replace safe pipe destinations (tail, head, grep, etc.) before checking
cmd_to_check = SAFE_PIPE_PATTERN.sub(' SAFE_PIPE ', cmd_to_check)
# Replace safe redirect patterns (2>&1, >/dev/null) before checking
cmd_to_check = SAFE_REDIRECT_PATTERN.sub(' ', cmd_to_check)
return bool(SHELL_INJECTION_PATTERNS.search(cmd_without_safe_doubles)) return bool(SHELL_INJECTION_PATTERNS.search(cmd_to_check))
def is_python_command(cmd: str) -> bool: def is_python_command(cmd: str) -> bool:
......
...@@ -764,6 +764,16 @@ gh api repos/$(gh repo view --json nameWithOwner -q .nameWithOwner)/pulls/2524/c ...@@ -764,6 +764,16 @@ gh api repos/$(gh repo view --json nameWithOwner -q .nameWithOwner)/pulls/2524/c
# The test framework treats passthrough the same as allow for good_commands.txt. # The test framework treats passthrough the same as allow for good_commands.txt.
REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner) && gh api "repos/${REPO}/pulls/2524/comments" --paginate | jq -r '.[].body' 2>/dev/null | head -200 REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner) && gh api "repos/${REPO}/pulls/2524/comments" --paginate | jq -r '.[].body' 2>/dev/null | head -200
# =============================================================================
# SAFE $(cat ...) SUBCOMMAND PATTERNS (passthrough - cat is read-only)
# =============================================================================
# Loading GraphQL query from file into gh api graphql argument
gh api graphql -F owner=dyad-sh -F repo=dyad -F pr=2284 -f query="$(cat /tmp/pr_threads_query.graphql)" > /tmp/all_threads.json
# Note: $(cat ...) is recognized as safe (read-only command) and neutralized before
# shell injection checking. The > redirect doesn't match any injection pattern either
# (only >( process substitution does). This command passes as a passthrough.
# ============================================================================= # =============================================================================
# GH ISSUE COMMANDS WITH SHELL METACHARACTERS (allowed - common workflow patterns) # GH ISSUE COMMANDS WITH SHELL METACHARACTERS (allowed - common workflow patterns)
# ============================================================================= # =============================================================================
......
...@@ -96,3 +96,9 @@ python -W ignore -X dev .claude/script.py ...@@ -96,3 +96,9 @@ python -W ignore -X dev .claude/script.py
python -- .claude/script.py python -- .claude/script.py
python3 -- .claude/script.py python3 -- .claude/script.py
python -u -- .claude/script.py python -u -- .claude/script.py
# =============================================================================
# PIPES TO SAFE COMMANDS
# =============================================================================
python .claude/script.py | cat
...@@ -56,3 +56,7 @@ python3 -m pytest ./tests ...@@ -56,3 +56,7 @@ python3 -m pytest ./tests
# Pytest with options and paths # Pytest with options and paths
python -m pytest -v tests/ python -m pytest -v tests/
python3 -m pytest --tb=short tests/unit/ python3 -m pytest --tb=short tests/unit/
# Pytest with pipes and redirects (common for test output formatting)
python -m pytest .claude/hooks/tests/test_gh_permission_hook.py -v 2>&1
python -m pytest .claude/hooks/tests/test_gh_permission_hook.py -v 2>&1 | tail -60
...@@ -19,7 +19,7 @@ python .claude/script.py || malicious_command ...@@ -19,7 +19,7 @@ python .claude/script.py || malicious_command
python3 .claude/script.py || rm -rf / python3 .claude/script.py || rm -rf /
# Pipe to another command # Pipe to another command
python .claude/script.py | cat # Note: python .claude/script.py | cat moved to good_commands (cat is safe)
python3 .claude/script.py | sh python3 .claude/script.py | sh
# Background + another command # Background + another command
......
Markdown 格式
0%
您添加了 0 到此讨论。请谨慎行事。
请先完成此评论的编辑!
注册 或者 后发表评论