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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
