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 <kingthorin@users.noreply.github.com>
zaproxy · Dec 19, 2024 · 90013c4 · 90013c4
1 parent 3a22683
commit 90013c4
Show file tree

Hide file tree

Showing 7 changed files with 56 additions and 15 deletions.
diff --git 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
@@ -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<String, RepoEntry> 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
@@ -29,7 +29,8 @@
 @JsonIgnoreProperties({"above", "cwe"})
 public class Vulnerability {
 
-    private static final Map<String, Integer> SEVERITY_MAP =
+    /** Increased visibility for testing purposes only */
+    static final Map<String, Integer> 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
@@ -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
@@ -40,10 +40,13 @@
 
 /** Unit test for {@link Repo}. */
 class RepoUnitTest {
+
+    private static final String PATH_PREFIX = "/org/zaproxy/addon/retire/model/samples/";
+
     @Test
     void shouldReadEmptyRepo() throws IOException {
         // Given
-        Reader data = reader("repo-empty.json");
+        Reader data = reader(PATH_PREFIX + "repo-empty.json");
         // When
         Map<String, RepoEntry> entries = Repo.createEntries(data);
         // Then
@@ -53,7 +56,7 @@ void shouldReadEmptyRepo() throws IOException {
     @Test
     void shouldReadEmptyLib() throws IOException {
         // Given
-        Reader data = reader("lib-empty.json");
+        Reader data = reader(PATH_PREFIX + "lib-empty.json");
         // When
         Map<String, RepoEntry> entries = Repo.createEntries(data);
         // Then
@@ -67,7 +70,7 @@ void shouldReadEmptyLib() throws IOException {
     @Test
     void shouldReadLibWithNoVulnerabilities() throws IOException {
         // Given
-        Reader data = reader("vulns-empty.json");
+        Reader data = reader(PATH_PREFIX + "vulns-empty.json");
         // When
         Map<String, RepoEntry> entries = Repo.createEntries(data);
         // Then
@@ -79,7 +82,7 @@ void shouldReadLibWithNoVulnerabilities() throws IOException {
     @Test
     void shouldReadLibWithVulnerabilities() throws IOException {
         // Given
-        Reader data = reader("vulns-all.json");
+        Reader data = reader(PATH_PREFIX + "vulns-all.json");
         // When
         Map<String, RepoEntry> entries = Repo.createEntries(data);
         // Then
@@ -96,13 +99,13 @@ 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
     void shouldReadLibWithNoExtractors() throws IOException {
         // Given
-        Reader data = reader("extractors-empty.json");
+        Reader data = reader(PATH_PREFIX + "extractors-empty.json");
         // When
         Map<String, RepoEntry> entries = Repo.createEntries(data);
         // Then
@@ -121,7 +124,7 @@ void shouldReadLibWithNoExtractors() throws IOException {
     @Test
     void shouldReadLibWithAllExtractors() throws IOException {
         // Given
-        Reader data = reader("extractors-all.json");
+        Reader data = reader(PATH_PREFIX + "extractors-all.json");
         // When
         Map<String, RepoEntry> entries = Repo.createEntries(data);
         // Then
@@ -147,9 +150,9 @@ void shouldReadLibWithAllExtractors() throws IOException {
         assertThat(extractors.getUri(), contains("uri 1", "uri 2", "[0-9][0-9a-z._\\-]+?"));
     }
 
-    private static Reader reader(String fileName) throws IOException {
+    public static Reader reader(String fileName) throws IOException {
         String content;
-        try (var is = RepoUnitTest.class.getResourceAsStream("samples/" + fileName)) {
+        try (var is = RepoUnitTest.class.getResourceAsStream(fileName)) {
             content = IOUtils.toString(is, StandardCharsets.UTF_8);
         }
         return new StringReader(content);

diff --git a/.../retire/UpstreamJsRepositoryUnitTest.java → ...e/model/UpstreamJsRepositoryUnitTest.java b/.../retire/UpstreamJsRepositoryUnitTest.java → ...e/model/UpstreamJsRepositoryUnitTest.java
@@ -17,12 +17,15 @@
  * 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.hasItem;
 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 +34,14 @@ class UpstreamJsRepositoryUnitTest {
     void shouldParseUpstreamJsRepository() {
         // Given
         String path = "/org/zaproxy/addon/retire/resources/jsrepository.json";
+        Set<String> 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(
+                        expectedSeverities, hasItem(vuln.getSeverity().toLowerCase(Locale.ROOT)));
+            }
+        }
     }
 }
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
@@ -3,7 +3,7 @@
     "vulnerabilities" : [ {
       "atOrAbove" : "atorabove",
       "below" : "below",
-      "severity" : "severity",
+      "severity" : "high",
       "cwe" : [ "cwe 1", "cwe 2" ],
       "identifiers" : {
         "CVE" : [ "cve 1", "cve 2" ],