Skip to content

ci(claude): gate the @claude trigger on author association - #124

Open
dmnyc wants to merge 3 commits into
mainfrom
fix/97-claude-workflow-gating
Open

ci(claude): gate the @claude trigger on author association#124
dmnyc wants to merge 3 commits into
mainfrom
fix/97-claude-workflow-gating

Conversation

@dmnyc

@dmnyc dmnyc commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Fixes #97 (M-3).

claude.yml fired on any commenter's @claude with contents: 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.yml drops id-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).


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@dmnyc

dmnyc commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Aug 15, 2026

Copy link
Copy Markdown

Claude finished @dmnyc's task in 2m 12s —— View job


Review: ci(claude): gate the @claude trigger on author association

Right fix for M-3, and the reasoning in the added comments is sound. Two things would stop this from working as intended, plus a few residual items.


🔴 1. MEMBER is missing — this will likely break the trigger entirely

.github/workflows/claude.yml:17-24

Resolvr-io/apogee is an organization-owned repo. GitHub's author_association precedence means:

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)$'

Fix this →


🟠 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.yml from main. 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 @claude on main and check the job actually starts. Given item 1, please do that check right after merge rather than assuming.
  • Until merge, main still 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.

Fix this →

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.comment yields an empty author_association, so a future trigger type added to on: won't accidentally bypass the gate.
  • contents: read + dropping id-token: write on 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@v4 and anthropics/claude-code-action@v1 are 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-150 to 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

@dmnyc

dmnyc commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Review feedback addressed in f436cbf:

On #2: agreed this can only be validated post-merge (comment workflows run the default-branch file). Will verify @claude fires on main right after merge. On #4: the fork-PR auto-review claim in the body was wrong — updating the PR description now to match the file comment (fork PRs get no secrets, so the auto-review job fails rather than runs read-only).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[M-3] Gate @claude workflows on author association; drop id-token:write

1 participant