diff --git a/addOns/automation/CHANGELOG.md b/addOns/automation/CHANGELOG.md index 920c79b640d..aa553cc218d 100644 --- a/addOns/automation/CHANGELOG.md +++ b/addOns/automation/CHANGELOG.md @@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Fixed - Templates generated with `-autogenmin` or `-autogenmax` were invalid in some cases. - Allow to choose one thread for the `activeScan` job through the GUI. +- Active Scan jobs will once again use the default policy if neither a policy nor a policyDefinition has been set. ## [0.43.0] - 2024-10-07 ### Fixed diff --git a/addOns/automation/src/main/java/org/zaproxy/addon/automation/jobs/ActiveScanJob.java b/addOns/automation/src/main/java/org/zaproxy/addon/automation/jobs/ActiveScanJob.java index e1f6a18d580..6f62d0f6f5b 100644 --- a/addOns/automation/src/main/java/org/zaproxy/addon/automation/jobs/ActiveScanJob.java +++ b/addOns/automation/src/main/java/org/zaproxy/addon/automation/jobs/ActiveScanJob.java @@ -98,10 +98,6 @@ public void verifyParameters(AutomationProgress progress) { params, this.parameters, this.getName(), null, progress); break; case "policyDefinition": - // Parse the policy defn - PolicyDefinition.parsePolicyDefinition( - jobData.get(key), policyDefinition, this.getName(), progress); - break; case "name": case "tests": case "type": @@ -115,7 +111,8 @@ public void verifyParameters(AutomationProgress progress) { break; } } - + policyDefinition.parsePolicyDefinition( + jobData.get("policyDefinition"), this.getName(), progress); this.verifyUser(this.getParameters().getUser(), progress); } diff --git a/addOns/automation/src/main/java/org/zaproxy/addon/automation/jobs/ActiveScanPolicyJob.java b/addOns/automation/src/main/java/org/zaproxy/addon/automation/jobs/ActiveScanPolicyJob.java index ccbcdda9b66..01584e1ec14 100644 --- a/addOns/automation/src/main/java/org/zaproxy/addon/automation/jobs/ActiveScanPolicyJob.java +++ b/addOns/automation/src/main/java/org/zaproxy/addon/automation/jobs/ActiveScanPolicyJob.java @@ -86,8 +86,8 @@ public void verifyParameters(AutomationProgress progress) { break; case "policyDefinition": // Parse the policy defn - PolicyDefinition.parsePolicyDefinition( - jobData.get(key), policyDefinition, this.getName(), progress); + policyDefinition.parsePolicyDefinition( + jobData.get(key), this.getName(), progress); break; case "name": case "tests": diff --git a/addOns/automation/src/main/java/org/zaproxy/addon/automation/jobs/PolicyDefinition.java b/addOns/automation/src/main/java/org/zaproxy/addon/automation/jobs/PolicyDefinition.java index 5c30959c4e5..1911527656e 100644 --- a/addOns/automation/src/main/java/org/zaproxy/addon/automation/jobs/PolicyDefinition.java +++ b/addOns/automation/src/main/java/org/zaproxy/addon/automation/jobs/PolicyDefinition.java @@ -22,6 +22,7 @@ import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; import lombok.Getter; import lombok.Setter; import org.parosproxy.paros.Constant; @@ -31,36 +32,49 @@ import org.parosproxy.paros.core.scanner.PluginFactory; import org.zaproxy.addon.automation.AutomationData; import org.zaproxy.addon.automation.AutomationProgress; -import org.zaproxy.addon.automation.jobs.PolicyDefinition.Rule; import org.zaproxy.zap.extension.ascan.ScanPolicy; @Getter @Setter public class PolicyDefinition extends AutomationData { + private static final String DEFAULT_STRENGTH_KEY = "defaultStrength"; + private static final String DEFAULT_THRESHOLD_KEY = "defaultThreshold"; + protected static final String RULES_ELEMENT_NAME = "rules"; private String defaultStrength = JobUtils.strengthToI18n(AttackStrength.MEDIUM.name()); private String defaultThreshold = JobUtils.thresholdToI18n(AlertThreshold.MEDIUM.name()); private List rules = new ArrayList<>(); - public static void parsePolicyDefinition( - Object policyDefnObj, - PolicyDefinition policyDefinition, - String jobName, - AutomationProgress progress) { + public void parsePolicyDefinition( + Object policyDefnObj, String jobName, AutomationProgress progress) { + if (policyDefnObj == null) { + this.defaultStrength = null; + return; + } if (policyDefnObj instanceof LinkedHashMap) { - LinkedHashMap policyDefnData = (LinkedHashMap) policyDefnObj; + @SuppressWarnings("unchecked") + LinkedHashMap policyDefnData = + (LinkedHashMap) policyDefnObj; + + checkAndSetDefault(policyDefnData, DEFAULT_STRENGTH_KEY, AttackStrength.MEDIUM.name()); + checkAndSetDefault(policyDefnData, DEFAULT_THRESHOLD_KEY, AlertThreshold.MEDIUM.name()); + + if (policyDefnData.isEmpty() || undefinedDefinition(policyDefnData)) { + this.defaultStrength = null; + return; + } JobUtils.applyParamsToObject( policyDefnData, - policyDefinition, + this, jobName, new String[] {PolicyDefinition.RULES_ELEMENT_NAME}, progress); - List rules = new ArrayList<>(); + this.rules = new ArrayList<>(); ScanPolicy scanPolicy = new ScanPolicy(); PluginFactory pluginFactory = scanPolicy.getPluginFactory(); @@ -89,7 +103,7 @@ public static void parsePolicyDefinition( if (strength != null) { rule.setStrength(strength.name().toLowerCase()); } - rules.add(rule); + this.rules.add(rule); } else { progress.warn( @@ -98,7 +112,6 @@ public static void parsePolicyDefinition( } } } - policyDefinition.setRules(rules); } else if (o != null) { progress.warn( Constant.messages.getString( @@ -117,7 +130,32 @@ public static void parsePolicyDefinition( } } + private static void checkAndSetDefault( + LinkedHashMap policyDefnData, String key, String value) { + if (policyDefnData.containsKey(key) && policyDefnData.get(key) == null) { + policyDefnData.put(key, value); + } + } + + private static boolean undefinedDefinition(Map policyDefnData) { + Object rules = policyDefnData.get(RULES_ELEMENT_NAME); + boolean rulesInvalid = false; + if (rules instanceof List) { + rulesInvalid = ((List) rules).isEmpty(); + } else if ((String) rules == null) { + rulesInvalid = true; + } + return (String) policyDefnData.get(DEFAULT_STRENGTH_KEY) == null + && (String) policyDefnData.get(DEFAULT_THRESHOLD_KEY) == null + && rulesInvalid; + } + public ScanPolicy getScanPolicy(String jobName, AutomationProgress progress) { + if (getDefaultStrength() == null) { + // Nothing defined + return null; + } + ScanPolicy scanPolicy = new ScanPolicy(); // Set default strength diff --git a/addOns/automation/src/main/javahelp/org/zaproxy/addon/automation/resources/help/contents/job-ascan.html b/addOns/automation/src/main/javahelp/org/zaproxy/addon/automation/resources/help/contents/job-ascan.html index a62bf34a342..e06039e9863 100644 --- a/addOns/automation/src/main/javahelp/org/zaproxy/addon/automation/resources/help/contents/job-ascan.html +++ b/addOns/automation/src/main/javahelp/org/zaproxy/addon/automation/resources/help/contents/job-ascan.html @@ -44,6 +44,8 @@

YAML

threshold: # String: The Alert Threshold for this rule, one of Off, Low, Medium, High, default: Medium +Note: Unless the defaultThreshold of the policyDefinition is OFF all rules will be enabled to start with. +

The policy can be one defined by a previous activeScan-policy job, or by a scan policy file that has been put in policies directory under ZAP's HOME directory . diff --git a/addOns/automation/src/test/java/org/zaproxy/addon/automation/jobs/ActiveScanJobUnitTest.java b/addOns/automation/src/test/java/org/zaproxy/addon/automation/jobs/ActiveScanJobUnitTest.java index 598b5a334e5..8e501db6c1e 100644 --- a/addOns/automation/src/test/java/org/zaproxy/addon/automation/jobs/ActiveScanJobUnitTest.java +++ b/addOns/automation/src/test/java/org/zaproxy/addon/automation/jobs/ActiveScanJobUnitTest.java @@ -48,6 +48,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; import org.mockito.ArgumentMatcher; import org.mockito.MockedStatic; import org.mockito.Mockito; @@ -435,7 +437,7 @@ void shouldReturnWarningOnUnexpectedElement() throws MalformedURLException { } @Test - void shouldReturnScanPolicyForDefaultData() throws MalformedURLException { + void shouldReturnNullScanPolicyForEmptyData() { // Given ActiveScanJob job = new ActiveScanJob(); AutomationProgress progress = new AutomationProgress(); @@ -447,14 +449,66 @@ void shouldReturnScanPolicyForDefaultData() throws MalformedURLException { job.verifyParameters(progress); ScanPolicy policy = job.getData().getPolicyDefinition().getScanPolicy(null, progress); + // Then + assertThat(policy, is(equalTo(null))); + assertThat(progress.hasWarnings(), is(equalTo(false))); + assertThat(progress.hasErrors(), is(equalTo(false))); + } + + @ParameterizedTest + @EnumSource( + value = AttackStrength.class, + mode = EnumSource.Mode.EXCLUDE, + names = {"DEFAULT"}) + void shouldReturnScanPolicyIfOnlyDefaultStrength(AttackStrength attackStrength) { + // Given + ActiveScanJob job = new ActiveScanJob(); + AutomationProgress progress = new AutomationProgress(); + LinkedHashMap> data = new LinkedHashMap<>(); + LinkedHashMap policyDefn = new LinkedHashMap<>(); + policyDefn.put("defaultStrength", attackStrength.name()); + data.put("policyDefinition", policyDefn); + + // When + job.setJobData(data); + job.verifyParameters(progress); + ScanPolicy policy = job.getData().getPolicyDefinition().getScanPolicy(null, progress); + // Then assertThat(policy, is(notNullValue())); - assertThat(policy.getDefaultStrength(), is(AttackStrength.MEDIUM)); + assertThat(policy.getDefaultStrength(), is(attackStrength)); assertThat(policy.getDefaultThreshold(), is(AlertThreshold.MEDIUM)); assertThat(progress.hasWarnings(), is(equalTo(false))); assertThat(progress.hasErrors(), is(equalTo(false))); } + @ParameterizedTest + @EnumSource( + value = AlertThreshold.class, + mode = EnumSource.Mode.EXCLUDE, + names = {"DEFAULT"}) + void shouldReturnScanPolicyIfOnlyDefaultThreshold(AlertThreshold alertThreshold) { + // Given + ActiveScanJob job = new ActiveScanJob(); + AutomationProgress progress = new AutomationProgress(); + LinkedHashMap> data = new LinkedHashMap<>(); + LinkedHashMap policyDefn = new LinkedHashMap<>(); + policyDefn.put("defaultThreshold", alertThreshold.name()); + data.put("policyDefinition", policyDefn); + + // When + job.setJobData(data); + job.verifyParameters(progress); + ScanPolicy policy = job.getData().getPolicyDefinition().getScanPolicy(null, progress); + + // Then + assertThat(policy, is(notNullValue())); + assertThat(policy.getDefaultStrength(), is(AttackStrength.MEDIUM)); + assertThat(policy.getDefaultThreshold(), is(alertThreshold)); + assertThat(progress.hasWarnings(), is(equalTo(false))); + assertThat(progress.hasErrors(), is(equalTo(false))); + } + @Test void shouldSetScanPolicyDefaults() throws MalformedURLException { // Given diff --git a/addOns/automation/src/test/java/org/zaproxy/addon/automation/jobs/ActiveScanJobPolicyUnitTest.java b/addOns/automation/src/test/java/org/zaproxy/addon/automation/jobs/ActiveScanPolicyJobUnitTest.java similarity index 99% rename from addOns/automation/src/test/java/org/zaproxy/addon/automation/jobs/ActiveScanJobPolicyUnitTest.java rename to addOns/automation/src/test/java/org/zaproxy/addon/automation/jobs/ActiveScanPolicyJobUnitTest.java index 737316cf981..6ebe8747915 100644 --- a/addOns/automation/src/test/java/org/zaproxy/addon/automation/jobs/ActiveScanJobPolicyUnitTest.java +++ b/addOns/automation/src/test/java/org/zaproxy/addon/automation/jobs/ActiveScanPolicyJobUnitTest.java @@ -65,7 +65,7 @@ import org.zaproxy.zap.utils.I18N; import org.zaproxy.zap.utils.ZapXmlConfiguration; -class ActiveScanPolicyJobPolicyUnitTest { +class ActiveScanPolicyJobUnitTest { private static MockedStatic mockedCmdLine; private ExtensionActiveScan extAScan; diff --git a/addOns/automation/src/test/java/org/zaproxy/addon/automation/jobs/PolicyDefinitionUnitTest.java b/addOns/automation/src/test/java/org/zaproxy/addon/automation/jobs/PolicyDefinitionUnitTest.java index 7fbcb794422..fc939851689 100644 --- a/addOns/automation/src/test/java/org/zaproxy/addon/automation/jobs/PolicyDefinitionUnitTest.java +++ b/addOns/automation/src/test/java/org/zaproxy/addon/automation/jobs/PolicyDefinitionUnitTest.java @@ -22,6 +22,8 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; import java.io.IOException; import java.nio.file.Files; @@ -33,17 +35,23 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.MockedStatic; import org.mockito.Mockito; import org.parosproxy.paros.CommandLine; import org.parosproxy.paros.Constant; import org.parosproxy.paros.core.scanner.AbstractPlugin; +import org.parosproxy.paros.core.scanner.Plugin; +import org.parosproxy.paros.core.scanner.Plugin.AlertThreshold; +import org.parosproxy.paros.core.scanner.Plugin.AttackStrength; import org.parosproxy.paros.core.scanner.PluginFactory; import org.parosproxy.paros.core.scanner.PluginFactoryTestHelper; import org.parosproxy.paros.core.scanner.PluginTestHelper; import org.yaml.snakeyaml.Yaml; import org.zaproxy.addon.automation.AutomationProgress; import org.zaproxy.addon.automation.jobs.PolicyDefinition.Rule; +import org.zaproxy.zap.extension.ascan.ScanPolicy; import org.zaproxy.zap.utils.I18N; class PolicyDefinitionUnitTest { @@ -189,7 +197,7 @@ void shouldParseValidDefinition() { Object data = yaml.load(yamlStr); // When - PolicyDefinition.parsePolicyDefinition(data, policyDefinition, "test", progress); + policyDefinition.parsePolicyDefinition(data, "test", progress); // Then assertThat(progress.hasErrors(), is(equalTo(false))); @@ -223,7 +231,7 @@ void shouldWarnIfUnknownRule() { Object data = yaml.load(yamlStr); // When - PolicyDefinition.parsePolicyDefinition(data, policyDefinition, "test", progress); + policyDefinition.parsePolicyDefinition(data, "test", progress); // Then assertThat(progress.hasErrors(), is(equalTo(false))); @@ -250,7 +258,7 @@ void shouldWarnIfDefnNotList() { Object data = yaml.load(yamlStr); // When - PolicyDefinition.parsePolicyDefinition(data, policyDefinition, "test", progress); + policyDefinition.parsePolicyDefinition(data, "test", progress); // Then assertThat(progress.hasErrors(), is(equalTo(false))); @@ -270,7 +278,7 @@ void shouldWarnIfRulesNotList() { Object data = yaml.load(yamlStr); // When - PolicyDefinition.parsePolicyDefinition(data, policyDefinition, "test", progress); + policyDefinition.parsePolicyDefinition(data, "test", progress); // Then assertThat(progress.hasErrors(), is(equalTo(false))); @@ -279,4 +287,66 @@ void shouldWarnIfRulesNotList() { assertThat( progress.getWarnings().get(0), is(equalTo("!automation.error.options.badlist!"))); } + + @ParameterizedTest + @ValueSource( + strings = { + " defaultStrength: \n" + " defaultThreshold: \n" + " rules: ", + "defaultStrength:", + "defaultThreshold:" + }) + void shouldReturnPolicyWithDefaultsIfDefinitionYamlContainsUndefinedStrengthThreshold( + String defnYamlStr) { + // Given + AutomationProgress progress = new AutomationProgress(); + Yaml yaml = new Yaml(); + Object data = yaml.load(defnYamlStr); + + // When + policyDefinition.parsePolicyDefinition(data, "test", progress); + + // Then + assertThat(progress.hasErrors(), is(equalTo(false))); + assertThat(progress.hasWarnings(), is(equalTo(false))); + ScanPolicy policy = policyDefinition.getScanPolicy("test", progress); + assertThat(policy, is(notNullValue())); + assertThat(policy.getDefaultStrength(), is(equalTo(AttackStrength.MEDIUM))); + assertThat(policy.getDefaultThreshold(), is(equalTo(AlertThreshold.MEDIUM))); + List rules = policy.getPluginFactory().getAllPlugin(); + assertValueAppliedToRules( + rules.get(0), + rules.get(rules.size() - 1), + AttackStrength.MEDIUM, + AlertThreshold.MEDIUM); + } + + private static void assertValueAppliedToRules( + Plugin first, Plugin last, AttackStrength expectedStr, AlertThreshold expectedThold) { + assertThat(first.getAttackStrength(), is(equalTo(expectedStr))); + assertThat(last.getAttackStrength(), is(equalTo(expectedStr))); + assertThat(first.getAlertThreshold(), is(equalTo(expectedThold))); + assertThat(last.getAlertThreshold(), is(equalTo(expectedThold))); + } + + @ParameterizedTest + @ValueSource( + strings = { + "{}", + "", + "rules: \n", + }) + void shouldReturnNullPolicyIfDefinitionYamlIsEmptyOrNullObject(String defnYamlStr) { + // Given + AutomationProgress progress = new AutomationProgress(); + Yaml yaml = new Yaml(); + Object data = yaml.load(defnYamlStr); + + // When + policyDefinition.parsePolicyDefinition(data, "test", progress); + + // Then + assertThat(progress.hasErrors(), is(equalTo(false))); + assertThat(progress.hasWarnings(), is(equalTo(false))); + assertThat(policyDefinition.getScanPolicy("test", progress), is(nullValue())); + } } diff --git a/addOns/sequence/CHANGELOG.md b/addOns/sequence/CHANGELOG.md index 0546d150c1b..63a5f2719fd 100644 --- a/addOns/sequence/CHANGELOG.md +++ b/addOns/sequence/CHANGELOG.md @@ -9,8 +9,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - `sequence-import` to import HARs as sequences. - `sequence-activeScan` to active scan sequences. - Data for reporting. -- Sequence active scan policy. - Stats for import automation and active scan. +- Sequence active scan policy which will be used if neither a policy nor policyDefinition are set. ### Changed - Update minimum ZAP version to 2.15.0. diff --git a/addOns/sequence/src/main/java/org/zaproxy/zap/extension/sequence/automation/SequenceActiveScanJob.java b/addOns/sequence/src/main/java/org/zaproxy/zap/extension/sequence/automation/SequenceActiveScanJob.java index 6908195169d..894e75ce1ff 100644 --- a/addOns/sequence/src/main/java/org/zaproxy/zap/extension/sequence/automation/SequenceActiveScanJob.java +++ b/addOns/sequence/src/main/java/org/zaproxy/zap/extension/sequence/automation/SequenceActiveScanJob.java @@ -70,6 +70,8 @@ public class SequenceActiveScanJob extends AutomationJob { private static final String PARAM_SEQUENCE = "sequence"; private static final String PARAM_USER = "user"; + private static final String DEFAULT_SEQ_POLICY = "Sequence"; + private final ExtensionActiveScan extAScan; private final ExtensionScript extScript; @@ -115,10 +117,6 @@ public void verifyParameters(AutomationProgress progress) { params, this.parameters, this.getName(), null, progress); break; case "policyDefinition": - // Parse the policy defn - PolicyDefinition.parsePolicyDefinition( - jobData.get(key), policyDefinition, this.getName(), progress); - break; case "name": case "tests": case "type": @@ -132,6 +130,8 @@ public void verifyParameters(AutomationProgress progress) { break; } } + policyDefinition.parsePolicyDefinition( + jobData.get("policyDefinition"), this.getName(), progress); this.verifyUser(this.getParameters().getUser(), progress); } @@ -376,6 +376,6 @@ public static class Parameters extends AutomationData { private String sequence = ""; private String context = ""; private String user = ""; - private String policy = ""; + private String policy = DEFAULT_SEQ_POLICY; } } diff --git a/addOns/sequence/src/main/javahelp/org/zaproxy/zap/extension/sequence/resources/help/contents/automation.html b/addOns/sequence/src/main/javahelp/org/zaproxy/zap/extension/sequence/resources/help/contents/automation.html index 35998b4e249..5645d11d570 100644 --- a/addOns/sequence/src/main/javahelp/org/zaproxy/zap/extension/sequence/resources/help/contents/automation.html +++ b/addOns/sequence/src/main/javahelp/org/zaproxy/zap/extension/sequence/resources/help/contents/automation.html @@ -31,7 +31,7 @@

Job: sequence-activeScan

sequence: # String: The name of the sequence, or empty to active scan all sequences. context: # String: Context to use when active scanning, default: first context. user: # String: An optional user to use for authentication, must be defined in the env. - policy: # String: Name of the scan policy to be used, default: Default Policy. + policy: # String: Name of the scan policy to be used, default: Sequence. policyDefinition: # The policy definition - only used if the 'policy' is not set defaultStrength: # String: The default Attack Strength for all rules, one of Low, Medium, High, Insane (not recommended), default: Medium defaultThreshold: # String: The default Alert Threshold for all rules, one of Off, Low, Medium, High, default: Medium @@ -57,5 +57,7 @@

Job: sequence-activeScan

onFail: 'info' # String: One of 'warn', 'error', 'info', mandatory +Note: Unless the defaultThreshold of the policyDefinition is OFF all rules will be enabled to start with. + \ No newline at end of file diff --git a/addOns/sequence/src/main/resources/org/zaproxy/zap/extension/sequence/resources/sequence-activeScan-max.yaml b/addOns/sequence/src/main/resources/org/zaproxy/zap/extension/sequence/resources/sequence-activeScan-max.yaml index ca8bc1a7b6f..5500fdf6976 100644 --- a/addOns/sequence/src/main/resources/org/zaproxy/zap/extension/sequence/resources/sequence-activeScan-max.yaml +++ b/addOns/sequence/src/main/resources/org/zaproxy/zap/extension/sequence/resources/sequence-activeScan-max.yaml @@ -3,7 +3,7 @@ sequence: # String: The name of the sequence, or empty to active scan all sequences. context: # String: Context to use when active scanning, default: first context. user: # String: An optional user to use for authentication, must be defined in the env. - policy: # String: Name of the scan policy to be used, default: Default Policy. + policy: # String: Name of the scan policy to be used, default: Sequence. policyDefinition: # The policy definition - only used if the 'policy' is not set defaultStrength: # String: The default Attack Strength for all rules, one of Low, Medium, High, Insane (not recommended), default: Medium defaultThreshold: # String: The default Alert Threshold for all rules, one of Off, Low, Medium, High, default: Medium