Repository navigation
fix(output): align HTML report.json with --json output shape - #1176
Conversation
sonukapoor
left a comment
sonukapoor
left a comment
There was a problem hiding this comment.
Thanks for taking #237, and for stating the direction explicitly in the description. That made this reviewable.
I checked the claim that matters, which is that --json is unchanged. Built main and your branch separately, ran both against a shared pre-warmed advisory cache so OSV data could not drift between runs, normalised scannedAt, and diffed:
| example | manager | result |
|---|---|---|
| nest | npm | identical, 316,442 bytes |
| analog | pnpm | identical, 660,767 bytes |
| yarn-berry | Yarn | identical, 33,615 bytes |
| bun-simple | Bun | identical, 27,621 bytes |
2-app multi-folder, with --check-overrides --check-maintenance |
npm | identical, 26,131 bytes |
Key order preserved too, not just the key set, so order-sensitive consumers are fine. That is the thing that would have broken other people's tooling and it does not.
I also checked that the 62 removed lines genuinely moved rather than being dropped, since multi-folder-scan.ts losing 35 and write-outputs.ts losing 27 is where a silent behaviour loss would hide. They moved, same key order, same spread order.
And I chased one risk that is not obvious from the diff: the embedded const reportData in index.html is not only the download source, the page JS reads reportData.findings[i] by index for filter, search and sort. Swapping its shape could have broken index alignment. On a real 69-finding report, 69 embedded findings against 69 rendered rows on both builds, and every field the page reads is present in serializeFinding. Fine.
146 suites, 1944 tests, Docusaurus builds clean, and your new e2e test is the valuable one because it runs the real CLI twice and compares artifacts rather than asserting on a helper.
One thing before I merge it, and it is a real one.
report.json loses cliVersion, scannedAt and lockfileSource, plus per-finding riskSummary and nextAction. That is the intended direction of #237, but it is a breaking change for anyone parsing report.json today, and the docs edit ("Same JSON schema as --json") does not say so.
The part I would push back on specifically is scannedAt and cliVersion. Provenance matters more in a file someone keeps on disk than in a stdout dump they pipe somewhere. There is also an asymmetry worth deciding deliberately rather than inheriting: the multi-folder payload keeps scannedAt, so after this change single-folder report.json has no timestamp at all while multi-folder does.
Either keep those two fields in the report payload, or say plainly in the docs what is being removed. I am easy on which, but silently dropping provenance from a saved artifact is not something I want to ship without it being written down.
Two smaller things, neither blocking:
renderHtmlReport(data, jsonPayload?) takes the payload as optional and falls back to data, which is the old wrong shape. The only production call site always passes it, and writeHtmlReport.jsonPayload is correctly required, so the risk is contained, but a future call site that forgets it silently reintroduces this bug with no type error. Making it required would close that off.
And aggregateMultiFolderCompleteness(results) now runs twice per multi-folder scan, at multi-folder-scan.ts:348 and again inside buildMultiFolderScanJson. It is pure so the output is unaffected, but the helper could take completeness as a parameter.
|
Kept |
sonukapoor
left a comment
sonukapoor
left a comment
There was a problem hiding this comment.
Provenance is back on both paths: single-folder report.json and the multi-folder one both carry scannedAt and cliVersion, so the asymmetry I was worried about is gone too.
I re-ran the check that matters. Comparing your branch against current main showed a 16KB difference, which looked alarming for about a minute. It is not yours: every difference is main being ahead, firstFixedVersion 2.5.4 against 2.5.6 plus the stale-hint note, and null EPSS, all from things merged this week. Diffed against your own merge-base instead, the --json payload is byte-identical including key order.
So the central claim of this PR holds. Nobody's tooling breaks.
You left the optional jsonPayload and the double aggregateMultiFolderCompleteness call alone, which is fine, they were explicitly non-blocking. Worth knowing the first one is the kind of thing that bites a future call site silently, so if you are in that file again it is a small change.
This is behind main too and will want a rebase before merge.
|
Heads up, this has picked up conflicts with main and needs a rebase before it can land. The overlap is with two things that merged after you opened it: #1215 added Nothing else is outstanding. The approval stands and covers the work as it is, so once this is rebased and green it is a one-step merge from my side. |
b9324e8 to
6379f5f
Compare
|
Rebased onto latest main and CI is green. The force-push dismissed the earlier approval, so it will need a quick re-approve. |
sonukapoor
left a comment
sonukapoor
left a comment
There was a problem hiding this comment.
The core of this is right and I could measure it rather than take it on trust: --json, SARIF,
CycloneDX and the rendered HTML are all unchanged across five scenarios, single and multi folder,
with and without the three opt-in checks. That is the hard part of a refactor like this and you
got it.
The second commit is the part I liked most. You noticed your own first commit had dropped
scannedAt and cliVersion from report.json and put them back without leaking them into
--json, and the comment says why. That is the right call.
Three things before I merge it.
Two are one-line fixture changes to tests you already wrote, and both currently compare an empty
array against an empty array, so they cannot fail. In the new e2e test, the fixture is built on a
safe package and produces zero findings, so the report.json comparison passes trivially. Point
it at a fixture with one vulnerable package and it does what you meant it to do. Same in the new
write-outputs test: the mock scan state has empty sorted, coverage, notes, warnings and
skippedDependencies, so changing coverage: scanState.coverage to coverage: [] in the source
leaves the suite green. Give the mock one finding and non-empty arrays.
The third is a scope question for me rather than a mistake by you. This drops riskSummary and
nextAction from report.json. Nothing in the repo reads that file and the GitHub App reads
--json, which you left unchanged, so I do not think anything breaks. But they are the two
human-readable strings the HTML shows in expanded rows, and the removal is not in the PR body or
the docs. I would rather keep them, since the HTML already computes them and adding them back is
additive. If you disagree, say so and I will document the removal instead.
Two optional notes. renderHtmlReport(data, jsonPayload?) falls back to the old shape when the
second argument is missing, which only tests do today, but a silent fallback to the shape this PR
exists to retire is the drift this PR is fixing. Making it required would close that off. And
handleSingleFolderScan is now 626 lines because the payload block went in inline; moving it
into a named helper in scan-json.ts would fit that module's purpose.
6379f5f to
099a877
Compare
|
2 new high findings introduced by this PR
npm update brace-expansionOnly findings this PR changed are listed. Nothing is posted when the set is unchanged. Scanned by CVE Lite CLI, a free and open source OWASP project. ❤️ ShareIf this caught something before it merged, a mention helps other maintainers find it.
|
|
Thanks, all three addressed (amended into the second commit):
Left the two optional notes for now to keep this PR focused. |
The HTML report wrote report.json from the internal ReportData / MultiFolderReportData shapes, while --json used a different envelope built from findings. Tooling that parsed one could not consume the other. Extract buildScanJson and buildMultiFolderScanJson as the single serializers for --json, then reuse them when writing report.json and when embedding the in-page JSON download. ReportData stays for HTML rendering only. Closes OWASP#237
- Keep scannedAt and cliVersion on HTML report.json and the in-page download, single and multi folder; --json output is unchanged. - Keep per-finding riskSummary and nextAction on report.json as additive fields on top of the --json payload. - Point the report.json vs --json e2e test at a vulnerable fixture so the findings comparison is not vacuous. - Give the write-outputs --json test a non-empty scan state so dropping coverage, notes, warnings or findings fails it. - Document the extra report.json fields.
099a877 to
b05a464
Compare
|
Rebased onto latest main and CI is green. The only conflict was |
sonukapoor
left a comment
sonukapoor
left a comment
There was a problem hiding this comment.
All three are done, and I checked the two test ones by re-running the mutations that used to survive. coverage: scanState.coverage to coverage: [] now fails your write-outputs test, and findings: scanState.sorted to findings: [] now fails the e2e one. Both left the whole suite green on the previous head, so the fixtures were the right fix.
Thanks for restoring riskSummary and nextAction as additive fields rather than arguing the point. --json is byte-identical to a main build on single and multi folder, and so are SARIF, CycloneDX and SPDX, across twelve scenarios. --ratchet is unchanged.
The rebase resolution is right too. Multi-folder --json matches main key for key including order, with warnings between diagnostics and findings, and report.json gained it rather than losing it.
One thing to mention rather than ask you to change: report.json no longer carries lockfileSource. packageManager became source and completeness flattened, both of which are renames with nothing lost, but lockfileSource has no --json equivalent so unifying the schema drops it. Nothing in the repo reads it and the page still renders it from the report data, and you documented the new shape, so I am taking it as is. If you want report.json to stay a strict superset it is one additive line, but not in this PR.
And an unintended good outcome worth telling you about: your new e2e test is now what will catch a field going missing from report.json in my own in-flight branch. That key-set equality assertion is doing more than it was asked to.
|
Merged, thank you @alex-powell. Three rebases on this one and the shape came out right. The thing I keep coming back to is the key-set equality assertion in your e2e test. It is already catching a field that would otherwise go missing from I have filed the |
…uilder #1176 moved the --json and report.json payloads into src/output/scan-json.ts after this branch was written, so the field had to move with them. The single-folder report.json call is gated on --check-overrides while the overrideFindings line beside it is not. That asymmetry is deliberate: an absent skippedOverrideRules means override checking never ran and an empty array means it ran with every rule running, and passing it unconditionally collapses those two. Verified on a real report.json, present with the flag and absent without it. overrideSkipped is optional on MultiFolderScanJsonFolder because that type structurally duplicates MultiFolderScanResult rather than projecting it, so a required member would not be enforced at the call sites and would only break fixtures written before the field existed.
…rules that could not run (#1262) * fix(overrides): record and return the rules that could not run audit() populated skippedDetectors in five places, had ten detectors honouring it, and then never returned it, so a rule that could not run was indistinguishable from a rule that ran and found nothing. The recording was also only in catch blocks, so the common case, the network opt-in simply being absent, recorded nothing at all. That is now recorded, with a separate reason when the opt-in is present but no advisory source was wired, since that one is ours to fix rather than the user's. Nothing is recorded when the manifest declares no overrides, matching the guard the lockfile and node_modules skips already use: there is no audit to be incomplete. The batch registry fetchers now report which lookups failed instead of dropping the names, so a total failure records a skip rather than leaving OA007 and OA010 reporting nothing. A 404 is classified as the package being absent rather than a failed lookup, because this client only talks to registry.npmjs.org and every private scoped package in an overrides block would otherwise read as a network problem. skippedDetectors.ruleId is now the OverrideRuleId union rather than a bare string: detectors self-suppress by matching their own id against the list, so a typo would compile and then silently never suppress or render anything. * fix(cli): accept --check-network on the scan path and guard it The flag only existed on the overrides subcommand, so the documented scan command rejected it as an unknown option and OA007 and OA010 could never run from the path we document everywhere. Two guards come with it. Without --check-overrides the flag does nothing, so it is refused rather than silently ignored. And both offline flags are refused alongside it, because the network-backed rules query the npm registry for dist-tags and published version lists, which no local advisory database can supply; honouring it only for the advisory source would print "no external advisory calls will be made" and then call the registry anyway. requiresMessage joins conflictMessage so the two shapes of flag error stay consistent, and the existing --allow-private-osv-url message now uses it. --check-network is added to the scan flag list in --help, where it was missing, and the subcommand help no longer documents a conflict that cannot occur there. * fix(scan): build the advisory source OA010 needs, in one place for all three callers The scan path threaded the network opt-in through but never built an advisory source, so OA010 still could not run even once the flag was accepted. The single folder scan, the multi-folder scan and the overrides subcommand each wired the audit up by hand, which is how that drift happened. runOverrideAudit is now the one place it is wired. It builds the context, builds the advisory source when the opt-in is present, closes it in a finally, and forwards the cert-aware fetch into the registry calls so a CA certificate set through `config set ca-cert` applies to OA007 and OA010; NODE_EXTRA_CA_CERTS is only read at process start, which is why it has to be threaded explicitly. Multi-folder was holding a fetch implementation and not passing it, and the overrides subcommand was getting neither that nor a debug logger, on the one surface that accepts --debug and the first place a TLS problem gets investigated. Both now do. * feat(output): disclose override rules that could not run, on every surface A clean bill of health from an audit that skipped a rule is worse than no audit, so the skips now reach the terminal, --json, both HTML reports and SARIF, and are scoped per folder in multi-folder mode. Grouped by reason rather than keyed on rule ids, so it covers the lockfile and node_modules skips as well as the network-backed ones. The headline sentence and the grouping live in one module because they had already drifted to two different pluralisations when they lived in two. The all-clear is qualified when rules were skipped, since an unqualified tick is the line a skimming reader takes away. In --json the field is present whenever override checking ran and absent when it did not, so an empty array means every rule ran rather than being indistinguishable from never having audited. CI and the GitHub App read that file. SARIF puts them in invocations[].toolExecutionNotifications, with a descriptor declared on the driver for every id referenced, since a notification's descriptor resolves against driver.notifications and not driver.rules. Emitted only when there is something to report, so a plain scan's SARIF is unchanged. * fix(oa010): put --check-network in the fix command for the rule that needs it The finding printed `cve-lite <path> overrides --fix --rule OA010`, and running that reported no issues followed by a notice that OA010 had not run. Now that OA010 is reachable from the main scan the contradiction is visible rather than theoretical. * test(overrides): pin the disclosure on every surface it has to reach Each of these kills a mutation that previously left the whole suite green. The advisory source could stop being built, which is the original bug, and nothing failed. The notice could be deleted from the terminal, from the JSON writer, from the HTML report data, from the multi-folder JSON, from the overrides subcommand or from either HTML reporter, and nothing failed. The cert-aware fetch could stop being forwarded, the custom OSV url could stop being passed, and the stylesheet rule for the HTML notice could be removed, and nothing failed. tests/override-audit.test.ts exercises the module the fix lives in, which nothing imported before: the two suites that named it mocked it away. Three existing suites were retargeted because the shared helper moved the seam they mock at, and two tests that were quietly reaching the real npm registry now use an injected fetch. * docs: say what --check-network actually does and where it works The CLI reference described it as OA007 only, omitted OA010, and showed it only under the overrides subcommand. It now names both rules, says it is accepted on a plain scan alongside --check-overrides, and says both rules are reported as skipped without it. The Action table previously implied the flag works on the inline check-overrides path. It does not: action.yml only reads it in the dedicated audit step, so it needs overrides: "true". Wiring it into the scan step is a separate change. * fix(output): thread the skipped rules through the unified scan-json builder #1176 moved the --json and report.json payloads into src/output/scan-json.ts after this branch was written, so the field had to move with them. The single-folder report.json call is gated on --check-overrides while the overrideFindings line beside it is not. That asymmetry is deliberate: an absent skippedOverrideRules means override checking never ran and an empty array means it ran with every rule running, and passing it unconditionally collapses those two. Verified on a real report.json, present with the flag and absent without it. overrideSkipped is optional on MultiFolderScanJsonFolder because that type structurally duplicates MultiFolderScanResult rather than projecting it, so a required member would not be enforced at the call sites and would only break fixtures written before the field existed.
Summary
Unifies the HTML report's on-disk
report.json(and the in-page JSON download) with the existing--jsonCLI output shape so tooling built for one works with the other.Direction: HTML/
report.jsonnow matches--json. The--jsonconsumer-visible format is unchanged (shared serializers extracted from the previous inline builders).buildScanJson/buildMultiFolderScanJsoninsrc/output/scan-json.ts--jsonand HTMLreport.jsonwriters through those helpersReportDatafor HTML UI rendering onlyreport.jsonmatches the--jsonpayload;--jsonshape regressions coveredwebsite/docs/html-report.mdCloses #237
Test plan
npm ci && npm run lint:tests && npm run build && node dist/index.js advisories sync && npm testreport.jsonmatches--json