migrate danger CI job to GHA

[chris48s]

This PR migrates the danger CI job from Circle CI to GHA.

[chris48s]

This is really the crucial line in this PR and needs some explanation. From the docs:

This event runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does. This prevents execution of unsafe 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 your workflow to do things like label or comment on pull requests from forks. Avoid using this event if you need to build or run code from the pull request.

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

This means:

• We can assign extra permissions to the token used by the workflow (allowing danger to comment on the PR and update check status), but a user can't just push a commit that exfiltrates the token to gain additional priveledges.
• Danger will not run on this PR because the base (master) doesn't have the action on it yet, but we can see an example of another PR where danger has run and posted a comment on the PR:
  • https://github.com/badges/shields/actions/runs/3185380526/jobs/5194890773
  • demonstrate danger js running using github actions #8479 (comment)
    (note the base of PR demonstrate danger js running using github actions #8479 )
• There are certain things we can't do in dangerfile.js (as noted in Recommend using pull_request_target on GitHub Actions danger/danger-js#1060 and https://github.com/danger/danger-js/blob/1e7ee107b3755f208ff1ec3dd393e83045340b35/source/ci_source/providers/GitHubActions.ts#L192-L194 ), but in our dangerfile.js we aren't relying on directly reading the files in the PR as things stand. Everything we do is based on reading the diff/files via the GH API. We don't change this frequently but we may need to bear this in mind if we add further danger checks in future.

On balance, I think this is the right approach for us.

[calebcartwright]

Sounds reasonable, so long as we've got the permissions properly configured in the repository settings (wouldn't want to rely on the in-file permission definition approach)

[chris48s]

Sounds reasonable, so long as we've got the permissions properly configured in the repository settings (wouldn't want to rely on the in-file permission definition approach)

This is a bit fiddly, so I just want to try and clarify this a bit more. Apologies if I am re-stating what you've already understood.

---

If an action runs

on: pull_request

no secrets are available to the build.
There is nothing we can change in the repo settings.
There is nothing we can change in any workflow file.
They are just not exposed to the build.

The code that gets checked out (to lint, or test, or whatever) is whatever the user submitting the PR pushed to the tip of their branch.

This means that even if the user submitting the PR pushes changes to their branch that exfiltrate your secrets, it doesn't matter. They're not exposed.

---

If an action runs

on: pull_request_target

our secrets are available to the build
BUT
The code that gets checked out is whatever is on our default branch, not whatever the user submitting the PR pushed to the tip of their branch.

This means if we have a on: pull_request_target workflow:

• even if the user pushes code that tries to exfiltrate your secrets, it doesn't matter that your secrets are available to the build because the code that is checked out is our code, not theirs (unless you go and explicitly check out their code in the build).
• if we run tests or lint checks or whatever, what we're actually checking is the code on the default branch, not the code in the pull request

The reason why this works OK for this case is because danger isn't looking at what is checked out in the workflow environment - it is just using the GitHub API to look at what files changed in the PR, match patterns in the PR diff, etc so we can look at what has changed in the PR without having the user code checked out or executed.

[chris48s]

Thinking about this in the context of #8535 I am hoping that the combination of

shields/.github/workflows/danger.yml

Lines 3 to 4 in de3b3ce

	pull_request_target:
	types: [opened, edited, reopened, synchronize]

and

shields/.github/workflows/danger.yml

Line 14 in de3b3ce

if: github.actor != 'dependabot[bot]'

should keep things in check.

We might consider tweaking https://github.com/badges/shields/blob/master/dangerfile.js#L104 down slightly as that means a PR modifying 100 files can now eat 10% of our rate limit, as opposed to just 2%.