diff --git a/zap/src/main/java/org/parosproxy/paros/core/scanner/Alert.java b/zap/src/main/java/org/parosproxy/paros/core/scanner/Alert.java index 86a85f46937..374c5b46d07 100644 --- a/zap/src/main/java/org/parosproxy/paros/core/scanner/Alert.java +++ b/zap/src/main/java/org/parosproxy/paros/core/scanner/Alert.java @@ -1004,6 +1004,8 @@ public Map getTags() { public void setTags(Map tags) { if (tags != null) { this.tags = tags; + // The tags changed, the value cached might no longer be correct + this.systemic = null; } } diff --git a/zap/src/main/java/org/zaproxy/zap/extension/alert/AlertAPI.java b/zap/src/main/java/org/zaproxy/zap/extension/alert/AlertAPI.java index ba0554be1b4..5aff5fae1e8 100644 --- a/zap/src/main/java/org/zaproxy/zap/extension/alert/AlertAPI.java +++ b/zap/src/main/java/org/zaproxy/zap/extension/alert/AlertAPI.java @@ -211,23 +211,9 @@ public String getPrefix() { public ApiResponse handleApiView(String name, JSONObject params) throws ApiException { ApiResponse result = null; if (VIEW_ALERT.equals(name)) { - TableAlert tableAlert = Model.getSingleton().getDb().getTableAlert(); - TableAlertTag tableAlertTag = Model.getSingleton().getDb().getTableAlertTag(); - RecordAlert recordAlert; - Map alertTags; - try { - recordAlert = tableAlert.read(this.getParam(params, PARAM_ID, -1)); - alertTags = tableAlertTag.getTagsByAlertId(this.getParam(params, PARAM_ID, -1)); - } catch (DatabaseException e) { - LOGGER.error("Failed to read the alert from the session:", e); - throw new ApiException(ApiException.Type.INTERNAL_ERROR); - } - if (recordAlert == null) { - throw new ApiException(ApiException.Type.DOES_NOT_EXIST); - } - Alert alert = new Alert(recordAlert); - alert.setTags(alertTags); - result = new ApiResponseElement(alertToSet(alert)); + result = + new ApiResponseElement( + alertToSet(getAlertFromDb(this.getParam(params, PARAM_ID, -1)))); } else if (VIEW_ALERTS.equals(name)) { final ApiResponseList resultList = new ApiResponseList(name); String contextName = this.getParam(params, PARAM_CONTEXT_NAME, ""); @@ -663,19 +649,17 @@ private static List getAlertIds(String alertIds) throws ApiException { return idsList; } - private static Alert getAlertFromDb(int alertId) throws ApiException { - RecordAlert recAlert; + private Alert getAlertFromDb(int alertId) throws ApiException { try { - recAlert = Model.getSingleton().getDb().getTableAlert().read(alertId); + Alert alert = extension.getAlert(alertId); + if (alert == null) { + throw new ApiException(ApiException.Type.DOES_NOT_EXIST, String.valueOf(alertId)); + } + return alert; } catch (DatabaseException e) { LOGGER.error(e.getMessage(), e); throw new ApiException(ApiException.Type.INTERNAL_ERROR, e); } - - if (recAlert == null) { - throw new ApiException(ApiException.Type.DOES_NOT_EXIST, String.valueOf(alertId)); - } - return new Alert(recAlert); } private void processAlertUpdate(Alert updatedAlert) throws ApiException { diff --git a/zap/src/main/java/org/zaproxy/zap/extension/alert/AlertTreeModel.java b/zap/src/main/java/org/zaproxy/zap/extension/alert/AlertTreeModel.java index b356eb34735..71561a205d8 100644 --- a/zap/src/main/java/org/zaproxy/zap/extension/alert/AlertTreeModel.java +++ b/zap/src/main/java/org/zaproxy/zap/extension/alert/AlertTreeModel.java @@ -42,14 +42,28 @@ class AlertTreeModel extends DefaultTreeModel { private static final Logger LOGGER = LogManager.getLogger(AlertTreeModel.class); private ExtensionAlert ext; + private boolean mainTreeModel; AlertTreeModel(ExtensionAlert ext) { + this(ext, true); + } + + /** + * Creates a tree model. + * + * @param ext the extension. + * @param mainTreeModel whether the model is the main alerts tree, the systemic limit is only + * applied to it, the other models (e.g. the filtered alerts tree) are a subset of it and + * thus do not need to apply the limit. + */ + AlertTreeModel(ExtensionAlert ext, boolean mainTreeModel) { super( new AlertNode( -1, Constant.messages.getString("alerts.tree.title"), GROUP_ALERT_CHILD_COMPARATOR)); this.ext = ext; + this.mainTreeModel = mainTreeModel; } void addPath(final Alert alert) { @@ -90,20 +104,45 @@ protected synchronized AlertNode addPathEventHandler(Alert alert) { + (StringUtils.isNotEmpty(alert.getNodeName()) ? alert.getNodeName() : alert.getUri()); - return addLeaf(parent, name, alert); + AlertNode node = addLeaf(parent, name, alert); + if (node == null && parent.getChildCount() == 0) { + // The alert was not added (e.g. it's over the systemic limit) so remove the group + // node added for it, otherwise an empty node is left behind in the tree. + this.removeNodeFromParent(parent); + nodeStructureChanged(getRoot()); + } + return node; } + /** + * Finds the node for the given alert, preferring the node of the alert itself, falling back to + * an equivalent alert (i.e. an alert de-duplicated with the given one). + */ private AlertNode findLeafNodeForAlert(AlertNode parent, Alert alert) { + AlertNode node = findLeafNodeForAlert(parent, alert, true); + if (node == null) { + node = findLeafNodeForAlert(parent, alert, false); + } + return node; + } + + /** + * Finds the node for the given alert, matching the alert itself if {@code exactMatch}, + * otherwise any equivalent alert. Note that the returned node can be a group node with no + * alerts (i.e. it has no children but its parent is the root). + */ + private AlertNode findLeafNodeForAlert(AlertNode parent, Alert alert, boolean exactMatch) { for (int i = 0; i < parent.getChildCount(); i++) { AlertNode child = parent.getChildAt(i); if (child.getChildCount() == 0) { - // Its a leaf node - if (child.getAlert() != null && child.getAlert().compareTo(alert) == 0) { + // Its a leaf node, or a group node with no alerts + Alert childAlert = child.getAlert(); + if (childAlert != null && matches(childAlert, alert, exactMatch)) { return child; } } else { // check its children - AlertNode node = findLeafNodeForAlert(child, alert); + AlertNode node = findLeafNodeForAlert(child, alert, exactMatch); if (node != null) { return node; } @@ -112,6 +151,13 @@ private AlertNode findLeafNodeForAlert(AlertNode parent, Alert alert) { return null; } + private static boolean matches(Alert alert, Alert otherAlert, boolean exactMatch) { + if (exactMatch) { + return alert.getAlertId() == otherAlert.getAlertId(); + } + return alert.compareTo(otherAlert) == 0; + } + public AlertNode getAlertNode(Alert alert) { AlertNode parent = getRoot(); int risk = alert.getRisk(); @@ -157,13 +203,24 @@ public void run() { private synchronized void updatePathEventHandler(Alert alert) { AlertNode node = findLeafNodeForAlert(getRoot(), alert); - if (node != null) { + if (node == null) { + // The alert is not shown in the tree, e.g. it was not added when raised (because it was + // de-duplicated or over the systemic limit), add it now that it changed. + this.addPath(alert); + return; + } - // Remove the old version - AlertNode parent = node.getParent(); + // Remove the old version + AlertNode parent = node.getParent(); + if (parent.isRoot()) { + // The node is a group node with no alerts, it represents the alert so remove it, + // it will be added back as needed below. + this.removeNodeFromParent(node); + nodeStructureChanged(this.getRoot()); + } else { // Cannot use removeNodeFromParent as the risk or name might have changed - removeChildById(parent, alert.getAlertId()); + removeChildNode(parent, node); nodeStructureChanged(parent); if (parent.getChildCount() == 0) { @@ -172,14 +229,21 @@ private synchronized void updatePathEventHandler(Alert alert) { nodeStructureChanged(this.getRoot()); } } + // Add it back in again this.addPath(alert); } - private void removeChildById(AlertNode parent, int alertId) { + /** + * Removes the given child node from the given parent node. + * + *

The node is removed by identity, not by alert ID, as the node found for an alert can be an + * equivalent (de-duplicated) alert. + */ + private static void removeChildNode(AlertNode parent, AlertNode node) { int idx = -1; for (int i = 0; i < parent.getChildCount(); i++) { - if (parent.getChildAt(i).getAlert().getAlertId() == alertId) { + if (parent.getChildAt(i) == node) { idx = i; break; } @@ -190,13 +254,9 @@ private void removeChildById(AlertNode parent, int alertId) { } private AlertNode findAndAddGroup(AlertNode parent, String nodeName, Alert alert) { - int risk = alert.getRisk(); - if (alert.getConfidence() == Alert.CONFIDENCE_FALSE_POSITIVE) { - // Special case! - risk = -1; - } - - AlertNode node = new AlertNode(risk, nodeName, alert.getAlertRef(), ALERT_CHILD_COMPARATOR); + AlertNode node = + new AlertNode( + getRisk(alert), nodeName, alert.getAlertRef(), ALERT_CHILD_COMPARATOR); int idx = parent.findIndex(node); if (idx < 0) { idx = -(idx + 1); @@ -209,12 +269,37 @@ private AlertNode findAndAddGroup(AlertNode parent, String nodeName, Alert alert return parent.getChildAt(idx); } - private AlertNode addLeaf(AlertNode parent, String nodeName, Alert alert) { - int risk = alert.getRisk(); + /** + * Returns the node of the group of alerts the given alert is, or would be, added to, or {@code + * null} if the tree has no such group. + * + * @param alert the alert. + * @return the node of the group of alerts, or {@code null} if not shown in the tree. + */ + AlertNode getGroupNode(Alert alert) { + AlertNode node = + new AlertNode( + getRisk(alert), + alert.getName(), + alert.getAlertRef(), + ALERT_CHILD_COMPARATOR); + int idx = getRoot().findIndex(node); + if (idx < 0) { + return null; + } + return getRoot().getChildAt(idx); + } + + private static int getRisk(Alert alert) { if (alert.getConfidence() == Alert.CONFIDENCE_FALSE_POSITIVE) { // Special case! - risk = -1; + return -1; } + return alert.getRisk(); + } + + private AlertNode addLeaf(AlertNode parent, String nodeName, Alert alert) { + int risk = getRisk(alert); AlertNode needle = new AlertNode(risk, nodeName, alert.getAlertRef(), ALERT_CHILD_COMPARATOR); @@ -222,7 +307,9 @@ private AlertNode addLeaf(AlertNode parent, String nodeName, Alert alert) { int idx = parent.findIndex(needle); if (idx < 0) { // Not a duplicate alert - if (ext.isOverSystemicLimit(alert)) { + // The limit is applied to the main tree model only, the other models (e.g. the filtered + // alerts tree) are a subset of it and thus are not subject to it. + if (mainTreeModel && ext.isOverSystemicLimit(alert, parent)) { if (!parent.isSystemic()) { parent.setSystemic(true); nodeChanged(parent); @@ -239,10 +326,15 @@ private AlertNode addLeaf(AlertNode parent, String nodeName, Alert alert) { } public synchronized void deletePath(Alert alert) { - AlertNode node = findLeafNodeForAlert(getRoot(), alert); if (node != null) { AlertNode parent = node.getParent(); + if (parent.isRoot()) { + // The node is a group node with no alerts, just remove it + this.removeNodeFromParent(node); + this.nodeStructureChanged(parent); + return; + } if (parent.getChildCount() == 1) { // Parent has no other children, remove it also parent.remove(0); diff --git a/zap/src/main/java/org/zaproxy/zap/extension/alert/ExtensionAlert.java b/zap/src/main/java/org/zaproxy/zap/extension/alert/ExtensionAlert.java index 6e9ea3fa282..b3ba5289321 100644 --- a/zap/src/main/java/org/zaproxy/zap/extension/alert/ExtensionAlert.java +++ b/zap/src/main/java/org/zaproxy/zap/extension/alert/ExtensionAlert.java @@ -33,10 +33,9 @@ import java.util.SortedSet; import java.util.TreeSet; import java.util.Vector; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.atomic.AtomicInteger; import javax.swing.JTree; import javax.swing.tree.TreePath; +import org.apache.commons.httpclient.URI; import org.apache.commons.httpclient.URIException; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; @@ -97,9 +96,6 @@ public class ExtensionAlert extends ExtensionAdaptor private Properties alertOverrides = new Properties(); private AlertAddDialog dialogAlertAdd; - private Map> siteToSystemicAlertMap = - new ConcurrentHashMap<>(); - public ExtensionAlert() { super(NAME); this.setOrder(27); @@ -247,8 +243,10 @@ public void alertFound(Alert alert, HistoryReference ref) { private void alertFoundEventHandler(Alert alert, HistoryReference ref) { try { + boolean added = false; synchronized (this.getTreeModel()) { if (this.getTreeModel().addPathEventHandler(alert) != null) { + added = true; if (isInFilter(alert)) { this.getFilteredTreeModel().addPath(alert); } @@ -256,21 +254,25 @@ private void alertFoundEventHandler(Alert alert, HistoryReference ref) { getAlertPanel().expandRoot(); this.recalcAlerts(); } - } else { - return; } } - try { - SessionStructure.addPath(Model.getSingleton(), ref, alert.getMessage()); - ref.addAlert(alert); - } catch (Exception e) { - LOGGER.error(e.getMessage(), e); + if (added) { + try { + SessionStructure.addPath(Model.getSingleton(), ref, alert.getMessage()); + ref.addAlert(alert); + } catch (Exception e) { + LOGGER.error(e.getMessage(), e); + } } // Clear the message so that it can be GC'ed alert.setMessage(null); + // Publish the event even when the alert was not added to the tree (it was + // de-duplicated or is over the systemic limit) so that consumers, e.g. alert filters, + // can act on it, otherwise the alert could be left in the database with its original + // risk/confidence while everything shown to the user has the new values. publishAlertEvent(alert, AlertEventPublisher.ALERT_ADDED_EVENT); } catch (Exception e) { @@ -519,7 +521,8 @@ AlertTreeModel getTreeModel() { private AlertTreeModel getFilteredTreeModel() { if (filteredTreeModel == null) { - filteredTreeModel = new AlertTreeModel(this); + // The filtered tree is a subset of the main tree, it does not apply the systemic limit + filteredTreeModel = new AlertTreeModel(this, false); } return filteredTreeModel; } @@ -689,7 +692,6 @@ private void sessionChangedEventHandler(Session session) { treeModel = null; filteredTreeModel = null; hrefs = new HashMap<>(); - siteToSystemicAlertMap = new ConcurrentHashMap<>(); if (session == null) { // Null session indicated we're shutting down @@ -975,11 +977,32 @@ void recalcAlerts() { footer.setAlertHigh(totalHigh); } + /** + * Returns the alert with the given ID, as stored in the database, with the tags loaded, or + * {@code null} if there is no alert with the given ID. + * + * @param alertId the ID of the alert. + * @return the alert, or {@code null} if there is no alert with the given ID. + * @throws DatabaseException if an error occurred while reading the alert. + * @since 2.18.0 + */ + public Alert getAlert(int alertId) throws DatabaseException { + RecordAlert recAlert = getModel().getDb().getTableAlert().read(alertId); + if (recAlert == null) { + return null; + } + Alert alert = new Alert(recAlert); + Map tags = getModel().getDb().getTableAlertTag().getTagsByAlertId(alertId); + if (tags != null) { + alert.setTags(tags); + } + return alert; + } + public List getAllAlerts() { List allAlerts = new ArrayList<>(); TableAlert tableAlert = getModel().getDb().getTableAlert(); - TableAlertTag tableAlertTag = getModel().getDb().getTableAlertTag(); Vector v; try { // TODO this doesn't work, but should be used when its fixed :/ @@ -988,13 +1011,13 @@ public List getAllAlerts() { v = tableAlert.getAlertList(); for (int i = 0; i < v.size(); i++) { - int alertId = v.get(i); - RecordAlert recAlert = tableAlert.read(alertId); - Alert alert = new Alert(recAlert); + Alert alert = getAlert(v.get(i)); + if (alert == null) { + continue; + } if (alert.getHistoryRef() != null) { // Only use the alert if it has a history reference. if (!allAlerts.contains(alert)) { - alert.setTags(tableAlertTag.getTagsByAlertId(alertId)); allAlerts.add(alert); } } @@ -1257,29 +1280,111 @@ public boolean isNewAlert(Alert alertToCheck) { } /** - * Returns true if the given alert is over the systemic limit. If the alert is systemic then it - * will increment the count of that type of alert. + * Returns true if the given alert would be over the systemic limit if added to the given group + * of alerts, that is, if the group already has the maximum number of alerts of the same host. + * + *

If the alert is systemic then it counts towards the limit for its risk/confidence, alerts + * of the same host sharing the same budget. + * + *

The alerts already in the group are counted, thus the given alert does not count more than + * once no matter how many times this is called for it, e.g. because it is reprocessed by an + * alert filter. + * + * @param alert the alert, might be {@code null}. + * @param parent the node of the group of alerts the alert is, or would be, added to. + * @return {@code true} if the alert is over the systemic limit, {@code false} otherwise. + */ + protected boolean isOverSystemicLimit(Alert alert, AlertNode parent) { + if (alert == null || parent == null || !alert.isSystemic()) { + return false; + } + int limit = getAlertParam().getSystemicLimit(); + if (limit <= 0) { + return false; + } + String host = getAlertHost(alert); + int count = 0; + for (int i = 0; i < parent.getChildCount(); i++) { + AlertNode node = parent.getChildAt(i); + // Group nodes have no alerts of their own + if (node.getChildCount() > 0) { + continue; + } + Alert counted = node.getAlert(); + // Alerts that are not systemic do not count towards the limit + if (counted == null || !counted.isSystemic()) { + continue; + } + if (host.equals(getAlertHost(counted))) { + count++; + if (count >= limit) { + return true; + } + } + } + return false; + } + + /** + * Returns true if the given alert would be over the systemic limit if added to the alerts tree. * + * @param alert the alert, might be {@code null}. + * @return {@code true} if the alert is over the systemic limit, {@code false} otherwise. * @since 2.17.0 */ public boolean isOverSystemicLimit(Alert alert) { - if (alert == null || !alert.isSystemic()) { + if (alert == null) { return false; } + return isOverSystemicLimit(alert, getTreeModel().getGroupNode(alert)); + } + + /** + * Returns the host of the given alert, used to count the alerts towards the systemic limit, + * which is the host of the message that raised it, falling back to the host of the alert URI. + * + * @param alert the alert, might be {@code null}. + * @return the host, or an empty string if not known. + */ + private static String getAlertHost(Alert alert) { + if (alert == null) { + return ""; + } + URI uri = getSystemicLimitUri(alert); + if (uri == null) { + return ""; + } try { - // Always count locally, even if the systemicLimit is zero as that could be changed - Map m = - siteToSystemicAlertMap.computeIfAbsent( - SessionStructure.getHostName(alert.getMsgUri()), - a -> new ConcurrentHashMap<>()); - int count = - m.computeIfAbsent(alert.getAlertRef(), a -> new AtomicInteger()) - .incrementAndGet(); - int limit = getAlertParam().getSystemicLimit(); - return limit > 0 && count > limit; + return SessionStructure.getHostName(uri); } catch (URIException e) { - // Ignore + return ""; + } + } + + /** + * Returns the URI used to tell the host of the given alert for the systemic limit, which is the + * URI of the message that raised it, falling back to the URI of the alert. + * + *

Alerts rebuilt from the database might not have the message (nor the history reference) + * that raised them set, in which case the URI of the alert is used, otherwise the alert is not + * attributed to any host. + * + * @param alert the alert. + * @return the URI of the alert, or {@code null} if not available. + */ + private static URI getSystemicLimitUri(Alert alert) { + URI msgUri = alert.getMsgUri(); + if (msgUri != null) { + return msgUri; + } + String uri = alert.getUri(); + if (uri == null || uri.isEmpty()) { + return null; + } + try { + return new URI(uri, true); + } catch (URIException e) { + return null; } - return false; } } diff --git a/zap/src/main/java/org/zaproxy/zap/extension/alert/TextAlertTree.java b/zap/src/main/java/org/zaproxy/zap/extension/alert/TextAlertTree.java index 6277b7d25b9..3ed258232a9 100644 --- a/zap/src/main/java/org/zaproxy/zap/extension/alert/TextAlertTree.java +++ b/zap/src/main/java/org/zaproxy/zap/extension/alert/TextAlertTree.java @@ -47,7 +47,7 @@ private static void dumpRoot(AlertNode root, StringBuilder sb) { private static void dumpAlert(AlertNode node, StringBuilder sb) { sb.append(" - "); - sb.append(Alert.MSG_RISK[node.getRisk()]); + sb.append(riskName(node)); sb.append(": "); sb.append(node.getNodeName()); sb.append("\n"); @@ -56,6 +56,15 @@ private static void dumpAlert(AlertNode node, StringBuilder sb) { .forEachRemaining(child -> dumpAlertInstance((AlertNode) child, sb)); } + private static String riskName(AlertNode node) { + int risk = node.getRisk(); + if (risk < 0) { + // Nodes for false positives don't have a risk + return Alert.MSG_CONFIDENCE[Alert.CONFIDENCE_FALSE_POSITIVE]; + } + return Alert.MSG_RISK[risk]; + } + private static void dumpAlertInstance(AlertNode node, StringBuilder sb) { sb.append(" - "); sb.append(node.getNodeName()); diff --git a/zap/src/test/java/org/zaproxy/zap/extension/alert/AlertAPIUnitTest.java b/zap/src/test/java/org/zaproxy/zap/extension/alert/AlertAPIUnitTest.java index da6e020ebe1..a9bf0dd0f37 100644 --- a/zap/src/test/java/org/zaproxy/zap/extension/alert/AlertAPIUnitTest.java +++ b/zap/src/test/java/org/zaproxy/zap/extension/alert/AlertAPIUnitTest.java @@ -29,6 +29,7 @@ import java.util.Arrays; import java.util.List; +import java.util.Map; import java.util.Vector; import java.util.stream.Collectors; import net.sf.json.JSONObject; @@ -116,7 +117,10 @@ void shouldReturnAlertData() throws Exception { given(recordAlert.getSourceId()).willReturn(2); given(recordAlert.getAlertRef()).willReturn("1234-1"); - given(tableAlert.read(alertId)).willReturn(recordAlert); + // The alert must be built before the stubbing starts, as building it interacts with the + // recordAlert mock. + Alert alert = new Alert(recordAlert); + given(extensionAlert.getAlert(alertId)).willReturn(alert); // When ApiResponse response = api.handleApiView(name, params); // Then @@ -129,6 +133,33 @@ void shouldReturnAlertData() throws Exception { "{\"alert\":{\"sourceid\":\"2\",\"other\":\"other info\",\"method\":\"\",\"evidence\":\"evidence\",\"pluginId\":\"1234\",\"cweid\":\"10\",\"confidence\":\"Medium\",\"sourceMessageId\":1234,\"wascid\":\"11\",\"description\":\"Alert Description\",\"messageId\":\"123\",\"inputVector\":\"input Vector\",\"url\":\"uri\",\"tags\":{},\"reference\":\"reference\",\"solution\":\"solution\",\"alert\":\"Alert Name\",\"param\":\"param\",\"attack\":\"attack\",\"name\":\"Alert Name\",\"risk\":\"Low\",\"id\":\"1\",\"alertRef\":\"1234-1\"}}"))); } + @Test + void shouldReturnAlertDataWithTags() throws Exception { + // Given + String name = "alert"; + JSONObject params = new JSONObject(); + int alertId = 1; + params.put("id", alertId); + RecordAlert recordAlert = mock(RecordAlert.class); + given(recordAlert.getAlertId()).willReturn(alertId); + given(recordAlert.getPluginId()).willReturn(1234); + given(recordAlert.getAlert()).willReturn("Alert Name"); + given(recordAlert.getAlertRef()).willReturn("1234-1"); + + Alert alert = new Alert(recordAlert); + alert.setTags(Map.of("SYSTEMIC", "true")); + given(extensionAlert.getAlert(alertId)).willReturn(alert); + + // When + ApiResponse response = api.handleApiView(name, params); + + // Then + assertThat(response, is(instanceOf(ApiResponseElement.class))); + assertThat( + ((JSONObject) response.toJSON()).getJSONObject("alert").get("tags").toString(), + is(equalTo("{\"SYSTEMIC\":\"true\"}"))); + } + @Test void shouldNotReturnFalsePositiveAlertsByDefault() throws Exception { // Given diff --git a/zap/src/test/java/org/zaproxy/zap/extension/alert/AlertTreeModelUnitTest.java b/zap/src/test/java/org/zaproxy/zap/extension/alert/AlertTreeModelUnitTest.java index 97705ac853d..9654fe41a0b 100644 --- a/zap/src/test/java/org/zaproxy/zap/extension/alert/AlertTreeModelUnitTest.java +++ b/zap/src/test/java/org/zaproxy/zap/extension/alert/AlertTreeModelUnitTest.java @@ -20,6 +20,8 @@ package org.zaproxy.zap.extension.alert; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; @@ -433,6 +435,306 @@ void shouldChangeUniqueAlert() { assertEquals(a4, atModel.getRoot().getChildAt(2).getChildAt(0).getAlert()); } + @Test + void shouldNotAddNodeWhenAlertOverSystemicLimit() { + // Given + given(extAlert.isOverSystemicLimit(any(), any())).willReturn(true); + Alert a1 = + newAlert( + 1, + 0, + "Alert A", + "https://www.example.com", + "https://www.example.com", + Alert.RISK_LOW, + Alert.CONFIDENCE_MEDIUM); + + // When + atModel.addPath(a1); + + // Then + assertEquals(0, atModel.getRoot().getChildCount()); + } + + @Test + void shouldNotLeaveOldNodeWhenUpdatingAlertOverSystemicLimit() { + // Given + ExtensionHistory extHistory = mock(ExtensionHistory.class); + given(extensionLoader.getExtension(ExtensionHistory.class)).willReturn(extHistory); + given(extAlert.isOverSystemicLimit(any(), any())) + .willAnswer( + invocation -> { + Alert alert = invocation.getArgument(0); + return alert.getUri().endsWith("/a2"); + }); + Alert a1 = + newAlert( + 1, + 0, + "Alert A", + "https://www.example.com/a1", + "https://www.example.com/a1", + Alert.RISK_LOW, + Alert.CONFIDENCE_MEDIUM); + Alert a2 = + newAlert( + 1, + 1, + "Alert A", + "https://www.example.com/a2", + "https://www.example.com/a2", + Alert.RISK_LOW, + Alert.CONFIDENCE_MEDIUM); + atModel.addPath(a1); + updatePathToFalsePositive(a1); + + atModel.addPath(a2); + assertNoEmptyGroupNodes(atModel); + + // When + updatePathToFalsePositive(a2); + + // Then + assertEquals( + """ + - Alerts + - False Positive: Alert A + - GET:https://www.example.com/a1 + """, + TextAlertTree.toString(atModel)); + assertNoEmptyGroupNodes(atModel); + } + + @Test + void shouldNotApplySystemicLimitToFilteredTreeModel() { + // Given - the limit is reached + given(extAlert.isOverSystemicLimit(any(), any())).willReturn(true); + Alert a1 = + newAlert( + 1, + 0, + "Alert A", + "https://www.example.com/a1", + "https://www.example.com/a1", + Alert.RISK_LOW, + Alert.CONFIDENCE_MEDIUM); + + // When - the alert is added to the filtered tree, which is a subset of the main tree + AlertTreeModel filteredModel = new AlertTreeModel(extAlert, false); + filteredModel.addPath(a1); + + // Then - it is added, the limit is applied to the main tree model only + assertEquals(1, filteredModel.getRoot().getChildCount()); + assertEquals(1, filteredModel.getRoot().getChildAt(0).getChildCount()); + + // ...while the main tree model does apply it + atModel.addPath(a1); + assertEquals(0, atModel.getRoot().getChildCount()); + } + + @Test + void shouldNotAddUpdatedAlertToNewGroupWhenOverSystemicLimit() { + // Given - the alert is shown but the group of alerts it now belongs to is over the systemic + // limit + given(extAlert.isOverSystemicLimit(any(), any())) + .willAnswer( + invocation -> + ((Alert) invocation.getArgument(0)).getConfidence() + != Alert.CONFIDENCE_MEDIUM); + Alert a1 = + newAlert( + 1, + 0, + "Alert A", + "https://www.example.com/a1", + "https://www.example.com/a1", + Alert.RISK_LOW, + Alert.CONFIDENCE_MEDIUM); + atModel.addPath(a1); + + // When - the alert is changed to a false positive, as an alert filter would do + updatePathToFalsePositive(a1); + + // Then - the alert is not shown, the systemic limit is still honoured, and no node is left + // behind in the tree + assertEquals(0, atModel.getRoot().getChildCount()); + } + + @Test + void shouldNotLeaveOldNodeWhenUpdatingDeDuplicatedAlerts() { + // Given - two equivalent alerts (e.g. the same URL scanned twice), only one is added + ExtensionHistory extHistory = mock(ExtensionHistory.class); + given(extensionLoader.getExtension(ExtensionHistory.class)).willReturn(extHistory); + + Alert a1 = + newAlert( + 1, + 0, + "Alert A", + "https://www.example.com", + "https://www.example.com", + Alert.RISK_LOW, + Alert.CONFIDENCE_MEDIUM); + Alert a2 = + newAlert( + 1, + 1, + "Alert A", + "https://www.example.com", + "https://www.example.com", + Alert.RISK_LOW, + Alert.CONFIDENCE_MEDIUM); + atModel.addPath(a1); + atModel.addPath(a2); + + // When - both alerts are changed to false positives and the tree updated + updatePathToFalsePositive(a1); + updatePathToFalsePositive(a2); + + // Then - only the false positive node is left, no node with the old risk + assertEquals( + """ + - Alerts + - False Positive: Alert A + - GET:https://www.example.com + """, + TextAlertTree.toString(atModel)); + assertNoEmptyGroupNodes(atModel); + } + + @Test + void shouldNotLeaveOldNodeWhenAlertOverSystemicLimitIsNotUpdated() { + // Given - the first alert is added and changed to a false positive, the second alert is + // over the systemic limit so no node is added for it (and no alert added event is published + // for it, i.e. it is never updated in the tree) + given(extAlert.isOverSystemicLimit(any(), any())) + .willAnswer( + invocation -> { + Alert alert = invocation.getArgument(0); + return alert.getUri().endsWith("/a2"); + }); + Alert a1 = + newAlert( + 1, + 0, + "Alert A", + "https://www.example.com/a1", + "https://www.example.com/a1", + Alert.RISK_LOW, + Alert.CONFIDENCE_MEDIUM); + Alert a2 = + newAlert( + 1, + 1, + "Alert A", + "https://www.example.com/a2", + "https://www.example.com/a2", + Alert.RISK_LOW, + Alert.CONFIDENCE_MEDIUM); + atModel.addPath(a1); + updatePathToFalsePositive(a1); + atModel.addPath(a2); + + // Then - no node with the old risk is left in the tree, the node that is shown represents + // the false positive alert + assertEquals( + """ + - Alerts + - False Positive: Alert A + - GET:https://www.example.com/a1 + """, + TextAlertTree.toString(atModel)); + assertNoEmptyGroupNodes(atModel); + assertEquals( + Alert.CONFIDENCE_FALSE_POSITIVE, + atModel.getRoot().getChildAt(0).getAlert().getConfidence()); + } + + @Test + void shouldAddFalsePositiveNodeWhenAlertOverSystemicLimitChangedToFalsePositive() { + // Given - the alert is over the systemic limit so no node is added for it + given(extAlert.isOverSystemicLimit(any(), any())) + .willAnswer( + invocation -> { + Alert alert = invocation.getArgument(0); + return alert.getConfidence() != Alert.CONFIDENCE_FALSE_POSITIVE; + }); + Alert a1 = + newAlert( + 1, + 0, + "Alert A", + "https://www.example.com", + "https://www.example.com", + Alert.RISK_LOW, + Alert.CONFIDENCE_MEDIUM); + atModel.addPath(a1); + + // When - the alert filter changes it to a false positive + updatePathToFalsePositive(a1); + + // Then - the false positive node is added, no node with the old risk + assertEquals( + """ + - Alerts + - False Positive: Alert A + - GET:https://www.example.com + """, + TextAlertTree.toString(atModel)); + assertNoEmptyGroupNodes(atModel); + } + + @Test + void shouldRemoveEmptyGroupNodeWhenAlertUpdated() { + // Given - a group node without alerts, which should not be in the tree + Alert a1 = + newAlert( + 1, + 0, + "Alert A", + "https://www.example.com", + "https://www.example.com", + Alert.RISK_LOW, + Alert.CONFIDENCE_MEDIUM); + atModel.addPath(a1); + atModel.getRoot().getChildAt(0).remove(0); + + // When + updatePathToFalsePositive(a1); + + // Then - the group node with the old risk is replaced by the false positive node + assertEquals( + """ + - Alerts + - False Positive: Alert A + - GET:https://www.example.com + """, + TextAlertTree.toString(atModel)); + assertNoEmptyGroupNodes(atModel); + } + + @Test + void shouldRemoveEmptyGroupNodeWhenAlertDeleted() { + // Given - a group node without alerts, which should not be in the tree + Alert a1 = + newAlert( + 1, + 0, + "Alert A", + "https://www.example.com", + "https://www.example.com", + Alert.RISK_LOW, + Alert.CONFIDENCE_MEDIUM); + atModel.addPath(a1); + atModel.getRoot().getChildAt(0).remove(0); + + // When + atModel.deletePath(a1); + + // Then - the group node without alerts is removed and no other nodes are affected + assertEquals(0, atModel.getRoot().getChildCount()); + } + @Test void shouldDeleteNodeWhenNoAlertsLeft() { // Given @@ -489,6 +791,42 @@ void shouldDeleteNodeWhenNoAlertsLeft() { assertEquals(a4, atModel.getRoot().getChildAt(0).getChildAt(0).getAlert()); } + /** + * Updates the given alert in the tree as the alert filter does, i.e. with a new alert instance + * with false positive confidence, calling the update twice as done by {@code ExtensionAlert} + * and the {@code alertFilters} add-on. + */ + private void updatePathToFalsePositive(Alert alert) { + atModel.updatePath(falsePositive(alert)); + atModel.updatePath(falsePositive(alert)); + } + + private static Alert falsePositive(Alert alert) { + Alert fp = + new Alert( + alert.getPluginId(), + alert.getRisk(), + Alert.CONFIDENCE_FALSE_POSITIVE, + alert.getName()); + fp.setAlertRef(alert.getAlertRef()); + fp.setUri(alert.getUri()); + fp.setNodeName(alert.getNodeName()); + fp.setAlertId(alert.getAlertId()); + fp.setHistoryRef(alert.getHistoryRef()); + return fp; + } + + /** Asserts that every group node (i.e. every node below the root) has alerts. */ + private static void assertNoEmptyGroupNodes(AlertTreeModel model) { + AlertNode root = model.getRoot(); + for (int i = 0; i < root.getChildCount(); i++) { + AlertNode groupNode = root.getChildAt(i); + assertTrue( + groupNode.getChildCount() > 0, + "Group node without alerts in the tree: " + groupNode.getNodeName()); + } + } + private static Alert newAlert( int pluginId, int id, diff --git a/zap/src/test/java/org/zaproxy/zap/extension/alert/ExtensionAlertUnitTest.java b/zap/src/test/java/org/zaproxy/zap/extension/alert/ExtensionAlertUnitTest.java index a44323c3e66..3de03a04b88 100644 --- a/zap/src/test/java/org/zaproxy/zap/extension/alert/ExtensionAlertUnitTest.java +++ b/zap/src/test/java/org/zaproxy/zap/extension/alert/ExtensionAlertUnitTest.java @@ -23,18 +23,27 @@ import static org.hamcrest.Matchers.hasEntry; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Stream; import org.apache.commons.httpclient.URI; import org.junit.jupiter.api.BeforeEach; @@ -46,10 +55,17 @@ import org.mockito.MockedStatic; import org.parosproxy.paros.Constant; import org.parosproxy.paros.core.scanner.Alert; +import org.parosproxy.paros.db.Database; +import org.parosproxy.paros.db.RecordAlert; +import org.parosproxy.paros.db.TableAlert; import org.parosproxy.paros.model.HistoryReference; import org.parosproxy.paros.model.Model; import org.parosproxy.paros.model.Session; import org.parosproxy.paros.network.HttpMessage; +import org.zaproxy.zap.ZAP; +import org.zaproxy.zap.db.TableAlertTag; +import org.zaproxy.zap.eventBus.Event; +import org.zaproxy.zap.eventBus.EventConsumer; import org.zaproxy.zap.model.ParameterParser; import org.zaproxy.zap.model.StandardParameterParser; import org.zaproxy.zap.utils.I18N; @@ -76,7 +92,11 @@ class ExtensionAlertUnitTest { private static final Map NEW_TAG = Collections.singletonMap("Original Key", "New Value"); + private static final int HISTORY_ID = 7; + private ExtensionAlert extAlert; + private TableAlert tableAlert; + private TableAlertTag tableAlertTag; @BeforeEach void setUp() throws Exception { @@ -694,60 +714,531 @@ void shouldPrependAlertTagCorrectly(String value) { } @Test - void shouldIdentifySystemicAlerts() { - // Given + void shouldShowAlertsUpToSystemicLimit() throws Exception { + // Given - a systemic rule with a limit of three + HistoryReference href = setUpExtensionWithDb(); extAlert.getAlertParam().load(new ZapXmlConfiguration()); extAlert.getAlertParam().setSystemicLimit(3); - Alert a1 = - newAlert( - 1, - 0, - "Alert A", - "https://www.example.com(a)", - "https://www.example.com?a=1"); - Alert a2 = - newAlert( - 1, - 1, - "Alert A", - "https://www.example.com(a)", - "https://www.example.com?a=2"); - Alert a3 = - newAlert( - 1, - 2, - "Alert A", - "https://www.example.com(a)", - "https://www.example.com?a=3"); - Alert a4 = - newAlert( - 1, - 3, - "Alert A", - "https://www.example.com(a)", - "https://www.example.com?a=4"); + // When - four alerts of the rule are raised + try (MockedStatic hr = mockStatic(HistoryReference.class)) { + hr.when(() -> HistoryReference.getTags(anyInt())).thenReturn(List.of()); + for (int i = 1; i <= 4; i++) { + Alert alert = newAlertToRaise("https://www.example.com/" + i); + alert.setTags(Map.of("SYSTEMIC", "true")); + extAlert.alertFound(alert, href); + } + } + // Then - only the alerts up to the limit are shown + AlertNode root = extAlert.getTreeModel().getRoot(); + assertEquals(1, root.getChildCount()); + assertEquals(3, root.getChildAt(0).getChildCount()); + } + + @Test + void shouldNotCountTheSameAlertTwiceTowardsTheSystemicLimit() throws Exception { + // Given - a systemic rule with a limit of one, with one alert shown + HistoryReference href = setUpExtensionWithDb(); + extAlert.getAlertParam().load(new ZapXmlConfiguration()); + extAlert.getAlertParam().setSystemicLimit(1); + Alert a1 = newAlertToRaise("https://www.example.com/1"); a1.setTags(Map.of("SYSTEMIC", "true")); + Alert a2 = newAlertToRaise("https://www.example.com/2"); a2.setTags(Map.of("SYSTEMIC", "true")); - a3.setTags(Map.of("SYSTEMIC", "true")); - a4.setTags(Map.of("SYSTEMIC", "true")); + try (MockedStatic hr = mockStatic(HistoryReference.class)) { + hr.when(() -> HistoryReference.getTags(anyInt())).thenReturn(List.of()); + extAlert.alertFound(a1, href); + extAlert.alertFound(a2, href); + } + AlertNode root = extAlert.getTreeModel().getRoot(); + assertEquals(1, root.getChildAt(0).getChildCount()); + + // When - the shown alert is changed, as an alert filter does, which takes it out of the + // tree and adds it back + for (int i = 0; i < 3; i++) { + a1.setRiskConfidence(a1.getRisk(), Alert.CONFIDENCE_MEDIUM); + extAlert.updateAlert(a1); + } + + // Then - it does not count more than once, thus it is still shown and the other alert is + // still not shown + assertEquals(1, root.getChildCount()); + assertEquals(1, root.getChildAt(0).getChildCount()); + } + + @Test + void shouldCountSystemicLimitPerHost() throws Exception { + // Given - a systemic rule with a limit of two + HistoryReference href = setUpExtensionWithDb(); + extAlert.getAlertParam().load(new ZapXmlConfiguration()); + extAlert.getAlertParam().setSystemicLimit(2); + + // When - three alerts are raised for each of two hosts + try (MockedStatic hr = mockStatic(HistoryReference.class)) { + hr.when(() -> HistoryReference.getTags(anyInt())).thenReturn(List.of()); + for (String host : List.of("https://www.example.com", "https://www.example.net")) { + for (int i = 1; i <= 3; i++) { + Alert alert = newAlertToRaise(host + "/" + i); + alert.setTags(Map.of("SYSTEMIC", "true")); + extAlert.alertFound(alert, href); + } + } + } + + // Then - the limit is counted for each host + AlertNode root = extAlert.getTreeModel().getRoot(); + assertEquals(1, root.getChildCount()); + assertEquals(4, root.getChildAt(0).getChildCount()); + } + + @Test + void shouldNotApplySystemicLimitToAlertsThatAreNotSystemic() throws Exception { + // Given - a limit of one, for a rule whose alerts are not systemic + HistoryReference href = setUpExtensionWithDb(); + extAlert.getAlertParam().load(new ZapXmlConfiguration()); + extAlert.getAlertParam().setSystemicLimit(1); + + // When - three alerts of the rule are raised + try (MockedStatic hr = mockStatic(HistoryReference.class)) { + hr.when(() -> HistoryReference.getTags(anyInt())).thenReturn(List.of()); + for (int i = 1; i <= 3; i++) { + extAlert.alertFound(newAlertToRaise("https://www.example.com/" + i), href); + } + } + + // Then - all of them are shown, the limit does not apply to them + AlertNode root = extAlert.getTreeModel().getRoot(); + assertEquals(1, root.getChildCount()); + assertEquals(3, root.getChildAt(0).getChildCount()); + } + + @Test + void shouldDeriveGroupWhenCheckingSystemicLimit() throws Exception { + // Given - a systemic rule with a limit of one, with one alert of the rule shown + HistoryReference href = setUpExtensionWithDb(); + extAlert.getAlertParam().load(new ZapXmlConfiguration()); + extAlert.getAlertParam().setSystemicLimit(1); + try (MockedStatic hr = mockStatic(HistoryReference.class)) { + hr.when(() -> HistoryReference.getTags(anyInt())).thenReturn(List.of()); + Alert a1 = newAlertToRaise("https://www.example.com/1"); + a1.setTags(Map.of("SYSTEMIC", "true")); + extAlert.alertFound(a1, href); + } + + // When/Then - the group of an alert of the same rule and risk/confidence is derived, the + // limit is already reached + Alert sameGroup = newAlertToRaise("https://www.example.com/2"); + sameGroup.setTags(Map.of("SYSTEMIC", "true")); + assertTrue(extAlert.isOverSystemicLimit(sameGroup)); + + // ...and an alert of another rule, for which the tree has no group, is not over the limit + Alert otherGroup = newAlertToRaise("https://www.example.com/2"); + otherGroup.setName("Alert B"); + otherGroup.setTags(Map.of("SYSTEMIC", "true")); + assertFalse(extAlert.isOverSystemicLimit(otherGroup)); + + // ...and the checks have no side effects, the limit is still reached + assertTrue(extAlert.isOverSystemicLimit(sameGroup)); + assertFalse(extAlert.isOverSystemicLimit(null)); + } + + @Test + void shouldNotShowMoreAlertsThanTheSystemicLimitWhenFiltered() throws Exception { + // Given - fifty alerts of a systemic rule, with the default limit of five, thus only the + // first ones raised are shown in the tree + HistoryReference href = setUpExtensionWithDb(); + extAlert.getAlertParam().load(new ZapXmlConfiguration()); + extAlert.getAlertParam().setSystemicLimit(5); + List raised = new ArrayList<>(); + try (MockedStatic hr = mockStatic(HistoryReference.class)) { + hr.when(() -> HistoryReference.getTags(anyInt())).thenReturn(List.of()); + for (int i = 1; i <= 50; i++) { + Alert alert = newAlertToRaise("https://www.example.com/" + i); + alert.setTags(Map.of("SYSTEMIC", "true")); + extAlert.alertFound(alert, href); + raised.add(alert); + } + } + assertEquals(5, extAlert.getTreeModel().getRoot().getChildAt(0).getChildCount()); + + // When - all the alerts are changed to false positives, as the alert filters do + for (Alert alert : raised) { + alert.setRiskConfidence(alert.getRisk(), Alert.CONFIDENCE_FALSE_POSITIVE); + extAlert.updateAlert(alert); + } + + // Then - the limit is still honoured, the alerts are not shown above it just because they + // changed to the same risk/confidence + AlertNode root = extAlert.getTreeModel().getRoot(); + assertEquals(1, root.getChildCount()); + // False positive alerts are shown with a risk of -1 in the tree + assertEquals(-1, root.getChildAt(0).getRisk()); + assertTrue( + root.getChildAt(0).getChildCount() <= 5, + "Alerts shown above the systemic limit: " + root.getChildAt(0).getChildCount()); + } + + @Test + void shouldShowAllAlertsAsFalsePositiveWhenFiltered() throws Exception { + // Given - nine alerts of a systemic rule, with a limit of five, thus only the first five + // are shown in the tree + HistoryReference href = setUpExtensionWithDb(); + extAlert.getAlertParam().load(new ZapXmlConfiguration()); + extAlert.getAlertParam().setSystemicLimit(5); + List raised = new ArrayList<>(); + try (MockedStatic hr = mockStatic(HistoryReference.class)) { + hr.when(() -> HistoryReference.getTags(anyInt())).thenReturn(List.of()); + for (int i = 1; i <= 9; i++) { + Alert alert = newAlertToRaise("https://www.example.com/" + i); + alert.setTags(Map.of("SYSTEMIC", "true")); + extAlert.alertFound(alert, href); + raised.add(alert); + } + } + AlertNode root = extAlert.getTreeModel().getRoot(); + assertEquals(1, root.getChildCount()); + assertEquals(5, root.getChildAt(0).getChildCount()); + + // When - all the alerts are changed to false positives, as the alert filters do + for (Alert alert : raised) { + alert.setRiskConfidence(alert.getRisk(), Alert.CONFIDENCE_FALSE_POSITIVE); + extAlert.updateAlert(alert); + } + + // Then - the alerts shown in the tree are all false positives, none is left with the + // original risk, the alerts that were over the systemic limit are not shown + assertEquals( + """ + - Alerts + - False Positive: Alert A + - :https://www.example.com/1 + - :https://www.example.com/2 + - :https://www.example.com/3 + - :https://www.example.com/4 + - :https://www.example.com/5 + """, + TextAlertTree.toString(extAlert.getTreeModel())); + } + + @Test + void shouldUpdateAlertRebuiltWithoutMessage() throws Exception { + // Given - an alert shown in the tree + HistoryReference href = setUpExtensionWithDb(); + extAlert.getAlertParam().load(new ZapXmlConfiguration()); + extAlert.getAlertParam().setSystemicLimit(5); + try (MockedStatic hr = mockStatic(HistoryReference.class)) { + hr.when(() -> HistoryReference.getTags(anyInt())).thenReturn(List.of()); + extAlert.alertFound(newAlertToRaise("https://www.example.com/1"), href); + } + AlertNode root = extAlert.getTreeModel().getRoot(); + assertEquals(1, root.getChildAt(0).getChildCount()); + + // When - the alert is rebuilt from the database, without the message (nor the history + // reference) that raised it, as the alert filters do, and changed to a false positive + Alert rebuilt = newSystemicAlertWithoutMessage(1); + rebuilt.setName("Alert A"); + rebuilt.setHistoryId(HISTORY_ID); + rebuilt.setNodeName("https://www.example.com/1"); + rebuilt.setRiskConfidence(Alert.RISK_MEDIUM, Alert.CONFIDENCE_FALSE_POSITIVE); + extAlert.updateAlert(rebuilt); + + // Then - the alert is no longer shown with the original confidence + assertEquals(1, root.getChildCount()); + assertEquals(1, root.getChildAt(0).getChildCount()); + assertEquals( + Alert.CONFIDENCE_FALSE_POSITIVE, + root.getChildAt(0).getChildAt(0).getAlert().getConfidence()); + } + + @Test + void shouldReadAlertWithTags() throws Exception { + // Given - an alert stored with tags + HistoryReference href = setUpExtensionWithDb(); + given(tableAlertTag.getTagsByAlertId(anyLong())).willReturn(Map.of("SYSTEMIC", "true")); + Alert raised = newAlertToRaise("https://www.example.com/"); + try (MockedStatic hr = mockStatic(HistoryReference.class)) { + hr.when(() -> HistoryReference.getTags(anyInt())).thenReturn(List.of()); + extAlert.alertFound(raised, href); + } // When - boolean b1 = extAlert.isOverSystemicLimit(a1); - boolean b2 = extAlert.isOverSystemicLimit(a2); - boolean b3 = extAlert.isOverSystemicLimit(a3); - boolean b4 = extAlert.isOverSystemicLimit(a4); - - // Then - assertTrue(a1.isSystemic()); - assertTrue(a2.isSystemic()); - assertTrue(a3.isSystemic()); - assertTrue(a4.isSystemic()); - assertFalse(b1); - assertFalse(b2); - assertFalse(b3); - assertTrue(b4); + Alert alert = extAlert.getAlert(raised.getAlertId()); + + // Then - the alert is read with the tags stored for it + assertEquals(raised.getAlertId(), alert.getAlertId()); + assertEquals(Map.of("SYSTEMIC", "true"), alert.getTags()); + assertTrue(alert.isSystemic()); + } + + @Test + void shouldReturnNullWhenAlertNotFound() throws Exception { + // Given + setUpExtensionWithDb(); + given(tableAlert.read(anyInt())).willReturn(null); + + // When/Then + assertNull(extAlert.getAlert(1234)); + } + + @Test + void shouldNotDeleteStoredTagsWhenAlertReadAndUpdated() throws Exception { + // Given - an alert stored with tags + HistoryReference href = setUpExtensionWithDb(); + given(tableAlertTag.getTagsByAlertId(anyLong())).willReturn(Map.of("SYSTEMIC", "true")); + Alert raised = newAlertToRaise("https://www.example.com/"); + try (MockedStatic hr = mockStatic(HistoryReference.class)) { + hr.when(() -> HistoryReference.getTags(anyInt())).thenReturn(List.of()); + extAlert.alertFound(raised, href); + } + Alert alert = extAlert.getAlert(raised.getAlertId()); + + // When - the alert is changed, as done by the API + alert.setRisk(Alert.RISK_LOW); + extAlert.updateAlert(alert); + + // Then - the tags stored for the alert were not removed + verify(tableAlertTag, never()).delete(anyLong(), anyString()); + verify(tableAlertTag, never()).deleteAllTagsForAlert(anyLong()); + } + + @Test + void shouldApplySystemicLimitToAlertsReadFromDb() throws Exception { + // Given - six alerts of a systemic rule, with a limit of five, thus only the first five are + // shown in the tree + HistoryReference href = setUpExtensionWithDb(); + extAlert.getAlertParam().load(new ZapXmlConfiguration()); + extAlert.getAlertParam().setSystemicLimit(5); + // The tags of the alerts are stored, as written when they were raised + given(tableAlertTag.getTagsByAlertId(anyLong())).willReturn(Map.of("SYSTEMIC", "true")); + List raised = new ArrayList<>(); + try (MockedStatic hr = mockStatic(HistoryReference.class)) { + hr.when(() -> HistoryReference.getTags(anyInt())).thenReturn(List.of()); + for (int i = 1; i <= 6; i++) { + Alert alert = newAlertToRaise("https://www.example.com/" + i); + alert.setTags(Map.of("SYSTEMIC", "true")); + extAlert.alertFound(alert, href); + raised.add(alert); + } + } + AlertNode root = extAlert.getTreeModel().getRoot(); + assertEquals(1, root.getChildCount()); + assertEquals(5, root.getChildAt(0).getChildCount()); + + // And - the alerts shown are changed to false positives, using up the limit + for (int i = 0; i < 5; i++) { + Alert alert = raised.get(i); + alert.setRiskConfidence(alert.getRisk(), Alert.CONFIDENCE_FALSE_POSITIVE); + extAlert.updateAlert(alert); + } + assertEquals(1, root.getChildCount()); + assertEquals(5, root.getChildAt(0).getChildCount()); + + // And - the sixth alert, which was not shown, is read from the database and changed to a + // false positive, as the alert filters do + Alert read = extAlert.getAlert(raised.get(5).getAlertId()); + read.setRiskConfidence(read.getRisk(), Alert.CONFIDENCE_FALSE_POSITIVE); + + // When + extAlert.updateAlert(read); + + // Then - the alert is known to be systemic, as its tags were read, and counted, the limit + // is already reached so it is not shown + assertTrue(read.isSystemic()); + assertEquals(1, root.getChildCount()); + assertEquals(5, root.getChildAt(0).getChildCount()); + } + + @Test + void shouldPublishAlertAddedEventWhenAlertIsNotAddedToTree() throws Exception { + // Given - two identical alerts (e.g. the same URL scanned twice), only the first is added + HistoryReference href = setUpExtensionWithDb(); + List events = new ArrayList<>(); + EventConsumer consumer = events::add; + String publisherName = AlertEventPublisher.getPublisher().getPublisherName(); + ZAP.getEventBus() + .registerConsumer(consumer, publisherName, AlertEventPublisher.ALERT_ADDED_EVENT); + try { + // When + try (MockedStatic hr = mockStatic(HistoryReference.class)) { + hr.when(() -> HistoryReference.getTags(anyInt())).thenReturn(List.of()); + extAlert.alertFound(newAlertToRaise("https://www.example.com/"), href); + extAlert.alertFound(newAlertToRaise("https://www.example.com/"), href); + } + + // Then - only the first alert was added to the tree + AlertNode root = extAlert.getTreeModel().getRoot(); + assertEquals(1, root.getChildCount()); + assertEquals(1, root.getChildAt(0).getChildCount()); + // ...but both alerts were published so consumers, e.g. alert filters, can act on them + assertEquals(2, events.size()); + } finally { + ZAP.getEventBus().unregisterConsumer(consumer, publisherName); + } + } + + @Test + void shouldPublishAlertAddedEventWhenAlertIsOverSystemicLimit() throws Exception { + // Given - only one alert of the rule is allowed + HistoryReference href = setUpExtensionWithDb(); + extAlert.getAlertParam().load(new ZapXmlConfiguration()); + extAlert.getAlertParam().setSystemicLimit(1); + Alert a1 = newAlertToRaise("https://www.example.com/1"); + Alert a2 = newAlertToRaise("https://www.example.com/2"); + a1.setTags(Map.of("SYSTEMIC", "true")); + a2.setTags(Map.of("SYSTEMIC", "true")); + + List events = new ArrayList<>(); + EventConsumer consumer = events::add; + String publisherName = AlertEventPublisher.getPublisher().getPublisherName(); + ZAP.getEventBus() + .registerConsumer(consumer, publisherName, AlertEventPublisher.ALERT_ADDED_EVENT); + try { + // When + try (MockedStatic hr = mockStatic(HistoryReference.class)) { + hr.when(() -> HistoryReference.getTags(anyInt())).thenReturn(List.of()); + extAlert.alertFound(a1, href); + extAlert.alertFound(a2, href); + } + + // Then - the second alert is over the limit and was not added to the tree + AlertNode root = extAlert.getTreeModel().getRoot(); + assertEquals(1, root.getChildCount()); + assertEquals(1, root.getChildAt(0).getChildCount()); + // ...but both alerts were published so consumers, e.g. alert filters, can act on them + assertEquals(2, events.size()); + } finally { + ZAP.getEventBus().unregisterConsumer(consumer, publisherName); + } + } + + @Test + void shouldUpdateStoredTagsWhenAlertUpdatedWithTags() throws Exception { + // Given - an alert stored with tags, updated with an alert that has its tags loaded + HistoryReference href = setUpExtensionWithDb(); + given(tableAlertTag.getTagsByAlertId(anyLong())).willReturn(Map.of("SYSTEMIC", "true")); + Alert raised = newAlertToRaise("https://www.example.com/"); + try (MockedStatic hr = mockStatic(HistoryReference.class)) { + hr.when(() -> HistoryReference.getTags(anyInt())).thenReturn(List.of()); + extAlert.alertFound(raised, href); + } + + Alert updated = new Alert(1, Alert.RISK_MEDIUM, Alert.CONFIDENCE_MEDIUM, "Alert A"); + updated.setUri("https://www.example.com/"); + updated.setAlertId(raised.getAlertId()); + updated.setHistoryId(HISTORY_ID); + updated.setTags(Map.of("New", "Value")); + extAlert.updateAlert(updated); + + // Then - the stored tags are replaced with the new ones + verify(tableAlertTag).delete(anyLong(), eq("SYSTEMIC")); + verify(tableAlertTag).insertOrUpdate(anyLong(), eq("New"), eq("Value")); + } + + /** + * Initialises the extension with a mocked model and database so that alerts can be raised and + * updated, and returns the {@code HistoryReference} used for the raised alerts. Each alert + * raised is written to the database with a new alert ID. + */ + private HistoryReference setUpExtensionWithDb() throws Exception { + Constant.messages = new I18N(Locale.ENGLISH); + + Session session = mock(Session.class); + Model model = mock(Model.class); + Model.setSingletonForTesting(model); + given(model.getSession()).willReturn(session); + given(session.getUrlParamParser(anyString())).willReturn(new StandardParameterParser()); + extAlert.initModel(model); + + Database database = mock(Database.class); + given(model.getDb()).willReturn(database); + tableAlert = mock(TableAlert.class); + given(database.getTableAlert()).willReturn(tableAlert); + tableAlertTag = mock(TableAlertTag.class); + given(database.getTableAlertTag()).willReturn(tableAlertTag); + AtomicInteger nextAlertId = new AtomicInteger(); + Map writtenAlerts = new HashMap<>(); + given( + tableAlert.write( + anyInt(), anyInt(), any(), anyInt(), anyInt(), any(), any(), any(), + any(), any(), any(), any(), any(), anyInt(), anyInt(), anyInt(), + anyInt(), anyInt(), any(), any(), any())) + .willAnswer( + invocation -> { + RecordAlert recordAlert = mock(RecordAlert.class); + int alertId = nextAlertId.incrementAndGet(); + given(recordAlert.getAlertId()).willReturn(alertId); + given(recordAlert.getHistoryId()) + .willReturn(invocation.getArgument(15)); + // Echo the values written, as they are read back + given(recordAlert.getPluginId()).willReturn(invocation.getArgument(1)); + given(recordAlert.getAlert()).willReturn(invocation.getArgument(2)); + given(recordAlert.getRisk()).willReturn(invocation.getArgument(3)); + given(recordAlert.getConfidence()) + .willReturn(invocation.getArgument(4)); + given(recordAlert.getDescription()) + .willReturn(invocation.getArgument(5)); + given(recordAlert.getUri()).willReturn(invocation.getArgument(6)); + given(recordAlert.getParam()).willReturn(invocation.getArgument(7)); + given(recordAlert.getAttack()).willReturn(invocation.getArgument(8)); + given(recordAlert.getOtherInfo()).willReturn(invocation.getArgument(9)); + given(recordAlert.getSolution()).willReturn(invocation.getArgument(10)); + given(recordAlert.getReference()) + .willReturn(invocation.getArgument(11)); + given(recordAlert.getEvidence()).willReturn(invocation.getArgument(12)); + given(recordAlert.getCweId()).willReturn(invocation.getArgument(13)); + given(recordAlert.getWascId()).willReturn(invocation.getArgument(14)); + given(recordAlert.getSourceHistoryId()) + .willReturn(invocation.getArgument(16)); + given(recordAlert.getSourceId()).willReturn(invocation.getArgument(17)); + given(recordAlert.getAlertRef()).willReturn(invocation.getArgument(18)); + given(recordAlert.getInputVector()) + .willReturn(invocation.getArgument(19)); + given(recordAlert.getNodeName()).willReturn(invocation.getArgument(20)); + writtenAlerts.put(alertId, recordAlert); + return recordAlert; + }); + given(tableAlert.read(anyInt())) + .willAnswer(invocation -> writtenAlerts.get(invocation.getArgument(0))); + + HistoryReference href = mock(HistoryReference.class); + given(href.getHistoryId()).willReturn(HISTORY_ID); + given(href.getHistoryType()).willReturn(HistoryReference.TYPE_SCANNER); + return href; + } + + private static Alert newAlertToRaise(String uri) throws Exception { + Alert alert = new Alert(1, Alert.RISK_MEDIUM, Alert.CONFIDENCE_MEDIUM, "Alert A"); + alert.setUri(uri); + HttpMessage msg = new HttpMessage(); + msg.getRequestHeader().setURI(new URI(uri, true)); + alert.setMessage(msg); + return alert; + } + + private static Alert newAlertOfRule(int id) { + return newAlert( + 1, id, "Alert A", "https://www.example.com(a)", "https://www.example.com?a=" + id); + } + + private static Alert newSystemicAlert(int id) { + Alert alert = newAlertOfRule(id); + alert.setTags(Map.of("SYSTEMIC", "true")); + return alert; + } + + private static Alert newSystemicAlertWithoutMessage(int id) { + Alert alert = newAlertWithoutMessage(id); + alert.setTags(Map.of("SYSTEMIC", "true")); + return alert; + } + + private static Alert newAlertWithoutMessage(int id) { + Alert alert = new Alert(1, Alert.RISK_MEDIUM, Alert.CONFIDENCE_MEDIUM, "Alert A"); + alert.setUri("https://www.example.com?a=" + id); + alert.setAlertId(id); + alert.setNodeName("https://www.example.com(a)"); + return alert; } private static Alert newAlert(int pluginId, int id, String name, String nodeName, String uri) {