From 758174c52ee1ce050cf671ca7e53c79c4115d8a3 Mon Sep 17 00:00:00 2001 From: "Erica S. Kane" Date: Wed, 10 Apr 2024 18:42:09 -0400 Subject: [PATCH 1/3] WW-5400 Extend default configuration options for the CSP interceptor. Previously, it was impossible to set global options for the CSP interceptor. The only options was to have every action individually implement CspSettingsAware. To fix this, we add an interceptor parameter of defaultCspSettingsClassName. Values from this class will be used in the CSP header instead of DefaultCspSettings. Users may define their own custom class which implements CspSettings, and that will be the default for all actions that do not implement the CspSettingsAware interface. It is now possible to create this custom class by simply extending DefaultCspSettings. I have fixed a spelling error in DefaultCspSettings.java -- cratePolicyFormat renamed to createPolicyFormat. --- .../interceptor/csp/CspInterceptor.java | 42 +++++++++- .../struts2/interceptor/csp/CspSettings.java | 6 ++ .../interceptor/csp/DefaultCspSettings.java | 34 ++++++-- .../interceptor/CspInterceptorTest.java | 83 +++++++++++++++++-- 4 files changed, 147 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java index 32d677786..d382dce94 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java @@ -46,6 +46,9 @@ public final class CspInterceptor extends AbstractInterceptor { private boolean prependServletContext = true; private boolean enforcingMode; private String reportUri; + private String reportTo; + + private String defaultCspSettingsClassName = DefaultCspSettings.class.getName(); @Override public String intercept(ActionInvocation invocation) throws Exception { @@ -54,8 +57,24 @@ public final class CspInterceptor extends AbstractInterceptor { LOG.trace("Using CspSettings provided by the action: {}", action); applySettings(invocation, ((CspSettingsAware) action).getCspSettings()); } else { - LOG.trace("Using DefaultCspSettings with action: {}", action); - applySettings(invocation, new DefaultCspSettings()); + LOG.trace("Using {} with action: {}", defaultCspSettingsClassName, action); + + // if the defaultCspSettingsClassName is not a real class, throw an exception + try { + Class.forName(defaultCspSettingsClassName, false, Thread.currentThread().getContextClassLoader()); + } + catch (ClassNotFoundException e) { + throw new IllegalArgumentException("The defaultCspSettingsClassName must be a real class."); + } + + // if defaultCspSettingsClassName does not implement CspSettings, throw an exception + if (!CspSettings.class.isAssignableFrom(Class.forName(defaultCspSettingsClassName))) { + throw new IllegalArgumentException("The defaultCspSettingsClassName must implement CspSettings."); + } + + CspSettings cspSettings = (CspSettings) Class.forName(defaultCspSettingsClassName) + .getDeclaredConstructor().newInstance(); + applySettings(invocation, cspSettings); } return invocation.invoke(); } @@ -76,6 +95,12 @@ public final class CspInterceptor extends AbstractInterceptor { } cspSettings.setReportUri(finalReportUri); + + // apply reportTo if set + if (reportTo != null) { + LOG.trace("Applying: {} to reportTo", reportTo); + cspSettings.setReportTo(reportTo); + } } invocation.addPreResultListener((actionInvocation, resultCode) -> { @@ -97,6 +122,10 @@ public final class CspInterceptor extends AbstractInterceptor { this.reportUri = reportUri; } + public void setReportTo(String reportTo) { + this.reportTo = reportTo; + } + private Optional buildUri(String reportUri) { try { return Optional.of(URI.create(reportUri)); @@ -124,4 +153,11 @@ public final class CspInterceptor extends AbstractInterceptor { this.prependServletContext = prependServletContext; } -} + /** + * Sets the class name of the default {@link CspSettings} implementation to use when the action does not + * set its own values. If not set, the default is {@link DefaultCspSettings}. + */ + public void setDefaultCspSettingsClassName(String defaultCspSettingsClassName) { + this.defaultCspSettingsClassName = defaultCspSettingsClassName; + } +} \ No newline at end of file diff --git a/core/src/main/java/org/apache/struts2/interceptor/csp/CspSettings.java b/core/src/main/java/org/apache/struts2/interceptor/csp/CspSettings.java index acb142962..3b2cadb96 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/csp/CspSettings.java +++ b/core/src/main/java/org/apache/struts2/interceptor/csp/CspSettings.java @@ -37,6 +37,7 @@ public interface CspSettings { String SCRIPT_SRC = "script-src"; String BASE_URI = "base-uri"; String REPORT_URI = "report-uri"; + String REPORT_TO = "report-to"; String NONE = "none"; String STRICT_DYNAMIC = "strict-dynamic"; String HTTP = "http:"; @@ -56,6 +57,11 @@ public interface CspSettings { */ void setReportUri(String uri); + /** + * Sets the report group where csp violation reports will be sent + */ + void setReportTo(String group); + /** * Sets CSP headers in enforcing mode when true, and report-only when false */ diff --git a/core/src/main/java/org/apache/struts2/interceptor/csp/DefaultCspSettings.java b/core/src/main/java/org/apache/struts2/interceptor/csp/DefaultCspSettings.java index d1768e869..51c76cfa8 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/csp/DefaultCspSettings.java +++ b/core/src/main/java/org/apache/struts2/interceptor/csp/DefaultCspSettings.java @@ -20,6 +20,7 @@ package org.apache.struts2.interceptor.csp; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.struts2.action.CspSettingsAware; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -31,7 +32,11 @@ import static java.lang.String.format; /** * Default implementation of {@link CspSettings}. - * The default policy implements strict CSP with a nonce based approach and follows the guide: https://csp.withgoogle.com/docs/index.html/ + * The default policy implements strict CSP with a nonce based approach and follows the guide: + * https://csp.withgoogle.com/docs/index.html/ + * You may extend or replace this class if you wish to customize the default policy further, and use your class + * by setting the {@link CspInterceptor} defaultCspSettingsClassName parameter. Actions that + * implement the {@link CspSettingsAware} interface will ignore the defaultCspSettingsClassName parameter. * * @see CspSettings * @see CspInterceptor @@ -42,20 +47,22 @@ public class DefaultCspSettings implements CspSettings { private final SecureRandom sRand = new SecureRandom(); - private String reportUri; + protected String reportUri; + protected String reportTo; // default to reporting mode - private String cspHeader = CSP_REPORT_HEADER; + protected String cspHeader = CSP_REPORT_HEADER; @Override public void addCspHeaders(HttpServletResponse response) { throw new UnsupportedOperationException("Unsupported implementation, use #addCspHeaders(HttpServletRequest request, HttpServletResponse response)"); } + @Override public void addCspHeaders(HttpServletRequest request, HttpServletResponse response) { if (isSessionActive(request)) { LOG.trace("Session is active, applying CSP settings"); associateNonceWithSession(request); - response.setHeader(cspHeader, cratePolicyFormat(request)); + response.setHeader(cspHeader, createPolicyFormat(request)); } else { LOG.trace("Session is not active, ignoring CSP settings"); } @@ -70,7 +77,7 @@ public class DefaultCspSettings implements CspSettings { request.getSession().setAttribute("nonce", nonceValue); } - private String cratePolicyFormat(HttpServletRequest request) { + protected String createPolicyFormat(HttpServletRequest request) { StringBuilder policyFormatBuilder = new StringBuilder() .append(OBJECT_SRC) .append(format(" '%s'; ", NONE)) @@ -84,13 +91,18 @@ public class DefaultCspSettings implements CspSettings { if (reportUri != null) { policyFormatBuilder .append(REPORT_URI) - .append(format(" %s", reportUri)); + .append(format(" %s; ", reportUri)); + if(reportTo != null) { + policyFormatBuilder + .append(REPORT_TO) + .append(format(" %s; ", reportTo)); + } } return format(policyFormatBuilder.toString(), getNonceString(request)); } - private String getNonceString(HttpServletRequest request) { + protected String getNonceString(HttpServletRequest request) { Object nonce = request.getSession().getAttribute("nonce"); return Objects.toString(nonce); } @@ -101,20 +113,28 @@ public class DefaultCspSettings implements CspSettings { return ret; } + @Override public void setEnforcingMode(boolean enforcingMode) { if (enforcingMode) { cspHeader = CSP_ENFORCE_HEADER; } } + @Override public void setReportUri(String reportUri) { this.reportUri = reportUri; } + @Override + public void setReportTo(String reportTo) { + this.reportTo = reportTo; + } + @Override public String toString() { return "DefaultCspSettings{" + "reportUri='" + reportUri + '\'' + + "reportTo='" + reportTo + '\'' + ", cspHeader='" + cspHeader + '\'' + '}'; } diff --git a/core/src/test/java/org/apache/struts2/interceptor/CspInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/CspInterceptorTest.java index 0b03c6e54..cd59c347d 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/CspInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/CspInterceptorTest.java @@ -31,6 +31,7 @@ import org.apache.struts2.interceptor.csp.DefaultCspSettings; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpSession; import static org.junit.Assert.assertNotEquals; @@ -74,8 +75,10 @@ public class CspInterceptorTest extends StrutsInternalTestCase { public void testEnforcingCspHeadersSet() throws Exception { String reportUri = "/csp-reports"; + String reportTo = "csp-group"; boolean enforcingMode = true; interceptor.setReportUri(reportUri); + interceptor.setReportTo(reportTo); interceptor.setEnforcingMode(enforcingMode); session.setAttribute("nonce", "foo"); @@ -84,13 +87,15 @@ public class CspInterceptorTest extends StrutsInternalTestCase { assertNotNull("Nonce key does not exist", session.getAttribute("nonce")); assertFalse("Nonce value is empty", Strings.isEmpty((String) session.getAttribute("nonce"))); assertNotEquals("New nonce value couldn't be set", "foo", session.getAttribute("nonce")); - checkHeader(reportUri, enforcingMode); + checkHeader(reportUri, reportTo, enforcingMode); } public void testReportingCspHeadersSet() throws Exception { String reportUri = "/csp-reports"; + String reportTo = "csp-group"; boolean enforcingMode = false; interceptor.setReportUri(reportUri); + interceptor.setReportTo(reportTo); interceptor.setEnforcingMode(enforcingMode); session.setAttribute("nonce", "foo"); @@ -98,7 +103,7 @@ public class CspInterceptorTest extends StrutsInternalTestCase { assertNotNull("Nonce value is empty", session.getAttribute("nonce")); assertNotEquals("New nonce value couldn't be set", "foo", session.getAttribute("nonce")); - checkHeader(reportUri, enforcingMode); + checkHeader(reportUri, reportTo, enforcingMode); } public void test_uriSetOnlyWhenSetIsCalled() throws Exception { @@ -174,7 +179,47 @@ public class CspInterceptorTest extends StrutsInternalTestCase { checkHeader("/report-uri", enforcingMode); } + public void testInvalidDefaultCspSettingsClassName() throws Exception { + boolean enforcingMode = true; + mai.setAction(new TestAction()); + request.setContextPath("/app"); + + interceptor.setEnforcingMode(enforcingMode); + interceptor.setReportUri("/report-uri"); + interceptor.setPrependServletContext(false); + + try { + interceptor.setDefaultCspSettingsClassName("foo"); + interceptor.intercept(mai); + assert (false); + } catch (IllegalArgumentException e) { + assert (true); + } + } + + public void testCustomDefaultCspSettingsClassName() throws Exception { + boolean enforcingMode = true; + mai.setAction(new TestAction()); + request.setContextPath("/app"); + + interceptor.setEnforcingMode(enforcingMode); + interceptor.setReportUri("/report-uri"); + interceptor.setPrependServletContext(false); + interceptor.setDefaultCspSettingsClassName(CustomDefaultCspSettings.class.getName()); + + interceptor.intercept(mai); + + String header = response.getHeader(CspSettings.CSP_ENFORCE_HEADER); + + // no other customization matters for this particular class + assertEquals("foo", header); + } + public void checkHeader(String reportUri, boolean enforcingMode) { + checkHeader(reportUri, null, enforcingMode); + } + + public void checkHeader(String reportUri, String reportTo, boolean enforcingMode) { String expectedCspHeader; if (Strings.isEmpty(reportUri)) { expectedCspHeader = String.format("%s '%s'; %s 'nonce-%s' '%s' %s %s; %s '%s'; ", @@ -183,12 +228,23 @@ public class CspInterceptorTest extends StrutsInternalTestCase { CspSettings.BASE_URI, CspSettings.NONE ); } else { - expectedCspHeader = String.format("%s '%s'; %s 'nonce-%s' '%s' %s %s; %s '%s'; %s %s", - CspSettings.OBJECT_SRC, CspSettings.NONE, - CspSettings.SCRIPT_SRC, session.getAttribute("nonce"), CspSettings.STRICT_DYNAMIC, CspSettings.HTTP, CspSettings.HTTPS, - CspSettings.BASE_URI, CspSettings.NONE, - CspSettings.REPORT_URI, reportUri - ); + if (Strings.isEmpty(reportTo)) { + expectedCspHeader = String.format("%s '%s'; %s 'nonce-%s' '%s' %s %s; %s '%s'; %s %s; ", + CspSettings.OBJECT_SRC, CspSettings.NONE, + CspSettings.SCRIPT_SRC, session.getAttribute("nonce"), CspSettings.STRICT_DYNAMIC, CspSettings.HTTP, CspSettings.HTTPS, + CspSettings.BASE_URI, CspSettings.NONE, + CspSettings.REPORT_URI, reportUri + ); + } + else { + expectedCspHeader = String.format("%s '%s'; %s 'nonce-%s' '%s' %s %s; %s '%s'; %s %s; %s %s; ", + CspSettings.OBJECT_SRC, CspSettings.NONE, + CspSettings.SCRIPT_SRC, session.getAttribute("nonce"), CspSettings.STRICT_DYNAMIC, CspSettings.HTTP, CspSettings.HTTPS, + CspSettings.BASE_URI, CspSettings.NONE, + CspSettings.REPORT_URI, reportUri, + CspSettings.REPORT_TO, reportTo + ); + } } String header; @@ -230,4 +286,15 @@ public class CspInterceptorTest extends StrutsInternalTestCase { return settings; } } + + /** + * Custom DefaultCspSettings class that overrides the createPolicyFormat method + * to return a fixed value. + */ + public static class CustomDefaultCspSettings extends DefaultCspSettings { + + protected String createPolicyFormat(HttpServletRequest request) { + return "foo"; + } + } } From 0151bdeb9ca23f750c2454acd4a5d94635993307 Mon Sep 17 00:00:00 2001 From: "Erica S. Kane" Date: Thu, 11 Apr 2024 16:52:44 -0400 Subject: [PATCH 2/3] WW-5400 Better toString formatting --- .../org/apache/struts2/interceptor/csp/DefaultCspSettings.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/csp/DefaultCspSettings.java b/core/src/main/java/org/apache/struts2/interceptor/csp/DefaultCspSettings.java index 51c76cfa8..b245ab352 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/csp/DefaultCspSettings.java +++ b/core/src/main/java/org/apache/struts2/interceptor/csp/DefaultCspSettings.java @@ -134,7 +134,7 @@ public class DefaultCspSettings implements CspSettings { public String toString() { return "DefaultCspSettings{" + "reportUri='" + reportUri + '\'' + - "reportTo='" + reportTo + '\'' + + ", reportTo='" + reportTo + '\'' + ", cspHeader='" + cspHeader + '\'' + '}'; } From 6ac8b04ad222f59b4e21711f0782bb717b28e042 Mon Sep 17 00:00:00 2001 From: "Erica S. Kane" Date: Fri, 12 Apr 2024 13:40:53 -0400 Subject: [PATCH 3/3] WW-5400 Added @since Struts 6.5.0 to new properties as requested --- .../apache/struts2/interceptor/csp/CspInterceptor.java | 10 ++++++++++ .../apache/struts2/interceptor/csp/CspSettings.java | 2 ++ 2 files changed, 12 insertions(+) diff --git a/core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java index d382dce94..54d9eeab1 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java @@ -122,6 +122,14 @@ public final class CspInterceptor extends AbstractInterceptor { this.reportUri = reportUri; } + /** + * Sets the report group where csp violation reports will be sent. This will + * only be used if the reportUri is set. + * + * @param reportTo the report group where csp violation reports will be sent + * + * @since Struts 6.5.0 + */ public void setReportTo(String reportTo) { this.reportTo = reportTo; } @@ -156,6 +164,8 @@ public final class CspInterceptor extends AbstractInterceptor { /** * Sets the class name of the default {@link CspSettings} implementation to use when the action does not * set its own values. If not set, the default is {@link DefaultCspSettings}. + * + * @since Struts 6.5.0 */ public void setDefaultCspSettingsClassName(String defaultCspSettingsClassName) { this.defaultCspSettingsClassName = defaultCspSettingsClassName; diff --git a/core/src/main/java/org/apache/struts2/interceptor/csp/CspSettings.java b/core/src/main/java/org/apache/struts2/interceptor/csp/CspSettings.java index 3b2cadb96..a8c2a68c2 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/csp/CspSettings.java +++ b/core/src/main/java/org/apache/struts2/interceptor/csp/CspSettings.java @@ -59,6 +59,8 @@ public interface CspSettings { /** * Sets the report group where csp violation reports will be sent + * + * @since Struts 6.5.0 */ void setReportTo(String group);