Merge pull request #593 from apache/WW-5218-disable

[WW-5218] Allows to disable CSP related interceptors

core/src/main/java/org/apache/struts2/interceptor/CoepInterceptor.java

@@ -52,21 +52,30 @@ public class CoepInterceptor extends AbstractInterceptor implements PreResultLis
 
     @Override
     public String intercept(ActionInvocation invocation) throws Exception {
-        invocation.addPreResultListener(this);
+        if (disabled) {
+            LOG.trace("COEP interceptor has been disabled");
+        } else {
+            invocation.addPreResultListener(this);
+        }
         return invocation.invoke();
     }
 
     @Override
     public void beforeResult(ActionInvocation invocation, String resultCode) {
+        if (disabled) {
+            return;
+        }
+
         HttpServletRequest req = invocation.getInvocationContext().getServletRequest();
-        HttpServletResponse res = invocation.getInvocationContext().getServletResponse();
         final String path = req.getContextPath();
 
         if (exemptedPaths.contains(path)) {
             // no need to add headers
-            LOG.debug("Skipping COEP header for exempted path {}", path);
-        } else if (!disabled) {
-            res.setHeader(header, REQUIRE_COEP_HEADER);
+            LOG.debug("Skipping COEP header for exempted path: {}", path);
+        } else {
+            LOG.trace("Applying COEP header: {} with value: {}", header, REQUIRE_COEP_HEADER);
+            HttpServletResponse response = invocation.getInvocationContext().getServletResponse();
+            response.setHeader(header, REQUIRE_COEP_HEADER);
         }
     }
 

core/src/main/java/org/apache/struts2/interceptor/CoopInterceptor.java

@@ -49,25 +49,34 @@ public class CoopInterceptor extends AbstractInterceptor implements PreResultLis
     private static final String COOP_HEADER = "Cross-Origin-Opener-Policy";
 
     private final Set<String> exemptedPaths = new HashSet<>();
+    private boolean disabled = false;
     private String mode = SAME_ORIGIN;
 
     @Override
     public String intercept(ActionInvocation invocation) throws Exception {
-        invocation.addPreResultListener(this);
+        if (disabled) {
+            LOG.trace("COOP interceptor has been disabled");
+        } else {
+            invocation.addPreResultListener(this);
+        }
         return invocation.invoke();
     }
 
     @Override
     public void beforeResult(ActionInvocation invocation, String resultCode) {
+        if (disabled) {
+            return;
+        }
         HttpServletRequest request = invocation.getInvocationContext().getServletRequest();
-        HttpServletResponse response = invocation.getInvocationContext().getServletResponse();
         String path = request.getContextPath();
 
         if (isExempted(path)) {
             // no need to add headers
             LOG.debug("Skipping COOP header for exempted path {}", path);
         } else {
-            response.setHeader(COOP_HEADER, getMode());
+            LOG.trace("Applying COOP header: {} with value: {}", COOP_HEADER, mode);
+            HttpServletResponse response = invocation.getInvocationContext().getServletResponse();
+            response.setHeader(COOP_HEADER, mode);
         }
     }
 
@@ -79,15 +88,14 @@ public void setExemptedPaths(String paths) {
         exemptedPaths.addAll(TextParseUtil.commaDelimitedStringToSet(paths));
     }
 
-    private String getMode() {
-        return mode;
-    }
-
     public void setMode(String mode) {
         if (!(mode.equals(SAME_ORIGIN) || mode.equals(SAME_ORIGIN_ALLOW_POPUPS) || mode.equals(UNSAFE_NONE))) {
             throw new IllegalArgumentException(String.format("Mode '%s' not recognized!", mode));
         }
         this.mode = mode;
     }
 
+    public void setDisabled(String value) {
+        this.disabled = Boolean.parseBoolean(value);
+    }
 }

core/src/main/java/org/apache/struts2/interceptor/FetchMetadataInterceptor.java

@@ -18,24 +18,25 @@
  */
 package org.apache.struts2.interceptor;
 
-import static org.apache.struts2.interceptor.ResourceIsolationPolicy.SEC_FETCH_DEST_HEADER;
-import static org.apache.struts2.interceptor.ResourceIsolationPolicy.SEC_FETCH_MODE_HEADER;
-import static org.apache.struts2.interceptor.ResourceIsolationPolicy.SEC_FETCH_SITE_HEADER;
-import static org.apache.struts2.interceptor.ResourceIsolationPolicy.SEC_FETCH_USER_HEADER;
-import static org.apache.struts2.interceptor.ResourceIsolationPolicy.VARY_HEADER;
-
 import com.opensymphony.xwork2.ActionContext;
 import com.opensymphony.xwork2.ActionInvocation;
 import com.opensymphony.xwork2.inject.Inject;
 import com.opensymphony.xwork2.interceptor.AbstractInterceptor;
 import com.opensymphony.xwork2.util.TextParseUtil;
-import java.util.HashSet;
-import java.util.Set;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.util.HashSet;
+import java.util.Set;
+
+import static org.apache.struts2.interceptor.ResourceIsolationPolicy.SEC_FETCH_DEST_HEADER;
+import static org.apache.struts2.interceptor.ResourceIsolationPolicy.SEC_FETCH_MODE_HEADER;
+import static org.apache.struts2.interceptor.ResourceIsolationPolicy.SEC_FETCH_SITE_HEADER;
+import static org.apache.struts2.interceptor.ResourceIsolationPolicy.SEC_FETCH_USER_HEADER;
+import static org.apache.struts2.interceptor.ResourceIsolationPolicy.VARY_HEADER;
+
 /**
  * Interceptor that implements Fetch Metadata policy on incoming requests used to protect against
  * CSRF, XSSI, and cross-origin information leaks. Uses {@link StrutsResourceIsolationPolicy} to
@@ -46,20 +47,27 @@
  **/
 
 public class FetchMetadataInterceptor extends AbstractInterceptor {
+
     private static final Logger LOG = LogManager.getLogger(FetchMetadataInterceptor.class);
     private static final String VARY_HEADER_VALUE = String.format("%s,%s,%s,%s", SEC_FETCH_DEST_HEADER, SEC_FETCH_MODE_HEADER, SEC_FETCH_SITE_HEADER, SEC_FETCH_USER_HEADER);
     private static final String SC_FORBIDDEN = String.valueOf(HttpServletResponse.SC_FORBIDDEN);
 
     private final Set<String> exemptedPaths = new HashSet<>();
     private final ResourceIsolationPolicy resourceIsolationPolicy = new StrutsResourceIsolationPolicy();
 
-    @Inject (required=false)
+    private boolean disabled = false;
+
+    @Inject(required = false)
     public void setExemptedPaths(String paths) {
         this.exemptedPaths.addAll(TextParseUtil.commaDelimitedStringToSet(paths));
     }
 
     @Override
     public String intercept(ActionInvocation invocation) throws Exception {
+        if (disabled) {
+            LOG.trace("Fetch Metadata interceptor has been disabled");
+            return invocation.invoke();
+        }
         ActionContext context = invocation.getInvocationContext();
         HttpServletRequest request = context.getServletRequest();
 
@@ -76,31 +84,34 @@ public String intercept(ActionInvocation invocation) throws Exception {
             return invocation.invoke();
         }
 
-        LOG.info("Fetch metadata rejected cross-origin request to [{}]", contextPath);
+        LOG.warn("Fetch metadata rejected cross-origin request to: {}", contextPath);
         return SC_FORBIDDEN;
     }
 
     /**
-     * Sets {@link SEC_FETCH_DEST_HEADER}, {@link SEC_FETCH_MODE_HEADER}, {@link SEC_FETCH_SITE_HEADER}, and {@link SEC_FETCH_USER_HEADER}
-     * elements in the provided ActionInvocation's HttpServletResponse {@link VARY_HEADER} response header.
-     * 
+     * Sets {@link ResourceIsolationPolicy#SEC_FETCH_DEST_HEADER}, {@link ResourceIsolationPolicy#SEC_FETCH_MODE_HEADER},
+     * {@link ResourceIsolationPolicy#SEC_FETCH_SITE_HEADER}, and {@link ResourceIsolationPolicy#SEC_FETCH_USER_HEADER}
+     * elements in the provided ActionInvocation's HttpServletResponse {@link ResourceIsolationPolicy#VARY_HEADER} response header.
+     * <p>
      * Note: This method will replace any previous Vary header content already set for the response.
-     * Note: In order to be effective, the Vary header modification must take place at (or very near) the start of this interceptor's processing.
-     * 
-     * @param invocation  Supplies the HttpServletResponse (if present) to which the SEC_FETCH_* header names are be added to its {@link VARY_HEADER} response header.
+     * Note: In order to be effective, the Vary header modification must take place at (or very near) the start of this
+     * interceptor's processing.
+     *
+     * @param invocation Supplies the HttpServletResponse (if present) to which the SEC_FETCH_* header names are be added
+     *                   to its {@link ResourceIsolationPolicy#VARY_HEADER} response header.
      */
     private void addVaryHeaders(ActionInvocation invocation) {
         HttpServletResponse response = invocation.getInvocationContext().getServletResponse();
-        if (response != null) {
-            // TODO: Whenever servlet 3.x becomes the baseline for Struts, consider revising this method to use
-            //       getHeader(VARY_HEADER) and preserve any VARY_HEADER content already set in the response.
-            //       This will probably require some tokenization logic for the header contents.
-            if (LOG.isDebugEnabled() && response.containsHeader(VARY_HEADER)) {
-                LOG.debug("HTTP response already has a [{}] header set, the old value will be overwritten (replaced)", VARY_HEADER);
-            }
-            response.setHeader(VARY_HEADER, VARY_HEADER_VALUE);
-        } else {
-            LOG.debug("HTTP response is null, cannot add a new [{}] header", VARY_HEADER);
+        // TODO: Whenever servlet 3.x becomes the baseline for Struts, consider revising this method to use
+        //       getHeader(VARY_HEADER) and preserve any VARY_HEADER content already set in the response.
+        //       This will probably require some tokenization logic for the header contents.
+        if (LOG.isDebugEnabled() && response.containsHeader(VARY_HEADER)) {
+            LOG.debug("HTTP response already has header: {} set, the old value will be overwritten (replaced)", VARY_HEADER);
         }
+        response.setHeader(VARY_HEADER, VARY_HEADER_VALUE);
+    }
+
+    public void setDisabled(String value) {
+        this.disabled = Boolean.parseBoolean(value);
     }
 }

core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java

@@ -21,10 +21,13 @@
 import com.opensymphony.xwork2.ActionInvocation;
 import com.opensymphony.xwork2.interceptor.AbstractInterceptor;
 import com.opensymphony.xwork2.interceptor.PreResultListener;
-import java.net.URI;
-import java.util.Optional;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
+import java.net.URI;
+import java.util.Optional;
 
 /**
  * Interceptor that implements Content Security Policy on incoming requests used to protect against
@@ -38,15 +41,26 @@
  **/
 public final class CspInterceptor extends AbstractInterceptor implements PreResultListener {
 
+    private static final Logger LOG = LogManager.getLogger(CspInterceptor.class);
+
     private final CspSettings settings = new DefaultCspSettings();
 
+    private boolean disabled = false;
+
     @Override
     public String intercept(ActionInvocation invocation) throws Exception {
-        invocation.addPreResultListener(this);
+        if (disabled) {
+            LOG.trace("CSP interceptor has been disabled");
+        } else {
+            invocation.addPreResultListener(this);
+        }
         return invocation.invoke();
     }
 
     public void beforeResult(ActionInvocation invocation, String resultCode) {
+        if (disabled) {
+            return;
+        }
         HttpServletRequest request = invocation.getInvocationContext().getServletRequest();
         HttpServletResponse response = invocation.getInvocationContext().getServletResponse();
         settings.addCspHeaders(request, response);
@@ -74,8 +88,12 @@ private Optional<URI> buildUri(String reportUri) {
         return Optional.empty();
     }
 
-    public void setEnforcingMode(String value){
+    public void setEnforcingMode(String value) {
         boolean enforcingMode = Boolean.parseBoolean(value);
         settings.setEnforcingMode(enforcingMode);
     }
+
+    public void setDisabled(String value) {
+        this.disabled = Boolean.parseBoolean(value);
+    }
 }

core/src/main/java/org/apache/struts2/interceptor/csp/DefaultCspSettings.java

@@ -53,11 +53,11 @@ public void addCspHeaders(HttpServletResponse response) {
 
     public void addCspHeaders(HttpServletRequest request, HttpServletResponse response) {
         if (isSessionActive(request)) {
-            LOG.debug("Session is active, applying CSP settings");
+            LOG.trace("Session is active, applying CSP settings");
             associateNonceWithSession(request);
             response.setHeader(cspHeader, cratePolicyFormat(request));
         } else {
-            LOG.debug("Session is not active, ignoring CSP settings");
+            LOG.trace("Session is not active, ignoring CSP settings");
         }
     }
 

core/src/main/resources/struts-default.xml

@@ -392,6 +392,7 @@
                 <interceptor-ref name="servletConfig"/>
                 <interceptor-ref name="i18n"/>
                 <interceptor-ref name="cspInterceptor">
+                    <param name="disabled">false</param>
                     <param name="enforcingMode">false</param>
                 </interceptor-ref>
                 <interceptor-ref name="prepare"/>
@@ -407,15 +408,18 @@
                 <interceptor-ref name="params"/>
                 <interceptor-ref name="conversionError"/>
                 <interceptor-ref name="coepInterceptor">
-                    <param name="enforcingMode">false</param>
                     <param name="disabled">false</param>
+                    <param name="enforcingMode">false</param>
                     <param name="exemptedPaths"/>
                 </interceptor-ref>
                 <interceptor-ref name="coopInterceptor">
+                    <param name="disabled">false</param>
                     <param name="exemptedPaths"/>
                     <param name="mode">same-origin</param>
                 </interceptor-ref>
-                <interceptor-ref name="fetchMetadata"/>
+                <interceptor-ref name="fetchMetadata">
+                    <param name="disabled">false</param>
+                </interceptor-ref>
                 <interceptor-ref name="validation">
                     <param name="excludeMethods">input,back,cancel,browse</param>
                 </interceptor-ref>

core/src/test/java/org/apache/struts2/interceptor/CoopInterceptorTest.java

@@ -34,7 +34,6 @@ public class CoopInterceptorTest extends StrutsInternalTestCase {
     private final MockHttpServletResponse response = new MockHttpServletResponse();
 
     String SAME_ORIGIN = "same-origin";
-    String SAME_SITE = "same-site";
     String UNSAFE_NONE = "unsafe-none";
     String COOP_HEADER = "Cross-Origin-Opener-Policy";
 
@@ -65,7 +64,7 @@ public void testChangeDefaultMode() throws Exception {
         assertEquals("Coop header is not same-origin", UNSAFE_NONE, header);
     }
 
-    public void testErrorNotRecognizedMode() throws Exception {
+    public void testErrorNotRecognizedMode() {
         request.setContextPath("/some");
 
         try{
@@ -76,6 +75,15 @@ public void testErrorNotRecognizedMode() throws Exception {
         }
     }
 
+    public void testDisabled() throws Exception {
+        interceptor.setDisabled("true");
+
+        interceptor.intercept(mai);
+
+        String header = response.getHeader(COOP_HEADER);
+        assertTrue("COOP is not disabled", Strings.isEmpty(header));
+    }
+
     @Override
     protected void setUp() throws Exception {
         super.setUp();

core/src/test/java/org/apache/struts2/interceptor/CspInterceptorTest.java

@@ -121,7 +121,7 @@ public void test_uriSetOnlyWhenSetIsCalled() throws Exception {
         checkHeader(reportUri, enforcingMode);
     }
 
-    public void testCannotParseUri() throws Exception {
+    public void testCannotParseUri() {
         String enforcingMode = "false";
         interceptor.setEnforcingMode(enforcingMode);
 
@@ -133,7 +133,7 @@ public void testCannotParseUri() throws Exception {
         }
     }
 
-    public void testCannotParseRelativeUri() throws Exception {
+    public void testCannotParseRelativeUri() {
         String enforcingMode = "false";
         interceptor.setEnforcingMode(enforcingMode);
 
@@ -145,8 +145,19 @@ public void testCannotParseRelativeUri() throws Exception {
         }
     }
 
+    public void testDisabled() throws Exception {
+        interceptor.setDisabled("true");
+
+        interceptor.intercept(mai);
+
+        String header = response.getHeader(CSP_ENFORCE_HEADER);
+        assertTrue("CSP is not disabled", Strings.isEmpty(header));
+        header = response.getHeader(CSP_REPORT_HEADER);
+        assertTrue("CSP is not disabled", Strings.isEmpty(header));
+    }
+
     public void checkHeader(String reportUri, String enforcingMode) {
-        String expectedCspHeader = "";
+        String expectedCspHeader;
         if (Strings.isEmpty(reportUri)) {
             expectedCspHeader = String.format("%s '%s'; %s 'nonce-%s' '%s' %s %s; %s '%s'; ",
                 OBJECT_SRC, NONE,
@@ -162,7 +173,7 @@ public void checkHeader(String reportUri, String enforcingMode) {
             );
         }
 
-        String header = "";
+        String header;
         if (enforcingMode.equals("true")) {
             header = response.getHeader(CSP_ENFORCE_HEADER);
         } else {