This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5659-lazy-params-request-scoping in repository https://gitbox.apache.org/repos/asf/struts.git
commit 0cafec4d24cc751e554ef58c9850fa671ab2ab73 Author: Lukasz Lenart <[email protected]> AuthorDate: Mon Jul 27 11:59:33 2026 +0200 WW-5659 fix(core): mark params unusable when a lazy value cannot be applied resolveInto had two failure branches behaving oppositely. An unresolvable ${...} skipped the write and notified the holder, so a fail-closed holder such as UploadPolicy could reject the upload. A value the holder's setter could not accept — a non-numeric String for the Long maximumSize, say — also skipped the write but notified nothing, leaving the policy reporting isUnresolved() == false and maximumSize null, which acceptFile reads as "no size limit". The cap was silently off and the file accepted: fail-open, through a branch the design never enumerated. Notify the holder from the catch block too, and record in the spec the general rule that every path skipping a write must notify, so a future failure mode gets checked against it. Co-Authored-By: Claude Opus 5 <[email protected]> --- .../apache/struts2/interceptor/WithLazyParams.java | 32 ++++++++++++++++------ .../struts2/interceptor/LazyParamInjectorTest.java | 14 +++++++++- ...7-WW-5659-lazy-params-request-scoping-design.md | 24 ++++++++++++++-- 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java b/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java index 85d8e6d71..64cea0499 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java +++ b/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java @@ -98,13 +98,22 @@ public interface WithLazyParams<P extends InterceptorParams> { /** * Resolves configured params into a per-invocation holder, leaving the interceptor untouched. * <p> - * A {@code ${...}} expression that resolves to null or an empty value is not written: the - * holder keeps its seeded configuration value and is notified via - * {@link InterceptorParams#unresolved(String)}. This also catches an expression that - * legitimately evaluates to an empty string, which is indistinguishable from a failed - * resolution (see {@link #isUnresolved}); 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. + * <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 */ @@ -125,8 +134,9 @@ public interface WithLazyParams<P extends InterceptorParams> { // 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 [{}]; check the interceptor configuration", + 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); + target.unresolved(paramName); } } return target; @@ -141,6 +151,12 @@ public interface WithLazyParams<P extends InterceptorParams> { * to; a param that legitimately evaluates to an empty string is therefore also reported as * unresolved. That is a deliberate fail-closed choice: for a security-sensitive param (e.g. * an allowlist), silently accepting an unintended empty value is worse than refusing it. + * <p> + * <strong>Partial resolution is not detected.</strong> A value mixing several expressions, + * e.g. {@code ${a},${b}}, still parses to a non-empty string when only one of them resolves, + * so it is reported as resolved and the truncated value is written. For an allowlist that + * narrows the accepted set rather than widening it, so it does not relax validation, but the + * holder receives fewer entries than configured and gets no {@code unresolved} notification. */ private boolean isUnresolved(String rawValue, Object paramValue) { return rawValue != null diff --git a/core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java b/core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java index 46f5282f0..48ef39c56 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java @@ -51,6 +51,7 @@ public class LazyParamInjectorTest extends StrutsInternalTestCase { public String getLabel() { return "resolved-label"; } public Long getLimit() { return 4096L; } public String getBlank() { return ""; } + public String getNotANumber() { return "5MB"; } } private ActionContext context; @@ -127,6 +128,16 @@ public class LazyParamInjectorTest extends StrutsInternalTestCase { assertThat(holder.getUnresolvedCalls()).containsExactly("name"); } + public void testValueThatCannotBeConvertedSkipsWriteAndNotifiesHolder() { + Map<String, String> params = new HashMap<>(); + params.put("size", "${notANumber}"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getSize()).isNull(); + assertThat(holder.getUnresolvedCalls()).containsExactly("size"); + } + public void testResolvesDisabledOntoDisableParams() { Map<String, String> params = new HashMap<>(); params.put("disabled", "true"); @@ -136,7 +147,7 @@ public class LazyParamInjectorTest extends StrutsInternalTestCase { assertThat(holder.isDisabled()).isTrue(); } - public void testUnknownParamIsIgnoredWithoutFailingTheInvocation() { + public void testUnknownParamDoesNotFailTheInvocationButNotifiesHolder() { Map<String, String> params = new HashMap<>(); params.put("noSuchParam", "whatever"); @@ -144,5 +155,6 @@ public class LazyParamInjectorTest extends StrutsInternalTestCase { assertThat(holder.getName()).isNull(); assertThat(holder.getSize()).isNull(); + assertThat(holder.getUnresolvedCalls()).containsExactly("noSuchParam"); } } diff --git a/docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md b/docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md index 397b79be3..221dff23f 100644 --- a/docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md +++ b/docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md @@ -173,8 +173,9 @@ Two behaviours are added: expression. - **Unknown-param warning.** A param with no matching property on the holder currently no-ops silently, because `ognlUtil.setProperty` swallows the `OgnlException` - (`OgnlUtil.java:297-299`). Log a WARN. Not a regression — a typo no-ops today too — but - this design makes it more likely to matter. + (`OgnlUtil.java:297-299`). Set `throwPropertyExceptions=true`, log a WARN, and call + `unresolved(paramName)` — see the general rule under *Error handling*. Not a regression in + detection — a typo no-ops today too — but this design makes it more likely to matter. ### `DefaultActionInvocation` (changed) @@ -290,6 +291,25 @@ Under this design: `unresolved(paramName)` is called. - A WARN is logged naming interceptor, param and expression. +**The rule is general: every path in `resolveInto` that skips a write must notify the holder +via `unresolved(paramName)`.** Any skipped write leaves the holder reporting a value the +configuration did not ask for, and a holder that is not told cannot fail closed — it validates +against a dimension that was silently dropped, which is precisely the failure this ticket +exists to remove. There are three such paths: + +1. **Unresolvable or empty `${...}`** — detected by `isUnresolved`, as above. +2. **A resolved value the holder's setter cannot accept** — e.g. `${uploadConfig.maxFileSize}` + returning a non-numeric `String` for the `Long` `maximumSize`. OGNL conversion throws + `ReflectionException` (`resolveInto` sets `throwPropertyExceptions=true`), the write is + skipped, and `maximumSize` stays `null` — which `acceptFile` reads as "no size limit". Left + unnotified this branch is fail-*open*, so it must call `unresolved` too. +3. **A param with no matching property on the holder** — the same `ReflectionException` + (`NoSuchPropertyException`) from a typo'd param name. Config-time reflection does not reject + it (`DefaultInterceptorFactory` calls `setProperties` without `throwPropertyExceptions`), so + the lazy path is where it first surfaces; it is notified for the same reason. + +A new failure mode added to `resolveInto` later must be checked against this rule. + `UploadPolicy.unresolved(param)` records the parameter and marks the whole policy unusable, regardless of the seeded value. A static fallback therefore applies only when the param is absent from the lazy map entirely, i.e. pure static configuration, which never triggers `unresolved`.
