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>

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).

addOns/retire/src/main/java/org/zaproxy/addon/retire/model/Repo.java

@@ -70,7 +70,8 @@ public Repo(Path file) throws IOException {
         }
     }
 
-    static Map<String, RepoEntry> createEntries(Reader reader) throws IOException {
+    // ** Increased visibility for testing purposes only */
+    public static Map<String, RepoEntry> createEntries(Reader reader) throws IOException {
         try {
             return new ObjectMapper()
                     .readValue(reader, new TypeReference<Map<String, RepoEntry>>() {});
@@ -288,7 +289,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 +308,11 @@ private static boolean isGoodCandidate(String version) {
         return isAllZeros != 0; // Not a good value if all zero
     }
 
+    /** For Testing purposes only */
+    public Map<String, RepoEntry> getEntries() {
+        return entries;
+    }
+
     public static class VulnerabilityData {
         public static final VulnerabilityData EMPTY = new VulnerabilityData();
 

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 */
+    public static final Map<String, Integer> SEVERITY_MAP =
             Map.of("high", Alert.RISK_HIGH, "medium", Alert.RISK_MEDIUM, "low", Alert.RISK_LOW);
 
     private String below;

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

addOns/retire/src/test/java/org/zaproxy/addon/retire/RetireTestUtils.java

@@ -0,0 +1,40 @@
+/*
+ * Zed Attack Proxy (ZAP) and its related class files.
+ *
+ * ZAP is an HTTP/HTTPS proxy for assessing web application security.
+ *
+ * Copyright 2024 The ZAP Development Team
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.zaproxy.addon.retire;
+
+import java.io.IOException;
+import java.io.Reader;
+import java.io.StringReader;
+import java.nio.charset.StandardCharsets;
+import org.apache.commons.io.IOUtils;
+
+public final class RetireTestUtils {
+
+    private RetireTestUtils() {
+    }
+
+    public static Reader reader(String fileName) throws IOException {
+        String content;
+        try (var is = RetireTestUtils.class.getResourceAsStream(fileName)) {
+            content = IOUtils.toString(is, StandardCharsets.UTF_8);
+        }
+        return new StringReader(content);
+    }
+}

addOns/retire/src/test/java/org/zaproxy/addon/retire/UpstreamJsRepositoryUnitTest.java

@@ -19,10 +19,17 @@
  */
 package org.zaproxy.addon.retire;
 
-import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
 
+import java.io.IOException;
+import java.io.Reader;
+import java.util.Locale;
+import java.util.Map;
 import org.junit.jupiter.api.Test;
 import org.zaproxy.addon.retire.model.Repo;
+import org.zaproxy.addon.retire.model.RepoEntry;
+import org.zaproxy.addon.retire.model.Vulnerability;
 
 /** Tests for upstream {@code jsrepository.json} file. */
 class UpstreamJsRepositoryUnitTest {
@@ -31,7 +38,29 @@ class UpstreamJsRepositoryUnitTest {
     void shouldParseUpstreamJsRepository() {
         // Given
         String path = "/org/zaproxy/addon/retire/resources/jsrepository.json";
-        // When / Then
-        assertDoesNotThrow(() -> new Repo(path));
+        Reader data;
+        Map<String, RepoEntry> entries = null;
+        // When
+        try {
+            data = RetireTestUtils.reader(path);
+            entries = Repo.createEntries(data);
+        } catch (IOException e) {
+            fail();
+        }
+        // Then
+        entries.values()
+                .forEach(
+                        entry ->
+                                entry.getVulnerabilities()
+                                        .forEach(
+                                                vuln ->
+                                                        assertTrue(
+                                                                Vulnerability.SEVERITY_MAP
+                                                                        .keySet()
+                                                                        .contains(
+                                                                                vuln.getSeverity()
+                                                                                        .toLowerCase(
+                                                                                                Locale
+                                                                                                        .ROOT)))));
     }
 }

addOns/retire/src/test/java/org/zaproxy/addon/retire/model/RepoUnitTest.java

@@ -31,19 +31,20 @@
 
 import java.io.IOException;
 import java.io.Reader;
-import java.io.StringReader;
-import java.nio.charset.StandardCharsets;
 import java.util.List;
 import java.util.Map;
-import org.apache.commons.io.IOUtils;
 import org.junit.jupiter.api.Test;
+import org.zaproxy.addon.retire.RetireTestUtils;
 
 /** 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 = RetireTestUtils.reader(PATH_PREFIX + "repo-empty.json");
         // When
         Map<String, RepoEntry> entries = Repo.createEntries(data);
         // Then
@@ -53,7 +54,7 @@ void shouldReadEmptyRepo() throws IOException {
     @Test
     void shouldReadEmptyLib() throws IOException {
         // Given
-        Reader data = reader("lib-empty.json");
+        Reader data = RetireTestUtils.reader(PATH_PREFIX + "lib-empty.json");
         // When
         Map<String, RepoEntry> entries = Repo.createEntries(data);
         // Then
@@ -67,7 +68,7 @@ void shouldReadEmptyLib() throws IOException {
     @Test
     void shouldReadLibWithNoVulnerabilities() throws IOException {
         // Given
-        Reader data = reader("vulns-empty.json");
+        Reader data = RetireTestUtils.reader(PATH_PREFIX + "vulns-empty.json");
         // When
         Map<String, RepoEntry> entries = Repo.createEntries(data);
         // Then
@@ -79,7 +80,7 @@ void shouldReadLibWithNoVulnerabilities() throws IOException {
     @Test
     void shouldReadLibWithVulnerabilities() throws IOException {
         // Given
-        Reader data = reader("vulns-all.json");
+        Reader data = RetireTestUtils.reader(PATH_PREFIX + "vulns-all.json");
         // When
         Map<String, RepoEntry> entries = Repo.createEntries(data);
         // Then
@@ -96,13 +97,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 = RetireTestUtils.reader(PATH_PREFIX + "extractors-empty.json");
         // When
         Map<String, RepoEntry> entries = Repo.createEntries(data);
         // Then
@@ -121,7 +122,7 @@ void shouldReadLibWithNoExtractors() throws IOException {
     @Test
     void shouldReadLibWithAllExtractors() throws IOException {
         // Given
-        Reader data = reader("extractors-all.json");
+        Reader data = RetireTestUtils.reader(PATH_PREFIX + "extractors-all.json");
         // When
         Map<String, RepoEntry> entries = Repo.createEntries(data);
         // Then
@@ -146,12 +147,4 @@ void shouldReadLibWithAllExtractors() throws IOException {
                         hasEntry("§§version§§", "§§version§§")));
         assertThat(extractors.getUri(), contains("uri 1", "uri 2", "[0-9][0-9a-z._\\-]+?"));
     }
-
-    private static Reader reader(String fileName) throws IOException {
-        String content;
-        try (var is = RepoUnitTest.class.getResourceAsStream("samples/" + fileName)) {
-            content = IOUtils.toString(is, StandardCharsets.UTF_8);
-        }
-        return new StringReader(content);
-    }
 }

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" ],