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!"); ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
