WW-5585 Keep dynamic upload validation request-scoped

[deprrous]

Follow-up to WW-5585.

When allowedTypes, allowedExtensions, or maximumSize are resolved lazily from ${...}, those resolved values currently get written back onto the shared interceptor instance. That lets concurrent requests bleed into each other before file validation runs.

This keeps the lazily resolved upload policy request-scoped for the lifetime of the interceptor call and clears it afterwards. Static interceptor configuration still behaves the same.

Tests:

• ./mvnw -pl core -Dtest=org.apache.struts2.interceptor.ActionFileUploadInterceptorTest#testConcurrentDynamicPoliciesStayIsolatedPerRequest test
• ./mvnw -pl core -Dtest=org.apache.struts2.interceptor.ActionFileUploadInterceptorTest,org.apache.struts2.DefaultActionInvocationTest test

[Copilot]

Pull request overview

This PR addresses a concurrency issue in Struts’ file upload validation when allowedTypes, allowedExtensions, or maximumSize are lazily resolved via ${...}: previously, the resolved values could be written back onto a shared interceptor instance and bleed across concurrent requests. The change keeps dynamically-resolved upload validation policy request-scoped for the lifetime of the interceptor invocation and clears it afterward, while preserving existing behavior for static interceptor configuration.

Changes:

• Add a request-scoped UploadValidationPolicy (via ThreadLocal) in AbstractFileUploadInterceptor and use it during acceptFile(...).
• Track when lazy param injection is in progress (WithLazyParams.LazyParamInjector), so setters can route dynamically injected values into the request-scoped policy rather than the interceptor’s shared fields.
• Ensure request-scoped policy is cleared in ActionFileUploadInterceptor.intercept() and add a concurrent regression test.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java	Refactors dynamic-policy tests to inject ${...} params and adds a concurrency regression test to ensure per-request isolation.
core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java	Adds a thread-local “injection in progress” marker around lazy param injection.
core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java	Clears request-scoped upload validation policy in a finally block after interception.
core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java	Implements request-scoped upload validation policy storage and uses it during file acceptance checks.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[lukaszlenart]

Thanks for this — the underlying problem is real and you diagnosed it correctly. I traced it independently and it holds up:

LazyParamInjector#injectParams (WithLazyParams.java:78-84) writes resolved values straight onto the interceptor, and that interceptor is a config-time singleton (InterceptorBuilder.java:73-74, :175-177). It's worse than same-action concurrency: InterceptorBuilder.java:79 hands out the same InterceptorMapping objects to every action that references a stack without overriding params, so the instance is shared across actions too. Between DefaultActionInvocation.java:269 (resolve, write shared fields) and :275 → acceptFile (read shared fields) there is no guard at all. Good catch.

I don't want to merge this particular fix though, for a few reasons:

1. static ThreadLocal PARAM_INJECTION_IN_PROGRESS makes setAllowedTypes/setAllowedExtensions/setMaximumSize behave differently depending on hidden thread state. A setter whose target depends on an ambient flag is hard to reason about and hard to test.
2. clearRequestScopedUploadValidationPolicy() is only called from ActionFileUploadInterceptor#intercept's finally. WithLazyParams is public API — any other implementer that forgets that call leaks the ThreadLocal.
3. The unsafe write path still exists; it's conditionally bypassed rather than removed. The next WithLazyParams implementer gets the original bug back by default.
4. It doesn't cover DefaultActionInvocation.java:262-267, which does params.putAll(...) on the live shared map returned by InterceptorMapping#getParams (:60-62) — an unsynchronised write to a shared HashMap on every request.

So rather than patch the one implementer, I'd like to fix the WithLazyParams contract so the failure mode can't be expressed. The direction is to have resolved params written into a per-invocation object that the interceptor supplies and receives back, leaving the singleton immutable after init():

public interface WithLazyParams<P> {
    P newLazyParams();
    String intercept(ActionInvocation invocation, P lazyParams) throws Exception;
}

That keeps OGNL type conversion (the holder has typed setters, so maximumSize still converts to Long), removes the mutation entirely, and makes the data flow an explicit argument instead of ambient state. It's an interface change, so it targets the next minor rather than 7.2.x.

I've filed WW-5659 to track it and I'm writing up the design now. Your regression test is the valuable part of this PR — the scenario it pins down is exactly what the new design has to keep passing, and I'd like to carry it over (with attribution) once the interface lands. If you'd like to take the implementation on the new contract, say so and it's yours; otherwise I'll pick it up and credit you on the ticket.

One process note for next time, not a criticism of this one: when a change touches something that could weaken a security control — upload validation, parameter filtering, OGNL access — please email [email protected] before opening a public PR. SECURITY.md covers why. We looked at this one and concluded it's a thread-safety defect rather than a vulnerability (it needs the opt-in ${...} config, and struts-default.xml:60 ships actionFileUpload with no params, so default setups write nothing), which is why it's fine to keep the discussion here.

[deprrous]

Thanks for the detailed review. I’d like to take WW-5659 on this branch direction. I’ll rework this around the explicit per-invocation lazy-params contract and keep the concurrency regression scenario in place.

[deprrous]

Pushed a rework on top of this branch for WW-5659.

The lazy params now live in a per-invocation holder instead of mutating the singleton interceptor instance. DefaultActionInvocation resolves a fresh holder for each call, ActionFileUploadInterceptor consumes an explicit upload policy object, and the concurrent upload isolation regression is still in place. I also added a small factory regression for typed lazy params so ${...} values are deferred until invocation time instead of being pushed through singleton setters during interceptor construction.

[lukaszlenart]

Thanks for the rework — I read it properly and it's a real step up. You independently found several things that took my side multiple review rounds to catch:

• the ConditionalInterceptor interaction. AbstractInterceptor implements ConditionalInterceptor, so a lazy interceptor reaches intercept through executeConditional, and leaving the two branches as siblings silently voids the dynamic policy. You merged them.
• the shared InterceptorMapping param map, now copied rather than mutated per request — and with LinkedHashMap, preserving configuration order.
• the abstract-method problem, handled correctly with the explicit WithLazyParams.InvocationScoped.super.intercept(invocation) delegation. An interface default alone would not have satisfied AbstractInterceptor's abstract declaration.
• the OGNL allowlist. ce7c3fcaa is the one that impressed me. Moving the write target off the interceptor means the new holder isn't allowlisted, and with struts.allowlist.enable=true — the shipped default — every upload gets rejected. I hit exactly the same trap and it broke CI on every job in WW-5659 Resolve lazy interceptor params per invocation #1816; the showcase integration tests were the only thing that caught it. You found it yourself and your ThreadAllowlist route is sound (ModelDrivenInterceptor sets the precedent, and java.lang.Object is excluded independently at SecurityMemberAccess:86, so the hierarchy walk doesn't widen anything).

Where this is going

I'm going to land #1816 rather than this PR, and I want to be straight with you about why, because your design had one genuine advantage over mine.

Your WithLazyParams.InvocationScoped<P> is an optional sub-interface, so WithLazyParams stays source-compatible and no third-party implementer breaks. That's better than my approach on that axis, and it would have allowed a 7.2.x patch for a bug that's live in 7.2.0 and 7.2.1. I weighed that seriously.

What settles it is release planning rather than design: 7.3.0 already carries other compatibility-breaking changes, so there is no 7.2.2 to target. Once the fix ships in a version that breaks compatibility anyway, the compat-preserving shape stops buying anything, and the tradeoff inverts — a single mandatory contract is easier to reason about than a mandatory one plus an optional one, and it removes the unsafe path instead of letting implementers opt out of it. In your version the legacy WithLazyParams branch still writes to the interceptor, so the race remains for anyone who doesn't adopt InvocationScoped.

Alongside that, #1816 carries work this PR doesn't: fail-closed handling when an expression can't be resolved (previously an unresolvable ${...} produced an empty allowlist, which reads as "no restriction" — validation silently off), configuration-time validation of param names so a typo fails at startup instead of rejecting uploads in production, and InterceptorParams extends Serializable because Interceptor is.

One bug worth knowing about, wherever it lands

In this PR a lazily resolved disabled is silently ignored. UploadValidationPolicy has no disabled property, filterFactoryTimeParams withholds it from the interceptor at build time, and ognlUtil.setProperty without throwPropertyExceptions no-ops rather than complaining — so <param name="disabled">${expr}</param> does nothing at all and shouldIntercept reads the interceptor's permanently-false field. Before your change it was racy but functional; after, it's quietly dead. That's what pushed me toward making the holder's disabled support explicit (DisableParams) and validating the param names at configuration time.

What I've taken from your PR

filterFactoryTimeParams is a better idea than what I had, and it's now in #1816 as 83d2b61dd with attribution to you and a Co-Authored-By trailer. My version seeded the interceptor with the raw ${...} literal, so allowedTypes held the string "${uploadConfig.allowedMimeTypes}" — meaningless as a policy value — and maximumSize failed conversion outright, since a Long can't hold that text. Withholding those params at build time is simply correct, and it also makes the "what value survives a failed resolution" documentation honest instead of hand-wavy.

Your concurrency regression test from the first round is also in #1816, carried over with attribution — the scenario it pins is the acceptance criterion for the whole change, and reverting the fix still makes it fail exactly as the bug describes.

So: I'm closing this one, but two pieces of it ship, and you're credited on the ticket and in the commits. The allowlist catch in particular was independent work on a trap that had already bitten me — thank you for it.

One process note, same as last time and still worth repeating: when a change touches upload validation, parameter filtering, or OGNL access, please email [email protected] before opening a public PR. SECURITY.md covers the reasoning. This one is a thread-safety defect rather than a vulnerability — it needs the opt-in ${...} config, and struts-default.xml:60 ships actionFileUpload with no params — which is why the discussion stayed here.