Repository navigation
fix(cli): reject a value flag whose value is missing or another flag - #1382
alex-powell wants to merge 1 commit into
Conversation
--offline-db, --cache-dir, --osv-url, --audit-log and --rule read the next argv token unconditionally, so "--audit-log --json" wrote the audit log to a file named --json and dropped JSON output. Require a non-empty value that does not start with - for the spaced and = forms. Fixes OWASP#1381
There was a problem hiding this comment.
🟢 Approval recommended
The targeted parser changes consistently enforce the requested validation, with focused regression coverage and no blocking issues identified.
No open issues 🎉
What changed in this PR
Fixes CLI argument parsing for issue #1381 so value flags cannot silently consume following flags such as --json or --offline.
Changes:
- Adds shared validation that rejects missing, empty, or dash-prefixed values.
- Applies validation to spaced and
=forms of the affected scan andoverridesflags. - Adds regression tests for invalid inputs and valid values in both forms.
| File | Description |
|---|---|
| tests/cli/value-flag-args.test.ts | Adds parameterized tests for required flag values. |
| src/cli/args.ts | Adds and applies requireFlagValue to the affected parser branches. |
Review effort: Balanced
sonukapoor
left a comment
There was a problem hiding this comment.
Approved. Good report and a clean fix, and I am asking you for nothing here.
I reproduced every case from the issue on main before looking at your change, because the second one is worse than it reads: asking for a cache directory and offline mode together creates a directory named after the offline flag and then goes to the network anyway, exit 0. Someone who explicitly told us not to make network calls got network calls and no warning. The audit-log case writes the log to a file literally named after whichever flag it ate.
With your change, all six places the issue names reject a missing, empty or flag-shaped value in both the spaced and = forms, each with a message naming the flag and the kind of value it wants. I checked the other direction too, since the new rule rejects anything starting with a dash and I wanted to know whether a working command could start failing. Normal relative paths are unaffected, only a bare dash-leading name is refused, and there is an obvious escape hatch in writing it as ./-name. I also checked our own Action, which passes two of these flags on behalf of adopters: both are already guarded against being empty, so nobody's workflow starts failing on upgrade.
Two notes, neither of them a change request.
The empty-value rejection is a behaviour change, not only a tightening. Before this, an empty value ran a normal scan, because an empty cache directory fell through to the default. Now it exits 1 at parse time. That is what the issue asked for and I think it is right, but anyone wrapping us in a script that interpolates an unset variable will see a new failure rather than a silent fallback. Worth knowing it is deliberate.
The same flag family is not finished, and that is on me rather than you. Four more flags swallow the next token the same way, and one of them is the CI gate, where the effect is that the gate silently reverts to its default and exits 0 on a tree with findings. I have filed that as #1391 so your helper gets applied there rather than duplicated. I also found that --ca-cert now behaves three different ways depending on the form and the subcommand, in the file you touched. Both are outside what #1381 asked for, so they are follow-ups, not something to grow this PR with.
One more thing worth separating out, because it is the most serious of the set and nothing to do with missing values: an unrecognised severity on the gate flag is silently accepted and the threshold falls back to the default. --fail-on moderate on a tree with high findings prints the findings and exits 0, and moderate is npm audit's own severity name. Filed as #1390.
So your fix found its way to two further issues, which is more than the change itself. Thanks for the detail in the report, the reproduction cases were exact.
|
This needs a rebase now. #1393 merged yesterday and lands in the same place. That one jumped the queue because it fixes a silent false negative that was live in 1.40.0: an unrecognised The conflict is one region, sixteen lines, one file. Both changes add a helper at the same point, immediately above the existing format parser. Mine validates the severity passed to one flag, yours rejects a missing value on several, so the resolution is keeping both rather than picking one. Nothing of yours is superseded. You also do not have to work it out. @prx-my stacked #1392 on this branch and rebased onto current main overnight, so your commit already exists rebased past this exact conflict with your authorship intact. I applied that version to current main on its own: no conflict, clean build, full suite green at 178 suites and 2,488 tests. The two helpers sit side by side and it works. The push will dismiss the approval, since this repository dismisses reviews on any content change. That is not a fresh review cycle for you. I will re-approve as soon as it lands. Worth knowing too: #1392 is stacked on this one and reduces to a single follow-up commit once it merges, so this unblocks that as well. |
Fixes #1381
Summary
--offline-db,--cache-dir,--osv-url,--audit-log(scan andoverrides) and--ruleread the next argv token without checking it, so a flag given without its value swallowed the following flag.cve-lite . --audit-log --jsonwrote the audit log to./--jsonand printed human output;cve-lite . --cache-dir --offlinecreated a--offlinedirectory and dropped offline mode.Change
requireFlagValueinsrc/cli/args.ts, which throws<flag> requires a <noun> argumentwhen the value is missing, empty, or starts with-, matching the existing--outputguard.=forms of the five flags above.Tests
tests/cli/value-flag-args.test.ts: next token is a flag, flag at end of argv, empty=form, and a valid value for each flag. 18 of its cases fail on main.npm run buildandnpm testpass.