Skip to content

fix(cli): reject missing values for threshold and size flags - #1392

Open
prx-my wants to merge 4 commits into
OWASP:mainfrom
prx-my:fix/issue-1391-value-flag-fallbacks
Open

prx-my wants to merge 4 commits into
OWASP:mainfrom
prx-my:fix/issue-1391-value-flag-fallbacks

Conversation

@prx-my

@prx-my prx-my commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1391

Summary

#1382 added requireFlagValue and used it for the five flags that consumed the next token unchecked. Three more flags on the scan path kept the same bug but were quieter: they assigned through a default fallback, so a missing value silently dropped both the value and the following flag.

This applies the existing helper to:

  • --min-severity
  • --search-depth
  • --batch-size

--fail-on is not part of this diff: #1393 is on main and covers that flag on both paths, validating the value as well as rejecting a missing one.

The issue's "Also in scope: --ca-cert" section is addressed by converging the scan-path branches on the same helper. The spaced and = forms now both reject a missing path and a value that is itself a flag, matching advisories sync. --report keeps its optional value and is untouched.

Stacked on #1382

The first commit (3609c48) is #1382 verbatim, because this follow-up needs its requireFlagValue helper. Merge #1382 first; after that this pull request reduces to the single follow-up commit.

Behaviour

Before:

cve-lite . --min-severity --all --no-cache
# prints findings, ignores --all, exits 0

After:

Error: --min-severity requires a level argument
Run `cve-lite --help` to see supported options.

Exit code is non-zero, and the message names the flag.

Tests

  • Parser tests for each flag in both spaced and = forms.
  • Cases cover: next token is another flag, flag at the end of argv, empty =, flag as the = value, and valid values.
  • Each per-flag case fails if that flag's guard is removed.
  • npm run lint:tests, npm run build, and npm test (179 suites / 2,507 tests) pass.

@prx-my
prx-my requested a review from sonukapoor as a code owner October 22, 2026 09:50
Copilot AI balanced review requested due to automatic review settings October 22, 2026 09:50

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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

Good work, and it is nearly there. Everything you built is right. One piece of the issue is still open, and that affects the closing keyword.

What I checked. All three flags reject a missing value in both the spaced and = forms, exit 1, and name the flag using the helper's wording. Valid values produce output byte-identical to main. Reverting any one of the three guards on its own fails exactly 4 of the 39 tests across your two files and leaves 35 passing, so each flag is pinned separately rather than one test covering the lot, which is what the issue asked for. Merge result against current main is clean at 179 suites and 2,503 tests.

You also respected the trap. --report deliberately takes an optional value, and both --report with nothing after it and --report=x.html produce output identical to main, so the helper has not been applied where it would have broken something.

Dropping the --fail-on arm was the right call. #1393 is on main now and covers that flag on both paths, validating the value as well as rejecting a missing one, so there was nothing left there for you.

The one thing outstanding. The issue carries a section headed "Also in scope: --ca-cert behaves three different ways", and that part is untouched. On your branch, as on main, the spaced form rejects a missing path while the = form with nothing after it exits 0 and sets the path to an empty string. It is a single assignment on the scan path, the only one of the four certificate branches with no check at all, and the message the other three throw by hand is character for character what the helper already produces. So it is the substitution the issue describes rather than new work.

Because the body says Fixes #1391, merging as it stands would close that issue with this part unbuilt. Either way out is fine by me: add the one line, which I think is worth doing while you are already in the file, or change the keyword to a plain reference and I will keep the issue open for it.

Two housekeeping notes from the rebase. The body still lists --fail-on among the flags you guard, and it still cites the commit id the rebase replaced. Worth a quick edit so the description matches the branch for whoever reads this later.

And thank you for how you handled the stacking. Declaring it under its own heading, rather than quietly carrying someone else's commit, is the right way to do this, and rebasing #1382's commit onto current main kept Alex's authorship on it. I have asked him to rebase his branch so that one goes first: we squash on merge, so taking yours first would collapse his commit into yours and lose his name from the history. Once his lands, this reduces to your single follow-up commit exactly as you said it would.

@prx-my
prx-my requested a review from sonukapoor October 24, 2026 15:16
@prx-my

prx-my commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

@sonukapoor thank you — this is one of the more useful reviews I've had. You clearly read past the diff: catching that the --ca-cert section of #1391 was still unbuilt, noticing the body had drifted from the branch after the rebase, and flagging the squash-merge ordering so Alex's authorship survives are all things a shallower pass would have missed. Your replies are consistently more insightful than the usual "LGTM with nits", and the way you explained why each point mattered made the follow-up straightforward.

Done in c83d081: both scan-path --ca-cert branches now go through requireFlagValue, so the spaced and = forms both reject a missing path and a flag used as a value, matching advisories sync. I added --ca-cert to the parser table tests, and the PR body now drops --fail-on (pointing at #1393), cites 3609c48, and records the --ca-cert change. npm run lint:tests, npm run build, and npm test pass at 179 suites / 2,507 tests. Happy to hold until #1382 lands, as you suggested.

Separately, and outside this PR: I'd like to propose taking on the docs site at https://owasp.github.io/cve-lite-cli/. I've been through it and the front end has real issues, including some security-relevant ones, and the presentation reads as very AI-generated rather than something the project would stand behind. I'd like to fix it properly — one PR per issue, the same scoped, test-backed, individually reviewable shape as this one, with the issue list agreed before any code so nothing sprawls. The goal is a site that actually earns its place as the front door for the tool: accurate, accessible, fast, and maintained. Would that be welcome? If so, I'll open the issues first so we can scope them together before I start.

alex-powell and others added 3 commits October 25, 2026 19:06
--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
--ca-cert= set the path to an empty string and the spaced form accepted
another flag as its value. Route both branches through requireFlagValue,
matching the guard the other value flags and advisories sync already use.

Fixes OWASP#1391
@prx-my
prx-my force-pushed the fix/issue-1391-value-flag-fallbacks branch from c83d081 to e2306a3 Compare October 25, 2026 13:36

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

Thanks, this closes the last gap. --ca-cert= with nothing after it now fails with the standard message instead of quietly setting an empty path, and --ca-cert=--all no longer treats --all as a file. Approved.

Two small edits to the description so it matches the branch: the first commit is now ef08155 rather than 3609c48, and once #1382 lands this reduces to two follow-up commits, not one. Also, "matching advisories sync" isn't quite accurate: advisories sync --ca-cert --all still takes --all as the path. That's outside #1391, so I'll file it separately.

The merge order doesn't change: #1382 goes first, then this.

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): four more value flags swallow the next flag when given without a value, silently dropping both

4 participants