From 560a3744802d2d68c177eae4816a7fed266a7ac6 Mon Sep 17 00:00:00 2001 From: Sam Snyder Date: Mon, 22 Jul 2024 23:49:42 -0700 Subject: [PATCH] Fix a subtle issue with missed dependency upgrades due to advisory database sometimes missing a ".RELEASE" suffix in its fixed versions. --- .../DependencyVulnerabilityCheck.java | 77 ++++++++++++++----- .../java/dependencies/Vulnerability.java | 2 +- .../DependencyVulnerabilityCheckTest.java | 8 +- 3 files changed, 64 insertions(+), 23 deletions(-) diff --git a/src/main/java/org/openrewrite/java/dependencies/DependencyVulnerabilityCheck.java b/src/main/java/org/openrewrite/java/dependencies/DependencyVulnerabilityCheck.java index eede2c0..b6e1b1a 100644 --- a/src/main/java/org/openrewrite/java/dependencies/DependencyVulnerabilityCheck.java +++ b/src/main/java/org/openrewrite/java/dependencies/DependencyVulnerabilityCheck.java @@ -242,11 +242,22 @@ public Tree visit(@Nullable Tree tree, ExecutionContext ctx) { gav.getValue().getVersion(), null, overrideTransitive, null) .getVisitor(acc.getMavenUdvAcc()) .visitNonNull(t, ctx); + if (t == t2 && gav.getValue().isDotReleaseAmbiguous()) { + t2 = new UpgradeDependencyVersion(gav.getKey().getGroupId(), gav.getKey().getArtifactId(), + gav.getValue().getVersion() + ".RELEASE", null, overrideTransitive, null) + .getVisitor(acc.getMavenUdvAcc()) + .visitNonNull(t, ctx); + } if (t == t2) { if (Boolean.TRUE.equals(overrideTransitive)) { AddManagedDependency amd = new AddManagedDependency(gav.getKey().getGroupId(), gav.getKey().getArtifactId(), gav.getValue().getVersion(), null, null, null, null, null, null, null); t2 = amd.getVisitor(amd.getInitialValue(ctx)) .visitNonNull(t, ctx); + if (t == t2 && gav.getValue().isDotReleaseAmbiguous()) { + amd = new AddManagedDependency(gav.getKey().getGroupId(), gav.getKey().getArtifactId(), gav.getValue().getVersion() + ".RELEASE", null, null, null, null, null, null, null); + t2 = amd.getVisitor(amd.getInitialValue(ctx)) + .visitNonNull(t, ctx); + } if (t != t2) { t = CommitMessage.message(t2, DependencyVulnerabilityCheck.this, requireNonNull(gav.getValue().getBecause())); } @@ -262,23 +273,35 @@ public Tree visit(@Nullable Tree tree, ExecutionContext ctx) { if (!vulnerabilities.getGavToVulnerabilities().isEmpty()) { t = enumerateFixesGradleGroovy(vulnerabilities.getGavToVulnerabilities(), acc.getScope(), fixes).visitNonNull(t, ctx); for (Map.Entry gaToVb : fixes.entrySet()) { - String because = gaToVb.getValue().getBecause(); + VersionBecause vb = gaToVb.getValue(); Tree t2 = new org.openrewrite.gradle.UpgradeDependencyVersion(gaToVb.getKey().getGroupId(), gaToVb.getKey().getArtifactId(), - gaToVb.getValue().getVersion(), null) + vb.getVersion(), null) .getVisitor(acc.getGradleUdvAcc()) .visitNonNull(t, ctx); + if (t == t2 && vb.isDotReleaseAmbiguous()) { + t2 = new org.openrewrite.gradle.UpgradeDependencyVersion(gaToVb.getKey().getGroupId(), gaToVb.getKey().getArtifactId(), + vb.getVersion() + ".RELEASE", null) + .getVisitor(acc.getGradleUdvAcc()) + .visitNonNull(t, ctx); + } if (t == t2) { if (Boolean.TRUE.equals(overrideTransitive)) { t2 = new UpgradeTransitiveDependencyVersion(gaToVb.getKey().getGroupId(), gaToVb.getKey().getArtifactId(), - gaToVb.getValue().getVersion(), null, because, null) + vb.getVersion(), null, vb.getBecause(), null) .getVisitor() .visitNonNull(t, ctx); + if(t == t2 && vb.isDotReleaseAmbiguous()) { + t2 = new UpgradeTransitiveDependencyVersion(gaToVb.getKey().getGroupId(), gaToVb.getKey().getArtifactId(), + vb.getVersion() + ".RELEASE", null, vb.getBecause(), null) + .getVisitor() + .visitNonNull(t, ctx); + } if (t != t2) { - t = CommitMessage.message(t2, DependencyVulnerabilityCheck.this, because); + t = CommitMessage.message(t2, DependencyVulnerabilityCheck.this, vb.getBecause()); } } } else { - t = CommitMessage.message(t2, DependencyVulnerabilityCheck.this, because); + t = CommitMessage.message(t2, DependencyVulnerabilityCheck.this, vb.getBecause()); } } } @@ -363,6 +386,8 @@ private static class VersionBecause { @Nullable String because; + + boolean dotReleaseAmbiguous; } private void analyzeDependency( @@ -376,20 +401,26 @@ private void analyzeDependency( nextVulnerability: for (Vulnerability v : vs) { + // Some dependencies have a ".RELEASE" suffix. + // For example spring-security-core had a .RELEASE suffix for versions >=2.0.5 and <5.4.0. No suffixes since then + // The vulnerability database is inconsistent about whether the ".RELEASE" is included in the fixed version + // This inconsistency complicates comparisons because "5.3.0" != "5.3.0.RELEASE" + // This inconsistency complicates dependency upgrade since we don't know which version number format to request + // Therefore ignore the suffix during comparison but record it so that version upgrades can try both with and without the suffix + // The edge case of ".RELEASE" being introduced into a version scheme between patch versions is possible but hopefully rare + boolean dotReleaseAmbiguous = resolvedDependency.getVersion().endsWith(".RELEASE") && !v.getFixedVersion().endsWith(".RELEASE"); boolean isLessThanFixed = StringUtils.isBlank(v.getFixedVersion()); - if (!isLessThanFixed) { - if (resolvedDependency.getVersion().endsWith(".RELEASE")) { - String versionWithoutRelease = resolvedDependency.getVersion().substring(0, - resolvedDependency.getVersion().length() - ".RELEASE".length()); - if (vc.compare(versionParser.transform(v.getFixedVersion()), versionParser.transform(versionWithoutRelease)) > 0) { - isLessThanFixed = true; - } - } else if (vc.compare(versionParser.transform(v.getFixedVersion()), versionParser.transform(resolvedDependency.getVersion())) > 0) { - isLessThanFixed = true; - } + if (!isLessThanFixed + && vc.compare( + versionParser.transform(stripExtraneousVersionSuffix(v.getFixedVersion())), + versionParser.transform(stripExtraneousVersionSuffix(resolvedDependency.getVersion()))) > 0) { + isLessThanFixed = true; } - if (isLessThanFixed && vc.compare(versionParser.transform(v.getIntroducedVersion()), versionParser.transform(resolvedDependency.getVersion())) <= 0) { + if (isLessThanFixed + && vc.compare( + versionParser.transform(stripExtraneousVersionSuffix(v.getIntroducedVersion())), + versionParser.transform(stripExtraneousVersionSuffix(resolvedDependency.getVersion()))) <= 0) { if (gavVs == null) { gavVs = vulnerabilities.computeIfAbsent(resolvedDependency.getGav(), ga -> new TreeSet<>( Comparator.comparing((MinimumDepthVulnerability vDep) -> vDep.getVulnerability().getSeverity()).reversed() @@ -403,7 +434,7 @@ private void analyzeDependency( } } - gavVs.add(new MinimumDepthVulnerability(resolvedDependency.getDepth(), v)); + gavVs.add(new MinimumDepthVulnerability(resolvedDependency.getDepth(), v, dotReleaseAmbiguous)); } } } @@ -435,7 +466,7 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) { if (!StringUtils.isBlank(v.getFixedVersion()) && new LatestPatch(null).isValid(gav.getVersion(), v.getFixedVersion())) { if (fixVersion == null || new StaticVersionComparator().compare(versionParser.transform(v.getFixedVersion()), versionParser.transform(fixVersion)) > 0) { - fixes.put(ga, new VersionBecause(v.getFixedVersion(), v.getCve())); + fixes.put(ga, new VersionBecause(v.getFixedVersion(), v.getCve(), vDepth.dotReleaseAmbiguous)); } } } @@ -501,7 +532,7 @@ public G.CompilationUnit visitCompilationUnit(G.CompilationUnit cu, ExecutionCon if (!StringUtils.isBlank(v.getFixedVersion()) && new LatestPatch(null).isValid(gav.getVersion(), v.getFixedVersion())) { if (fixVersion == null || new StaticVersionComparator().compare(versionParser.transform(v.getFixedVersion()), versionParser.transform(fixVersion)) > 0) { - fixes.put(ga, new VersionBecause(v.getFixedVersion(), v.getCve())); + fixes.put(ga, new VersionBecause(v.getFixedVersion(), v.getCve(), vDepth.dotReleaseAmbiguous)); } } } @@ -519,5 +550,13 @@ public static class MinimumDepthVulnerability { @NonFinal int minDepth; Vulnerability vulnerability; + boolean dotReleaseAmbiguous; + } + + private static String stripExtraneousVersionSuffix(String version) { + if (version.endsWith(".RELEASE")) { + return version.substring(0, version.length() - ".RELEASE".length()); + } + return version; } } diff --git a/src/main/java/org/openrewrite/java/dependencies/Vulnerability.java b/src/main/java/org/openrewrite/java/dependencies/Vulnerability.java index a70c938..cd2203b 100644 --- a/src/main/java/org/openrewrite/java/dependencies/Vulnerability.java +++ b/src/main/java/org/openrewrite/java/dependencies/Vulnerability.java @@ -21,7 +21,7 @@ import java.time.ZonedDateTime; @Value -@EqualsAndHashCode(onlyExplicitlyIncluded = false) +@EqualsAndHashCode public class Vulnerability { @EqualsAndHashCode.Include String cve; diff --git a/src/test/java/org/openrewrite/java/dependencies/DependencyVulnerabilityCheckTest.java b/src/test/java/org/openrewrite/java/dependencies/DependencyVulnerabilityCheckTest.java index 924f47a..3065cf3 100644 --- a/src/test/java/org/openrewrite/java/dependencies/DependencyVulnerabilityCheckTest.java +++ b/src/test/java/org/openrewrite/java/dependencies/DependencyVulnerabilityCheckTest.java @@ -191,11 +191,13 @@ void maven() { my-app 1 - + org.springframework.security spring-security-core - 4.2.13.RELEASE + 4.2.16.RELEASE