diff --git a/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java b/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java index 6885dd50d1..0a671648a5 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.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; @@ -41,6 +43,7 @@ import java.util.ArrayList; import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.concurrent.Callable; @@ -257,19 +260,11 @@ public String invoke() throws Exception { if (asyncManager == null || !asyncManager.hasAsyncActionResult()) { if (interceptors.hasNext()) { final InterceptorMapping interceptorMapping = interceptors.next(); - Interceptor interceptor = interceptorMapping.getInterceptor(); - if (interceptor instanceof WithLazyParams) { - Map params = interceptorMapping.getParams(); - - proxy.getConfig().getInterceptors().stream() - .filter(im -> im.getName().equals(interceptorMapping.getName())) - .findFirst() - .ifPresent(im -> params.putAll(im.getParams())); - - interceptor = lazyParamInjector.injectParams(interceptor, params, invocationContext); - } - if (interceptor instanceof ConditionalInterceptor conditionalInterceptor) { - resultCode = executeConditional(conditionalInterceptor); + final Interceptor interceptor = interceptorMapping.getInterceptor(); + if (interceptor instanceof WithLazyParams lazyInterceptor) { + resultCode = invokeWithLazyParams(lazyInterceptor, interceptorMapping); + } else if (interceptor instanceof ConditionalInterceptor conditionalInterceptor) { + resultCode = executeConditional(conditionalInterceptor, interceptorMapping.getName()); } else { LOG.debug("Executing normal interceptor: {}", interceptorMapping.getName()); resultCode = interceptor.intercept(this); @@ -312,12 +307,66 @@ public String invoke() throws Exception { return resultCode; } - protected String executeConditional(ConditionalInterceptor conditionalInterceptor) throws Exception { + /** + * Resolves lazy params into a per-invocation holder and dispatches to the interceptor. + *

+ * {@link org.apache.struts2.interceptor.AbstractInterceptor} implements + * {@link ConditionalInterceptor}, so a lazy interceptor is normally conditional too; both the + * 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

String invokeWithLazyParams( + WithLazyParams

lazyInterceptor, InterceptorMapping interceptorMapping) throws Exception { + P lazyParams = lazyParamInjector.resolveInto( + lazyInterceptor.newLazyParams(), mergedParams(interceptorMapping), invocationContext); + + 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: {} declined by shouldIntercept() on the lazy params path, skipping to next", interceptorMapping.getName()); + return this.invoke(); + } + LOG.debug("Executing lazy params interceptor: {}", interceptorMapping.getName()); + return lazyInterceptor.intercept(this, lazyParams); + } + + /** + * Merges the params declared on the interceptor-ref with those of the mapping being invoked. + *

+ * 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 preserving the configuration order, so params are applied to the holder + * deterministically; the mapping's own param map is shared across requests and must not be mutated + */ + private Map mergedParams(InterceptorMapping interceptorMapping) { + Map merged = new LinkedHashMap<>(interceptorMapping.getParams()); + proxy.getConfig().getInterceptors().stream() + .filter(im -> im.getName().equals(interceptorMapping.getName())) + .findFirst() + .ifPresent(im -> merged.putAll(im.getParams())); + return merged; + } + + /** + * Replaces the single-argument form removed in 7.3.0. That one had no callers left once the + * mapping name became available here, so a subclass still overriding it would have gone quietly + * dead; removing it turns that into a compile error instead. + * + * @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/factory/DefaultInterceptorFactory.java b/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java index cd5bc09bd2..38e0ac741b 100644 --- a/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java +++ b/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java @@ -22,14 +22,24 @@ import org.apache.logging.log4j.Logger; import org.apache.struts2.ObjectFactory; import org.apache.struts2.config.ConfigurationException; +import org.apache.struts2.config.ConfigurationUtil; import org.apache.struts2.config.entities.InterceptorConfig; import org.apache.struts2.inject.Inject; import org.apache.struts2.interceptor.Interceptor; +import org.apache.struts2.interceptor.InterceptorParams; import org.apache.struts2.interceptor.WithLazyParams; +import org.apache.struts2.ognl.ProviderAllowlist; +import org.apache.struts2.util.reflection.ReflectionException; import org.apache.struts2.util.reflection.ReflectionProvider; +import java.beans.IntrospectionException; +import java.lang.reflect.Field; +import java.lang.reflect.Modifier; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; /** * Default implementation @@ -40,6 +50,7 @@ public class DefaultInterceptorFactory implements InterceptorFactory { private ObjectFactory objectFactory; private ReflectionProvider reflectionProvider; + private ProviderAllowlist providerAllowlist; @Inject public void setObjectFactory(ObjectFactory objectFactory) { @@ -51,6 +62,11 @@ public void setReflectionProvider(ReflectionProvider reflectionProvider) { this.reflectionProvider = reflectionProvider; } + @Inject + public void setProviderAllowlist(ProviderAllowlist providerAllowlist) { + this.providerAllowlist = providerAllowlist; + } + public Interceptor buildInterceptor(InterceptorConfig interceptorConfig, Map interceptorRefParams) throws ConfigurationException { String interceptorClassName = interceptorConfig.getClassName(); Map thisInterceptorClassParams = interceptorConfig.getParams(); @@ -68,14 +84,28 @@ public Interceptor buildInterceptor(InterceptorConfig interceptorConfig, Map ? withoutLazyExpressions(params) : params, + interceptor); + + interceptor.init(); + + if (interceptor instanceof WithLazyParams lazyInterceptor) { LOG.debug("Interceptor {} implements {} - expression parameters will be re-evaluated during action invocation", interceptorClassName, WithLazyParams.class.getName()); + InterceptorParams lazyParams = lazyInterceptor.newLazyParams(); + validateLazyParamNames(interceptorConfig, lazyParams, interceptorRefParams); + allowlistLazyParamsHolder(lazyInterceptor, lazyParams); } - interceptor.init(); return interceptor; + } catch (ConfigurationException e) { + // already carries its own message and location, don't bury it in a generic wrapper + throw e; } catch (InstantiationException e) { cause = e; message = "Unable to instantiate an instance of Interceptor class [" + interceptorClassName + "]."; @@ -96,4 +126,123 @@ public Interceptor buildInterceptor(InterceptorConfig interceptorConfig, Map + * Only the interceptor-ref params are checked, and that is exactly the set that reaches + * the holder. {@link org.apache.struts2.config.providers.InterceptorBuilder} passes the + * very map it hands to this factory on to the {@code InterceptorMapping} it creates, and + * {@code DefaultActionInvocation.mergedParams} feeds that map to + * {@link WithLazyParams.LazyParamInjector#resolveInto}. The params declared on the + * {@code } definition are deliberately not checked: they never reach the mapping, + * they are only applied to the interceptor instance a few lines above, and requiring them to + * exist on the holder too would reject working configurations - {@code } + * on an interceptor definition, for instance, is honoured through + * {@link org.apache.struts2.interceptor.AbstractInterceptor#shouldIntercept} alone. + *

+ * Without this check an unknown name is only noticed per request, where + * {@code resolveInto} logs a WARN and notifies the holder - which for + * {@link org.apache.struts2.interceptor.UploadPolicy} means rejecting every upload of every + * request. The names are fully known at configuration-parse time, so a typo belongs to startup. + * The runtime handling stays in place as defence in depth for holders built outside this factory. + *

+ * A param whose name is a compound OGNL expression (it contains a {@code .}) is not checked: + * only its leading segment would have to be readable rather than writable on the holder, which + * this simple property check cannot decide. Such names are left to the runtime path. + */ + private void validateLazyParamNames(InterceptorConfig interceptorConfig, InterceptorParams lazyParams, Map interceptorRefParams) { + if (lazyParams == null || interceptorRefParams == null || interceptorRefParams.isEmpty()) { + return; + } + Class holderClass = lazyParams.getClass(); + for (String paramName : interceptorRefParams.keySet()) { + if (paramName == null || paramName.indexOf('.') >= 0) { + LOG.debug("Skipping configuration-time check of compound lazy param name [{}] on holder [{}]", + paramName, holderClass.getName()); + continue; + } + if (!isWritableOnHolder(holderClass, paramName)) { + throw new ConfigurationException(String.format( + "Param [%s] of interceptor [%s] (%s) is not a writable property of its lazy params holder [%s]." + + " Params of a %s interceptor-ref are resolved onto that holder for each invocation," + + " so this one could never be applied - check the interceptor configuration for a typo.", + paramName, interceptorConfig.getName(), interceptorConfig.getClassName(), + holderClass.getName(), WithLazyParams.class.getSimpleName()), interceptorConfig); + } + } + } + + /** + * @return true when OGNL could write {@code paramName} on the holder, i.e. there is a setter for + * it anywhere in the holder's hierarchy, or failing that a public field of that name + */ + private boolean isWritableOnHolder(Class holderClass, String paramName) { + try { + if (reflectionProvider.getSetMethod(holderClass, paramName) != null) { + return true; + } + } catch (IntrospectionException | ReflectionException e) { + LOG.debug("Could not introspect setter [{}] on lazy params holder [{}], falling back to field lookup", + paramName, holderClass.getName(), e); + } + Field field = reflectionProvider.getField(holderClass, paramName); + return field != null && Modifier.isPublic(field.getModifiers()); + } + + /** + * Allowlists the params holder of a {@link WithLazyParams} interceptor for OGNL member access. + *

+ * {@link WithLazyParams.LazyParamInjector#resolveInto} writes the resolved {@code ${...}} values + * onto the holder with OGNL. With {@code struts.allowlist.enable=true} (the default) that write + * is refused unless the holder's class is allowlisted, and the fail-closed handling in + * {@code resolveInto} would then mark every lazy param unresolved. The interceptor itself is + * allowlisted by {@code XmlDocConfigurationProvider.allowAndLoadClass} when its config is read; + * the holder is never named in any configuration, so it has to be registered here. + *

+ * Only the holder's own class hierarchy is registered - its class, its superclasses and the + * interfaces it implements - because the setter being written may be declared on any of them + * ({@code disabled} for instance is declared on {@link org.apache.struts2.interceptor.DisableParams}), + * and {@code SecurityMemberAccess} requires both the target class and the declaring class of the + * member to be allowlisted. {@link Object} is deliberately dropped from that set: it is a + * universal supertype that would say nothing about this holder, and it is excluded by default + * anyway ({@code struts.excludedClasses}), so registering it could only ever mislead a reader. + * No package is allowlisted and nothing beyond the hierarchy is added. + *

+ * The holder class is used as the registration key so repeated registrations - the factory is a + * prototype and several interceptors may share a holder type - collapse onto one entry. + */ + private void allowlistLazyParamsHolder(WithLazyParams lazyInterceptor, InterceptorParams lazyParams) { + if (providerAllowlist == null) { + LOG.warn("No ProviderAllowlist available, cannot allowlist the lazy params holder of [{}];" + + " lazy params will fail to resolve if the OGNL allowlist is enabled", lazyInterceptor.getClass().getName()); + return; + } + if (lazyParams == null) { + LOG.warn("Interceptor [{}] returned no lazy params holder, nothing to allowlist", lazyInterceptor.getClass().getName()); + return; + } + Class holderClass = lazyParams.getClass(); + Set> holderTypes = ConfigurationUtil.getAllClassTypes(holderClass).stream() + .filter(type -> type != Object.class) + .collect(Collectors.toSet()); + LOG.debug("Allowlisting lazy params holder [{}] and its supertypes {}", holderClass.getName(), holderTypes); + providerAllowlist.registerAllowlist(holderClass, holderTypes); + } + + /** + * Drops the params carrying a {@code ${...}} expression, which {@link WithLazyParams} resolves per + * invocation into its params holder instead. + *

+ * The predicate matches {@code WithLazyParams.LazyParamInjector}'s, so a value treated as an + * expression at resolution time is the same one withheld here. + * + * @return the params to apply to the interceptor at configuration time + */ + private static Map withoutLazyExpressions(Map params) { + return params.entrySet().stream() + .filter(entry -> entry.getValue() == null || !entry.getValue().contains("${")) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, (first, second) -> second, LinkedHashMap::new)); + } + } diff --git a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java index c7cbb65c8d..dac450b450 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java @@ -24,7 +24,6 @@ import org.apache.struts2.text.TextProviderFactory; import org.apache.struts2.inject.Container; import org.apache.struts2.inject.Inject; -import org.apache.struts2.util.TextParseUtil; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.dispatcher.LocalizedMessage; @@ -35,7 +34,6 @@ import java.text.NumberFormat; import java.util.Arrays; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Set; @@ -58,10 +56,9 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor public static final String STRUTS_MESSAGES_INVALID_CONTENT_TYPE_KEY = "struts.messages.invalid.content.type"; public static final String STRUTS_MESSAGES_ERROR_CONTENT_TYPE_NOT_ALLOWED_KEY = "struts.messages.error.content.type.not.allowed"; public static final String STRUTS_MESSAGES_ERROR_FILE_EXTENSION_NOT_ALLOWED_KEY = "struts.messages.error.file.extension.not.allowed"; + public static final String STRUTS_MESSAGES_ERROR_UPLOAD_POLICY_UNRESOLVED_KEY = "struts.messages.error.upload.policy.unresolved"; - private Long maximumSize; - private Set allowedTypesSet = Collections.emptySet(); - private Set allowedExtensionsSet = Collections.emptySet(); + private final UploadPolicy configuredPolicy = new UploadPolicy(); private ContentTypeMatcher matcher; private Container container; @@ -77,35 +74,48 @@ public void setContainer(Container container) { } /** - * Sets the allowed extensions + * Sets the allowed extensions. Applied at configuration time only; the effective policy for + * an invocation is a copy, see {@link ActionFileUploadInterceptor#newLazyParams()}. * * @param allowedExtensions A comma-delimited list of extensions */ public void setAllowedExtensions(String allowedExtensions) { - allowedExtensionsSet = TextParseUtil.commaDelimitedStringToSet(allowedExtensions); + configuredPolicy.setAllowedExtensions(allowedExtensions); } /** - * Sets the allowed mimetypes + * Sets the allowed mimetypes. Applied at configuration time only; the effective policy for + * an invocation is a copy, see {@link ActionFileUploadInterceptor#newLazyParams()}. * * @param allowedTypes A comma-delimited list of types */ public void setAllowedTypes(String allowedTypes) { - allowedTypesSet = TextParseUtil.commaDelimitedStringToSet(allowedTypes); + configuredPolicy.setAllowedTypes(allowedTypes); } /** - * Sets the maximum size of an uploaded file + * Sets the maximum size of an uploaded file. Applied at configuration time only; the + * effective policy for an invocation is a copy, see + * {@link ActionFileUploadInterceptor#newLazyParams()}. * * @param maximumSize The maximum size in bytes */ public void setMaximumSize(Long maximumSize) { - this.maximumSize = maximumSize; + configuredPolicy.setMaximumSize(maximumSize); + } + + /** + * @return an independent copy of the configured policy, to be resolved for one invocation + * @since 7.3.0 + */ + protected UploadPolicy copyConfiguredPolicy() { + return configuredPolicy.copy(); } /** * Override for added functionality. Checks if the proposed file is acceptable based on contentType and size. * + * @param policy - the effective upload policy for this invocation. * @param action - uploading action for message retrieval. * @param file - proposed upload file. * @param originalFilename - name of the file. @@ -113,7 +123,7 @@ public void setMaximumSize(Long maximumSize) { * @param inputName - inputName of the file. * @return true if the proposed file is acceptable by contentType and size. */ - protected boolean acceptFile(Object action, UploadedFile file, String originalFilename, String contentType, String inputName) { + protected boolean acceptFile(UploadPolicy policy, Object action, UploadedFile file, String originalFilename, String contentType, String inputName) { Set errorMessages = new HashSet<>(); ValidationAware validation = null; @@ -131,21 +141,32 @@ protected boolean acceptFile(Object action, UploadedFile file, String originalFi return false; } - if (maximumSize != null && maximumSize < file.length()) { + if (policy.isUnresolved()) { + String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_UPLOAD_POLICY_UNRESOLVED_KEY, new String[]{ + inputName, originalFilename, String.join(", ", policy.getUnresolvedParams()) + }); + if (validation != null) { + validation.addFieldError(inputName, errMsg); + } + LOG.warn(errMsg); + return false; + } + + if (policy.getMaximumSize() != null && policy.getMaximumSize() < file.length()) { String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_FILE_TOO_LARGE_KEY, new String[]{ - inputName, originalFilename, file.getName(), "" + file.length(), getMaximumSizeStr(action) + inputName, originalFilename, file.getName(), "" + file.length(), getMaximumSizeStr(action, policy.getMaximumSize()) }); errorMessages.add(errMsg); LOG.warn(errMsg); } - if ((!allowedTypesSet.isEmpty()) && (!containsItem(allowedTypesSet, contentType))) { + if ((!policy.getAllowedTypes().isEmpty()) && (!containsItem(policy.getAllowedTypes(), contentType))) { String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_CONTENT_TYPE_NOT_ALLOWED_KEY, new String[]{ inputName, originalFilename, file.getName(), contentType }); errorMessages.add(errMsg); LOG.warn(errMsg); } - if ((!allowedExtensionsSet.isEmpty()) && (!hasAllowedExtension(allowedExtensionsSet, originalFilename))) { + if ((!policy.getAllowedExtensions().isEmpty()) && (!hasAllowedExtension(policy.getAllowedExtensions(), originalFilename))) { String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_FILE_EXTENSION_NOT_ALLOWED_KEY, new String[]{ inputName, originalFilename, file.getName(), contentType }); @@ -161,7 +182,7 @@ protected boolean acceptFile(Object action, UploadedFile file, String originalFi return errorMessages.isEmpty(); } - private String getMaximumSizeStr(Object action) { + private String getMaximumSizeStr(Object action, Long maximumSize) { return NumberFormat.getNumberInstance(getLocaleProvider(action).getLocale()).format(maximumSize); } diff --git a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java index 79d020a345..c898275f09 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java @@ -198,16 +198,39 @@ * } * * + *

+ * Dynamic parameters are resolved into a fresh {@link UploadPolicy} for each invocation, so the + * interceptor itself is never modified per request and concurrent uploads cannot observe each + * other's policy. An expression that cannot be resolved does not relax validation: the policy is + * marked unresolved and affected uploads are rejected. A lazily resolved {@code disabled} param + * only takes effect if the interceptor's params holder extends {@link DisableParams} — there is + * deliberately no fallback to the interceptor instance. Likewise, an interceptor that overrides + * {@link ConditionalInterceptor#shouldIntercept(ActionInvocation) shouldIntercept} to read its own + * lazily-injected fields would see only config-time values, since resolution never touches the + * interceptor; {@code ActionFileUploadInterceptor} does not override {@code shouldIntercept}, so + * this does not affect it. + *

+ * * @see WithLazyParams * @see UploadedFilesAware * @see AbstractFileUploadInterceptor */ -public class ActionFileUploadInterceptor extends AbstractFileUploadInterceptor implements WithLazyParams { +public class ActionFileUploadInterceptor extends AbstractFileUploadInterceptor implements WithLazyParams { protected static final Logger LOG = LogManager.getLogger(ActionFileUploadInterceptor.class); + @Override + public UploadPolicy newLazyParams() { + return copyConfiguredPolicy(); + } + @Override public String intercept(ActionInvocation invocation) throws Exception { + return intercept(invocation, newLazyParams()); + } + + @Override + public String intercept(ActionInvocation invocation, UploadPolicy policy) throws Exception { HttpServletRequest request = invocation.getInvocationContext().getServletRequest(); MultiPartRequestWrapper multiWrapper = request instanceof HttpServletRequestWrapper wrapper ? findMultipartRequestWrapper(wrapper) @@ -245,7 +268,7 @@ public String intercept(ActionInvocation invocation) throws Exception { } } else { for (UploadedFile uploadedFile : uploadedFiles) { - if (acceptFile(action, uploadedFile, uploadedFile.getOriginalName(), uploadedFile.getContentType(), inputName)) { + if (acceptFile(policy, action, uploadedFile, uploadedFile.getOriginalName(), uploadedFile.getContentType(), inputName)) { acceptedFiles.add(uploadedFile); } } diff --git a/core/src/main/java/org/apache/struts2/interceptor/DisableParams.java b/core/src/main/java/org/apache/struts2/interceptor/DisableParams.java new file mode 100644 index 0000000000..df016f72e7 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/DisableParams.java @@ -0,0 +1,60 @@ +/* + * 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; + +/** + * Opt-in support for the {@code disabled} interceptor parameter. + *

+ * Interceptors implementing {@link WithLazyParams} must have their params holder extend this + * class to support {@code ...}; there is deliberately no + * fallback to the interceptor instance, which would reintroduce shared mutable state. + * + * @since 7.3.0 + */ +public class DisableParams implements InterceptorParams { + + /** + * Name of the parameter bound to {@link #setDisabled(String)}. + * + * @since 7.3.0 + */ + public static final String DISABLED_PARAM = "disabled"; + + private static final long serialVersionUID = 1L; + + private boolean disabled; + + public DisableParams() { + } + + protected DisableParams(DisableParams other) { + this.disabled = other.disabled; + } + + /** + * @param disable if {@code true}, execution of the interceptor is skipped for this invocation + */ + public void setDisabled(String disable) { + this.disabled = Boolean.parseBoolean(disable); + } + + public boolean isDisabled() { + return disabled; + } +} diff --git a/core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java b/core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java new file mode 100644 index 0000000000..bace8139ad --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java @@ -0,0 +1,55 @@ +/* + * 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; + +import java.io.Serializable; + +/** + * Contract for an object holding the parameters of a single interceptor. + *

+ * Implementations are per-invocation value objects: the framework resolves configured + * parameters into a fresh instance for each action invocation, so nothing is written back + * onto the interceptor, which stays immutable after {@link Interceptor#init()}. + *

+ * Extends {@link Serializable} because an interceptor holding its configured params is itself + * {@link Interceptor serializable}; a non-serializable holder would silently break serialization + * of any interceptor that keeps one as a field. + * + * @since 7.3.0 + */ +public interface InterceptorParams extends Serializable { + + /** + * Called when a configured parameter could not be applied for the current invocation - either a + * {@code ${...}} expression that did not resolve, or a value the holder's setter could not + * accept. The framework skips the write and notifies the holder so it can decide how to degrade. + *

+ * Whatever the holder was seeded with stays in place, but it is rarely a usable default: the + * seed comes from applying the raw configuration string at build time, so for a {@code ${...}} + * param it is the unevaluated literal, or nothing at all when that literal could not be + * converted to the property's type. A holder guarding a security-sensitive dimension should + * therefore treat this as a reason to fail closed rather than carry on. + *

+ * The default implementation does nothing. + * + * @param paramName name of the parameter that could not be applied + */ + default void unresolved(String paramName) { + } +} diff --git a/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java new file mode 100644 index 0000000000..4d03f711a8 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java @@ -0,0 +1,132 @@ +/* + * 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; + +import org.apache.struts2.util.TextParseUtil; + +import java.util.Collections; +import java.util.LinkedHashSet; +import java.util.Set; + +/** + * Per-invocation upload validation policy for {@link ActionFileUploadInterceptor}. + *

+ * A configured instance is held by the interceptor and copied for each invocation, so lazily + * resolved values never reach the shared interceptor. + * + * @since 7.3.0 + */ +public class UploadPolicy extends DisableParams { + + private static final long serialVersionUID = 1L; + + private Long maximumSize; + private Set allowedTypes = Collections.emptySet(); + private Set allowedExtensions = Collections.emptySet(); + private final Set unresolvedParams = new LinkedHashSet<>(); + + public UploadPolicy() { + } + + private UploadPolicy(UploadPolicy other) { + super(other); + this.maximumSize = other.maximumSize; + this.allowedTypes = other.allowedTypes; + this.allowedExtensions = other.allowedExtensions; + this.unresolvedParams.addAll(other.unresolvedParams); + } + + /** + * @param allowedTypes a comma-delimited list of content types, or null for no restriction + */ + public void setAllowedTypes(String allowedTypes) { + this.allowedTypes = toSet(allowedTypes); + } + + /** + * @param allowedExtensions a comma-delimited list of extensions, or null for no restriction + */ + public void setAllowedExtensions(String allowedExtensions) { + this.allowedExtensions = toSet(allowedExtensions); + } + + /** + * @param maximumSize the maximum size in bytes, or null for no limit + */ + public void setMaximumSize(Long maximumSize) { + this.maximumSize = maximumSize; + } + + public Long getMaximumSize() { + return maximumSize; + } + + public Set getAllowedTypes() { + return allowedTypes; + } + + public Set getAllowedExtensions() { + return allowedExtensions; + } + + /** + * 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. + *

+ * {@link DisableParams#DISABLED_PARAM} is the one exception and is not recorded. It is not a + * validation dimension: its unresolved value is simply {@code false}, which leaves the + * interceptor running and every other part of the policy intact, so it cannot relax validation. + * Recording it would let a broken {@code ${...}} reject every + * upload of the invocation, which is a failure mode of its own rather than a safe default. + */ + @Override + public void unresolved(String paramName) { + if (DISABLED_PARAM.equals(paramName)) { + return; + } + unresolvedParams.add(paramName); + } + + public boolean isUnresolved() { + return !unresolvedParams.isEmpty(); + } + + public Set getUnresolvedParams() { + return Collections.unmodifiableSet(unresolvedParams); + } + + /** + * @return an independent copy, used to seed a per-invocation policy from the configured one + */ + public UploadPolicy copy() { + 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 toSet(String commaDelimited) { + return commaDelimited == null + ? Collections.emptySet() + : Collections.unmodifiableSet(TextParseUtil.commaDelimitedStringToSet(commaDelimited)); + } +} diff --git a/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java b/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java index cdfb3a8adb..9a37fec69b 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java +++ b/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java @@ -18,12 +18,16 @@ */ package org.apache.struts2.interceptor; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.apache.struts2.ActionContext; +import org.apache.struts2.ActionInvocation; import org.apache.struts2.inject.Inject; import org.apache.struts2.ognl.OgnlUtil; import org.apache.struts2.util.TextParseUtil; import org.apache.struts2.util.TextParser; import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.reflection.ReflectionException; import org.apache.struts2.util.reflection.ReflectionProvider; import java.util.Map; @@ -41,14 +45,32 @@ *

* The {@link Interceptor#init()} method is called after initial parameter setting, so interceptors * can rely on configured values during initialization. Expression parameters (containing ${...}) - * are re-evaluated at invocation time via {@link LazyParamInjector}. + * are re-evaluated at invocation time via {@link LazyParamInjector} and written into a fresh + * {@link InterceptorParams} holder, never back onto the interceptor, which is shared across + * requests and stays untouched after {@link Interceptor#init()}. * * @since 2.5.9 */ -public interface WithLazyParams { +public interface WithLazyParams

{ + + /** + * @return a fresh holder for one invocation, seeded from the configured values + * @since 7.3.0 + */ + P newLazyParams(); + + /** + * Invoked in place of {@link Interceptor#intercept(ActionInvocation)} when lazy params apply. + * + * @param lazyParams params resolved for this invocation only + * @since 7.3.0 + */ + String intercept(ActionInvocation invocation, P lazyParams) throws Exception; class LazyParamInjector { + private static final Logger LOG = LogManager.getLogger(LazyParamInjector.class); + protected OgnlUtil ognlUtil; protected TextParser textParser; protected ReflectionProvider reflectionProvider; @@ -75,12 +97,79 @@ public void setOgnlUtil(OgnlUtil ognlUtil) { this.ognlUtil = ognlUtil; } - public Interceptor injectParams(Interceptor interceptor, Map params, ActionContext invocationContext) { + /** + * Resolves configured params into a per-invocation holder, leaving the interceptor untouched. + *

+ * Every path that skips a write notifies the holder via + * {@link InterceptorParams#unresolved(String)}, so the holder can fail closed rather than + * silently validating against a dimension that was dropped. Two such paths exist: + *

    + *
  • a {@code ${...}} expression that resolves to null or an empty value (see + * {@link #isUnresolved})
    • + *
  • a resolved value the holder's setter cannot accept, e.g. a non-numeric string for a + * {@code Long} property, which OGNL reports as a + * {@link ReflectionException} during conversion
    • + *
+ * In both cases the injector does nothing beyond skipping the write, notifying the holder and + * logging a WARN; what that means for the invocation is the holder's decision, since + * {@code unresolved} is a no-op by default. Whatever the holder was seeded with is left in + * place, but for a {@code ${...}} param that is not a usable default: the seed comes from + * applying the raw configuration string at build time, so it is either the unevaluated + * {@code ${...}} literal or, when the literal could not be converted to the property's type, + * nothing at all. + *

+ * The empty-value rule also catches an expression that legitimately evaluates to an empty + * string, which is indistinguishable from a failed resolution; for a fail-closed policy such + * as an allowlist, treating both as unusable is the safe reading, so a broken expression + * cannot silently relax a validation policy. + * + * @since 7.3.0 + */ + public

P resolveInto(P target, Map params, ActionContext invocationContext) { for (Map.Entry entry : params.entrySet()) { - Object paramValue = textParser.evaluate(new char[]{'$'}, entry.getValue(), valueEvaluator, TextParser.DEFAULT_LOOP_COUNT); - ognlUtil.setProperty(entry.getKey(), paramValue, interceptor, invocationContext.getContextMap()); + String paramName = entry.getKey(); + String rawValue = entry.getValue(); + Object paramValue = textParser.evaluate(new char[]{'$'}, rawValue, valueEvaluator, TextParser.DEFAULT_LOOP_COUNT); + + if (isUnresolved(rawValue, paramValue)) { + LOG.warn("Param [{}] of [{}] could not be resolved from expression [{}] - the value was not written and the params holder was notified", + paramName, target.getClass().getName(), rawValue); + target.unresolved(paramName); + continue; + } + try { + // throwPropertyExceptions=true so a param with no matching property on the holder is + // reported rather than silently ignored; OgnlUtil only warns in devMode otherwise + ognlUtil.setProperty(paramName, paramValue, target, invocationContext.getContextMap(), true); + } catch (ReflectionException e) { + LOG.warn("Param [{}] could not be applied to [{}] - the value was not written and the params holder was notified; check the interceptor configuration", + paramName, target.getClass().getName(), e); + target.unresolved(paramName); + } } - return interceptor; + return target; + } + + /** + * A {@code ${...}} param is treated as unresolved when its evaluated value is null or empty. + *

+ * {@link org.apache.struts2.util.OgnlTextParser} yields the same empty string both when an + * expression fails to resolve and when it resolves to a legitimately empty value — there is + * no way to tell the two apart from the parser's output alone. This method does not attempt + * to; a param that legitimately evaluates to an empty string is therefore also reported as + * unresolved. That is a deliberate fail-closed choice: for a security-sensitive param (e.g. + * an allowlist), silently accepting an unintended empty value is worse than refusing it. + *

+ * Partial resolution is not detected. A value mixing several expressions, + * e.g. {@code ${a},${b}}, still parses to a non-empty string when only one of them resolves, + * so it is reported as resolved and the truncated value is written. For an allowlist that + * narrows the accepted set rather than widening it, so it does not relax validation, but the + * holder receives fewer entries than configured and gets no {@code unresolved} notification. + */ + private boolean isUnresolved(String rawValue, Object paramValue) { + return rawValue != null + && rawValue.contains("${") + && (paramValue == null || paramValue.toString().isEmpty()); } } } diff --git a/core/src/main/resources/org/apache/struts2/struts-messages.properties b/core/src/main/resources/org/apache/struts2/struts-messages.properties index 63514b000f..ecd99000bd 100644 --- a/core/src/main/resources/org/apache/struts2/struts-messages.properties +++ b/core/src/main/resources/org/apache/struts2/struts-messages.properties @@ -50,6 +50,10 @@ struts.messages.error.content.type.not.allowed=Content-Type not allowed: {0} "{1 # 2 - file name after uploading the file # 3 - content type of the file struts.messages.error.file.extension.not.allowed=File extension not allowed: {0} "{1}" "{2}" {3} +# 0 - input name +# 1 - original filename +# 2 - comma-delimited list of unresolved parameter names +struts.messages.error.upload.policy.unresolved=The upload validation policy could not be resolved, rejecting the file: {0} "{1}"; unresolved parameters: {2} # dedicated messages used to handle various problems with file upload - check {@link JakartaMultiPartRequest#parse(HttpServletRequest, String)} # params depend on exception being handled # FileUploadByteCountLimitException diff --git a/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java b/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java index 6a8e9fc21c..6c355860a4 100644 --- a/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java +++ b/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java @@ -24,6 +24,7 @@ import org.apache.struts2.config.entities.ResultConfig; import org.apache.struts2.config.providers.XmlConfigurationProvider; import org.apache.struts2.dispatcher.HttpParameters; +import org.apache.struts2.interceptor.ConditionalInterceptor; import org.apache.struts2.interceptor.Interceptor; import org.apache.struts2.interceptor.WithLazyParams; import org.apache.struts2.mock.MockActionProxy; @@ -42,6 +43,7 @@ import java.util.concurrent.TimeUnit; import static org.apache.struts2.ognl.OgnlUtilTest.createOgnlUtil; +import static org.assertj.core.api.Assertions.assertThat; /** @@ -160,6 +162,40 @@ public void testInvokeWithDisabledInterceptors() throws Exception { assertTrue(defaultActionInvocation.isExecuted()); } + /** + * WW-5659: {@code executeConditional(ConditionalInterceptor)} was removed in 7.3.0 - it had no + * callers left, so a subclass still overriding it would have gone silently dead. The + * two-argument form is now the only extension point and must be handed the interceptor + * mapping's name, which is what the removed overload could not supply. + */ + public void testExecuteConditionalOverrideReceivesTheMappingName() throws Exception { + // given + List interceptorMappings = new ArrayList<>(); + MockInterceptor mockInterceptor = new MockInterceptor(); + mockInterceptor.setFoo("test1"); + mockInterceptor.setExpectedFoo("test1"); + interceptorMappings.add(new InterceptorMapping("namedInStack", mockInterceptor)); + + List observedNames = new ArrayList<>(); + DefaultActionInvocation defaultActionInvocation = new DefaultActionInvocationTester(interceptorMappings) { + @Override + protected String executeConditional(ConditionalInterceptor conditionalInterceptor, String interceptorName) throws Exception { + observedNames.add(interceptorName); + return super.executeConditional(conditionalInterceptor, interceptorName); + } + }; + container.inject(defaultActionInvocation); + defaultActionInvocation.stack = container.getInstance(ValueStackFactory.class).createValueStack(); + + // when + defaultActionInvocation.setResultCode(""); + defaultActionInvocation.invoke(); + + // then + assertThat(observedNames).containsExactly("namedInStack"); + assertThat(mockInterceptor.isExecuted()).isTrue(); + } + public void testInvokingExistingExecuteMethod() throws Exception { // given DefaultActionInvocation dai = new DefaultActionInvocation(ActionContext.getContext().getContextMap(), false); @@ -423,6 +459,58 @@ public void testInvokeWithLazyParamsStackConfiguration() throws Exception { assertEquals("static value", action.getBlah()); } + /** + * Regression for WW-5659: a {@code disabled} param resolved lazily from the value stack must skip + * the interceptor for that invocation. It arrives through the interceptor mapping's params, so it + * lands on the per-invocation holder and is honoured there, never on the shared interceptor. + */ + public void testInvokeWithLazyParamsSkipsLazilyDisabledInterceptor() throws Exception { + HashMap params = new HashMap<>(); + params.put("blah", "true"); + + ActionContext extraContext = ActionContext.of() + .withParameters(HttpParameters.create(params).build()); + + DefaultActionInvocation defaultActionInvocation = new DefaultActionInvocation(extraContext.getContextMap(), true); + container.inject(defaultActionInvocation); + + ActionProxy actionProxy = actionProxyFactory.createActionProxy("", "LazyFooLazilyDisabled", null, extraContext.getContextMap()); + defaultActionInvocation.init(actionProxy); + defaultActionInvocation.invoke(); + + SimpleAction action = (SimpleAction) defaultActionInvocation.getAction(); + + // the rest of the stack still ran, so the params interceptor applied blah... + assertThat(action.getBlah()).isEqualTo("true"); + // ...but the lazy interceptor was skipped, so it never applied its foo param to the action + assertThat(action.getName()).isNull(); + } + + /** + * Regression for WW-5659: {@code disabled} configured on the interceptor definition never reaches + * the params holder, so it can only be honoured through {@link org.apache.struts2.interceptor.ConditionalInterceptor#shouldIntercept}. + * The lazy path must still consult it. + */ + public void testInvokeWithLazyParamsSkipsStaticallyDisabledInterceptor() throws Exception { + HashMap params = new HashMap<>(); + params.put("blah", "dynamic value"); + + ActionContext extraContext = ActionContext.of() + .withParameters(HttpParameters.create(params).build()); + + DefaultActionInvocation defaultActionInvocation = new DefaultActionInvocation(extraContext.getContextMap(), true); + container.inject(defaultActionInvocation); + + ActionProxy actionProxy = actionProxyFactory.createActionProxy("", "LazyFooStaticallyDisabled", null, extraContext.getContextMap()); + defaultActionInvocation.init(actionProxy); + defaultActionInvocation.invoke(); + + SimpleAction action = (SimpleAction) defaultActionInvocation.getAction(); + + assertThat(action.getBlah()).isEqualTo("dynamic value"); + assertThat(action.getName()).isNull(); + } + public void testInvokeWithAsyncManager() throws Exception { DefaultActionInvocation dai = new DefaultActionInvocation(new HashMap<>(), false); dai.stack = container.getInstance(ValueStackFactory.class).createValueStack(); diff --git a/core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java b/core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java new file mode 100644 index 0000000000..e837d41e4b --- /dev/null +++ b/core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java @@ -0,0 +1,188 @@ +/* + * 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.factory; + +import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.config.ConfigurationException; +import org.apache.struts2.config.entities.InterceptorConfig; +import org.apache.struts2.interceptor.ActionFileUploadInterceptor; +import org.apache.struts2.interceptor.Interceptor; +import org.apache.struts2.interceptor.UploadPolicy; +import org.apache.struts2.mock.MockInterceptor; +import org.apache.struts2.mock.MockLazyInterceptor; + +import java.util.LinkedHashMap; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Covers the configuration-time validation of {@code WithLazyParams} interceptor params (WW-5659). + *

+ * Params of a lazy interceptor-ref are resolved onto a per-invocation params holder, so a name the + * holder cannot accept can never be applied. Before this check that was only noticed per request, + * where it costs a WARN and - for {@link UploadPolicy} - the rejection of every upload. The names + * are known when the configuration is parsed, so it fails there instead. + */ +public class DefaultInterceptorFactoryTest extends StrutsInternalTestCase { + + private InterceptorFactory factory; + + @Override + protected void setUp() throws Exception { + super.setUp(); + factory = container.getInstance(InterceptorFactory.class); + } + + private static Map params(String... keysAndValues) { + Map params = new LinkedHashMap<>(); + for (int i = 0; i < keysAndValues.length; i += 2) { + params.put(keysAndValues[i], keysAndValues[i + 1]); + } + return params; + } + + private static InterceptorConfig config(String name, Class clazz) { + return new InterceptorConfig.Builder(name, clazz.getName()).build(); + } + + /** + * A {@code ${...}} param must not be applied to the interceptor at configuration time: the raw + * literal is meaningless as a policy value, and for the {@code Long} maximumSize it cannot even be + * converted. Idea from @deprrous in GitHub PR #1815. + */ + public void testLazyExpressionsDoNotSeedTheInterceptor() throws Exception { + InterceptorConfig config = config("actionFileUpload", ActionFileUploadInterceptor.class); + + ActionFileUploadInterceptor interceptor = (ActionFileUploadInterceptor) factory.buildInterceptor(config, params( + "allowedTypes", "${uploadConfig.allowedMimeTypes}", + "maximumSize", "${uploadConfig.maxFileSize}")); + + UploadPolicy configured = interceptor.newLazyParams(); + assertThat(configured.getAllowedTypes()).isEmpty(); + assertThat(configured.getMaximumSize()).isNull(); + } + + /** + * Static params, by contrast, are the interceptor's configured policy and must still seed it - they + * are what a per-invocation holder starts from, and what survives a lazy param failing to resolve. + */ + public void testStaticParamsStillSeedTheInterceptor() throws Exception { + InterceptorConfig config = config("actionFileUpload", ActionFileUploadInterceptor.class); + + ActionFileUploadInterceptor interceptor = (ActionFileUploadInterceptor) factory.buildInterceptor(config, params( + "allowedTypes", "text/plain,text/html", + "maximumSize", "2048")); + + UploadPolicy configured = interceptor.newLazyParams(); + assertThat(configured.getAllowedTypes()).containsExactlyInAnyOrder("text/plain", "text/html"); + assertThat(configured.getMaximumSize()).isEqualTo(2048L); + } + + public void testRefParamsWritableOnTheHolderAreAccepted() { + InterceptorConfig config = config("actionFileUpload", ActionFileUploadInterceptor.class); + + assertThatCode(() -> factory.buildInterceptor(config, params( + "allowedTypes", "${uploadConfig.allowedMimeTypes}", + "allowedExtensions", "${uploadConfig.allowedExtensions}", + "maximumSize", "${uploadConfig.maxFileSize}", + "disabled", "${uploadConfig.skip}"))).doesNotThrowAnyException(); + } + + public void testUnknownRefParamFailsConfiguration() { + InterceptorConfig config = config("actionFileUpload", ActionFileUploadInterceptor.class); + + Map refParams = params("maximumSizes", "1024"); + + assertThatThrownBy(() -> factory.buildInterceptor(config, refParams)) + .isInstanceOf(ConfigurationException.class) + .hasMessageContaining("maximumSizes") + .hasMessageContaining("actionFileUpload") + .hasMessageContaining(ActionFileUploadInterceptor.class.getName()) + .hasMessageContaining(UploadPolicy.class.getName()); + } + + /** + * The failure must not be buried by the factory's generic {@code catch (Exception)} wrapper, + * otherwise the operator reads "Caught Exception while registering Interceptor class" and has to + * dig through the cause chain to find the param name. + */ + public void testUnknownRefParamFailureIsNotRewrapped() { + InterceptorConfig config = config("actionFileUpload", ActionFileUploadInterceptor.class); + + Map refParams = params("nope", "x"); + + assertThatThrownBy(() -> factory.buildInterceptor(config, refParams)) + .isInstanceOf(ConfigurationException.class) + .hasNoCause(); + } + + /** + * Params declared on the {@code } definition are applied to the interceptor and + * never reach the {@code InterceptorMapping}, so they never reach the holder either. Validating + * them would reject working configurations. + */ + public void testInterceptorDefinitionParamsAreNotValidatedAgainstTheHolder() { + InterceptorConfig config = new InterceptorConfig.Builder("lazy", MockLazyInterceptor.class.getName()) + .addParam("interceptorOnly", "applied at build time") + .build(); + + Interceptor interceptor = factory.buildInterceptor(config, params()); + + assertThat(interceptor).isInstanceOf(MockLazyInterceptor.class); + assertThat(((MockLazyInterceptor) interceptor).getInterceptorOnly()).isEqualTo("applied at build time"); + } + + /** + * Same property, this time on the interceptor-ref: now it does reach the holder, and the holder + * has no such property. + */ + public void testSameParamOnTheRefIsValidated() { + InterceptorConfig config = config("lazy", MockLazyInterceptor.class); + + Map refParams = params("interceptorOnly", "x"); + + assertThatThrownBy(() -> factory.buildInterceptor(config, refParams)) + .isInstanceOf(ConfigurationException.class) + .hasMessageContaining("interceptorOnly") + .hasMessageContaining(MockLazyInterceptor.MockLazyParams.class.getName()); + } + + /** + * A compound name is an OGNL navigation expression, not a property of the holder, so this simple + * check cannot decide it and leaves it to the runtime path. + */ + public void testCompoundParamNameIsNotValidated() { + InterceptorConfig config = config("lazy", MockLazyInterceptor.class); + + assertThatCode(() -> factory.buildInterceptor(config, params("nested.foo", "x"))).doesNotThrowAnyException(); + } + + /** + * Non-lazy interceptors keep the lenient behaviour: their params go straight onto the instance + * and an unknown one is ignored, which is long-standing and out of scope here. + */ + public void testNonLazyInterceptorRefParamsAreNotValidated() { + InterceptorConfig config = config("test", MockInterceptor.class); + + assertThatCode(() -> factory.buildInterceptor(config, params("noSuchProperty", "x"))).doesNotThrowAnyException(); + } +} diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java index 058dabe115..22f4484086 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -34,6 +34,8 @@ import org.apache.struts2.mock.MockActionInvocation; import org.apache.struts2.mock.MockActionProxy; import org.apache.struts2.util.ClassLoaderUtil; +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.ValueStackFactory; import org.assertj.core.util.Files; import org.springframework.mock.web.MockHttpServletRequest; @@ -41,8 +43,16 @@ import java.net.URI; import java.net.URL; import java.nio.charset.StandardCharsets; +import java.util.HashMap; import java.util.List; import java.util.Locale; +import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import static org.assertj.core.api.Assertions.assertThat; @@ -63,7 +73,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { public void testAcceptFileWithEmptyAllowedTypesAndExtensions() { // when allowed type is empty ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -75,7 +85,7 @@ public void testAcceptFileWithoutEmptyTypes() { // when file is of allowed types ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -83,7 +93,7 @@ public void testAcceptFileWithoutEmptyTypes() { // when file is not of allowed types validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/html", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/html", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -94,7 +104,7 @@ public void testAcceptFileWithWildcardContent() { interceptor.setAllowedTypes("text/*"); ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -102,7 +112,7 @@ public void testAcceptFileWithWildcardContent() { interceptor.setAllowedTypes("text/h*"); validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/plain", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/plain", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -114,7 +124,7 @@ public void testAcceptFileWithoutEmptyExtensions() { // when file is of allowed extensions ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -122,7 +132,7 @@ public void testAcceptFileWithoutEmptyExtensions() { // when file is not of allowed extensions validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/html", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/html", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -130,7 +140,7 @@ public void testAcceptFileWithoutEmptyExtensions() { interceptor.setAllowedExtensions(".txt,.lol"); validation = new ValidationAwareSupport(); - ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.lol", "text/plain", "inputName"); + ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.lol", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -142,7 +152,7 @@ public void testAcceptFileWithNoFile() { // when file is not of allowed types ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, null, "filename.html", "text/html", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, null, "filename.html", "text/html", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -159,7 +169,7 @@ public void testAcceptFileWithNoContent() { interceptor.setAllowedTypes("text/plain"); ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, createTestFile(null), "filename.html", "text/plain", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(null), "filename.html", "text/plain", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -183,7 +193,7 @@ public void testAcceptFileDoesNotMaterializeInMemoryUpload() { .withInputName("inputName") .build(); - boolean ok = interceptor.acceptFile(validation, file, "f.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, file, "f.txt", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.hasErrors()).isFalse(); @@ -203,7 +213,7 @@ public void testRejectedInMemoryUploadIsStillNotMaterialized() { .build(); // wrong content type -> rejected - boolean ok = interceptor.acceptFile(validation, file, "f.html", "text/html", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, file, "f.html", "text/html", "inputName"); assertThat(ok).isFalse(); assertThat(validation.hasErrors()).isTrue(); @@ -221,7 +231,7 @@ public void testAcceptFileWithMaxSize() throws Exception { File file = new File(new URI(url.toString())); assertThat(file).exists(); UploadedFile uploadedFile = StrutsUploadedFile.Builder.create(file).withContentType("text/html").withOriginalName("filename").build(); - boolean notOk = interceptor.acceptFile(validation, uploadedFile, "filename", "text/html", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, uploadedFile, "filename", "text/html", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -536,14 +546,7 @@ private MultiPartRequestWrapper createMultipartRequestMaxStringLength() { } private MultiPartRequestWrapper createMultipartRequest(int maxsize, int maxfilesize, int maxfiles, int maxStringLength) { - JakartaMultiPartRequest jak = new JakartaMultiPartRequest(); - jak.setMaxSize(String.valueOf(maxsize)); - jak.setMaxFileSize(String.valueOf(maxfilesize)); - jak.setMaxFiles(String.valueOf(maxfiles)); - jak.setMaxStringLength(String.valueOf(maxStringLength)); - jak.setDefaultEncoding(StandardCharsets.UTF_8.name()); - - return new MultiPartRequestWrapper(jak, request, tempDir.getAbsolutePath(), new DefaultLocaleProvider()); + return createMultipartRequest(request, maxsize, maxfilesize, maxfiles, maxStringLength); } protected void setUp() throws Exception { @@ -645,11 +648,10 @@ public void testDynamicParameterEvaluation() throws Exception { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - // Simulate WithLazyParams injection by manually setting the parameters - // In real execution, DefaultActionInvocation.invoke() would call LazyParamInjector - interceptor.setAllowedTypes(action.getAllowedMimeTypes()); - - interceptor.intercept(mai); + // Exercise the real resolution path: LazyParamInjector resolves ${allowedMimeTypes} + // into a fresh UploadPolicy instead of mutating the shared interceptor. + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, false, false); + interceptor.intercept(mai, policy); List files = action.getUploadFiles(); @@ -683,8 +685,8 @@ public void testDynamicParametersChangePerRequest() throws Exception { ActionContext.getContext().getValueStack().push(action1); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setAllowedTypes(action1.getAllowedMimeTypes()); - interceptor.intercept(mai1); + UploadPolicy policy1 = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, false, false); + interceptor.intercept(mai1, policy1); assertThat(action1.getUploadFiles()).isNotNull().hasSize(1); assertThat(action1.getUploadFiles().get(0).getContentType()).isEqualTo("text/plain"); @@ -712,8 +714,8 @@ public void testDynamicParametersChangePerRequest() throws Exception { ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); // Simulate new parameter evaluation for second request - interceptor.setAllowedTypes(action2.getAllowedMimeTypes()); - interceptor.intercept(mai2); + UploadPolicy policy2 = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, false, false); + interceptor.intercept(mai2, policy2); assertThat(action2.getUploadFiles()).isNotNull().hasSize(1); assertThat(action2.getUploadFiles().get(0).getContentType()).isEqualTo("text/html"); @@ -743,8 +745,8 @@ public void testDynamicExtensionValidation() throws Exception { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setAllowedExtensions(action.getAllowedExtensions()); - interceptor.intercept(mai); + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), false, true, false); + interceptor.intercept(mai, policy); List files = action.getUploadFiles(); @@ -782,8 +784,8 @@ public void testDynamicMaximumSizeValidation() throws Exception { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setMaximumSize(action.getMaxFileSize()); - interceptor.intercept(mai); + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), false, false, true); + interceptor.intercept(mai, policy); // File should be rejected due to size assertThat(action.hasFieldErrors()).isTrue(); @@ -816,9 +818,8 @@ public void testSecurityValidationWithDynamicParameters() throws Exception { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setAllowedTypes(action.getAllowedMimeTypes()); - interceptor.setAllowedExtensions(action.getAllowedExtensions()); - interceptor.intercept(mai); + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, true, false); + interceptor.intercept(mai, policy); List files = action.getUploadFiles(); @@ -853,8 +854,8 @@ public void testWildcardMatchingWithDynamicParameters() throws Exception { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setAllowedTypes(action.getAllowedMimeTypes()); - interceptor.intercept(mai); + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, false, false); + interceptor.intercept(mai, policy); List files = action.getUploadFiles(); @@ -885,6 +886,7 @@ public static class MyDynamicFileUploadAction extends ActionSupport implements U private String allowedMimeTypes; private String allowedExtensions; private Long maxFileSize; + private String uploadDisabled; @Override public void withUploadedFiles(List uploadedFiles) { @@ -918,6 +920,337 @@ public Long getMaxFileSize() { public void setMaxFileSize(Long maxFileSize) { this.maxFileSize = maxFileSize; } + + public String getUploadDisabled() { + return uploadDisabled; + } + + public void setUploadDisabled(String uploadDisabled) { + this.uploadDisabled = uploadDisabled; + } + } + + public void testUploadPolicyParsesAndCopies() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes("text/plain, text/html"); + policy.setAllowedExtensions(".txt,.html"); + policy.setMaximumSize(1024L); + policy.setDisabled("true"); + + UploadPolicy copy = policy.copy(); + + assertThat(copy.getAllowedTypes()).containsExactlyInAnyOrder("text/plain", "text/html"); + assertThat(copy.getAllowedExtensions()).containsExactlyInAnyOrder(".txt", ".html"); + assertThat(copy.getMaximumSize()).isEqualTo(1024L); + assertThat(copy.isDisabled()).isTrue(); + } + + public void testUploadPolicyCopyIsIndependentOfTheOriginal() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes("text/plain"); + + UploadPolicy copy = policy.copy(); + copy.setAllowedTypes("text/html"); + + assertThat(policy.getAllowedTypes()).containsExactly("text/plain"); + assertThat(copy.getAllowedTypes()).containsExactly("text/html"); + } + + public void testUploadPolicyTreatsNullAsNoRestriction() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes(null); + policy.setAllowedExtensions(null); + + assertThat(policy.getAllowedTypes()).isEmpty(); + assertThat(policy.getAllowedExtensions()).isEmpty(); + } + + /** + * Regression for WW-5659: two concurrent invocations resolving different policies must not + * see each other's values. Scenario contributed by @deprrous in GitHub PR #1815. + */ + public void testConcurrentDynamicPoliciesStayIsolatedPerRequest() throws Exception { + CoordinatedActionFileUploadInterceptor sharedInterceptor = new CoordinatedActionFileUploadInterceptor(); + container.inject(sharedInterceptor); + + MyDynamicFileUploadAction plainPolicyAction = new MyDynamicFileUploadAction(); + plainPolicyAction.setAllowedMimeTypes("text/plain"); + container.inject(plainPolicyAction); + + MyDynamicFileUploadAction htmlPolicyAction = new MyDynamicFileUploadAction(); + htmlPolicyAction.setAllowedMimeTypes("text/html"); + container.inject(htmlPolicyAction); + + ExecutorService executor = Executors.newFixedThreadPool(2); + try { + Future plainResult = executor.submit(() -> runUploadAttempt( + sharedInterceptor, plainPolicyAction, createUploadRequest("plain-policy.html", "text/html", htmlContent))); + + assertThat(sharedInterceptor.awaitFirstValidation()).isTrue(); + + Future htmlResult = executor.submit(() -> runUploadAttempt( + sharedInterceptor, htmlPolicyAction, createUploadRequest("html-policy.html", "text/html", htmlContent))); + + assertThat(htmlResult.get(10, TimeUnit.SECONDS)).isEqualTo("success"); + sharedInterceptor.releaseFirstValidation(); + assertThat(plainResult.get(10, TimeUnit.SECONDS)).isEqualTo("success"); + } finally { + sharedInterceptor.releaseFirstValidation(); + executor.shutdownNow(); + sharedInterceptor.destroy(); + } + + // the text/plain policy must have rejected the text/html upload despite the concurrent + // text/html invocation resolving a more permissive policy on the same interceptor + assertThat(plainPolicyAction.getUploadFiles()).isNull(); + assertThat(plainPolicyAction.getFieldErrors()).containsKey("file"); + + assertThat(htmlPolicyAction.hasFieldErrors()).isFalse(); + assertThat(htmlPolicyAction.getUploadFiles()).isNotNull().hasSize(1); + assertThat(htmlPolicyAction.getUploadFiles().get(0).getOriginalName()).isEqualTo("html-policy.html"); + } + + /** + * Regression for WW-5659: resolving an invocation's params must leave the interceptor + * singleton exactly as configured. + */ + public void testResolutionDoesNotMutateTheInterceptor() throws Exception { + ActionFileUploadInterceptor configuredInterceptor = new ActionFileUploadInterceptor(); + container.inject(configuredInterceptor); + configuredInterceptor.setAllowedTypes("text/plain"); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes("text/html"); + container.inject(action); + + runUploadAttempt(configuredInterceptor, action, createUploadRequest("f.html", "text/html", htmlContent)); + + assertThat(configuredInterceptor.newLazyParams().getAllowedTypes()).containsExactly("text/plain"); + } + + /** + * Regression for WW-5659: a lazily resolved {@code disabled} must apply to one invocation only. + */ + public void testDisabledIsResolvedPerInvocation() { + ActionFileUploadInterceptor sharedInterceptor = new ActionFileUploadInterceptor(); + container.inject(sharedInterceptor); + + MyDynamicFileUploadAction disablingAction = new MyDynamicFileUploadAction(); + disablingAction.setUploadDisabled("true"); + container.inject(disablingAction); + + MyDynamicFileUploadAction enablingAction = new MyDynamicFileUploadAction(); + enablingAction.setUploadDisabled("false"); + container.inject(enablingAction); + + UploadPolicy disabledPolicy = resolveDisabled(sharedInterceptor, disablingAction); + UploadPolicy enabledPolicy = resolveDisabled(sharedInterceptor, enablingAction); + + // the second resolution must not have cleared the first invocation's flag... + assertThat(disabledPolicy.isDisabled()).isTrue(); + assertThat(enabledPolicy.isDisabled()).isFalse(); + // ...and neither resolution may reach the shared interceptor + assertThat(sharedInterceptor.newLazyParams().isDisabled()).isFalse(); + } + + private UploadPolicy resolveDisabled(ActionFileUploadInterceptor actionFileUploadInterceptor, + MyDynamicFileUploadAction action) { + ValueStack valueStack = container.getInstance(ValueStackFactory.class).createValueStack(); + valueStack.push(action); + + ActionContext context = ActionContext.of(valueStack.getContext()) + .withContainer(container) + .withValueStack(valueStack) + .bind(); + try { + WithLazyParams.LazyParamInjector injector = new WithLazyParams.LazyParamInjector(valueStack); + container.inject(injector); + return injector.resolveInto(actionFileUploadInterceptor.newLazyParams(), + Map.of("disabled", "${uploadDisabled}"), context); + } finally { + ActionContext.clear(); + } + } + + /** + * Resolves the given flags into a fresh {@link UploadPolicy} via the real + * {@link WithLazyParams.LazyParamInjector} path, using the already-bound {@code context} + * (and its ValueStack, with the action already pushed by the caller) rather than fabricating + * a new one. This mirrors what {@code DefaultActionInvocation} does at request time, so tests + * exercise the actual resolution instead of hand-calling a setter on the shared interceptor. + */ + private UploadPolicy injectDynamicUploadPolicy(ActionFileUploadInterceptor actionFileUploadInterceptor, + ActionContext context, + boolean includeAllowedTypes, + boolean includeAllowedExtensions, + boolean includeMaximumSize) { + Map params = new HashMap<>(); + if (includeAllowedTypes) { + params.put("allowedTypes", "${allowedMimeTypes}"); + } + if (includeAllowedExtensions) { + params.put("allowedExtensions", "${allowedExtensions}"); + } + if (includeMaximumSize) { + params.put("maximumSize", "${maxFileSize}"); + } + + WithLazyParams.LazyParamInjector injector = new WithLazyParams.LazyParamInjector(context.getValueStack()); + container.inject(injector); + return injector.resolveInto(actionFileUploadInterceptor.newLazyParams(), params, context); + } + + private String runUploadAttempt(ActionFileUploadInterceptor actionFileUploadInterceptor, + MyDynamicFileUploadAction action, + MockHttpServletRequest uploadRequest) throws Exception { + MultiPartRequestWrapper multiPartRequest = createMultipartRequest(uploadRequest, -1, -1, 3, -1); + ValueStack valueStack = container.getInstance(ValueStackFactory.class).createValueStack(); + valueStack.push(action); + + ActionContext context = ActionContext.of(valueStack.getContext()) + .withContainer(container) + .withValueStack(valueStack) + .withServletRequest(multiPartRequest) + .bind(); + try { + MockActionInvocation invocation = new MockActionInvocation(); + invocation.setAction(action); + invocation.setResultCode("success"); + invocation.setInvocationContext(context); + + Map params = new HashMap<>(); + params.put("allowedTypes", "${allowedMimeTypes}"); + + WithLazyParams.LazyParamInjector injector = new WithLazyParams.LazyParamInjector(valueStack); + container.inject(injector); + UploadPolicy policy = injector.resolveInto(actionFileUploadInterceptor.newLazyParams(), params, context); + + return actionFileUploadInterceptor.intercept(invocation, policy); + } finally { + ActionContext.clear(); + } + } + + private MockHttpServletRequest createUploadRequest(String filename, String contentType, String content) { + MockHttpServletRequest uploadRequest = new MockHttpServletRequest(); + uploadRequest.setCharacterEncoding(StandardCharsets.UTF_8.name()); + uploadRequest.setMethod("POST"); + uploadRequest.addHeader("Content-type", "multipart/form-data; boundary=\"" + boundary + "\""); + uploadRequest.setContent((encodeTextFile(filename, contentType, content) + endLine + "--" + boundary + "--") + .getBytes(StandardCharsets.UTF_8)); + return uploadRequest; + } + + private MultiPartRequestWrapper createMultipartRequest(MockHttpServletRequest multipartRequest, int maxsize, int maxfilesize, int maxfiles, int maxStringLength) { + JakartaMultiPartRequest jak = new JakartaMultiPartRequest(); + jak.setMaxSize(String.valueOf(maxsize)); + jak.setMaxFileSize(String.valueOf(maxfilesize)); + jak.setMaxFiles(String.valueOf(maxfiles)); + jak.setMaxStringLength(String.valueOf(maxStringLength)); + jak.setDefaultEncoding(StandardCharsets.UTF_8.name()); + return new MultiPartRequestWrapper(jak, multipartRequest, tempDir.getAbsolutePath(), new DefaultLocaleProvider()); + } + + /** Pauses the first validation so a second invocation can overlap it. From PR #1815 by @deprrous. */ + private static final class CoordinatedActionFileUploadInterceptor extends ActionFileUploadInterceptor { + private final AtomicBoolean pauseFirstValidation = new AtomicBoolean(true); + private final CountDownLatch firstValidationEntered = new CountDownLatch(1); + private final CountDownLatch allowFirstValidationToContinue = new CountDownLatch(1); + + @Override + protected boolean acceptFile(UploadPolicy policy, Object action, UploadedFile file, String originalFilename, String contentType, String inputName) { + if (pauseFirstValidation.compareAndSet(true, false)) { + firstValidationEntered.countDown(); + awaitUnchecked(allowFirstValidationToContinue); + } + return super.acceptFile(policy, action, file, originalFilename, contentType, inputName); + } + + private boolean awaitFirstValidation() throws InterruptedException { + return firstValidationEntered.await(10, TimeUnit.SECONDS); + } + + private void releaseFirstValidation() { + allowFirstValidationToContinue.countDown(); + } + + private void awaitUnchecked(CountDownLatch latch) { + try { + if (!latch.await(10, TimeUnit.SECONDS)) { + throw new AssertionError("Timed out waiting for concurrent validation release"); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new AssertionError("Interrupted while waiting for concurrent validation release", e); + } + } + } + + public void testUnresolvedPolicyRejectsTheUpload() throws Exception { + ActionFileUploadInterceptor uploadInterceptor = new ActionFileUploadInterceptor(); + container.inject(uploadInterceptor); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes(null); // ${allowedMimeTypes} will not resolve + container.inject(action); + + runUploadAttempt(uploadInterceptor, action, createUploadRequest("f.txt", "text/plain", plainContent)); + + assertThat(action.getUploadFiles()).isNull(); + assertThat(action.getFieldErrors()).containsKey("file"); + } + + public void testResolvedPolicyStillAcceptsTheUpload() throws Exception { + ActionFileUploadInterceptor uploadInterceptor = new ActionFileUploadInterceptor(); + container.inject(uploadInterceptor); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes("text/plain"); + container.inject(action); + + runUploadAttempt(uploadInterceptor, action, createUploadRequest("f.txt", "text/plain", plainContent)); + + assertThat(action.hasFieldErrors()).isFalse(); + assertThat(action.getUploadFiles()).isNotNull().hasSize(1); + } + + public void testUploadPolicyTracksUnresolvedParams() { + UploadPolicy policy = new UploadPolicy(); + assertThat(policy.isUnresolved()).isFalse(); + + policy.unresolved("allowedTypes"); + + assertThat(policy.isUnresolved()).isTrue(); + assertThat(policy.getUnresolvedParams()).containsExactly("allowedTypes"); + } + + /** + * An unresolvable {@code disabled} cannot relax validation - its absent value is {@code false}, + * so the interceptor simply runs - and must not take the whole policy down with it. + */ + public void testUnresolvedDisabledDoesNotMakeThePolicyUnusable() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes("text/plain"); + + policy.unresolved(DisableParams.DISABLED_PARAM); + + assertThat(policy.isUnresolved()).isFalse(); + assertThat(policy.getUnresolvedParams()).isEmpty(); + assertThat(policy.isDisabled()).isFalse(); + assertThat(policy.getAllowedTypes()).containsExactly("text/plain"); + } + + /** + * ...but a validation dimension failing alongside it still does. + */ + public void testUnresolvedDisabledDoesNotMaskOtherUnresolvedParams() { + UploadPolicy policy = new UploadPolicy(); + + policy.unresolved(DisableParams.DISABLED_PARAM); + policy.unresolved("maximumSize"); + + assertThat(policy.isUnresolved()).isTrue(); + assertThat(policy.getUnresolvedParams()).containsExactly("maximumSize"); } } diff --git a/core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java b/core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java new file mode 100644 index 0000000000..3a41b0b843 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java @@ -0,0 +1,60 @@ +/* + * 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; + +import junit.framework.TestCase; + +import static org.assertj.core.api.Assertions.assertThat; + +public class DisableParamsTest extends TestCase { + + public void testDisabledDefaultsToFalse() { + assertThat(new DisableParams().isDisabled()).isFalse(); + } + + public void testSetDisabledParsesStringValue() { + DisableParams params = new DisableParams(); + params.setDisabled("true"); + assertThat(params.isDisabled()).isTrue(); + + params.setDisabled("false"); + assertThat(params.isDisabled()).isFalse(); + } + + public void testSetDisabledTreatsNonBooleanTextAsFalse() { + DisableParams params = new DisableParams(); + params.setDisabled("yes"); + assertThat(params.isDisabled()).isFalse(); + } + + public void testCopyConstructorCarriesDisabledFlag() { + DisableParams original = new DisableParams(); + original.setDisabled("true"); + + DisableParams copy = new DisableParams(original); + + assertThat(copy.isDisabled()).isTrue(); + } + + public void testUnresolvedDefaultsToNoOp() { + DisableParams params = new DisableParams(); + params.unresolved("someParam"); // must not throw + assertThat(params.isDisabled()).isFalse(); + } +} diff --git a/core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java b/core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java new file mode 100644 index 0000000000..48ef39c566 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java @@ -0,0 +1,160 @@ +/* + * 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; + +import org.apache.struts2.ActionContext; +import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.ValueStackFactory; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; + +public class LazyParamInjectorTest extends StrutsInternalTestCase { + + public static class Holder extends DisableParams { + private String name; + private Long size; + private final List unresolvedCalls = new ArrayList<>(); + + public void setName(String name) { this.name = name; } + public void setSize(Long size) { this.size = size; } + public String getName() { return name; } + public Long getSize() { return size; } + public List getUnresolvedCalls() { return unresolvedCalls; } + + @Override + public void unresolved(String paramName) { unresolvedCalls.add(paramName); } + } + + public static class Bean { + public String getLabel() { return "resolved-label"; } + public Long getLimit() { return 4096L; } + public String getBlank() { return ""; } + public String getNotANumber() { return "5MB"; } + } + + private ActionContext context; + private WithLazyParams.LazyParamInjector injector; + + @Override + protected void setUp() throws Exception { + super.setUp(); + ValueStack stack = container.getInstance(ValueStackFactory.class).createValueStack(); + stack.push(new Bean()); + context = ActionContext.of(stack.getContext()).withContainer(container).withValueStack(stack).bind(); + injector = new WithLazyParams.LazyParamInjector(stack); + container.inject(injector); + } + + @Override + protected void tearDown() throws Exception { + ActionContext.clear(); + super.tearDown(); + } + + public void testResolvesExpressionsIntoTheHolder() { + Map params = new HashMap<>(); + params.put("name", "${label}"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getName()).isEqualTo("resolved-label"); + assertThat(holder.getUnresolvedCalls()).isEmpty(); + } + + public void testAppliesOgnlTypeConversion() { + Map params = new HashMap<>(); + params.put("size", "${limit}"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getSize()).isEqualTo(4096L); + } + + public void testPassesLiteralValuesThrough() { + Map params = new HashMap<>(); + params.put("name", "plain-text"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getName()).isEqualTo("plain-text"); + assertThat(holder.getUnresolvedCalls()).isEmpty(); + } + + public void testUnresolvableExpressionSkipsWriteAndNotifiesHolder() { + Holder seeded = new Holder(); + seeded.setName("seeded-value"); + + Map params = new HashMap<>(); + params.put("name", "${noSuchProperty}"); + + Holder holder = injector.resolveInto(seeded, params, context); + + assertThat(holder.getName()).isEqualTo("seeded-value"); + assertThat(holder.getUnresolvedCalls()).containsExactly("name"); + } + + public void testExpressionResolvingToEmptyIsTreatedAsUnresolved() { + Holder seeded = new Holder(); + seeded.setName("seeded-value"); + + Map params = new HashMap<>(); + params.put("name", "${blank}"); + + Holder holder = injector.resolveInto(seeded, params, context); + + assertThat(holder.getName()).isEqualTo("seeded-value"); + assertThat(holder.getUnresolvedCalls()).containsExactly("name"); + } + + public void testValueThatCannotBeConvertedSkipsWriteAndNotifiesHolder() { + Map params = new HashMap<>(); + params.put("size", "${notANumber}"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getSize()).isNull(); + assertThat(holder.getUnresolvedCalls()).containsExactly("size"); + } + + public void testResolvesDisabledOntoDisableParams() { + Map params = new HashMap<>(); + params.put("disabled", "true"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.isDisabled()).isTrue(); + } + + public void testUnknownParamDoesNotFailTheInvocationButNotifiesHolder() { + Map params = new HashMap<>(); + params.put("noSuchParam", "whatever"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getName()).isNull(); + assertThat(holder.getSize()).isNull(); + assertThat(holder.getUnresolvedCalls()).containsExactly("noSuchParam"); + } +} diff --git a/core/src/test/java/org/apache/struts2/interceptor/LazyParamsAllowlistTest.java b/core/src/test/java/org/apache/struts2/interceptor/LazyParamsAllowlistTest.java new file mode 100644 index 0000000000..8e07147949 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/LazyParamsAllowlistTest.java @@ -0,0 +1,123 @@ +/* + * 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; + +import org.apache.struts2.ActionContext; +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.dispatcher.PrepareOperations; +import org.apache.struts2.ognl.ProviderAllowlist; +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.ValueStackFactory; + +import java.util.HashMap; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Regression for WW-5659: lazy interceptor params must still resolve when the OGNL allowlist is + * enforced. + *

+ * {@link WithLazyParams.LazyParamInjector} writes resolved values onto a per-invocation + * {@link InterceptorParams} holder with OGNL. The holder is never named in any configuration, so + * unlike the interceptor it is not allowlisted by + * {@code XmlDocConfigurationProvider.allowAndLoadClass}; without an explicit registration + * {@code SecurityMemberAccess} refuses the write and the fail-closed handling marks every param + * unresolved, which rejects every upload. + *

+ * Every other core test runs with {@code struts.allowlist.enable=false} (see + * {@code StrutsTestCaseHelper}), so this is the only core coverage of that production setting for + * this feature - the showcase {@code DynamicFileUploadTest} integration test is the other one. + */ +public class LazyParamsAllowlistTest extends StrutsInternalTestCase { + + /** + * Stands in for the action whose state drives the upload rules. Allowlisted explicitly below, + * as a real action class would be by the configuration provider that loads it. + */ + public static class UploadRules { + + public String getAllowedMimeTypes() { + return "text/plain"; + } + + public Long getMaxFileSize() { + return 2048L; + } + } + + @Override + protected void setUp() throws Exception { + PrepareOperations.clearDevModeOverride(); + Map params = new HashMap<>(); + // the shipped default, which StrutsTestCaseHelper otherwise turns off for tests + params.put(StrutsConstants.STRUTS_ALLOWLIST_ENABLE, "true"); + // only the value-stack root needs help; the params holder must be allowlisted by the framework + params.put(StrutsConstants.STRUTS_ALLOWLIST_CLASSES, UploadRules.class.getName()); + initDispatcher(params); + } + + public void testAllowlistIsActuallyEnforced() { + assertThat(container.getInstance(String.class, StrutsConstants.STRUTS_ALLOWLIST_ENABLE)).isEqualTo("true"); + } + + public void testParamsHolderHierarchyIsAllowlistedAtConfigurationTime() { + ProviderAllowlist providerAllowlist = container.getInstance(ProviderAllowlist.class); + + assertThat(providerAllowlist.getProviderAllowlist()).contains( + UploadPolicy.class, // the holder itself, the OGNL target + DisableParams.class, // declares setDisabled + InterceptorParams.class // declares unresolved + ); + } + + /** + * The regression proper: a {@code ${...}} param must reach the holder, not be swallowed by the + * allowlist and reported as unresolved. + */ + public void testLazyParamsResolveOntoTheHolderWithAllowlistEnabled() { + ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); + container.inject(interceptor); + + ValueStack valueStack = container.getInstance(ValueStackFactory.class).createValueStack(); + valueStack.push(new UploadRules()); + ActionContext context = ActionContext.of(valueStack.getContext()) + .withContainer(container) + .withValueStack(valueStack) + .bind(); + try { + WithLazyParams.LazyParamInjector injector = new WithLazyParams.LazyParamInjector(valueStack); + container.inject(injector); + + Map params = new HashMap<>(); + params.put("allowedTypes", "${allowedMimeTypes}"); + params.put("maximumSize", "${maxFileSize}"); + + UploadPolicy policy = injector.resolveInto(interceptor.newLazyParams(), params, context); + + assertThat(policy.getUnresolvedParams()).isEmpty(); + assertThat(policy.isUnresolved()).isFalse(); + assertThat(policy.getAllowedTypes()).containsExactly("text/plain"); + assertThat(policy.getMaximumSize()).isEqualTo(2048L); + } finally { + ActionContext.clear(); + } + } +} diff --git a/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java b/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java index ae3aea3cc3..eda9c6e93b 100644 --- a/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java +++ b/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java @@ -21,12 +21,53 @@ import org.apache.struts2.ActionInvocation; import org.apache.struts2.SimpleAction; import org.apache.struts2.interceptor.AbstractInterceptor; +import org.apache.struts2.interceptor.DisableParams; import org.apache.struts2.interceptor.WithLazyParams; -public class MockLazyInterceptor extends AbstractInterceptor implements WithLazyParams { +public class MockLazyInterceptor extends AbstractInterceptor implements WithLazyParams { + + /** + * Per-invocation holder, seeded from the configured values. Extends {@link DisableParams} so a + * lazily resolved {@code disabled} param applies to a single invocation. + */ + public static class MockLazyParams extends DisableParams { + + private String foo = ""; + private String bar = ""; + + public void setFoo(String foo) { + this.foo = foo; + } + + public String getFoo() { + return foo; + } + + public void setBar(String bar) { + this.bar = bar; + } + + public String getBar() { + return bar; + } + } private String foo = ""; private String bar = ""; + private String interceptorOnly = ""; + + /** + * A property of the interceptor with no counterpart on {@link MockLazyParams}. Configuring it on + * the {@code } definition is legitimate - definition params are applied to the + * interceptor and never reach the holder - so the configuration-time check must not reject it. + */ + public void setInterceptorOnly(String interceptorOnly) { + this.interceptorOnly = interceptorOnly; + } + + public String getInterceptorOnly() { + return interceptorOnly; + } public void setFoo(String foo) { this.foo = foo; @@ -44,12 +85,26 @@ public String getBar() { return bar; } + @Override + public MockLazyParams newLazyParams() { + MockLazyParams params = new MockLazyParams(); + params.setFoo(foo); + params.setBar(bar); + return params; + } + + @Override public String intercept(ActionInvocation invocation) throws Exception { + return intercept(invocation, newLazyParams()); + } + + @Override + public String intercept(ActionInvocation invocation, MockLazyParams lazyParams) throws Exception { if (invocation.getAction() instanceof SimpleAction) { - ((SimpleAction) invocation.getAction()).setName(foo); + ((SimpleAction) invocation.getAction()).setName(lazyParams.getFoo()); // Only set blah if bar is configured (not empty) - if (bar != null && !bar.isEmpty()) { - ((SimpleAction) invocation.getAction()).setBlah(bar); + if (lazyParams.getBar() != null && !lazyParams.getBar().isEmpty()) { + ((SimpleAction) invocation.getAction()).setBlah(lazyParams.getBar()); } } return invocation.invoke(); diff --git a/core/src/test/resources/xwork-sample.xml b/core/src/test/resources/xwork-sample.xml index 5bc189d821..5615e8ef2f 100644 --- a/core/src/test/resources/xwork-sample.xml +++ b/core/src/test/resources/xwork-sample.xml @@ -72,6 +72,27 @@ + + + + + + should not be applied + ${blah} + + + + + + + + + should not be applied + + + 17 23 diff --git a/core/src/test/resources/xwork-test-default.xml b/core/src/test/resources/xwork-test-default.xml index 31f92d9d66..87931c7d29 100644 --- a/core/src/test/resources/xwork-test-default.xml +++ b/core/src/test/resources/xwork-test-default.xml @@ -42,6 +42,11 @@ expectedFoo + + + true + diff --git a/docs/superpowers/plans/2026-07-27-WW-5659-lazy-params-request-scoping.md b/docs/superpowers/plans/2026-07-27-WW-5659-lazy-params-request-scoping.md new file mode 100644 index 0000000000..8391167aee --- /dev/null +++ b/docs/superpowers/plans/2026-07-27-WW-5659-lazy-params-request-scoping.md @@ -0,0 +1,1323 @@ +# WW-5659 Request-Scoped Lazy Interceptor Params Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Stop `WithLazyParams` from writing per-request resolved `${...}` params onto the shared interceptor singleton, so concurrent requests can no longer read one another's upload validation policy. + +**Architecture:** Resolved params are written into a per-invocation holder object that the interceptor supplies (`newLazyParams()`) and receives back (`intercept(invocation, params)`). The interceptor singleton becomes immutable after `init()`. A new `InterceptorParams` marker is the general contract; `DisableParams` is opt-in support for the `disabled` param; `UploadPolicy` is the file-upload holder. Unresolvable expressions fail closed instead of silently disabling validation. + +**Tech Stack:** Java 17, Maven, JUnit 3-style tests (`junit.framework.TestCase` via `XWorkTestCase`), AssertJ assertions, Log4j2. + +**Spec:** `docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md` + +## Global Constraints + +- **Ticket prefix:** every commit message starts with `WW-5659`. +- **Target version:** 7.3.0. This is a deliberate public API break; no backport to 7.2.x. +- **Test style:** `core` tests are JUnit 3 style — `protected void setUp()`, tests named `public void testXxx()`, no annotations. **A JUnit 5 `@Test` annotation compiles and silently never runs.** Never add one. Extend `StrutsInternalTestCase` (which extends `XWorkTestCase`, which extends `junit.framework.TestCase`) when the test needs the Struts container or `ActionContext`; extend `junit.framework.TestCase` directly for pure value-object tests that need neither. +- **Assertions:** use AssertJ (`import static org.assertj.core.api.Assertions.assertThat;`), matching the existing `ActionFileUploadInterceptorTest`. +- **Package:** all new production types go in `org.apache.struts2.interceptor`. +- **License header:** every new `.java` file starts with the ASF header, copied verbatim from `core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java:1-18`. +- **Message bundles:** new keys go into `core/src/main/resources/org/apache/struts2/struts-messages.properties` **only**. The five locale bundles (`_da`, `_de`, `_en`, `_pl`, `_pt`) are partial (12–14 keys vs 20 in the base) and fall back to the base bundle. Do not fabricate translations. +- **Build command:** `mvn test -DskipAssembly -pl core -Dtest=ClassName#methodName` + +## Deviation from the spec — read before Task 5 + +The spec's Error Handling section says `unresolved(param)` should mark a dimension unusable **only if** the seeded config-time value still contains `${`. That rule is not implementable deterministically: it relies on introspecting the seed, and for `maximumSize` the seed is a `Long`, so a `${...}` literal cannot survive there — whether config-time OGNL conversion of `"${maxFileSize}"` to `Long` yields `null`, throws, or skips the setter is not knowable without testing, and the rule would silently behave differently per param type. + +**This plan implements the simpler, deterministic rule instead: on unresolved, the dimension is marked unusable unconditionally.** A static fallback then applies only when the param is absent from the lazy map entirely (pure static config), which never triggers `unresolved`. This is stricter, type-independent, and consistent with the chosen fail-closed policy. + +Task 6 includes a step to update the spec to match. **If the reviewer prefers the spec's original rule, stop and re-plan Task 5** rather than implementing both. + +## File Structure + +**Created:** +- `core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java` — general params-holder contract; generic bound for `WithLazyParams`. +- `core/src/main/java/org/apache/struts2/interceptor/DisableParams.java` — opt-in holder state for the `disabled` param. +- `core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java` — file-upload holder: sizes, types, extensions, unresolved tracking. +- `core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java` +- `core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java` + +**Modified:** +- `core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java` — new generic contract; `injectParams` → `resolveInto`. +- `core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java` — single config-time `UploadPolicy`; `acceptFile` takes the policy. +- `core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java` — implements `WithLazyParams`. +- `core/src/main/java/org/apache/struts2/DefaultActionInvocation.java:258-276` — merged lazy/conditional path; non-mutating `mergedParams`. +- `core/src/main/resources/org/apache/struts2/struts-messages.properties` — new fail-closed message key. +- `core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java` — regression + migration. + +--- + +### Task 1: `InterceptorParams` and `DisableParams` + +Purely additive — nothing references these yet. Establishes the vocabulary the rest of the plan builds on. + +**Files:** +- Create: `core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java` +- Create: `core/src/main/java/org/apache/struts2/interceptor/DisableParams.java` +- Test: `core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java` + +**Interfaces:** +- Consumes: nothing. +- Produces: `interface InterceptorParams { default void unresolved(String paramName) {} }`; `class DisableParams implements InterceptorParams` with `void setDisabled(String)`, `boolean isDisabled()`, and `protected DisableParams(DisableParams other)` copy constructor. + +- [ ] **Step 1: Write the failing test** + +Create `core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java` (ASF header first): + +```java +package org.apache.struts2.interceptor; + +import junit.framework.TestCase; + +import static org.assertj.core.api.Assertions.assertThat; + +public class DisableParamsTest extends TestCase { + + public void testDisabledDefaultsToFalse() { + assertThat(new DisableParams().isDisabled()).isFalse(); + } + + public void testSetDisabledParsesStringValue() { + DisableParams params = new DisableParams(); + params.setDisabled("true"); + assertThat(params.isDisabled()).isTrue(); + + params.setDisabled("false"); + assertThat(params.isDisabled()).isFalse(); + } + + public void testSetDisabledTreatsNonBooleanTextAsFalse() { + DisableParams params = new DisableParams(); + params.setDisabled("yes"); + assertThat(params.isDisabled()).isFalse(); + } + + public void testCopyConstructorCarriesDisabledFlag() { + DisableParams original = new DisableParams(); + original.setDisabled("true"); + + DisableParams copy = new DisableParams(original); + + assertThat(copy.isDisabled()).isTrue(); + } + + public void testUnresolvedDefaultsToNoOp() { + DisableParams params = new DisableParams(); + params.unresolved("someParam"); // must not throw + assertThat(params.isDisabled()).isFalse(); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=DisableParamsTest` +Expected: compilation failure — `DisableParams` does not exist. + +- [ ] **Step 3: Write minimal implementation** + +Create `core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java` (ASF header first): + +```java +package org.apache.struts2.interceptor; + +/** + * Contract for an object holding the parameters of a single interceptor. + *

+ * Implementations are per-invocation value objects: the framework resolves configured + * parameters into a fresh instance for each action invocation, so nothing is written back + * onto the interceptor, which stays immutable after {@link Interceptor#init()}. + * + * @since 7.3.0 + */ +public interface InterceptorParams { + + /** + * Called when a {@code ${...}} parameter could not be resolved for the current invocation. + * The framework skips the write, leaving the seeded configuration value in place, and + * notifies the holder so it can decide how to degrade. + *

+ * The default implementation does nothing. + * + * @param paramName name of the parameter that could not be resolved + */ + default void unresolved(String paramName) { + } +} +``` + +Create `core/src/main/java/org/apache/struts2/interceptor/DisableParams.java` (ASF header first): + +```java +package org.apache.struts2.interceptor; + +/** + * Opt-in support for the {@code disabled} interceptor parameter. + *

+ * Interceptors implementing {@link WithLazyParams} must have their params holder extend this + * class to support {@code ...}; there is deliberately no + * fallback to the interceptor instance, which would reintroduce shared mutable state. + * + * @since 7.3.0 + */ +public class DisableParams implements InterceptorParams { + + private boolean disabled; + + public DisableParams() { + } + + protected DisableParams(DisableParams other) { + this.disabled = other.disabled; + } + + /** + * @param disable if {@code true}, execution of the interceptor is skipped for this invocation + */ + public void setDisabled(String disable) { + this.disabled = Boolean.parseBoolean(disable); + } + + public boolean isDisabled() { + return disabled; + } +} +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `mvn test -DskipAssembly -pl core -Dtest=DisableParamsTest` +Expected: PASS, 5 tests. + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java \ + core/src/main/java/org/apache/struts2/interceptor/DisableParams.java \ + core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java +git commit -m "WW-5659 feat(core): add InterceptorParams contract and DisableParams holder" +``` + +--- + +### Task 2: `LazyParamInjector.resolveInto` + +Adds the new resolution method **alongside** the existing `injectParams`, so the tree keeps compiling. `injectParams` is removed in Task 4 when the call site moves. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java:78-84` +- Test: `core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java` + +**Interfaces:** +- Consumes: `InterceptorParams` from Task 1. +- Produces: `public

P resolveInto(P target, Map params, ActionContext invocationContext)` on `WithLazyParams.LazyParamInjector`. + +- [ ] **Step 1: Write the failing test** + +Create `core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java` (ASF header first): + +```java +package org.apache.struts2.interceptor; + +import org.apache.struts2.ActionContext; +import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.ValueStackFactory; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; + +public class LazyParamInjectorTest extends StrutsInternalTestCase { + + public static class Holder extends DisableParams { + private String name; + private Long size; + private final List unresolvedCalls = new ArrayList<>(); + + public void setName(String name) { this.name = name; } + public void setSize(Long size) { this.size = size; } + public String getName() { return name; } + public Long getSize() { return size; } + public List getUnresolvedCalls() { return unresolvedCalls; } + + @Override + public void unresolved(String paramName) { unresolvedCalls.add(paramName); } + } + + public static class Bean { + public String getLabel() { return "resolved-label"; } + public Long getLimit() { return 4096L; } + } + + private ActionContext context; + private WithLazyParams.LazyParamInjector injector; + + @Override + protected void setUp() throws Exception { + super.setUp(); + ValueStack stack = container.getInstance(ValueStackFactory.class).createValueStack(); + stack.push(new Bean()); + context = ActionContext.of(stack.getContext()).withContainer(container).withValueStack(stack).bind(); + injector = new WithLazyParams.LazyParamInjector(stack); + container.inject(injector); + } + + @Override + protected void tearDown() throws Exception { + ActionContext.clear(); + super.tearDown(); + } + + public void testResolvesExpressionsIntoTheHolder() { + Map params = new HashMap<>(); + params.put("name", "${label}"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getName()).isEqualTo("resolved-label"); + assertThat(holder.getUnresolvedCalls()).isEmpty(); + } + + public void testAppliesOgnlTypeConversion() { + Map params = new HashMap<>(); + params.put("size", "${limit}"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getSize()).isEqualTo(4096L); + } + + public void testPassesLiteralValuesThrough() { + Map params = new HashMap<>(); + params.put("name", "plain-text"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getName()).isEqualTo("plain-text"); + assertThat(holder.getUnresolvedCalls()).isEmpty(); + } + + public void testUnresolvableExpressionSkipsWriteAndNotifiesHolder() { + Holder seeded = new Holder(); + seeded.setName("seeded-value"); + + Map params = new HashMap<>(); + params.put("name", "${noSuchProperty}"); + + Holder holder = injector.resolveInto(seeded, params, context); + + assertThat(holder.getName()).isEqualTo("seeded-value"); + assertThat(holder.getUnresolvedCalls()).containsExactly("name"); + } + + public void testResolvesDisabledOntoDisableParams() { + Map params = new HashMap<>(); + params.put("disabled", "true"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.isDisabled()).isTrue(); + } + + public void testUnknownParamIsIgnoredWithoutFailingTheInvocation() { + Map params = new HashMap<>(); + params.put("noSuchParam", "whatever"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getName()).isNull(); + assertThat(holder.getSize()).isNull(); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=LazyParamInjectorTest` +Expected: compilation failure — `resolveInto` does not exist. + +- [ ] **Step 3: Write minimal implementation** + +In `WithLazyParams.java`, add these imports: + +```java +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.util.reflection.ReflectionException; +``` + +Add a logger as the first member of `class LazyParamInjector`: + +```java +private static final Logger LOG = LogManager.getLogger(LazyParamInjector.class); +``` + +Add the new method **below** the existing `injectParams` (leave `injectParams` in place for now): + +```java +/** + * Resolves configured params into a per-invocation holder, leaving the interceptor untouched. + *

+ * A {@code ${...}} expression that cannot be resolved is not written: the holder keeps its + * seeded configuration value and is notified via {@link InterceptorParams#unresolved(String)}, + * so a broken expression cannot silently relax a validation policy. + * + * @since 7.3.0 + */ +public

P resolveInto(P target, Map params, ActionContext invocationContext) { + for (Map.Entry entry : params.entrySet()) { + String paramName = entry.getKey(); + String rawValue = entry.getValue(); + Object paramValue = textParser.evaluate(new char[]{'$'}, rawValue, valueEvaluator, TextParser.DEFAULT_LOOP_COUNT); + + if (isUnresolved(rawValue, paramValue)) { + LOG.warn("Param [{}] of [{}] could not be resolved from expression [{}]; keeping the configured value", + paramName, target.getClass().getName(), rawValue); + target.unresolved(paramName); + continue; + } + try { + // throwPropertyExceptions=true so a param with no matching property on the holder is + // reported rather than silently ignored; OgnlUtil only warns in devMode otherwise + ognlUtil.setProperty(paramName, paramValue, target, invocationContext.getContextMap(), true); + } catch (ReflectionException e) { + LOG.warn("Param [{}] cannot be applied to [{}]; check the interceptor configuration", + paramName, target.getClass().getName(), e); + } + } + return target; +} + +/** + * {@link org.apache.struts2.util.OgnlTextParser} yields an empty string for an expression that + * does not resolve and gives no other signal, so the raw template is needed to tell that apart + * from a legitimately empty value. + */ +private boolean isUnresolved(String rawValue, Object paramValue) { + return rawValue != null + && rawValue.contains("${") + && (paramValue == null || paramValue.toString().isEmpty()); +} +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `mvn test -DskipAssembly -pl core -Dtest=LazyParamInjectorTest` +Expected: PASS, 6 tests. + +- [ ] **Step 5: Confirm nothing else broke** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest,DefaultActionInvocationTest` +Expected: PASS — `injectParams` is untouched, so existing behaviour is unchanged. + +- [ ] **Step 6: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java \ + core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java +git commit -m "WW-5659 feat(core): resolve lazy params into a holder instead of the interceptor" +``` + +--- + +### Task 3: `UploadPolicy` and the `acceptFile` signature + +Pure refactor: the interceptor stops holding three loose fields and holds one config-time `UploadPolicy`; `acceptFile` receives the policy explicitly. **No behaviour change** — the old `injectParams` path still mutates the singleton via the setters, which now delegate into `configuredPolicy`. Task 4 removes that. + +**Files:** +- Create: `core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java` +- Modify: `core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java:62-64, 84-104, 116-166` +- Modify: `core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java:210-264` + +**Interfaces:** +- Consumes: `DisableParams` from Task 1. +- Produces: `class UploadPolicy extends DisableParams` with `setMaximumSize(Long)`, `setAllowedTypes(String)`, `setAllowedExtensions(String)`, `getMaximumSize()`, `getAllowedTypes()`, `getAllowedExtensions()`, `copy()`; and `protected boolean acceptFile(UploadPolicy policy, Object action, UploadedFile file, String originalFilename, String contentType, String inputName)`. + +- [ ] **Step 1: Write the failing test** + +Append to `core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java`, before the closing brace: + +```java + public void testUploadPolicyParsesAndCopies() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes("text/plain, text/html"); + policy.setAllowedExtensions(".txt,.html"); + policy.setMaximumSize(1024L); + policy.setDisabled("true"); + + UploadPolicy copy = policy.copy(); + + assertThat(copy.getAllowedTypes()).containsExactlyInAnyOrder("text/plain", "text/html"); + assertThat(copy.getAllowedExtensions()).containsExactlyInAnyOrder(".txt", ".html"); + assertThat(copy.getMaximumSize()).isEqualTo(1024L); + assertThat(copy.isDisabled()).isTrue(); + } + + public void testUploadPolicyCopyIsIndependentOfTheOriginal() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes("text/plain"); + + UploadPolicy copy = policy.copy(); + copy.setAllowedTypes("text/html"); + + assertThat(policy.getAllowedTypes()).containsExactly("text/plain"); + assertThat(copy.getAllowedTypes()).containsExactly("text/html"); + } + + public void testUploadPolicyTreatsNullAsNoRestriction() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes(null); + policy.setAllowedExtensions(null); + + assertThat(policy.getAllowedTypes()).isEmpty(); + assertThat(policy.getAllowedExtensions()).isEmpty(); + } +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest#testUploadPolicyParsesAndCopies` +Expected: compilation failure — `UploadPolicy` does not exist. + +- [ ] **Step 3: Create `UploadPolicy`** + +Create `core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java` (ASF header first): + +```java +package org.apache.struts2.interceptor; + +import org.apache.struts2.util.TextParseUtil; + +import java.util.Collections; +import java.util.Set; + +/** + * Per-invocation upload validation policy for {@link ActionFileUploadInterceptor}. + *

+ * A configured instance is held by the interceptor and copied for each invocation, so lazily + * resolved values never reach the shared interceptor. + * + * @since 7.3.0 + */ +public class UploadPolicy extends DisableParams { + + private Long maximumSize; + private Set allowedTypes = Collections.emptySet(); + private Set allowedExtensions = Collections.emptySet(); + + public UploadPolicy() { + } + + private UploadPolicy(UploadPolicy other) { + super(other); + this.maximumSize = other.maximumSize; + this.allowedTypes = other.allowedTypes; + this.allowedExtensions = other.allowedExtensions; + } + + /** + * @param allowedTypes a comma-delimited list of content types, or null for no restriction + */ + public void setAllowedTypes(String allowedTypes) { + this.allowedTypes = toSet(allowedTypes); + } + + /** + * @param allowedExtensions a comma-delimited list of extensions, or null for no restriction + */ + public void setAllowedExtensions(String allowedExtensions) { + this.allowedExtensions = toSet(allowedExtensions); + } + + /** + * @param maximumSize the maximum size in bytes, or null for no limit + */ + public void setMaximumSize(Long maximumSize) { + this.maximumSize = maximumSize; + } + + public Long getMaximumSize() { + return maximumSize; + } + + public Set getAllowedTypes() { + return allowedTypes; + } + + public Set getAllowedExtensions() { + return allowedExtensions; + } + + /** + * @return an independent copy, used to seed a per-invocation policy from the configured one + */ + public UploadPolicy copy() { + return new UploadPolicy(this); + } + + private static Set toSet(String commaDelimited) { + return commaDelimited == null + ? Collections.emptySet() + : TextParseUtil.commaDelimitedStringToSet(commaDelimited); + } +} +``` + +Note the null guard: `TextParseUtil.commaDelimitedStringToSet` throws `NullPointerException` on null (`TextParseUtil.java:257` calls `s.split(",")` unguarded), and the old setters had the same defect. Treating null as "no restriction" matches how an empty value already behaves. + +- [ ] **Step 4: Rewrite the policy state in `AbstractFileUploadInterceptor`** + +Replace the three fields at `:62-64`: + +```java + private Long maximumSize; + private Set allowedTypesSet = Collections.emptySet(); + private Set allowedExtensionsSet = Collections.emptySet(); +``` + +with: + +```java + private final UploadPolicy configuredPolicy = new UploadPolicy(); +``` + +Replace the three setters at `:84-104` with delegating versions: + +```java + /** + * Sets the allowed extensions. Applied at configuration time only; the effective policy for + * an invocation is a copy, see {@link ActionFileUploadInterceptor#newLazyParams()}. + * + * @param allowedExtensions A comma-delimited list of extensions + */ + public void setAllowedExtensions(String allowedExtensions) { + configuredPolicy.setAllowedExtensions(allowedExtensions); + } + + /** + * Sets the allowed mimetypes. Applied at configuration time only; the effective policy for + * an invocation is a copy, see {@link ActionFileUploadInterceptor#newLazyParams()}. + * + * @param allowedTypes A comma-delimited list of types + */ + public void setAllowedTypes(String allowedTypes) { + configuredPolicy.setAllowedTypes(allowedTypes); + } + + /** + * Sets the maximum size of an uploaded file. Applied at configuration time only; the + * effective policy for an invocation is a copy, see + * {@link ActionFileUploadInterceptor#newLazyParams()}. + * + * @param maximumSize The maximum size in bytes + */ + public void setMaximumSize(Long maximumSize) { + configuredPolicy.setMaximumSize(maximumSize); + } + + /** + * @return an independent copy of the configured policy, to be resolved for one invocation + * @since 7.3.0 + */ + protected UploadPolicy copyConfiguredPolicy() { + return configuredPolicy.copy(); + } +``` + +- [ ] **Step 5: Change `acceptFile` to take the policy** + +Replace the signature and the three checks at `:116` and `:134-154`: + +```java + protected boolean acceptFile(UploadPolicy policy, Object action, UploadedFile file, String originalFilename, String contentType, String inputName) { +``` + +and inside it: + +```java + if (policy.getMaximumSize() != null && policy.getMaximumSize() < file.length()) { + String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_FILE_TOO_LARGE_KEY, new String[]{ + inputName, originalFilename, file.getName(), "" + file.length(), getMaximumSizeStr(action, policy.getMaximumSize()) + }); + errorMessages.add(errMsg); + LOG.warn(errMsg); + } + if ((!policy.getAllowedTypes().isEmpty()) && (!containsItem(policy.getAllowedTypes(), contentType))) { + String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_CONTENT_TYPE_NOT_ALLOWED_KEY, new String[]{ + inputName, originalFilename, file.getName(), contentType + }); + errorMessages.add(errMsg); + LOG.warn(errMsg); + } + if ((!policy.getAllowedExtensions().isEmpty()) && (!hasAllowedExtension(policy.getAllowedExtensions(), originalFilename))) { + String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_FILE_EXTENSION_NOT_ALLOWED_KEY, new String[]{ + inputName, originalFilename, file.getName(), contentType + }); + errorMessages.add(errMsg); + LOG.warn(errMsg); + } +``` + +Replace `getMaximumSizeStr` at `:164-166`: + +```java + private String getMaximumSizeStr(Object action, Long maximumSize) { + return NumberFormat.getNumberInstance(getLocaleProvider(action).getLocale()).format(maximumSize); + } +``` + +Remove the now-unused `java.util.Collections` import if the compiler flags it. + +- [ ] **Step 6: Update the single caller** + +In `ActionFileUploadInterceptor.java`, at the top of `intercept` (`:210`), after the `UploadedFilesAware` check at `:229`, add: + +```java + UploadPolicy policy = copyConfiguredPolicy(); +``` + +and change the call at `:248`: + +```java + if (acceptFile(policy, action, uploadedFile, uploadedFile.getOriginalName(), uploadedFile.getContentType(), inputName)) { +``` + +- [ ] **Step 7: Run tests to verify they pass** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest` +Expected: PASS — the three new `UploadPolicy` tests plus every pre-existing test, unchanged. If a pre-existing test fails, the refactor changed behaviour and must be corrected, not the test. + +- [ ] **Step 8: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java \ + core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java \ + core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java \ + core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +git commit -m "WW-5659 refactor(core): hold upload policy in one value object" +``` + +--- + +### Task 4: New `WithLazyParams` contract and invocation wiring + +The behaviour change. After this task the interceptor singleton is never written per request. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java` +- Modify: `core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java` +- Modify: `core/src/main/java/org/apache/struts2/DefaultActionInvocation.java:258-276` +- Test: `core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java` + +**Interfaces:** +- Consumes: `resolveInto` (Task 2), `UploadPolicy` and `copyConfiguredPolicy()` (Task 3). +- Produces: `interface WithLazyParams

{ P newLazyParams(); String intercept(ActionInvocation, P) throws Exception; }`; `ActionFileUploadInterceptor implements WithLazyParams`. + +- [ ] **Step 1: Write the failing tests** + +Append to `ActionFileUploadInterceptorTest`, before the closing brace. The first test is carried over from PR #1815 by @deprrous, adapted to the new contract — keep the attribution comment. + +```java + /** + * Regression for WW-5659: two concurrent invocations resolving different policies must not + * see each other's values. Scenario contributed by @deprrous in GitHub PR #1815. + */ + public void testConcurrentDynamicPoliciesStayIsolatedPerRequest() throws Exception { + CoordinatedActionFileUploadInterceptor sharedInterceptor = new CoordinatedActionFileUploadInterceptor(); + container.inject(sharedInterceptor); + + MyDynamicFileUploadAction plainPolicyAction = new MyDynamicFileUploadAction(); + plainPolicyAction.setAllowedMimeTypes("text/plain"); + container.inject(plainPolicyAction); + + MyDynamicFileUploadAction htmlPolicyAction = new MyDynamicFileUploadAction(); + htmlPolicyAction.setAllowedMimeTypes("text/html"); + container.inject(htmlPolicyAction); + + ExecutorService executor = Executors.newFixedThreadPool(2); + try { + Future plainResult = executor.submit(() -> runUploadAttempt( + sharedInterceptor, plainPolicyAction, createUploadRequest("plain-policy.html", "text/html", htmlContent))); + + assertThat(sharedInterceptor.awaitFirstValidation()).isTrue(); + + Future htmlResult = executor.submit(() -> runUploadAttempt( + sharedInterceptor, htmlPolicyAction, createUploadRequest("html-policy.html", "text/html", htmlContent))); + + assertThat(htmlResult.get(10, TimeUnit.SECONDS)).isEqualTo("success"); + sharedInterceptor.releaseFirstValidation(); + assertThat(plainResult.get(10, TimeUnit.SECONDS)).isEqualTo("success"); + } finally { + sharedInterceptor.releaseFirstValidation(); + executor.shutdownNow(); + sharedInterceptor.destroy(); + } + + // the text/plain policy must have rejected the text/html upload despite the concurrent + // text/html invocation resolving a more permissive policy on the same interceptor + assertThat(plainPolicyAction.getUploadFiles()).isNull(); + assertThat(plainPolicyAction.getFieldErrors()).containsKey("file"); + + assertThat(htmlPolicyAction.hasFieldErrors()).isFalse(); + assertThat(htmlPolicyAction.getUploadFiles()).isNotNull().hasSize(1); + assertThat(htmlPolicyAction.getUploadFiles().get(0).getOriginalName()).isEqualTo("html-policy.html"); + } + + /** + * Regression for WW-5659: resolving an invocation's params must leave the interceptor + * singleton exactly as configured. + */ + public void testResolutionDoesNotMutateTheInterceptor() throws Exception { + ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); + container.inject(interceptor); + interceptor.setAllowedTypes("text/plain"); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes("text/html"); + container.inject(action); + + runUploadAttempt(interceptor, action, createUploadRequest("f.html", "text/html", htmlContent)); + + assertThat(interceptor.newLazyParams().getAllowedTypes()).containsExactly("text/plain"); + } + + /** + * Regression for WW-5659: a lazily resolved {@code disabled} must apply to one invocation only. + */ + public void testDisabledIsResolvedPerInvocation() throws Exception { + ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); + container.inject(interceptor); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes("text/plain"); + container.inject(action); + + UploadPolicy policy = interceptor.newLazyParams(); + policy.setDisabled("true"); + + assertThat(policy.isDisabled()).isTrue(); + assertThat(interceptor.newLazyParams().isDisabled()).isFalse(); + } +``` + +Add these helpers — none of them exist in the test class yet. `runUploadAttempt`, `createUploadRequest`, the `MockHttpServletRequest` overload of `createMultipartRequest` and `CoordinatedActionFileUploadInterceptor` all originate in PR #1815 by @deprrous and are reproduced here, adapted to the new contract, so this task is self-contained: + +```java + private String runUploadAttempt(ActionFileUploadInterceptor actionFileUploadInterceptor, + MyDynamicFileUploadAction action, + MockHttpServletRequest uploadRequest) throws Exception { + MultiPartRequestWrapper multiPartRequest = createMultipartRequest(uploadRequest, -1, -1, 3, -1); + ValueStack valueStack = container.getInstance(ValueStackFactory.class).createValueStack(); + valueStack.push(action); + + ActionContext context = ActionContext.of(valueStack.getContext()) + .withContainer(container) + .withValueStack(valueStack) + .withServletRequest(multiPartRequest) + .bind(); + try { + MockActionInvocation invocation = new MockActionInvocation(); + invocation.setAction(action); + invocation.setResultCode("success"); + invocation.setInvocationContext(context); + + Map params = new HashMap<>(); + params.put("allowedTypes", "${allowedMimeTypes}"); + + WithLazyParams.LazyParamInjector injector = new WithLazyParams.LazyParamInjector(valueStack); + container.inject(injector); + UploadPolicy policy = injector.resolveInto(actionFileUploadInterceptor.newLazyParams(), params, context); + + return actionFileUploadInterceptor.intercept(invocation, policy); + } finally { + ActionContext.clear(); + } + } + + private MockHttpServletRequest createUploadRequest(String filename, String contentType, String content) { + MockHttpServletRequest uploadRequest = new MockHttpServletRequest(); + uploadRequest.setCharacterEncoding(StandardCharsets.UTF_8.name()); + uploadRequest.setMethod("POST"); + uploadRequest.addHeader("Content-type", "multipart/form-data; boundary=\"" + boundary + "\""); + uploadRequest.setContent((encodeTextFile(filename, contentType, content) + endLine + "--" + boundary + "--") + .getBytes(StandardCharsets.UTF_8)); + return uploadRequest; + } + + private MultiPartRequestWrapper createMultipartRequest(MockHttpServletRequest multipartRequest, int maxsize, int maxfilesize, int maxfiles, int maxStringLength) { + JakartaMultiPartRequest jak = new JakartaMultiPartRequest(); + jak.setMaxSize(String.valueOf(maxsize)); + jak.setMaxFileSize(String.valueOf(maxfilesize)); + jak.setMaxFiles(String.valueOf(maxfiles)); + jak.setMaxStringLength(String.valueOf(maxStringLength)); + jak.setDefaultEncoding(StandardCharsets.UTF_8.name()); + return new MultiPartRequestWrapper(jak, multipartRequest, tempDir.getAbsolutePath(), new DefaultLocaleProvider()); + } + + /** Pauses the first validation so a second invocation can overlap it. From PR #1815 by @deprrous. */ + private static final class CoordinatedActionFileUploadInterceptor extends ActionFileUploadInterceptor { + private final AtomicBoolean pauseFirstValidation = new AtomicBoolean(true); + private final CountDownLatch firstValidationEntered = new CountDownLatch(1); + private final CountDownLatch allowFirstValidationToContinue = new CountDownLatch(1); + + @Override + protected boolean acceptFile(UploadPolicy policy, Object action, UploadedFile file, String originalFilename, String contentType, String inputName) { + if (pauseFirstValidation.compareAndSet(true, false)) { + firstValidationEntered.countDown(); + awaitUnchecked(allowFirstValidationToContinue); + } + return super.acceptFile(policy, action, file, originalFilename, contentType, inputName); + } + + private boolean awaitFirstValidation() throws InterruptedException { + return firstValidationEntered.await(10, TimeUnit.SECONDS); + } + + private void releaseFirstValidation() { + allowFirstValidationToContinue.countDown(); + } + + private void awaitUnchecked(CountDownLatch latch) { + try { + if (!latch.await(10, TimeUnit.SECONDS)) { + throw new AssertionError("Timed out waiting for concurrent validation release"); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new AssertionError("Interrupted while waiting for concurrent validation release", e); + } + } + } +``` + +To avoid duplicating the body, change the existing no-request `createMultipartRequest(int, int, int, int)` to delegate to the new overload: + +```java + private MultiPartRequestWrapper createMultipartRequest(int maxsize, int maxfilesize, int maxfiles, int maxStringLength) { + return createMultipartRequest(request, maxsize, maxfilesize, maxfiles, maxStringLength); + } +``` + +Required test imports: + +```java +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.ValueStackFactory; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest#testResolutionDoesNotMutateTheInterceptor` +Expected: compilation failure — `newLazyParams()` and the two-arg `intercept` do not exist. + +- [ ] **Step 3: Change the `WithLazyParams` interface** + +Replace the interface declaration in `WithLazyParams.java` (keep `LazyParamInjector` nested inside it unchanged apart from the deletion below): + +```java +public interface WithLazyParams

{ + + /** + * @return a fresh holder for one invocation, seeded from the configured values + * @since 7.3.0 + */ + P newLazyParams(); + + /** + * Invoked in place of {@link Interceptor#intercept(ActionInvocation)} when lazy params apply. + * + * @param lazyParams params resolved for this invocation only + * @since 7.3.0 + */ + String intercept(ActionInvocation invocation, P lazyParams) throws Exception; +``` + +Add the import `org.apache.struts2.ActionInvocation` and delete the old `injectParams` method entirely. + +- [ ] **Step 4: Implement the contract on the interceptor** + +In `ActionFileUploadInterceptor.java`, change the class declaration: + +```java +public class ActionFileUploadInterceptor extends AbstractFileUploadInterceptor implements WithLazyParams { +``` + +Add, above `intercept`: + +```java + @Override + public UploadPolicy newLazyParams() { + return copyConfiguredPolicy(); + } + + @Override + public String intercept(ActionInvocation invocation) throws Exception { + return intercept(invocation, newLazyParams()); + } +``` + +Change the existing `intercept` to the two-arg form, replacing the `UploadPolicy policy = copyConfiguredPolicy();` line added in Task 3: + +```java + @Override + public String intercept(ActionInvocation invocation, UploadPolicy policy) throws Exception { +``` + +- [ ] **Step 5: Wire `DefaultActionInvocation`** + +Replace `:258-276` with: + +```java + if (interceptors.hasNext()) { + final InterceptorMapping interceptorMapping = interceptors.next(); + Interceptor interceptor = interceptorMapping.getInterceptor(); + if (interceptor instanceof WithLazyParams lazyInterceptor) { + resultCode = invokeWithLazyParams(lazyInterceptor, interceptorMapping); + } else if (interceptor instanceof ConditionalInterceptor conditionalInterceptor) { + resultCode = executeConditional(conditionalInterceptor); + } else { + LOG.debug("Executing normal interceptor: {}", interceptorMapping.getName()); + resultCode = interceptor.intercept(this); + } + } else { + resultCode = invokeActionOnly(); + } +``` + +Add these two methods next to `executeConditional`: + +```java + /** + * Resolves lazy params into a per-invocation holder and dispatches to the interceptor. + *

+ * {@link org.apache.struts2.interceptor.AbstractInterceptor} implements + * {@link ConditionalInterceptor}, so a lazy interceptor is normally conditional too; both the + * 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

String invokeWithLazyParams( + WithLazyParams

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()); + return this.invoke(); + } + if (lazyInterceptor instanceof ConditionalInterceptor conditionalInterceptor + && !conditionalInterceptor.shouldIntercept(this)) { + LOG.debug("Interceptor: {} is disabled, skipping to next", interceptorMapping.getName()); + return this.invoke(); + } + LOG.debug("Executing lazy params interceptor: {}", interceptorMapping.getName()); + return lazyInterceptor.intercept(this, lazyParams); + } + + /** + * @return a fresh map; the mapping's own param map is shared across requests and must not be mutated + */ + private Map mergedParams(InterceptorMapping interceptorMapping) { + Map merged = new HashMap<>(interceptorMapping.getParams()); + proxy.getConfig().getInterceptors().stream() + .filter(im -> im.getName().equals(interceptorMapping.getName())) + .findFirst() + .ifPresent(im -> merged.putAll(im.getParams())); + return merged; + } +``` + +Add `import java.util.HashMap;` if not present. + +- [ ] **Step 6: Run the new tests** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest` +Expected: PASS, including the three new regressions. + +- [ ] **Step 7: Run the wider suite** + +Run: `mvn test -DskipAssembly -pl core` +Expected: PASS. `DefaultActionInvocationTest` exercises the rewritten `invoke()` branch and must stay green. + +- [ ] **Step 8: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java \ + core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java \ + core/src/main/java/org/apache/struts2/DefaultActionInvocation.java \ + core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +git commit -m "WW-5659 fix(core): resolve lazy interceptor params per invocation + +Co-Authored-By: deprrous " +``` + +--- + +### Task 5: Fail closed on unresolvable expressions + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java` +- Modify: `core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java` +- Modify: `core/src/main/resources/org/apache/struts2/struts-messages.properties` +- Test: `core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java` + +**Interfaces:** +- Consumes: `InterceptorParams.unresolved(String)` (Task 1), `resolveInto` (Task 2), `UploadPolicy` (Task 3). +- Produces: `UploadPolicy.isUnresolved()`, `UploadPolicy.getUnresolvedParams()`; constant `STRUTS_MESSAGES_ERROR_UPLOAD_POLICY_UNRESOLVED_KEY`. + +Implements the deterministic rule from the *Deviation from the spec* section: any param reported unresolved marks the policy unusable, regardless of the seeded value. + +- [ ] **Step 1: Write the failing test** + +Append to `ActionFileUploadInterceptorTest`: + +```java + public void testUnresolvedPolicyRejectsTheUpload() throws Exception { + ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); + container.inject(interceptor); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes(null); // ${allowedMimeTypes} will not resolve + container.inject(action); + + runUploadAttempt(interceptor, action, createUploadRequest("f.txt", "text/plain", plainContent)); + + assertThat(action.getUploadFiles()).isNull(); + assertThat(action.getFieldErrors()).containsKey("file"); + } + + public void testResolvedPolicyStillAcceptsTheUpload() throws Exception { + ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); + container.inject(interceptor); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes("text/plain"); + container.inject(action); + + runUploadAttempt(interceptor, action, createUploadRequest("f.txt", "text/plain", plainContent)); + + assertThat(action.hasFieldErrors()).isFalse(); + assertThat(action.getUploadFiles()).isNotNull().hasSize(1); + } + + public void testUploadPolicyTracksUnresolvedParams() { + UploadPolicy policy = new UploadPolicy(); + assertThat(policy.isUnresolved()).isFalse(); + + policy.unresolved("allowedTypes"); + + assertThat(policy.isUnresolved()).isTrue(); + assertThat(policy.getUnresolvedParams()).containsExactly("allowedTypes"); + } +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest#testUploadPolicyTracksUnresolvedParams` +Expected: compilation failure — `isUnresolved()` does not exist. + +- [ ] **Step 3: Track unresolved params on the policy** + +In `UploadPolicy.java` add the field, copy it, and override `unresolved`: + +```java + private final Set unresolvedParams = new LinkedHashSet<>(); +``` + +In the copy constructor add: + +```java + this.unresolvedParams.addAll(other.unresolvedParams); +``` + +and add: + +```java + /** + * 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. + */ + @Override + public void unresolved(String paramName) { + unresolvedParams.add(paramName); + } + + public boolean isUnresolved() { + return !unresolvedParams.isEmpty(); + } + + public Set getUnresolvedParams() { + return Collections.unmodifiableSet(unresolvedParams); + } +``` + +Add imports `java.util.LinkedHashSet`. + +- [ ] **Step 4: Reject in `acceptFile`** + +In `AbstractFileUploadInterceptor.java` add the key constant next to the others at `:47-60`: + +```java + public static final String STRUTS_MESSAGES_ERROR_UPLOAD_POLICY_UNRESOLVED_KEY = "struts.messages.error.upload.policy.unresolved"; +``` + +In `acceptFile`, immediately after the existing missing-file check that returns false at `:125-132`, add: + +```java + if (policy.isUnresolved()) { + String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_UPLOAD_POLICY_UNRESOLVED_KEY, new String[]{ + inputName, originalFilename, String.join(", ", policy.getUnresolvedParams()) + }); + if (validation != null) { + validation.addFieldError(inputName, errMsg); + } + LOG.warn(errMsg); + return false; + } +``` + +- [ ] **Step 5: Add the message** + +Append to `core/src/main/resources/org/apache/struts2/struts-messages.properties`: + +```properties +struts.messages.error.upload.policy.unresolved=The upload validation policy could not be resolved, rejecting the file: {0} "{1}"; unresolved parameters: {2} +``` + +Base bundle only — the locale bundles are partial and fall back to it. + +- [ ] **Step 6: Run tests to verify they pass** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest` +Expected: PASS. + +- [ ] **Step 7: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java \ + core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java \ + core/src/main/resources/org/apache/struts2/struts-messages.properties \ + core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +git commit -m "WW-5659 fix(core): reject uploads when the policy cannot be resolved" +``` + +--- + +### Task 6: Migrate legacy tests, javadoc, and spec alignment + +**Files:** +- Modify: `core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java` +- Modify: `core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java:70-204` (class javadoc) +- Modify: `docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md` + +**Interfaces:** +- Consumes: everything from Tasks 1–5. +- Produces: nothing new. + +- [ ] **Step 1: Migrate the setter-shortcut tests** + +Five tests simulate injection by calling a setter directly — `testDynamicParameterEvaluation`, `testDynamicParametersChangePerRequest`, `testDynamicExtensionValidation`, `testDynamicMaximumSizeValidation`, `testSecurityValidationWithDynamicParameters`, `testWildcardMatchingWithDynamicParameters`. Each contains a line of the form `interceptor.setAllowedTypes(action.getAllowedMimeTypes());` followed by `interceptor.intercept(mai);`. + +Replace each such pair with a real resolution through the injector, e.g.: + +```java + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, false, false); + interceptor.intercept(mai, policy); +``` + +and add the helper: + +```java + private UploadPolicy injectDynamicUploadPolicy(ActionFileUploadInterceptor actionFileUploadInterceptor, + ActionContext context, + boolean includeAllowedTypes, + boolean includeAllowedExtensions, + boolean includeMaximumSize) { + Map params = new HashMap<>(); + if (includeAllowedTypes) { + params.put("allowedTypes", "${allowedMimeTypes}"); + } + if (includeAllowedExtensions) { + params.put("allowedExtensions", "${allowedExtensions}"); + } + if (includeMaximumSize) { + params.put("maximumSize", "${maxFileSize}"); + } + + WithLazyParams.LazyParamInjector injector = new WithLazyParams.LazyParamInjector(context.getValueStack()); + container.inject(injector); + return injector.resolveInto(actionFileUploadInterceptor.newLazyParams(), params, context); + } +``` + +Set the flags per test to match the param that test previously set by hand: `allowedTypes` for the four type/wildcard tests, `allowedExtensions` for `testDynamicExtensionValidation`, `maximumSize` for `testDynamicMaximumSizeValidation`, and both types and extensions for `testSecurityValidationWithDynamicParameters`. + +- [ ] **Step 2: Run the suite** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest` +Expected: PASS. These tests now exercise the real resolution path rather than a hand-called setter. + +- [ ] **Step 3: Update the interceptor javadoc** + +`ActionFileUploadInterceptor`'s class javadoc at `:70-204` documents the dynamic-parameter feature with a `uploadConfig.setAllowedExtensions(".jpg,.png")` example. Add a paragraph directly above `@see WithLazyParams`: + +```java + *

+ * Dynamic parameters are resolved into a fresh {@link UploadPolicy} for each invocation, so the + * interceptor itself is never modified per request and concurrent uploads cannot observe each + * other's policy. An expression that cannot be resolved does not relax validation: the policy is + * marked unresolved and affected uploads are rejected. +``` + +- [ ] **Step 4: Align the spec with the implemented rule** + +In the spec's *Error handling* section, replace the two-case "genuine static fallback / no fallback" rule and the sentence beginning "The rule: `unresolved(param)` marks the dimension unusable **only if**..." with: + +```markdown +`UploadPolicy.unresolved(param)` records the parameter and marks the whole policy unusable, +regardless of the seeded value. A static fallback therefore applies only when the param is absent +from the lazy map entirely, i.e. pure static configuration, which never triggers `unresolved`. + +The seed-introspection alternative — honouring a seeded value that does not itself contain +`${` — was rejected during planning: it is not implementable deterministically, because +`maximumSize` is seeded as a `Long` and cannot carry a `${...}` literal, so the rule would behave +differently per parameter type. +``` + +Update the corresponding Testing bullet to drop the static-fallback case. + +- [ ] **Step 5: Full build** + +Run: `mvn test -DskipAssembly` +Expected: PASS across all modules. Plugins do not implement `WithLazyParams` (verified: `ActionFileUploadInterceptor` is the only implementer), but the interface change is compile-visible, so a full build is the check. + +- [ ] **Step 6: Commit** + +```bash +git add core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java \ + core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java \ + docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md +git commit -m "WW-5659 test(core): exercise real lazy param resolution in dynamic upload tests" +``` + +--- + +## Done criteria + +- `mvn test -DskipAssembly` passes across all modules. +- `ActionFileUploadInterceptor` has no mutable state written after `init()`; `testResolutionDoesNotMutateTheInterceptor` proves it. +- No `ThreadLocal`, no `finally` cleanup, and no per-request write to any shared map remains on the lazy path. +- The spec and the implementation agree on the unresolved-param rule. diff --git a/docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md b/docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md new file mode 100644 index 0000000000..9f2566b976 --- /dev/null +++ b/docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md @@ -0,0 +1,368 @@ +# WW-5659: Request-scoped resolution of lazy interceptor params + +**Jira:** [WW-5659](https://issues.apache.org/jira/browse/WW-5659) +**Type:** Bug +**Affects:** 7.2.0, 7.2.1 +**Fix version:** 7.3.0 +**Date:** 2026-07-27 +**Reported via:** GitHub PR [#1815](https://github.com/apache/struts/pull/1815) (approach not adopted) + +## Problem + +`WithLazyParams.LazyParamInjector#injectParams` resolves `${...}` interceptor params +once per request and writes the resolved values straight onto the interceptor: + +```java +// WithLazyParams.java:78-84 +public Interceptor injectParams(Interceptor interceptor, Map params, ActionContext invocationContext) { + for (Map.Entry entry : params.entrySet()) { + Object paramValue = textParser.evaluate(new char[]{'$'}, entry.getValue(), valueEvaluator, TextParser.DEFAULT_LOOP_COUNT); + ognlUtil.setProperty(entry.getKey(), paramValue, interceptor, invocationContext.getContextMap()); + } + return interceptor; +} +``` + +That interceptor is a singleton, built once at configuration-parse time +(`InterceptorBuilder.java:73-74`, `:175-177`) and reused for every request. When an +action references a stack without overriding params, `InterceptorBuilder.java:79` +(`result.addAll(stackConfig.getInterceptors())`) hands the same `InterceptorMapping` +objects to every such action, so the instance is shared across actions too. + +Request-scoped state is therefore written to process-wide state with no synchronisation. + +### Consequence for file upload validation + +`ActionFileUploadInterceptor` is the only `WithLazyParams` implementer today. Its +resolved policy lands in plain instance fields (`AbstractFileUploadInterceptor.java:62-64`, +written at `:85`, `:94`, `:103`) and is read back by `acceptFile` (`:134`, `:141`, `:148`). + +Per request: + +- `DefaultActionInvocation.java:269` — resolve, write the shared fields +- `DefaultActionInvocation.java:275` — `intercept()` → `acceptFile()` reads the shared fields + +Nothing guards the interval. Two concurrent requests resolving different policies can have +their `allowedTypes`, `allowedExtensions` and `maximumSize` cross over, so a request can be +validated against another request's upload policy. + +### `disabled` is affected too + +`AbstractInterceptor.java:28` holds `disabled` as a shared `boolean`, written by +`setDisabled` (`:56-58`) and read by `shouldIntercept` (`:61-63`). It travels through +`injectParams` like any other param, so `${...}` has the +same cross-request bleed — and it decides whether the interceptor runs at all. + +Scope note: for interceptors that are *not* `WithLazyParams`, `setDisabled` is only ever +called at config time by `buildInterceptor`, never per request. `disabled` is racy only on +the lazy path. + +### Secondary defect + +`DefaultActionInvocation.java:262-267` calls `params.putAll(...)` on the map returned by +`InterceptorMapping#getParams` (`:60-62`), which is the live shared map — an unsynchronised +write to a shared `HashMap` on every request. + +### Not affected + +`struts-default.xml:60` declares `actionFileUpload` with no params, so a default +configuration resolves nothing and writes nothing. Static (non-expression) params resolve +to the same value on every request. Only applications opting into the dynamic `${...}` +form added in WW-5585 are affected. + +`actionFileUpload` sits at position 15 of `defaultStack`, ahead of `staticParams` (19), +`actionMappingParams` (20) and `params` (21), so no request parameter has been bound to the +action at resolution time — the expression's *value* is not attacker-supplied. Which policy +is selected can still depend on request-derived state populated by `prepare` (11), +`scopedModelDriven` (13) or `modelDriven` (14); the shipped showcase does exactly that in +`DynamicFileUploadAction.java:136-139`. + +This was assessed as a thread-safety defect rather than a framework vulnerability. Released +7.2.0 and 7.2.1 are not being backported. + +## Decision + +Fix the `WithLazyParams` contract rather than patching its one implementer, so no future +implementer can reintroduce the bug. Resolved params are written into a per-invocation +object the interceptor supplies and receives back; the interceptor singleton stays +immutable after `init()`. + +Rejected alternatives: + +- **Per-invocation copy of the interceptor** (`copyForInvocation()`): smallest diff, but a + copy constructor rots silently when a field is added, it copies injected container + dependencies, and it creates transient instances that never receive `destroy()`. It hides + the mutable snapshot rather than separating config-time from request-time state. +- **Interceptor-owned resolution** (framework stops injecting, interceptor resolves raw + templates itself): loses OGNL type conversion, duplicates resolution in every + implementer, and silently changes what the setters mean. +- **`ThreadLocal` on the interceptor** (PR #1815): makes setter behaviour depend on hidden + thread state, requires a cleanup call that any implementer can forget, and leaves the + unsafe write path in place, merely bypassed. + +This is a clean interface break in the next minor. `WithLazyParams` is public API since +2.5.9, but `ActionFileUploadInterceptor` is its only implementer in the repo; third-party +implementers get a compile error rather than silent breakage, noted in the migration guide. + +## Components + +### `InterceptorParams` (new) + +`org.apache.struts2.interceptor.InterceptorParams` — the general contract for a params +holder, and the generic bound for `WithLazyParams`. + +```java +public interface InterceptorParams { + /** Notified when a {@code ${...}} param could not be resolved for this invocation. */ + default void unresolved(String paramName) { } +} +``` + +The default no-op keeps the interface free for implementers that do not care. `UploadPolicy` +overrides it to support fail-closed validation. + +### `DisableParams` (new) + +`org.apache.struts2.interceptor.DisableParams` — opt-in support for the `disabled` param. +A class, not an interface, because it holds state and needs a setter for OGNL. + +```java +public class DisableParams implements InterceptorParams { + private boolean disabled; + public void setDisabled(String disable) { this.disabled = Boolean.parseBoolean(disable); } + public boolean isDisabled() { return disabled; } +} +``` + +`disabled` is universal across interceptors while lazy resolution is rare, so `DisableParams` +is *not* a subtype of any lazy-specific type; both sit under `InterceptorParams`. + +A holder that does not extend `DisableParams` has no `disabled` property, so a `disabled` +param on such an interceptor resolves to nothing and is reported by the unknown-param +warning below. There is deliberately no fallback to the singleton — that is the racy path +being removed. Interceptors supporting `disabled` must extend `DisableParams`. + +### `WithLazyParams` (changed) + +```java +public interface WithLazyParams

{ + P newLazyParams(); + String intercept(ActionInvocation invocation, P lazyParams) throws Exception; +} +``` + +`AbstractInterceptor.java:48` declares `intercept(ActionInvocation)` abstract, and a +class-declared abstract method takes precedence over an interface default, so a `default` +single-arg implementation on this interface would not satisfy it. Implementers write the +one-line delegation themselves. + +### `LazyParamInjector` (changed) + +`injectParams(Interceptor, ...)` becomes `resolveInto(InterceptorParams target, ...)`. It +still uses `textParser.evaluate` followed by `ognlUtil.setProperty`, so OGNL type conversion +against the holder's typed setters is preserved (`maximumSize` still converts `String` → +`Long`). + +Two behaviours are added: + +- **Unresolved detection.** `OgnlTextParser.java:85-88` yields `""` for an expression that + does not resolve and gives no signal distinguishing that from a legitimate empty value. + The injector holds the raw template, so `raw.contains("${") && resolved.isEmpty()` + recovers it. On unresolved: skip the write (the holder keeps its seeded config-time + value), call `target.unresolved(paramName)`, and log a WARN naming interceptor, param and + expression. +- **Unknown-param warning.** A param with no matching property on the holder currently + no-ops silently, because `ognlUtil.setProperty` swallows the `OgnlException` + (`OgnlUtil.java:297-299`). Set `throwPropertyExceptions=true`, log a WARN, and call + `unresolved(paramName)` — see the general rule under *Error handling*. Not a regression in + detection — a typo no-ops today too — but this design makes it more likely to matter. + +### `DefaultActionInvocation` (changed) + +`AbstractInterceptor.java:26` implements `ConditionalInterceptor`, so every interceptor +extending it — `ActionFileUploadInterceptor` included — takes the `instanceof +ConditionalInterceptor` branch at `:271` and is invoked through `executeConditional` → +`conditionalInterceptor.intercept(this)` at `:318`. That works today only because +`injectParams` had already mutated the singleton. Under the new contract the lazy and +conditional paths must be merged, or the single-arg `intercept` would run with unresolved +values and the dynamic policy would silently vanish. + +```java +private

String invokeWithLazyParams(WithLazyParams

lazy, + InterceptorMapping mapping) throws Exception { + P params = lazy.newLazyParams(); + lazyParamInjector.resolveInto(params, mergedParams(mapping), invocationContext); + if (params instanceof DisableParams dp && dp.isDisabled()) { + return invoke(); + } + if (lazy instanceof ConditionalInterceptor ci && !ci.shouldIntercept(this)) { + return invoke(); + } + return lazy.intercept(this, params); +} +``` + +Checking both `isDisabled()` and `shouldIntercept` keeps custom `shouldIntercept` overrides +working while making the lazily-resolved `disabled` request-scoped. The singleton's +`disabled` field is never written on this path, so `AbstractInterceptor.shouldIntercept` +returns `true` and the holder is authoritative. + +`mergedParams(mapping)` returns a fresh map instead of mutating the shared one, retiring the +`putAll` at `:262-267`. + +### `UploadPolicy` (new) + +`org.apache.struts2.interceptor.UploadPolicy` — top-level, not nested, because it appears in +the interceptor's public signature. + +```java +public class UploadPolicy extends DisableParams { + private Long maximumSize; + private Set allowedTypes = Collections.emptySet(); + private Set allowedExtensions = Collections.emptySet(); + // typed setters (OGNL converts String -> Long for maximumSize), getters, copy() + // overrides unresolved(String) to record dimensions that could not be resolved +} +``` + +### `AbstractFileUploadInterceptor` / `ActionFileUploadInterceptor` (changed) + +The interceptor holds a single config-time `UploadPolicy` rather than keeping the three +fields at `:62-64` *and* adding a holder: + +```java +private final UploadPolicy configuredPolicy = new UploadPolicy(); + +public void setAllowedTypes(String csv) { configuredPolicy.setAllowedTypes(csv); } // config-time only +// ... +@Override public UploadPolicy newLazyParams() { return configuredPolicy.copy(); } +``` + +Public setter signatures are unchanged, so static `struts.xml` config and `buildInterceptor` +reflection are untouched. They are documented as config-time only, which is what they +already are — no framework code calls them outside OGNL injection. + +`acceptFile` takes the policy explicitly, and `getMaximumSizeStr` (`:164-166`) takes the +value rather than reading a field: + +```java +protected boolean acceptFile(UploadPolicy policy, Object action, UploadedFile file, + String originalFilename, String contentType, String inputName) +``` + +This is a `protected` break for any subclass overriding `acceptFile`. + +`ActionFileUploadInterceptor implements WithLazyParams`; its single-arg +`intercept` delegates to `intercept(invocation, newLazyParams())` so direct use outside the +lazy path still works. + +## Data flow + +**Config time (once at startup)** — unchanged. `InterceptorBuilder` → +`objectFactory.buildInterceptor(config, params)` reflects params onto the singleton, +populating `configuredPolicy` with literal values including any raw `${...}` text. +`InterceptorMapping` stores the raw param map. + +**Request time (per invocation)** + +1. `DefaultActionInvocation.invoke()` takes the next `InterceptorMapping`; + `instanceof WithLazyParams` routes to `invokeWithLazyParams`. +2. `mergedParams(mapping)` builds a fresh map. +3. `lazy.newLazyParams()` → `configuredPolicy.copy()`. +4. `lazyParamInjector.resolveInto(holder, merged, invocationContext)`. +5. `disabled` check, then `shouldIntercept`. +6. `lazy.intercept(invocation, holder)`. +7. The holder becomes garbage when the invocation ends. + +Step 7 is the design's own check: **there is no cleanup step.** No `finally`, nothing to +clear, no `ThreadLocal` to leak. If a future change makes cleanup necessary, the separation +has regressed. + +## Error handling + +Unresolvable `${...}` is **fail-closed**. Today it produces `""` +(`OgnlTextParser.java:85-88`) → empty set (`TextParseUtil.java:257`) → treated as "no +restriction" by `acceptFile` (`:141`, `:148` both guard on `!isEmpty()`), so a typo or a +null intermediate silently switches off an upload restriction. + +Under this design: + +- The write is skipped, so the holder keeps its seeded config-time value, and + `unresolved(paramName)` is called. +- A WARN is logged naming interceptor, param and expression. + +**The rule is general: every path in `resolveInto` that skips a write must notify the holder +via `unresolved(paramName)`.** Any skipped write leaves the holder reporting a value the +configuration did not ask for, and a holder that is not told cannot fail closed — it validates +against a dimension that was silently dropped, which is precisely the failure this ticket +exists to remove. There are three such paths: + +1. **Unresolvable or empty `${...}`** — detected by `isUnresolved`, as above. +2. **A resolved value the holder's setter cannot accept** — e.g. `${uploadConfig.maxFileSize}` + returning a non-numeric `String` for the `Long` `maximumSize`. OGNL conversion throws + `ReflectionException` (`resolveInto` sets `throwPropertyExceptions=true`), the write is + skipped, and `maximumSize` stays `null` — which `acceptFile` reads as "no size limit". Left + unnotified this branch is fail-*open*, so it must call `unresolved` too. +3. **A param with no matching property on the holder** — the same `ReflectionException` + (`NoSuchPropertyException`) from a typo'd param name. This is now rejected at configuration + time: `DefaultInterceptorFactory` checks every *interceptor-ref* param of a `WithLazyParams` + interceptor against its holder type and throws `ConfigurationException` if it is not writable + there. Only the ref params are checked, because only those become + `InterceptorMapping#getParams()` and reach `resolveInto`; params on the `` + definition are applied to the interceptor instance and never reach the holder, so requiring + them on the holder would reject working configurations. The runtime notification stays as + defence in depth for holders and mappings built outside the factory. + +A new failure mode added to `resolveInto` later must be checked against this rule. + +`UploadPolicy.unresolved(param)` records the parameter and marks the whole policy unusable, +regardless of the seeded value. A static fallback therefore applies only when the param is absent +from the lazy map entirely, i.e. pure static configuration, which never triggers `unresolved`. + +The seed-introspection alternative — honouring a seeded value that does not itself contain +`${` — was rejected during planning: it is not implementable deterministically, because +`maximumSize` is seeded as a `Long` and cannot carry a `${...}` literal, so the rule would behave +differently per parameter type. + +A dimension marked unusable causes `acceptFile` to reject the file with a dedicated message +rather than the opaque one produced by matching content types against literal `${...}` text. +This needs a new bundle key — `struts.messages.error.upload.policy.unresolved` — added to +the shipped properties. + +This is a behaviour change for 7.2.x applications with a broken expression. Those +applications are currently running with that validation silently disabled, which is the +reason to surface it. + +The configuration-time param check is a behaviour change of its own and needs a release note: +an application carrying a latent typo in a `` param of a `WithLazyParams` +interceptor now fails to start instead of failing every affected request. That is the point — +the alternative is an outage that only shows up under upload traffic — but it is a startup +failure where there was none. + +## Testing + +`ActionFileUploadInterceptorTest` is JUnit 3 style — `protected void setUp()`, plain +`public void testX()` methods, no annotations. A JUnit 5 `@Test` added there compiles and +silently never runs. New tests must follow the existing style. + +- Carry over PR #1815's concurrency regression, adapted to the new signature, retaining the + contributor's attribution. Its scenario is the acceptance criterion. +- Direct guard for the defect: after an invocation resolves a policy, assert + `newLazyParams()` still returns the configured values — the singleton was never written. +- `disabled` request-scoping: concurrent invocations resolving different `disabled` values; + assert only the intended one is skipped. +- Fail-closed: unresolved expression → rejected with the new message. +- `ConditionalInterceptor` interaction: a `WithLazyParams` interceptor that is also + conditional still honours a custom `shouldIntercept`. +- Migrate existing dynamic tests (`testDynamicParameterEvaluation` and friends) off the + "simulate injection by calling the setter directly" shortcut onto real `LazyParamInjector` + calls. PR #1815 already started this. + +## Out of scope + +Defining *all* interceptor params via dedicated params classes, rather than loose setters on +each interceptor, is the natural extension of `InterceptorParams`. It would touch 44 +interceptor implementations across core and plugins plus every third-party interceptor, so +it is an ecosystem-wide breaking change belonging to 8.0.0 alongside the struts2-api +extraction (WW-4759). It is not required for correctness here: `disabled` is racy only on +the lazy path, which this change already covers. A separate ticket will be filed.