ci/README: add github-script policy#453985
Conversation
infinisil
left a comment
There was a problem hiding this comment.
My main argument against this is really that everybody is already familiar with bash just because it's so prevalent in Nixpkgs. So requiring JS instead makes it much harder for people to contribute.
You do make good arguments for JS. A large part comes from the fact that you already spent so much effort on it though, when much would've also worked for any other language, including bash.
9de7625 to
be1d034
Compare
I extended the commit message reasoning / PR body with some new points, one of them addressing this as well. I don't think that's actually true. A lot of people are familiar with bash to write builder. Few people, if any, are familiar with writing safe Bash code for CI purposes. Also, it's not only Bash, you need to do a lot of Requiring JS does not make it harder to contribute, I believe. Initially, it is a hurdle to take, especially for someone like you, who is one of the few contributors actually having experience with writing CI in Bash. But on the bigger scale, this doesn't change a bit.
When we started the migration, I had exactly zero experience with I'd like to encourage you and just try it. I'm sure you will eventually be convinced that this is much better. |
|
In addition to everything that Wolfgang argued above, which I agree with, I have to double down on my own perspective about reviewing the new CI code versus the bash CI code. The real challenge is that bash has an extensive surface area of implicit behaviors that interact in non-obvious ways. I need to verify proper quoting around variable expansions, understand whether you’re dealing with the test command The documentation landscape also presents challenges. I often need to cross-reference the bash manual, POSIX specifications, and community knowledge bases to determine the semantics of particular constructs are in the first place... let alone whether they’ll behave differently across environments. Complex scripts with heavy use of parameter expansion, process substitution, and pipeline chains require careful analysis to understand both intent and correctness. This isn’t to diminish bash’s utility—it remains an excellent tool for its domain—but effective code review in bash demands a level of detail-oriented scrutiny and deep language knowledge that exceeds what’s typically required for even notoriously "wat‽" languages like JavaScript. Another significant factor is bash’s limitation to primitive data structures—essentially strings, arrays, and associative arrays. JavaScript provides rich object models where you can encapsulate data with behavior, define clear interfaces, and leverage type systems (even dynamic ones) to make code intent explicit. I see objects with methods, clear data transformations, and structured error handling. In bash, everything ultimately reduces to text manipulation, which means you’re constantly parsing and validating data inline. There’s no For this specific domain, I'm in pretty strong agreement that bash is not the right language for future additions. |
philiptaron
left a comment
There was a problem hiding this comment.
I tried to caveat it to take into account @MattSturgeon's feedback. If it's a feature: let's not implement it in Bash.
Over the last couple of months we have been migrating a lot of the old bash code to JavaScript, which is supported in GitHub Actions via `actions/github-script`. This change documents a "manual ratchet check" for this migration - new code should only be introduced as JavaScript and not as Bash. This will help us to eventually succeed with the migration and ensure quality and maintainability. We are migrating to JavaScript, because: 1. Using JavaScript is GitHub's [recommendation] against injection attacks. Using `actions/github-script` has first-class support for the event context and does not require to resort back to environment variables in most cases. When environment variables need to be used, these are accessed via `process.env`, without a risk for accidental injections. Using `actions/github-script` is also recommended in a recent [survey] of open source supply chain compromises: > Finally, since two out of three compromises were due to shell injection, > it might be safer to use a proper programming language, like JavaScript > with actions/github-script, or any other language accessing the context > via environment variables instead of YAML interpolation. 2. Handling even environment variables in Bash safely is almost impossible. For example arithmetic expressions cause arbitrary code execution vulnerabilities. While a lot of contributors are somehwat familiar writing Bash code for builders, writing *safe* Bash code for CI is a very different matter. Few people, if any, know how to do this. 3. GitHub Action's security model is quite unintuitive and even if some code runs with trusted inputs today, it may later be used in a more exposed context. Instead of making judgement calls about language choice case by case, a clear policy helps writing things defensively from the beginning. 4. We have developed a framework around our github-script based tools in `ci/github-script`. This provides a local `nix-shell` environment with the right dependencies and a local runner for these scripts for quick testing, debugging and development. No matter, whether you're developing a new feature, fixing bugs or reviewing a PR - this allows much quicker verification of the scripts, *without* running everything in a fork or test organization. 5. This framework also provides helpers for challenges that come up with GHA. One example is rate-limiting, where we have a helper script that will handle all rate-limiting needs for us, preventing us from running out of API calls and thus breaking CI entirely. We can only use these tools consistently, if we consistently use JavaScript code. 6. Using JavaScript allows us to handle JSON natively. Using `octokit/rest.js` provides first-class integration with GitHub's API. Together, this makes these scripts much more maintainable than resorting to `gh` and `jq`. [recommendation]: https://docs.github.com/en/actions/reference/security/secure-use#use-an-action-instead-of-an-inline-script [survey]: https://words.filippo.io/compromise-survey/
be1d034 to
ce8c42d
Compare
|
Two approvals + more feedback in the form of reactions (also by the security team). Let's do it. |
|
Successfully created backport PR for |
Over the last couple of months we have been migrating a lot of the old bash code to JavaScript, which is supported in GitHub Actions via
actions/github-script. This change documents a "manual ratchet check" for this migration - new code should only be introduced as JavaScript and not as Bash. This will help us to eventually succeed with the migration and ensure quality and maintainability.We are migrating to JavaScript, because:
Using JavaScript is GitHub's recommendation against injection attacks. Using
actions/github-scripthas first-class support for the event context and does not require to resort back to environment variables in most cases. When environment variables need to be used, these are accessed viaprocess.env, without a risk for accidental injections. Usingactions/github-scriptis also recommended in a recent survey of open source supply chain compromises:Handling even environment variables in Bash safely is almost impossible. For example arithmetic expressions cause arbitrary code execution vulnerabilities. While a lot of contributors are somehwat familiar writing Bash code for builders, writing safe Bash code for CI is a very different matter. Few people, if any, know how to do this.
GitHub Action's security model is quite unintuitive and even if some code runs with trusted inputs today, it may later be used in a more exposed context. Instead of making judgement calls about language choice case by case, a clear policy helps writing things defensively from the beginning.
We have developed a framework around our github-script based tools in
ci/github-script. This provides a localnix-shellenvironment with the right dependencies and a local runner for these scripts for quick testing, debugging and development. No matter, whether you're developing a new feature, fixing bugs or reviewing a PR - this allows much quicker verification of the scripts, without running everything in a fork or test organization.This framework also provides helpers for challenges that come up with GHA. One example is rate-limiting, where we have a helper script that will handle all rate-limiting needs for us, preventing us from running out of API calls and thus breaking CI entirely. We can only use these tools consistently, if we consistently use JavaScript code.
Using JavaScript allows us to handle JSON natively. Using
octokit/rest.jsprovides first-class integration with GitHub's API. Together, this makes these scripts much more maintainable than resorting toghandjq.Context
This came up in #450864 (comment). The migration has been happening in practice for some months already, but since it was mostly myself who was working on the code, it was never formalized.
This PR seeks to clarify the situation, asking specifically for input of the remaining @NixOS/nixpkgs-ci team and for additional input from @NixOS/security.
Things done
Add a 👍 reaction to pull requests you find important.