Posted by galnagli 20 hours ago
Use zizmor in CI https://github.com/zizmorcore/zizmor
error[template-injection]: code injection via template expansion
--> .github/workflows/jira_issue.yml:24:29
|
22 | run: |
| --- this run block
23 | # Escape special characters in title and body
24 | TITLE=$(echo '${{ github.event.issue.title }}' | sed 's/"/\\"/g' | sed "s/'/\\\'/g")
| ^^^^^^^^^^^^^^^^^^^^^^^^ may expand into attacker-controllable code
|
= note: audit confidence → High
= note: this finding has an auto-fix> The workflow had an if: condition that appeared protective:
> if: (github.event_name == 'issues' && github.event.pull_request.user.login != 'whitesource-for-github-com[bot]')
> However, on issues events, github.event.pull_request is always null. So the condition reduces to (null != 'whitesource-for-github-com[bot]'). This is always true, and every GitHub user passes the gate.
Speaking broadly: it's a massive reminder that AI is trained on a veritable mountain of insecure GitHub Actions examples, many of which "fail open" in highly unpredictable ways even if widely used. Actions is almost unique in this regard, with the combination of a difficult-to-audit language and the type of privileged RCE environment that makes attackers salivate.
(I do think that this stems in part from GitHub's often-inscrutable documentation, and a decision to release Actions without a robust security linting solution, leaving that to the community - but I do understand how it's an uphill battle, and we could have ended up with a much less flexible CI/CD system without this having shipped fast.)
(Source: I am zizmor’s maintainer.)
Nullable event payloads silently null-coalescing to '' are a real "spooky action at a distance" kind of issue, because something that works perfectly when running and being QA'd on PRs, can silently fall apart if made to also run on the main/develop branch (which is only really monitored if the "build breaks," and thus a silent failure or skip might easily sneak through).
Our codebase indeed has comments like this, without which we'd be totally lost:
# Note contains('') is false if there is no PR at all e.g. on a push to develop,
# so this will always run on pushes to the develop branch.
if: ${{ !contains(github.event.pull_request.labels.*.name, 'ci-skip-backend') }}
TITLE=$(echo '${{ github.event.issue.title }}' | sed 's/"/\\"/g' | sed "s/'/\\\'/g")
Even in ruby it would have been something like
TITLE=shell_escape(github.event.issue.title)
where shell_escape could at least have unit tests. I don't know anything about github actions, but anything that takes user input needs to be processed with a language that uses static typing I believe to help ensure that the user input doesn't get into expressions given to other programs, shell, sql, or whatever.
Unix was just not made for this sort of thing. It was made for in-house people to write patent applications.
That's critical for a platform like GitHub and for devops pipelines in general.
The failure is that "data" ends up being a "terrible custom DSL" that is bad at everything: Not good at data, not a good DSL, and not even a proper programming language.
The best approaches I have seen to this kind of thing are:
- Pulumi: You get to run custom code, but it outputs data. In other words, your "build automation script" must be a pure function taking data in and returning data out. The resulting data is then treated as the "thing" that the pipeline executes, which means that all decisions (parameters, inputs, etc...) have to be "baked in", before the pipeline starts executing.
- Google CUE (Configure Unify Execute): lets you build up JSON using a strongly typed constraint language. Great for huge, complex configuration.
Use `env:` instead and just work with environment variables in your shell script.
Yes, you still need to vet your script. Quoting is a common source of problems. Use shellcheck. Do not call eval/source/python/perl/whatever with untrusted input.
But you removed one layer of problems already by not pasting a value into your shell script code directly.
We all know that people make mistakes. I.e. crash a car from time to time.
We all are sold a view that AI will save humanity, cure all diseases, solve all problems, allow for autonomous driving and many other (lies?).
While making basic mistakes or crashing on trivial crossroads..
Hype is way overblown.
Funny of you to mention strawman after presenting one.
AI does it a lot faster and ignores rules even harder than humans do, but it's not the root problem here.
But yes, there is an interesting change in the past decade, where everything new must be over-hyped.
Perhaps it is attention overload and needing to shout. Perhaps it’s that technological progress has significantly slowed while communication options have exploded (coincidence?).
I look at it a lot like EVs. They’re great, if your use case is inside the specific band. But, that isn’t who they were being marketed to. And now… “pushback” is putting it lightly.
Or, it's that the last two years have been the largest and fastest shift in the daily life of a programmer since the compiler, with near everyone moving, simultaneously, to this new tech, not because of hype, but because of practical personal benefit.
Regardless, everything being hyped isn't new. There's always been silly hype in tech.
It's a shame Github is buried under their current server issues, because it would be great to get improvements all of this - at least warning/erroring on these sorts of things themselves.
> Workflows like jira_close.yml use deprecated atlassian JIRA actions and have a dependency on the gh-actions repo. This is not ideal and unecessarily complex. PR updates jira_close workflow to use direct API calls via curl. It preserves custom fields used too.
I won't speak to this projects' management and how they prioritize things, but from my own experience, pre-AI, this type of change would have been firmly in the "this is a minor annoyance, put it in the Tech Debt Backlog alongside the 50000 other tickets" and never actually done. The cost of a human investing the time understanding how to fix the problem, doing code changes, testing them, and deploying them is just way too high for what actual value this change brings, which is close to nothing.
Now with AI, it's as simple as firing up an agent and telling them to make a change; as much effort as writing that backlog Jira ticket in the first place.
Similar to the problem open source is having with low-value PRs, companies are going to have to start realizing that code is not free to review or maintain, even when it's generated for ~free, in their internal processes. Just because an agent can fix a minor tech debt annoyance with a few lines of instructions doesn't mean it should.
[0] https://github.com/snowflakedb/snowflake-connector-net/pull/...
I'm interested in how that turns out 6 months later.
In my team, we have plenty of enhancement requests from users. We address those that make obvious sense and are trivial to do but withhold from others, even though the code change itself is likely small. Because we don't know if there is more than a single user that can actually benefit from it, if it has unintended consequences, or if it causes maintainence issue down the road.
That really is the real question, isn't it?
For me it has been a mixed bag so far, I've seen some companies use this tech in a slow and deliberate manner to do what they were already doing but a little bit faster. I've also seen hail Mary passes where the whole codebase was turned over to agents to go wild on with an undersized team and little to no QA. Time will tell...
Prediction: programming is going to change massively not only because the cost of creating code will go down, but because people are so tired of this sort of gatekeeping "we know better" from programmers.
demand was suppressed by an access bottleneck, the bottleneck is gone, expect a magnitude of change proportional to how much demand was actually pent up rather than proportional to how much programmers currently think was reasonable to deny.
Explicit things I am not saying: gatekeeping was bad, feature creep is good or desirable, etc. etc. etc.
> people are so tired of this sort of gatekeeping "we know better" from programmers
People have to take 'no' for an answer sometimes, even if they don't accept it. You can't really 'gatekeep' your own product.
The work to go from software to usable software system is vast.
I assume the "gatekeeping" decision to not implement a feature request is coming from someone responsible for the product, not from a developer.
In its quest to make markup "human readable", it has created countless footguns.
I honestly prefer XML at this point.
Take CI for example, it was Jenkins and it ran a csh/bash/zsh whatever script and captured the output. Nice and simple (even if the scripts sometimes got insane).
GitHub actions is nothing like that. Weird home grown extensions to YAML with their own idiosyncrasies and dynamically pulling in plugins from god knows where. You can't just take a workflow and execute it locally like you could with a bash script.
How funny that GitHub Actions' lack of tools forces you to make a bunch of billed cloud runs with GitHub for workflow edits. I'm sure their PMs are very concerned about this trend.
Jenkins still exists and it's no less complicated than Github Actions. The complexity gets hidden in obscure script files, obscure tabbed UI, and remote services for doing things Jenkins itself can't do. Defaulting to system tooling makes it almost impossible to predict what a job will do unless you know exactly how paths and tooling are set up (and what versions they're running).
None of my personal experiences with Jenkins had scripts that ran locally, they all relied on pre-installed software on the server because that was the thing people would do before the great YAMLification. You could copy-paste the Jenkins job, but unless you have Jabberwocky v2018.3 installed in /home/JabberWock/RELEASE, the script will fail.
The entire software development flow has been made incredibly complex by hooking up automations into every nook and cranny.
All of these complications need to be enabled manually, though. If your flow is complicated, you can cut it down to manageable size by doing a few more processes manually.
Luckily, all of the simplication and reproduction steps for Github also apply to Jenkins. Github YAML files are just scripts with different syntax, after all. Just like you can run bash locally, you can run act and reproduce whatever Github trigger you need. From there, you can simplify pipelines, stop curl2bashing "plugins", and so on.
In my experience, containerization resolved this issue. Now you just need to be able to "docker run public.ecr.aws/carroll/jabberwocky:v2018.3 [etc]".
steps:
- name: Run jabberwocky
run: docker run -it public.ecr.aws/carroll/jabberwocky:v2018.3 [etc]And I also agree that you could use GitHub actions in a similar way to how I talk about Jenkins. I also think you could use Jenkins in a way that is similar to GitHub actions with an extremely convoluted Jenkins file that relies on Jenkins plugins etc.
But nowhere I worked did that with Jenkins, whereas everywhere I worked with GitHub actions does create extremely complex pipelines relying on 3rd party actions (with many being vendor supplied). I'm sure(hope?) the DevOps folks looked at what each action actually does by going to wherever it's pulled from and read it (and the actions it pulls in).
I'm not necessarily arguing that things are worse now. Just that you don't get a "free" understanding anymore by just having a good familiarity of the Linux userland. When there's a problem, I stick my head in the sand and let the DevOps people handle it.
I also haven't worked anywhere that uses Jenkins in 10+ years, so maybe if I went back, it would be the same as Github.
Or it's an obscure bash oneliner hidden in another repository as a package that you can depend on, which is either wrapped by another YAML config or maybe some JS.
I don't think the criticisms of Jenkins compare because Jenkins is really much more than a CI layer, predates the concept of devops and our modern understanding of CI/CD, and it has a lot of complexity to make that work. That was back when you'd have IT teams provisioning your servers for you and shit.
But that's the core of the problem, isn't it? People setting up devops environments are the reason these flows are so complicated. You can set up the same flows by having Jenkins call out to Jenkins to trigger chains of jobs that can conditionally fail.
People generally don't do it, because there are better alternatives out there for when you want those types of pipelines (which includes Github Actions and their counterparts). Setting up a four-step build+test+scan+tag+push+deploy pipeline in Jenkins is just as hard as it is with Github, if not harder. [Jenkinsfile](https://www.jenkins.io/doc/book/pipeline/jenkinsfile/) is just YAML with extra parentheses.
The modern YAML files are a simplification of the manual steps required to do it all in tools that predated the YAML files. There's nothing stopping you from having a Github runner configuration that's just a call to a single Docker image containing all the bash/perl/Makefile/Kotlinscript/Go/Ruby/etc. files you want to execute. Or you can forego the Docker part and run code directly inside of the repository, with all the security risks that come with it (not dissimilar to having a fully open Jenkins server).
Complicated YAML is a symptom. It's no better or worse than Bash or TOML or Makefile when you start adding conditional automation and multi-step CI/CD flows.
"On the virtues of the trailing comma" https://devblogs.microsoft.com/oldnewthing/20240209-00/?p=10...
At least XML permitted comments, fhe shift to JSON on everything almost makes me nostalgic.
{ "//":"make sure these are divisible by 8", "width": 640, "height": 480 }
It seems like they were thinking of some json parsers that’ll ignore just straight up anything after // before a new line in JSON (looking at you visual studio). So a .NET dev who has never touched anything but Visual Studio may make comments in the appsettings.json for a .NET project and it made all my command line parsers throw errors. Thanks Microsoft.
Better move as much of that as possible into your own scripts. And your scripts can be portable between forges, and even run locally!
How do you specify actions and workflows without interpolation and logic kind sir?
Every time I've tried in the past, something as simple as making a value a list had some nonsense expectations. I can't wrap my head around how that spec got any traction and wasn't laughed off the face of the earth the first time it was looked at by someone who didn't create it.
However, GitHub Advanced Security, a bot, flagged something but not the real issue. So maybe that bot contributed a false sense of security.
Commit: https://github.com/snowflakedb/snowflake-connector-net/pull/...
In any case, I'm pretty sure you misattributed the vulnerability to Copilot because the PR got squash-merged and _all_ of the changes were then attributed to every contributor in that PR, despite Copilot only appearing on one of the commits.
[1] https://github.com/snowflakedb/snowflake-connector-net/commi...
[2] https://github.com/snowflakedb/snowflake-connector-net/commi...
github.event.issue.title is very obviously data. It should never be POSSIBLE to treat that as an instruction.
Furthermore, the idea of any code being able to access the tokens instead of allowlisted software and only with specific commands, and also no housekeeping to prevent the DATA of the token from ever being sent to anything other than a desired host… all of it feels fundamentally wrong.
The fact that our OSes don’t help with that is so saddening.
What I'm seeing now in industry -- and I think this autofix issue is a precise example of it -- is a natural evolution of the "LGTM!" review that's so prevalent in software development and similar disciplines.
For years, the dramatic majority of "code review" was a quick glance followed by "Looks good to me." Sure, critical workflows have more scrutiny. Sure, not everyone fell victim to this trap. Sure, there are many exceptions. But it's a meme for a reason: most people weren't really reviewing code assigned to them. They were effectively rubber-stamping most things.
So now, in the age of AI, those same people are (sometimes still) expected to be responsible for what their automated developer friend Claude is doing. It's absolutely unreasonable to think that most people are giving the PR more than a glance, and in many organizations they're explicitly trying to remove humans from the loop.
One day, AI development and code review will be so good that mistakes like this will be extraordinarily rare. For the near-future, though, I anticipate we'll see more of this before we see less.
I think once we figure out how to tighten the loop of user feedback, expert analysis, automatic/static verification and AI generation then technology is going to make another leap.
It's best to take all these things in moderation.
> Workflows like jira_close.yml use deprecated atlassian JIRA actions and have a dependency on the gh-actions repo. This is not ideal and unecessarily complex.
And then goes on:
> PR updates jira_close workflow to use direct API calls via curl.
Duplicating the logic into OUR codebase via a hand rolled curl, so we can get rid of “needless abstractions”. Auch. And of course the whole thing embedded into a yaml file.
This code is the typical kaleidoscope sometimes written by junior devs (and LLMs). On review you just kindly ask to be rewritten into a simple program or just close it as the effort doesn’t worth it.
> if: (github.event_name == 'issues' && github.event.pull_request.user.login != 'whitesource-for-github-com[bot]')
> However, on issues events, github.event.pull_request is always null.
This is extra dumb because even if you thought this condition was correctly testing the user's identity, it shouldn't have "appeared protective" upon even a moment's thought. If it worked correctly, it would obviously just exclude one bot user while allowing all other users, so it wouldn't provide any protection at all.
But more likely, this condition was never intended to be "protective" at all, and it's only being described that way because the writeup is LLM slop.