ci: strip safe-to-review label on new fork pushes (#992)
* ci: strip safe-to-review label on new fork pushes Mirrors the strip-safe-to-test job in preview-env.yml so a once-approved fork PR cannot run the Claude review against new (potentially malicious) code on subsequent pushes without fresh maintainer re-approval. Closes a gap between the two pull_request_target workflows flagged by an external scanner. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci: close strip-label race on fork PRs Three reviewers (CodeRabbit, cubic, Greptile) flagged that strip-safe-to-review ran in parallel with claude-review-fork on the same synchronize event, so the label was still present in the webhook snapshot when the review job's if was evaluated — the strip only blocked the next push, not the current one. Same flaw existed for preview-env.yml's deploy-fork (worse there, since it runs the fork's cmd/preview code with SPRITES_API_TOKEN). Fix: - Add `labeled` to pull_request_target types so a maintainer applying the opt-in label triggers a review/deploy against the current head SHA. - Guard the label clause in claude-review-fork / deploy-fork with `github.event.action != 'synchronize'`. Trusted-author paths (OWNER / MEMBER / COLLABORATOR and the *_ALLOWLIST repo variable) still authorize on every event; only the label path is per-commit. - Wrap removeLabel calls in try/catch that swallows 404 on races. - Add workflow-level concurrency to dedupe rapid pushes (cancel-in-progress is false on `closed` for preview-env so cleanup can finish). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci: reorder fork PR if-gate branches to be fromJSON-safe cubic flagged that the previous reorder put the `fromJSON(vars.*_ALLOWLIST)` branch before the label clause. A non-empty but malformed allowlist value would throw and skip the whole job, masking a legitimate maintainer label approval. Reorder to: author_association -> label (with synchronize guard) -> allowlist, so the cheap/safe branches short-circuit before any potentially throwing fromJSON call. Applied to both fork workflows. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Kandev Agent <agent@kandev.dev> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
João Salavisa
@@ -15,7 +15,15 @@ on:
|
||||
pull_request:
|
||||
types: [opened, synchronize, ready_for_review, reopened]
|
||||
pull_request_target:
|
||||
types: [opened, synchronize, ready_for_review, reopened]
|
||||
# `labeled` is needed so a maintainer applying `safe-to-review` to a fork
|
||||
# PR triggers a review against the current head SHA. Subsequent fork pushes
|
||||
# (synchronize) do NOT re-trigger the label path — see claude-review-fork's
|
||||
# if-gate and strip-safe-to-review below.
|
||||
types: [opened, synchronize, ready_for_review, reopened, labeled]
|
||||
|
||||
concurrency:
|
||||
group: claude-review-${{ github.event.pull_request.number || github.ref }}
|
||||
cancel-in-progress: true
|
||||
|
||||
jobs:
|
||||
claude-review-same-repo:
|
||||
@@ -66,17 +74,56 @@ jobs:
|
||||
--allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*)"
|
||||
--disallowedTools "Edit,Write,MultiEdit,NotebookEdit,WebFetch,WebSearch,Bash(git add:*),Bash(git commit:*),Bash(git push:*),Bash(git rebase:*),Bash(git reset:*),Bash(gh pr edit:*),Bash(curl:*),Bash(wget:*)"
|
||||
|
||||
# Strip safe-to-review label on new pushes to fork PRs.
|
||||
# Together with the `github.event.action != 'synchronize'` guard on the
|
||||
# label path in claude-review-fork's if-gate, this gives true per-commit
|
||||
# opt-in: the maintainer must re-apply `safe-to-review` after each push for
|
||||
# the review job to run again. A new push on its own never re-triggers the
|
||||
# review on the label path.
|
||||
strip-safe-to-review:
|
||||
if: >
|
||||
github.event_name == 'pull_request_target' &&
|
||||
github.event.action == 'synchronize' &&
|
||||
github.event.pull_request.head.repo.full_name != github.repository &&
|
||||
contains(github.event.pull_request.labels.*.name, 'safe-to-review')
|
||||
runs-on: ubuntu-latest
|
||||
permissions:
|
||||
pull-requests: write
|
||||
steps:
|
||||
- uses: actions/github-script@v7
|
||||
with:
|
||||
script: |
|
||||
try {
|
||||
await github.rest.issues.removeLabel({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: context.payload.pull_request.number,
|
||||
name: 'safe-to-review',
|
||||
});
|
||||
} catch (error) {
|
||||
if (error.status !== 404) throw error;
|
||||
core.info('safe-to-review label already absent; nothing to remove.');
|
||||
}
|
||||
|
||||
claude-review-fork:
|
||||
# Fork PRs only, via pull_request_target so secrets are available.
|
||||
# Gated by allowlist: OWNER/MEMBER/COLLABORATOR author, `safe-to-review`
|
||||
# label (maintainer opt-in), or CLAUDE_REVIEW_ALLOWLIST repo variable
|
||||
# (JSON array string, e.g. ["alice","bob"], in Settings -> Secrets and
|
||||
# variables -> Actions -> Variables).
|
||||
# Three approval paths:
|
||||
# - OWNER/MEMBER/COLLABORATOR author_association (trusted user, all events)
|
||||
# - CLAUDE_REVIEW_ALLOWLIST repo variable (trusted login list, all events;
|
||||
# JSON array, e.g. ["alice","bob"], in Settings -> Secrets and variables
|
||||
# -> Actions -> Variables)
|
||||
# - `safe-to-review` label (maintainer opt-in, per-commit). Explicitly
|
||||
# blocked on `synchronize` so a contributor's push cannot re-use a
|
||||
# stale label — the maintainer must re-apply it after each push for
|
||||
# the `labeled` event to trigger a fresh review against the new SHA.
|
||||
# Branch order matters: keep cheap/safe checks before `fromJSON(vars.*)`,
|
||||
# which throws (and skips the whole job) if the repo variable is
|
||||
# non-empty but unparseable JSON.
|
||||
if: >
|
||||
github.event_name == 'pull_request_target' &&
|
||||
github.event.pull_request.head.repo.full_name != github.repository &&
|
||||
(contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.pull_request.author_association) ||
|
||||
contains(github.event.pull_request.labels.*.name, 'safe-to-review') ||
|
||||
(github.event.action != 'synchronize' && contains(github.event.pull_request.labels.*.name, 'safe-to-review')) ||
|
||||
(vars.CLAUDE_REVIEW_ALLOWLIST != '' && contains(fromJSON(vars.CLAUDE_REVIEW_ALLOWLIST), github.event.pull_request.user.login)))
|
||||
runs-on: ubuntu-latest
|
||||
permissions:
|
||||
|
||||
@@ -8,7 +8,16 @@ on:
|
||||
pull_request:
|
||||
types: [opened, synchronize, reopened, closed]
|
||||
pull_request_target:
|
||||
types: [opened, synchronize, reopened, closed]
|
||||
# `labeled` is needed so a maintainer applying `safe-to-test` to a fork PR
|
||||
# triggers a deploy against the current head SHA. Subsequent fork pushes
|
||||
# (synchronize) do NOT re-trigger the label path — see deploy-fork's
|
||||
# if-gate and strip-safe-to-test below.
|
||||
types: [opened, synchronize, reopened, closed, labeled]
|
||||
|
||||
concurrency:
|
||||
group: preview-env-${{ github.event.pull_request.number || github.ref }}
|
||||
# Don't cancel cleanup on close — let it finish destroying the sprite.
|
||||
cancel-in-progress: ${{ github.event.action != 'closed' }}
|
||||
|
||||
jobs:
|
||||
# -----------------------------------------------------------------------
|
||||
@@ -59,17 +68,27 @@ jobs:
|
||||
# -----------------------------------------------------------------------
|
||||
# Deploy / update preview (fork PRs, gated by allowlist)
|
||||
# Uses pull_request_target so secrets are available.
|
||||
# Gated by: OWNER/MEMBER/COLLABORATOR author_association,
|
||||
# `safe-to-test` label (maintainer opt-in), or
|
||||
# PREVIEW_ENV_ALLOWLIST repo variable (JSON array of logins).
|
||||
# Three approval paths:
|
||||
# - OWNER/MEMBER/COLLABORATOR author_association (trusted user, all events)
|
||||
# - PREVIEW_ENV_ALLOWLIST repo variable (trusted login list, all events;
|
||||
# JSON array of logins)
|
||||
# - `safe-to-test` label (maintainer opt-in, per-commit). Explicitly
|
||||
# blocked on `synchronize` so a contributor's push cannot re-use a
|
||||
# stale label — the maintainer must re-apply it after each push for
|
||||
# the `labeled` event to trigger a fresh deploy of the new SHA. This
|
||||
# matters more here than for claude-code-review.yml because deploy
|
||||
# actually runs the fork's `cmd/preview` code with SPRITES_API_TOKEN.
|
||||
# -----------------------------------------------------------------------
|
||||
# Branch order matters: keep cheap/safe checks before `fromJSON(vars.*)`,
|
||||
# which throws (and skips the whole job) if the repo variable is
|
||||
# non-empty but unparseable JSON.
|
||||
deploy-fork:
|
||||
if: >
|
||||
github.event.action != 'closed' &&
|
||||
github.event_name == 'pull_request_target' &&
|
||||
github.event.pull_request.head.repo.full_name != github.repository &&
|
||||
(contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.pull_request.author_association) ||
|
||||
contains(github.event.pull_request.labels.*.name, 'safe-to-test') ||
|
||||
(github.event.action != 'synchronize' && contains(github.event.pull_request.labels.*.name, 'safe-to-test')) ||
|
||||
(vars.PREVIEW_ENV_ALLOWLIST != '' && contains(fromJSON(vars.PREVIEW_ENV_ALLOWLIST), github.event.pull_request.user.login)))
|
||||
runs-on: ubuntu-latest
|
||||
permissions:
|
||||
@@ -115,8 +134,11 @@ jobs:
|
||||
|
||||
# -----------------------------------------------------------------------
|
||||
# Strip safe-to-test label on new pushes to fork PRs.
|
||||
# Prevents a once-approved fork from running updated (potentially malicious)
|
||||
# code on subsequent pushes without a fresh maintainer re-approval.
|
||||
# Together with the `github.event.action != 'synchronize'` guard on the
|
||||
# label path in deploy-fork's if-gate, this gives true per-commit opt-in:
|
||||
# the maintainer must re-apply `safe-to-test` after each push for the
|
||||
# deploy job to run again. A new push on its own never re-triggers deploy
|
||||
# on the label path.
|
||||
# -----------------------------------------------------------------------
|
||||
strip-safe-to-test:
|
||||
if: >
|
||||
@@ -131,12 +153,17 @@ jobs:
|
||||
- uses: actions/github-script@v7
|
||||
with:
|
||||
script: |
|
||||
await github.rest.issues.removeLabel({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: context.payload.pull_request.number,
|
||||
name: 'safe-to-test',
|
||||
});
|
||||
try {
|
||||
await github.rest.issues.removeLabel({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: context.payload.pull_request.number,
|
||||
name: 'safe-to-test',
|
||||
});
|
||||
} catch (error) {
|
||||
if (error.status !== 404) throw error;
|
||||
core.info('safe-to-test label already absent; nothing to remove.');
|
||||
}
|
||||
|
||||
# -----------------------------------------------------------------------
|
||||
# Cleanup (all closed PRs — no allowlist check)
|
||||
|
||||
Reference in New Issue
Block a user