So now anyone with a slightly used account can raise a PR from their fork that spits your secrets out to STDOUT and it will automatically be run because they’re not “new to GitHub”?
GitHub doesn’t expose repo secrets to workflows from forks. This approval “feature” is relatively recent, GitHub actions and secrets predate it, and it is unrelated to your repo security. If this actually did work this way wrt secrets, that’d be a gaping security hole with the default option as well.
Ah fair enough. I think it does work that way for private repos, but then you have to gate access by inviting people to the private repo which is totally acceptable at that point.
Seems there’s a difference in behaviour for public repositories from reading the docs - I hadn’t taken that on board looking this feature up originally. Thanks for the clarification.
Implications of the two settings seem fairly obvious to me:
with the default, first contribution to the project is a dance of pinging the maintainer after every change and waiting a human RTT, just to check if CI doesn’t complain about formatting or some such. Given that a lot of contributions are first-time-contributions (for this project), and that the first experience is a major deciding factor for whether you’ll get a recurring contribution, placing this gigantic road block right there is obviously wrong!
with the suggested option, it works more or less the same way it always used to work: someone opens PR, and, by the time you notice it and start a review, the CI is already green, and there are a bunch of “fix tests” commits present.
I don’t think this is really debatable :-) It is of course possible to rationalize some bizarre, wrong reason for why it works this way (eg, that it somehow makes the secrets more secret or what not), but really this feature is just an outsourced-to-human-maintainers v0.1 spam filter and nothing more.
Originally, GitHub just run the CI for PRs and there was much rejoicing (really, unbelievable amount of rejoicing when it became possible to trash travis.yaml and appveyor.yaml!). Then, later there was a surge of abusing of GitHub’s compute for proof-of-work crypto mining. Basically, you could submit PRs to thousands of repos which replace CI with mining. That, understandably, was a giant pain for GitHub, and they implemented a quick hot-fix by requiring maintainers to approve running CI on PRs. As a hot-fix, this was very much the correct solution to combat crypto spam. As a long-term solution, that’s obviously wrong: you don’t want humans to babysit robots as they work, it should be the other way around.
Some time later, GitHub implemented a proper spam filter, where only “real” accounts can use free CI, which made banning account as a way to combat CI effective again. However, this proper solution:
wasn’t enabled by default
wasn’t even properly advertised (I learned about it only when a contributor to my project told me that this option exists)
To clarify, GitHubs handling of a real problem of CI abuse was quite admirable (to say nothing of GitHub actions existing at all), this wrong default is a really minor point in terms of what is to be done (but a rather huge roadblock for a significant fraction of OSS contributions!).
It’s not only about the secrets, but also about billing.
GH Actions opened on a PR may accrue billing charges for that org/user.
It would also not be very hard to get a GitHub account, open a couple proper PRs, to not be seen as a new user anymore. And then open a PR against a project someone wants to prejudice, such that the action runs for hours.
So being cautious by default seems like a reasonable setting to me.
GH Actions opened on a PR may accrue billing charges for that org/user.
GitHub Actions usage is free for standard GitHub-hosted runners in public repositories
It would also not be very hard to get a GitHub account, open a couple proper PRs, to not be seen as a new user anymore. And then open a PR against a project someone wants to prejudice, such that the action runs for hours.
The user gets reported and banned. “open a couple proper PRs against other repos” is not meaningfully different than “open a typo-fixing PR against this repo”.
The user gets reported and banned. “open a couple proper PRs against other repos” is not meaningfully different than “open a typo-fixing PR against this repo”
Which actually is a bad argument, because it is irrelevant, if we look at the base probabilities. The correct argument is that “users send PR to abuse particular project compute resources” happens very rarely, if at all, while “user sends a PR” happens all the time.
Even in the world where CI for OSS projects costs $, the correct solution would be to just rate-limit CI minutes of a particular user, and have an option of requiring approval for particular rare projects which are prone to targeted abuse in this way.
The motivation for the post is to serve as a “saved reply” for when I contribute to someone else’s project and have to wait until they wake up to get CI feedback on my changes. As such, it intentionally only points out the existence of the option. I don’t want to discuss pros and cons there, as the answer is pretty obvious if you maintain GithubActions based CI.
The project maintainer is paying for that CI run, so it’s perfectly reasonable to gate that cost on their approval. If you want faster feedback loops, you can fork the repo, so that the costs of your work are no longer externalized.
GitHub Actions usage is free for standard GitHub-hosted runners in public repositories
Microsoft pays for these CI minutes, not project maintainers. Maintainers play a role of a free spam filter to reduce Microsoft’s costs. This is still a great deal for the maintainer, but it can be Pareto-better with a small tweak of the defaults.
I believe there’s no limit for public repos, only ToS (that is, you might get banned if you clearly abuse the system, but otherwise run however much you want)
If a resource (GitHub Action runs) has costs that are produced by one group of actors (arbitrary contributors) but paid for by a different set of actors (the repo owner/maintainers) then what you’ve got is a textbook tragedy of the commons. That dog ain’t gon’ hunt.
Hmm what’s the general philosophy on contributing from upstream/project vs andyfork/project ?
That is, do most people send a PR from the repo itself, or a fork of it? I don’t know which I would choose personally – it seems like an unnecessary choice Github forces on you, which has weird consequences.
I have seen the latter, but then contributors can’t run the Github Actions with secrets. Then I have to tell them to send it from the main repo, which is annoying busywork. It’s hard for me to review the PR without seeing the CI results.
Apparently there is NO way to approve it give secrets from andyfork/project, but there is on upstream/project.
That doesn’t really make sense to me? Why is it any different? I feel like the secrets should depend on the USER and not the REPO.
Additionally, it seems to be available for private repos.
To enable these users to run workflows on fork pull requests, we’ve introduced three new settings at the enterprise, organization, and repository level for private repositories only.
So it just seems like a very weird limitation of Github the product. I’m guessing there is some obscure reason, but I wonder if other people accepting PRs on Github, and having CI with secrets, have run into this issue.
For OSS projects, I always contribute from my personal fork, even if I have commit access to the repo. That way, I am in the same boat as first time contributors.
Checks that determine whether the stuff is good to land don’t use any secrets: anyone should be able to run correctness part of the CI.
Jobs that require secrets (eg, making releases) are not checks. They assume that the code is already correct, and only run on the master branch post merge.
That is, I logically split my CI in two parts:
secret-less pre-merge checks, which gate master
post merge jobs which kick once we have new canonical version of code
Ah OK, thanks for the feedback. I would also like to be on “equal footing” with contributors, but right now we use a secret to publish results to a web server.
So now anyone with a slightly used account can raise a PR from their fork that spits your secrets out to STDOUT and it will automatically be run because they’re not “new to GitHub”?
I think the default is spot on personally.
GitHub doesn’t expose repo secrets to workflows from forks. This approval “feature” is relatively recent, GitHub actions and secrets predate it, and it is unrelated to your repo security. If this actually did work this way wrt secrets, that’d be a gaping security hole with the default option as well.
Ah fair enough. I think it does work that way for private repos, but then you have to gate access by inviting people to the private repo which is totally acceptable at that point.
Seems there’s a difference in behaviour for public repositories from reading the docs - I hadn’t taken that on board looking this feature up originally. Thanks for the clarification.
The article lacks an explanation of why this is wrong. This setting being a good default or not is quite debatable.
Implications of the two settings seem fairly obvious to me:
I don’t think this is really debatable :-) It is of course possible to rationalize some bizarre, wrong reason for why it works this way (eg, that it somehow makes the secrets more secret or what not), but really this feature is just an outsourced-to-human-maintainers v0.1 spam filter and nothing more.
Originally, GitHub just run the CI for PRs and there was much rejoicing (really, unbelievable amount of rejoicing when it became possible to trash travis.yaml and appveyor.yaml!). Then, later there was a surge of abusing of GitHub’s compute for proof-of-work crypto mining. Basically, you could submit PRs to thousands of repos which replace CI with mining. That, understandably, was a giant pain for GitHub, and they implemented a quick hot-fix by requiring maintainers to approve running CI on PRs. As a hot-fix, this was very much the correct solution to combat crypto spam. As a long-term solution, that’s obviously wrong: you don’t want humans to babysit robots as they work, it should be the other way around.
Some time later, GitHub implemented a proper spam filter, where only “real” accounts can use free CI, which made banning account as a way to combat CI effective again. However, this proper solution:
To clarify, GitHubs handling of a real problem of CI abuse was quite admirable (to say nothing of GitHub actions existing at all), this wrong default is a really minor point in terms of what is to be done (but a rather huge roadblock for a significant fraction of OSS contributions!).
It’s not only about the secrets, but also about billing. GH Actions opened on a PR may accrue billing charges for that org/user.
It would also not be very hard to get a GitHub account, open a couple proper PRs, to not be seen as a new user anymore. And then open a PR against a project someone wants to prejudice, such that the action runs for hours.
So being cautious by default seems like a reasonable setting to me.
GitHub Actions usage is free for standard GitHub-hosted runners in public repositories
The user gets reported and banned. “open a couple proper PRs against other repos” is not meaningfully different than “open a typo-fixing PR against this repo”.
Which actually is a bad argument, because it is irrelevant, if we look at the base probabilities. The correct argument is that “users send PR to abuse particular project compute resources” happens very rarely, if at all, while “user sends a PR” happens all the time.
Even in the world where CI for OSS projects costs $, the correct solution would be to just rate-limit CI minutes of a particular user, and have an option of requiring approval for particular rare projects which are prone to targeted abuse in this way.
You clearly have strong opinions on this. My original point is that you should have detailed that in your blog post, not here.
The motivation for the post is to serve as a “saved reply” for when I contribute to someone else’s project and have to wait until they wake up to get CI feedback on my changes. As such, it intentionally only points out the existence of the option. I don’t want to discuss pros and cons there, as the answer is pretty obvious if you maintain GithubActions based CI.
The project maintainer is paying for that CI run, so it’s perfectly reasonable to gate that cost on their approval. If you want faster feedback loops, you can fork the repo, so that the costs of your work are no longer externalized.
Again, quoting GitHub docs:
Microsoft pays for these CI minutes, not project maintainers. Maintainers play a role of a free spam filter to reduce Microsoft’s costs. This is still a great deal for the maintainer, but it can be Pareto-better with a small tweak of the defaults.
Yes, but only to a limit, which can be exhausted.
I believe there’s no limit for public repos, only ToS (that is, you might get banned if you clearly abuse the system, but otherwise run however much you want)
There is a limit, especially if your project uses macOS/windows builders as those are billed at multiples of the Linux machines in build minutes.
I have ran out of them on projects I maintain before or projects I’ve forked.
Unless this changed in the last year, it’s still a very real concern. I have billing enabled to get me more CI minutes.
If a resource (GitHub Action runs) has costs that are produced by one group of actors (arbitrary contributors) but paid for by a different set of actors (the repo owner/maintainers) then what you’ve got is a textbook tragedy of the commons. That dog ain’t gon’ hunt.
Hmm what’s the general philosophy on contributing from
upstream/projectvsandyfork/project?That is, do most people send a PR from the repo itself, or a fork of it? I don’t know which I would choose personally – it seems like an unnecessary choice Github forces on you, which has weird consequences.
I have seen the latter, but then contributors can’t run the Github Actions with secrets. Then I have to tell them to send it from the main repo, which is annoying busywork. It’s hard for me to review the PR without seeing the CI results.
Apparently there is NO way to approve it give secrets from
andyfork/project, but there is onupstream/project.That doesn’t really make sense to me? Why is it any different? I feel like the secrets should depend on the USER and not the REPO.
Additionally, it seems to be available for private repos.
https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/
So it just seems like a very weird limitation of Github the product. I’m guessing there is some obscure reason, but I wonder if other people accepting PRs on Github, and having CI with secrets, have run into this issue.
For OSS projects, I always contribute from my personal fork, even if I have commit access to the repo. That way, I am in the same boat as first time contributors.
Checks that determine whether the stuff is good to land don’t use any secrets: anyone should be able to run correctness part of the CI.
Jobs that require secrets (eg, making releases) are not checks. They assume that the code is already correct, and only run on the master branch post merge.
That is, I logically split my CI in two parts:
Ah OK, thanks for the feedback. I would also like to be on “equal footing” with contributors, but right now we use a secret to publish results to a web server.
I guess most people use Github Actions as the web dashboard, but I’ve found that HTML is a lot nicer than plain terminal text … I have a little scripts that generate tables like this: http://travis-ci.oilshell.org/github-jobs/2023-03-02__19-13-01.wwz/_tmp/spec/cpp/osh-summary.html
And they are important to see in the failure case too … hm
Not saying this is better, but I think workflows can have artifacts even if they are from a fork:
https://docs.github.com/en/actions/using-workflows/storing-workflow-data-as-artifacts
So, you CI can publish HTML as a workflow artifact, and then some static page with a bit of JS can pull it from GitHub’s API