Repository navigation
feat(output): add --output <dir> for SARIF, JSON and SBOM files - #1215
Conversation
The scan wrote --sarif, --json and --sbom files into the working directory with no way to choose the location, so CI had to glob and rename the timestamped file before handing it to a fixed-path step. The scan parser already accepted --output and ignored it. --output <dir> now sets the directory for those files. Relative paths resolve against the working directory, the directory is created when missing, and the default stays the working directory. The printed path is relative to the working directory, so it is unchanged without the flag. --output without --json, --sarif or --sbom is rejected instead of silently doing nothing, and a missing directory argument is an error. The GitHub Action gains a matching `output` input. --report is left as the HTML dashboard. Closes #1214
sonukapoor
left a comment
sonukapoor
left a comment
There was a problem hiding this comment.
This is a good change and it solves something genuinely awkward. A timestamped file dropped into the working directory meant every CI job had to glob and rename before it could hand a path to the next step, and I like that --output with none of --json, --sarif or --sbom is an error rather than a silent no-op.
A few things I checked rather than assumed. The default really is unchanged. Every writer goes through the new directory. The multi-folder path only needed its JSON branch because it rejects --sarif and --cdx anyway. And I neutered resolveOutputDir locally to see whether the tests would notice: four failed, including both writeOutputs ones, so the coverage is doing real work.
Two things I would like to fix before it lands, both small, and one optional question.
The first is the one that matters. The action appends --output whenever the input is set, but the CLI rejects --output without one of the three file flags and exits 1, and the no-fail-on branch swallows anything at or below 1. So with: { output: reports } on its own is a step that passes having scanned nothing. I ran it to be sure. For a security check that is the worst shape of failure, because the signal it gives is indistinguishable from a clean scan. Detail inline.
The second is that --output takes the next token whatever it is, so cve-lite . --json --output --sarif creates a directory called --sarif and quietly drops the SARIF. The parse weakness predates you, but before this PR output was ignored so nothing came of it. Detail inline.
The optional one: displayOutputPath reads well for a relative directory, but an absolute --output prints ../../../tmp/reports/cve-lite-scan-....json, and absolute is the common CI case. Your call, you documented the relative behaviour deliberately.
One thing that is ours, not yours, so please do not pick it up: multi-folder mode silently ignores --sbom entirely, writing nothing and exiting 0. It is the same on main and your diff does not touch that guard. I will file it.
--output took whatever token followed it, so `--output --sarif` created a directory named --sarif and dropped the SARIF flag. Values starting with a dash are now rejected, as --sbom already does. The action appended --output whenever the input was set, but the CLI rejects it without a file output and the action tolerates exit 1, so a step with only `output` set passed without scanning. The flag is now added only when sarif or cdx is enabled, with a warning otherwise, and the input description names just those two. The printed path falls back to the absolute form when the file lies outside the working directory, instead of a ../ chain.
sonukapoor
left a comment
sonukapoor
left a comment
There was a problem hiding this comment.
Both fixed, and the action guard is better than what I asked for: warning the user beats silently dropping the flag. Verified by running all three cases rather than reading the diff. Thanks for turning this around so quickly.
|
Merged, thank you @bennycode! First contribution here and a genuinely useful one. It will be in the next release. |
|
Thanks for reviewing and releasing it so quickly, @sonukapoor! The new |
|
Yes @bennycode - It should be available on version 1. |
What changed and why
--sarif,--jsonand--sbomwrote a timestamped file into the working directory with no way to choose the location, so CI had to glob and rename the file before passing it to a fixed-path step. The scan parser already accepted--outputand ignored it.--output <dir>now sets the directory for those files. Relative paths resolve against the working directory, the directory is created when missing, and the default stays the working directory, so nothing changes for existing users. The printed path is relative to the working directory, so it is the bare filename without the flag.--outputwithout--json,--sarifor--sbomis rejected rather than silently doing nothing, and a missing directory argument is an error. The GitHub Action gains a matchingoutputinput.--reportkeeps its meaning as the HTML dashboard.Tests cover the directory helper, the writers given a directory, the
writeOutputswiring, validation, argument parsing, help text and the action input.One note: CONTRIBUTING mentions a Prettier config and a CI formatting check, but the repo has no Prettier config and a default run flags most existing files, so I matched the surrounding style instead of formatting.
Closes #1214