fix(security): Remove unsafe PR head checkout in pull_request_target workflow#790
Closed
fix-it-felix-sentry[bot] wants to merge 1 commit intomasterfrom
Closed
fix(security): Remove unsafe PR head checkout in pull_request_target workflow#790fix-it-felix-sentry[bot] wants to merge 1 commit intomasterfrom
fix-it-felix-sentry[bot] wants to merge 1 commit intomasterfrom
Conversation
…workflow This commit addresses a high-severity security vulnerability where the workflow would fall back to checking out the PR head when merge conflicts existed. With pull_request_target, checking out untrusted PR code could allow attackers to execute arbitrary code with access to repository secrets. Changes: - Removed the fallback checkout step that used github.event.pull_request.head.sha - Removed continue-on-error from the merge ref checkout - Added a failure handler that provides clear error messages when merge conflicts exist - Updated security notes to document the safeguards The workflow now fails safely when merge conflicts are present, requiring them to be resolved before the changelog preview can run. Fixes: https://linear.app/getsentry/issue/DI-1825 Related: https://linear.app/getsentry/issue/VULN-1416 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Comment on lines
+96
to
+101
| - name: Check for merge conflicts | ||
| if: failure() | ||
| run: | | ||
| echo "::error::Unable to checkout merge ref. This PR likely has merge conflicts." | ||
| echo "::error::Please resolve merge conflicts before the changelog preview can be generated." | ||
| exit 1 |
There was a problem hiding this comment.
Bug: The error handling step with if: failure() will not execute when the checkout-merge step fails because continue-on-error: true was removed from the failing step.
Severity: MEDIUM
Suggested Fix
Reinstate continue-on-error: true on the checkout-merge step. This will allow the workflow to proceed to the next step, where the if: failure() condition can be evaluated to display the intended error message when merge conflicts occur.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/workflows/changelog-preview.yml#L96-L101
Potential issue: The `checkout-merge` step had `continue-on-error: true` removed, and a
new step with `if: failure()` was added to handle checkout failures. However, GitHub
Actions jobs halt immediately on a failed step. Subsequent steps, even those with an
`if: failure()` condition, are not evaluated unless the failing step is marked with
`continue-on-error: true`. Consequently, if the `checkout-merge` step fails (e.g., due
to merge conflicts), the intended error message will not be displayed, and the workflow
will fail without providing the helpful context to the user.
Did we get this right? 👍 / 👎 to inform future reviews.
Member
|
This is intentional |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes a high-severity security vulnerability (VULN-1416) in the changelog-preview workflow where untrusted PR code could be executed with access to repository secrets.
Problem
The workflow uses
pull_request_targetwhich runs with elevated permissions and access to secrets. The vulnerable code had a fallback that would checkout the PR head ref (github.event.pull_request.head.sha) when merge conflicts existed:This allowed attackers to potentially execute arbitrary code from malicious PRs with full repository access.
Solution
Changes
continue-on-error: truefrom merge ref checkoutTesting
This is a workflow configuration change. The behavior change is:
No existing tests are affected as there are no automated tests for workflow files.
References