[
https://issues.apache.org/jira/browse/WW-5659?focusedWorklogId=1032372&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1032372
]
ASF GitHub Bot logged work on WW-5659:
--------------------------------------
Author: ASF GitHub Bot
Created on: 27/Jul/26 10:19
Start Date: 27/Jul/26 10:19
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1816:
URL: https://github.com/apache/struts/pull/1816
Fixes [WW-5659](https://issues.apache.org/jira/browse/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:
```java
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. This was 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()`.
```java
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. One
defaulted method, `unresolved(String)`.
- `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.
## Fail-closed
Previously an expression that failed to resolve yielded `""`, which became
an empty set, which `acceptFile` read as "no restriction" — so a typo or a null
intermediate silently switched off an upload restriction. Now any param that
cannot be applied marks the policy unusable and the upload is rejected with
`struts.messages.error.upload.policy.unresolved`.
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 — **worth calling out in the
release notes.**
## Breaking changes for 7.3.0
`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.
- `AbstractFileUploadInterceptor.acceptFile` gained a leading `UploadPolicy`
parameter (`protected`).
- A lazily resolved `disabled` only takes effect if the holder extends
`DisableParams`. There is deliberately no fallback to the interceptor instance
— that is the racy path being removed.
- 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.
## Two open questions for review
Draft because these are worth a second opinion before merge:
1. **Unknown params now fail closed too.** Fixing a fail-open path meant
notifying the holder whenever a write is skipped, and that catch also covers
`NoSuchPropertyException` — `OgnlUtil.internalSetProperty` wraps every
`OgnlException` into the same `ReflectionException`, so the cases cannot be
separated at the call site. A typo'd param on the lazy path therefore rejects
uploads instead of being silently ignored. `UploadPolicy` exposes exactly the
four legitimate params, so anything else is definitionally a misconfiguration —
but it is a widening worth agreeing on.
2. **`executeConditional` overload.** Getting the mapping name into the log
needed a two-argument form; the one-argument version is kept `@Deprecated` and
delegating. There are no current callers of the old form, and this branch
already breaks `protected acceptFile`, so collapsing to a single method would
be equally safe and leaner. Happy to do that if preferred.
## Follow-ups, not in this PR
- `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 the pre-existing
code; recorded in a comment, ticket to follow.
- `isUnresolved` infers failure from an empty string because
`OgnlTextParser` discards the distinction between "did not resolve" and
"resolved to empty". Threading a real signal out of the parser would also close
the partial-resolution blind spot (`${a},${b}` with only `${b}` failing writes
a truncated value).
## Testing
`mvn test -DskipAssembly` — 28 modules, 4220 tests, 0 failures.
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.
Issue Time Tracking
-------------------
Worklog Id: (was: 1032372)
Remaining Estimate: 0h
Time Spent: 10m
> WithLazyParams resolves dynamic interceptor params onto the shared
> interceptor instance
> ---------------------------------------------------------------------------------------
>
> Key: WW-5659
> URL: https://issues.apache.org/jira/browse/WW-5659
> Project: Struts 2
> Issue Type: Bug
> Components: Core Interceptors
> Reporter: Lukasz Lenart
> Priority: Major
> Fix For: 7.3.0
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> h3. Problem
> {{WithLazyParams.LazyParamInjector#injectParams}} resolves {{${...}}}
> interceptor
> parameters per request and writes the resolved values straight onto the
> interceptor
> instance:
> {code:java}
> // WithLazyParams.java:78-84
> 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;
> }
> {code}
> That interceptor is a singleton. It is built once at configuration-parse time
> ({{InterceptorBuilder}}:73-74 and :175-177) and reused for every request.
> When an action
> references an interceptor stack without overriding params,
> {{InterceptorBuilder}}:79
> (_result.addAll(stackConfig.getInterceptors())_) hands out the same
> {{InterceptorMapping}}
> objects, so the same interceptor instance is shared across actions as well.
> The result is that request-scoped state is written to process-wide state with
> no
> synchronisation.
> h3. Consequence for file upload validation
> {{ActionFileUploadInterceptor}} is currently the only {{WithLazyParams}}
> implementer. Its
> resolved policy lands in plain instance fields
> ({{AbstractFileUploadInterceptor}}:62-64, written at :85, :94, :103) and is
> read back
> later by {{acceptFile}} ({{AbstractFileUploadInterceptor}}:134, :141, :148).
> Per request the order is:
> * {{DefaultActionInvocation}}:269 - resolve and write the shared fields
> * {{DefaultActionInvocation}}:275 - {{intercept()}} -> {{acceptFile()}} reads
> the shared fields
> Nothing guards the interval between the two. Two concurrent requests that
> resolve
> different policies can therefore have their {{allowedTypes}},
> {{allowedExtensions}} and
> {{maximumSize}} cross over, and a request can be validated against another
> request's
> upload policy.
> This only affects applications that opt into the dynamic {{${...}}} form
> added in
> WW-5585. {{struts-default.xml}}:60 declares {{actionFileUpload}} with no
> params, so the
> default configuration writes nothing and is unaffected. Static
> (non-expression) params
> resolve to the same value on every request and are likewise unaffected.
> Note that {{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 gets 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#prepareUpload}}:136-139.
> h3. Secondary defect
> {{DefaultActionInvocation}}:262-267 calls {{params.putAll(...)}} on the map
> returned by
> {{InterceptorMapping#getParams}}:60-62, which is the live shared map. That is
> an
> unsynchronised write to a shared {{HashMap}} on every request.
> h3. Proposed fix
> Fix the {{WithLazyParams}} contract rather than patching the one implementer,
> so that no
> future implementer can reintroduce the same bug. Resolved parameters should
> be written
> into a per-invocation object supplied by the interceptor and passed back to
> it, leaving
> the interceptor singleton immutable after {{init()}}:
> {code:java}
> public interface WithLazyParams<P> {
> P newLazyParams();
> String intercept(ActionInvocation invocation, P lazyParams) throws
> Exception;
> }
> {code}
> Targeting a clean interface change in the next minor release. A design
> document will
> follow.
> h3. References
> * WW-5585 - introduced dynamic parameter evaluation for file upload validation
> * GitHub PR [#1815|https://github.com/apache/struts/pull/1815] - reported the
> problem; approach not adopted
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
