mirror of
https://github.com/crewAIInc/crewAI.git
synced 2026-08-10 08:21:54 +00:00
fix(agents): close guard bypasses and false denies from review
- pip3.12 and python3.12 -m pip bypassed the direct-pip rule - git push -n is --dry-run, not skip-hooks, so it was denied wrongly - rm -rf docs/images without a trailing slash was not matched - path rules crossed shell separators, denying unrelated later segments - writes into docs/v*/ via redirection, cp, or sed -i were not covered - heredoc bodies were matched as commands, blocking commit messages that merely discuss a blocked command — found by the guard blocking this commit Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ASfWmW3RGy4qAQm6s8U9jH
This commit is contained in:
@@ -26,23 +26,35 @@ OVERRIDE_MARKER = "# policy-override:"
|
||||
#: (pattern, reason). Patterns match the raw Bash command string.
|
||||
BASH_RULES: tuple[tuple[str, str], ...] = (
|
||||
(
|
||||
r"(?:^|[;&|(\n])\s*(?:sudo\s+)?(?:pip3?|python3?\s+-m\s+pip)\s+"
|
||||
# Version-suffixed interpreters (pip3.12, python3.12 -m pip) bypass a bare
|
||||
# pip3? match. `uv pip` is unaffected: the anchor requires command position.
|
||||
r"(?:^|[;&|(\n])\s*(?:sudo\s+)?"
|
||||
r"(?:pip(?:[23](?:\.\d+)?)?|python(?:[23](?:\.\d+)?)?\s+-m\s+pip)\s+"
|
||||
r"(?:install|uninstall)\b",
|
||||
"CONTRIBUTING.md (Dependency Management): do not use pip directly. "
|
||||
"Use `uv add --package <pkg> <dep>`, `uv add --dev <dep>`, or `uv sync`.",
|
||||
),
|
||||
(
|
||||
r"\bgit\b[^\n;&|]*\b(?:commit|push)\b[^\n;&|]*(?:--no-verify\b|\s-n(?=\s|$))",
|
||||
# `-n` is only the skip-hooks flag for commit; on push it means --dry-run,
|
||||
# which is safe and must stay allowed.
|
||||
r"\bgit\b[^\n;&|]*\b(?:commit|push)\b[^\n;&|]*--no-verify\b"
|
||||
r"|\bgit\b[^\n;&|]*\bcommit\b[^\n;&|]*\s-n(?=\s|$)",
|
||||
"CONTRIBUTING.md (Commits): do not use --no-verify to skip hooks. "
|
||||
"Fix what pre-commit reports instead.",
|
||||
),
|
||||
(
|
||||
r"\b(?:rm|mv)\b[^\n]*\bdocs/images/",
|
||||
# Stops at shell separators so a later segment merely naming the path does
|
||||
# not trigger, and matches the directory with or without a trailing slash.
|
||||
r"\b(?:rm|mv)\b[^\n;&|]*\bdocs/images(?![\w.-])",
|
||||
"AGENTS.md (Changing Docs, rule 3): do not delete or rename files under "
|
||||
"docs/images/ — frozen doc snapshots still reference them.",
|
||||
),
|
||||
(
|
||||
r"\b(?:rm|mv|tee)\b[^\n]*\bdocs/v[0-9]",
|
||||
# Write verbs and output redirection. Read-only access (cat, grep, less)
|
||||
# is deliberately allowed. `cp` is matched in either direction: failing
|
||||
# closed on a copy out of the tree is cheaper than missing a copy into it.
|
||||
r"\b(?:rm|mv|cp|tee|sed\s+-i)\b[^\n;&|]*\bdocs/v[0-9]"
|
||||
r"|>>?\s*[^\n;&|]*\bdocs/v[0-9]",
|
||||
"AGENTS.md (Changing Docs, rule 2): docs/v*/ are frozen release snapshots "
|
||||
"managed by devtools. Edit docs/edge/en/ instead.",
|
||||
),
|
||||
@@ -74,12 +86,30 @@ def deny(reason: str) -> None:
|
||||
)
|
||||
|
||||
|
||||
def strip_heredocs(command: str) -> str:
|
||||
"""Replace heredoc bodies with a placeholder.
|
||||
|
||||
A heredoc body is data — a commit message, a file being written, a PR body —
|
||||
not a command. Matching rules against it produces false denials, such as
|
||||
blocking a commit whose message merely discusses `rm -rf docs/images`. The
|
||||
redirection and the delimiter stay on the command line, so a heredoc that
|
||||
genuinely writes into a protected path is still caught.
|
||||
"""
|
||||
return re.sub(
|
||||
r"(<<-?\s*[\"']?(\w+)[\"']?).*?^\s*\2\b",
|
||||
r"\1 HEREDOC_BODY",
|
||||
command,
|
||||
flags=re.DOTALL | re.MULTILINE,
|
||||
)
|
||||
|
||||
|
||||
def bash_violation(command: str) -> str | None:
|
||||
"""Return the reason this command is blocked, or None if it is allowed."""
|
||||
if OVERRIDE_MARKER in command:
|
||||
return None
|
||||
inspectable = strip_heredocs(command)
|
||||
for pattern, reason in BASH_RULES:
|
||||
if re.search(pattern, command):
|
||||
if re.search(pattern, inspectable):
|
||||
return reason
|
||||
return None
|
||||
|
||||
|
||||
@@ -46,6 +46,19 @@ BLOCKED_COMMANDS: list[tuple[str, str]] = [
|
||||
("rm docs/images/flow.png", "docs/images/"),
|
||||
("mv docs/images/a.png docs/images/b.png", "docs/images/"),
|
||||
("rm -rf docs/v1.15.0", "frozen release snapshots"),
|
||||
# Version-suffixed interpreters must not bypass the pip rule.
|
||||
("pip3.12 install ruff", "do not use pip directly"),
|
||||
("pip2 install ruff", "do not use pip directly"),
|
||||
("python3.12 -m pip install ruff", "do not use pip directly"),
|
||||
# The protected directory named without a trailing slash.
|
||||
("rm -rf docs/images", "docs/images/"),
|
||||
("mv docs/images docs/img", "docs/images/"),
|
||||
# Writes into a frozen snapshot by redirection or copy, not just rm/mv.
|
||||
("echo x > docs/v1.15.0/index.mdx", "frozen release snapshots"),
|
||||
("cat tmp.mdx >> docs/v1.15.0/index.mdx", "frozen release snapshots"),
|
||||
("cp new.mdx docs/v1.15.0/index.mdx", "frozen release snapshots"),
|
||||
("sed -i 's/a/b/' docs/v1.15.0/index.mdx", "frozen release snapshots"),
|
||||
("tee docs/v1.15.0/index.mdx < new.mdx", "frozen release snapshots"),
|
||||
]
|
||||
|
||||
ALLOWED_COMMANDS: list[str] = [
|
||||
@@ -63,7 +76,19 @@ ALLOWED_COMMANDS: list[str] = [
|
||||
"uv run pytest -n auto lib/crewai/tests",
|
||||
"uv run pip-audit --skip-editable --ignore-vuln PYSEC-2024-277",
|
||||
"rm docs/edge/en/scratch.mdx",
|
||||
# Read-only access to a frozen snapshot is allowed; only writes are blocked.
|
||||
"cat docs/v1.15.0/index.mdx",
|
||||
"grep -rn 'agents' docs/v1.15.0/ > /tmp/hits.txt",
|
||||
"less docs/v1.15.0/index.mdx",
|
||||
"ls docs/images/",
|
||||
# `-n` on push is --dry-run, not skip-hooks.
|
||||
"git push -n origin main",
|
||||
"git push --dry-run origin main",
|
||||
# A protected path named in a later, unrelated command segment.
|
||||
"rm /tmp/scratch.txt && ls docs/images/",
|
||||
"rm /tmp/scratch.txt && cat docs/v1.15.0/index.mdx",
|
||||
# Paths that merely start with the same prefix.
|
||||
"rm docs/imagesets/old.png",
|
||||
]
|
||||
|
||||
|
||||
@@ -88,6 +113,36 @@ def test_blocked_reasons_cite_a_committed_document() -> None:
|
||||
assert "CONTRIBUTING.md" in reason or "AGENTS.md" in reason
|
||||
|
||||
|
||||
HEREDOC_COMMIT = """git commit -m "$(cat <<'EOF'
|
||||
fix(agents): close guard bypasses found in review
|
||||
|
||||
- rm -rf docs/images without a trailing slash was not matched
|
||||
- pip install was reachable via pip3.12
|
||||
EOF
|
||||
)"
|
||||
"""
|
||||
|
||||
|
||||
def test_heredoc_bodies_are_data_not_commands() -> None:
|
||||
"""A commit message discussing a blocked command must not itself be blocked."""
|
||||
assert guard.bash_violation(HEREDOC_COMMIT) is None
|
||||
|
||||
|
||||
def test_a_heredoc_writing_into_a_frozen_snapshot_is_still_blocked() -> None:
|
||||
"""Stripping the body must not hide a redirection on the command line."""
|
||||
command = "cat > docs/v1.15.0/index.mdx <<'EOF'\nsome new content\nEOF\n"
|
||||
reason = guard.bash_violation(command)
|
||||
assert reason is not None
|
||||
assert "frozen release snapshots" in reason
|
||||
|
||||
|
||||
def test_a_command_after_a_heredoc_is_still_inspected() -> None:
|
||||
command = "cat <<'EOF' > notes.txt\njust notes\nEOF\npip install ruff\n"
|
||||
reason = guard.bash_violation(command)
|
||||
assert reason is not None
|
||||
assert "do not use pip directly" in reason
|
||||
|
||||
|
||||
def test_override_marker_allows_a_stated_exception() -> None:
|
||||
command = "pip install vendored.whl # policy-override: offline wheel, no index"
|
||||
assert guard.bash_violation(command) is None
|
||||
|
||||
Reference in New Issue
Block a user