pscanrulesBeta: fix isolation scan rule success check (#5957)

Change check to !getHelper().isSuccess(msg) and add stub implementation for all tests.

addOns/pscanrulesBeta/CHANGELOG.md

@@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ### Fixed
 - Fix typo in log message.
+- Fix Insufficient Site Isolation scan rule check that filters responses based on whether a response is a success or not.
 
 ### Changed
 - Maintenance changes.

addOns/pscanrulesBeta/src/main/java/org/zaproxy/zap/extension/pscanrulesBeta/SiteIsolationScanRule.java

@@ -34,7 +34,6 @@
 import org.parosproxy.paros.network.HttpHeader;
 import org.parosproxy.paros.network.HttpMessage;
 import org.parosproxy.paros.network.HttpResponseHeader;
-import org.parosproxy.paros.network.HttpStatusCode;
 import org.zaproxy.addon.commonlib.CommonAlertTag;
 import org.zaproxy.zap.extension.pscan.PluginPassiveScanner;
 
@@ -83,13 +82,10 @@ public class SiteIsolationScanRule extends PluginPassiveScanner
 
     @Override
     public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) {
-
-        // Specs don't state that errors pages should be excluded
-        // However, successful responses are associated to a resource
-        // that should be protected.
-        // Only consider HTTP Status code 2XX to avoid a False Positive
-        if (!HttpStatusCode.isSuccess(msg.getResponseHeader().getStatusCode())
-                || getHelper().isPage200(msg)) {
+        // Specs don't state that errors pages should be excluded. However, successful responses are
+        // associated to a resource that should be protected, while error pages are not. Therefore,
+        // only consider HTTP Status code 2XX to avoid a False Positive
+        if (!getHelper().isSuccess(msg)) {
             return;
         }
 

addOns/pscanrulesBeta/src/test/java/org/zaproxy/zap/extension/pscanrulesBeta/SiteIsolationScanRuleTest.java

@@ -36,6 +36,25 @@
 import org.zaproxy.addon.commonlib.CommonAlertTag;
 
 class SiteIsolationScanRuleTest extends PassiveScannerTest<SiteIsolationScanRule> {
+    @Test
+    void shouldRaiseCorpAlertGivenSiteIsNotIsolated() throws Exception {
+        // Given
+        HttpMessage msg = new HttpMessage();
+        msg.setRequestHeader("GET / HTTP/1.1");
+        msg.setResponseHeader("HTTP/1.1 200 OK\r\n");
+        given(passiveScanData.isSuccess(any())).willReturn(true);
+
+        // When
+        scanHttpResponseReceive(msg);
+
+        // Then
+        assertThat(alertsRaised, hasSize(1));
+        assertThat(
+                alertsRaised.get(0).getParam(),
+                equalTo(SiteIsolationScanRule.CorpHeaderScanRule.HEADER));
+        assertThat(alertsRaised.get(0).getEvidence(), equalTo(""));
+    }
+
     @Test
     void shouldNotRaiseAlertGivenSiteIsIsolated() throws Exception {
         // Given
@@ -46,7 +65,7 @@ void shouldNotRaiseAlertGivenSiteIsIsolated() throws Exception {
                         + "Cross-Origin-Resource-Policy: same-origin\r\n"
                         + "Cross-Origin-Embedder-Policy: require-corp\r\n"
                         + "Cross-Origin-Opener-Policy: same-origin\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(false);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -66,7 +85,7 @@ void shouldNotRaiseAlertGivenSiteIsIsolatedWhenSuccessIdentifiedByCustomPage()
                         + "Cross-Origin-Resource-Policy: same-origin\r\n"
                         + "Cross-Origin-Embedder-Policy: require-corp\r\n"
                         + "Cross-Origin-Opener-Policy: same-origin\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(true);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -86,7 +105,7 @@ void shouldNotRaiseAlertGivenSiteIsIsolatedWhenSuccessAndIdentifiedByCustomPage(
                         + "Cross-Origin-Resource-Policy: same-origin\r\n"
                         + "Cross-Origin-Embedder-Policy: require-corp\r\n"
                         + "Cross-Origin-Opener-Policy: same-origin\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(true);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -95,6 +114,38 @@ void shouldNotRaiseAlertGivenSiteIsIsolatedWhenSuccessAndIdentifiedByCustomPage(
         assertThat(alertsRaised, hasSize(0));
     }
 
+    @Test
+    void shouldNotRaiseAlertGivenSiteIsNotIsolatedWhenSuccessNotIdentifiedByCustomPage()
+            throws Exception {
+        // Given
+        HttpMessage msg = new HttpMessage();
+        msg.setRequestHeader("GET / HTTP/1.1");
+        msg.setResponseHeader("HTTP/1.1 200 OK\r\n" + "Content-Type: text/html\r\n");
+        given(passiveScanData.isSuccess(any())).willReturn(false);
+
+        // When
+        scanHttpResponseReceive(msg);
+
+        // Then
+        assertThat(alertsRaised, hasSize(0));
+    }
+
+    @Test
+    void shouldRaiseAlertGivenSiteIsNotIsolatedWhenSuccessIdentifiedByCustomPage()
+            throws Exception {
+        // Given
+        HttpMessage msg = new HttpMessage();
+        msg.setRequestHeader("GET / HTTP/1.1");
+        msg.setResponseHeader("HTTP/1.1 400 OK\r\n" + "Content-Type: text/html\r\n");
+        given(passiveScanData.isSuccess(any())).willReturn(true);
+
+        // When
+        scanHttpResponseReceive(msg);
+
+        // Then
+        assertThat(alertsRaised, hasSize(3));
+    }
+
     @Test
     void shouldRaiseCorpAlertGivenResponseDoesntSendCorpHeader() throws Exception {
         // Given
@@ -104,7 +155,7 @@ void shouldRaiseCorpAlertGivenResponseDoesntSendCorpHeader() throws Exception {
                 "HTTP/1.1 200 OK\r\n"
                         + "Cross-Origin-Embedder-Policy: require-corp\r\n"
                         + "Cross-Origin-Opener-Policy: same-origin\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(false);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -127,6 +178,7 @@ void shouldRaiseCorpAlertGivenCorpHeaderIsSetForSameSite() throws Exception {
                         + "Cross-Origin-Resource-Policy: same-site\r\n"
                         + "Cross-Origin-Embedder-Policy: require-corp\r\n"
                         + "Cross-Origin-Opener-Policy: same-origin\r\n");
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -149,7 +201,7 @@ void shouldRaiseCorpAlertGivenCorpHeaderContentIsUnexpected() throws Exception {
                         + "Cross-Origin-Resource-Policy: unexpected\r\n"
                         + "Cross-Origin-Embedder-Policy: require-corp\r\n"
                         + "Cross-Origin-Opener-Policy: same-origin\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(false);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -172,7 +224,7 @@ void shouldRaiseCorpAlertCaseInsensitive() throws Exception {
                         + "Cross-Origin-Resource-Policy: same-SITE\r\n"
                         + "Cross-Origin-Embedder-Policy: require-corp\r\n"
                         + "Cross-Origin-Opener-Policy: same-origin\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(false);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -196,7 +248,7 @@ void shouldNotRaiseCorpAlertGivenCorpHeaderIsSetForCrossOrigin() throws Exceptio
                         + "Cross-Origin-Resource-Policy: cross-origin\r\n"
                         + "Cross-Origin-Embedder-Policy: require-corp\r\n"
                         + "Cross-Origin-Opener-Policy: same-origin\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(false);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -211,7 +263,7 @@ void shouldRaiseCorpAlertOnlyForSuccessfulQueries() throws Exception {
         HttpMessage msg = new HttpMessage();
         msg.setRequestHeader("GET / HTTP/1.1");
         msg.setResponseHeader("HTTP/1.1 500 Internal Server Error\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(false);
+        given(passiveScanData.isSuccess(any())).willReturn(false);
 
         // When
         scanHttpResponseReceive(msg);
@@ -232,7 +284,7 @@ void shouldNotRaiseCorpAlertGivenCorsHeaderIsSet(String corsFieldName) throws Ex
                         + ": *\r\n"
                         + "Cross-Origin-Embedder-Policy: require-corp\r\n"
                         + "Cross-Origin-Opener-Policy: same-origin\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(false);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -251,7 +303,7 @@ void shouldRaiseAlertGivenCoepHeaderIsMissing() throws Exception {
                         + "Content-Type: application/xml\r\n"
                         + "Cross-Origin-Resource-Policy: same-origin\r\n"
                         + "Cross-Origin-Opener-Policy: same-origin\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(false);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -275,7 +327,7 @@ void shouldRaiseAlertGivenCoepHeaderIsNotEqualsToRequireCorp() throws Exception
                         + "Cross-Origin-Resource-Policy: same-origin\r\n"
                         + "Cross-Origin-Embedder-Policy: unsafe-none\r\n"
                         + "Cross-Origin-Opener-Policy: same-origin\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(false);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -298,7 +350,7 @@ void shouldRaiseAlertGivenCoopHeaderIsMissing() throws Exception {
                         + "Content-Type: text/html;charset=utf-8\r\n"
                         + "Cross-Origin-Resource-Policy: same-origin\r\n"
                         + "Cross-Origin-Embedder-Policy: require-corp\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(false);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -322,7 +374,7 @@ void shouldRaiseAlertGivenCoopHeaderIsNotSameOrigin() throws Exception {
                         + "Cross-Origin-Resource-Policy: same-origin\r\n"
                         + "Cross-Origin-Embedder-Policy: require-corp\r\n"
                         + "Cross-Origin-Opener-Policy: same-origin-allow-popups\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(false);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -345,7 +397,7 @@ void shouldNotRaiseCoepOrCoopAlertGivenResourceIsNotAnHtmlOrXmlDocument() throws
                 "HTTP/1.1 200 OK\r\n"
                         + "Content-Type: application/json\r\n"
                         + "Cross-Origin-Resource-Policy: same-origin\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(false);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -364,7 +416,7 @@ void shouldNotRaiseAlertGivenNoHeaderContentTypeIsPresent() throws Exception {
         msg.setRequestHeader("GET / HTTP/1.1");
         msg.setResponseHeader(
                 "HTTP/1.1 200 OK\r\n" + "Cross-Origin-Resource-Policy: same-origin\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(false);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);
@@ -384,7 +436,7 @@ void shouldNotRaiseAlertForReportingAPI() throws Exception {
                         + "cross-origin-embedder-policy: require-corp;report-to=\"coep\"\r\n"
                         + "cross-origin-opener-policy: same-origin;report-to=\"coop\"\r\n"
                         + "Cross-Origin-Resource-Policy: same-origin;report-to=\"corp\"\r\n");
-        given(passiveScanData.isPage200(any())).willReturn(false);
+        given(passiveScanData.isSuccess(any())).willReturn(true);
 
         // When
         scanHttpResponseReceive(msg);