From 3a6ad5a5576f9a30957b5a93f06f950af5e616db Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 5 Jun 2024 07:37:36 +0200 Subject: [PATCH] WW-5400 Simplifies how CspSettings is created --- .../interceptor/csp/CspInterceptor.java | 45 ++++++++------- .../interceptor/CspInterceptorTest.java | 55 +++++++++++++------ 2 files changed, 61 insertions(+), 39 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 54d9eeab1c..49bc04d30f 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 @@ -19,7 +19,9 @@ package org.apache.struts2.interceptor.csp; import com.opensymphony.xwork2.ActionInvocation; +import com.opensymphony.xwork2.config.ConfigurationException; import com.opensymphony.xwork2.interceptor.AbstractInterceptor; +import com.opensymphony.xwork2.util.ClassLoaderUtil; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.action.CspSettingsAware; @@ -48,7 +50,7 @@ public final class CspInterceptor extends AbstractInterceptor { private String reportUri; private String reportTo; - private String defaultCspSettingsClassName = DefaultCspSettings.class.getName(); + private String cspSettingsClassName = DefaultCspSettings.class.getName(); @Override public String intercept(ActionInvocation invocation) throws Exception { @@ -57,26 +59,28 @@ public String intercept(ActionInvocation invocation) throws Exception { LOG.trace("Using CspSettings provided by the action: {}", action); applySettings(invocation, ((CspSettingsAware) action).getCspSettings()); } else { - LOG.trace("Using {} with action: {}", defaultCspSettingsClassName, action); + LOG.trace("Using {} with action: {}", cspSettingsClassName, action); + CspSettings cspSettings = createCspSettings(invocation); + applySettings(invocation, cspSettings); + } + return invocation.invoke(); + } - // 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."); - } + private CspSettings createCspSettings(ActionInvocation invocation) throws ClassNotFoundException { + Class cspSettingsClass; - // if defaultCspSettingsClassName does not implement CspSettings, throw an exception - if (!CspSettings.class.isAssignableFrom(Class.forName(defaultCspSettingsClassName))) { - throw new IllegalArgumentException("The defaultCspSettingsClassName must implement CspSettings."); - } + try { + cspSettingsClass = ClassLoaderUtil.loadClass(cspSettingsClassName, getClass()); + } catch (ClassNotFoundException e) { + throw new ConfigurationException(String.format("The class %s doesn't exist!", cspSettingsClassName)); + } - CspSettings cspSettings = (CspSettings) Class.forName(defaultCspSettingsClassName) - .getDeclaredConstructor().newInstance(); - applySettings(invocation, cspSettings); + if (!CspSettings.class.isAssignableFrom(Class.forName(cspSettingsClassName))) { + throw new ConfigurationException(String.format("The class %s doesn't implement %s!", + cspSettingsClassName, CspSettings.class.getName())); } - return invocation.invoke(); + + return (CspSettings) invocation.getInvocationContext().getContainer().inject(cspSettingsClass); } private void applySettings(ActionInvocation invocation, CspSettings cspSettings) { @@ -127,7 +131,6 @@ public void setReportUri(String reportUri) { * 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) { @@ -167,7 +170,7 @@ public void setPrependServletContext(boolean prependServletContext) { * * @since Struts 6.5.0 */ - public void setDefaultCspSettingsClassName(String defaultCspSettingsClassName) { - this.defaultCspSettingsClassName = defaultCspSettingsClassName; + public void setCspSettingsClassName(String cspSettingsClassName) { + this.cspSettingsClassName = cspSettingsClassName; } -} \ No newline at end of file +} 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 cd59c347da..221e725db2 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/CspInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/CspInterceptorTest.java @@ -19,6 +19,7 @@ package org.apache.struts2.interceptor; import com.opensymphony.xwork2.ActionContext; +import com.opensymphony.xwork2.config.ConfigurationException; import com.opensymphony.xwork2.mock.MockActionInvocation; import org.apache.logging.log4j.util.Strings; import org.apache.struts2.StrutsInternalTestCase; @@ -75,7 +76,7 @@ public void test_whenNonceAlreadySetInSession_andRequestReceived_thenNewNonceIsS public void testEnforcingCspHeadersSet() throws Exception { String reportUri = "/csp-reports"; - String reportTo = "csp-group"; + String reportTo = "csp-group"; boolean enforcingMode = true; interceptor.setReportUri(reportUri); interceptor.setReportTo(reportTo); @@ -92,7 +93,7 @@ public void testEnforcingCspHeadersSet() throws Exception { public void testReportingCspHeadersSet() throws Exception { String reportUri = "/csp-reports"; - String reportTo = "csp-group"; + String reportTo = "csp-group"; boolean enforcingMode = false; interceptor.setReportUri(reportUri); interceptor.setReportTo(reportTo); @@ -179,7 +180,7 @@ public void testNoPrependContext() throws Exception { checkHeader("/report-uri", enforcingMode); } - public void testInvalidDefaultCspSettingsClassName() throws Exception { + public void testNonExistingCspSettingsClassName() throws Exception { boolean enforcingMode = true; mai.setAction(new TestAction()); request.setContextPath("/app"); @@ -189,15 +190,15 @@ public void testInvalidDefaultCspSettingsClassName() throws Exception { interceptor.setPrependServletContext(false); try { - interceptor.setDefaultCspSettingsClassName("foo"); + interceptor.setCspSettingsClassName("foo"); interceptor.intercept(mai); - assert (false); - } catch (IllegalArgumentException e) { - assert (true); + fail("Expected exception"); + } catch (ConfigurationException e) { + assertEquals("The class foo doesn't exist!", e.getMessage()); } } - public void testCustomDefaultCspSettingsClassName() throws Exception { + public void testInvalidCspSettingsClassName() throws Exception { boolean enforcingMode = true; mai.setAction(new TestAction()); request.setContextPath("/app"); @@ -205,7 +206,25 @@ public void testCustomDefaultCspSettingsClassName() throws Exception { interceptor.setEnforcingMode(enforcingMode); interceptor.setReportUri("/report-uri"); interceptor.setPrependServletContext(false); - interceptor.setDefaultCspSettingsClassName(CustomDefaultCspSettings.class.getName()); + + try { + interceptor.setCspSettingsClassName(Integer.class.getName()); + interceptor.intercept(mai); + fail("Expected exception"); + } catch (ConfigurationException e) { + assertEquals("The class java.lang.Integer doesn't implement org.apache.struts2.interceptor.csp.CspSettings!", e.getMessage()); + } + } + + public void testCustomCspSettingsClassName() throws Exception { + boolean enforcingMode = true; + mai.setAction(new TestAction()); + request.setContextPath("/app"); + + interceptor.setEnforcingMode(enforcingMode); + interceptor.setReportUri("/report-uri"); + interceptor.setPrependServletContext(false); + interceptor.setCspSettingsClassName(CustomDefaultCspSettings.class.getName()); interceptor.intercept(mai); @@ -223,9 +242,9 @@ 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'; ", - CspSettings.OBJECT_SRC, CspSettings.NONE, - CspSettings.SCRIPT_SRC, session.getAttribute("nonce"), CspSettings.STRICT_DYNAMIC, CspSettings.HTTP, CspSettings.HTTPS, - CspSettings.BASE_URI, CspSettings.NONE + CspSettings.OBJECT_SRC, CspSettings.NONE, + CspSettings.SCRIPT_SRC, session.getAttribute("nonce"), CspSettings.STRICT_DYNAMIC, CspSettings.HTTP, CspSettings.HTTPS, + CspSettings.BASE_URI, CspSettings.NONE ); } else { if (Strings.isEmpty(reportTo)) { @@ -235,8 +254,7 @@ public void checkHeader(String reportUri, String reportTo, boolean enforcingMode CspSettings.BASE_URI, CspSettings.NONE, CspSettings.REPORT_URI, reportUri ); - } - else { + } 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, @@ -263,10 +281,11 @@ protected void setUp() throws Exception { super.setUp(); container.inject(interceptor); ActionContext context = ActionContext.getContext() - .withServletRequest(request) - .withServletResponse(response) - .withSession(new SessionMap(request)) - .bind(); + .withContainer(container) + .withServletRequest(request) + .withServletResponse(response) + .withSession(new SessionMap(request)) + .bind(); mai.setInvocationContext(context); session = request.getSession(); }