fix(remediation): stop parent-update rendering a child's version as the parent's - #1174
Conversation
Breaks a Jest ESM circular import that left formatParentUpdateChildNote unavailable when loading the CLI entrypoint in integration tests.
sonukapoor
left a comment
There was a problem hiding this comment.
This is a good fix and it is more valuable than the issue I filed described. Verified rather than read: built it, ran it against nine npm examples with a shared warm cache, and diffed full output against main.
The bug is worse on main than #1152 says. On examples/wrong-parent:
main: karma 6.4.4 -> 1.20.8 [breaking]
PR: karma 6.4.4 -> 6.4.4 refresh body-parser to 1.20.8
1.20.8 is body-parser's version, so main reads as a three-major downgrade of karma and raises a false breaking badge on what is actually a refresh within range. Same shape on the axios/form-data case in the issue.
The commands are provably unchanged, which was the thing I most wanted to confirm given #1113. Of nine npm examples, five differ by exactly one line, the Target cell, and four are byte-identical. Every emitted command line is the same. Structurally that holds because buildTargetCommand short-circuits on kind === "parent-update" && target.command before any interpolation, so targetVersion never reaches a command string.
And it closes a second bug neither of us had spotted. On main, the axios direct finding was matching the parent-update target, because the direct branch accepts compareVersions(item.targetVersion, directTarget) >= 0 and 4.0.6 >= 1.19.0 is true. So main handed a user npm update axios to fix axios's own CVE. After this change 1.16.1 >= 1.19.0 is false and the finding correctly gets npm install axios@1.19.0. Worth calling out in the PR description, because it is a real user-facing fix hiding inside a rendering change.
SARIF and CycloneDX are purely additive, create-pr improves (its advisory list was empty before because it looked up the parent), and --fix never lets targetVersion reach a package manager.
One thing before I merge it.
src/scan/single-scan.ts has no coverage. I reverted it on its own and the suite stayed at 1944/1944 green, so nothing there is tested. Two changes ship untested in that file, and both feed automation rather than a human reading output: the --fix --json applied[].advisories lookup switching to entry.childPackage ?? entry.package, and the new cve.fix.applied audit fields. You already wrote a test for the equivalent create-pr change, so this is asking for consistency rather than new work.
Three smaller things, none blocking, take or leave them:
The dedupe in fixTargetContext is a substring check on reason. If upsertTarget raises childTargetVersion while reason still names the lower one, the context reads refresh form-data to 4.0.9. axios@1.16.1 already allows form-data@4.0.6. Narrow and cosmetic.
Target 6.4.4 on a row whose Version is 6.4.4 reads as a no-op. Accurate, but a - might read better. Your call, it is a design question not a defect.
And the comment in findFixTargetForFinding's direct branch saying "Targets are unique per package name, so this cannot select a different package" stopped being true at #1113, and is exactly the assumption that allowed the axios mismatch above. Pre-existing, but it sits in a function you are editing.
…hild fields Extract the --fix JSON applied[] mapping and cve.fix.applied audit payload builders so parent-update childPackage advisory lookup and child fields have the same coverage as create-pr.
|
Addressed the blocking coverage ask: added tests for the single-scan |
sonukapoor
left a comment
There was a problem hiding this comment.
This is a careful piece of work, and I checked it rather than read it.
On examples/wrong-parent the karma target goes from 1.20.8, which is body-parser's version, to 6.4.4, which is karma's own, with the child moved onto childPackage and childTargetVersion. The emitted command stays npm update karma, correctly leaving alone the half #1113 already fixed.
The part that makes this issue harder than it looks is that a parent-update target has two shapes: one where package names the parent, and a self-refresh where package is the vulnerable child itself. Keeping the destination on targetVersion for self-refreshes and moving it to childPackage only for parent-named targets is the right split, and the comment on the matcher explaining that falling through would attach the wrong sibling is exactly the trap. A previous attempt at this issue fell through it.
Coverage follows every surface you touched rather than sampling one: cyclonedx, sarif, create-pr, audit-log, printers and fix-runner all have their own assertions. I mutation-tested the matcher by deleting the childPackage branch and five tests failed, so it is genuinely pinned rather than merely referenced.
No change requests, which is not usual on 800 lines, and I went looking.
|
Merged, thank you @alex-powell, and welcome. A first contribution that resolves an ambiguity in the data model rather than working around it is a good way to start. |
|
You're welcome @sonukapoor |
|
@alex-powell Thanks, Alex - I would love to know more about the use of CVEL in your project. Are you using it as part of your workflow at your company? |
@sonukapoor I can't go into detail, but you can look me up on LinkedIn to see my industry. Apologies but I can only be vague |
What changed and why
#1113 split the
parent-updatekeyspace so the emitted command is correct. It left the target shape alone:packageis the parent (axios) buttargetVersionwas the child's version (form-data@4.0.6). Every renderer treatstargetVersionas this package's version, so the terminal table, HTML, JSON, andisBreakingUpgradeall implied axios was jumping three majors — the remaining half of #1007 / #1152.targetVersionnow always belongs topackage. For a parent-update that names the parent rather than the child itself, that is the parent's unchanged version, and the child moves ontochildPackage/childTargetVersion. Self-refreshes (deep-chainnpm update, pnpm/yarn/bun lockfile refresh) still keep the child's destination ontargetVersion.Renderers show the child in Context / the HTML row / SARIF and CycloneDX fix metadata instead of as an upgrade of the parent.
isBreakingUpgradeneeds no special case: comparing the parent to itself is not a breaking upgrade.--fixalso spawnednpm update <parent>once per child. The displayed plan already dedupes that command; the runner now does the same, and each applied record still names the child it refreshed.Verification
npx tsc --noEmit--runInBand):tests/output/parent-update-display.test.ts,tests/remediation/fix-commands-parent-update-merge.test.ts,tests/output.test.ts,tests/sarif.test.ts,tests/cyclonedx.test.ts,tests/fix-runner-apply.test.ts— 6 suites, 237 passedCloses #1152