You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by lu...@apache.org on 2026/07/27 10:19:14 UTC
(struts) 13/13: WW-5659 chore(core): harden the policy sets and tidy the lazy params dispatch
This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch WW-5659-lazy-params-request-scoping
in repository https://gitbox.apache.org/repos/asf/struts.git
commit ac4676db5daf7f453f993331b90dcc3e8c88846b
Author: Lukasz Lenart <lu...@apache.org>
AuthorDate: Mon Jul 27 11:59:41 2026 +0200
WW-5659 chore(core): harden the policy sets and tidy the lazy params dispatch
UploadPolicy handed out the mutable HashSet built by commaDelimitedStringToSet,
which the copy constructor shares by reference with the configured policy, so a
subclass overriding the protected acceptFile could have rewritten process-wide
config from a request thread. The sets are unmodifiable now.
Also document that unresolved() records `disabled` like any other param, so an
unresolvable disabled expression leaves the interceptor enabled and rejects
every upload; and in DefaultActionInvocation use normal imports for the params
types, make the interceptor local final, word the three skip logs consistently
and identify the interceptor by its mapping name, and note that the name-based
param merge is inherited behaviour whose duplicate-ref handling is questionable.
Co-Authored-By: Claude Opus 5 <no...@anthropic.com>
---
.../apache/struts2/DefaultActionInvocation.java | 38 +++++++++++++++++-----
.../apache/struts2/interceptor/UploadPolicy.java | 14 +++++++-
2 files changed, 43 insertions(+), 9 deletions(-)
diff --git a/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java b/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java
index 913590410..72a35afe5 100644
--- a/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java
+++ b/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java
@@ -30,7 +30,9 @@ import org.apache.struts2.config.entities.ResultConfig;
import org.apache.struts2.inject.Container;
import org.apache.struts2.inject.Inject;
import org.apache.struts2.interceptor.ConditionalInterceptor;
+import org.apache.struts2.interceptor.DisableParams;
import org.apache.struts2.interceptor.Interceptor;
+import org.apache.struts2.interceptor.InterceptorParams;
import org.apache.struts2.interceptor.PreResultListener;
import org.apache.struts2.interceptor.WithLazyParams;
import org.apache.struts2.ognl.OgnlUtil;
@@ -258,11 +260,11 @@ public class DefaultActionInvocation implements ActionInvocation {
if (asyncManager == null || !asyncManager.hasAsyncActionResult()) {
if (interceptors.hasNext()) {
final InterceptorMapping interceptorMapping = interceptors.next();
- Interceptor interceptor = interceptorMapping.getInterceptor();
+ final Interceptor interceptor = interceptorMapping.getInterceptor();
if (interceptor instanceof WithLazyParams<?> lazyInterceptor) {
resultCode = invokeWithLazyParams(lazyInterceptor, interceptorMapping);
} else if (interceptor instanceof ConditionalInterceptor conditionalInterceptor) {
- resultCode = executeConditional(conditionalInterceptor);
+ resultCode = executeConditional(conditionalInterceptor, interceptorMapping.getName());
} else {
LOG.debug("Executing normal interceptor: {}", interceptorMapping.getName());
resultCode = interceptor.intercept(this);
@@ -313,18 +315,18 @@ public class DefaultActionInvocation implements ActionInvocation {
* lazily resolved {@code disabled} flag and any custom {@code shouldIntercept} must be honoured
* here, because the single-argument {@code intercept} is not the entry point on this path.
*/
- private <P extends org.apache.struts2.interceptor.InterceptorParams> String invokeWithLazyParams(
+ private <P extends InterceptorParams> String invokeWithLazyParams(
WithLazyParams<P> lazyInterceptor, InterceptorMapping interceptorMapping) throws Exception {
P lazyParams = lazyParamInjector.resolveInto(
lazyInterceptor.newLazyParams(), mergedParams(interceptorMapping), invocationContext);
- if (lazyParams instanceof org.apache.struts2.interceptor.DisableParams disableParams && disableParams.isDisabled()) {
- LOG.debug("Interceptor: {} is disabled for this invocation, skipping to next", interceptorMapping.getName());
+ if (lazyParams instanceof DisableParams disableParams && disableParams.isDisabled()) {
+ LOG.debug("Interceptor: {} is disabled by its lazily resolved params, skipping to next", interceptorMapping.getName());
return this.invoke();
}
if (lazyInterceptor instanceof ConditionalInterceptor conditionalInterceptor
&& !conditionalInterceptor.shouldIntercept(this)) {
- LOG.debug("Interceptor: {} is disabled, skipping to next", interceptorMapping.getName());
+ LOG.debug("Interceptor: {} declined by shouldIntercept() on the lazy params path, skipping to next", interceptorMapping.getName());
return this.invoke();
}
LOG.debug("Executing lazy params interceptor: {}", interceptorMapping.getName());
@@ -332,6 +334,13 @@ public class DefaultActionInvocation implements ActionInvocation {
}
/**
+ * Merges the params declared on the interceptor-ref with those of the mapping being invoked.
+ * <p>
+ * The name-based lookup is inherited behaviour, kept as-is: the mapping is normally the very one
+ * found by name, so the merge is a no-op, and when a stack references the same interceptor name
+ * twice with different params it merges the first mapping's params over the current one, which
+ * is questionable. Changing it is out of scope here.
+ *
* @return a fresh map; the mapping's own param map is shared across requests and must not be mutated
*/
private Map<String, String> mergedParams(InterceptorMapping interceptorMapping) {
@@ -343,12 +352,25 @@ public class DefaultActionInvocation implements ActionInvocation {
return merged;
}
+ /**
+ * @deprecated since 7.3.0, use {@link #executeConditional(ConditionalInterceptor, String)} so the
+ * interceptor is identified by its mapping name in the logs.
+ */
+ @Deprecated
protected String executeConditional(ConditionalInterceptor conditionalInterceptor) throws Exception {
+ return executeConditional(conditionalInterceptor, conditionalInterceptor.getClass().getSimpleName());
+ }
+
+ /**
+ * @param interceptorName the name of the interceptor mapping being invoked, used for logging
+ * @since 7.3.0
+ */
+ protected String executeConditional(ConditionalInterceptor conditionalInterceptor, String interceptorName) throws Exception {
if (conditionalInterceptor.shouldIntercept(this)) {
- LOG.debug("Executing conditional interceptor: {}", conditionalInterceptor.getClass().getSimpleName());
+ LOG.debug("Executing conditional interceptor: {}", interceptorName);
return conditionalInterceptor.intercept(this);
} else {
- LOG.debug("Interceptor: {} is disabled, skipping to next", conditionalInterceptor.getClass().getSimpleName());
+ LOG.debug("Interceptor: {} declined by shouldIntercept(), skipping to next", interceptorName);
return this.invoke();
}
}
diff --git a/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java
index 08702dee4..5911a401e 100644
--- a/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java
+++ b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java
@@ -87,6 +87,13 @@ public class UploadPolicy extends DisableParams {
* A parameter that could not be resolved makes this policy unusable: the upload is rejected
* rather than validated against a partially-resolved policy, so a broken expression cannot
* silently relax validation.
+ * <p>
+ * <strong>Any</strong> param name is recorded, {@code disabled} included. An unresolvable
+ * {@code <param name="disabled">${...}</param>} therefore leaves the interceptor enabled (the
+ * flag keeps its configured value) <em>and</em> marks this policy unusable, so every upload in
+ * that invocation is rejected. That is deliberate — a broken dispatch flag is a broken
+ * configuration, and refusing the upload is the safe reading — but it means a typo in
+ * {@code disabled} takes out upload validation rather than only the disabling.
*/
@Override
public void unresolved(String paramName) {
@@ -108,9 +115,14 @@ public class UploadPolicy extends DisableParams {
return new UploadPolicy(this);
}
+ /**
+ * The resulting set is unmodifiable: the copy constructor shares it by reference with the
+ * configured policy, so every per-invocation policy that does not override the param would
+ * otherwise hand out a live handle on process-wide configuration.
+ */
private static Set<String> toSet(String commaDelimited) {
return commaDelimited == null
? Collections.emptySet()
- : TextParseUtil.commaDelimitedStringToSet(commaDelimited);
+ : Collections.unmodifiableSet(TextParseUtil.commaDelimitedStringToSet(commaDelimited));
}
}