From 9d2e572ffd2ccb6151fa50c475800e4d44e86667 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Thu, 21 May 2026 14:36:22 +0100 Subject: [PATCH] ci: strip safe-to-review label on new fork pushes (#992) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 * 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 --------- Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 4.7 --- .github/workflows/claude-code-review.yml | 59 +++++++++++++++++++++--- .github/workflows/preview-env.yml | 53 +++++++++++++++------ 2 files changed, 93 insertions(+), 19 deletions(-) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 00e7ec9aaf..713f8eeae9 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -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: diff --git a/.github/workflows/preview-env.yml b/.github/workflows/preview-env.yml index a5148417ec..d3187ad233 100644 --- a/.github/workflows/preview-env.yml +++ b/.github/workflows/preview-env.yml @@ -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)