Skip to content

fix(remediation): name the child in npm within-range parent updates (#1286) - #1287

Merged
sonukapoor merged 1 commit into
OWASP:mainfrom
alex-powell:fix/issue-1286-exact-pinned-parent-update
Oct 20, 2026
Merged

sonukapoor merged 1 commit into
OWASP:mainfrom
alex-powell:fix/issue-1286-exact-pinned-parent-update

Conversation

@alex-powell

Copy link
Copy Markdown
Contributor

What changed and why

For an npm transitive finding whose direct parent already allows a fixed child version (update-parent-within-range), the "parent updates within range" section suggested npm update <parent>. That command does not refresh the child: npm update only re-resolves the packages it is given, and a child that still satisfies the parent's range is left alone.

Checked on npm 9 and npm 10 with astro 5.13.2 and devalue 5.9.2 (astro declares devalue: ^5.1.1, fixed in 5.9.4):

astro in package.json npm update astro npm update devalue
5.13.2 (exact) no change, devalue stays 5.9.2 devalue 5.9.4
^5.13.2 astro moves to 5.18.2, devalue still 5.9.2 devalue 5.9.4

So the exact pin in #1286 is the most visible case, but naming the parent is the root cause. The deep-chain npm path and the pnpm / yarn / bun lockfile refresh already name the child; this brings the direct-parent npm path in line.

  • src/remediation/fix-commands.ts: the npm within-range command is now npm update <child>, with the existing workspace scoping (--workspace=<ws> / --workspaces). The target keeps the shape from fix(remediation): stop parent-update rendering a child's version as the parent's #1174: package and targetVersion are the parent's, the child is on childPackage / childTargetVersion. The no-explicit-command fallback in buildCommandForTargets names childPackage too.
  • src/utils/fix-runner.ts: --fix already runs the target's command, so it picks up the fix; only the comment about shared commands is updated.
  • examples/exact-pinned-parent-within-range: hand-crafted fixture (astro exact at 5.13.2, devalue 5.9.2), listed in examples/readme.md.
  • Tests: new fixture test in tests/fixture-scan.test.ts (remediation, plan command, target fields, findSuggestedCommandForFinding, buildTargetCommand), workspace-scoped variants, --fix runner dedupe cases, and existing assertions that expected npm update <parent> updated to the child.
  • Docs: README, how-it-works.md, remediation-strategy.mdx, comparison.md, comparison/npm-audit.md and comparison/snyk.md described this command as npm update <parent>; reworded to the child refresh.

npm run build, npm run lint:tests and npm test pass (163 suites, 2253 tests).

Fixes #1286

@prx-my

prx-my commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Heads-up: this branch is currently behind main, which blocks the merge. Could you update it?

  • Click Update branch on this PR, or
  • Rebase locally:
    git fetch upstream
    git rebase upstream/main
    git push --force-with-lease
    

Once updated, CI will re-run. Thanks!

@sonukapoor

Copy link
Copy Markdown
Collaborator

I owe you a straight explanation here.

Yesterday I hit this same bug from a different direction, through an adopter's scan artefacts, diagnosed it, and merged a fix in #1313 without reading far enough down the review queue to find that you had already reported it in #1286 and already fixed it here. Your report predates my work by seven days and your PR predates my branch by six. Our own rule gives the oldest duplicate priority, and that was yours.

Your diagnosis is also better than mine. I had concluded the defect fires when the parent is already at its newest allowed version, which is the case I happened to measure. Your table across both pin styles and two npm majors shows the real rule, that npm update only re-resolves the packages it is given, so naming the parent never refreshes the child whether or not the parent moves. I have corrected my own notes accordingly.

Two things in your PR are not on main and are still wanted: the buildCommandForTargets fallback naming childPackage, which I looked at and wrongly dismissed as unreachable, and the examples/exact-pinned-parent-within-range fixture.

#1313 has made this conflict, which is my doing and not something I am going to hand back to you. I will rebase your commits myself, keeping your authorship so it lands under your name, and carry the two pieces above. If you would rather do it, say so and I will leave it alone.

…WASP#1286)

For an npm transitive finding whose direct parent already allows a fixed
child version, the suggested command was `npm update <parent>`. npm update
only re-resolves the packages it is given, so a child that still satisfies
the parent range stays on the vulnerable version. With an exact-pinned
parent the command changes nothing at all.

Emit `npm update <child>` instead, keeping workspace scoping, and keep the
target shape from OWASP#1174 (package and targetVersion belong to the parent,
the child is on childPackage / childTargetVersion). The --fix runner uses
the same command. Adds the exact-pinned-parent-within-range fixture and
updates docs that described the command as `npm update <parent>`.

Fixes OWASP#1286
@sonukapoor
sonukapoor force-pushed the fix/issue-1286-exact-pinned-parent-update branch from 888270f to 94c731f Compare October 15, 2026 01:51
@sonukapoor

Copy link
Copy Markdown
Collaborator

Rebased this onto main for you. The conflict was mine to clear: #1313 landed my own partial fix for the same bug and moved these files underneath you, so that was not yours to untangle. Your commit is unchanged and still authored to you.

Worth saying what main has absorbed in the meantime, because the diff now reads smaller than it did. The line naming the child in the within-range command is already identical on main, and #1322 rewrote the chain matching and the target key around it. What this PR still carries that main does not: the fallback that names the child when several parent updates collapse into one command, and the exact-pinned-parent-within-range fixture with the scan coverage that goes with it.

Suite and build were green on the rebased branch. Nothing needed from you here. I will take the review from this point.

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

Picking this up again after rebasing it onto current main. The short version is that the bug this was opened for is already fixed, by our own #1313 on 2026-10-11, which closed #1286. That is not a criticism of your work: #1313 made the same call you made here, for the same reason, and your PR predates it by a week.

What that leaves is still worth landing, and one part of it is something we owe users.

The docs half is the valuable part now. README.md, how-it-works.md, comparison.md, comparison/npm-audit.md, comparison/snyk.md and remediation-strategy.mdx all still tell the reader that a within-range transitive fix is npm update <parent>. That has been false since #1313 shipped, so right now the product does one thing and six documents say another. Your rewording is the correction, and nothing else in the queue covers it.

The tests are real coverage rather than decoration, which I checked rather than assumed. Putting the pre-#1313 behaviour back, so the within-range command names the parent again, fails 9 tests across 5 suites on your branch against 6 across 3 suites on main alone. So your fixture and the workspace-scoped cases catch three regressions that main currently would not, including the end-to-end path through an actual lockfile.

One thing to drop, and it is mine to explain rather than yours to have known. The change in buildCommandForTargets, naming childPackage ?? package in the merged command, cannot run. Every parent-update target carries a command by construction, the args type requires it, so explicitCommands is never empty when there is a parent-update target and that npm branch is unreachable. I instrumented it and it was reached zero times across the full suite and six example trees. That is why reverting that line alone leaves all 2376 tests green: not a coverage gap, just dead code.

It is harmless and arguably more correct if the branch ever becomes live, so I am not going to make you remove it. I am noting it because the PR description leads on it, and because the dead branch itself is pre-existing and ours to clean up separately.

Two things for the description, since I rebased this and the body no longer matches the diff. The headline is now docs and coverage rather than a behaviour fix, and Fixes #1286 refers to an issue closed six days ago, so refs #1286 reads truer.

Neither holds this up. I am approving it, and rather than ask you to push again for a wording change on a branch I rebased myself, I will write the squash message to describe what actually lands. Edit the body yourself if you would prefer; it needs no new commit.

Approving. I merge these one at a time rather than in a batch, so it will land shortly.

Merge result against current main is clean, 168 suites and 2376 tests green, build clean.

@sonukapoor
sonukapoor merged commit 5997922 into OWASP:main Oct 20, 2026
9 checks passed
@sonukapoor

Copy link
Copy Markdown
Collaborator

Merged, thank you @alex-powell.

Worth saying plainly how this went, since you were on the wrong end of it. You reported the bug and opened this PR in the same minute on 4 October. We did not review it. On the 9th we opened our own fix for the same bug in the same files, merged that on the 11th, closed your issue by hand, and only then wrote to you. Our merge is also what put this branch into conflict, which is why it ended up being rebased for you. None of that should have happened in that order, and the reason it did is that nobody looked at this PR for seven days.

What landed here is the part our fix did not cover, and it is the part that keeps it honest: the exact-pinned-parent fixture with scan coverage, the workspace-scoping cases, and the fix-runner handling. The fixture is the piece I am most glad to have, because it is what stops this regressing quietly.

Verified on the merge result against current main: build clean, 169 suites and 2398 tests green.

You will be credited in the v1.40.0 notes both as the reporter and for the original fix, not just for this PR.

@sonukapoor sonukapoor mentioned this pull request Oct 21, 2026
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.

[Bug] parent-update suggests npm update <parent> that cannot refresh the child when the parent is exact-pinned

3 participants