fix(ci): restore Claude PR review under subprocess env scrubbing - #1539
Conversation
Claude Code Review has posted nothing since 2026-07-20 while still
burning $0.50-$1.00 per run. The workflow was unchanged; the regression
came in through the moving `anthropics/claude-code-action@v1` tag.
Because this workflow sets `allowed_non_write_users`, the action now
auto-enables `CLAUDE_CODE_SUBPROCESS_ENV_SCRUB=1` (its action.yml), which
strips GITHUB_TOKEN/GH_TOKEN from every subprocess Claude spawns. Our
prompt drove the whole review through `gh` — `gh pr diff` to read the
change and `gh pr comment` to publish it — so all of it failed
unauthenticated. Claude fell back to Read/Grep on the checkout, spent its
turns there, and had no way to publish. The run still reported success,
which is why this went unnoticed: the only visible symptoms were a benign
cache warning and the action's own "bypassing write permission check"
notice, neither of which is the cause.
Rather than opt out of the scrub (which would put the write-scoped token
back within reach of a prompt injection carried in an untrusted fork
diff), keep the hardening and route around `gh`:
- A workflow step stages the diff, changed-file list, and PR metadata
into `pr-context/` for Claude to Read.
- Inline findings keep using the `github_inline_comment` MCP server; its
token comes from the MCP server config, not the environment, so it
survives the scrub.
- Claude ends with the summary as its final message; a workflow step
publishes it from the action's `execution_file` output.
Bash is dropped from --allowedTools entirely: no token reaches the agent,
so no `gh` invocation there could have worked anyway.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe review workflow stages pull request data on disk, removes Claude’s Bash and network access, pins the Claude action, limits its tools to file inspection and inline comments, and posts its final structured review through a separate validated workflow step. ChangesClaude review workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant gh
participant pr_context
participant Claude
participant execution_file
participant PullRequest
GitHubActions->>gh: fetch pull request metadata, files, and diff
gh->>pr_context: write staged review inputs
GitHubActions->>Claude: provide staged context with restricted tools
Claude->>execution_file: write final review summary
GitHubActions->>execution_file: extract and validate result
GitHubActions->>PullRequest: post truncated Markdown summary
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Cloudflare PR preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/claude-code-review.yml:
- Around line 247-267: Update the summary-file handling around the jq extraction
and truncation logic to create transient files with mktemp under $RUNNER_TEMP,
rather than using checkout-relative summary.md or summary.trimmed paths. Ensure
all subsequent size checks, reads, truncation, and gh pr comment --body-file
usage reference the generated temporary file and avoid following symlinks from
the untrusted checkout.
- Around line 9-31: Update the Run Claude Code Review step’s
anthropics/claude-code-action uses reference from the mutable v1 tag to the
exact full commit SHA for the intended action version, preserving the existing
action configuration and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 09a7e330-af10-4e40-80f1-71e6f439d04e
📒 Files selected for processing (1)
.github/workflows/claude-code-review.yml
🔍 GitHub Pages PR preview
|
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
- Write the summary to a mktemp file under $RUNNER_TEMP instead of
`summary.md` in the workspace. The checkout is the untrusted PR head, so
a PR shipping `summary.md` as a symlink would have the redirection follow
it and clobber the target (or fail the step before the review posts).
- Pin `anthropics/claude-code-action` to commit be7b93b instead of the
mutable `v1` tag. This workflow depends on action internals that `v1` has
silently changed before — that drift is the bug this PR fixes — so a SHA
turns the next such change into a reviewable Dependabot PR. be7b93b is
what `v1` already resolved to, so behaviour is unchanged. Note that
`v1` is an annotated tag, so the pin is its dereferenced commit, not the
tag object SHA.
- Pass `github.repository` through `env:` in the two steps this PR adds
rather than expanding `${{ }}` inside the shell body, clearing the
template-injection warning zizmor raised on the summary step.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 108fb9ed89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - name: Post review summary | ||
| if: ${{ steps.claude-review.outputs.execution_file != '' }} | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
When a PR adds a .claude/settings.json hook, Claude Code can execute that attacker-controlled shell command while reviewing the checked-out head; project hooks are explicitly shell commands according to the Claude Code hooks documentation. Although subprocess scrubbing hides the current token, the hook can persist changes on the runner—for example, placing a malicious gh earlier in the next step's PATH—so this newly added step then supplies its write-scoped GH_TOKEN to attacker-controlled code. Post the captured result from a fresh job/runner, or otherwise prevent project hooks and runner mutation before introducing a token-bearing step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks — this is the right thing to worry about, but I believe it's already closed, and by the action itself rather than by anything in this PR. Leaving the thread open for @giswqs to weigh in since it's a P1 security call.
1. The PR's .claude/ never reaches Claude. restoreConfigFromBase (src/github/operations/restore-config.ts:88) exists for exactly this vector. Its doc comment names it:
The CLI's non-interactive mode trusts cwd: it reads
.mcp.json,.claude/settings.json, and.claude/settings.local.jsonfrom the working directory and acts on them before any tool-permission gating — executing hooks (including SessionStart), setting env vars (NODE_OPTIONS, LD_PRELOAD, PATH), running apiKeyHelper/awsAuthRefresh shell commands […] When this action checks out a PR head, all of these are attacker-controlled.
It snapshots the PR's copies to .claude-pr/ (explicitly not executed), deletes them, then restores the base versions. This is observable in our own run logs:
Restoring .claude, .mcp.json, .claude.json, .gitmodules, .ripgreprc, CLAUDE.md, CLAUDE.local.md, .husky from origin/main (PR head is untrusted)
Preserved PR's sensitive paths -> .claude-pr/ for review agents (not executed)
Worth noting for this repo specifically: git ls-files .claude .mcp.json .claude.json .husky returns 0. Nothing is tracked on main, so "paths absent on base are deleted" means a PR-added .claude/settings.json is removed outright, not merely overwritten.
2. PATH and the loader env are sanitised before my step runs. action.yml:383 — Re-prepend system bin dirs to PATH, gated on always() && inputs.allowed_non_write_users != '', which is precisely our configuration. It re-prepends /usr/bin and /bin to $GITHUB_PATH and clears BASH_ENV, LD_PRELOAD, LD_LIBRARY_PATH, NODE_OPTIONS, and the DYLD_* set. That is the specific "malicious gh earlier in PATH" defence, and it runs inside the composite action, i.e. before the Post review summary step this PR adds.
3. The agent has no write primitive here. --allowedTools is Read,Grep,Glob,mcp__github_inline_comment__create_inline_comment — no Bash, no Write, no Edit. Claude cannot mutate the runner directly; hooks were the only execution route, and (1) closes it.
Residual risk, stated honestly: this rests on the action's SENSITIVE_PATHS list staying complete as Claude Code evolves. If a future version reads settings from a path not on that list, the gap reopens silently. That's an argument for the SHA pin added in 7242641 — the pin makes such a change arrive as a reviewable Dependabot PR instead of drifting in.
So I'd rather not add a separate posting job: it's real complexity (artifact round-trip plus a second job) against a vector with three independent mitigations. Happy to implement it if you'd prefer belt-and-braces — say the word and I'll do it in a follow-up.
The problem
Claude Code Review has posted nothing since 2026-07-20 — on fork PRs and on our own — while still spending $0.50–$1.00 per run. Runs report success, so this went unnoticed.
The two messages surfaced in the run annotations are both red herrings: the cache-write warning is benign, and
⚠️ SECURITY WARNING: Bypassing write permission checkis just the action noting it honored ourallowed_non_write_userssetting.Root cause
This workflow hasn't changed since 2026-07-20. The regression arrived through the moving
anthropics/claude-code-action@v1tag (now on Claude Code 2.1.220).Because we set
allowed_non_write_users: "*", the action auto-enables subprocess env scrubbing —action.yml:290:Per the action's
docs/security.md, that does a "best-effort scrub of Anthropic, cloud, and GitHub Actions secrets from subprocess environments."Our prompt drove the entire review through
gh. With no token in Claude's subprocess environment,gh pr diff/gh pr view/gh pr commentall failed unauthenticated. Claude fell back toRead/Grepon the checkout (hence the turns and the cost), groped for non-allowlisted alternatives (the permission denials), and had no way to publish anything.The fix
Opting out via
CLAUDE_CODE_SUBPROCESS_ENV_SCRUB: 0would be two lines, but it puts the write-scopedGITHUB_TOKENback within reach of a prompt injection carried in an untrusted fork diff — I verified the allowlist permits command substitution, sogh pr comment N --body "$(printenv GITHUB_TOKEN)"would go through. Not worth it on a public repo.Instead, keep the hardening and route around
gh:pr-context/{diff.patch,changed-files.txt,metadata.json}; Claude reads them withRead/Grep. Runs after checkout and clears the directory first, so a PR can't pre-seed its ownpr-context/. Diff is capped at 1.5 MB with an explicit truncation marker.github_inline_commentMCP server. Its token is injected through the MCP server config rather than inherited from the environment, so it is unaffected by the scrub.execution_fileoutput and posts it withgh pr comment --body-file.Bashis dropped from--allowedToolsentirely — no token reaches the agent, so noghinvocation there could have worked regardless.track_progress: trueis not an option here:src/modes/detector.ts:84restricts it topull_request/issues/comment events, and this workflow triggers onpull_request_target.Verification
execution_fileholds the unredacted messages —writeExecutionFile(messages)serializes the raw array; only console output goes throughsanitizeSdkOutput. The console "full output hidden for security" line is why the denials were invisible in the logs.jqagainst success,is_error: true, and missing-result-turn fixtures; the last two yield empty and hit the skip guard.metadata.json.bash -non all threerun:blocks; YAML parses;pre-commit run --filesclean.Not verifiable before merge:
pull_request_targetworkflows run from the base branch, so the end-to-end path only exercises once this lands. If the summary step misfires it fails loudly (::warning::+ captured output) rather than posting a broken comment.Summary by CodeRabbit