Prevent failures for PRs from forks

[rekmarks]

#37 attempted to fix this action updating the commit status when in the context of a PR from a fork. #42 reverted this, as the implementation did not work.

We can still fix this action for PRs from forks, by using the pull_request_target even trigger, which runs the workflow in the context of the base repository. However, as discussed in #42, it's not safe to check out untrusted PRs, so we have to either:

1. Split the expected consumer workflows into two, one using pull_request_target and the other using some trigger that runs the workflow in the context of the PR's repository.
2. Figure out how to fetch the git history we need without "fully" checking out the PR branch.

It'd be best if we could pursue option 2, as that would be the least disruptive. GitHub security article here for reference: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

[rekmarks]

Pinging @aloisklink to inform you of our findings.

Also cc: @Gudahtt @rickycodes

[aloisklink]

Hi @rekmarks, I've tested v1.0.2 (which includes #37) on MetaMask/eth-trezor-keyring#106 and it works for non-release PRs.

See https://github.com/MetaMask/eth-trezor-keyring/runs/5268487563?check_suite_focus=true

Do you have any logs for runs where v1.0.2 fails? If it's failing on the git checkout, it's because you also need to update the checkout action in that repo to use head.sha instead of head.ref, e.g. like

action-require-additional-reviewer/.github/workflows/require-additional-reviewer.yml

Line 24 in 90a0877

ref: ${{ github.event.pull_request.head.sha }}

[Gudahtt]

^ Cross-posing the answer to that question: #42 (comment)

Edit: Oh, though it looks like the example given does use head.ref rather than head.sha. We can try that again.

Edit2: Tried changing head.ref to head.sha, to no effect. The job succeeds but the status check still needs to be set.

[Gudahtt]

Perhaps we could store the username of the person who triggered the release in the release PR body rather than as an artifact. That would simplify this a great deal, and would no longer require git history.

[aloisklink]

#37 didn't work since I forgot that there are two status checks (my bad!), one that describes whether the action succeeded/failed, and a 2nd one that is created by the action (and requires extra permissions).

Using pull_request_target should be perfectly safe in this action.
This action never runs git checkout on the PR fork, and so never runs any untrusted code, it only downloads an artifact with the author name in it: https://github.com/MetaMask/action-require-additional-reviewer/blob/main/scripts/download-artifact.sh

The problem is that pull_request_target can't currently be setup to run whenever a PR is reviewed.

Potential fixes:

1. Remove the "Additional Reviews for Releases" status check and instead change "Require Additional Reviewer for Releases / require-..." to be required and to fail (exit 1) when there's not enough reviewers.
   • downside: GitHub might send out notifications for Failure for Release PRs before they've been reviewed (although I'm unsure since the GitHub-Action bot will be the author)
2. Make a 2nd action using workflow_run that has permission to publish status checks, which runs after the require-additional-review action.
   • downside: now you need to split require-additional-review into two different actions, so two different git repos and two different *.yml files in each repo that uses this action.