fix(remediation): drop a stale command when a merged target is raised - #1142
Merged
Merged
Conversation
When one package enters the plan twice, upsertTarget raises targetVersion to the higher of the two but kept `existing.command ?? next.command`, so an explicit command string built for the lower version survived the merge. buildTargetCommand prefers an explicit command over deriving one from targetVersion, so the stale string won whenever the target had no workspace scoping to override it. Measured on a real repo: nx validated at 22.7.7 after 18 versions were scanned and 17 found still vulnerable, while the emitted command read `pnpm add nx@22.5.1`. recommendedAction is derived from the finding and stayed correct, which is why the two disagreed. The mirror case is guarded too: when the existing target wins, a command built for the lower incoming version is equally stale.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A copy-run fix command could name a version the scan itself had already determined was still vulnerable.
Measured on a real repo, for
nx@22.5.0:22.7.2, which validation found still vulnerable (18 versions scanned above current, 17 still vulnerable)validatedFirstFixedVersion:22.7.7recommendedAction: "Upgrade nx to 22.7.7+", correctrunnableFixCommand:pnpm add nx@22.5.1, neither of thoseA package can enter the plan twice. Here
axioscarried a transitive remediation whosepackagewasnx, creating a chain-resolution target with an explicit command built fornx@22.5.1, whilenxalso entered as a direct target at the validated22.7.7.upsertTargetraisedtargetVersioncorrectly but keptexisting.command ?? next.command, andbuildTargetCommandprefers an explicit command over deriving one fromtargetVersion.recommendedActionis derived from the finding fields, which is why it stayed right while the command went wrong.The mirror case is guarded as well: when the existing target wins the comparison, a command built for the lower incoming version is equally stale.
Verified with a differential across eight projects and 51 direct findings with commands: zero disagreements between the emitted command and the validated fix version, where the new test fails without the change.
Closes #1140
Related to #1007 and #1113. The review on #1113 asked for
plan.targetsas well asplan.command;runnableFixCommandreads from targets, so this is that half. Original diagnosis of the underlying cross-wiring belongs to @osfv.