Should this be under announcements? It might have more visibility there.
Thanks for pushing this through!
And @zimbatm as well!
Now moved to announcements
This wasn’t the case before? Glad this security risk is patched
How was it a security risk? The same people can now create a pull request and immediately self-merge it.
EDIT: I mean, perhaps the RFC or some other place should state those security improvements, if significant.
The RFC does: https://github.com/tweag/rfcs/blob/709c8979ece291291ff12da8e206cb212a14652e/rfcs/0156-no-direct-pushes.md#motivation
I still don’t really see “security risk” improvement in there, I’m afraid, but I suppose I’m nitpicking about details of that formulation.
I agree that it will make somewhat better visible who pushed a change, which could dissuade potential malicious pushes/merges. Though there’s a catch that GitHub often misattributes who merged a pull request. (It happened to me many times that push-merge by me gets marked as merged by someone else who pushed around similar time. I reported it to support at least twice over the years, but clearly they don’t care enough.)
And to be clear, I do agree with this change in protection, at least from what I know.
At the very least it helps prevent embarrassing mistakes, so easy to accidentally push to the wrong branch sometimes… And I suppose accidental pushes could at least in theory result in a security mess (“oops, I just accidentally disabled everybody’s firewalls while mucking about on a feature branch trying to make the default nftables, my bad”).
I don’t see this change as a security improvement on its own, but it unblocks many security improvements that can’t happen otherwise (e.g. enforcing a 2-person rule for code getting merged to nixpkgs, and in the future requiring CI which can be used to ensure security critical properties stay upheld and prevent mistakes).
s/security/safety/ I guess
What branches are affected?
The
master
andrelease-*
branches that are consumed by Hydra.
Were the nixos-*
branches already protected in some way?
Yes those branches can only be pushed to by the channel updater scripts (@zimbatm confirmed this when I asked the same question)
To be honest, given the strong focus on PRs in nixpkgs, I was surprised to learn that this wasn’t already the case (I was even shocked to learn that there were up to 58 commits not associated with PRs in the last year!).
Given that it is very clear a majority of those are only in the list because GitHub fails at data consistency (the majority of commits not associated with a PR… are signed by GitHub as web merges of PRs, but these are not counted in the 58), you have an option to believe all 58 are GitHub glitches, not direct pushes! Especially if this is a more comfortable thought. (You do not have an option to believe GitHub has no glitches, sorry)
I believe those are non-false positives direct pushes, collected by @Infinisil manually.
At least, I can say for sure that my commits there are direct pushes.
The first step is to ask GitHub what it considers direct pushes. Out of between a hundred and a couple hundred, @Infinisil has looked at the commits and discarded obvious false positives (more than half). Thanks for the confirmation that not all the remaining cases are false positives!
Nit that I’ve run into: it’s impossible to push multiple merges at once. Even if pushing the exact same commit hashes succeeds when done one by one. It’s certainly not an issue. Otherwise everything works fine for me and I’m glad that I’m not getting those spurious direct-push alerts anymore.