From c4f72002e19cd84ad531a99e90fbe1bada03e5c4 Mon Sep 17 00:00:00 2001 From: kingthorin Date: Wed, 18 Dec 2024 09:07:45 -0500 Subject: [PATCH] retire: Fix False Positive issue - CHANGELOG > Added fix note. - Repo > Added handling for fall through of essentially "empty" object. - Tests > Added additional unit tests/assertions. Signed-off-by: kingthorin --- addOns/retire/CHANGELOG.md | 3 +++ .../org/zaproxy/addon/retire/model/Repo.java | 7 ++++++- .../addon/retire/model/Vulnerability.java | 3 ++- .../addon/retire/RetireScanRuleUnitTest.java | 19 +++++++++++++++++++ .../addon/retire/model/RepoUnitTest.java | 2 +- .../UpstreamJsRepositoryUnitTest.java | 16 +++++++++++++--- .../addon/retire/model/samples/vulns-all.json | 2 +- 7 files changed, 45 insertions(+), 7 deletions(-) rename addOns/retire/src/test/java/org/zaproxy/addon/retire/{ => model}/UpstreamJsRepositoryUnitTest.java (64%) diff --git a/addOns/retire/CHANGELOG.md b/addOns/retire/CHANGELOG.md index d007710bf00..e50f5e02755 100644 --- a/addOns/retire/CHANGELOG.md +++ b/addOns/retire/CHANGELOG.md @@ -4,6 +4,9 @@ All notable changes to this add-on will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## Unreleased +### Fixed +- An issue that was resulting in False Positives. + ### Changed - Update minimum ZAP version to 2.16.0. - The scan rule now uses a more specific CWE (Issue 8732). diff --git a/addOns/retire/src/main/java/org/zaproxy/addon/retire/model/Repo.java b/addOns/retire/src/main/java/org/zaproxy/addon/retire/model/Repo.java index 2a37acdcac7..8728555cebf 100644 --- a/addOns/retire/src/main/java/org/zaproxy/addon/retire/model/Repo.java +++ b/addOns/retire/src/main/java/org/zaproxy/addon/retire/model/Repo.java @@ -288,7 +288,7 @@ private static VulnerabilityData isVersionVulnerable( vulnData.setRisk(vnext.getRisk()); } } - return vulnData; + return vulnData.getRisk() > 0 ? vulnData : VulnerabilityData.EMPTY; } private static boolean isGoodCandidate(String version) { @@ -307,6 +307,11 @@ private static boolean isGoodCandidate(String version) { return isAllZeros != 0; // Not a good value if all zero } + /** For testing purposes only */ + Map getEntries() { + return entries; + } + public static class VulnerabilityData { public static final VulnerabilityData EMPTY = new VulnerabilityData(); diff --git a/addOns/retire/src/main/java/org/zaproxy/addon/retire/model/Vulnerability.java b/addOns/retire/src/main/java/org/zaproxy/addon/retire/model/Vulnerability.java index acf0dd01718..ac23cccc283 100644 --- a/addOns/retire/src/main/java/org/zaproxy/addon/retire/model/Vulnerability.java +++ b/addOns/retire/src/main/java/org/zaproxy/addon/retire/model/Vulnerability.java @@ -29,7 +29,8 @@ @JsonIgnoreProperties({"above", "cwe"}) public class Vulnerability { - private static final Map SEVERITY_MAP = + /** Increased visibility for testing purposes only */ + static final Map SEVERITY_MAP = Map.of("high", Alert.RISK_HIGH, "medium", Alert.RISK_MEDIUM, "low", Alert.RISK_LOW); private String below; diff --git a/addOns/retire/src/test/java/org/zaproxy/addon/retire/RetireScanRuleUnitTest.java b/addOns/retire/src/test/java/org/zaproxy/addon/retire/RetireScanRuleUnitTest.java index 0e139b91a59..d80a2f77d29 100644 --- a/addOns/retire/src/test/java/org/zaproxy/addon/retire/RetireScanRuleUnitTest.java +++ b/addOns/retire/src/test/java/org/zaproxy/addon/retire/RetireScanRuleUnitTest.java @@ -21,6 +21,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertSame; @@ -183,6 +184,24 @@ void shouldRaiseAlertOnVulnerableContent() { assertEquals(4, alertsRaised.get(0).getTags().size()); } + @Test + void shouldNotRaiseAlertOnNonVulnerableContent() { + // Given + String content = + "/*!\n" + + " * Bootstrap v3.4.0 (http://getbootstrap.com)\n" + + " * Copyright 2011-2016 Twitter, Inc.\n" + + " * Licensed under the MIT license\n" + + " */"; + HttpMessage msg = createMessage("http://example.com/angular.min.js", content); + msg.getResponseHeader().setHeader(HttpFieldsNames.CONTENT_TYPE, "text/javascript"); + given(passiveScanData.isPage200(any())).willReturn(true); + // When + scanHttpResponseReceive(msg); + // Then + assertThat(alertsRaised, hasSize(0)); + } + @Test void shouldRaiseAlertOnHashOfVulnerableContent() { // Given diff --git a/addOns/retire/src/test/java/org/zaproxy/addon/retire/model/RepoUnitTest.java b/addOns/retire/src/test/java/org/zaproxy/addon/retire/model/RepoUnitTest.java index 4854caba7df..264bde27f0b 100644 --- a/addOns/retire/src/test/java/org/zaproxy/addon/retire/model/RepoUnitTest.java +++ b/addOns/retire/src/test/java/org/zaproxy/addon/retire/model/RepoUnitTest.java @@ -96,7 +96,7 @@ void shouldReadLibWithVulnerabilities() throws IOException { assertThat(identifiers.getCve(), contains("cve 1", "cve 2")); assertThat(identifiers.getSummary(), is("summary")); assertThat(vuln.getInfo(), contains("info 1", "info 2")); - assertThat(vuln.getSeverity(), is("severity")); + assertThat(vuln.getSeverity(), is("high")); } @Test diff --git a/addOns/retire/src/test/java/org/zaproxy/addon/retire/UpstreamJsRepositoryUnitTest.java b/addOns/retire/src/test/java/org/zaproxy/addon/retire/model/UpstreamJsRepositoryUnitTest.java similarity index 64% rename from addOns/retire/src/test/java/org/zaproxy/addon/retire/UpstreamJsRepositoryUnitTest.java rename to addOns/retire/src/test/java/org/zaproxy/addon/retire/model/UpstreamJsRepositoryUnitTest.java index ef5e32e6b2e..da003283e22 100644 --- a/addOns/retire/src/test/java/org/zaproxy/addon/retire/UpstreamJsRepositoryUnitTest.java +++ b/addOns/retire/src/test/java/org/zaproxy/addon/retire/model/UpstreamJsRepositoryUnitTest.java @@ -17,12 +17,16 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.zaproxy.addon.retire; +package org.zaproxy.addon.retire.model; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.in; +import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import java.util.Locale; +import java.util.Set; import org.junit.jupiter.api.Test; -import org.zaproxy.addon.retire.model.Repo; /** Tests for upstream {@code jsrepository.json} file. */ class UpstreamJsRepositoryUnitTest { @@ -31,7 +35,13 @@ class UpstreamJsRepositoryUnitTest { void shouldParseUpstreamJsRepository() { // Given String path = "/org/zaproxy/addon/retire/resources/jsrepository.json"; + Set expectedSeverities = Vulnerability.SEVERITY_MAP.keySet(); // When / Then - assertDoesNotThrow(() -> new Repo(path)); + Repo repo = assertDoesNotThrow(() -> new Repo(path)); + for (RepoEntry entry : repo.getEntries().values()) { + for (Vulnerability vuln : entry.getVulnerabilities()) { + assertThat(vuln.getSeverity().toLowerCase(Locale.ROOT), is(in(expectedSeverities))); + } + } } } diff --git a/addOns/retire/src/test/resources/org/zaproxy/addon/retire/model/samples/vulns-all.json b/addOns/retire/src/test/resources/org/zaproxy/addon/retire/model/samples/vulns-all.json index b72252cb976..4422ee83b78 100644 --- a/addOns/retire/src/test/resources/org/zaproxy/addon/retire/model/samples/vulns-all.json +++ b/addOns/retire/src/test/resources/org/zaproxy/addon/retire/model/samples/vulns-all.json @@ -3,7 +3,7 @@ "vulnerabilities" : [ { "atOrAbove" : "atorabove", "below" : "below", - "severity" : "severity", + "severity" : "high", "cwe" : [ "cwe 1", "cwe 2" ], "identifiers" : { "CVE" : [ "cve 1", "cve 2" ],