Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have a mechanism to manually run automerge checks for PRs opened by non-authorised users/bots #116716

Open
giordano opened this issue Oct 6, 2024 · 4 comments
Labels
CI continuous integration

Comments

@giordano
Copy link
Member

giordano commented Oct 6, 2024

Currently Automerge checks are run only for PRs opened by a limited list of authorised bots. There are some users who host their code on services different from github.com and gitlab.com for whom using JuliaRegistrator or the JuliaHub services isn't an option, however this means that

  1. they need to manually open PRs to this repository (presumably they're using LocalRegistry.jl which at least makes this process simpler)
  2. when they open the PR to General, Automerge doesn't run. While this is reasonable because Automerge is meant to be run only for PRs proposing new versions or new packages, arbitrary PRs shouldn't trigger automerge checks, this makes life for the registry maintainers harder

I think we should be able to have a mechanism (comment-based? adding a label?) to let the repo maintainers trigger Automerge checks for PRs that they evaluate are safe candidates for Automerge checks. One challenge is that the result of the automerge check should be invalidated if the PR is later modified (e.g. by pushing a new commit), to prevent malicious action.

@giordano giordano added the CI continuous integration label Oct 6, 2024
@GunnarFarneback
Copy link
Contributor

safe candidates for Automerge checks

Safety isn't that hard for Automerge to assess itself. It could e.g. require that some key information is given in the PR body, then call RegistryTools itself to see if it gets the same file changes as in the PR.

Another question is whether the PR author should be allowed to make registrations for the package. That could e.g. be solved by having a file in the package repo listing approved users.

@DilumAluthge
Copy link
Member

One challenge is that the result of the automerge check should be invalidated if the PR is later modified (e.g. by pushing a new commit), to prevent malicious action.

AutoMerge has two "components": the PR job that runs on the PR and decides whether the PR "passes AutoMerge" or "fails AutoMerge" (but doesn't actually merge anything), and the cron job that goes through the list of PRs and does the merges. The way that the two components communicate with each other is via the automerge/decision commit status. The PR job creates the automerge/decision commit status, and then the cron job reads the value of the automerge/decision commit status.

Now, if I recall correctly, when the AutoMerge PR job creates the automerge/decision commit status, we tie the commit status to a specific commit hash. So if a new commit is pushed, the old automerge/decision commit status will be associated with the old commit, not the new commit (if I'm remembering everything correctly), and thus the new commit won't be able to get automerged.

So, I think the existing decision of AutoMerge already takes care of this aspect of the OP.

@DilumAluthge
Copy link
Member

DilumAluthge commented Oct 21, 2024

safe candidates for Automerge checks

Safety isn't that hard for Automerge to assess itself.

The registry maintainer needs to manually assess safety themselves prior to triggering AutoMerge.

Otherwise, someone can just open a PR modify the manifest files in .ci/ to use a malicious fork of RegistryCI.jl. The registry maintainer triggers AutoMerge, the AutoMerge GitHub Actions job runs, it runs the code in the malicious fork of RegistryCI.jl, and then (because the AutoMerge GitHub Action job runs with a GITHUB_TOKEN that has write access to the General registry), the malicious codebase steals the GITHUB_TOKEN, and the attacker has compromised the General registry.

@GunnarFarneback
Copy link
Contributor

GunnarFarneback commented Oct 21, 2024

Good points. Somehow it seems like you would like to run the CI checks from a different repository, to separate the testing infrastructure from the registry content. I have no idea what token nightmares that would lead to or whether it's technically possible at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI continuous integration
Projects
None yet
Development

No branches or pull requests

3 participants