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.
--
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]
