Skip to content

fix(cli): reject a value flag whose value is missing or another flag - #1382

Open
alex-powell wants to merge 1 commit into
OWASP:mainfrom
alex-powell:fix/cli-value-flag-swallow
Open

alex-powell wants to merge 1 commit into
OWASP:mainfrom
alex-powell:fix/cli-value-flag-swallow

Conversation

@alex-powell

Copy link
Copy Markdown
Contributor

Fixes #1381

Summary

--offline-db, --cache-dir, --osv-url, --audit-log (scan and overrides) and --rule read the next argv token without checking it, so a flag given without its value swallowed the following flag. cve-lite . --audit-log --json wrote the audit log to ./--json and printed human output; cve-lite . --cache-dir --offline created a --offline directory and dropped offline mode.

Change

  • Add requireFlagValue in src/cli/args.ts, which throws <flag> requires a <noun> argument when the value is missing, empty, or starts with -, matching the existing --output guard.
  • Use it for the spaced and = forms of the five flags above.

Tests

  • New 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 build and npm test pass.

--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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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 and overrides flags.
  • 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 sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sonukapoor

Copy link
Copy Markdown
Collaborator

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 --fail-on severity disarmed the gate, so a vulnerable tree exited 0.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cli): a value flag given without its value swallows the next flag, so --json or --offline is silently dropped

3 participants