Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WW-5400 Extend default configuration options for the CSP interceptor. #913

Merged
merged 3 commits into from
May 11, 2024
Merged

WW-5400 Extend default configuration options for the CSP interceptor. #913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
758174c
WW-5400 Extend default configuration options for the CSP interceptor.
eschulma Apr 10, 2024
0151bde
WW-5400 Better toString formatting
eschulma Apr 11, 2024
6ac8b04
WW-5400 Added @since Struts 6.5.0 to new properties as requested
eschulma Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 49 additions & 3 deletions core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -54,8 +57,24 @@ 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 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);
Comment on lines +60 to +77
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can move this code into init() method of the interceptor as right now a new instance is created per each invocation

}
return invocation.invoke();
}
Expand All @@ -76,6 +95,12 @@ private void applySettings(ActionInvocation invocation, CspSettings cspSettings)
}

cspSettings.setReportUri(finalReportUri);

// apply reportTo if set
if (reportTo != null) {
LOG.trace("Applying: {} to reportTo", reportTo);
cspSettings.setReportTo(reportTo);
}
}

invocation.addPreResultListener((actionInvocation, resultCode) -> {
Expand All @@ -97,6 +122,18 @@ public void setReportUri(String reportUri) {
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;
}

private Optional<URI> buildUri(String reportUri) {
try {
return Optional.of(URI.create(reportUri));
Expand Down Expand Up @@ -124,4 +161,13 @@ public void setPrependServletContext(boolean prependServletContext) {
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}.
*
* @since Struts 6.5.0
*/
eschulma marked this conversation as resolved.
Show resolved Hide resolved
public void setDefaultCspSettingsClassName(String defaultCspSettingsClassName) {
this.defaultCspSettingsClassName = defaultCspSettingsClassName;
}
Comment on lines +170 to +172
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use Struts inject mechanism instead of using raw class and creating the instance by yourself. It's all about defining a <bean name="customCspSettings" class="..."/> and then annotating the setter with @Inject("customCspSettings").

I assume you never played with Struts @Inject, so let's leave it as is and I will change that in the next PR.

}
8 changes: 8 additions & 0 deletions core/src/main/java/org/apache/struts2/interceptor/csp/CspSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:";
Expand All @@ -56,6 +57,13 @@ public interface CspSettings {
*/
void setReportUri(String uri);

/**
* Sets the report group where csp violation reports will be sent
*
* @since Struts 6.5.0
*/
eschulma marked this conversation as resolved.
Show resolved Hide resolved
void setReportTo(String group);

/**
* Sets CSP headers in enforcing mode when true, and report-only when false
*/
Expand Down
34 changes: 27 additions & 7 deletions core/src/main/java/org/apache/struts2/interceptor/csp/DefaultCspSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

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;
Expand All @@ -31,7 +32,11 @@

/**
* Default implementation of {@link CspSettings}.
* The default policy implements strict CSP with a nonce based approach and follows the guide: <a href="https://csp.withgoogle.com/docs/index.html">https://csp.withgoogle.com/docs/index.html/</a>
* The default policy implements strict CSP with a nonce based approach and follows the guide:
* <a href="https://csp.withgoogle.com/docs/index.html">https://csp.withgoogle.com/docs/index.html/</a>
* 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
Expand All @@ -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");
}
Expand All @@ -70,7 +77,7 @@ private void associateNonceWithSession(HttpServletRequest request) {
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))
Expand All @@ -84,13 +91,18 @@ private String cratePolicyFormat(HttpServletRequest request) {
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);
}
Expand All @@ -101,20 +113,28 @@ private byte[] getRandomBytes() {
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 + '\'' +
'}';
}
Expand Down
83 changes: 75 additions & 8 deletions core/src/test/java/org/apache/struts2/interceptor/CspInterceptorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
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;
Expand Down Expand Up @@ -74,8 +75,10 @@ public void test_whenNonceAlreadySetInSession_andRequestReceived_thenNewNonceIsS

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");

Expand All @@ -84,21 +87,23 @@ public void testEnforcingCspHeadersSet() throws Exception {
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");

interceptor.intercept(mai);

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 {
Expand Down Expand Up @@ -174,7 +179,47 @@ public void testNoPrependContext() throws Exception {
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'; ",
Expand All @@ -183,12 +228,23 @@ public void checkHeader(String reportUri, boolean enforcingMode) {
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;
Expand Down Expand Up @@ -230,4 +286,15 @@ public CspSettings getCspSettings() {
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";
}
}
}