Not merging with "pull_request_target"

[jckimble]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the bug has not already been reported

Fastify version

3

Plugin version

No response

Node.js version

16

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

ubuntu-latest

Description

Dependabot pr's don't merge when using pull_request_target.

This can be fixed by adding "pull_request_target" with "pull_request" as github events to run with in action.yml.

I have no problem submitting a pull request adding this little detail, just let me know if you want the fix or if this was a design decision.

Steps to Reproduce

use pull_request_target for merging pull requests action

Expected Behavior

Should merge pull requests instead of failing silently.

[simoneb]

I don't remember the details but I have a feeling that pull_request_target may not work, or it may but have implications that need to be considered in terms of how we recommend to use it and possible security implications.

It's worth having a closer look to figure out if we can simply allow this so that, if used with that trigger, it just works, or if we should change anything else.

[jckimble]

I think if anything it's a security implication cause pull_request_target have write permissions to the target repository when pull_request doesn't. In theory a forked repo can use pull_request_target to modify a parent repo but in the case of dependabot updates I don't believe it would be an issue.

Edit: It should just work since pull_request_target is just pull_request with more permissions. I can create a test repo to double check and submit a pull request if this is correct

[climba03003]

pull_request_target would not reflect the latest commit as you thought.
It is always the Last commit on the PR base branch not Last merge commit on the PR merge branch refs/pull/:prNumber/merge branch.

So, when you run the unit test to ensure everything is ok, it is not.
I am against on supporting pull_request_target.

[Uzlopak]

I am also against this feature.

Pull_Request is an event which for security reasons does not run modified and potential malicious workflows. Pull_request_target has security implications.

[simoneb]

I think if anything it's a security implication cause pull_request_target have write permissions to the target repository when pull_request doesn't. In theory a forked repo can use pull_request_target to modify a parent repo but in the case of dependabot updates I don't believe it would be an issue.

Edit: It should just work since pull_request_target is just pull_request with more permissions. I can create a test repo to double check and submit a pull request if this is correct

I don't think that pr_target is the same as pr with more permissions, it's a rather different behavior. As I commented previously, I am not saying that it will not work, but it will affect how this action should be used.
As an example, we currently recommend using this action in the same workflow where you run your CI, e.g. automated tests. The reason for this is that it makes sense that we try merging dependabot PRs only when CI is green, and this is the simplest way (although not the only one) to do it. If you use pr_target, the workflow will run in the scope of the base repository, so it won't "see" the version of the source code in the dependabot PR, meaning that running CI on it is useless. This is the most obvious implication, there are probably others.

Did you have any specific reason why you wanted to run this in pr_target?

[jckimble]

Seems I need to read up more on pull request target then, I knew everything ran on the base repository but I wasn't aware the source code from the pull request wasn't pulled also.

Mostly for that I was using pull request target in the past with other dependabot auto merge bots, and didn't want to have to redo actions for it. I have already rewritten them to use pull_request a week ago so it's not really needed at this point personally short of making it 'just work' if for some reason I have to use pull_request_target again cause there was a reason I had to use it in the past I just can't remember why I discovered it in the first place.

[simoneb]

Shall we close this issue then?

[max-sixty]

FWIW I would have expected that pull-request-target would be the natural place for this action, given that it's doing privileged things.

From GH's blog: https://github.blog/changelog/2021-02-19-github-actions-workflows-triggered-by-dependabot-prs-will-run-with-read-only-permissions/

If your workflow needs to have a write token or access to secrets, you can use the pull_request_target event; however, please read Keeping your GitHub Actions and workflows secure: Preventing pwn requests to better understand the risks.

That said, because dependabot is on the repo rather than a forks means it can do privileged things anyway, so it doesn't make a big difference...

[climba03003]

FWIW I would have expected that pull-request-target would be the natural place for this action, given that it's doing privileged things.

Have you really read the whole conversation? pull-request-target is run on PR base branch that means all the automerge action is tested against the base branch (most likely the main branch).
So, you can never ensure the merged dependent is non-breaking inside the same workflow.

That said, because dependabot is on the repo rather than a forks means it can do privileged things anyway, so it doesn't make a big difference...

pull-request-target is granted read / write by default because it is run on the existing code (PR base branch) not the PR code itself. So, it can guarantee the harmful PR content will not run with write permission.

[max-sixty]

Right, — the build would still need to run on pull_request — but the approval could run under pull_request_target — e.g. with use-github-auto-merge: true

[climba03003]

approval only can be done in various of method.
Combination of use-github-auto-merge is dangerous because it requires the user to provide the correct configuration in first place. But if it went wrong, it is same as merging all PR from dependabot without verify.

I can see the reason, but we need to provide extra check to ensure the user is not misconfiguration.

[diranged]

Have you really read the whole conversation? pull-request-target is run on PR base branch that means all the automerge action is tested against the base branch (most likely the main branch). So, you can never ensure the merged dependent is non-breaking inside the same workflow.

@climba03003,
This is true by default, but there are very simple ways around this... the following configuration is how you run a pull-request-target: {} triggered Github Action against the code from the actual pull request:

      - name: Checkout
        uses: actions/checkout@v3
        with:
          ref: ${{ github.event.pull_request.head.ref }}
          repository: ${{ github.event.pull_request.head.repo.full_name }}

This pattern is extremely common on sensitive repos ... you ensure that the build flow itself is coming from your trusted main branch, but you run that flow against the code in the user supplied pull request.

At the end of the day, I don't see why this particular action should have an opinionated view of which types of triggers should be allowed to be used... let the repository owner dictate that based on their needs, and just be flexible would be my preference.

[simoneb]

@diranged what's the benefit of using pull_request_target then pull the code from the PR branch? I'm actually not even sure that the code you showed above works. If the workflow is triggered by pull_request_target, are you sure that github.event.pull_request is even populated?

[diranged]

@diranged what's the benefit of using pull_request_target then pull the code from the PR branch? I'm actually not even sue that the code you showed above works. If the workflow is triggered by pull_request_target, are you sure that github.event.pull_request is even populated?

Here's the primary motivation: dependabot/dependabot-core#3253

[simoneb]

I see your point but after having used this action extensively, I've never actually experienced that issue. Here are the reasons:

• this action is expected to be used in CI, it's usually unlikely that in CI you need to access secrets because you're only building and testing your software
• first time contributions to projects, and in general forks, will require approval before any workflow in a PR are executed
• github's token permissions have been for quite a while restricted

While I understand that these are not necessarily strong enough safety nets, I would like to understand and validate that the approach you propose actually works.

As I asked in the previous message, are you sure that github.event.pull_request exists if the workflow is populated by a pull_request_target trigger?

[diranged]

@simoneb,
Regarding it's usually unlikely that in CI you need to access secrets because you're only building and testing your software - the issue we run into is that we are using private repos and installing libraries from other private repos (private github hosted NPM), and that is where the requirement comes from to use the pull_request_target pattern.

About the github.event.pull_request - you can see Github even talk about it at https://securitylab.github.com/research/github-actions-preventing-pwn-requests/. We use this pattern a lot for our internal private repos - where we trust our employees, and this makes sense as a reasonable tradeoff. For a public facing repository, this is indeed a scary footgun.

For this Github Action - I'd just like to see it be an option to change pull_request to pull_request_target..

[simoneb]

Okay understood, thanks for clarifying. We never built this with that intent but I can see its usefulness.

Would you mind sending a PR? I don't think this would require many changes to the source code, it will be mostly documentation to show the new use case.

[diranged]

Okay understood, thanks for clarifying. We never built this with that intent but I can see its usefulness.

Would you mind sending a PR? I don't think this would require many changes to the source code, it will be mostly documentation to show the new use case.

I haven't yet had a chance to test this - but I believe #500 will work.

[github-actions]

🎉 This issue has been resolved in version 3.10.0 🎉

The release is available on:

• GitHub release

Your optic bot 📦🚀