[
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)
