[
https://issues.apache.org/jira/browse/WW-5659?focusedWorklogId=1032374&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1032374
]
ASF GitHub Bot logged work on WW-5659:
--------------------------------------
Author: ASF GitHub Bot
Created on: 27/Jul/26 10:27
Start Date: 27/Jul/26 10:27
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #1816:
URL: https://github.com/apache/struts/pull/1816#discussion_r3656300019
##########
core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java:
##########
@@ -75,12 +95,73 @@ public void setOgnlUtil(OgnlUtil ognlUtil) {
this.ognlUtil = ognlUtil;
}
- public Interceptor injectParams(Interceptor interceptor, Map<String,
String> params, ActionContext invocationContext) {
+ /**
+ * Resolves configured params into a per-invocation holder, leaving
the interceptor untouched.
+ * <p>
+ * <strong>Every path that skips a write notifies the holder</strong>
via
+ * {@link InterceptorParams#unresolved(String)}, so the holder can
fail closed rather than
+ * silently validating against a dimension that was dropped. Two such
paths exist:
+ * <ul>
+ * <li>a {@code ${...}} expression that resolves to null or an empty
value (see
+ * {@link #isUnresolved})</li>
+ * <li>a resolved value the holder's setter cannot accept, e.g. a
non-numeric string for a
+ * {@code Long} property, which OGNL reports as a
+ * {@link ReflectionException} during conversion</li>
+ * </ul>
+ * In both cases the holder keeps its seeded configuration value and a
WARN is logged.
+ * <p>
+ * The empty-value rule also catches an expression that legitimately
evaluates to an empty
+ * string, which is indistinguishable from a failed resolution; for a
fail-closed policy such
+ * as an allowlist, treating both as unusable is the safe reading, so
a broken expression
+ * cannot silently relax a validation policy.
+ *
+ * @since 7.3.0
+ */
+ public <P extends InterceptorParams> P resolveInto(P target,
Map<String, String> params, ActionContext invocationContext) {
for (Map.Entry<String, String> entry : params.entrySet()) {
- Object paramValue = textParser.evaluate(new char[]{'$'},
entry.getValue(), valueEvaluator, TextParser.DEFAULT_LOOP_COUNT);
- ognlUtil.setProperty(entry.getKey(), paramValue, interceptor,
invocationContext.getContextMap());
+ String paramName = entry.getKey();
+ String rawValue = entry.getValue();
+ Object paramValue = textParser.evaluate(new char[]{'$'},
rawValue, valueEvaluator, TextParser.DEFAULT_LOOP_COUNT);
+
+ if (isUnresolved(rawValue, paramValue)) {
+ LOG.warn("Param [{}] of [{}] could not be resolved from
expression [{}]; keeping the configured value",
+ paramName, target.getClass().getName(), rawValue);
+ target.unresolved(paramName);
+ continue;
+ }
+ try {
+ // throwPropertyExceptions=true so a param with no
matching property on the holder is
+ // reported rather than silently ignored; OgnlUtil only
warns in devMode otherwise
+ ognlUtil.setProperty(paramName, paramValue, target,
invocationContext.getContextMap(), true);
+ } catch (ReflectionException e) {
+ LOG.warn("Param [{}] cannot be applied to [{}] - the value
was not written and the params are marked unusable; check the interceptor
configuration",
+ paramName, target.getClass().getName(), e);
Review Comment:
The WARN message in the ReflectionException path claims "the params are
marked unusable", but holders can legally ignore unresolved(...) (default is
no-op), so this log line can be misleading. Consider wording it neutrally
(e.g., keep configured value and notify holder) and let the holder decide
whether the policy becomes unusable.
Issue Time Tracking
-------------------
Worklog Id: (was: 1032374)
Time Spent: 20m (was: 10m)
> WithLazyParams resolves dynamic interceptor params onto the shared
> interceptor instance
> ---------------------------------------------------------------------------------------
>
> Key: WW-5659
> URL: https://issues.apache.org/jira/browse/WW-5659
> Project: Struts 2
> Issue Type: Bug
> Components: Core Interceptors
> Reporter: Lukasz Lenart
> Priority: Major
> Fix For: 7.3.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> h3. Problem
> {{WithLazyParams.LazyParamInjector#injectParams}} resolves {{${...}}}
> interceptor
> parameters per request and writes the resolved values straight onto the
> interceptor
> instance:
> {code:java}
> // WithLazyParams.java:78-84
> public Interceptor injectParams(Interceptor interceptor, Map<String, String>
> params, ActionContext invocationContext) {
> for (Map.Entry<String, String> entry : params.entrySet()) {
> Object paramValue = textParser.evaluate(new char[]{'$'},
> entry.getValue(), valueEvaluator, TextParser.DEFAULT_LOOP_COUNT);
> ognlUtil.setProperty(entry.getKey(), paramValue, interceptor,
> invocationContext.getContextMap());
> }
> return interceptor;
> }
> {code}
> That interceptor is a singleton. It is built once at configuration-parse time
> ({{InterceptorBuilder}}:73-74 and :175-177) and reused for every request.
> When an action
> references an interceptor stack without overriding params,
> {{InterceptorBuilder}}:79
> (_result.addAll(stackConfig.getInterceptors())_) hands out the same
> {{InterceptorMapping}}
> objects, so the same interceptor instance is shared across actions as well.
> The result is that request-scoped state is written to process-wide state with
> no
> synchronisation.
> h3. Consequence for file upload validation
> {{ActionFileUploadInterceptor}} is currently the only {{WithLazyParams}}
> implementer. Its
> resolved policy lands in plain instance fields
> ({{AbstractFileUploadInterceptor}}:62-64, written at :85, :94, :103) and is
> read back
> later by {{acceptFile}} ({{AbstractFileUploadInterceptor}}:134, :141, :148).
> Per request the order is:
> * {{DefaultActionInvocation}}:269 - resolve and write the shared fields
> * {{DefaultActionInvocation}}:275 - {{intercept()}} -> {{acceptFile()}} reads
> the shared fields
> Nothing guards the interval between the two. Two concurrent requests that
> resolve
> different policies can therefore have their {{allowedTypes}},
> {{allowedExtensions}} and
> {{maximumSize}} cross over, and a request can be validated against another
> request's
> upload policy.
> This only affects applications that opt into the dynamic {{${...}}} form
> added in
> WW-5585. {{struts-default.xml}}:60 declares {{actionFileUpload}} with no
> params, so the
> default configuration writes nothing and is unaffected. Static
> (non-expression) params
> resolve to the same value on every request and are likewise unaffected.
> Note that {{actionFileUpload}} sits at position 15 of {{defaultStack}}, ahead
> of
> {{staticParams}} (19), {{actionMappingParams}} (20) and {{params}} (21), so
> no request
> parameter has been bound to the action at resolution time. The expression's
> *value* is
> not attacker-supplied. Which policy gets selected can still depend on
> request-derived
> state populated by {{prepare}} (11), {{scopedModelDriven}} (13) or
> {{modelDriven}} (14) -
> the shipped showcase does exactly that in
> {{DynamicFileUploadAction#prepareUpload}}:136-139.
> h3. Secondary defect
> {{DefaultActionInvocation}}:262-267 calls {{params.putAll(...)}} on the map
> returned by
> {{InterceptorMapping#getParams}}:60-62, which is the live shared map. That is
> an
> unsynchronised write to a shared {{HashMap}} on every request.
> h3. Proposed fix
> Fix the {{WithLazyParams}} contract rather than patching the one implementer,
> so that no
> future implementer can reintroduce the same bug. Resolved parameters should
> be written
> into a per-invocation object supplied by the interceptor and passed back to
> it, leaving
> the interceptor singleton immutable after {{init()}}:
> {code:java}
> public interface WithLazyParams<P> {
> P newLazyParams();
> String intercept(ActionInvocation invocation, P lazyParams) throws
> Exception;
> }
> {code}
> Targeting a clean interface change in the next minor release. A design
> document will
> follow.
> h3. References
> * WW-5585 - introduced dynamic parameter evaluation for file upload validation
> * GitHub PR [#1815|https://github.com/apache/struts/pull/1815] - reported the
> problem; approach not adopted
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
