fix(ci): switch gpu-tests to pull_request_target to prevent approval bypass#199
fix(ci): switch gpu-tests to pull_request_target to prevent approval bypass#199k-chrispens merged 7 commits intomainfrom
Conversation
…bypass pull_request runs the workflow from the PR branch, allowing a collaborator to remove the environment gate and run on gpu-1 without approval. pull_request_target always uses the workflow from main. Also fix concurrency group to use PR number (github.ref is always refs/heads/main for pull_request_target) and add explicit checkout ref so the PR code is tested instead of main.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe GPU tests GitHub Actions workflow now runs under Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/gpu-tests.yml (1)
43-44: Explicit ref is necessary but introduces the security-sensitive code path.This checkout of PR code is required to actually test the PR changes (otherwise
pull_request_targetchecks outmain). However, this is the line that enables execution of untrusted code with elevated privileges.Consider adding a comment explaining the security model:
📝 Suggested documentation
- name: Checkout code uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + # SECURITY: Checking out PR code with pull_request_target requires + # the 'gpu-testing' environment approval gate to prevent malicious PRs + # from accessing secrets. Reviewers MUST inspect PR code before approving. with: ref: ${{ github.event.pull_request.head.sha || github.sha }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/gpu-tests.yml around lines 43 - 44, Add an inline comment above the checkout ref line (the line containing ref: ${{ github.event.pull_request.head.sha || github.sha }}) explaining why we must explicitly check out the PR head when using pull_request_target, that this enables running untrusted PR code under elevated workflow permissions, and list the mitigations in use (e.g., only running this job on non-sensitive runners, using least-privilege tokens, requiring approved maintainers for prod actions, and noting any additional safeguards). Keep the comment concise, reference the workflow trigger pull_request_target, and mention that reviewers should validate security-sensitive steps before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/gpu-tests.yml:
- Around line 43-44: Add an inline comment above the checkout ref line (the line
containing ref: ${{ github.event.pull_request.head.sha || github.sha }})
explaining why we must explicitly check out the PR head when using
pull_request_target, that this enables running untrusted PR code under elevated
workflow permissions, and list the mitigations in use (e.g., only running this
job on non-sensitive runners, using least-privilege tokens, requiring approved
maintainers for prod actions, and noting any additional safeguards). Keep the
comment concise, reference the workflow trigger pull_request_target, and mention
that reviewers should validate security-sensitive steps before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f6d500b-f698-460d-bca9-07fa3275e321
📒 Files selected for processing (1)
.github/workflows/gpu-tests.yml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/gpu-tests.yml (1)
12-24:⚠️ Potential issue | 🟠 MajorUse a PR-specific concurrency key after switching to
pull_request_target.Line 12 switches this workflow to
pull_request_target, which causesgithub.refat line 23 to resolve torefs/heads/main(the base branch). This means all PR runs collapse into a single concurrency group and can cancel each other, while also colliding withpushruns onmain.Suggested fix
concurrency: - group: gpu-tests-${{ github.ref }} + group: gpu-tests-${{ github.event.pull_request.number || github.sha }} cancel-in-progress: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/gpu-tests.yml around lines 12 - 24, The concurrency key uses github.ref (gpu-tests-${{ github.ref }}) while the workflow is using pull_request_target, which makes all PR runs share the same group; update the concurrency.group to include a PR-specific identifier (for example use github.event.pull_request.number when present) so PR runs are isolated—replace gpu-tests-${{ github.ref }} with something like gpu-tests-${{ github.event.pull_request.number || github.ref }} (or an equivalent conditional expression) in the concurrency.group value to ensure unique groups per PR while falling back to ref for non-PR events.
🧹 Nitpick comments (1)
.github/workflows/gpu-tests.yml (1)
41-44: Make the non-PR checkout fallback explicit.Line 44 is correct for PRs, but
push/workflow_dispatchnow depend onactions/checkouttreating an emptyrefas “use the event SHA”. Encoding the fallback directly makes the workflow behavior match the PR objective instead of relying on that implicit behavior.Suggested fix
- name: Checkout code uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 with: - ref: ${{ github.event.pull_request.head.sha }} + ref: ${{ github.event.pull_request.head.sha || github.sha }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/gpu-tests.yml around lines 41 - 44, Update the "Checkout code" step that uses actions/checkout so the ref is explicit for non-PR events: change the ref input (currently using ${{ github.event.pull_request.head.sha }}) to a conditional/fallback expression that uses the PR head SHA when present and falls back to the event SHA (e.g., github.sha) for push/workflow_dispatch; ensure the step name "Checkout code" and the uses line "actions/checkout" remain unchanged so the workflow behavior is deterministic across PR and non-PR runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/gpu-tests.yml:
- Around line 12-24: The concurrency key uses github.ref (gpu-tests-${{
github.ref }}) while the workflow is using pull_request_target, which makes all
PR runs share the same group; update the concurrency.group to include a
PR-specific identifier (for example use github.event.pull_request.number when
present) so PR runs are isolated—replace gpu-tests-${{ github.ref }} with
something like gpu-tests-${{ github.event.pull_request.number || github.ref }}
(or an equivalent conditional expression) in the concurrency.group value to
ensure unique groups per PR while falling back to ref for non-PR events.
---
Nitpick comments:
In @.github/workflows/gpu-tests.yml:
- Around line 41-44: Update the "Checkout code" step that uses actions/checkout
so the ref is explicit for non-PR events: change the ref input (currently using
${{ github.event.pull_request.head.sha }}) to a conditional/fallback expression
that uses the PR head SHA when present and falls back to the event SHA (e.g.,
github.sha) for push/workflow_dispatch; ensure the step name "Checkout code" and
the uses line "actions/checkout" remain unchanged so the workflow behavior is
deterministic across PR and non-PR runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2084da1e-f877-44ca-8391-50c94c84e7df
📒 Files selected for processing (1)
.github/workflows/gpu-tests.yml
k-chrispens
left a comment
k-chrispens
left a comment
There was a problem hiding this comment.
looks right to me. I was wondering if your earlier ci PR was determining if this was a problem!
2 participants
Summary
pull_requesttopull_request_targetso the workflow definition always comes frommain, preventing PRs from removing theenvironment: gpu-testingapproval gategithub.refis alwaysrefs/heads/mainforpull_request_targetevents, so all PRs would share one group)refto checkout step so PR code is tested instead of the base branchTest plan
gpu-tests.yml— verify the environment approval gate still firessrc/ortests/github.sha)Summary by CodeRabbit