Merge pull request #5534 from ricekot/scripts/handle-null-values

scripts: Handle null references in metadata
zaproxy · Jun 27, 2024 · 77fdd6d · 77fdd6d
2 parents f35be47 + 0be9a1c
commit 77fdd6d
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 10 deletions.
diff --git a/addOns/scripts/CHANGELOG.md b/addOns/scripts/CHANGELOG.md
@@ -7,6 +7,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ### Added
 - Provide the `script` API on newer ZAP versions.
 
+### Fixed
+- Handle missing "references" field in the script metadata correctly.
+
 ## [45.4.0] - 2024-05-16
 ### Added
 - Support for Automation Framework loaddir action, which loads all of the scripts under the specified directory.

diff --git a/...ripts/src/main/java/org/zaproxy/zap/extension/scripts/scanrules/ActiveScriptScanRule.java b/...ripts/src/main/java/org/zaproxy/zap/extension/scripts/scanrules/ActiveScriptScanRule.java
@@ -193,7 +193,10 @@ public String getSolution() {
 
     @Override
     public String getReference() {
-        return String.join("\n", metadata.getReferences());
+        if (metadata.getReferences() != null && !metadata.getReferences().isEmpty()) {
+            return String.join("\n", metadata.getReferences());
+        }
+        return "";
     }
 
     @Override

diff --git a/...ipts/src/main/java/org/zaproxy/zap/extension/scripts/scanrules/PassiveScriptScanRule.java b/...ipts/src/main/java/org/zaproxy/zap/extension/scripts/scanrules/PassiveScriptScanRule.java
@@ -79,15 +79,19 @@ public PluginPassiveScanner copy() {
 
     @Override
     public AlertBuilder newAlert() {
-        return super.newAlert()
-                .setRisk(metadata.getRisk().getValue())
-                .setConfidence(metadata.getConfidence().getValue())
-                .setDescription(metadata.getDescription())
-                .setSolution(metadata.getSolution())
-                .setCweId(metadata.getCweId())
-                .setWascId(metadata.getWascId())
-                .setReference(String.join("\n", metadata.getReferences()))
-                .setOtherInfo(metadata.getOtherInfo());
+        var alertBuilder =
+                super.newAlert()
+                        .setRisk(metadata.getRisk().getValue())
+                        .setConfidence(metadata.getConfidence().getValue())
+                        .setDescription(metadata.getDescription())
+                        .setSolution(metadata.getSolution())
+                        .setCweId(metadata.getCweId())
+                        .setWascId(metadata.getWascId())
+                        .setOtherInfo(metadata.getOtherInfo());
+        if (metadata.getReferences() != null && !metadata.getReferences().isEmpty()) {
+            alertBuilder.setReference(String.join("\n", metadata.getReferences()));
+        }
+        return alertBuilder;
     }
 
     @Override

diff --git a/...c/test/java/org/zaproxy/zap/extension/scripts/scanrules/ActiveScriptScanRuleUnitTest.java b/...c/test/java/org/zaproxy/zap/extension/scripts/scanrules/ActiveScriptScanRuleUnitTest.java
@@ -19,6 +19,9 @@
  */
 package org.zaproxy.zap.extension.scripts.scanrules;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.Mockito.mock;
@@ -30,6 +33,7 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.parosproxy.paros.control.Control;
+import org.parosproxy.paros.core.scanner.Alert;
 import org.parosproxy.paros.core.scanner.HostProcess;
 import org.parosproxy.paros.core.scanner.NameValuePair;
 import org.parosproxy.paros.core.scanner.ScannerParam;
@@ -38,6 +42,8 @@
 import org.parosproxy.paros.model.Model;
 import org.parosproxy.paros.network.HttpMessage;
 import org.parosproxy.paros.network.HttpRequestHeader;
+import org.zaproxy.addon.commonlib.scanrules.Confidence;
+import org.zaproxy.addon.commonlib.scanrules.Risk;
 import org.zaproxy.addon.commonlib.scanrules.ScanRuleMetadata;
 import org.zaproxy.addon.commonlib.scanrules.ScanRuleMetadataProvider;
 import org.zaproxy.zap.extension.ascan.VariantFactory;
@@ -145,6 +151,21 @@ public ScanRuleMetadata getMetadata() {
         verify(scriptActiveInterface, times(1)).scanNode(scanRule, message);
     }
 
+    @Test
+    void shouldHandleNullReferences() throws Exception {
+        // Given
+        ScriptWrapper script = mock(ScriptWrapper.class);
+        var metadata = new ScanRuleMetadata(12345, "Test Scan Rule");
+        metadata.setRisk(Risk.HIGH);
+        metadata.setConfidence(Confidence.HIGH);
+        metadata.setReferences(null);
+        var scanRule = new ActiveScriptScanRule(script, metadata);
+        // When
+        Alert alert = scanRule.newAlert().build();
+        // Then
+        assertThat(alert.getReference(), is(equalTo("")));
+    }
+
     private <T> ScriptWrapper createScriptWrapper(T scriptInterface, Class<T> scriptClass)
             throws Exception {
         var script = mock(ScriptWrapper.class);

diff --git a/.../test/java/org/zaproxy/zap/extension/scripts/scanrules/PassiveScriptScanRuleUnitTest.java b/.../test/java/org/zaproxy/zap/extension/scripts/scanrules/PassiveScriptScanRuleUnitTest.java
@@ -19,6 +19,9 @@
  */
 package org.zaproxy.zap.extension.scripts.scanrules;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
@@ -28,11 +31,15 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.parosproxy.paros.control.Control;
+import org.parosproxy.paros.core.scanner.Alert;
 import org.parosproxy.paros.extension.ExtensionLoader;
 import org.parosproxy.paros.model.Model;
 import org.parosproxy.paros.network.HttpMessage;
 import org.parosproxy.paros.network.HttpRequestHeader;
+import org.zaproxy.addon.commonlib.scanrules.Confidence;
+import org.zaproxy.addon.commonlib.scanrules.Risk;
 import org.zaproxy.addon.commonlib.scanrules.ScanRuleMetadata;
+import org.zaproxy.zap.extension.pscan.PassiveScanData;
 import org.zaproxy.zap.extension.script.ExtensionScript;
 import org.zaproxy.zap.extension.script.ScriptWrapper;
 import org.zaproxy.zap.testutils.TestUtils;
@@ -84,6 +91,22 @@ void shouldScanWithCopy() throws Exception {
         verify(scriptInterface, times(1)).scan(scanRule, message, source);
     }
 
+    @Test
+    void shouldHandleNullReferences() throws Exception {
+        // Given
+        ScriptWrapper script = mock(ScriptWrapper.class);
+        var metadata = new ScanRuleMetadata(12345, "Test Scan Rule");
+        metadata.setRisk(Risk.HIGH);
+        metadata.setConfidence(Confidence.HIGH);
+        metadata.setReferences(null);
+        var scanRule = new PassiveScriptScanRule(script, metadata);
+        scanRule.setHelper(mock(PassiveScanData.class));
+        // When
+        Alert alert = scanRule.newAlert().build();
+        // Then
+        assertThat(alert.getReference(), is(equalTo("")));
+    }
+
     private <T> ScriptWrapper createScriptWrapper(T scriptInterface, Class<T> scriptClass)
             throws Exception {
         var script = mock(ScriptWrapper.class);