[jira] [Work logged] (WW-5663) DefaultActionInvocation.mergedParams resolves interceptor params by name, merging the wrong ref when a name repeats

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/WW-5663?focusedWorklogId=1041219&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1041219 ]

ASF GitHub Bot logged work on WW-5663:
--------------------------------------

                Author: ASF GitHub Bot
            Created on: 13/Sep/26 17:51
            Start Date: 13/Sep/26 17:51
    Worklog Time Spent: 10m 
      Work Description: lukaszlenart opened a new pull request, #1933:
URL: https://github.com/apache/struts/pull/1933

   Fixes [WW-5663](https://issues.apache.org/jira/browse/WW-5663)
   
   `DefaultActionInvocation.mergedParams` looked the interceptor mapping up *again* by name in `proxy.getConfig().getInterceptors()` — the very list the invocation iterates — and merged the first match's params over the mapping's own. With a unique name that merged a map into a copy of itself; with a repeated name (a stack composed from another stack that already holds the interceptor, or a ref repeated with different params) every later invocation silently ran with the **first** ref's params. Only `WithLazyParams` interceptors are affected — `ActionFileUploadInterceptor` in-tree — so the symptom is the wrong upload policy, with no error.
   
   ### Why the lookup existed
   
   It came in with WW-5585 (`939576c1c`, 2025-11-22), written two days after WW-5587 was filed: at that point stack-ref params were not stored on the `InterceptorMapping` at all, and the lookup was an attempt to fetch them from the config list — which could never work, since it was searching the same objects. WW-5587 fixed the real cause in 7.2.1 by passing the params into the mapping, so nothing has needed the lookup since. The PR carries no other rationale for it.
   
   ### Change
   
   `mergedParams` and its WW-5659 explanatory comment are deleted; `invokeWithLazyParams` passes `interceptorMapping.getParams()` straight to `LazyParamInjector.resolveInto`. No defensive copy — `resolveInto` only iterates the map (verified), and the mapping's map is build-time configuration nothing writes to at runtime since WW-5659.
   
   Behaviour change: an interceptor-ref name appearing twice in one action's resolved list now applies each ref's own params. Present in 7.2.0–7.3.0; no 6.x.
   
   ### Test
   
   `DefaultActionInvocationTest.testInvokeWithLazyParamsRepeatedRefKeepsEachRefsOwnParams` with a new `LazyFooTwice` action in `xwork-sample.xml` referencing `lazy` twice (`foo=first`, `foo=second`). RED on `main`: `expected:<second> but was:<first>`.
   
   Full `core` suite passes.
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)




Issue Time Tracking
-------------------

            Worklog Id:     (was: 1041219)
    Remaining Estimate: 0h
            Time Spent: 10m

> 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
>            Assignee: Lukasz Lenart
>            Priority: Major
>             Fix For: 7.4.0
>
>          Time Spent: 10m
>  Remaining Estimate: 0h
>
> 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)