claude-code-review: the model runs in the same job as the writable token, so the reviewer deny list cannot be a security boundary

[d-morrison]

Raised by a cross-vendor (codex) review of #578, as its most substantial finding. Filing rather than fixing in that PR, because the fix is architectural and #578's own scope is the allowlist.

The gap

claude-code-review.yml runs the model and posts its output in the same job, so the model executes with a GITHUB_TOKEN carrying pull-requests: write and issues: write.

run-claude-review-attempt guards that with a disallowedTools list of command prefixes. Those guards cannot be a security boundary, for a reason that is structural rather than a matter of completeness:

• A prefix rule matches the command it is given. A general shell can reach a denied command by wrapping it, and Write(//tmp/**) scopes the Write tool while a redirect or sed -i writes wherever the runner account can.
• This is not new with fix(review): grant the reviewer a general shell and scratch writes (#566, #572, #564) #578, which is why it is a separate issue rather than a blocker on it. Bash(python3:*) has been granted for a long time, and python3 -c "subprocess.run(['gh','pr','merge',...])" matched the allowlist and no deny prefix. Every mutation the deny list names has been reachable that whole time. fix(review): grant the reviewer a general shell and scratch writes (#566, #572, #564) #578 shortens the route; it does not open it.

So the deny list is doing real work against the failure that actually happens --- a model acting in good faith issues the plain command, and the plain command is refused --- and no work at all against a model acting against its instructions.

What would actually close it

Split execution so the credential and the model never coincide:

1. A review job with contents: read only. It runs the model, and writes the review to an artifact.
2. A posting job that needs no model. It downloads the artifact and posts it, holding pull-requests: write / issues: write.

The model then has no writable forge credential in its environment, and the deny list stops being load-bearing for anything except keeping the reviewer on task.

Points to work through:

• The inline-comment MCP tool posts during the model turn, so it would have to move to the posting job (emit findings as structured data, post them downstream) or be dropped. That is the main design cost.
• claude-code-action performs an OIDC App-token exchange; whether the resulting token can be scoped down to contents: read needs checking against the action's source rather than assumed.
• The id-token: write grant stays on the review job.
• Worth checking whether the posting job can be pull_request_target-free, since the review already refuses forks (gha#235).

Related

• fix(review): grant the reviewer a general shell and scratch writes (#566, #572, #564) #578 --- the allowlist widening that prompted this, and which documents the guard-rail-not-boundary distinction in place.
• Decide whether/how to restrict claude-code-review's network egress (WebFetch + Bash(python3:*) + Bash(curl:*) together) #240 --- the standing question about the reviewer's network egress, which this would partly subsume: a contents: read model job with no writable token makes egress much less interesting.
• run-claude-review-attempt: the reviewer allowlist permits no git commands at all, so a PR about git behavior cannot be verified #566 / run-claude-review-attempt: the reviewer cannot run this repo's own shell test suites, so a PR that adds one starves the review #572 --- why narrowing the shell back down is not the alternative.