ci: Try backporting via pull_request_target

[The-Compiler]

In #9416, @bluetech says:

So the backport workflow doesn't work - permission error: https://github.com/pytest-dev/pytest/runs/4546997247?check_suite_focus=true. It did work for me in a private test repo, so I'll try to figure out why it fails here. Will create a manual backport for now.

And indeed the debug output reveals:

GITHUB_TOKEN Permissions
  Contents: read
  Metadata: read
  PullRequests: read
Secret source: None

which I think is due to this:

When you create a pull request from a forked repository to the base repository, GitHub sends the pull_request event to the base repository and no pull request events occur on the forked repository. [...] With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository. The permissions for the GITHUB_TOKEN in forked repositories is read-only. For more information, see "Authenticating with the GITHUB_TOKEN."

I assume it worked in @bluetech's test because that wasn't a PR from a forked repository?

When using pull_request_target instead, things look different:

This event runs in the context of the base of the pull request, rather than in the merge commit as the pull_request event does. This prevents executing unsafe workflow code from the head of the pull request that could alter your repository or steal any secrets you use in your workflow. This event allows you to do things like create workflows that label and comment on pull requests based on the contents of the event payload.

Note the security warning:

Warning: The pull_request_target event is granted a read/write repository token and can access secrets, even when it is triggered from a fork. Although the workflow runs in the context of the base of the pull request, you should make sure that you do not check out, build, or run untrusted code from the pull request with this event. [...]

But I think we should be fine here: We don't run anything, and this is triggered by labeling PRs, so it should not run on untrusted stuff anyways. For some more context and security considerations, see Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests | GitHub Security Lab

I don't really understand whether the workflow itself will need any changes due to the context now referring to the base - but we can't really know until we try, I guess.

[bluetech]

Thanks @The-Compiler for finding this. I read the article and while it's not particularly easy to understand, I believe this is safe, because: it can only be triggered by trusted users (label), and doesn't actually execute anything from the base branch.

As an added bit of safety, I'd also restrict backporting to only merged PRs, i.e. add

if: github.event.pull_request.merged

Finally, with pull_request_target the checkout in on the target repository not the base repository so I'm not sure whether the git cherry-pick -x --mainline 1 ${{ github.event.pull_request.merge_commit_sha }} part will work, but maybe it will, let's try it :)

[bluetech]

Would add a comment here:

Suggested change

	pull_request_target:
	# Note that `pull_request_target` has security implications:
	# https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
	# In particular:
	# - Only allow triggers that can be used only be trusted users
	# - Don't execute any code from the target branch
	# - Don't use cache
	pull_request_target:

[The-Compiler]

Apologies for the delay! Updated now. As for whether it will actually work or not, I guess we can only find out easily after merging...

[nicoddemus]

It worked: #9474 🎉

However I did had to add the label after it was merged, is that expected? Seems more convenient to be able to add the label while the PR is still running in CI.

[The-Compiler]

Yep, I was thinking about that as well - that would probably need to be a slightly different trigger (or even a separate workflow) though, which triggers when a PR is merged and then checks that PR's labels. I think ideally we would have both, so that the labels can be applied at any time and the proper thing will happen.

@bluetech is this something you'd be interested in? Personally I should take care of some dayjob stuff at the moment, so pytest 7 keeps me busy enough 🙂

[nicoddemus]

Another problem is that CI is not running for it 😕

[nicoddemus]

Another problem is that CI is not running for it

Closing and reopening the PR is a workaround

[bluetech]

Regarding the merged requirement, I suggested it out of caution because I'm not 100% sure which commit the github.event.pull_request.merge_commit_sha refers to at this point. Maybe it's also safe and working and we can allow it...

Regarding CI not running, right - CI doesn't run when triggered this way - this also happens for the create-pull-request actions and similar. Workaround is to close & reopen, then it triggers. It's also possible to use a user token (pytestbot), but that has its own security considerations.

[The-Compiler]

It's not as easy as removing the if condition, unfortunately - if the label is applied before the PR is merged, we should not backport the PR immediately (it's not even merged yet!), but pick it up once the PR gets merged.

As for using a pytestbot token, I'd be in favor of doing so. The risk seems manageable given that we already need to be careful about not running code from the PR and as we're not using third-party actions.