[
https://issues.apache.org/jira/browse/WW-5504?focusedWorklogId=983110&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-983110
]
ASF GitHub Bot logged work on WW-5504:
--------------------------------------
Author: ASF GitHub Bot
Created on: 15/Sep/25 05:33
Start Date: 15/Sep/25 05:33
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #1352:
URL: https://github.com/apache/struts/pull/1352#discussion_r2347914884
##########
plugins/javatemplates/src/test/java/org/apache/struts2/views/java/simple/PasswordTest.java:
##########
@@ -22,15 +22,14 @@
import org.apache.struts2.components.Password;
import org.apache.struts2.components.UIBean;
+import org.apache.struts2.interceptor.csp.CspNonceSource;
+import org.apache.struts2.interceptor.csp.StrutsCspNonceReader;
public class PasswordTest extends AbstractCommonAttributesTest {
private Password tag;
public void testRenderPassword() throws Exception {
- super.setUp();
- this.tag = new Password(stack, request, response);
-
tag.setName("name");
Review Comment:
The test methods `testRenderPassword` and `testRenderPasswordShowIt` are
calling `super.setUp()` and creating new `Password` instances inline within the
test methods, which is inconsistent with the pattern used in `setUp()` method.
The inline setup should be removed since `setUp()` is properly handling the
initialization.
##########
plugins/javatemplates/src/test/java/org/apache/struts2/views/java/simple/PasswordTest.java:
##########
@@ -85,8 +83,6 @@ protected void setUpStack() {
@Override
protected UIBean getUIBean() throws Exception {
- super.setUp();
- this.tag = new Password(stack, request, response);
return tag;
Review Comment:
The test methods `testRenderPassword` and `testRenderPasswordShowIt` are
calling `super.setUp()` and creating new `Password` instances inline within the
test methods, which is inconsistent with the pattern used in `setUp()` method.
The inline setup should be removed since `setUp()` is properly handling the
initialization.
##########
core/src/main/java/org/apache/struts2/interceptor/csp/DefaultCspSettings.java:
##########
@@ -43,63 +45,102 @@
*/
public class DefaultCspSettings implements CspSettings {
- private final static Logger LOG =
LogManager.getLogger(DefaultCspSettings.class);
+ private static final Logger LOG =
LogManager.getLogger(DefaultCspSettings.class);
+ private static final String NONCE_KEY = "nonce";
private final SecureRandom sRand = new SecureRandom();
+ private CspNonceSource nonceSource = CspNonceSource.SESSION;
+
protected String reportUri;
protected String reportTo;
// default to reporting mode
protected String cspHeader = CSP_REPORT_HEADER;
+ @Inject(value = StrutsConstants.STRUTS_CSP_NONCE_SOURCE, required = false)
+ public void setNonceSource(String nonceSource) {
+ if (StringUtils.isBlank(nonceSource)) {
+ this.nonceSource = CspNonceSource.SESSION;
+ } else {
+ this.nonceSource =
CspNonceSource.valueOf(nonceSource.toUpperCase());
+ }
+ }
+
@Override
public void addCspHeaders(HttpServletRequest request, HttpServletResponse
response) {
+ if (this.nonceSource == CspNonceSource.SESSION) {
+ addCspHeadersWithSession(request, response);
+ } else if (this.nonceSource == CspNonceSource.REQUEST) {
+ addCspHeadersWithRequest(request, response);
+ } else {
+ LOG.warn("Unknown nonce source: {}, ignoring CSP settings",
nonceSource);
+ }
+ }
+
+ private void addCspHeadersWithSession(HttpServletRequest request,
HttpServletResponse response) {
if (isSessionActive(request)) {
LOG.trace("Session is active, applying CSP settings");
- associateNonceWithSession(request);
- response.setHeader(cspHeader, createPolicyFormat(request));
+ String nonceValue = generateNonceValue();
+ request.getSession().setAttribute(NONCE_KEY, nonceValue);
+ response.setHeader(cspHeader, createPolicyFormat(nonceValue));
} else {
- LOG.trace("Session is not active, ignoring CSP settings");
+ LOG.debug("Session is not active, ignoring CSP settings");
}
}
+ private void addCspHeadersWithRequest(HttpServletRequest request,
HttpServletResponse response) {
+ String nonceValue = generateNonceValue();
+ request.setAttribute(NONCE_KEY, nonceValue);
+ response.setHeader(cspHeader, createPolicyFormat(nonceValue));
+ }
+
private boolean isSessionActive(HttpServletRequest request) {
return request.getSession(false) != null;
}
- private void associateNonceWithSession(HttpServletRequest request) {
- String nonceValue =
Base64.getUrlEncoder().encodeToString(getRandomBytes());
- request.getSession().setAttribute("nonce", nonceValue);
+ private String generateNonceValue() {
+ return Base64.getUrlEncoder().encodeToString(getRandomBytes());
}
- protected String createPolicyFormat(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));
+ protected String createPolicyFormat(String nonceValue) {
+ StringBuilder builder = new StringBuilder()
+ .append(OBJECT_SRC)
+ .append(format(" '%s'; ", NONE))
+ .append(SCRIPT_SRC)
+ .append(format(" 'nonce-%s' ", nonceValue))
+ .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));
- if(reportTo != null) {
- policyFormatBuilder
+ builder
+ .append(REPORT_URI)
+ .append(format(" %s; ", reportUri));
+ if (reportTo != null) {
+ builder
.append(REPORT_TO)
.append(format(" %s; ", reportTo));
}
}
- return format(policyFormatBuilder.toString(), getNonceString(request));
+ return builder.toString();
+ }
+
+ /**
+ * @deprecated since 6.8.0, for removal
+ */
+ @Deprecated
+ protected String createPolicyFormat(HttpServletRequest request) {
+ throw new UnsupportedOperationException("Unsupported implementation,
use createPolicyFormat(String) instead!");
}
+ /**
+ * @deprecated since 6.8.0, for removal
+ */
+ @Deprecated
protected String getNonceString(HttpServletRequest request) {
- Object nonce = request.getSession().getAttribute("nonce");
- return Objects.toString(nonce);
+ throw new UnsupportedOperationException("Unsupported implementation,
don't use!");
}
Review Comment:
Consider using `@Deprecated(since = \"6.8.0\", forRemoval = true)`
annotation on the deprecated method instead of just the comment. This provides
better IDE support and compile-time warnings.
##########
core/src/main/java/org/apache/struts2/interceptor/csp/DefaultCspSettings.java:
##########
@@ -43,63 +45,102 @@
*/
public class DefaultCspSettings implements CspSettings {
- private final static Logger LOG =
LogManager.getLogger(DefaultCspSettings.class);
+ private static final Logger LOG =
LogManager.getLogger(DefaultCspSettings.class);
+ private static final String NONCE_KEY = "nonce";
private final SecureRandom sRand = new SecureRandom();
+ private CspNonceSource nonceSource = CspNonceSource.SESSION;
+
protected String reportUri;
protected String reportTo;
// default to reporting mode
protected String cspHeader = CSP_REPORT_HEADER;
+ @Inject(value = StrutsConstants.STRUTS_CSP_NONCE_SOURCE, required = false)
+ public void setNonceSource(String nonceSource) {
+ if (StringUtils.isBlank(nonceSource)) {
+ this.nonceSource = CspNonceSource.SESSION;
+ } else {
+ this.nonceSource =
CspNonceSource.valueOf(nonceSource.toUpperCase());
+ }
+ }
+
@Override
public void addCspHeaders(HttpServletRequest request, HttpServletResponse
response) {
+ if (this.nonceSource == CspNonceSource.SESSION) {
+ addCspHeadersWithSession(request, response);
+ } else if (this.nonceSource == CspNonceSource.REQUEST) {
+ addCspHeadersWithRequest(request, response);
+ } else {
+ LOG.warn("Unknown nonce source: {}, ignoring CSP settings",
nonceSource);
+ }
+ }
+
+ private void addCspHeadersWithSession(HttpServletRequest request,
HttpServletResponse response) {
if (isSessionActive(request)) {
LOG.trace("Session is active, applying CSP settings");
- associateNonceWithSession(request);
- response.setHeader(cspHeader, createPolicyFormat(request));
+ String nonceValue = generateNonceValue();
+ request.getSession().setAttribute(NONCE_KEY, nonceValue);
+ response.setHeader(cspHeader, createPolicyFormat(nonceValue));
} else {
- LOG.trace("Session is not active, ignoring CSP settings");
+ LOG.debug("Session is not active, ignoring CSP settings");
}
}
+ private void addCspHeadersWithRequest(HttpServletRequest request,
HttpServletResponse response) {
+ String nonceValue = generateNonceValue();
+ request.setAttribute(NONCE_KEY, nonceValue);
+ response.setHeader(cspHeader, createPolicyFormat(nonceValue));
+ }
+
private boolean isSessionActive(HttpServletRequest request) {
return request.getSession(false) != null;
}
- private void associateNonceWithSession(HttpServletRequest request) {
- String nonceValue =
Base64.getUrlEncoder().encodeToString(getRandomBytes());
- request.getSession().setAttribute("nonce", nonceValue);
+ private String generateNonceValue() {
+ return Base64.getUrlEncoder().encodeToString(getRandomBytes());
}
- protected String createPolicyFormat(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));
+ protected String createPolicyFormat(String nonceValue) {
+ StringBuilder builder = new StringBuilder()
+ .append(OBJECT_SRC)
+ .append(format(" '%s'; ", NONE))
+ .append(SCRIPT_SRC)
+ .append(format(" 'nonce-%s' ", nonceValue))
+ .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));
- if(reportTo != null) {
- policyFormatBuilder
+ builder
+ .append(REPORT_URI)
+ .append(format(" %s; ", reportUri));
+ if (reportTo != null) {
+ builder
.append(REPORT_TO)
.append(format(" %s; ", reportTo));
}
}
- return format(policyFormatBuilder.toString(), getNonceString(request));
+ return builder.toString();
+ }
+
+ /**
+ * @deprecated since 6.8.0, for removal
+ */
+ @Deprecated
+ protected String createPolicyFormat(HttpServletRequest request) {
+ throw new UnsupportedOperationException("Unsupported implementation,
use createPolicyFormat(String) instead!");
}
Review Comment:
Consider using `@Deprecated(since = \"6.8.0\", forRemoval = true)`
annotation on the deprecated method instead of just the comment. This provides
better IDE support and compile-time warnings.
##########
plugins/javatemplates/src/test/java/org/apache/struts2/views/java/simple/PasswordTest.java:
##########
@@ -75,7 +71,9 @@ public void testRenderPasswordShowIt() throws Exception {
@Override
protected void setUp() throws Exception {
- //dont call base setup
+ super.setUp();
+ this.tag = new Password(stack, request, response);
+ this.tag.setCspNonceReader(new
StrutsCspNonceReader(CspNonceSource.SESSION.name()));
Review Comment:
The test methods `testRenderPassword` and `testRenderPasswordShowIt` are
calling `super.setUp()` and creating new `Password` instances inline within the
test methods, which is inconsistent with the pattern used in `setUp()` method.
The inline setup should be removed since `setUp()` is properly handling the
initialization.
Issue Time Tracking
-------------------
Worklog Id: (was: 983110)
Time Spent: 2h 40m (was: 2.5h)
> CSP Nonce changes within a page
> -------------------------------
>
> Key: WW-5504
> URL: https://issues.apache.org/jira/browse/WW-5504
> Project: Struts 2
> Issue Type: Bug
> Components: Core Interceptors
> Affects Versions: 6.7.0
> Reporter: Andreas Sachs
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 6.8.0, 7.1.0
>
> Time Spent: 2h 40m
> Remaining Estimate: 0h
>
> Sometimes the CSP nonce changes within a page.
>
> <script type="text/javascript" src="..." nonce="A"> </script>
> <script type="text/javascript" src="..." nonce="A"> </script>
> ...
> <script type="text/javascript" src="..." nonce="B"> </script>
>
> This happens if there are concurrent requests within the same session.
>
> Each request stores a new nonce in the session:
>
> DefaultCspSettings:
> request.getSession().setAttribute("nonce", nonceValue);
>
> If the first request is not finished, the second request will change the
> nonce of the first request.
>
>
>
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
