Harden claude-pr-action.yml#584
Open
Micky774 wants to merge 2 commits into
Open
Conversation
Triaged from a third-party security review of the workflow. This commit addresses the must-fix bundle (M1+M2+M3 from that triage); deferred items (show_full_output, install-script pinning, base-ref validation, job timeouts) will land in a follow-up PR. - Block fork PRs from non-collaborators before any PR code is staged. pull_request_target gives this job write-scoped GITHUB_TOKEN and the CLAUDE_CODE_OAUTH_TOKEN secret. Without this gate, a fork PR can hijack both via e.g. a .gitattributes textconv driver invoked by Claude's `git diff` calls. The new step runs first and exits 1 unless the head repo is the same as the base repo or the PR author has write/maintain/admin permission on the base. - Run the Claude warmup in mktemp -d instead of $GITHUB_WORKSPACE. Claude Code auto-discovers CLAUDE.md and .claude/ from CWD; with the PR tree checked out and CLAUDE_CODE_OAUTH_TOKEN in env, an attacker workspace CLAUDE.md could prompt-inject the warmup into echoing the token into workflow logs. - Pin third-party actions to commit SHAs: * actions/checkout@v4 -> @34e114876b0b... (v4.3.1) * actions/upload-artifact@v4 -> @ea165f8d65b6... (v4.6.2) * anthropics/claude-code-action@v1 -> @dde2242db6af... (v1.0.120) - Replace blocklist allowedTools with a strict allowlist for both the review and summary steps. The previous `Bash(gh *)` + denylist allowed e.g. `gh api --method DELETE repos/...` because the denylist only covered specific subcommands. New allowlist only matches read-shaped invocations (`gh api repos/*`, `git diff *`, etc.); summary additionally permits `gh api --method PATCH repos/*` so it can update prior walkthrough comments. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After landing the strict allowlist in the previous commit, I downloaded
the claude-execution-output.json artifacts from the 10 most recent
successful runs of this workflow and extracted every tool_use event.
Several Bash patterns that the model actually invokes were missing
from the new allowlist and would have caused regressions.
Verified-missing patterns added to the review job:
Bash(git fetch *) (shallow-clone deepening)
Bash(git status), Bash(git status *)
Bash(git ls-remote *) (used to verify remote refs)
Bash(git rev-list *)
Bash(git merge-base *)
Bash(echo *) (chained as `git log; echo ---; git diff`)
Bash(ls), Bash(ls *) (workspace inspection / skill discovery)
Verified-missing patterns added to the summary job (superset of above):
Bash(cat *) (cat > /tmp/walkthrough.md <<EOF heredoc)
Bash(gh api -X POST repos/*) and --method POST equivalents
(creates the top-level walkthrough comment;
past runs use this OR `gh pr comment`)
Bash(gh api -X PATCH repos/*) (alias for --method PATCH already allowed)
Comment block is also updated to record two pieces of mechanism
discovered while validating: (a) --allowedTools enforces only Bash(...)
and MCP tool patterns; built-in tools like Read/TodoWrite/Write bypass
it entirely, and (b) the matcher splits chained commands on `;`/`&&`/
`|` and matches each segment, which is why `Bash(git diff *)` covers
`BASE=…; git diff …` but `Bash(echo *)` is still needed for the
`echo "---"` separator the model uses between git steps.
Patterns were derived from runs 25510721849, 25537041205, 25537041220,
25696530024 (review and summary across PRs 543 and 547). Tools
referenced but never invoked in the data — `gh repo *`, `gh release *`,
`git push *`, `git reset *`, etc. — remain off the allowlist by
omission.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Micky774
requested review from
ipanfilo,
wangye805 and
wenchenvincent
as code owners
| # if it's missing/stale, the workflow will fail fast on checkout, which | ||
| # is the right behavior for an unreviewable PR. | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 |
Contributor
There was a problem hiding this comment.
Perhaps mention here why the pinning is done.
Contributor
Author
There was a problem hiding this comment.
Pinning is pretty standard practice for CI security just to avoid accidentally using a new and compromised version -- this isn't pinned to that exact version for a particular version, other than it is "known to work". I think we can probably avoid an extra comment on it, but I'm open to it if you still think it would be useful to folks. What do you think?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Triaged from a third-party security review of the workflow. This commit addresses the must-fix issues presented in the review.
Fixes # (issue)
Type of change
Changes
Please list the changes introduced in this PR:
Block fork PRs from non-collaborators before any PR code is staged. pull_request_target gives this job write-scoped GITHUB_TOKEN and the CLAUDE_CODE_OAUTH_TOKEN secret. Without this gate, a fork PR can hijack both via e.g. a .gitattributes textconv driver invoked by Claude's
git diffcalls. The new step runs first and exits 1 unless the head repo is the same as the base repo or the PR author has write/maintain/admin permission on the base.Run the Claude warmup in mktemp -d instead of $GITHUB_WORKSPACE. Claude Code auto-discovers CLAUDE.md and .claude/ from CWD; with the PR tree checked out and CLAUDE_CODE_OAUTH_TOKEN in env, an attacker workspace CLAUDE.md could prompt-inject the warmup into echoing the token into workflow logs.
Pin third-party actions to commit SHAs:
Replace blocklist allowedTools with a strict allowlist for both the review and summary steps. The previous
Bash(gh *)+ denylist allowed e.g.gh api --method DELETE repos/...because the denylist only covered specific subcommands. New allowlist only matches read-shaped invocations (gh api repos/*,git diff *, etc.); summary additionally permitsgh api --method PATCH repos/*so it can update prior walkthrough comments.Checklist: