[
https://issues.apache.org/jira/browse/WW-5504?focusedWorklogId=982148&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-982148
]
ASF GitHub Bot logged work on WW-5504:
--------------------------------------
Author: ASF GitHub Bot
Created on: 07/Sep/25 07:47
Start Date: 07/Sep/25 07:47
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #1174:
URL: https://github.com/apache/struts/pull/1174#discussion_r2328560371
##########
core/src/main/java/org/apache/struts2/interceptor/csp/CspNonceReader.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.struts2.interceptor.csp;
+
+import org.apache.struts2.util.ValueStack;
+
+/**
+ * Reads the nonce value using the ValueStack, {@link StrutsCspNonceReader} is
the default implementation
+ * @since 6.8.0
+ */
+public interface CspNonceReader {
+
+ NonceValue readNonceValue(ValueStack stack);
+
+ class NonceValue {
+ private final String nonceValue;
+ private final CspNonceSource source;
+
+ private NonceValue(String nonceValue, CspNonceSource source) {
+ this.nonceValue = nonceValue;
+ this.source = source;
+ }
+
+ public static NonceValue ofSession(String nonceValue) {
+ return new NonceValue(nonceValue, CspNonceSource.SESSION);
+ }
+
+ public static NonceValue ofRequest(String nonceValue) {
+ return new NonceValue(nonceValue, CspNonceSource.REQUEST);
+ }
+
+ public static NonceValue ofNullSession() {
+ return new NonceValue(null, CspNonceSource.REQUEST);
Review Comment:
The `ofNullSession()` method incorrectly uses `CspNonceSource.REQUEST`
instead of `CspNonceSource.SESSION`. This should be `CspNonceSource.SESSION` to
properly indicate the source type.
```suggestion
return new NonceValue(null, CspNonceSource.SESSION);
```
##########
core/src/main/java/org/apache/struts2/interceptor/csp/CspNonceReader.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.struts2.interceptor.csp;
+
+import org.apache.struts2.util.ValueStack;
+
+/**
+ * Reads the nonce value using the ValueStack, {@link StrutsCspNonceReader} is
the default implementation
+ * @since 6.8.0
+ */
+public interface CspNonceReader {
+
+ NonceValue readNonceValue(ValueStack stack);
+
+ class NonceValue {
+ private final String nonceValue;
+ private final CspNonceSource source;
+
+ private NonceValue(String nonceValue, CspNonceSource source) {
+ this.nonceValue = nonceValue;
+ this.source = source;
+ }
+
+ public static NonceValue ofSession(String nonceValue) {
+ return new NonceValue(nonceValue, CspNonceSource.SESSION);
+ }
+
+ public static NonceValue ofRequest(String nonceValue) {
+ return new NonceValue(nonceValue, CspNonceSource.REQUEST);
+ }
+
+ public static NonceValue ofNullSession() {
+ return new NonceValue(null, CspNonceSource.REQUEST);
+ }
+
+ public static NonceValue ofNullRequest() {
+ return new NonceValue(null, CspNonceSource.REQUEST);
+ }
+
+ public boolean isNonceValueSet() {
+ return nonceValue != null;
+ }
+
+ public String getNonceValue() {
+ return nonceValue;
+ }
+
+ public CspNonceSource getSource() {
+ return source;
+ }
+
+ @Override
+ public String toString() {
+ return "NonceValue{" +
+ String.format("nonceValue='%s**********'",
nonceValue.substring(0, 4)) +
Review Comment:
The `toString()` method will throw a `NullPointerException` when
`nonceValue` is null, which can happen for null session/request cases. Add a
null check before calling `substring()`.
```suggestion
String displayNonce;
if (nonceValue != null && nonceValue.length() >= 4) {
displayNonce = String.format("nonceValue='%s**********'",
nonceValue.substring(0, 4));
} else if (nonceValue != null) {
displayNonce = String.format("nonceValue='%s**********'",
nonceValue);
} else {
displayNonce = "nonceValue='<null>'";
}
return "NonceValue{" +
displayNonce +
```
##########
core/src/test/java/org/apache/struts2/components/UIBeanTest.java:
##########
@@ -21,6 +21,8 @@
import com.opensymphony.xwork2.ActionContext;
import com.opensymphony.xwork2.config.ConfigurationException;
import com.opensymphony.xwork2.util.ValueStack;
+import net.bytebuddy.implementation.InvokeDynamic;
Review Comment:
The import `net.bytebuddy.implementation.InvokeDynamic` appears to be unused
in this file. This import should be removed to keep the code clean.
```suggestion
```
##########
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:
The error message uses '#createPolicyFormat(String)' which is not standard
Java documentation syntax. It should be 'createPolicyFormat(String)' or use
proper JavaDoc syntax like '{@link #createPolicyFormat(String)}'.
```suggestion
throw new UnsupportedOperationException("Unsupported implementation,
use createPolicyFormat(String) instead!");
```
Issue Time Tracking
-------------------
Worklog Id: (was: 982148)
Time Spent: 1h 10m (was: 1h)
> 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
> Priority: Major
> Fix For: 6.8.0, 7.1.0
>
> Time Spent: 1h 10m
> 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)
