CI pipeline runs for arbitrary pull requests about asdf HOT 6 CLOSED

If your really concerned with malicious PRs being triggered in this way ASDF should be doing three things:

1. Setting autofix_prs = False in the .pre-commit-config.yaml so that autofixes are not automatically triggered.
2. Using pull_request_target instead of pull_request for triggers, as according to the GitHub docs changes actions so that they only run the action relative to what is currently committed in the repository rather than any new changes to the workflow. This prevents malicious things from being added to a CI job.
3. Adding an if: (github.actor != pre-commit-ci[bot]) to each of the workflows. This prevents the pre-commit.ci commits from being run. These commits should be rebased out in any case, meaning the user or maintainer will be force pushing a new commit in any case.

from asdf.

Thanks for bringing this to my attention. For now I've disabled pre-commit-ci integration and opened an issue: pre-commit-ci/issues#195

from asdf.

This is not a big issue in my opinion, ASDF uses the free github runners provided for all open source software. Many packages with user bases orders of magnitude larger than ASDF use the pre-commit.ci bot without issue, e.g. matplotlib, xarray, scikit-image, and pandas.

I for one do not think this is worth disabling the pre-commit bot. I do suggest disabling the autofix_prs setting for this and other reasons.

from asdf.

Using pull_request_target to checkout code from a PR and run tests is specifically warned against in the docs linked as it opens up more possibilities for malicious PRs.

from asdf.

Using pull_request_target to checkout code from a PR and run tests is specifically warned against in the docs linked as it opens up more possibilities for malicious PRs.

True enough, though most of those dangers can be mitigated by configuring the permissions granted to the workflow to be more strict than the defaults. That is set the permissions to read only.

The larger issue in this case is that this would cause the workflow to be essentially useless for CI as it checks out code relative to the base of the branch rather than the head of the PR.

All this is a distraction from my original point, which is that I do not think any of this is an actual issue.

As was pointed out in the issue you raised with pre-commit.ci, disabling pre-commit.ci does not increase the "security" in any meaningful way. All it does is barely limit the pool of attackers as it basically relies on the assumption that ASDF can "trust" its contributors.

It requires nearly the same amount of effort for an attacker to create a meaningless but useful PR, like correcting minor issues in the docs somewhere, in order to gain the same ability to run workflows or not. Similarly, even "trusted" contributors are susceptible to being unwillingly compromised, which again effectively gains an attacker the same ability to run actions. My point here is that there is little relative "security" benefit to being concerned about this particular case.

Since to my knowledge ASDF does not have any self-hosted runners or any paid runners, ASDF does not have any direct liability for actions being run in this way. So things like "crypto" attacks are basically a problem for the GitHub platform itself, not ASDF aside from possibly reporting such behaviors. The only realistic concern is exposure of secrets, which should be extremely limited as we have the permissions for our workflows configured according to current best practices. Any risk in this vein is the risk ASDF or any other public GitHub organization accepts in order to have any level of automation provided by the platform.

from asdf.

I agree. Opening an issue with pre-commit and GH about this gain-of-permission hack is good. But everyone uses the pre-commit bot and I haven't yet heard of this being a problem (hence you opening a new issue). I think turning off the bot is an overcorrection.
Pre-commit has become a standard and staple of quality code development. I use asdf on the regular and would be disheartened to see the org take active steps to decrease its code quality, readability, and maintainability.
Furthermore, many of my friends in sec-ops use pre-commit. This gain-of-permission hack doesn't worry them. It might worry GitHub, but then they should fix it.

from asdf.