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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
