[
https://issues.apache.org/jira/browse/WW-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5663:
------------------------------
Fix Version/s: 7.4.0
(was: 7.3.0)
> 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
> Priority: Major
> Fix For: 7.4.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)
