ci(claude): gate the @claude trigger on author association - #124
Conversation
|
@claude review |
|
Claude finished @dmnyc's task in 2m 12s —— View job Review:
|
| Who | author_association on an org repo |
|---|---|
| Org owner / org member (incl. write access via a team) | MEMBER |
| Repo collaborator who is not an org member | COLLABORATOR |
| Everyone else | CONTRIBUTOR / NONE / … |
OWNER is only emitted for user-owned repos — on an org repo essentially nobody gets it, so that arm of the condition is dead code here. If you (or anyone else with push access) are an org member rather than an outside collaborator, @claude will silently stop firing after this merges — the job just never gets created, no failed run, no feedback.
(github.event.comment.author_association == 'OWNER' ||
github.event.comment.author_association == 'MEMBER' ||
github.event.comment.author_association == 'COLLABORATOR') &&Trade-off worth stating explicitly in the comment: MEMBER means any member of the Resolvr-io org, including members with only read access to this repo. For a small org that's fine and is the practical fix. If the org ever grows past the set of people you'd hand contents: write to, the precise version is a gate step that queries actual repo permission:
gh api repos/${{ github.repository }}/collaborators/${{ github.event.comment.user.login }}/permission \
--jq '.permission' | grep -qE '^(admin|write)$'🟠 2. This change cannot be tested from the PR — and the vulnerable version is live until merge
issue_comment and pull_request_review_comment workflows always run the version of the file on the default branch, never the PR branch. So:
- The run producing this very comment used the ungated
claude.ymlfrommain. Don't read "@claude still answers on ci(claude): gate the @claude trigger on author association #124" as evidence the gate is correct. - The only way to validate it is to merge, then comment
@claudeonmainand check the job actually starts. Given item 1, please do that check right after merge rather than assuming. - Until merge,
mainstill carries the any-commenter write path. Worth merging promptly rather than letting this sit.
🟡 3. id-token: write is still on claude.yml
.github/workflows/claude.yml:4-8
The rationale in the new claude-code-review.yml comment — "there is no OIDC use (auth is the OAuth token secret)" — applies verbatim to claude.yml, which uses the identical CLAUDE_CODE_OAUTH_TOKEN env auth. It keeps id-token: write anyway. M-3's recommendation names both workflows. Author gating makes it lower-risk, but there's no reason to keep a capability nothing consumes.
contents: write there is genuinely needed (the action commits and pushes), so keeping it is correct.
🟡 4. The PR description contradicts the comment you added about fork PRs
The new comment in claude-code-review.yml:3-6 says the OAuth token "fork PRs never receive" — correct: pull_request from a fork gets no repository secrets. But the PR body says auto-review of outside contributions "keeps working". Both can't hold: with an empty CLAUDE_CODE_OAUTH_TOKEN the action has nothing to authenticate with, so on a fork PR the job doesn't run read-only — it fails.
That's pre-existing, not caused by this PR, and the file comment is the accurate one. But it means "deliberately NOT author-gated so #87 keeps working" is resting on a behaviour that doesn't exist. Either confirm with a real fork PR, or adjust the stated rationale. The usual way to actually get fork auto-review is pull_request_target, which I'd argue against here — it grants the base-repo context to untrusted PR content, exactly the exposure this PR is closing.
🟢 5. Residual risk this doesn't (and can't) close
Author gating trusts the commenter, not the content. A collaborator typing @claude fix this on a fork PR still hands a contents: write job a checkout of attacker-authored diff/branch content — the classic prompt-injection path, just with a human-in-the-loop trigger. Not a blocker and not solvable in YAML; worth one line in the comment block so the next reader doesn't over-read the guarantee.
✅ Correct as written
- Expression grouping is right:
(auth) && ((issue_comment && …) || (review_comment && …)). The parenthesisation of the||arm is the easy thing to get wrong here and it isn't wrong. - Fails closed: any event without
github.event.commentyields an emptyauthor_association, so a future trigger type added toon:won't accidentally bypass the gate. contents: read+ droppingid-token: writeon the review workflow is minimal-permission and matches M-3's recommendation.- The comments explain why, not what — they'll survive the next person editing these files.
Follow-ups (out of scope, from the same scan)
- L-3:
actions/checkout@v4andanthropics/claude-code-action@v1are mutable tags. SHA-pinning is the remaining CI supply-chain item and matters more on a repo shipping a wallet. - Consider updating
docs/security-vulnerability-scan-2026-08.md:146-150to mark M-3 as addressed once merged and verified.
Verdict: approach is correct; item 1 is a functional blocker in practice — I'd land MEMBER before merging, and verify on main immediately after.
--- • branch fix/97-claude-workflow-gating
|
Review feedback addressed in f436cbf:
On #2: agreed this can only be validated post-merge (comment workflows run the default-branch file). Will verify |
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Fixes #97 (M-3).
claude.ymlfired on any commenter's@claudewithcontents: write+id-token: write— on a public repo that was a drive-by write vector into the repo that ships the wallet. The comment trigger is now gated to OWNER/COLLABORATOR (the people the permissions exist for).claude-code-review.ymldropsid-token: write(auth is the OAuth secret, no OIDC use; fork PRs never receive secrets anyway). It is deliberately NOT author-gated — with a caveat: fork PRs receive no secrets, so their auto-review job fails rather than running read-only. The real fix for fork auto-review would be pull_request_target, deliberately avoided here (it grants base-repo context to untrusted PR content).Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.