Lukasz Lenart created WW-5663:
---------------------------------
Summary: DefaultActionInvocation.mergedParams resolves interceptor
params by name, merging the wrong ref when a name repeats
Key: WW-5663
URL: https://issues.apache.org/jira/browse/WW-5663
Project: Struts 2
Issue Type: Bug
Components: Core Interceptors
Reporter: Lukasz Lenart
Fix For: 7.3.0
h3. Problem
{{DefaultActionInvocation.mergedParams}} assembles the parameters handed to a
{{WithLazyParams}} interceptor for the current invocation:
{code:java}
private Map<String, String> mergedParams(InterceptorMapping interceptorMapping)
{
Map<String, String> merged = new
LinkedHashMap<>(interceptorMapping.getParams());
proxy.getConfig().getInterceptors().stream()
.filter(im -> im.getName().equals(interceptorMapping.getName()))
.findFirst()
.ifPresent(im -> merged.putAll(im.getParams()));
return merged;
}
{code}
The lookup searches {{proxy.getConfig().getInterceptors()}} for a mapping whose
*name* matches
the mapping already in hand. But that is the very list the invocation iterates:
{code:java}
// DefaultActionInvocation.createInterceptors
List<InterceptorMapping> interceptorList = new
ArrayList<>(proxy.getConfig().getInterceptors());
interceptors = interceptorList.iterator();
{code}
So {{interceptorMapping}} is itself an element of the list being searched. Two
consequences:
* *When the name is unique in the list* - the overwhelmingly common case -
{{findFirst}} returns
the same object, and {{merged.putAll(im.getParams())}} merges a map into a
copy of itself. The
entire lookup is a no-op.
* *When the name appears more than once* - Struts permits an interceptor name
to resolve into an
action's flattened interceptor list more than once, for instance when a stack
is composed from
another stack that already contains that interceptor, or when a ref is
repeated with different
params - {{findFirst}} always returns the *first* mapping. Every later
invocation of that name
then has the first ref's params merged over its own, silently overriding them.
h3. Impact
Low in practice and easy to miss, which is why it has gone unnoticed:
* it only affects interceptors implementing {{WithLazyParams}};
{{ActionFileUploadInterceptor}}
is currently the only one
* it requires the same interceptor name to appear more than once in one
action's resolved
interceptor list
* the symptom is an interceptor quietly running with another ref's parameters,
not an error
Where it does bite, the failure is silent: no warning, no exception, just the
wrong upload
policy.
No in-tree configuration is known to trigger it. It is reachable through
configuration Struts
permits, not through misuse.
h3. Origin
Introduced by WW-5585 ({{939576c1c}}, "Implement dynamic parameter evaluation
for file upload
validation"), so it is present in 7.2.0 and 7.2.1.
WW-5659 rewrote the surrounding method - it previously mutated the shared
{{InterceptorMapping}} param map in place via {{putAll}}, which was an
unsynchronised write to
shared configuration state on every request. That part is fixed: the method now
builds a fresh
{{LinkedHashMap}}. The name-based lookup was deliberately left untouched and is
documented in a
comment on the method, because changing it is a behaviour change that deserves
its own issue.
h3. What to decide
The first question is what the lookup was *for*. Since it resolves to the same
object in the
common case, its intended purpose is not evident from the code, and the WW-5585
commit does not
explain it. Candidate readings:
# It was meant to merge the {{<interceptor>}} *definition's* own params with the
{{<interceptor-ref>}} params. If so it does not work: {{InterceptorBuilder}}
passes only the
ref params into {{InterceptorMapping}}, so definition params never appear in
that list. They
are applied to the interceptor instance at build time instead.
# It was defensive against the mapping not being present in the config list. If
so, the
{{findFirst}} result should be ignored when it is the same object.
# It is simply redundant, and the method reduces to copying
{{interceptorMapping.getParams()}}.
Reading 3 looks most likely, in which case the fix is to delete the lookup -
which also removes
the repeated-name bug. That should be confirmed against WW-5585's intent rather
than assumed.
h3. Suggested work
* determine the intended purpose of the name-based lookup
* if redundant, remove it and reduce the method to a defensive copy
* if not, key the lookup on identity or position rather than name
* add a test with an action whose resolved interceptor list contains the same
{{WithLazyParams}}
interceptor twice with different params, asserting each invocation sees its
own
* remove the explanatory comment on {{mergedParams}} once resolved
h3. References
* {{core/src/main/java/org/apache/struts2/DefaultActionInvocation.java}} -
{{mergedParams}} and
{{createInterceptors}}
* WW-5585 - introduced the lookup
* WW-5659 - fixed the shared-map mutation in the same method; left this
deliberately
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
