feat: add Root column showing the top-level dependency behind each finding - #920
feat: add Root column showing the top-level dependency behind each finding#920fabioemoutinho wants to merge 3 commits into
Conversation
sonukapoor
left a comment
There was a problem hiding this comment.
Fabio, this is a genuinely useful addition - showing the top-level dependency behind a transitive finding is exactly the kind of actionable context we like to give people, and I love that you covered all three surfaces (terminal, JSON, HTML) with tests to match. I pulled it down and tried it locally and it reads really well.
Just a couple of small things and then I think it's good to go, both left as inline notes. The main one is an edge case where a direct dependency can end up showing a transitive root; the other is a tiny naming consistency nit. Neither is a big lift.
One admin thing whenever you get a chance: could you add Closes #923 to the PR description? I opened that issue to track the feature. Thanks so much for this - really nice work.
…t labeling getRootDependencies unioned roots across every dependencyPaths entry without checking finding.relationship, so a package classified "direct" (but also reachable via another root's transitive chain, deduped into the same PackageRef) could still surface a contradictory root. Gate it on relationship === "transitive", matching the convention already used by parent-upgrade.ts, transitive-chain-resolver.ts, and npm-transitive-resolution.ts. Also rename the HTML detail view's "Parent:" label to "Root:" so it matches the new column header instead of using two terms for the same concept. Addresses review feedback on OWASP#920.
|
Reran the repro locally and it's sorted - the contradictory row is gone, direct findings show a dash, and transitive ones still resolve their root. One more you might want to grab while you're in here, not a blocker: the expanded HTML detail row still shows a single root (it reads from Thanks for the quick turnaround. |
|
One more before this lands: Heads-up that PR #901 also touches that file, so this is cleanest to do once #901 lands - I'll ping you when it does. |
…nding Vulnerable transitive dependencies gave no indication of which direct dependency pulled them in. The terminal table and JSON output now show a compact "name +N" summary, while the HTML report lists every root dependency in full.
…t labeling getRootDependencies unioned roots across every dependencyPaths entry without checking finding.relationship, so a package classified "direct" (but also reachable via another root's transitive chain, deduped into the same PackageRef) could still surface a contradictory root. Gate it on relationship === "transitive", matching the convention already used by parent-upgrade.ts, transitive-chain-resolver.ts, and npm-transitive-resolution.ts. Also rename the HTML detail view's "Parent:" label to "Root:" so it matches the new column header instead of using two terms for the same concept. Addresses review feedback on OWASP#920.
… roots renderFindingRow is shared between the single-folder and multi-folder HTML reports. The multi-folder report kept its own 6-column table header and colspans, so the new Root <td> shifted its columns out of alignment. Add the matching <th>Root</th> and bump both colspans to 7. Also update the expanded detail row's Context column to list every root dependency (finding.rootDependencies) instead of only the first path's root (finding.primaryParent), so it no longer disagrees with the "name +N" summary shown in the column when multiple roots pull in the same finding. Addresses further review feedback on OWASP#920.
56173d9 to
449354a
Compare
|
Addressed both follow-ups in 449354a (rebased onto latest
Ran the full suite afterward — no regressions (same pre-existing environment-only failures as on |
Closes #923
Summary
name +Nform (e.g.express +2) when multiple direct dependencies pull in the same vulnerable transitive package.--json/ report data): fullrootDependencies: string[]array on each finding.-(there's no meaningful "root" distinct from the package itself).getRootDependencies(finding)helper insrc/utils/finding.ts, mirroring the existinggetPrimaryParentconvention (same length-based filtering of degenerate direct-dependency paths), and gated onrelationship === "transitive"so a package classifieddirectnever shows a contradictory root even if it's also reachable via another root's transitive chain.Parent:label toRoot:so it matches the new column instead of using two terms for the same concept.Test plan
getRootDependenciesunit tests: single root, multiple unique roots across several paths, degenerate direct-dependency path, empty paths, mixed direct+transitive-path edge case.formatRootDependencySummaryunit tests: single name,name +Nfor multiple roots,-fallback, mixed direct+transitive-path edge case.printTabletests: Root column header present,name +Ncompact form,-for direct dependencies.Rootheader present, full comma-separated list of root names,-fallback for direct dependencies,Root:detail label.serializeFindingtest updated to assertrootDependenciesis included.