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
> 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.)
zizmor wouldn’t catch that condition at the moment, although it does have similar checks for other unsound conditions and incorrect/vulnerable bot actor checks. This one wouldn’t be too hard to add, though.
That would be awesome - and thanks for your work, I'll want to start using it!
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') }}
Frankly it's absurd that you don't get a null reference error in that case. I knew Github Actions was bad... I didn't know it was that bad. This is 90% Github's fault.
Shell scripts on their own already are so perilous without static analysis. I'll never understand how we ended up deciding that embed them in yaml instead of requiring an external script file was a reasonable idea.
Bash script embedded in yaml. Wow, I'd view each of those with heavy suspicion on its own, and they're nonlinearly bad together because of how special chars interact.
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.
YAML seems reasonable because it allows the sequence of steps to be treated as data, which then enables GUI visualisation, introspection, etc... without actually having to run anything.
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.
I get scared when I see these string interpolations in GitHub Actions.
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.
It can be that while also causing problems. It's like when a self driving car crashes, it's big news and everyone runs around like their pants are on fire. When a human driver crashes, it's a blip on local news or not even that.
When a human driver controlling a bus crashes though it's a big story. Even bigger when it's a train. News scales with novelty and people impacted - 1 driver being a problem isn't an issue, an ai system used in many cars... That is more newsworthy.
Probably because there are 1000x or whatever human driven miles compared to AI. And no one wants to be crippled by an AI have have to fight $billon_dollar_corp for compensation.
The anti-hype is also way overblown. Nobody is saying that today's AI is omniscient and never makes mistakes. Future models will make fewer mistakes. They will still not hook into the universe with a cheat code and do everything at 100 percent reliability. It's a straw man.
The hype has significant real-world consequences such as nonsensical amounts of funding and valuations that could crash the world economy at some point. Until there some "check" on this system, i think the anti-hype is quite valid.
Sadly I'm not. Either way, how LLMs work mean that traditional software analysis tools are every bit as important as they were in the before times. This is why we see a lot of hype around LLMs and formal verification.
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.
> But yes, there is an interesting change in the past decade, where everything new must be over-hyped.
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.
Github Actions is actually so incredibly scary to have on public repo. It's full of so many footguns that's far from obvious.
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.
Would it be fair to say the blame falls squarely on Github? Why do they even allow pasting of arbitrary strings from a title directly into a script? And if they feel there is a reason, what did they imagine the safe way to do it was?
It's interesting to look at what was being attempted when the vulnerability was introduced[0]
> 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.
I have seen plenty of "my backlog has never been shorter" comments here.
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.
> I'm interested in how that turns out 6 months later.
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...
> but withhold from others, even though the code change itself is likely small.
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.
What you're describing as "gatekeeping" is actually Product Ownership and should be applauded, because the alternative is a product owner who abdicates responsibility to the customer.
Lots of responses to a comment I explicitly did not make. Let me try and spell it out for you folks:
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.
Can you explain to all of us how this gatekeeping is actually bad? Software would be way, way worse if we just allowed any arbitrary feature to be added to software, and ordinary people who want all the things would have to learn, the hard way, that doing all the things is not actually good.
Thank you. I'm spending a lot of time reminding my peers that cheerlead this sort of thing that feedback is usually presented to us a solution, not the underlying problem and that our job is to solve problems, not just vomit out the requested solution.
> 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.
I assume the "gatekeeping" decision to not implement a feature request is coming from someone responsible for the product, not from a developer.
Yeah, the YAMLification of everything kinda killed my ability to understand "everything". Previously, if you knew the Linux userland well, I felt like you could figure anything out with enough digging.
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.
Lack of a local runner is my biggest peeve. Any novel GHA workflow creation results in a PR with 100 commits, until you can finally sort out all the non-obvious idiosyncrasies. For any modestly complex workflow, I move everything to a bash or TS file and call that, and then you can use _coding tools_ and a _local_ dev/eval loop.
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.
A local runner would certainly help, but only to some extend. When you need to support multiple OS (Windows, Linux, macOS) and architecture (x64, arm64), not everything can be tested locally.
And worse: GitHub Actions not a full-fledged programming environment by itself either, so you're inevitably going to have to deal with nontrivial shell scripts on top of all the YAML mess.
Yeah. It makes sense when you're standing right next to it, but you take a step back and go "that doesn't look right". GitHub actions is the faster horse instead of a car.
The things done in yaml today would've been obscure bash oneliners had YAML not existed.
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.
You are not wrong, Jenkins had many problems. But I would argue that the solutions were always grounded in the Linux userland.
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.
Except that YAML in almost every devops or CI workflow is YAML plus obscure bash oneliners.
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.
In a similar vein, JSON's lack of comments makes me marvel at how consistently JavaScript seems to choose the worse option. I'm oh so glad it found its way into config files
As a data/serialization format (the original intended use of JSON), I think no comments is the right choice, since adding comments introduces a bunch of questions about how comments ought to be parsed into a datastructure (or if they should be sent/(de)serialized at all). But for config files the lack of comments is the wrong choice, since typically you want to explain why things are set the way they are. The lack of ordering of entries also makes total sense for data/(de)serialization, but makes no sense for a config. I think the problem is people are lazy and just want to JSON.parse() everything, and used JSON in places it doesn't belong, like config files.
Many JS toolchains will take js files as config to alleviate this problem. Which is actually nice if you ever need dynamic configs, cause that means not using some forsaken DSL. It still evaluates to JSON in the end.
Not the person you’re replying to but you were clear in what you were describing.
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.
I never figured out how the hell to write YAML and I definitely won't now that I trust the AI to do a better job than me. It's so unintuitive.
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.
I don't get it either. There was a point where I had to write YAML, but it wasn't intuitive to begin with, and months in I was still accidentally setting the wrong number of indents or some nonsense.
When I see YAML in a product tech stack, I know that the developers have probably made other similarly poor life decisions and try to steer clear of the entire iceberg.
Textproto is such much better than YAML for everything that YAML is used for. Sure, JSON has plenty of uses, but YAML is specifically used for configs that it's terrible at.
The first linked PR (#1218) has only one commit co-authored by Copilot and it's not related to the vulnerability, and neither are the other suggestions in the PR. Am I missing something?
I must be too tired because I cannot figure out what happened in that pull request. The PR/source branch was over a year old with none of the commits adding up to the full diff. There is [1], which introduced the vulnerability but didn't remove the environment variables above, then master is merged into it via [2] (but still doesn't show the variables being removed), yet in the full PR diff they're gone.
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.
Thanks @vultour and croemer for your proactiveness -- you are correct,I updated the blog to clarify that Copilot was a co-author that checked the merged PR and code change, and identified it as all-clear without noticing the critical vulnerabilities, it's unclear whether the code-change was AI-Assisted
We're going to see more of this before we see, hopefully, substantially less of it.
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.
Yeah I agree and I think code forges as well as AI harnesses are kinda the killer apps of this (relatively short) era.
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.
That's mostly because at some point a wave of nonsense swept over the field that brought with it the Scrum master, agile, pairing, middle managers thinking up elaborate Git branching schemes (they don't understand Git) and of course, the mandatory code review.
Where’s the bullet point about the way we do programming being horrible? Not even the work, but the tools and languages afforded to us.
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.
> 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.
> 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.
It's appalling that computing in general, and unix in particular, seems to have this habit of intermingling payload and overhead.
It's like in-band signalling in the telephone network, where if you whistled the right tones into your call, you could affect the way the network processed said call. Except Ma Bell responded to that system being exploited by designing a comprehensive overhaul of the way signalling was handled, and spent a squadzillion dollars upgrading millions of tons of switching equipment to categorically exclude that entire class of attack from ever being possible.
Software, on the other hand, would need to replace no equipment whatsoever. Existing processors are perfectly capable of running code that handles the length of a string separately from its contents. There are existing languages that do this, they're just.... not used. String escapes and buffer overflows exist, going on decades now, due to nothing more than laziness, inertia, and negligence.
> It's like in-band signalling in the telephone network...
The major LLM providers claim that they are very committed to security and that their products are very dangerous and capable of great harm.
Given that their tools do what Ma Bell's systems did in the mid-1900s, [0] -which the entire software world relearned was a terrible idea by the early 1990s- they are definitely
1) Lying about the extent of their commitment to security
2) Lying about the extent of the harm that their tools are capable of
3) Both
My money is on #3.
Because of the fact that -in the absence of unambiguous laws that require meaningfully-severe punishment- even the most fucknasty and amateur hour security failures nearly always have little to no impact on the company that causes them, the major LLM providers have absolutely done the smart thing by providing commercial tools that have remote code execution vulns that would automatically give them a Critical CVSS score.
To put it another way: "the market" has no idea how to evaluate computer security claims. Because of this, every dollar you spend on proactively fixing security problems is nearly always a dollar wasted... it's better to wait until someone important gets Big Mad before spending the money. Does this make the world worse? Absolutely! Does this make companies selling software and software services much more money? Definitely!
[0] If the major LLM providers did separate unsanitized data from program instructions and ensure that the two are never mixed, things like [1] would not be possible.
I recently called some website's tech support because their web form was giving error 500. They said I need to make sure there were no single quotes in the form. Horrifying enough, removing the single quote fixed it. So they've either got SQL injection or something even worse.
No, Snowflake allowing autofixes compromised their Jira. If you tell someone to shoot you in the foot, and they shoot you in the foot, you shot yourself in the foot, just with more steps. If someone else finds the memo that says you've set up foot shooting as a service, and then they trigger that service, you still shot yourself in the foot.
Help me understand. Snowflake configured their Github repo to allow auto fixes by Copilot. It got merged automatically without anyone's review? And introduced essentially script-injection vulnerability through the title field?
If this is the case, I would say Snowflake should shut down its repo and get off Github asap.
No. A Snowflake maintainer opened a PR, Copilot suggested a change (introducing a vulnerability), the maintainer accepted and committed it to their PR, and another Snowflake maintainer approved and merged the PR.
I don't see anything in the article that says that two maintainers, let alone one, reviewed the PR manually and approved it before merging. Where are you getting this information from?
Admitting that there was human review from not one but two maintainers would entirely defeat the purpose of the article, which was to sell you an AI solution to the AI problem and insist that no human in the loop is better. Which, to be fair, in this case might have been better.
The interesting lesson here isn't really “AI generated insecure code.” We've had insecure code for decades. The bigger issue is that AI makes it much cheaper to introduce changes, while the cost of reviewing those changes hasn't gone down nearly as much.
The bottleneck is moving from code generation to code verification.
Why is the original pattern (with the env var in double quotes) not vulnerable? Why can you close the single quotes early but you can't just include double quotes in your title? Is it something to do with the GitHub templating?
then in bash I believe this is safe, because bash will just substitute this as putting the BLAH variable as the first argument to echo without doing any further parsing. without the double quotes can be safe as well but more risky.
X=$(echo $BLAH)
and the only difference is bash will split the arguments. so if you have BLAH="x y" then bash will pass two arguments to echo. though, this can be dangerous if the command you are invoking has dangerous command line options.
however, they had something similar to:
TITLE=$(echo '${{ github.event.issue.title }}')
and this ${{ }} is some kind of template substitution that is happening before the command is sent to bash. so if the variable `github.event.issue.title` was `foo bar` then bash sees something like:
TITLE=$(echo 'foo bar')
and then you start to have problems because `'` can be put into the title to escape.
the bash variable substitution will protect you in a lot of cases from command line injection but if you pass user input directly into command evaluation without using variables then bash can't protect you.
I'm all for using a council of LLMs, I wrote a tool for it https://github.com/joelio/owl - but you still need to read through PRs yourself, at the very least.
It’s clear that they want this to be true so bad that they’re just not going to do it, and will spend a ton of money on quality gates and mitigation strategies instead of just reading some code.
I have been talking to people who want to autoreview and autoapprove "minor" AI prs. For security especially, I think if the models weren't enough to prevent the issues, they aren't enough to judge what is minor.
I think it's obvious too. I'd call out any case of `${{ }}` interpolation in a `run` block, and it's something I watch for in PRs. I also know other people don't watch for this, as I've corrected it about a hundred times. Over the last 10 years my average colleague understands less and less about injection or to watch for it at layer boundaries.
Issues will happen AI, or not AI. It's same as "self driving car made an accident"!
I'm not saying blindly trusting auto-fix is not bad. I'm just saying that interpreting singular issue as way to downplay AI-assisted engineering without giving a "denominator" is not honest reporting.
I probably would have made the same mistake. It is negligent to write GitHub Actions without using static analysis.
Use zizmor in CI https://github.com/zizmorcore/zizmor
This is a really cool tool! Would zizmor have caught the below as well? From the article:
> 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.)
zizmor wouldn’t catch that condition at the moment, although it does have similar checks for other unsound conditions and incorrect/vulnerable bot actor checks. This one wouldn’t be too hard to add, though.
(Source: I am zizmor’s maintainer.)
That would be awesome - and thanks for your work, I'll want to start using it!
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:
I imagine that's true of others as well!Frankly it's absurd that you don't get a null reference error in that case. I knew Github Actions was bad... I didn't know it was that bad. This is 90% Github's fault.
Shell scripts on their own already are so perilous without static analysis. I'll never understand how we ended up deciding that embed them in yaml instead of requiring an external script file was a reasonable idea.
Bash script embedded in yaml. Wow, I'd view each of those with heavy suspicion on its own, and they're nonlinearly bad together because of how special chars interact.
yeah it is so mad:
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.
YAML seems reasonable because it allows the sequence of steps to be treated as data, which then enables GUI visualisation, introspection, etc... without actually having to run anything.
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.
nit, CUE is independent from Google now, Marcel left to work on it full time years ago with some other folks, they started a company
I get scared when I see these string interpolations in GitHub Actions.
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.
There should be an array of arguments instead of space-separated string.
Difference is you are not a trillion dollar plus technology hyped as a harbinger of civilisational change.
It can be that while also causing problems. It's like when a self driving car crashes, it's big news and everyone runs around like their pants are on fire. When a human driver crashes, it's a blip on local news or not even that.
When a human driver controlling a bus crashes though it's a big story. Even bigger when it's a train. News scales with novelty and people impacted - 1 driver being a problem isn't an issue, an ai system used in many cars... That is more newsworthy.
Probably because there are 1000x or whatever human driven miles compared to AI. And no one wants to be crippled by an AI have have to fight $billon_dollar_corp for compensation.
Difference is in how you sell it.
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.
The anti-hype is also way overblown. Nobody is saying that today's AI is omniscient and never makes mistakes. Future models will make fewer mistakes. They will still not hook into the universe with a cheat code and do everything at 100 percent reliability. It's a straw man.
The hype has significant real-world consequences such as nonsensical amounts of funding and valuations that could crash the world economy at some point. Until there some "check" on this system, i think the anti-hype is quite valid.
Overblown? Not really. There is still not enough of criticism given asinine hype given by the LLM and adjacent companies.
Funny of you to mention strawman after presenting one.
CI is shell scripts as a service. There is no safe way to use it.
AI does it a lot faster and ignores rules even harder than humans do, but it's not the root problem here.
Sadly I'm not. Either way, how LLMs work mean that traditional software analysis tools are every bit as important as they were in the before times. This is why we see a lot of hype around LLMs and formal verification.
The problem is that if everyone is 10x faster with LLMs then we're also wrong ~10x as frequently as we were before
I mean… it’s only Monday!
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.
> But yes, there is an interesting change in the past decade, where everything new must be over-hyped.
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.
Github Actions is actually so incredibly scary to have on public repo. It's full of so many footguns that's far from obvious.
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.
Would it be fair to say the blame falls squarely on Github? Why do they even allow pasting of arbitrary strings from a title directly into a script? And if they feel there is a reason, what did they imagine the safe way to do it was?
Honestly I wouldn't make this mistake. I wouldn't know how to fix that particular loc while still using bash for this, but I'd know it's dangerous.
Proof that LLMs are trained on mediocre shit. That is by definition, mediocre shit.
Is that project named after the serial nyc subway advertising dermatologist?
yes, it says "Now you can have beautiful clean workflows!" and links to a youtube video of a TV ad for a dermatologist.
Yes :-)
It's interesting to look at what was being attempted when the vulnerability was introduced[0]
> 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/...
It's completely mad and has the hallmark of an AI generated change, always eager to change one more thing.
I have seen plenty of "my backlog has never been shorter" comments here.
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.
> I'm interested in how that turns out 6 months later.
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...
> but withhold from others, even though the code change itself is likely small.
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.
What you're describing as "gatekeeping" is actually Product Ownership and should be applauded, because the alternative is a product owner who abdicates responsibility to the customer.
Lots of responses to a comment I explicitly did not make. Let me try and spell it out for you folks:
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.
This really makes it sound as though the programmers were doing something wrong:
> people are so tired of this sort of gatekeeping "we know better" from programmers
This describes a product with no vision or purpose.
People have to take 'no' for an answer sometimes, even if they don't accept it. You can't really 'gatekeep' your own product.
Can you explain to all of us how this gatekeeping is actually bad? Software would be way, way worse if we just allowed any arbitrary feature to be added to software, and ordinary people who want all the things would have to learn, the hard way, that doing all the things is not actually good.
Thank you. I'm spending a lot of time reminding my peers that cheerlead this sort of thing that feedback is usually presented to us a solution, not the underlying problem and that our job is to solve problems, not just vomit out the requested solution.
And the software without gatekeepers will be regarded as confusing and complicated or buggy.
The work to go from software to usable software system is vast.
Knock yourself out and change the world, absolutely nobody is stopping you. That you haven’t already does tell us something though.
> 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.
I assume the "gatekeeping" decision to not implement a feature request is coming from someone responsible for the product, not from a developer.
>companies are going to have to start realizing that code is not free to review or maintain
What do you mean, we have both Claude and Cursor review our CC PRs. /s
YAML is a nightmare fuel spec.
In its quest to make markup "human readable", it has created countless footguns.
I honestly prefer XML at this point.
Yeah, the YAMLification of everything kinda killed my ability to understand "everything". Previously, if you knew the Linux userland well, I felt like you could figure anything out with enough digging.
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.
Lack of a local runner is my biggest peeve. Any novel GHA workflow creation results in a PR with 100 commits, until you can finally sort out all the non-obvious idiosyncrasies. For any modestly complex workflow, I move everything to a bash or TS file and call that, and then you can use _coding tools_ and a _local_ dev/eval loop.
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.
A local runner would certainly help, but only to some extend. When you need to support multiple OS (Windows, Linux, macOS) and architecture (x64, arm64), not everything can be tested locally.
And worse: GitHub Actions not a full-fledged programming environment by itself either, so you're inevitably going to have to deal with nontrivial shell scripts on top of all the YAML mess.
Yeah. It makes sense when you're standing right next to it, but you take a step back and go "that doesn't look right". GitHub actions is the faster horse instead of a car.
The things done in yaml today would've been obscure bash oneliners had YAML not existed.
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.
> You could copy-paste the Jenkins job, but unless you have Jabberwocky v2018.3 installed in /home/JabberWock/RELEASE, the script will fail.
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]".
And in Github that would be something like:
You are not wrong, Jenkins had many problems. But I would argue that the solutions were always grounded in the Linux userland.
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.
Except that YAML in almost every devops or CI workflow is YAML plus obscure bash oneliners.
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.
> in almost every devops or CI workflow
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.
In a similar vein, JSON's lack of comments makes me marvel at how consistently JavaScript seems to choose the worse option. I'm oh so glad it found its way into config files
As a data/serialization format (the original intended use of JSON), I think no comments is the right choice, since adding comments introduces a bunch of questions about how comments ought to be parsed into a datastructure (or if they should be sent/(de)serialized at all). But for config files the lack of comments is the wrong choice, since typically you want to explain why things are set the way they are. The lack of ordering of entries also makes total sense for data/(de)serialization, but makes no sense for a config. I think the problem is people are lazy and just want to JSON.parse() everything, and used JSON in places it doesn't belong, like config files.
Many JS toolchains will take js files as config to alleviate this problem. Which is actually nice if you ever need dynamic configs, cause that means not using some forsaken DSL. It still evaluates to JSON in the end.
Seems more like the opposite vein, JSON's lack of comments or other affordances has kept it safe from footguns
Of all the problems with YAML, how is comments a footgun?
The idea is it makes people think they can use YAML for sketchy stuff as long as they comment on it.
> or other affordances
Well, while we're on the topic:
"On the virtues of the trailing comma" https://devblogs.microsoft.com/oldnewthing/20240209-00/?p=10...
I've seen people put "//" keys in their json lol
I've also seen using "__" as a key for comments. I think it's better because it doesn't need to be escaped.
There _is_ a json variant with comments, so that’s what you’ve seen. Not all parsers support that though
It's non-standard JSON, so you can probably just assume most patsers don't support any given commented parser.
At least XML permitted comments, fhe shift to JSON on everything almost makes me nostalgic.
When you get too nostalgic just think back to XSD type system in Xquery. That should cure the nostalgia real fast.
Are you denying what I've seen with my own eyes? I am saying it was an object like this:
{ "//":"make sure these are divisible by 8", "width": 640, "height": 480 }
Oooh, I misunderstood. I thought you were seeing this: https://jsonc.org/
Not the person you’re replying to but you were clear in what you were describing.
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.
It's not terrible as far as I know, and it's less ambiguous than putting a "comment" key. I like it!
It’s find for actions and workflows as long as you do no interpolation and logic.
Better move as much of that as possible into your own scripts. And your scripts can be portable between forges, and even run locally!
The YAML spec/parse _itself_ does interpolation and logic - incorrectly in some cases. YAML is pretty much never the right solution.
Assigning an env var to an empty variable (env: { myvar: ${{unsetfoo}} }) should trigger an error, not silently pass an empty string.
What does that have to do with YAML?
It has to do with the gha bastardization of yaml, which clearly learned nothing from ansible's deficiencies.
> It’s find for actions and workflows as long as you do no interpolation and logic.
How do you specify actions and workflows without interpolation and logic kind sir?
Run the same script for all of the workflow triggers and pass relevant vars into the script as environment variables.
I never figured out how the hell to write YAML and I definitely won't now that I trust the AI to do a better job than me. It's so unintuitive.
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.
I don't get it either. There was a point where I had to write YAML, but it wasn't intuitive to begin with, and months in I was still accidentally setting the wrong number of indents or some nonsense.
When I see YAML in a product tech stack, I know that the developers have probably made other similarly poor life decisions and try to steer clear of the entire iceberg.
And thus procone spoketh the truth.
In accordance with the prophecy.
Textproto is such much better than YAML for everything that YAML is used for. Sure, JSON has plenty of uses, but YAML is specifically used for configs that it's terrible at.
The first linked PR (#1218) has only one commit co-authored by Copilot and it's not related to the vulnerability, and neither are the other suggestions in the PR. Am I missing something?
You're correct. The issue was introduced in this commit by a human not by copilot: https://github.com/snowflakedb/snowflake-connector-net/pull/...
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/...
Github is having some problems -- will check! thanks a lot!
I must be too tired because I cannot figure out what happened in that pull request. The PR/source branch was over a year old with none of the commits adding up to the full diff. There is [1], which introduced the vulnerability but didn't remove the environment variables above, then master is merged into it via [2] (but still doesn't show the variables being removed), yet in the full PR diff they're gone.
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...
Yes, it's misattributed, a human introduced it: https://github.com/snowflakedb/snowflake-connector-net/pull/...
Thanks @vultour and croemer for your proactiveness -- you are correct,I updated the blog to clarify that Copilot was a co-author that checked the merged PR and code change, and identified it as all-clear without noticing the critical vulnerabilities, it's unclear whether the code-change was AI-Assisted
We're going to see more of this before we see, hopefully, substantially less of it.
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.
Yeah I agree and I think code forges as well as AI harnesses are kinda the killer apps of this (relatively short) era.
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.
That's mostly because at some point a wave of nonsense swept over the field that brought with it the Scrum master, agile, pairing, middle managers thinking up elaborate Git branching schemes (they don't understand Git) and of course, the mandatory code review.
It's best to take all these things in moderation.
lol keep dreaming bro, mistakes like these were "extroardinarily rare" before LLM companies reared their thieving hands.
“There is absolutely no way Bitcoin will ever trade for more than $200.. Impossible!” he said.
The title is actually "Wiz Red Agent Finds Its Way Into Snowflake’s Internal Jira Due to an AI-Generated GitHub Copilot Autofix"
Too long for hackernews :(
I know like 3 of those words.
Where’s the bullet point about the way we do programming being horrible? Not even the work, but the tools and languages afforded to us.
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.
They didn’t really sell this PR well:
> 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.
> 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.
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.
The env + jq was there on purpose. Autofix swapped it for a string in a shell. That's the part that needed a person on the diff.
This really shows why most languages evaluate all NULL comparisons to FALSE.
For something as critical as Actions, it’s crazy to me that they wouldn’t fail-closed, and instead fail open when encountering a null. Scary stuff!
> a single quote in the title breaks out of echo '...' and allows arbitrary command execution.
Quote injection still alive and well in 2026. Gawd.
It's appalling that computing in general, and unix in particular, seems to have this habit of intermingling payload and overhead.
It's like in-band signalling in the telephone network, where if you whistled the right tones into your call, you could affect the way the network processed said call. Except Ma Bell responded to that system being exploited by designing a comprehensive overhaul of the way signalling was handled, and spent a squadzillion dollars upgrading millions of tons of switching equipment to categorically exclude that entire class of attack from ever being possible.
Software, on the other hand, would need to replace no equipment whatsoever. Existing processors are perfectly capable of running code that handles the length of a string separately from its contents. There are existing languages that do this, they're just.... not used. String escapes and buffer overflows exist, going on decades now, due to nothing more than laziness, inertia, and negligence.
And now the "AI" industry is doubling down on it - with hatbot prompt injection.
> It's like in-band signalling in the telephone network...
The major LLM providers claim that they are very committed to security and that their products are very dangerous and capable of great harm.
Given that their tools do what Ma Bell's systems did in the mid-1900s, [0] -which the entire software world relearned was a terrible idea by the early 1990s- they are definitely
1) Lying about the extent of their commitment to security
2) Lying about the extent of the harm that their tools are capable of
3) Both
My money is on #3.
Because of the fact that -in the absence of unambiguous laws that require meaningfully-severe punishment- even the most fucknasty and amateur hour security failures nearly always have little to no impact on the company that causes them, the major LLM providers have absolutely done the smart thing by providing commercial tools that have remote code execution vulns that would automatically give them a Critical CVSS score.
To put it another way: "the market" has no idea how to evaluate computer security claims. Because of this, every dollar you spend on proactively fixing security problems is nearly always a dollar wasted... it's better to wait until someone important gets Big Mad before spending the money. Does this make the world worse? Absolutely! Does this make companies selling software and software services much more money? Definitely!
[0] If the major LLM providers did separate unsanitized data from program instructions and ensure that the two are never mixed, things like [1] would not be possible.
[1] <https://www.schneier.com/blog/archives/2026/08/prompt-inject...>
I recently called some website's tech support because their web form was giving error 500. They said I need to make sure there were no single quotes in the form. Horrifying enough, removing the single quote fixed it. So they've either got SQL injection or something even worse.
No, Snowflake allowing autofixes compromised their Jira. If you tell someone to shoot you in the foot, and they shoot you in the foot, you shot yourself in the foot, just with more steps. If someone else finds the memo that says you've set up foot shooting as a service, and then they trigger that service, you still shot yourself in the foot.
Help me understand. Snowflake configured their Github repo to allow auto fixes by Copilot. It got merged automatically without anyone's review? And introduced essentially script-injection vulnerability through the title field?
If this is the case, I would say Snowflake should shut down its repo and get off Github asap.
No. A Snowflake maintainer opened a PR, Copilot suggested a change (introducing a vulnerability), the maintainer accepted and committed it to their PR, and another Snowflake maintainer approved and merged the PR.
I don't see anything in the article that says that two maintainers, let alone one, reviewed the PR manually and approved it before merging. Where are you getting this information from?
Admitting that there was human review from not one but two maintainers would entirely defeat the purpose of the article, which was to sell you an AI solution to the AI problem and insist that no human in the loop is better. Which, to be fair, in this case might have been better.
This is the PR: https://github.com/snowflakedb/snowflake-connector-net/pull/...
A human introduced the bug, not Copilot: https://news.ycombinator.com/item?id=49334262
And that's going to continue because no one is reading the code even when they approve it.
It's a very strange thing indeed, but not unexpected: we warned that skills not used will eventually atrophy.
Thanks.
The interesting lesson here isn't really “AI generated insecure code.” We've had insecure code for decades. The bigger issue is that AI makes it much cheaper to introduce changes, while the cost of reviewing those changes hasn't gone down nearly as much.
The bottleneck is moving from code generation to code verification.
Please do not post LLM generated comments here. Thank you.
What made you make that assessment?
- Three different instances of "The X is Y" in a single short comment
- Relatively new account, "AI Engineer building agentic systems"
- Most past comments contain em-dashes, more "The X is Y", etc.
Human error.
AI generated code, must be scanned for code quality, SAST, SCA, etc, just like a developer's code would.
It looks like they accepted AI code without verifying. Deserved!
Why is the original pattern (with the env var in double quotes) not vulnerable? Why can you close the single quotes early but you can't just include double quotes in your title? Is it something to do with the GitHub templating?
GitHub has an article about it
https://docs.github.com/en/actions/reference/security/secure...
i'm not sure if there was an original pattern where the env var was in double quotes.
https://github.com/snowflakedb/snowflake-connector-net/pull/...
but if you have:
X=$(echo "$BLAH")
then in bash I believe this is safe, because bash will just substitute this as putting the BLAH variable as the first argument to echo without doing any further parsing. without the double quotes can be safe as well but more risky.
X=$(echo $BLAH)
and the only difference is bash will split the arguments. so if you have BLAH="x y" then bash will pass two arguments to echo. though, this can be dangerous if the command you are invoking has dangerous command line options.
however, they had something similar to:
TITLE=$(echo '${{ github.event.issue.title }}')
and this ${{ }} is some kind of template substitution that is happening before the command is sent to bash. so if the variable `github.event.issue.title` was `foo bar` then bash sees something like:
TITLE=$(echo 'foo bar')
and then you start to have problems because `'` can be put into the title to escape.
the bash variable substitution will protect you in a lot of cases from command line injection but if you pass user input directly into command evaluation without using variables then bash can't protect you.
I think GitHub itself could use a nice "Autofix" right about now. ;-)
Peer review of changes is still important.
Multi-model cross-review is important
I'm all for using a council of LLMs, I wrote a tool for it https://github.com/joelio/owl - but you still need to read through PRs yourself, at the very least.
It's not actually, thats just shoving more shit into the shit pipeline.
Humans need to review this stuff yall there's no way around that, apparently to some, very inconvenient reality.
It’s clear that they want this to be true so bad that they’re just not going to do it, and will spend a ton of money on quality gates and mitigation strategies instead of just reading some code.
You can't rely on people spotting the significance of such changes
Tests would have caught it = https://github.com/rhysd/actionlint injection check
also been a huge fan of zizmor (https://github.com/zizmorcore/zizmor) lately, basically: "am I going to footgun myself?"
I have been talking to people who want to autoreview and autoapprove "minor" AI prs. For security especially, I think if the models weren't enough to prevent the issues, they aren't enough to judge what is minor.
^^
Absolutely.
Nothing in the PR jumps out as a red flag. Unless you know how the internals work, I suppose.
Shouldn't anyone reviewing such a PR know how the internals work?
Not anymore, it seems
Are you kidding? It's a very obvious case of quote injection. Not some subtle race condition or anything.
I think it's obvious too. I'd call out any case of `${{ }}` interpolation in a `run` block, and it's something I watch for in PRs. I also know other people don't watch for this, as I've corrected it about a hundred times. Over the last 10 years my average colleague understands less and less about injection or to watch for it at layer boundaries.
> Nothing in the PR jumps out as a red flag.
Made by AI?
Sheesh these anti ai posts feel like when I hear about a self driving car is doing something bad.. ie 'man bites dog' vs 'dog bites man'
Human responsibility over AI oversight folks.. even forgoing AI, we're still gonna get compromised code either way.. deal with it.
Someone forgot to add "make no mistakes!" when triggering autofix /s
Issues will happen AI, or not AI. It's same as "self driving car made an accident"!
I'm not saying blindly trusting auto-fix is not bad. I'm just saying that interpreting singular issue as way to downplay AI-assisted engineering without giving a "denominator" is not honest reporting.