You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@struts.apache.org by "eschulma (via GitHub)" <gi...@apache.org> on 2024/04/10 22:45:00 UTC
[PR] WW-5400 Extend default configuration options for the CSP interceptor. [struts]
eschulma opened a new pull request, #913:
URL: https://github.com/apache/struts/pull/913
Previously, it was impossible to set global options for the CSP interceptor. The only option was to have every action individually implement CspSettingsAware.
To fix this, we add an interceptor parameter of defaultCspSettingsClassName. Values from this class will be used in the CSP header instead of DefaultCspSettings. Users may define their own custom class which implements CspSettings, and that will be the default for all actions that do not implement the CspSettingsAware interface. It is now possible to create this custom class by simply extending DefaultCspSettings.
I have fixed a spelling error in DefaultCspSettings.java -- cratePolicyFormat renamed to createPolicyFormat.
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5400 Extend default configuration options for the CSP interceptor. [struts]
Posted by "eschulma (via GitHub)" <gi...@apache.org>.
eschulma commented on PR #913:
URL: https://github.com/apache/struts/pull/913#issuecomment-2049907568
@lukaszlenart submitted per your request
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5400 Extend default configuration options for the CSP interceptor. [struts]
Posted by "eschulma (via GitHub)" <gi...@apache.org>.
eschulma commented on PR #913:
URL: https://github.com/apache/struts/pull/913#issuecomment-2050535381
Ok all good.
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5400 Extend default configuration options for the CSP interceptor. [struts]
Posted by "eschulma (via GitHub)" <gi...@apache.org>.
eschulma commented on code in PR #913:
URL: https://github.com/apache/struts/pull/913#discussion_r1562942223
##########
core/src/main/java/org/apache/struts2/interceptor/csp/CspSettings.java:
##########
@@ -56,6 +57,11 @@ public interface CspSettings {
*/
void setReportUri(String uri);
+ /**
+ * Sets the report group where csp violation reports will be sent
+ */
Review Comment:
Done
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5400 Extend default configuration options for the CSP interceptor. [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart merged PR #913:
URL: https://github.com/apache/struts/pull/913
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5400 Extend default configuration options for the CSP interceptor. [struts]
Posted by "eschulma (via GitHub)" <gi...@apache.org>.
eschulma commented on code in PR #913:
URL: https://github.com/apache/struts/pull/913#discussion_r1562941786
##########
core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java:
##########
@@ -124,4 +153,11 @@ 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}.
+ */
Review Comment:
Done
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5400 Extend default configuration options for the CSP interceptor. [struts]
Posted by "eschulma (via GitHub)" <gi...@apache.org>.
eschulma commented on PR #913:
URL: https://github.com/apache/struts/pull/913#issuecomment-2050317405
Hold off a bit, I need to check something (this is what I get for implementing my own separate solution)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5400 Extend default configuration options for the CSP interceptor. [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #913:
URL: https://github.com/apache/struts/pull/913#discussion_r1582193457
##########
core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java:
##########
@@ -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);
Review Comment:
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
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5400 Extend default configuration options for the CSP interceptor. [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #913:
URL: https://github.com/apache/struts/pull/913#discussion_r1562033229
##########
core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java:
##########
@@ -124,4 +153,11 @@ 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}.
+ */
+ public void setDefaultCspSettingsClassName(String defaultCspSettingsClassName) {
+ this.defaultCspSettingsClassName = defaultCspSettingsClassName;
+ }
Review Comment:
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.
##########
core/src/main/java/org/apache/struts2/interceptor/csp/CspInterceptor.java:
##########
@@ -124,4 +153,11 @@ 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}.
+ */
Review Comment:
Please add `@since Struts 6.5.0` [annotation](https://www.oracle.com/pl/technical-resources/articles/java/javadoc-tool.html#@since)
##########
core/src/main/java/org/apache/struts2/interceptor/csp/CspSettings.java:
##########
@@ -56,6 +57,11 @@ public interface CspSettings {
*/
void setReportUri(String uri);
+ /**
+ * Sets the report group where csp violation reports will be sent
+ */
Review Comment:
Could you add [@since Struts 6.5.0](https://www.oracle.com/pl/technical-resources/articles/java/javadoc-tool.html#@since)?
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org