pre-commit-ci / issues

public issues for https://pre-commit.ci
17 stars 3 forks source link

autofix allows first-time contributors to run CI #195

Closed braingram closed 10 months ago

braingram commented 10 months ago

It was recently brought to my attention that using pre-commit ci allows first-time contributors to trigger the CI on a repository (even if the github setting requiring approval for first-time contributors is enabled).

An example can be found here: https://github.com/asdf-format/asdf/pull/1678 with the action that was triggered (without approval): https://github.com/asdf-format/asdf/actions/runs/6842075656/job/18603164734

The workaround involves opening a PR with a change that will be fixed by pre-commit-ci. The commit added by the bot allows the CI to run (as long as the bot is a prior contributor to the repository).

Disabling autofix appears to only be a partial fix as the first-time contributor can add a comment pre-commit.ci autofix to trigger the work-around.

Is there a setting that can prevent this workaround? I would like to continue using pre-commit-ci but also prevent first-time contributors from triggering the CI.

One possible solution might be to only allow the pre-commit.ci autofix comment to work for prior contributors.

I attempted to make a minimal case here (although it's a bit messy from playing around with various options). My account braingram created a repository with pre-commit-ci enabled: https://github.com/braingram/autofix_workaround this PR opened from a user thisisspammail was not approved to run yet adding the pre-commit.ci autofix comment (with the thisisspammail user) triggered the CI: https://github.com/braingram/autofix_workaround/pull/11 If it's helpful feel free to use https://github.com/braingram/autofix_workaround for testing this issue and please let me know if there's anything I can do to help.

asottile commented 10 months ago

this had been brought up to github last time this was surfaced and they did not provide any meaningful way forward unfortunately

they recommended using a different flow to limit CI usage such as environments or labels

asottile commented 10 months ago

hmm I had maybe one idea which would be something like maintainer_only_autofix: ...

could you elaborate on your use case and why you want to limit contributors in the first place? github originally added that feature as a stopgap for crypto miners but I don't think that's what's happening here

braingram commented 10 months ago

Thanks for the quick response and extra information. pre-commit-ci has been very useful in keeping code clean and up-to-date!

For this use (a small public open source project that doesn't hit limits that would require paying for github runners) the concern is minor. I think a combination of disabling autofix_prs and having a maintainer_only_autofix would solve the issue/concern.

My main concern is for the secrets used in the workflows (and less so for things like crypto mining, usage limits, or paid runners). Preventing first-time contributors from triggering the CI doesn't do much to protect the secrets but if a bug were to be found in permissions github places on these secrets (like this one that was already fixed) limiting the CI runs to existing contributors would limit how easy it would be to exploit this for any given package.

In addition to the maintainer_only_autofix setting, is it worth mentioning this issue that enabling pre-commit-ci allows first-time contributors to run the CI on the pre-commit-ci docs or install message?

asottile commented 10 months ago

limiting to prior contributors doesn't actually make you safer -- just limits the potential number of attackers

given that's the rationale here I'm not really inclined to implement something like this -- if you need to protect secrets or runners then use github's environments feature

in your case I'd encourage adjusting the setting to the new option: requiring users new to github -- which is much less of a pain for contributors and maintainers and really what github should have implemented in the first place to mitigate crypto mining

braingram commented 10 months ago

Thanks for the consideration.

I'm disappointed that this issue won't be addressed as pre-commit-ci has been a useful tool. Is the maintainer_only_autofix option difficult or impossible to implement? I fully understand that there are limitations to what can be done and don't want this to come across as demanding.

FWIW the conditions that cause this issue also means that a user new to github can trigger the CI by making a commit that contains an error that pre-commit-ci will autofix.

asottile commented 10 months ago

it's not that it's difficult -- it's more the principle of it. I'd rather spend my time implementing features for things that actually matter (paying customers, broad improvements, languages, etc.) instead of hacking around github's deficiency

and again the github feature isn't intended as protection for maintainers -- it's a half-assed solution to crypto mining. you can also accomplish what you want via environments (which do implement the authorization and credentials correctly)