Recommend the smallest upgrade that clears an advisory, not the highest

The rollup took the highest fix version across a component's advisories. That
is wrong whenever an advisory is patched on more than one release branch.

nanoid is the case that exposed it. Both advisories against it are fixed on two
branches - 3.3.16 and 5.1.16, 3.3.18 and 5.1.6 - and the rollup recommended
5.1.16 with a MAJOR warning. But nanoid is transitive, pulled in by postcss at
`^3.3.11`, a range no 5.x release can satisfy. The recommendation was not merely
a bigger job than necessary, it was unreachable: the actual target is 3.3.18, a
patch inside the existing range.

Per advisory the target is now the lowest published fix above the version in
use; per component it is the highest of those, since one upgrade has to satisfy
every advisory against it. Fixes on older branches are skipped rather than
offered as a downgrade - axios advisories list 0.30.x fixes that would have been
a downgrade from 1.13.2.

This also gives the MAJOR label meaning. It now fires only when no fix exists on
the current branch, rather than whenever some branch happened to be higher.

docs/DEPENDENCY-SECURITY.md

@@ -76,10 +76,17 @@ add an entry to make a build pass.
 
 ## What the report tells you to do
 
-The report ends with a per-component upgrade rollup: the highest fix version
+The report ends with a per-component upgrade rollup: the smallest upgrade that
-across that component's advisories, and how many it clears. Upgrading once to
+clears every advisory against that component, and how many it clears.
-the top of the list clears the ones below it.
 
-A target on a different major version is labelled `MAJOR`. It is a different
+"Smallest" is doing real work there. An advisory patched on several release
-job from a patch bump - it can be ESM-only, or drop an API - and it is marked
+branches lists a fix per branch - nanoid's is fixed in both 3.3.18 and 5.1.16 -
-rather than listed as though the two cost the same.
+and taking the highest recommends a major upgrade when a patch on the current
+branch would do. Where the package is transitive that major may not even be
+reachable: nanoid arrives under postcss, pinned `^3.3.11`, a range 5.x cannot
+satisfy. So the target is the lowest published fix above the version in use.
+Fixes on older branches are ignored rather than offered as a downgrade.
+
+A target on a different major version is still labelled `MAJOR`, but now that
+label means something: no fix exists on the current branch, so the upgrade is
+genuinely a different job - it can be ESM-only, or drop an API.

scripts/scan_sbom.py

@@ -274,21 +274,43 @@ def version_key(version):
     return parts
 
 
-def upgrade_plan(findings):
+def minimal_fix(fixedin, current):
-    """[(component, version, count)] - one row per component that has a fix.
+    """The SMALLEST published fix above the version in use, or None.
 
-    The highest fix version across that component's advisories, because
+    An advisory patched on several release branches lists a fix per branch:
-    upgrading once to the top of the list clears the ones below it.
+    nanoid's is fixed in both 3.3.18 and 5.1.16. Taking the highest recommends
+    a major upgrade when a patch on the current branch would do - and where the
+    package is transitive, that major may not even be reachable, because the
+    parent's semver range forbids it. The smallest fix above the current
+    version is the one that actually clears the advisory.
+
+    Fixes at or below the current version belong to older branches and are
+    ignored; recommending one would be a downgrade.
+    """
+    above = [v for v in fixedin if version_key(v) > version_key(current)]
+    if not above:
+        return None
+    return min(above, key=version_key)
+
+
+def upgrade_plan(findings):
+    """[(component, version, count, major)] - one row per component with a fix.
+
+    Per advisory, the minimal upgrade that clears it; per component, the
+    highest of those, since one upgrade has to satisfy every advisory against
+    it.
     """
     by_component = {}
     for f in findings:
         if f['allowlisted'] or not f['fixedin']:
             continue
+        target = minimal_fix(f['fixedin'], f['version'])
+        if target is None:
+            continue
         name = f['component']
         best, count, current = by_component.get(name, (None, 0, f['version']))
-        for version in f['fixedin']:
+        if best is None or version_key(target) > version_key(best):
-            if best is None or version_key(version) > version_key(best):
+            best = target
-                best = version
         by_component[name] = (best, count + 1, current)
 
     rows = []
@@ -337,7 +359,7 @@ def report(findings, blocking):
     if ship:
         upgrades = upgrade_plan(ship)
         if upgrades:
-            print('\nWHAT TO UPGRADE - shipped components, highest fix seen')
+            print('\nWHAT TO UPGRADE - shipped components, smallest upgrade that clears them')
             for component, version, count, major in upgrades:
                 note = '  MAJOR - check for breaking changes' if major else ''
                 print('  %-30s -> %-12s clears %-3d%s'

tests/test_scan_sbom.py

@@ -142,6 +142,37 @@ class TestUpgradePlan:
         findings = [self._finding('nanoid 3.3.11', '3.3.11', ['5.1.16'])]
         assert scan.upgrade_plan(findings)[0][3] is True
 
+    def test_a_branch_patch_beats_a_major_on_the_same_advisory(self, scan):
+        # The real case this was found on: nanoid is fixed in BOTH 3.3.18 and
+        # 5.1.16, and it is transitive under a postcss range of ^3.3.11 - so
+        # 5.x is not merely a bigger job, it is unreachable. Recommending the
+        # highest fix sent somebody to a major upgrade they could not make.
+        findings = [self._finding('nanoid 3.3.11', '3.3.11',
+                                  ['3.3.18', '5.1.16'])]
+        component, version, _count, major = scan.upgrade_plan(findings)[0]
+        assert version == '3.3.18'
+        assert major is False
+
+    def test_component_target_satisfies_every_advisory_against_it(self, scan):
+        # Two advisories, minimal fixes 3.3.16 and 3.3.18: the row must be the
+        # higher, or the upgrade clears one and leaves the other.
+        findings = [
+            self._finding('nanoid 3.3.11', '3.3.11', ['3.3.16', '5.1.16']),
+            self._finding('nanoid 3.3.11', '3.3.11', ['3.3.18', '5.1.6']),
+        ]
+        assert scan.upgrade_plan(findings)[0][1] == '3.3.18'
+
+    def test_a_fix_on_an_older_branch_is_never_a_downgrade(self, scan):
+        # axios advisories list fixes on the 0.30.x line too; on 1.13.2 those
+        # are behind us and must not be recommended.
+        findings = [self._finding('axios 1.13.2', '1.13.2',
+                                  ['0.30.2', '1.15.2'])]
+        assert scan.upgrade_plan(findings)[0][1] == '1.15.2'
+
+    def test_only_older_branch_fixes_means_no_row(self, scan):
+        findings = [self._finding('axios 1.13.2', '1.13.2', ['0.30.2'])]
+        assert scan.upgrade_plan(findings) == []
+
     def test_a_patch_bump_is_not_flagged(self, scan):
         findings = [self._finding('pyjwt 2.12.1', '2.12.1', ['2.13.0'])]
         assert scan.upgrade_plan(findings)[0][3] is False