The missing file glitch - a bug story

🐛 The Problem

On January 28th, 2025, we received an issue reported in our team's Slack channel stating that security scans started failing after merging a pull request with only TypeScript test files. After ensuring this was not a temporary issue or a trivial problem, we began the investigation.

🕵🏻‍♀️ The Analysis

Looking into the workflow, we saw this error:

Error: ENOENT: no such file or directory, open 'scan_result.json'

Scrolling further up the log output, these lines drew our attention:

 USAGE
  security-scanner scan create [flags]
[...]

Most of you who have worked with CLI tools before might have seen similar output. In this case, it indicates that the command line call was invalid - we passed something to security-scanner that it did not expect. So we started debugging, which in CI is unfortunately less convenient since there is no way to attach a debugger and gain insights into the code execution. The second-best option required us to add some debugging statements and re-run the workflow with debugging enabled to see what command would actually be executed.

##[debug]Running command in shell: security-scanner scan create --debug
 --scan-types sast --file-include *.ts --report-format summaryJSON

Well, that ... looks ... good. What the h*** is going on here? 🤨

The next day (and this is a great example that taking a step back and rethinking problems can be more efficient than getting into the zone), a suspicious line of code caught my attention. Why do we enable a shell when invoking the subprocess for security-scanner? The documentation says, "If the shell option is enabled, do not pass unsanitized user input to this function. Any input containing shell metacharacters may be used to trigger arbitrary command execution." Well, we pass unsanitized input from the config file. And we do not need any shell functionality (we use some at the moment though - more on that later). But besides the potential risks, what's the issue? Let's rethink what actually changed. The GitHub Action implementation did not change. So what changed in the reported commit that causes the issue? There's one thing in the security-scanner invocation shown above that looks like it could be impacted by the shell: --file-include *.ts

We're passing in an argument that has an asterisk in the value. A nice example of unsanitized user input. The shell interprets this string as a glob pattern and tries to expand it before executing the command. Interestingly, if the glob pattern cannot be expanded, it gets passed into security-scanner as is. However, if the pattern actually matches files, the pattern would be replaced by the matching file(s). Taking a look again into the "faulty" commit, we could see there are indeed new files in the repo matching *.ts. So the command we saw in the debug log in the context of this commit is not what is actually executed, but this is:

##[debug]Running command in shell: security-scanner scan create --debug
 --scan-types sast --file-include vitest.config.ts vitest.workspace.ts
 --report-format summaryJSON

🤦 That's the culprit! It is no longer the glob pattern that we pass to security-scanner, but a list of files that match it. Since security-scanner expects exactly one value for --file-include while now a list of two filepaths is passed, it yells at us complaining about incorrect usage.

💊 The Fix!

As always, there are multiple ways to address the problem. We could put the value for --file-include into quotation marks and the shell would not treat it as a glob pattern. Good, but why not fix the root of the evil and disable the shell in the first place? We do not run a script but invoke a single binary. No need to spawn a shell for this. Or is it? It turns out, there are a number of repositories running a shell command as part of their preRunCommands configuration. Luckily, it looks like they all do exactly the same and invoke pwd. A quick test revealed $(pwd) could be replaced by a simple . keeping the same semantics.

So as a result, a pull request was opened to fix the issue by disabling the shell option. However, this is a breaking change. The goal of LeanIX's Developer Experience team is to reduce our teams' cognitive load. Therefore, we aimed for a rollout that would require minimal effort from teams to adopt the change. So we decided to automatically create change requests replacing all pwd invocations. Once they all got merged, we could roll out the breaking change for security-scanner.

💡 The Learning

When invoking commands as a subprocess, do not make use of a shell if there is no explicit need to do so. It has a lot of power and thus is a big attack vector. Another learning here is debugging in CI is time-consuming and tricky. Try to keep GitHub Actions and workflows simple and move as much of the logic as possible into well-tested scripts.

Published by...

Robin Lungwitz

Visit author page