This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5215-csp-session in repository https://gitbox.apache.org/repos/asf/struts.git
commit 74d4e2371616f4e51fe9986f916271554080ddd1 Author: Lukasz Lenart <[email protected]> AuthorDate: Tue Aug 23 20:31:39 2022 +0200 WW-5215 Checks is session was already created before applying CSP settings --- .../struts2/interceptor/csp/CspInterceptor.java | 5 +- .../struts2/interceptor/csp/CspSettings.java | 9 +++ .../interceptor/csp/DefaultCspSettings.java | 83 +++++++++++--------- .../struts2/interceptor/CspInterceptorTest.java | 91 ++++++++++++---------- 4 files changed, 112 insertions(+), 76 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 250179636..ca77436cc 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 @@ -23,6 +23,7 @@ import com.opensymphony.xwork2.interceptor.AbstractInterceptor; import com.opensymphony.xwork2.interceptor.PreResultListener; import java.net.URI; import java.util.Optional; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; /** @@ -36,6 +37,7 @@ import javax.servlet.http.HttpServletResponse; * @see DefaultCspSettings **/ public final class CspInterceptor extends AbstractInterceptor implements PreResultListener { + private final CspSettings settings = new DefaultCspSettings(); @Override @@ -45,8 +47,9 @@ public final class CspInterceptor extends AbstractInterceptor implements PreResu } public void beforeResult(ActionInvocation invocation, String resultCode) { + HttpServletRequest request = invocation.getInvocationContext().getServletRequest(); HttpServletResponse response = invocation.getInvocationContext().getServletResponse(); - settings.addCspHeaders(response); + settings.addCspHeaders(request, response); } public void setReportUri(String reportUri) { 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 9699ab291..adf5b5072 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 @@ -18,6 +18,7 @@ */ package org.apache.struts2.interceptor.csp; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; /** @@ -42,9 +43,17 @@ public interface CspSettings { String HTTPS = "https:"; String CSP_REPORT_TYPE = "application/csp-report"; + /** + * @deprecated use {@link #addCspHeaders(HttpServletRequest, HttpServletResponse)} instead + */ + @Deprecated void addCspHeaders(HttpServletResponse response); + + void addCspHeaders(HttpServletRequest request, HttpServletResponse response); + // sets the uri where csp violation reports will be sent void setReportUri(String uri); + // sets CSP headers in enforcing mode when true, and report-only when false void setEnforcingMode(boolean value); } 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 5a99c0a5b..7ab70d226 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 @@ -18,13 +18,14 @@ */ package org.apache.struts2.interceptor.csp; -import com.opensymphony.xwork2.ActionContext; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import java.security.SecureRandom; import java.util.Base64; -import java.util.Map; -import java.util.function.Supplier; +import java.util.Objects; import static java.lang.String.format; @@ -37,50 +38,61 @@ import static java.lang.String.format; */ public class DefaultCspSettings implements CspSettings { - private final SecureRandom sRand = new SecureRandom(); + private final static Logger LOG = LogManager.getLogger(DefaultCspSettings.class); - // this supplier computes a policy format - private final Supplier<String> lazyPolicyBuilder = new Supplier<String>() { - @Override - public String get() { - StringBuilder policyFormatBuilder = new StringBuilder() - .append(OBJECT_SRC) - .append(format(" '%s'; ", NONE)) - .append(SCRIPT_SRC) - .append(" 'nonce-%s' ") // nonce placeholder - .append(format("'%s' ", STRICT_DYNAMIC)) - .append(format("%s %s; ", HTTP, HTTPS)) - .append(BASE_URI) - .append(format(" '%s'; ", NONE)); - - if (reportUri != null) { - policyFormatBuilder - .append(REPORT_URI) - .append(format(" %s", reportUri)); - } - - return format(policyFormatBuilder.toString(), getNonceString()); - } - }; + private final SecureRandom sRand = new SecureRandom(); private String reportUri; // default to reporting mode private String cspHeader = CSP_REPORT_HEADER; + @Override public void addCspHeaders(HttpServletResponse response) { - associateNonceWithSession(); - response.setHeader(cspHeader, lazyPolicyBuilder.get()); + throw new UnsupportedOperationException("Unsupported implementation, use #addCspHeaders(HttpServletRequest request, HttpServletResponse response)"); } - private String getNonceString() { - Map<String, Object> session = ActionContext.getContext().getSession(); - return (String) session.get("nonce"); + public void addCspHeaders(HttpServletRequest request, HttpServletResponse response) { + if (isSessionActive(request)) { + LOG.debug("Session is active, applying CSP settings"); + associateNonceWithSession(request); + response.setHeader(cspHeader, cratePolicyFormat(request)); + } else { + LOG.debug("Session is not active, ignoring CSP settings"); + } } - private void associateNonceWithSession() { - Map<String, Object> session = ActionContext.getContext().getSession(); + private boolean isSessionActive(HttpServletRequest request) { + return request.getSession(false) != null; + } + + private void associateNonceWithSession(HttpServletRequest request) { String nonceValue = Base64.getUrlEncoder().encodeToString(getRandomBytes()); - session.put("nonce", nonceValue); + request.getSession().setAttribute("nonce", nonceValue); + } + + private String cratePolicyFormat(HttpServletRequest request) { + StringBuilder policyFormatBuilder = new StringBuilder() + .append(OBJECT_SRC) + .append(format(" '%s'; ", NONE)) + .append(SCRIPT_SRC) + .append(" 'nonce-%s' ") // nonce placeholder + .append(format("'%s' ", STRICT_DYNAMIC)) + .append(format("%s %s; ", HTTP, HTTPS)) + .append(BASE_URI) + .append(format(" '%s'; ", NONE)); + + if (reportUri != null) { + policyFormatBuilder + .append(REPORT_URI) + .append(format(" %s", reportUri)); + } + + return format(policyFormatBuilder.toString(), getNonceString(request)); + } + + private String getNonceString(HttpServletRequest request) { + Object nonce = request.getSession().getAttribute("nonce"); + return Objects.toString(nonce); } private byte[] getRandomBytes() { @@ -98,4 +110,5 @@ public class DefaultCspSettings implements CspSettings { public void setReportUri(String reportUri) { this.reportUri = reportUri; } + } 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 504be8bb4..a9ee1be11 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/CspInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/CspInterceptorTest.java @@ -21,16 +21,25 @@ package org.apache.struts2.interceptor; import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.mock.MockActionInvocation; import org.apache.logging.log4j.util.Strings; -import org.apache.struts2.ServletActionContext; import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.dispatcher.SessionMap; import org.apache.struts2.interceptor.csp.CspInterceptor; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; -import java.util.HashMap; -import java.util.Map; +import javax.servlet.http.HttpSession; -import static org.apache.struts2.interceptor.csp.CspSettings.*; +import static org.apache.struts2.interceptor.csp.CspSettings.BASE_URI; +import static org.apache.struts2.interceptor.csp.CspSettings.CSP_ENFORCE_HEADER; +import static org.apache.struts2.interceptor.csp.CspSettings.CSP_REPORT_HEADER; +import static org.apache.struts2.interceptor.csp.CspSettings.HTTP; +import static org.apache.struts2.interceptor.csp.CspSettings.HTTPS; +import static org.apache.struts2.interceptor.csp.CspSettings.NONE; +import static org.apache.struts2.interceptor.csp.CspSettings.OBJECT_SRC; +import static org.apache.struts2.interceptor.csp.CspSettings.REPORT_URI; +import static org.apache.struts2.interceptor.csp.CspSettings.SCRIPT_SRC; +import static org.apache.struts2.interceptor.csp.CspSettings.STRICT_DYNAMIC; +import static org.junit.Assert.assertNotEquals; public class CspInterceptorTest extends StrutsInternalTestCase { @@ -38,7 +47,8 @@ public class CspInterceptorTest extends StrutsInternalTestCase { private final MockActionInvocation mai = new MockActionInvocation(); private final MockHttpServletRequest request = new MockHttpServletRequest(); private final MockHttpServletResponse response = new MockHttpServletResponse(); - private final Map<String, Object> session = new HashMap<>(); + + private HttpSession session; public void test_whenRequestReceived_thenNonceIsSetInSession_andCspHeaderContainsIt() throws Exception { String reportUri = "/barfoo"; @@ -48,8 +58,8 @@ public class CspInterceptorTest extends StrutsInternalTestCase { interceptor.intercept(mai); - assertTrue("Nonce key does not exist", session.containsKey("nonce")); - assertFalse("Nonce value is empty", Strings.isEmpty((String) session.get("nonce"))); + assertNotNull("Nonce key does not exist", session.getAttribute("nonce")); + assertFalse("Nonce value is empty", Strings.isEmpty((String) session.getAttribute("nonce"))); checkHeader(reportUri, reporting); } @@ -58,13 +68,13 @@ public class CspInterceptorTest extends StrutsInternalTestCase { String enforcingMode = "true"; interceptor.setReportUri(reportUri); interceptor.setEnforcingMode(enforcingMode); - session.put("nonce", "foo"); + session.setAttribute("nonce", "foo"); interceptor.intercept(mai); - assertTrue("Nonce key does not exist", session.containsKey("nonce")); - assertFalse("Nonce value is empty", Strings.isEmpty((String) session.get("nonce"))); - assertFalse("New nonce value couldn't be set", session.get("nonce").equals("foo")); + 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); } @@ -73,13 +83,13 @@ public class CspInterceptorTest extends StrutsInternalTestCase { String enforcingMode = "true"; interceptor.setReportUri(reportUri); interceptor.setEnforcingMode(enforcingMode); - session.put("nonce", "foo"); + session.setAttribute("nonce", "foo"); interceptor.intercept(mai); - assertTrue("Nonce key does not exist", session.containsKey("nonce")); - assertFalse("Nonce value is empty", Strings.isEmpty((String) session.get("nonce"))); - assertFalse("New nonce value couldn't be set", session.get("nonce").equals("foo")); + 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); } @@ -88,13 +98,12 @@ public class CspInterceptorTest extends StrutsInternalTestCase { String enforcingMode = "false"; interceptor.setReportUri(reportUri); interceptor.setEnforcingMode(enforcingMode); - session.put("nonce", "foo"); + session.setAttribute("nonce", "foo"); interceptor.intercept(mai); - assertTrue("Nonce key does not exist", session.containsKey("nonce")); - assertFalse("Nonce value is empty", Strings.isEmpty((String) session.get("nonce"))); - assertFalse("New nonce value couldn't be set", session.get("nonce").equals("foo")); + assertNotNull("Nonce value is empty", session.getAttribute("nonce")); + assertNotEquals("New nonce value couldn't be set", "foo", session.getAttribute("nonce")); checkHeader(reportUri, enforcingMode); } @@ -116,11 +125,11 @@ public class CspInterceptorTest extends StrutsInternalTestCase { String enforcingMode = "false"; interceptor.setEnforcingMode(enforcingMode); - try{ + try { interceptor.setReportUri("ww w. google.@com"); - assert(false); - } catch (IllegalArgumentException e){ - assert(true); + assert (false); + } catch (IllegalArgumentException e) { + assert (true); } } @@ -128,33 +137,33 @@ public class CspInterceptorTest extends StrutsInternalTestCase { String enforcingMode = "false"; interceptor.setEnforcingMode(enforcingMode); - try{ + try { interceptor.setReportUri("some-uri"); - assert(false); - } catch (IllegalArgumentException e){ - assert(true); + assert (false); + } catch (IllegalArgumentException e) { + assert (true); } } - public void checkHeader(String reportUri, String enforcingMode){ + public void checkHeader(String reportUri, String enforcingMode) { String expectedCspHeader = ""; if (Strings.isEmpty(reportUri)) { expectedCspHeader = String.format("%s '%s'; %s 'nonce-%s' '%s' %s %s; %s '%s'; ", - OBJECT_SRC, NONE, - SCRIPT_SRC, session.get("nonce"), STRICT_DYNAMIC, HTTP, HTTPS, - BASE_URI, NONE + OBJECT_SRC, NONE, + SCRIPT_SRC, session.getAttribute("nonce"), STRICT_DYNAMIC, HTTP, HTTPS, + BASE_URI, NONE ); } else { expectedCspHeader = String.format("%s '%s'; %s 'nonce-%s' '%s' %s %s; %s '%s'; %s %s", - OBJECT_SRC, NONE, - SCRIPT_SRC, session.get("nonce"), STRICT_DYNAMIC, HTTP, HTTPS, - BASE_URI, NONE, - REPORT_URI, reportUri + OBJECT_SRC, NONE, + SCRIPT_SRC, session.getAttribute("nonce"), STRICT_DYNAMIC, HTTP, HTTPS, + BASE_URI, NONE, + REPORT_URI, reportUri ); } String header = ""; - if (enforcingMode.equals("true")){ + if (enforcingMode.equals("true")) { header = response.getHeader(CSP_ENFORCE_HEADER); } else { header = response.getHeader(CSP_REPORT_HEADER); @@ -168,10 +177,12 @@ public class CspInterceptorTest extends StrutsInternalTestCase { protected void setUp() throws Exception { super.setUp(); container.inject(interceptor); - ServletActionContext.setRequest(request); - ServletActionContext.setResponse(response); - ActionContext context = ServletActionContext.getActionContext().bind(); - context.withSession(session); + ActionContext context = ActionContext.getContext() + .withServletRequest(request) + .withServletResponse(response) + .withSession(new SessionMap<>(request)) + .bind(); mai.setInvocationContext(context); + session = request.getSession(); } }
