CI pipeline runs for arbitrary pull requests

[eschnett]

I just submitted my first pull request for your project #1678. Your CI pipeline started and analyzed my changes, reporting errors.

I don't think this is what you intended. I think you chose the "require approval for first-time contributors". However, this is not what is happening for me.

I have the suspicion that [pre-commit.ci] auto fixes from pre-commit.com hooks is the culprit. When I commit a change that needs fixing, e.g. using single quotes for strings, then this stage triggers a commit, which triggers a new CI run, which starts automatically since this is now a trusted commit.

I confirmed this:

• Push a change with bad formatting: CI runs
• Push a change with good formatting: CI is waiting for approval

[braingram]

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

[WilliamJamieson]

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.

[WilliamJamieson]

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.

[braingram]

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.

[WilliamJamieson]

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.

[nstarman]

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.