Proposal: Allow CI to run tests safely for PRs from non-maintainer of stripe-samples

[hibariya]

Hi, I have a proposal to improve the CI. I appreciate your comments.

The Problem

Currently, we cannot run CI properly for PRs from non-maintainers since they don't have permission to read the secrets which are mandatory to run the tests. Because of that, we have to merge first before checking the result of the CI and it's inconvenient for everyone.

A Possible Solution

I think we can change the current CI workflow and make those failed CI jobs retryable with the right permission for only maintainers.

• Change to trigger CI with pull_request_target event instead of pull_request so that the job can read the secrets.
• Because pull_request_target triggers jobs with write permissions, that cannot be granted unconditionally. We make the workflow check if the user who triggered the event is in the list of the maintainers before running the tests.
  • We create and maintain a list of maintainers who can run tests.
  • If the job wasn't triggered by one of the maintainers, abort before running the tests.
  • If the job was triggered by one of the maintainers, run tests as ever.

The following chart illustrates how CI runs tests or aborts.

flowchart LR
A[Start CI] --> B{Triggered by a maintainter?}
B -->|Yes| C[Checkout Code]
C --> D[Run the tests w/ write permissons]
D --> E[End]
B -->|No| F[Abort]
F --> E

The workflow will be like the screenshot below. All the jobs will depend on the require-permission job and this job checkus if the job was triggered by a maintainer.

An Implementation

I created a PR to implement it: https://github.com/stripe-samples/accept-a-payment/pull/1612

How to Use It: A Typical Scenario

1. A non-maintainer opens a pull request to stripe-samples/accept-a-payment
2. CI runs but it aborts before running the tests since the pull request author is not in the mainainers list (like this)
3. One of the maintainers review the pull request and confirm it's harmless
4. The maintainer re-runs the aborted CI run
5. CI runs again and since the job is triggered by one of the maintainers this time, the CI runs all the tests with right permissions

Working Examples

:memo: Note that in my fork, I made myself (hibariya) as a maintainer for testing.

This example shows that if a non-maintainer opened the PR, the CI aborts. The CI is failing on the require-permission job. https://github.com/hibariya/accept-a-payment/pull/1315

This example shows that if a maintainer re-ran the aborted CI like the above, it runs tests. You can confirm that the latest run is triggered by a maintainer (me) here. https://github.com/hibariya/accept-a-payment/pull/1317

This example shows that if a maintainer opened the PR, the CI runs tests as ever. https://github.com/hibariya/accept-a-payment/pull/1316

This example shows that if a maintainer pushed commits to the main branch, the CI runs tests as ever. https://github.com/hibariya/accept-a-payment/commit/3bc03abd4754134c3ad8ed8b6f3aeeea85400cbb

The List of Maintainers

I'm thinking of maintaining the list as an ordinary environment variable in the ci.yml so far.

Another Solution: "safe to test" label

This article suggests using labels to run jobs triggered by pull_request_target safely.

https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

However, it also says it should be a temporary solution since it could lead to race conditions.

Note that this kind of label based verification is still prone to a race condition in which the attacker may push new changes after the workflow was approved (labeled), but has not started yet.

The proposal of this issue does not introduce these kinds of race conditions since it requires retrying the job that is associated with a known commit.