Skip to content

fix(security): Remove unsafe PR head checkout in pull_request_target workflow #790

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fix-it-felix-sentry wants to merge 1 commit into from
+10 −7
Closed

fix(security): Remove unsafe PR head checkout in pull_request_target workflow #790

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions .github/workflows/changelog-preview.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,11 @@ on:
#
# SECURITY NOTE:
# This workflow is safe to use with pull_request_target because:
# - We ONLY checkout the merge ref, NEVER the PR head ref
# - The Craft binary is downloaded from releases, NOT from the PR
# - Only git metadata (commits, tags) and .craft.yml config are read
# - No code from the PR is ever executed
# - If merge conflicts exist, the workflow fails safely without executing PR code
#
workflow_call:
inputs:
Expand Down Expand Up @@ -83,19 +85,20 @@ jobs:
runs-on: ubuntu-latest
steps:
# For pull_request_target, we must explicitly specify the ref to get the PR commits.
# Try the merge ref first; fall back to head ref if PR has merge conflicts.
# We ONLY use the merge ref for security reasons - never checkout PR head with pull_request_target.
# If the merge ref is unavailable (e.g., merge conflicts), we fail the workflow safely.
- uses: actions/checkout@v6
id: checkout-merge
continue-on-error: true
with:
fetch-depth: 0
ref: refs/pull/${{ github.event.pull_request.number }}/merge

- uses: actions/checkout@v6
if: steps.checkout-merge.outcome == 'failure'
with:
fetch-depth: 0
ref: ${{ github.event.pull_request.head.sha }}
- 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
Comment on lines +96 to +101
Copy link
Copy Markdown

@sentry sentry Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


- name: Install Craft
shell: bash
Expand Down
Loading