Use pull_request as trigger for workflow instead of pull_request_target
lnist opened this issue · 2 comments
Is your feature request related to a problem? Please describe.
pull_request_target allows a pull request coming from a fork to see secrets. We therefore have a guard in place, where a label has to be added. However, after adding a label, the author can push a malicious commit and steal secrets.
Describe the solution you'd like
When using pull_request as trigger then pull requests from forks should not be able to see secrets. We should verify this, but documentation says so in the last line of this:
We have already checked that pull_request_target does see secrets.
So we should switch to using pull_request, and avoid using secrets in the verification if at all possible. See also https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ . The only secrets we currently rely on for PRs are for slack notifications which I'm not sure we use, and for POLYGON_NODE_URL. We could use a cache or the node instead.
In case we do need secrets the linked post also has guidance on how to split the workflow.
By using pull_request it would also be easier for us maintainers with write access to the repo to make changes to workflows as they would be run in the expected context.
Describe alternatives you've considered
Maybe we could remove the 'safe for test' label with a push trigger. However, this would rely on the order of trigger execution. Alternatively the guard itself should remove the 'safe for test' so that is is only in effect once.
The current solution is documented here: https://www.notion.so/mangroveexchange/Build-system-CI-CD-390c665c844e48ad8a3af2fabc14682d#90be146c0bcf4017bd0aa803b2b027bf
This documentation should be updated if the solution is changed.
PRs similar to the one to mangrove-core have been applied to all our main repos and doc updated.