Repository navigation
revert(ci): ci-approved fork PR trigger (security + stability) - #1175
Conversation
This reverts commit b776668. Reverting as part of rolling back the ci-approved fork PR trigger added in #1095. Once the pull_request_target workflow trigger is removed, these e2e job conditions referring to `github.event_name != 'pull_request_target'` become dead code and would never evaluate, so they are reverted together. Refs: #1095, #1104 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…#1095) This reverts commit 381c5f6. Reverted for two reasons: 1) Security exposure (classic "pwn request" pattern). The workflow used pull_request_target with `types: [labeled, synchronize]` and then checked out the fork head SHA. Once a maintainer applies the `ci-approved` label, any subsequent push from the fork triggers `synchronize` and runs the new fork code on self-hosted runners (k8s-runner-cpu / k8s-runner-gpu) with access to workflow secrets (GITHUB_TOKEN, and ANTHROPIC_API_KEY via the Claude Code Review workflow which applied the same pattern). An attacker can push benign code, wait for the label, then force-push malicious code that exfiltrates secrets or persists on the self-hosted runner. 2) CI instability on main and open PRs. Internal PRs now fire both `pull_request` and `pull_request_target` into the same concurrency group (`gateway-tests-${{ pr.number || ref }}` with `cancel-in-progress: true`), so the two event types race and cancel each other. Recent 40 runs: pull_request 1/21 success, pull_request_target 3/18 success; the rest are cancelled or action_required. This blocks both landing on main and reviewing feature PRs. The fork-PR CI convenience should be reintroduced later with a safer design: either split privileged and unprivileged jobs into two workflows (pull_request_target runs only read-only steps, privileged tests stay on pull_request), or pin the approved head SHA at label-add time and auto-remove the label on push, or use a separate repo-level approval token flow. The current implementation should not be restored as-is. Refs: #1095 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTwo GitHub Actions workflows are consolidated to run exclusively on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36cffd5242
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - name: Checkout | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha }} | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
Removing the ref from actions/checkout makes pull_request runs check out GitHub’s synthetic merge ref instead of the PR head commit, but this workflow’s prompt still relies on git diff ${{ github.event.before }}..HEAD to review only the latest push. On synchronize events for long-lived PRs, HEAD now includes merge-base updates from main, so Claude can re-review unrelated upstream changes and post noisy/incorrect inline comments. Keeping checkout pinned to github.event.pull_request.head.sha avoids that regression.
Useful? React with 👍 / 👎.
Adds an if: condition on build-wheel that skips expensive GPU and paid-API jobs for fork PRs unless they carry the 'ci-approved' label. Internal PRs run unconditionally. build-wheel is the first-tier expensive job; benchmarks, e2e-* (1/2/4-GPU), e2e-vendor (paid Anthropic/OpenAI/xAI APIs), python-unit-tests, go-unit-tests, and go-bindings-e2e all inherit the gate transitively through their needs: build-wheel chain. Does not reintroduce pull_request_target or the prior ci-gate job (both reverted in #1175). To kick off expensive jobs on a fork PR after adding the label: wait for the contributor's next push (synchronize event), or click "Re-run all jobs" in the Actions UI. Recommended companion setting (repo admin): Settings -> Actions -> General -> Fork pull request workflows from outside collaborators: change to "Require approval for first-time contributors" so returning fork contributors don't need the manual approve-and-run click on every push. Signed-off-by: Chang Su <chang.s.su@oracle.com>
1 participant
Description
Problem
Two recently-merged CI changes — #1095 (
ci: allow 'ci-approved' label to bypass fork PR approval gate) and its follow-up #1104 (ci: apply change detection to pull_request_target events) — are causing two serious problems onmainand all open PRs.1) Security: classic "pwn request" exposure.
pr-test-rust.yml(andclaude-code-review.yml) were wired with:plus a
ci-approvedlabel gate, and every job checks outgithub.event.pull_request.head.sha— i.e. the fork's code — while running withpull_request_targetprivileges (base-branch workflow context + access to repo secrets + self-hosted runner access).Because
synchronizeis one of the trigger types, the exploit path is:ci-approved.synchronizefires → label is still present → CI runs the new fork code onk8s-runner-cpu/k8s-runner-gpuwithGITHUB_TOKENand (via the Claude review workflow)ANTHROPIC_API_KEY.Blast radius includes secret exfiltration plus potential pivot on the self-hosted runners. This is the pattern GitHub's Security Lab post specifically warns against.
2) CI instability on main and PRs.
Internal (non-fork) PRs now fire BOTH
pull_requestandpull_request_targetinto the same concurrency group:The two event types race and cancel each other. Last 40
pr-test-rust.ymlruns broke down as:pull_requestpull_request_targetpushSo ~10% of runs complete successfully; the rest are cancelled mid-flight or need manual approval. This blocks both landing on
mainand validating feature PRs.Solution
Revert both PRs. The fork-PR CI convenience should be re-introduced later with a secure design (see Follow-up below) rather than patched in place.
Changes
Two revert commits:
revert(ci): apply change detection to pull_request_target events (#1104)— drops the 7&& github.event_name != 'pull_request_target'guards from e2e job conditions (dead code once the trigger is removed).revert(ci): allow 'ci-approved' label to bypass fork PR approval gate (#1095)— removes thepull_request_targettrigger, theci-gatejob and allneeds: [ci-gate]edges, the per-jobref: ${{ github.event.pull_request.head.sha || github.sha }}overrides, and the equivalent changes in.github/workflows/claude-code-review.yml. Restores the originalconcurrencyconfig.Net diff:
+6 / -51lines across two workflow files. No source code touched.Follow-up (NOT in this PR)
When we re-add fork-PR CI support, it must use one of these safer patterns:
pull_request(untrusted code cannot access secrets). A separatepull_request_targetworkflow runs only read-only steps (labeler, PR comment, etc.) with narrowpermissions:.ci-approvedis added; only run CI for that exact SHA. Auto-remove the label on every new push so re-approval is required.Test plan
pull_request_target/ci-approved/ci-gatereferences remain in either workflow5319dc6ewhich addede2e-1gpu-completions)pull_requestCI runs are no longer cancelled by a competingpull_request_targetrunmainpush runs stop racing with PR runsRefs: #1095, #1104
Summary by CodeRabbit