WW-5659 Resolve lazy interceptor params per invocation

[lukaszlenart]

Fixes WW-5659

Supersedes #1815, which reported the problem. The concurrency scenario and test harness come from @deprrous's work there and are carried over with attribution.

Problem

WithLazyParams.LazyParamInjector#injectParams resolved ${...} interceptor params once per request and wrote the resolved values straight onto the interceptor:

ognlUtil.setProperty(entry.getKey(), paramValue, interceptor, invocationContext.getContextMap());

That interceptor is a singleton, built once at configuration-parse time and reused for every request. When an action references a stack without overriding params, InterceptorBuilder hands the same InterceptorMapping objects to every such action, so the instance is shared across actions too. Request-scoped state was being written to process-wide state with no synchronisation.

For ActionFileUploadInterceptor — the only implementer — the resolved policy landed in plain instance fields that acceptFile read back later. Nothing guarded the interval, so two concurrent requests resolving different policies could have their allowedTypes, allowedExtensions and maximumSize cross over. disabled was affected the same way, and it decides whether the interceptor runs at all.

DefaultActionInvocation also called params.putAll(...) on the live map returned by InterceptorMapping#getParams — an unsynchronised write to a shared HashMap on every request.

Scope: only applications opting into the dynamic ${...} form added in WW-5585 (7.2.0) are affected. struts-default.xml declares actionFileUpload with no params, so a default configuration resolves nothing and writes nothing. Static params resolve to the same value every request. Assessed as a thread-safety defect rather than a framework vulnerability — actionFileUpload runs ahead of staticParams/params in defaultStack, so no request parameter is bound to the action at resolution time and the expression's value is not attacker-supplied.

Approach

Fix the contract rather than 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 stays immutable after init().

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

New types in org.apache.struts2.interceptor:

• InterceptorParams — the general contract and the generic bound.
• DisableParams — opt-in support for the disabled param. disabled is universal across interceptors while lazy resolution is rare, so it is not a subtype of any lazy-specific type.
• UploadPolicy extends DisableParams — the upload holder, with typed setters so OGNL still converts String → Long for maximumSize.

LazyParamInjector.injectParams becomes resolveInto(P target, ...). DefaultActionInvocation merges the lazy and conditional dispatch paths, and mergedParams returns a fresh map.

There is no cleanup step — no finally, nothing to clear, no ThreadLocal. That absence is the design's own check: if a future change makes cleanup necessary, the separation has regressed.

Rejected alternatives, with reasoning, are in the design doc — including the ThreadLocal approach from #1815, which leaves the unsafe write path in place rather than removing it.

⚠️ Release notes / migration for 7.3.0

Four user-visible behaviour changes. Please pick these up when preparing the release announcement and docs.

1. An unresolvable ${...} upload param now rejects the upload instead of silently disabling validation.
Previously an expression that failed to resolve produced "", which became an empty set, which acceptFile read as "no restriction". Applications running with a broken expression have had that validation silently switched off; they will now see uploads rejected with struts.messages.error.upload.policy.unresolved. This is the change most likely to surprise on upgrade, and surfacing it is the point.

2. An unknown or misspelled lazy interceptor param now fails application startup.
Param names are known at configuration-parse time, so they are validated there and a bad one raises ConfigurationException rather than being ignored. An application carrying a latent typo in struts.xml that appeared to work will no longer boot until it is corrected.

3. A WithLazyParams holder must extend DisableParams to accept the disabled param.
There is deliberately no fallback to the interceptor instance — that fallback is the racy path being removed. A <param name="disabled"> on an interceptor whose holder does not extend DisableParams now fails startup. Third-party implementers must touch their code regardless, since the interface gained a type parameter.

4. newLazyParams() is now called once at configuration time, to discover the holder type for OGNL allowlist registration. The contract expects it to be side-effect free; an implementation that throws will now fail at startup rather than per request.

Breaking changes

WithLazyParams is public API since 2.5.9, but ActionFileUploadInterceptor is its only implementer in the repo, so third-party implementers get a compile error rather than silent breakage:

• WithLazyParams is now generic and declares newLazyParams() plus a two-argument intercept. injectParams is gone.
• InterceptorParams extends Serializable, because Interceptor is serializable and an interceptor holds its configured params as a field.
• AbstractFileUploadInterceptor.acceptFile gained a leading UploadPolicy parameter (protected).
• DefaultActionInvocation.executeConditional(ConditionalInterceptor) was replaced by a two-argument form carrying the interceptor name. The single-argument version had no callers.
• An interceptor overriding shouldIntercept to read its own lazily-injected fields now sees config-time values only, since resolution no longer touches the interceptor. ActionFileUploadInterceptor does not override it, so nothing shipped is affected.

Follow-ups, not in this PR

Tickets to be filed separately:

• Core tests run with the OGNL allowlist disabled, so a whole class of bug is invisible to them — an allowlist regression here reached CI on every job and was caught only by the showcase integration tests. An allowlist-enabled test slice would close that gap.
• DefaultActionInvocation.mergedParams looks up its own mapping by name, so the second putAll is a no-op for normal refs and merges the wrong params for a stack referencing one interceptor twice. Inherited from pre-existing code; recorded in a comment.
• isUnresolved infers failure from an empty string because OgnlTextParser discards the distinction between "did not resolve" and "resolved to empty". A real signal would also close the partial-resolution blind spot (${a},${b} with only ${b} failing writes a truncated value).
• The configuration-time ConfigurationException points at the <interceptor> definition rather than the offending <interceptor-ref>, because the InterceptorFactory SPI has no parameter for the ref's Location. The message is actionable; the file:line is misleading.
• DefaultActionInvocation is flagged by SonarCloud as a "Monster Class" (22 dependencies vs 20). Pre-existing; this branch added two imports and crossed the threshold.

Testing

mvn test -DskipAssembly — 28 modules, core 3104 tests, 0 failures. Showcase integration tests including DynamicFileUploadTest pass.

The concurrency regression is deterministic rather than timing-dependent: the first thread is parked inside acceptFile on a latch and the second is only submitted once that is confirmed. Reverting copyConfiguredPolicy() to return the shared instance makes it fail exactly as the bug describes — the text/plain invocation accepts a text/html upload. Both skip branches in the merged dispatch path are pinned one-to-one by dedicated fixtures, and LazyParamsAllowlistTest covers resolution with the OGNL allowlist enabled.

[Copilot]

Pull request overview

This PR fixes a concurrency/thread-safety defect in Struts’ lazy interceptor parameter resolution (${...}) by changing the WithLazyParams contract so request-scoped resolved values are written into a per-invocation params holder instead of being applied to the shared interceptor singleton. It also updates file upload validation to use a per-invocation UploadPolicy (and fail closed when lazy params can’t be resolved), and adds regression tests for concurrency, disabled, and the new fail-closed behavior.

Changes:

• Redesign WithLazyParams to resolve lazy params into a per-invocation InterceptorParams holder and invoke a new two-argument intercept(invocation, params) API.
• Refactor file upload validation to use a per-invocation UploadPolicy copied from configured values; reject uploads when any required lazy param is unresolved.
• Add/extend tests and test fixtures covering concurrency isolation, lazy disabled behavior, and unresolved/unknown param handling; update test XML configs accordingly.

Reviewed changes

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

Show a summary per file

File	Description
docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md	Design write-up for request-scoped lazy param resolution and fail-closed semantics.
docs/superpowers/plans/2026-07-27-WW-5659-lazy-params-request-scoping.md	Implementation plan documenting tasks, constraints, and intended behavior.
core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java	Adds params-holder contract with unresolved(String) hook.
core/src/main/java/org/apache/struts2/interceptor/DisableParams.java	Adds opt-in per-invocation disabled support for lazy params holders.
core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java	Introduces per-invocation upload policy holder with unresolved tracking.
core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java	Breaks/updates WithLazyParams API; adds holder-based resolution (resolveInto).
core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java	Refactors upload validation to accept an UploadPolicy and fail closed on unresolved policy.
core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java	Implements new WithLazyParams<UploadPolicy> contract; uses per-invocation policy during validation.
core/src/main/java/org/apache/struts2/DefaultActionInvocation.java	Routes WithLazyParams interceptors through new holder-based path; avoids mutating shared param maps; adds executeConditional overload.
core/src/main/resources/org/apache/struts2/struts-messages.properties	Adds new message key for unresolved upload policy rejection.
core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java	Unit tests for DisableParams.
core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java	Tests new resolveInto behavior, including unresolved/unknown param notifications and type conversion.
core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java	Updates tests for new policy/contract; adds concurrency and fail-closed regressions and helper harness.
core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java	Adds regressions ensuring lazy/static disabled is respected on the lazy path.
core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java	Updates mock interceptor to new generic WithLazyParams contract and per-invocation params holder.
core/src/test/resources/xwork-test-default.xml	Adds statically disabled lazy interceptor definition for test coverage.
core/src/test/resources/xwork-sample.xml	Adds actions exercising lazy-resolved vs statically configured disabled behavior.

---

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

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
90.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud