Repository navigation
Conversation
233f2b8 to
1e41433
Compare
71402a1 to
6248e39
Compare
sonukapoor
left a comment
There was a problem hiding this comment.
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.
|
@sonukapoor thank you — this is one of the more useful reviews I've had. You clearly read past the diff: catching that the Done in 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. |
--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
c83d081 to
e2306a3
Compare
sonukapoor
left a comment
There was a problem hiding this comment.
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.
Fixes #1391
Summary
#1382 added
requireFlagValueand 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-onis not part of this diff: #1393 is onmainand 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, matchingadvisories sync.--reportkeeps its optional value and is untouched.Stacked on #1382
The first commit (
3609c48) is #1382 verbatim, because this follow-up needs itsrequireFlagValuehelper. Merge #1382 first; after that this pull request reduces to the single follow-up commit.Behaviour
Before:
After:
Exit code is non-zero, and the message names the flag.
Tests
=forms.=, flag as the=value, and valid values.npm run lint:tests,npm run build, andnpm test(179 suites / 2,507 tests) pass.