Checking out a merge commit in pull_request_target workflows

[mpdude]

I am coming from dependabot/dependabot-core#3253, where there is a lot of confusion of how to safely run actions with secrets when untrusted code from external PRs comes into play.

The bottom line is that there may be situations where you – after you understood the risks – might want to use the pull_request_target event because it has access to secrets; but combine that with a checkout of the PR.

One suggested way of doing this is with

- name: Checkout
        uses: actions/checkout@v2
        with:
          ref: ${{ github.event.pull_request.head.sha }}

which will check out the PR head commit.

This is, however, not 100% what you'd get for a simple uses: actions/checkout@v2 on a pull_request event, because that would check out a merge commit.

I wonder whether it would be possible for this action here to also support checking out such a merge commit on pull_request_target events?

I don't know if creating such a merge commit involves advanced Git trickery to get-it-right™️ , so I thought this was the best place to ask.

[mpdude]

Here's another reason why it would be helpful if this could be supported directly in the action:

I'd like to use actions/checkout with persist-credentials: false to leak as little sensitive information as possible. But with this, I cannot easily run any git commands myself in subsequent steps to create a merge commit (for example, it seems I'd need an extra git fetch to get the PR head commit first).

If the merge commit could be created by the action, it could happen before the cleanup of persisted credentials.

[trim21]

      - name: Checkout code
        uses: actions/checkout@v2
        with:
          ref: "refs/pull/${{ github.event.number }}/merge"

this should work as you expected I think.

updated 2023-08-02

When I previsouly wrote this reply, github doesn't support disabling or limiting GitHub Actions for outside collaborators, so it's not a problem to simply use refs/pull/${{ github.event.number }}/merge.

But if your workflow require approval for outside collaborators, you should use this one

      - name: Checkout code
        uses: actions/checkout@v3
        with:
          ref: "${{ github.event.pull_request.merge_commit_sha }}"

[geekflyer]

      - name: Checkout code
        uses: actions/checkout@v2
        with:
          ref: "refs/pull/${{ github.event.number }}/merge"

this should work as you expected I think.

I wonder if this is safe fully safe though? With this approach, if a malicious PR author pushes unsafe code between PR approval and the checkout action being run, your CI system might checkout a different version of the code than what the maintainer approved.

ref: ${{ github.event.pull_request.head.sha }} on the other hand is safe since it references a fixed head.sha that corresponds to the time the pull_request_target has triggered.

[trim21]

I wonder if this is safe fully safe though? With this approach, if a malicious PR author pushes unsafe code between PR approval and the checkout action being run, your CI system might checkout a different version of the code than what the maintainer approved.

ref: ${{ github.event.pull_request.head.sha }} on the other hand is safe since it references a fixed head.sha that corresponds to the time the pull_request_target has triggered.

It's not safe.

That's why you shouldn't execute any test code or lint plugin from PR. They can read env and maybe able to read your repo secret.

you should only execute code like lint (with out loading plugin from repo) or formatter.

[geekflyer]

I wonder if this is safe fully safe though? With this approach, if a malicious PR author pushes unsafe code between PR approval and the checkout action being run, your CI system might checkout a different version of the code than what the maintainer approved.
ref: ${{ github.event.pull_request.head.sha }} on the other hand is safe since it references a fixed head.sha that corresponds to the time the pull_request_target has triggered.

It's not safe.

That's why you shouldn't execute any test code or lint plugin from PR. They can read env and maybe able to read your repo secret.

you should only execute code like lint (with out loading plugin from repo) or formatter.

I mean ref: ${{ github.event.pull_request.head.sha }} is safe IF the maintainer has fully reviewed the the PR code and determined that the code is safe (let's say for a README change). However ref: "refs/pull/${{ github.event.number }}/merge" is not safe, because even if the original PR was just changing some README, a malicious actor can push more code changes in a specific time window that are then being tested against.

[trim21]

Yes, but github.event.pull_request.head.sha is not what we want in this situation. We want to know that the repo will look like after the pr is merged.

[geekflyer]

Yes, but github.event.pull_request.head.sha is not what we want in this situation. We want to know that the repo will look like after the pr is merged.

yeah, I mean there is probably other ways to achieve the merge and test against the merged state. But using refs/pull/${{ github.event.number }}/merge" alone is not exactly the safest way since it references a ref that can be updated by the outside contributor.

[dvdhrm]

Workaround:

Checkout refs/pull/${{ github.event.number }}/merge and then verify HEAD^ equals github.event.pull_request.head.sha afterwards. If they don't match, cancel your workflow (a newer one is already scheduled, given that the head-sha changed).

Not ideal, but safe and works.

[antoniovazquezblanco]

Gist of the previous sugestion:

- name: Checkout
  uses: actions/checkout@v3
  with:
    ref: "refs/pull/${{ github.event.number }}/merge"

# Important security check: https://github.com/actions/checkout/issues/518
- name: Sanity check
  run: |
    [[ "$(git rev-parse 'HEAD^')" == "${{ github.event.pull_request.head.sha }}" ]]

[fxmarty]

@antoniovazquezblanco I believe the correct syntax should be:

- name: Checkout
  uses: actions/checkout@v3
  with:
    ref: "refs/pull/${{ github.event.number }}/merge"
    fetch-depth: 2

# Important security check: https://github.com/actions/checkout/issues/518
- name: Sanity check
  run: |
    [[ "$(git rev-parse 'HEAD^2')" == "${{ github.event.pull_request.head.sha }}" ]]

because the branch that is merged into is the first parent. Isn't it?

[mering]

What happens when this commit is not available (e.g. in case of a merge conflict)? The pull_request_target event will still be triggered (in contrast to the pull_request event), so do we need an additional if to check for that?

[AlekSi]

So ref: "${{ github.event.pull_request.merge_commit_sha }}" is the answer, and only the README.md (and possibly https://securitylab.github.com/research/github-actions-preventing-pwn-requests/) should be updated?

Edit: I see that github.event.pull_request.merge_commit_sha is null for pull_request_target event: https://github.com/FerretDB/FerretDB/actions/runs/6309162567/job/17128566077?pr=3433 This documentation page suggests that merge_commit_sha can be null while GitHub checks the mergeability of the PR in the background. Unfortunately, it is too late for GitHub Actions – the job is already started with github.event.pull_request.merge_commit_sha being null.

[AlekSi]

What happens when this commit is not available (e.g. in case of a merge conflict)? The pull_request_target event will still be triggered (in contrast to the pull_request event), so do we need an additional if to check for that?

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request says this:

Conversely, workflows with the pull_request_target event will run even if the pull request has a merge conflict.

[faximan]

Edit: I see that github.event.pull_request.merge_commit_sha is null for pull_request_target event: https://github.com/FerretDB/FerretDB/actions/runs/6309162567/job/17128566077?pr=3433 This documentation page suggests that merge_commit_sha can be null while GitHub checks the mergeability of the PR in the background. Unfortunately, it is too late for GitHub Actions – the job is already started with github.event.pull_request.merge_commit_sha being null.

I think I am observing this too. Using

ref: "${{ github.event.pull_request.merge_commit_sha }}"

seems to consistently check out a merge commit that is based on the second latest commit in the PR. When it is triggered on the first commit in the PR, it simply checks out main.

My best guess is a race and that merge_commit_sha returns the last computed merge commit until the mergeability of the current commit has been computed. Does this make sense?

[nguyentvan7]

Edit: I see that github.event.pull_request.merge_commit_sha is null for pull_request_target event: https://github.com/FerretDB/FerretDB/actions/runs/6309162567/job/17128566077?pr=3433 This documentation page suggests that merge_commit_sha can be null while GitHub checks the mergeability of the PR in the background. Unfortunately, it is too late for GitHub Actions – the job is already started with github.event.pull_request.merge_commit_sha being null.

I think I am observing this too. Using

ref: "${{ github.event.pull_request.merge_commit_sha }}"

seems to consistently check out a merge commit that is based on the second latest commit in the PR. When it is triggered on the first commit in the PR, it simply checks out main.

My best guess is a race and that merge_commit_sha returns the last computed merge commit until the mergeability of the current commit has been computed. Does this make sense?

I am hitting this too. Is there anyway to delay the job until the mergeability request is finished? I'm not sure if a delay within the workflow on a separate job before referencing the SHA is enough

[lujop]

I'm having precisely the same problem, that when the PR is created and I checkout the github.event.pull_request.merge_commit_sha the base branch is retrieved.
Then if I add another commit it works correctly.

[fxmarty]

cc @regisss

[nguyentvan7]

@lujop @faximan and others who hit the same issue

I worked around this by having a pull_request event action that creates an artifact. Then have another workflow_run action that triggers whenever this pr action completes. It will download the artifact, then do all the processing in this action.

• pull_request event does not have secret access for forks, but it does wait for the merge commit to be ready before triggering the workflow.
• Therefore, the artifact creation is guaranteed to be on a valid merge commit.
• The workflow_run action has full secret access as it is triggered on the master branch. There, it can download the artifact and do anything it needs with proper permissions

This doesn't help if you want the processing to show up on the PR, but it worked for us, as we just needed the secret access to trigger a fire-and-forget deployment action. And the part we really cared to check was the artifact creation to make sure it succeeds.

frzyc/genshin-optimizer@af59815

[lujop]

In our case I end up using github.event.pull_request.head.sha as we have the restriction to Require branches to be up to date before merging.
And in the end the update will be done and the validation run again.

[alexander-schranz]

I'm still stuck on this one. Specially as my CI task runs not only for pull_request_target but als for push.branches. Somebody successfully using pull_request_target still?

[infinisil]

This is a bit messy, but I wrote an inline script to check whether the PR is mergeable, and it seems to work decently: https://github.com/NixOS/nixpkgs/blob/c8db8bd9656ee3d373ce9445063c25c47f442118/.github/workflows/check-by-name.yml#L31-L86