diff --git a/docs/DEPENDENCY-SECURITY.md b/docs/DEPENDENCY-SECURITY.md index 25d8cbbb..0ef9b9ef 100644 --- a/docs/DEPENDENCY-SECURITY.md +++ b/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 -across that component's advisories, and how many it clears. Upgrading once to -the top of the list clears the ones below it. +The report ends with a per-component upgrade rollup: the smallest upgrade that +clears every advisory against that component, and how many it clears. -A target on a different major version is labelled `MAJOR`. It is a different -job from a patch bump - it can be ESM-only, or drop an API - and it is marked -rather than listed as though the two cost the same. +"Smallest" is doing real work there. An advisory patched on several release +branches lists a fix per branch - nanoid's is fixed in both 3.3.18 and 5.1.16 - +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. diff --git a/scripts/scan_sbom.py b/scripts/scan_sbom.py index c6b7b300..70cd0138 100644 --- a/scripts/scan_sbom.py +++ b/scripts/scan_sbom.py @@ -274,21 +274,43 @@ def version_key(version): return parts -def upgrade_plan(findings): - """[(component, version, count)] - one row per component that has a fix. +def minimal_fix(fixedin, current): + """The SMALLEST published fix above the version in use, or None. - The highest fix version across that component's advisories, because - upgrading once to the top of the list clears the ones below it. + An advisory patched on several release branches lists a fix per branch: + 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(version) > version_key(best): - best = version + if best is None or version_key(target) > version_key(best): + best = target 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' diff --git a/tests/test_scan_sbom.py b/tests/test_scan_sbom.py index de81b0bb..fd55d3f1 100644 --- a/tests/test_scan_sbom.py +++ b/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