This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5659-lazy-params-request-scoping in repository https://gitbox.apache.org/repos/asf/struts.git
commit 84aca8cd5e76e137cb125bebb07f8dc7b0575c9b Author: Lukasz Lenart <[email protected]> AuthorDate: Mon Jul 27 10:56:03 2026 +0200 WW-5659 fix(core): resolve lazy interceptor params per invocation Co-Authored-By: deprrous <[email protected]> --- .../apache/struts2/DefaultActionInvocation.java | 53 ++++-- .../interceptor/ActionFileUploadInterceptor.java | 14 +- .../apache/struts2/interceptor/WithLazyParams.java | 25 ++- .../ActionFileUploadInterceptorTest.java | 186 ++++++++++++++++++++- .../apache/struts2/mock/MockLazyInterceptor.java | 48 +++++- 5 files changed, 291 insertions(+), 35 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java b/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java index 6885dd50d..913590410 100644 --- a/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java +++ b/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java @@ -40,6 +40,7 @@ import org.apache.struts2.util.ValueStack; import org.apache.struts2.util.ValueStackFactory; import java.util.ArrayList; +import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -258,17 +259,9 @@ public class DefaultActionInvocation implements ActionInvocation { if (interceptors.hasNext()) { final InterceptorMapping interceptorMapping = interceptors.next(); Interceptor interceptor = interceptorMapping.getInterceptor(); - if (interceptor instanceof WithLazyParams) { - Map<String, String> 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) { + 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()); @@ -312,6 +305,44 @@ public class DefaultActionInvocation implements ActionInvocation { return resultCode; } + /** + * Resolves lazy params into a per-invocation holder and dispatches to the interceptor. + * <p> + * {@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 <P extends org.apache.struts2.interceptor.InterceptorParams> String invokeWithLazyParams( + WithLazyParams<P> lazyInterceptor, InterceptorMapping interceptorMapping) throws Exception { + P lazyParams = lazyParamInjector.resolveInto( + lazyInterceptor.newLazyParams(), mergedParams(interceptorMapping), invocationContext); + + if (lazyParams instanceof org.apache.struts2.interceptor.DisableParams disableParams && disableParams.isDisabled()) { + LOG.debug("Interceptor: {} is disabled for this invocation, skipping to next", interceptorMapping.getName()); + 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<String, String> mergedParams(InterceptorMapping interceptorMapping) { + Map<String, String> 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; + } + protected String executeConditional(ConditionalInterceptor conditionalInterceptor) throws Exception { if (conditionalInterceptor.shouldIntercept(this)) { LOG.debug("Executing conditional interceptor: {}", conditionalInterceptor.getClass().getSimpleName()); 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 4aea0f782..82ff798e3 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java @@ -202,12 +202,22 @@ import java.util.List; * @see UploadedFilesAware * @see AbstractFileUploadInterceptor */ -public class ActionFileUploadInterceptor extends AbstractFileUploadInterceptor implements WithLazyParams { +public class ActionFileUploadInterceptor extends AbstractFileUploadInterceptor implements WithLazyParams<UploadPolicy> { 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) @@ -228,8 +238,6 @@ public class ActionFileUploadInterceptor extends AbstractFileUploadInterceptor i return invocation.invoke(); } - UploadPolicy policy = copyConfiguredPolicy(); - applyValidation(action, multiWrapper); // bind allowed Files 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 f683ebb7c..85d8e6d71 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java +++ b/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java @@ -21,6 +21,7 @@ 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; @@ -48,7 +49,21 @@ import java.util.Map; * * @since 2.5.9 */ -public interface WithLazyParams { +public interface WithLazyParams<P extends InterceptorParams> { + + /** + * @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 { @@ -80,14 +95,6 @@ public interface WithLazyParams { this.ognlUtil = ognlUtil; } - public Interceptor injectParams(Interceptor interceptor, Map<String, String> params, ActionContext invocationContext) { - for (Map.Entry<String, String> 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; - } - /** * Resolves configured params into a per-invocation holder, leaving the interceptor untouched. * <p> 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 c45091d77..c2cf40719 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.locale.DefaultLocaleProvider; 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.io.File; 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; @@ -536,14 +546,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { } 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 { @@ -955,4 +958,171 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { 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<String> plainResult = executor.submit(() -> runUploadAttempt( + sharedInterceptor, plainPolicyAction, createUploadRequest("plain-policy.html", "text/html", htmlContent))); + + assertThat(sharedInterceptor.awaitFirstValidation()).isTrue(); + + Future<String> 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(); + } + + 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<String, String> 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); + } + } + } + } 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 ae3aea3cc..75d3d021a 100644 --- a/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java +++ b/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java @@ -21,9 +21,35 @@ package org.apache.struts2.mock; import org.apache.struts2.ActionInvocation; import org.apache.struts2.SimpleAction; import org.apache.struts2.interceptor.AbstractInterceptor; +import org.apache.struts2.interceptor.InterceptorParams; import org.apache.struts2.interceptor.WithLazyParams; -public class MockLazyInterceptor extends AbstractInterceptor implements WithLazyParams { +public class MockLazyInterceptor extends AbstractInterceptor implements WithLazyParams<MockLazyInterceptor.MockLazyParams> { + + /** + * Per-invocation holder, seeded from the configured values. + */ + public static class MockLazyParams implements InterceptorParams { + + 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 = ""; @@ -44,12 +70,26 @@ public class MockLazyInterceptor extends AbstractInterceptor implements WithLazy 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();
