From 08c9ac40c4ebd80c261c90bf81d439c10d0053d5 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 10:01:09 +0200 Subject: [PATCH 01/23] WW-5659 docs: design for request-scoped lazy interceptor params WithLazyParams#injectParams resolves ${...} params onto the interceptor singleton, so concurrent requests can read one another's resolved values. For ActionFileUploadInterceptor that means allowedTypes, allowedExtensions, maximumSize and disabled can cross between requests. Design fixes the contract rather than the one implementer: resolved params go into a per-invocation holder the interceptor supplies and receives back, leaving the singleton immutable after init(). Adds InterceptorParams as the general contract with DisableParams as opt-in support for the disabled param, and makes unresolvable expressions fail closed instead of silently disabling validation. Reported via GitHub PR #1815; that approach (ThreadLocal on the interceptor) is not adopted. Co-Authored-By: Claude Opus 5 --- ...5659-lazy-params-request-scoping-design.md | 343 ++++++++++++++++++ 1 file changed, 343 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md 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 new file mode 100644 index 0000000000..20e8a7c962 --- /dev/null +++ b/docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md @@ -0,0 +1,343 @@ +# WW-5659: Request-scoped resolution of lazy interceptor params + +**Jira:** [WW-5659](https://issues.apache.org/jira/browse/WW-5659) +**Type:** Bug +**Affects:** 7.2.0, 7.2.1 +**Fix version:** 7.3.0 +**Date:** 2026-07-27 +**Reported via:** GitHub PR [#1815](https://github.com/apache/struts/pull/1815) (approach not adopted) + +## Problem + +`WithLazyParams.LazyParamInjector#injectParams` resolves `${...}` interceptor params +once per request and writes the resolved values straight onto the interceptor: + +```java +// WithLazyParams.java:78-84 +public Interceptor injectParams(Interceptor interceptor, Map params, ActionContext invocationContext) { + for (Map.Entry 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; +} +``` + +That interceptor is a singleton, built once at configuration-parse time +(`InterceptorBuilder.java:73-74`, `:175-177`) and reused for every request. When an +action references a stack without overriding params, `InterceptorBuilder.java:79` +(`result.addAll(stackConfig.getInterceptors())`) hands the same `InterceptorMapping` +objects to every such action, so the instance is shared across actions too. + +Request-scoped state is therefore written to process-wide state with no synchronisation. + +### Consequence for file upload validation + +`ActionFileUploadInterceptor` is the only `WithLazyParams` implementer today. Its +resolved policy lands in plain instance fields (`AbstractFileUploadInterceptor.java:62-64`, +written at `:85`, `:94`, `:103`) and is read back by `acceptFile` (`:134`, `:141`, `:148`). + +Per request: + +- `DefaultActionInvocation.java:269` — resolve, write the shared fields +- `DefaultActionInvocation.java:275` — `intercept()` → `acceptFile()` reads the shared fields + +Nothing guards the interval. Two concurrent requests resolving different policies can have +their `allowedTypes`, `allowedExtensions` and `maximumSize` cross over, so a request can be +validated against another request's upload policy. + +### `disabled` is affected too + +`AbstractInterceptor.java:28` holds `disabled` as a shared `boolean`, written by +`setDisabled` (`:56-58`) and read by `shouldIntercept` (`:61-63`). It travels through +`injectParams` like any other param, so `${...}` has the +same cross-request bleed — and it decides whether the interceptor runs at all. + +Scope note: for interceptors that are *not* `WithLazyParams`, `setDisabled` is only ever +called at config time by `buildInterceptor`, never per request. `disabled` is racy only on +the lazy path. + +### Secondary defect + +`DefaultActionInvocation.java:262-267` calls `params.putAll(...)` on the map returned by +`InterceptorMapping#getParams` (`:60-62`), which is the live shared map — an unsynchronised +write to a shared `HashMap` on every request. + +### Not affected + +`struts-default.xml:60` declares `actionFileUpload` with no params, so a default +configuration resolves nothing and writes nothing. Static (non-expression) params resolve +to the same value on every request. Only applications opting into the dynamic `${...}` +form added in WW-5585 are affected. + +`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 +is 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.java:136-139`. + +This was assessed as a thread-safety defect rather than a framework vulnerability. Released +7.2.0 and 7.2.1 are not being backported. + +## Decision + +Fix the `WithLazyParams` contract rather than patching its one implementer, so no future +implementer can reintroduce the bug. Resolved params are written into a per-invocation +object the interceptor supplies and receives back; the interceptor singleton stays +immutable after `init()`. + +Rejected alternatives: + +- **Per-invocation copy of the interceptor** (`copyForInvocation()`): smallest diff, but a + copy constructor rots silently when a field is added, it copies injected container + dependencies, and it creates transient instances that never receive `destroy()`. It hides + the mutable snapshot rather than separating config-time from request-time state. +- **Interceptor-owned resolution** (framework stops injecting, interceptor resolves raw + templates itself): loses OGNL type conversion, duplicates resolution in every + implementer, and silently changes what the setters mean. +- **`ThreadLocal` on the interceptor** (PR #1815): makes setter behaviour depend on hidden + thread state, requires a cleanup call that any implementer can forget, and leaves the + unsafe write path in place, merely bypassed. + +This is a clean interface break in the next minor. `WithLazyParams` is public API since +2.5.9, but `ActionFileUploadInterceptor` is its only implementer in the repo; third-party +implementers get a compile error rather than silent breakage, noted in the migration guide. + +## Components + +### `InterceptorParams` (new) + +`org.apache.struts2.interceptor.InterceptorParams` — the general contract for a params +holder, and the generic bound for `WithLazyParams`. + +```java +public interface InterceptorParams { + /** Notified when a {@code ${...}} param could not be resolved for this invocation. */ + default void unresolved(String paramName) { } +} +``` + +The default no-op keeps the interface free for implementers that do not care. `UploadPolicy` +overrides it to support fail-closed validation. + +### `DisableParams` (new) + +`org.apache.struts2.interceptor.DisableParams` — opt-in support for the `disabled` param. +A class, not an interface, because it holds state and needs a setter for OGNL. + +```java +public class DisableParams implements InterceptorParams { + private boolean disabled; + public void setDisabled(String disable) { this.disabled = Boolean.parseBoolean(disable); } + public boolean isDisabled() { return disabled; } +} +``` + +`disabled` is universal across interceptors while lazy resolution is rare, so `DisableParams` +is *not* a subtype of any lazy-specific type; both sit under `InterceptorParams`. + +A holder that does not extend `DisableParams` has no `disabled` property, so a `disabled` +param on such an interceptor resolves to nothing and is reported by the unknown-param +warning below. There is deliberately no fallback to the singleton — that is the racy path +being removed. Interceptors supporting `disabled` must extend `DisableParams`. + +### `WithLazyParams` (changed) + +```java +public interface WithLazyParams

{ + P newLazyParams(); + String intercept(ActionInvocation invocation, P lazyParams) throws Exception; +} +``` + +`AbstractInterceptor.java:48` declares `intercept(ActionInvocation)` abstract, and a +class-declared abstract method takes precedence over an interface default, so a `default` +single-arg implementation on this interface would not satisfy it. Implementers write the +one-line delegation themselves. + +### `LazyParamInjector` (changed) + +`injectParams(Interceptor, ...)` becomes `resolveInto(InterceptorParams target, ...)`. It +still uses `textParser.evaluate` followed by `ognlUtil.setProperty`, so OGNL type conversion +against the holder's typed setters is preserved (`maximumSize` still converts `String` → +`Long`). + +Two behaviours are added: + +- **Unresolved detection.** `OgnlTextParser.java:85-88` yields `""` for an expression that + does not resolve and gives no signal distinguishing that from a legitimate empty value. + The injector holds the raw template, so `raw.contains("${") && resolved.isEmpty()` + recovers it. On unresolved: skip the write (the holder keeps its seeded config-time + value), call `target.unresolved(paramName)`, and log a WARN naming interceptor, param and + 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. + +### `DefaultActionInvocation` (changed) + +`AbstractInterceptor.java:26` implements `ConditionalInterceptor`, so every interceptor +extending it — `ActionFileUploadInterceptor` included — takes the `instanceof +ConditionalInterceptor` branch at `:271` and is invoked through `executeConditional` → +`conditionalInterceptor.intercept(this)` at `:318`. That works today only because +`injectParams` had already mutated the singleton. Under the new contract the lazy and +conditional paths must be merged, or the single-arg `intercept` would run with unresolved +values and the dynamic policy would silently vanish. + +```java +private

String invokeWithLazyParams(WithLazyParams

lazy, + InterceptorMapping mapping) throws Exception { + P params = lazy.newLazyParams(); + lazyParamInjector.resolveInto(params, mergedParams(mapping), invocationContext); + if (params instanceof DisableParams dp && dp.isDisabled()) { + return invoke(); + } + if (lazy instanceof ConditionalInterceptor ci && !ci.shouldIntercept(this)) { + return invoke(); + } + return lazy.intercept(this, params); +} +``` + +Checking both `isDisabled()` and `shouldIntercept` keeps custom `shouldIntercept` overrides +working while making the lazily-resolved `disabled` request-scoped. The singleton's +`disabled` field is never written on this path, so `AbstractInterceptor.shouldIntercept` +returns `true` and the holder is authoritative. + +`mergedParams(mapping)` returns a fresh map instead of mutating the shared one, retiring the +`putAll` at `:262-267`. + +### `UploadPolicy` (new) + +`org.apache.struts2.interceptor.UploadPolicy` — top-level, not nested, because it appears in +the interceptor's public signature. + +```java +public class UploadPolicy extends DisableParams { + private Long maximumSize; + private Set allowedTypes = Collections.emptySet(); + private Set allowedExtensions = Collections.emptySet(); + // typed setters (OGNL converts String -> Long for maximumSize), getters, copy() + // overrides unresolved(String) to record dimensions that could not be resolved +} +``` + +### `AbstractFileUploadInterceptor` / `ActionFileUploadInterceptor` (changed) + +The interceptor holds a single config-time `UploadPolicy` rather than keeping the three +fields at `:62-64` *and* adding a holder: + +```java +private final UploadPolicy configuredPolicy = new UploadPolicy(); + +public void setAllowedTypes(String csv) { configuredPolicy.setAllowedTypes(csv); } // config-time only +// ... +@Override public UploadPolicy newLazyParams() { return configuredPolicy.copy(); } +``` + +Public setter signatures are unchanged, so static `struts.xml` config and `buildInterceptor` +reflection are untouched. They are documented as config-time only, which is what they +already are — no framework code calls them outside OGNL injection. + +`acceptFile` takes the policy explicitly, and `getMaximumSizeStr` (`:164-166`) takes the +value rather than reading a field: + +```java +protected boolean acceptFile(UploadPolicy policy, Object action, UploadedFile file, + String originalFilename, String contentType, String inputName) +``` + +This is a `protected` break for any subclass overriding `acceptFile`. + +`ActionFileUploadInterceptor implements WithLazyParams`; its single-arg +`intercept` delegates to `intercept(invocation, newLazyParams())` so direct use outside the +lazy path still works. + +## Data flow + +**Config time (once at startup)** — unchanged. `InterceptorBuilder` → +`objectFactory.buildInterceptor(config, params)` reflects params onto the singleton, +populating `configuredPolicy` with literal values including any raw `${...}` text. +`InterceptorMapping` stores the raw param map. + +**Request time (per invocation)** + +1. `DefaultActionInvocation.invoke()` takes the next `InterceptorMapping`; + `instanceof WithLazyParams` routes to `invokeWithLazyParams`. +2. `mergedParams(mapping)` builds a fresh map. +3. `lazy.newLazyParams()` → `configuredPolicy.copy()`. +4. `lazyParamInjector.resolveInto(holder, merged, invocationContext)`. +5. `disabled` check, then `shouldIntercept`. +6. `lazy.intercept(invocation, holder)`. +7. The holder becomes garbage when the invocation ends. + +Step 7 is the design's own check: **there is no cleanup step.** No `finally`, nothing to +clear, no `ThreadLocal` to leak. If a future change makes cleanup necessary, the separation +has regressed. + +## Error handling + +Unresolvable `${...}` is **fail-closed**. Today it produces `""` +(`OgnlTextParser.java:85-88`) → empty set (`TextParseUtil.java:257`) → treated as "no +restriction" by `acceptFile` (`:141`, `:148` both guard on `!isEmpty()`), so a typo or a +null intermediate silently switches off an upload restriction. + +Under this design: + +- The write is skipped, so the holder keeps its seeded config-time value, and + `unresolved(paramName)` is called. +- A WARN is logged naming interceptor, param and expression. + +`UploadPolicy.unresolved(param)` then has to decide whether the seeded value is usable. Two +cases exist, and they are distinguishable: + +- **A genuine static fallback.** The interceptor's own `` definition carried a + literal value for that param and the `` overrode it with an expression, + so the seed is a real value (e.g. `image/png`). That value applies and validation + proceeds normally. +- **No fallback.** The only configuration for that param is the expression itself, so + `buildInterceptor` seeded the holder with the literal `${...}` text — as a set containing + the string `"${uploadConfig.allowedMimeTypes}"`, which matches no content type. + +The rule: `unresolved(param)` marks the dimension unusable **only if** the seeded value for +that dimension still contains `${`. Otherwise a genuine static fallback exists and is used. +A dimension marked unusable causes `acceptFile` to reject the file with a dedicated message +rather than the opaque one produced by matching content types against literal `${...}` text. +This needs a new bundle key — `struts.messages.error.upload.policy.unresolved` — added to +the shipped properties. + +This is a behaviour change for 7.2.x applications with a broken expression. Those +applications are currently running with that validation silently disabled, which is the +reason to surface it. + +## Testing + +`ActionFileUploadInterceptorTest` is JUnit 3 style — `protected void setUp()`, plain +`public void testX()` methods, no annotations. A JUnit 5 `@Test` added there compiles and +silently never runs. New tests must follow the existing style. + +- Carry over PR #1815's concurrency regression, adapted to the new signature, retaining the + contributor's attribution. Its scenario is the acceptance criterion. +- Direct guard for the defect: after an invocation resolves a policy, assert + `newLazyParams()` still returns the configured values — the singleton was never written. +- `disabled` request-scoping: concurrent invocations resolving different `disabled` values; + assert only the intended one is skipped. +- Fail-closed: unresolved expression with no static fallback → rejected with the new + message; unresolved *with* a static fallback (literal on the `` definition, + expression on the ``) → the static value applies and validation proceeds. +- `ConditionalInterceptor` interaction: a `WithLazyParams` interceptor that is also + conditional still honours a custom `shouldIntercept`. +- Migrate existing dynamic tests (`testDynamicParameterEvaluation` and friends) off the + "simulate injection by calling the setter directly" shortcut onto real `LazyParamInjector` + calls. PR #1815 already started this. + +## Out of scope + +Defining *all* interceptor params via dedicated params classes, rather than loose setters on +each interceptor, is the natural extension of `InterceptorParams`. It would touch 44 +interceptor implementations across core and plugins plus every third-party interceptor, so +it is an ecosystem-wide breaking change belonging to 8.0.0 alongside the struts2-api +extraction (WW-4759). It is not required for correctness here: `disabled` is racy only on +the lazy path, which this change already covers. A separate ticket will be filed. From e51a5ce36dc33f1a784a971e3249afddf62b5367 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 10:10:01 +0200 Subject: [PATCH 02/23] WW-5659 docs: implementation plan for request-scoped lazy params Six tasks, each independently testable and compiling: new InterceptorParams and DisableParams types, LazyParamInjector.resolveInto alongside the old path, a pure refactor onto a single UploadPolicy value object, the contract switch plus DefaultActionInvocation wiring, fail-closed handling, and test migration. Records one deviation from the spec: the unresolved-param rule is applied unconditionally rather than by introspecting the seeded value, which is not implementable deterministically for the Long-typed maximumSize. Task 6 updates the spec to match. Co-Authored-By: Claude Opus 5 --- ...-27-WW-5659-lazy-params-request-scoping.md | 1323 +++++++++++++++++ 1 file changed, 1323 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-27-WW-5659-lazy-params-request-scoping.md diff --git a/docs/superpowers/plans/2026-07-27-WW-5659-lazy-params-request-scoping.md b/docs/superpowers/plans/2026-07-27-WW-5659-lazy-params-request-scoping.md new file mode 100644 index 0000000000..f1af340e83 --- /dev/null +++ b/docs/superpowers/plans/2026-07-27-WW-5659-lazy-params-request-scoping.md @@ -0,0 +1,1323 @@ +# WW-5659 Request-Scoped Lazy Interceptor Params Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Stop `WithLazyParams` from writing per-request resolved `${...}` params onto the shared interceptor singleton, so concurrent requests can no longer read one another's upload validation policy. + +**Architecture:** Resolved params are written into a per-invocation holder object that the interceptor supplies (`newLazyParams()`) and receives back (`intercept(invocation, params)`). The interceptor singleton becomes immutable after `init()`. A new `InterceptorParams` marker is the general contract; `DisableParams` is opt-in support for the `disabled` param; `UploadPolicy` is the file-upload holder. Unresolvable expressions fail closed instead of silently disabling validation. + +**Tech Stack:** Java 17, Maven, JUnit 3-style tests (`junit.framework.TestCase` via `XWorkTestCase`), AssertJ assertions, Log4j2. + +**Spec:** `docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md` + +## Global Constraints + +- **Ticket prefix:** every commit message starts with `WW-5659`. +- **Target version:** 7.3.0. This is a deliberate public API break; no backport to 7.2.x. +- **Test style:** `core` tests are JUnit 3 style — extend `StrutsInternalTestCase` (which extends `XWorkTestCase`, which extends `junit.framework.TestCase`), use `protected void setUp()`, and name tests `public void testXxx()`. **A JUnit 5 `@Test` annotation compiles and silently never runs.** Never add one. +- **Assertions:** use AssertJ (`import static org.assertj.core.api.Assertions.assertThat;`), matching the existing `ActionFileUploadInterceptorTest`. +- **Package:** all new production types go in `org.apache.struts2.interceptor`. +- **License header:** every new `.java` file starts with the ASF header, copied verbatim from `core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java:1-18`. +- **Message bundles:** new keys go into `core/src/main/resources/org/apache/struts2/struts-messages.properties` **only**. The five locale bundles (`_da`, `_de`, `_en`, `_pl`, `_pt`) are partial (12–14 keys vs 20 in the base) and fall back to the base bundle. Do not fabricate translations. +- **Build command:** `mvn test -DskipAssembly -pl core -Dtest=ClassName#methodName` + +## Deviation from the spec — read before Task 5 + +The spec's Error Handling section says `unresolved(param)` should mark a dimension unusable **only if** the seeded config-time value still contains `${`. That rule is not implementable deterministically: it relies on introspecting the seed, and for `maximumSize` the seed is a `Long`, so a `${...}` literal cannot survive there — whether config-time OGNL conversion of `"${maxFileSize}"` to `Long` yields `null`, throws, or skips the setter is not knowable without testing, and the rule would silently behave differently per param type. + +**This plan implements the simpler, deterministic rule instead: on unresolved, the dimension is marked unusable unconditionally.** A static fallback then applies only when the param is absent from the lazy map entirely (pure static config), which never triggers `unresolved`. This is stricter, type-independent, and consistent with the chosen fail-closed policy. + +Task 6 includes a step to update the spec to match. **If the reviewer prefers the spec's original rule, stop and re-plan Task 5** rather than implementing both. + +## File Structure + +**Created:** +- `core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java` — general params-holder contract; generic bound for `WithLazyParams`. +- `core/src/main/java/org/apache/struts2/interceptor/DisableParams.java` — opt-in holder state for the `disabled` param. +- `core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java` — file-upload holder: sizes, types, extensions, unresolved tracking. +- `core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java` +- `core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java` + +**Modified:** +- `core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java` — new generic contract; `injectParams` → `resolveInto`. +- `core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java` — single config-time `UploadPolicy`; `acceptFile` takes the policy. +- `core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java` — implements `WithLazyParams`. +- `core/src/main/java/org/apache/struts2/DefaultActionInvocation.java:258-276` — merged lazy/conditional path; non-mutating `mergedParams`. +- `core/src/main/resources/org/apache/struts2/struts-messages.properties` — new fail-closed message key. +- `core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java` — regression + migration. + +--- + +### Task 1: `InterceptorParams` and `DisableParams` + +Purely additive — nothing references these yet. Establishes the vocabulary the rest of the plan builds on. + +**Files:** +- Create: `core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java` +- Create: `core/src/main/java/org/apache/struts2/interceptor/DisableParams.java` +- Test: `core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java` + +**Interfaces:** +- Consumes: nothing. +- Produces: `interface InterceptorParams { default void unresolved(String paramName) {} }`; `class DisableParams implements InterceptorParams` with `void setDisabled(String)`, `boolean isDisabled()`, and `protected DisableParams(DisableParams other)` copy constructor. + +- [ ] **Step 1: Write the failing test** + +Create `core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java` (ASF header first): + +```java +package org.apache.struts2.interceptor; + +import junit.framework.TestCase; + +import static org.assertj.core.api.Assertions.assertThat; + +public class DisableParamsTest extends TestCase { + + public void testDisabledDefaultsToFalse() { + assertThat(new DisableParams().isDisabled()).isFalse(); + } + + public void testSetDisabledParsesStringValue() { + DisableParams params = new DisableParams(); + params.setDisabled("true"); + assertThat(params.isDisabled()).isTrue(); + + params.setDisabled("false"); + assertThat(params.isDisabled()).isFalse(); + } + + public void testSetDisabledTreatsNonBooleanTextAsFalse() { + DisableParams params = new DisableParams(); + params.setDisabled("yes"); + assertThat(params.isDisabled()).isFalse(); + } + + public void testCopyConstructorCarriesDisabledFlag() { + DisableParams original = new DisableParams(); + original.setDisabled("true"); + + DisableParams copy = new DisableParams(original); + + assertThat(copy.isDisabled()).isTrue(); + } + + public void testUnresolvedDefaultsToNoOp() { + DisableParams params = new DisableParams(); + params.unresolved("someParam"); // must not throw + assertThat(params.isDisabled()).isFalse(); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=DisableParamsTest` +Expected: compilation failure — `DisableParams` does not exist. + +- [ ] **Step 3: Write minimal implementation** + +Create `core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java` (ASF header first): + +```java +package org.apache.struts2.interceptor; + +/** + * Contract for an object holding the parameters of a single interceptor. + *

+ * Implementations are per-invocation value objects: the framework resolves configured + * parameters into a fresh instance for each action invocation, so nothing is written back + * onto the interceptor, which stays immutable after {@link Interceptor#init()}. + * + * @since 7.3.0 + */ +public interface InterceptorParams { + + /** + * Called when a {@code ${...}} parameter could not be resolved for the current invocation. + * The framework skips the write, leaving the seeded configuration value in place, and + * notifies the holder so it can decide how to degrade. + *

+ * The default implementation does nothing. + * + * @param paramName name of the parameter that could not be resolved + */ + default void unresolved(String paramName) { + } +} +``` + +Create `core/src/main/java/org/apache/struts2/interceptor/DisableParams.java` (ASF header first): + +```java +package org.apache.struts2.interceptor; + +/** + * Opt-in support for the {@code disabled} interceptor parameter. + *

+ * Interceptors implementing {@link WithLazyParams} must have their params holder extend this + * class to support {@code ...}; there is deliberately no + * fallback to the interceptor instance, which would reintroduce shared mutable state. + * + * @since 7.3.0 + */ +public class DisableParams implements InterceptorParams { + + private boolean disabled; + + public DisableParams() { + } + + protected DisableParams(DisableParams other) { + this.disabled = other.disabled; + } + + /** + * @param disable if {@code true}, execution of the interceptor is skipped for this invocation + */ + public void setDisabled(String disable) { + this.disabled = Boolean.parseBoolean(disable); + } + + public boolean isDisabled() { + return disabled; + } +} +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `mvn test -DskipAssembly -pl core -Dtest=DisableParamsTest` +Expected: PASS, 5 tests. + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java \ + core/src/main/java/org/apache/struts2/interceptor/DisableParams.java \ + core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java +git commit -m "WW-5659 feat(core): add InterceptorParams contract and DisableParams holder" +``` + +--- + +### Task 2: `LazyParamInjector.resolveInto` + +Adds the new resolution method **alongside** the existing `injectParams`, so the tree keeps compiling. `injectParams` is removed in Task 4 when the call site moves. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java:78-84` +- Test: `core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java` + +**Interfaces:** +- Consumes: `InterceptorParams` from Task 1. +- Produces: `public

P resolveInto(P target, Map params, ActionContext invocationContext)` on `WithLazyParams.LazyParamInjector`. + +- [ ] **Step 1: Write the failing test** + +Create `core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java` (ASF header first): + +```java +package org.apache.struts2.interceptor; + +import org.apache.struts2.ActionContext; +import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.ValueStackFactory; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; + +public class LazyParamInjectorTest extends StrutsInternalTestCase { + + public static class Holder extends DisableParams { + private String name; + private Long size; + private final List unresolvedCalls = new ArrayList<>(); + + public void setName(String name) { this.name = name; } + public void setSize(Long size) { this.size = size; } + public String getName() { return name; } + public Long getSize() { return size; } + public List getUnresolvedCalls() { return unresolvedCalls; } + + @Override + public void unresolved(String paramName) { unresolvedCalls.add(paramName); } + } + + public static class Bean { + public String getLabel() { return "resolved-label"; } + public Long getLimit() { return 4096L; } + } + + private ActionContext context; + private WithLazyParams.LazyParamInjector injector; + + @Override + protected void setUp() throws Exception { + super.setUp(); + ValueStack stack = container.getInstance(ValueStackFactory.class).createValueStack(); + stack.push(new Bean()); + context = ActionContext.of(stack.getContext()).withContainer(container).withValueStack(stack).bind(); + injector = new WithLazyParams.LazyParamInjector(stack); + container.inject(injector); + } + + @Override + protected void tearDown() throws Exception { + ActionContext.clear(); + super.tearDown(); + } + + public void testResolvesExpressionsIntoTheHolder() { + Map params = new HashMap<>(); + params.put("name", "${label}"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getName()).isEqualTo("resolved-label"); + assertThat(holder.getUnresolvedCalls()).isEmpty(); + } + + public void testAppliesOgnlTypeConversion() { + Map params = new HashMap<>(); + params.put("size", "${limit}"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getSize()).isEqualTo(4096L); + } + + public void testPassesLiteralValuesThrough() { + Map params = new HashMap<>(); + params.put("name", "plain-text"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getName()).isEqualTo("plain-text"); + assertThat(holder.getUnresolvedCalls()).isEmpty(); + } + + public void testUnresolvableExpressionSkipsWriteAndNotifiesHolder() { + Holder seeded = new Holder(); + seeded.setName("seeded-value"); + + Map params = new HashMap<>(); + params.put("name", "${noSuchProperty}"); + + Holder holder = injector.resolveInto(seeded, params, context); + + assertThat(holder.getName()).isEqualTo("seeded-value"); + assertThat(holder.getUnresolvedCalls()).containsExactly("name"); + } + + public void testResolvesDisabledOntoDisableParams() { + Map params = new HashMap<>(); + params.put("disabled", "true"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.isDisabled()).isTrue(); + } + + public void testUnknownParamIsIgnoredWithoutFailingTheInvocation() { + Map params = new HashMap<>(); + params.put("noSuchParam", "whatever"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getName()).isNull(); + assertThat(holder.getSize()).isNull(); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=LazyParamInjectorTest` +Expected: compilation failure — `resolveInto` does not exist. + +- [ ] **Step 3: Write minimal implementation** + +In `WithLazyParams.java`, add these imports: + +```java +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.util.reflection.ReflectionException; +``` + +Add a logger as the first member of `class LazyParamInjector`: + +```java +private static final Logger LOG = LogManager.getLogger(LazyParamInjector.class); +``` + +Add the new method **below** the existing `injectParams` (leave `injectParams` in place for now): + +```java +/** + * Resolves configured params into a per-invocation holder, leaving the interceptor untouched. + *

+ * A {@code ${...}} expression that cannot be resolved is not written: the holder keeps its + * seeded configuration value and is notified via {@link InterceptorParams#unresolved(String)}, + * so a broken expression cannot silently relax a validation policy. + * + * @since 7.3.0 + */ +public

P resolveInto(P target, Map params, ActionContext invocationContext) { + for (Map.Entry entry : params.entrySet()) { + 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 [{}]; check the interceptor configuration", + paramName, target.getClass().getName(), e); + } + } + return target; +} + +/** + * {@link org.apache.struts2.util.OgnlTextParser} yields an empty string for an expression that + * does not resolve and gives no other signal, so the raw template is needed to tell that apart + * from a legitimately empty value. + */ +private boolean isUnresolved(String rawValue, Object paramValue) { + return rawValue != null + && rawValue.contains("${") + && (paramValue == null || paramValue.toString().isEmpty()); +} +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `mvn test -DskipAssembly -pl core -Dtest=LazyParamInjectorTest` +Expected: PASS, 6 tests. + +- [ ] **Step 5: Confirm nothing else broke** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest,DefaultActionInvocationTest` +Expected: PASS — `injectParams` is untouched, so existing behaviour is unchanged. + +- [ ] **Step 6: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java \ + core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java +git commit -m "WW-5659 feat(core): resolve lazy params into a holder instead of the interceptor" +``` + +--- + +### Task 3: `UploadPolicy` and the `acceptFile` signature + +Pure refactor: the interceptor stops holding three loose fields and holds one config-time `UploadPolicy`; `acceptFile` receives the policy explicitly. **No behaviour change** — the old `injectParams` path still mutates the singleton via the setters, which now delegate into `configuredPolicy`. Task 4 removes that. + +**Files:** +- Create: `core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java` +- Modify: `core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java:62-64, 84-104, 116-166` +- Modify: `core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java:210-264` + +**Interfaces:** +- Consumes: `DisableParams` from Task 1. +- Produces: `class UploadPolicy extends DisableParams` with `setMaximumSize(Long)`, `setAllowedTypes(String)`, `setAllowedExtensions(String)`, `getMaximumSize()`, `getAllowedTypes()`, `getAllowedExtensions()`, `copy()`; and `protected boolean acceptFile(UploadPolicy policy, Object action, UploadedFile file, String originalFilename, String contentType, String inputName)`. + +- [ ] **Step 1: Write the failing test** + +Append to `core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java`, before the closing brace: + +```java + public void testUploadPolicyParsesAndCopies() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes("text/plain, text/html"); + policy.setAllowedExtensions(".txt,.html"); + policy.setMaximumSize(1024L); + policy.setDisabled("true"); + + UploadPolicy copy = policy.copy(); + + assertThat(copy.getAllowedTypes()).containsExactlyInAnyOrder("text/plain", "text/html"); + assertThat(copy.getAllowedExtensions()).containsExactlyInAnyOrder(".txt", ".html"); + assertThat(copy.getMaximumSize()).isEqualTo(1024L); + assertThat(copy.isDisabled()).isTrue(); + } + + public void testUploadPolicyCopyIsIndependentOfTheOriginal() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes("text/plain"); + + UploadPolicy copy = policy.copy(); + copy.setAllowedTypes("text/html"); + + assertThat(policy.getAllowedTypes()).containsExactly("text/plain"); + assertThat(copy.getAllowedTypes()).containsExactly("text/html"); + } + + public void testUploadPolicyTreatsNullAsNoRestriction() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes(null); + policy.setAllowedExtensions(null); + + assertThat(policy.getAllowedTypes()).isEmpty(); + assertThat(policy.getAllowedExtensions()).isEmpty(); + } +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest#testUploadPolicyParsesAndCopies` +Expected: compilation failure — `UploadPolicy` does not exist. + +- [ ] **Step 3: Create `UploadPolicy`** + +Create `core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java` (ASF header first): + +```java +package org.apache.struts2.interceptor; + +import org.apache.struts2.util.TextParseUtil; + +import java.util.Collections; +import java.util.Set; + +/** + * Per-invocation upload validation policy for {@link ActionFileUploadInterceptor}. + *

+ * A configured instance is held by the interceptor and copied for each invocation, so lazily + * resolved values never reach the shared interceptor. + * + * @since 7.3.0 + */ +public class UploadPolicy extends DisableParams { + + private Long maximumSize; + private Set allowedTypes = Collections.emptySet(); + private Set allowedExtensions = Collections.emptySet(); + + public UploadPolicy() { + } + + private UploadPolicy(UploadPolicy other) { + super(other); + this.maximumSize = other.maximumSize; + this.allowedTypes = other.allowedTypes; + this.allowedExtensions = other.allowedExtensions; + } + + /** + * @param allowedTypes a comma-delimited list of content types, or null for no restriction + */ + public void setAllowedTypes(String allowedTypes) { + this.allowedTypes = toSet(allowedTypes); + } + + /** + * @param allowedExtensions a comma-delimited list of extensions, or null for no restriction + */ + public void setAllowedExtensions(String allowedExtensions) { + this.allowedExtensions = toSet(allowedExtensions); + } + + /** + * @param maximumSize the maximum size in bytes, or null for no limit + */ + public void setMaximumSize(Long maximumSize) { + this.maximumSize = maximumSize; + } + + public Long getMaximumSize() { + return maximumSize; + } + + public Set getAllowedTypes() { + return allowedTypes; + } + + public Set getAllowedExtensions() { + return allowedExtensions; + } + + /** + * @return an independent copy, used to seed a per-invocation policy from the configured one + */ + public UploadPolicy copy() { + return new UploadPolicy(this); + } + + private static Set toSet(String commaDelimited) { + return commaDelimited == null + ? Collections.emptySet() + : TextParseUtil.commaDelimitedStringToSet(commaDelimited); + } +} +``` + +Note the null guard: `TextParseUtil.commaDelimitedStringToSet` throws `NullPointerException` on null (`TextParseUtil.java:257` calls `s.split(",")` unguarded), and the old setters had the same defect. Treating null as "no restriction" matches how an empty value already behaves. + +- [ ] **Step 4: Rewrite the policy state in `AbstractFileUploadInterceptor`** + +Replace the three fields at `:62-64`: + +```java + private Long maximumSize; + private Set allowedTypesSet = Collections.emptySet(); + private Set allowedExtensionsSet = Collections.emptySet(); +``` + +with: + +```java + private final UploadPolicy configuredPolicy = new UploadPolicy(); +``` + +Replace the three setters at `:84-104` with delegating versions: + +```java + /** + * Sets the allowed extensions. Applied at configuration time only; the effective policy for + * an invocation is a copy, see {@link ActionFileUploadInterceptor#newLazyParams()}. + * + * @param allowedExtensions A comma-delimited list of extensions + */ + public void setAllowedExtensions(String allowedExtensions) { + configuredPolicy.setAllowedExtensions(allowedExtensions); + } + + /** + * Sets the allowed mimetypes. Applied at configuration time only; the effective policy for + * an invocation is a copy, see {@link ActionFileUploadInterceptor#newLazyParams()}. + * + * @param allowedTypes A comma-delimited list of types + */ + public void setAllowedTypes(String allowedTypes) { + configuredPolicy.setAllowedTypes(allowedTypes); + } + + /** + * Sets the maximum size of an uploaded file. Applied at configuration time only; the + * effective policy for an invocation is a copy, see + * {@link ActionFileUploadInterceptor#newLazyParams()}. + * + * @param maximumSize The maximum size in bytes + */ + public void setMaximumSize(Long maximumSize) { + configuredPolicy.setMaximumSize(maximumSize); + } + + /** + * @return an independent copy of the configured policy, to be resolved for one invocation + * @since 7.3.0 + */ + protected UploadPolicy copyConfiguredPolicy() { + return configuredPolicy.copy(); + } +``` + +- [ ] **Step 5: Change `acceptFile` to take the policy** + +Replace the signature and the three checks at `:116` and `:134-154`: + +```java + protected boolean acceptFile(UploadPolicy policy, Object action, UploadedFile file, String originalFilename, String contentType, String inputName) { +``` + +and inside it: + +```java + if (policy.getMaximumSize() != null && policy.getMaximumSize() < file.length()) { + String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_FILE_TOO_LARGE_KEY, new String[]{ + inputName, originalFilename, file.getName(), "" + file.length(), getMaximumSizeStr(action, policy.getMaximumSize()) + }); + errorMessages.add(errMsg); + LOG.warn(errMsg); + } + if ((!policy.getAllowedTypes().isEmpty()) && (!containsItem(policy.getAllowedTypes(), contentType))) { + String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_CONTENT_TYPE_NOT_ALLOWED_KEY, new String[]{ + inputName, originalFilename, file.getName(), contentType + }); + errorMessages.add(errMsg); + LOG.warn(errMsg); + } + if ((!policy.getAllowedExtensions().isEmpty()) && (!hasAllowedExtension(policy.getAllowedExtensions(), originalFilename))) { + String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_FILE_EXTENSION_NOT_ALLOWED_KEY, new String[]{ + inputName, originalFilename, file.getName(), contentType + }); + errorMessages.add(errMsg); + LOG.warn(errMsg); + } +``` + +Replace `getMaximumSizeStr` at `:164-166`: + +```java + private String getMaximumSizeStr(Object action, Long maximumSize) { + return NumberFormat.getNumberInstance(getLocaleProvider(action).getLocale()).format(maximumSize); + } +``` + +Remove the now-unused `java.util.Collections` import if the compiler flags it. + +- [ ] **Step 6: Update the single caller** + +In `ActionFileUploadInterceptor.java`, at the top of `intercept` (`:210`), after the `UploadedFilesAware` check at `:229`, add: + +```java + UploadPolicy policy = copyConfiguredPolicy(); +``` + +and change the call at `:248`: + +```java + if (acceptFile(policy, action, uploadedFile, uploadedFile.getOriginalName(), uploadedFile.getContentType(), inputName)) { +``` + +- [ ] **Step 7: Run tests to verify they pass** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest` +Expected: PASS — the three new `UploadPolicy` tests plus every pre-existing test, unchanged. If a pre-existing test fails, the refactor changed behaviour and must be corrected, not the test. + +- [ ] **Step 8: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java \ + core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java \ + core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java \ + core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +git commit -m "WW-5659 refactor(core): hold upload policy in one value object" +``` + +--- + +### Task 4: New `WithLazyParams` contract and invocation wiring + +The behaviour change. After this task the interceptor singleton is never written per request. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java` +- Modify: `core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java` +- Modify: `core/src/main/java/org/apache/struts2/DefaultActionInvocation.java:258-276` +- Test: `core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java` + +**Interfaces:** +- Consumes: `resolveInto` (Task 2), `UploadPolicy` and `copyConfiguredPolicy()` (Task 3). +- Produces: `interface WithLazyParams

{ P newLazyParams(); String intercept(ActionInvocation, P) throws Exception; }`; `ActionFileUploadInterceptor implements WithLazyParams`. + +- [ ] **Step 1: Write the failing tests** + +Append to `ActionFileUploadInterceptorTest`, before the closing brace. The first test is carried over from PR #1815 by @deprrous, adapted to the new contract — keep the attribution comment. + +```java + /** + * Regression for WW-5659: two concurrent invocations resolving different policies must not + * see each other's values. Scenario contributed by @deprrous in GitHub PR #1815. + */ + public void testConcurrentDynamicPoliciesStayIsolatedPerRequest() throws Exception { + CoordinatedActionFileUploadInterceptor sharedInterceptor = new CoordinatedActionFileUploadInterceptor(); + container.inject(sharedInterceptor); + + MyDynamicFileUploadAction plainPolicyAction = new MyDynamicFileUploadAction(); + plainPolicyAction.setAllowedMimeTypes("text/plain"); + container.inject(plainPolicyAction); + + MyDynamicFileUploadAction htmlPolicyAction = new MyDynamicFileUploadAction(); + htmlPolicyAction.setAllowedMimeTypes("text/html"); + container.inject(htmlPolicyAction); + + ExecutorService executor = Executors.newFixedThreadPool(2); + try { + Future plainResult = executor.submit(() -> runUploadAttempt( + sharedInterceptor, plainPolicyAction, createUploadRequest("plain-policy.html", "text/html", htmlContent))); + + assertThat(sharedInterceptor.awaitFirstValidation()).isTrue(); + + Future htmlResult = executor.submit(() -> runUploadAttempt( + sharedInterceptor, htmlPolicyAction, createUploadRequest("html-policy.html", "text/html", htmlContent))); + + assertThat(htmlResult.get(10, TimeUnit.SECONDS)).isEqualTo("success"); + sharedInterceptor.releaseFirstValidation(); + assertThat(plainResult.get(10, TimeUnit.SECONDS)).isEqualTo("success"); + } finally { + sharedInterceptor.releaseFirstValidation(); + executor.shutdownNow(); + sharedInterceptor.destroy(); + } + + // the text/plain policy must have rejected the text/html upload despite the concurrent + // text/html invocation resolving a more permissive policy on the same interceptor + assertThat(plainPolicyAction.getUploadFiles()).isNull(); + assertThat(plainPolicyAction.getFieldErrors()).containsKey("file"); + + assertThat(htmlPolicyAction.hasFieldErrors()).isFalse(); + assertThat(htmlPolicyAction.getUploadFiles()).isNotNull().hasSize(1); + assertThat(htmlPolicyAction.getUploadFiles().get(0).getOriginalName()).isEqualTo("html-policy.html"); + } + + /** + * Regression for WW-5659: resolving an invocation's params must leave the interceptor + * singleton exactly as configured. + */ + public void testResolutionDoesNotMutateTheInterceptor() throws Exception { + ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); + container.inject(interceptor); + interceptor.setAllowedTypes("text/plain"); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes("text/html"); + container.inject(action); + + runUploadAttempt(interceptor, action, createUploadRequest("f.html", "text/html", htmlContent)); + + assertThat(interceptor.newLazyParams().getAllowedTypes()).containsExactly("text/plain"); + } + + /** + * Regression for WW-5659: a lazily resolved {@code disabled} must apply to one invocation only. + */ + public void testDisabledIsResolvedPerInvocation() throws Exception { + ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); + container.inject(interceptor); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes("text/plain"); + container.inject(action); + + UploadPolicy policy = interceptor.newLazyParams(); + policy.setDisabled("true"); + + assertThat(policy.isDisabled()).isTrue(); + assertThat(interceptor.newLazyParams().isDisabled()).isFalse(); + } +``` + +Add these helpers — none of them exist in the test class yet. `runUploadAttempt`, `createUploadRequest`, the `MockHttpServletRequest` overload of `createMultipartRequest` and `CoordinatedActionFileUploadInterceptor` all originate in PR #1815 by @deprrous and are reproduced here, adapted to the new contract, so this task is self-contained: + +```java + private String runUploadAttempt(ActionFileUploadInterceptor actionFileUploadInterceptor, + MyDynamicFileUploadAction action, + MockHttpServletRequest uploadRequest) throws Exception { + MultiPartRequestWrapper multiPartRequest = createMultipartRequest(uploadRequest, -1, -1, 3, -1); + ValueStack valueStack = container.getInstance(ValueStackFactory.class).createValueStack(); + valueStack.push(action); + + ActionContext context = ActionContext.of(valueStack.getContext()) + .withContainer(container) + .withValueStack(valueStack) + .withServletRequest(multiPartRequest) + .bind(); + try { + MockActionInvocation invocation = new MockActionInvocation(); + invocation.setAction(action); + invocation.setResultCode("success"); + invocation.setInvocationContext(context); + + Map params = new HashMap<>(); + params.put("allowedTypes", "${allowedMimeTypes}"); + + WithLazyParams.LazyParamInjector injector = new WithLazyParams.LazyParamInjector(valueStack); + container.inject(injector); + UploadPolicy policy = injector.resolveInto(actionFileUploadInterceptor.newLazyParams(), params, context); + + return actionFileUploadInterceptor.intercept(invocation, policy); + } finally { + ActionContext.clear(); + } + } + + private MockHttpServletRequest createUploadRequest(String filename, String contentType, String content) { + MockHttpServletRequest uploadRequest = new MockHttpServletRequest(); + uploadRequest.setCharacterEncoding(StandardCharsets.UTF_8.name()); + uploadRequest.setMethod("POST"); + uploadRequest.addHeader("Content-type", "multipart/form-data; boundary=\"" + boundary + "\""); + uploadRequest.setContent((encodeTextFile(filename, contentType, content) + endLine + "--" + boundary + "--") + .getBytes(StandardCharsets.UTF_8)); + return uploadRequest; + } + + private MultiPartRequestWrapper createMultipartRequest(MockHttpServletRequest multipartRequest, int maxsize, int maxfilesize, int maxfiles, int maxStringLength) { + JakartaMultiPartRequest jak = new JakartaMultiPartRequest(); + jak.setMaxSize(String.valueOf(maxsize)); + jak.setMaxFileSize(String.valueOf(maxfilesize)); + jak.setMaxFiles(String.valueOf(maxfiles)); + jak.setMaxStringLength(String.valueOf(maxStringLength)); + jak.setDefaultEncoding(StandardCharsets.UTF_8.name()); + return new MultiPartRequestWrapper(jak, multipartRequest, tempDir.getAbsolutePath(), new DefaultLocaleProvider()); + } + + /** Pauses the first validation so a second invocation can overlap it. From PR #1815 by @deprrous. */ + private static final class CoordinatedActionFileUploadInterceptor extends ActionFileUploadInterceptor { + private final AtomicBoolean pauseFirstValidation = new AtomicBoolean(true); + private final CountDownLatch firstValidationEntered = new CountDownLatch(1); + private final CountDownLatch allowFirstValidationToContinue = new CountDownLatch(1); + + @Override + protected boolean acceptFile(UploadPolicy policy, Object action, UploadedFile file, String originalFilename, String contentType, String inputName) { + if (pauseFirstValidation.compareAndSet(true, false)) { + firstValidationEntered.countDown(); + awaitUnchecked(allowFirstValidationToContinue); + } + return super.acceptFile(policy, action, file, originalFilename, contentType, inputName); + } + + private boolean awaitFirstValidation() throws InterruptedException { + return firstValidationEntered.await(10, TimeUnit.SECONDS); + } + + private void releaseFirstValidation() { + allowFirstValidationToContinue.countDown(); + } + + private void awaitUnchecked(CountDownLatch latch) { + try { + if (!latch.await(10, TimeUnit.SECONDS)) { + throw new AssertionError("Timed out waiting for concurrent validation release"); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new AssertionError("Interrupted while waiting for concurrent validation release", e); + } + } + } +``` + +To avoid duplicating the body, change the existing no-request `createMultipartRequest(int, int, int, int)` to delegate to the new overload: + +```java + private MultiPartRequestWrapper createMultipartRequest(int maxsize, int maxfilesize, int maxfiles, int maxStringLength) { + return createMultipartRequest(request, maxsize, maxfilesize, maxfiles, maxStringLength); + } +``` + +Required test imports: + +```java +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.ValueStackFactory; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest#testResolutionDoesNotMutateTheInterceptor` +Expected: compilation failure — `newLazyParams()` and the two-arg `intercept` do not exist. + +- [ ] **Step 3: Change the `WithLazyParams` interface** + +Replace the interface declaration in `WithLazyParams.java` (keep `LazyParamInjector` nested inside it unchanged apart from the deletion below): + +```java +public interface WithLazyParams

{ + + /** + * @return a fresh holder for one invocation, seeded from the configured values + * @since 7.3.0 + */ + P newLazyParams(); + + /** + * Invoked in place of {@link Interceptor#intercept(ActionInvocation)} when lazy params apply. + * + * @param lazyParams params resolved for this invocation only + * @since 7.3.0 + */ + String intercept(ActionInvocation invocation, P lazyParams) throws Exception; +``` + +Add the import `org.apache.struts2.ActionInvocation` and delete the old `injectParams` method entirely. + +- [ ] **Step 4: Implement the contract on the interceptor** + +In `ActionFileUploadInterceptor.java`, change the class declaration: + +```java +public class ActionFileUploadInterceptor extends AbstractFileUploadInterceptor implements WithLazyParams { +``` + +Add, above `intercept`: + +```java + @Override + public UploadPolicy newLazyParams() { + return copyConfiguredPolicy(); + } + + @Override + public String intercept(ActionInvocation invocation) throws Exception { + return intercept(invocation, newLazyParams()); + } +``` + +Change the existing `intercept` to the two-arg form, replacing the `UploadPolicy policy = copyConfiguredPolicy();` line added in Task 3: + +```java + @Override + public String intercept(ActionInvocation invocation, UploadPolicy policy) throws Exception { +``` + +- [ ] **Step 5: Wire `DefaultActionInvocation`** + +Replace `:258-276` with: + +```java + if (interceptors.hasNext()) { + final InterceptorMapping interceptorMapping = interceptors.next(); + Interceptor interceptor = interceptorMapping.getInterceptor(); + if (interceptor instanceof WithLazyParams lazyInterceptor) { + resultCode = invokeWithLazyParams(lazyInterceptor, interceptorMapping); + } else if (interceptor instanceof ConditionalInterceptor conditionalInterceptor) { + resultCode = executeConditional(conditionalInterceptor); + } else { + LOG.debug("Executing normal interceptor: {}", interceptorMapping.getName()); + resultCode = interceptor.intercept(this); + } + } else { + resultCode = invokeActionOnly(); + } +``` + +Add these two methods next to `executeConditional`: + +```java + /** + * Resolves lazy params into a per-invocation holder and dispatches to the interceptor. + *

+ * {@link org.apache.struts2.interceptor.AbstractInterceptor} implements + * {@link ConditionalInterceptor}, so a lazy interceptor is normally conditional too; both the + * lazily resolved {@code disabled} flag and any custom {@code shouldIntercept} must be honoured + * here, because the single-argument {@code intercept} is not the entry point on this path. + */ + private

String invokeWithLazyParams( + WithLazyParams

lazyInterceptor, InterceptorMapping interceptorMapping) throws Exception { + P lazyParams = lazyParamInjector.resolveInto( + lazyInterceptor.newLazyParams(), mergedParams(interceptorMapping), invocationContext); + + if (lazyParams instanceof org.apache.struts2.interceptor.DisableParams disableParams && disableParams.isDisabled()) { + LOG.debug("Interceptor: {} is disabled for this invocation, skipping to next", interceptorMapping.getName()); + return this.invoke(); + } + if (lazyInterceptor instanceof ConditionalInterceptor conditionalInterceptor + && !conditionalInterceptor.shouldIntercept(this)) { + LOG.debug("Interceptor: {} is disabled, skipping to next", interceptorMapping.getName()); + return this.invoke(); + } + LOG.debug("Executing lazy params interceptor: {}", interceptorMapping.getName()); + return lazyInterceptor.intercept(this, lazyParams); + } + + /** + * @return a fresh map; the mapping's own param map is shared across requests and must not be mutated + */ + private Map mergedParams(InterceptorMapping interceptorMapping) { + Map merged = new HashMap<>(interceptorMapping.getParams()); + proxy.getConfig().getInterceptors().stream() + .filter(im -> im.getName().equals(interceptorMapping.getName())) + .findFirst() + .ifPresent(im -> merged.putAll(im.getParams())); + return merged; + } +``` + +Add `import java.util.HashMap;` if not present. + +- [ ] **Step 6: Run the new tests** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest` +Expected: PASS, including the three new regressions. + +- [ ] **Step 7: Run the wider suite** + +Run: `mvn test -DskipAssembly -pl core` +Expected: PASS. `DefaultActionInvocationTest` exercises the rewritten `invoke()` branch and must stay green. + +- [ ] **Step 8: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java \ + core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java \ + core/src/main/java/org/apache/struts2/DefaultActionInvocation.java \ + core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +git commit -m "WW-5659 fix(core): resolve lazy interceptor params per invocation + +Co-Authored-By: deprrous " +``` + +--- + +### Task 5: Fail closed on unresolvable expressions + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java` +- Modify: `core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java` +- Modify: `core/src/main/resources/org/apache/struts2/struts-messages.properties` +- Test: `core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java` + +**Interfaces:** +- Consumes: `InterceptorParams.unresolved(String)` (Task 1), `resolveInto` (Task 2), `UploadPolicy` (Task 3). +- Produces: `UploadPolicy.isUnresolved()`, `UploadPolicy.getUnresolvedParams()`; constant `STRUTS_MESSAGES_ERROR_UPLOAD_POLICY_UNRESOLVED_KEY`. + +Implements the deterministic rule from the *Deviation from the spec* section: any param reported unresolved marks the policy unusable, regardless of the seeded value. + +- [ ] **Step 1: Write the failing test** + +Append to `ActionFileUploadInterceptorTest`: + +```java + public void testUnresolvedPolicyRejectsTheUpload() throws Exception { + ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); + container.inject(interceptor); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes(null); // ${allowedMimeTypes} will not resolve + container.inject(action); + + runUploadAttempt(interceptor, action, createUploadRequest("f.txt", "text/plain", plainContent)); + + assertThat(action.getUploadFiles()).isNull(); + assertThat(action.getFieldErrors()).containsKey("file"); + } + + public void testResolvedPolicyStillAcceptsTheUpload() throws Exception { + ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); + container.inject(interceptor); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes("text/plain"); + container.inject(action); + + runUploadAttempt(interceptor, action, createUploadRequest("f.txt", "text/plain", plainContent)); + + assertThat(action.hasFieldErrors()).isFalse(); + assertThat(action.getUploadFiles()).isNotNull().hasSize(1); + } + + public void testUploadPolicyTracksUnresolvedParams() { + UploadPolicy policy = new UploadPolicy(); + assertThat(policy.isUnresolved()).isFalse(); + + policy.unresolved("allowedTypes"); + + assertThat(policy.isUnresolved()).isTrue(); + assertThat(policy.getUnresolvedParams()).containsExactly("allowedTypes"); + } +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest#testUploadPolicyTracksUnresolvedParams` +Expected: compilation failure — `isUnresolved()` does not exist. + +- [ ] **Step 3: Track unresolved params on the policy** + +In `UploadPolicy.java` add the field, copy it, and override `unresolved`: + +```java + private final Set unresolvedParams = new LinkedHashSet<>(); +``` + +In the copy constructor add: + +```java + this.unresolvedParams.addAll(other.unresolvedParams); +``` + +and add: + +```java + /** + * A parameter that could not be resolved makes this policy unusable: the upload is rejected + * rather than validated against a partially-resolved policy, so a broken expression cannot + * silently relax validation. + */ + @Override + public void unresolved(String paramName) { + unresolvedParams.add(paramName); + } + + public boolean isUnresolved() { + return !unresolvedParams.isEmpty(); + } + + public Set getUnresolvedParams() { + return Collections.unmodifiableSet(unresolvedParams); + } +``` + +Add imports `java.util.LinkedHashSet`. + +- [ ] **Step 4: Reject in `acceptFile`** + +In `AbstractFileUploadInterceptor.java` add the key constant next to the others at `:47-60`: + +```java + public static final String STRUTS_MESSAGES_ERROR_UPLOAD_POLICY_UNRESOLVED_KEY = "struts.messages.error.upload.policy.unresolved"; +``` + +In `acceptFile`, immediately after the existing missing-file check that returns false at `:125-132`, add: + +```java + if (policy.isUnresolved()) { + String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_UPLOAD_POLICY_UNRESOLVED_KEY, new String[]{ + inputName, originalFilename, String.join(", ", policy.getUnresolvedParams()) + }); + if (validation != null) { + validation.addFieldError(inputName, errMsg); + } + LOG.warn(errMsg); + return false; + } +``` + +- [ ] **Step 5: Add the message** + +Append to `core/src/main/resources/org/apache/struts2/struts-messages.properties`: + +```properties +struts.messages.error.upload.policy.unresolved=The upload validation policy could not be resolved, rejecting the file: {0} "{1}"; unresolved parameters: {2} +``` + +Base bundle only — the locale bundles are partial and fall back to it. + +- [ ] **Step 6: Run tests to verify they pass** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest` +Expected: PASS. + +- [ ] **Step 7: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java \ + core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java \ + core/src/main/resources/org/apache/struts2/struts-messages.properties \ + core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +git commit -m "WW-5659 fix(core): reject uploads when the policy cannot be resolved" +``` + +--- + +### Task 6: Migrate legacy tests, javadoc, and spec alignment + +**Files:** +- Modify: `core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java` +- Modify: `core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java:70-204` (class javadoc) +- Modify: `docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md` + +**Interfaces:** +- Consumes: everything from Tasks 1–5. +- Produces: nothing new. + +- [ ] **Step 1: Migrate the setter-shortcut tests** + +Five tests simulate injection by calling a setter directly — `testDynamicParameterEvaluation`, `testDynamicParametersChangePerRequest`, `testDynamicExtensionValidation`, `testDynamicMaximumSizeValidation`, `testSecurityValidationWithDynamicParameters`, `testWildcardMatchingWithDynamicParameters`. Each contains a line of the form `interceptor.setAllowedTypes(action.getAllowedMimeTypes());` followed by `interceptor.intercept(mai);`. + +Replace each such pair with a real resolution through the injector, e.g.: + +```java + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, false, false); + interceptor.intercept(mai, policy); +``` + +and add the helper: + +```java + private UploadPolicy injectDynamicUploadPolicy(ActionFileUploadInterceptor actionFileUploadInterceptor, + ActionContext context, + boolean includeAllowedTypes, + boolean includeAllowedExtensions, + boolean includeMaximumSize) { + Map params = new HashMap<>(); + if (includeAllowedTypes) { + params.put("allowedTypes", "${allowedMimeTypes}"); + } + if (includeAllowedExtensions) { + params.put("allowedExtensions", "${allowedExtensions}"); + } + if (includeMaximumSize) { + params.put("maximumSize", "${maxFileSize}"); + } + + WithLazyParams.LazyParamInjector injector = new WithLazyParams.LazyParamInjector(context.getValueStack()); + container.inject(injector); + return injector.resolveInto(actionFileUploadInterceptor.newLazyParams(), params, context); + } +``` + +Set the flags per test to match the param that test previously set by hand: `allowedTypes` for the four type/wildcard tests, `allowedExtensions` for `testDynamicExtensionValidation`, `maximumSize` for `testDynamicMaximumSizeValidation`, and both types and extensions for `testSecurityValidationWithDynamicParameters`. + +- [ ] **Step 2: Run the suite** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest` +Expected: PASS. These tests now exercise the real resolution path rather than a hand-called setter. + +- [ ] **Step 3: Update the interceptor javadoc** + +`ActionFileUploadInterceptor`'s class javadoc at `:70-204` documents the dynamic-parameter feature with a `uploadConfig.setAllowedExtensions(".jpg,.png")` example. Add a paragraph directly above `@see WithLazyParams`: + +```java + *

+ * Dynamic parameters are resolved into a fresh {@link UploadPolicy} for each invocation, so the + * interceptor itself is never modified per request and concurrent uploads cannot observe each + * other's policy. An expression that cannot be resolved does not relax validation: the policy is + * marked unresolved and affected uploads are rejected. +``` + +- [ ] **Step 4: Align the spec with the implemented rule** + +In the spec's *Error handling* section, replace the two-case "genuine static fallback / no fallback" rule and the sentence beginning "The rule: `unresolved(param)` marks the dimension unusable **only if**..." with: + +```markdown +`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`. + +The seed-introspection alternative — honouring a seeded value that does not itself contain +`${` — was rejected during planning: it is not implementable deterministically, because +`maximumSize` is seeded as a `Long` and cannot carry a `${...}` literal, so the rule would behave +differently per parameter type. +``` + +Update the corresponding Testing bullet to drop the static-fallback case. + +- [ ] **Step 5: Full build** + +Run: `mvn test -DskipAssembly` +Expected: PASS across all modules. Plugins do not implement `WithLazyParams` (verified: `ActionFileUploadInterceptor` is the only implementer), but the interface change is compile-visible, so a full build is the check. + +- [ ] **Step 6: Commit** + +```bash +git add core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java \ + core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java \ + docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md +git commit -m "WW-5659 test(core): exercise real lazy param resolution in dynamic upload tests" +``` + +--- + +## Done criteria + +- `mvn test -DskipAssembly` passes across all modules. +- `ActionFileUploadInterceptor` has no mutable state written after `init()`; `testResolutionDoesNotMutateTheInterceptor` proves it. +- No `ThreadLocal`, no `finally` cleanup, and no per-request write to any shared map remains on the lazy path. +- The spec and the implementation agree on the unresolved-param rule. From 34ffa9a67471b39fb28754c57b625a17066f49b6 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 10:16:47 +0200 Subject: [PATCH 03/23] WW-5659 docs: clarify test base class constraint in the plan Co-Authored-By: Claude Opus 5 --- .../plans/2026-07-27-WW-5659-lazy-params-request-scoping.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/superpowers/plans/2026-07-27-WW-5659-lazy-params-request-scoping.md b/docs/superpowers/plans/2026-07-27-WW-5659-lazy-params-request-scoping.md index f1af340e83..8391167aee 100644 --- a/docs/superpowers/plans/2026-07-27-WW-5659-lazy-params-request-scoping.md +++ b/docs/superpowers/plans/2026-07-27-WW-5659-lazy-params-request-scoping.md @@ -14,7 +14,7 @@ - **Ticket prefix:** every commit message starts with `WW-5659`. - **Target version:** 7.3.0. This is a deliberate public API break; no backport to 7.2.x. -- **Test style:** `core` tests are JUnit 3 style — extend `StrutsInternalTestCase` (which extends `XWorkTestCase`, which extends `junit.framework.TestCase`), use `protected void setUp()`, and name tests `public void testXxx()`. **A JUnit 5 `@Test` annotation compiles and silently never runs.** Never add one. +- **Test style:** `core` tests are JUnit 3 style — `protected void setUp()`, tests named `public void testXxx()`, no annotations. **A JUnit 5 `@Test` annotation compiles and silently never runs.** Never add one. Extend `StrutsInternalTestCase` (which extends `XWorkTestCase`, which extends `junit.framework.TestCase`) when the test needs the Struts container or `ActionContext`; extend `junit.framework.TestCase` directly for pure value-object tests that need neither. - **Assertions:** use AssertJ (`import static org.assertj.core.api.Assertions.assertThat;`), matching the existing `ActionFileUploadInterceptorTest`. - **Package:** all new production types go in `org.apache.struts2.interceptor`. - **License header:** every new `.java` file starts with the ASF header, copied verbatim from `core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java:1-18`. From c35483c65eab332e0635f508399804732bf36d98 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 10:18:42 +0200 Subject: [PATCH 04/23] WW-5659 feat(core): add InterceptorParams contract and DisableParams holder Co-Authored-By: Claude Opus 5 --- .../struts2/interceptor/DisableParams.java | 51 ++++++++++++++++ .../interceptor/InterceptorParams.java | 43 +++++++++++++ .../interceptor/DisableParamsTest.java | 60 +++++++++++++++++++ 3 files changed, 154 insertions(+) create mode 100644 core/src/main/java/org/apache/struts2/interceptor/DisableParams.java create mode 100644 core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java create mode 100644 core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java diff --git a/core/src/main/java/org/apache/struts2/interceptor/DisableParams.java b/core/src/main/java/org/apache/struts2/interceptor/DisableParams.java new file mode 100644 index 0000000000..aa34467879 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/DisableParams.java @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.interceptor; + +/** + * Opt-in support for the {@code disabled} interceptor parameter. + *

+ * Interceptors implementing {@link WithLazyParams} must have their params holder extend this + * class to support {@code ...}; there is deliberately no + * fallback to the interceptor instance, which would reintroduce shared mutable state. + * + * @since 7.3.0 + */ +public class DisableParams implements InterceptorParams { + + private boolean disabled; + + public DisableParams() { + } + + protected DisableParams(DisableParams other) { + this.disabled = other.disabled; + } + + /** + * @param disable if {@code true}, execution of the interceptor is skipped for this invocation + */ + public void setDisabled(String disable) { + this.disabled = Boolean.parseBoolean(disable); + } + + public boolean isDisabled() { + return disabled; + } +} diff --git a/core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java b/core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java new file mode 100644 index 0000000000..68cf3e2bae --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java @@ -0,0 +1,43 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.interceptor; + +/** + * Contract for an object holding the parameters of a single interceptor. + *

+ * Implementations are per-invocation value objects: the framework resolves configured + * parameters into a fresh instance for each action invocation, so nothing is written back + * onto the interceptor, which stays immutable after {@link Interceptor#init()}. + * + * @since 7.3.0 + */ +public interface InterceptorParams { + + /** + * Called when a {@code ${...}} parameter could not be resolved for the current invocation. + * The framework skips the write, leaving the seeded configuration value in place, and + * notifies the holder so it can decide how to degrade. + *

+ * The default implementation does nothing. + * + * @param paramName name of the parameter that could not be resolved + */ + default void unresolved(String paramName) { + } +} diff --git a/core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java b/core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java new file mode 100644 index 0000000000..3a41b0b843 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/DisableParamsTest.java @@ -0,0 +1,60 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.interceptor; + +import junit.framework.TestCase; + +import static org.assertj.core.api.Assertions.assertThat; + +public class DisableParamsTest extends TestCase { + + public void testDisabledDefaultsToFalse() { + assertThat(new DisableParams().isDisabled()).isFalse(); + } + + public void testSetDisabledParsesStringValue() { + DisableParams params = new DisableParams(); + params.setDisabled("true"); + assertThat(params.isDisabled()).isTrue(); + + params.setDisabled("false"); + assertThat(params.isDisabled()).isFalse(); + } + + public void testSetDisabledTreatsNonBooleanTextAsFalse() { + DisableParams params = new DisableParams(); + params.setDisabled("yes"); + assertThat(params.isDisabled()).isFalse(); + } + + public void testCopyConstructorCarriesDisabledFlag() { + DisableParams original = new DisableParams(); + original.setDisabled("true"); + + DisableParams copy = new DisableParams(original); + + assertThat(copy.isDisabled()).isTrue(); + } + + public void testUnresolvedDefaultsToNoOp() { + DisableParams params = new DisableParams(); + params.unresolved("someParam"); // must not throw + assertThat(params.isDisabled()).isFalse(); + } +} From 687963abae8c79e708f5a62b06ca70c32649839c Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 10:23:20 +0200 Subject: [PATCH 05/23] WW-5659 feat(core): resolve lazy params into a holder instead of the interceptor --- .../struts2/interceptor/WithLazyParams.java | 49 +++++++ .../interceptor/LazyParamInjectorTest.java | 134 ++++++++++++++++++ 2 files changed, 183 insertions(+) create mode 100644 core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java 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 cdfb3a8adb..8c74b398df 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java +++ b/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java @@ -18,12 +18,15 @@ */ package org.apache.struts2.interceptor; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.apache.struts2.ActionContext; import org.apache.struts2.inject.Inject; import org.apache.struts2.ognl.OgnlUtil; import org.apache.struts2.util.TextParseUtil; import org.apache.struts2.util.TextParser; import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.reflection.ReflectionException; import org.apache.struts2.util.reflection.ReflectionProvider; import java.util.Map; @@ -49,6 +52,8 @@ public interface WithLazyParams { class LazyParamInjector { + private static final Logger LOG = LogManager.getLogger(LazyParamInjector.class); + protected OgnlUtil ognlUtil; protected TextParser textParser; protected ReflectionProvider reflectionProvider; @@ -82,5 +87,49 @@ public Interceptor injectParams(Interceptor interceptor, Map par } return interceptor; } + + /** + * Resolves configured params into a per-invocation holder, leaving the interceptor untouched. + *

+ * A {@code ${...}} expression that cannot be resolved is not written: the holder keeps its + * seeded configuration value and is notified via {@link InterceptorParams#unresolved(String)}, + * so a broken expression cannot silently relax a validation policy. + * + * @since 7.3.0 + */ + public

P resolveInto(P target, Map params, ActionContext invocationContext) { + for (Map.Entry entry : params.entrySet()) { + 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 [{}]; check the interceptor configuration", + paramName, target.getClass().getName(), e); + } + } + return target; + } + + /** + * {@link org.apache.struts2.util.OgnlTextParser} yields an empty string for an expression that + * does not resolve and gives no other signal, so the raw template is needed to tell that apart + * from a legitimately empty value. + */ + private boolean isUnresolved(String rawValue, Object paramValue) { + return rawValue != null + && rawValue.contains("${") + && (paramValue == null || paramValue.toString().isEmpty()); + } } } diff --git a/core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java b/core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java new file mode 100644 index 0000000000..8939c13134 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java @@ -0,0 +1,134 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.interceptor; + +import org.apache.struts2.ActionContext; +import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.ValueStackFactory; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; + +public class LazyParamInjectorTest extends StrutsInternalTestCase { + + public static class Holder extends DisableParams { + private String name; + private Long size; + private final List unresolvedCalls = new ArrayList<>(); + + public void setName(String name) { this.name = name; } + public void setSize(Long size) { this.size = size; } + public String getName() { return name; } + public Long getSize() { return size; } + public List getUnresolvedCalls() { return unresolvedCalls; } + + @Override + public void unresolved(String paramName) { unresolvedCalls.add(paramName); } + } + + public static class Bean { + public String getLabel() { return "resolved-label"; } + public Long getLimit() { return 4096L; } + } + + private ActionContext context; + private WithLazyParams.LazyParamInjector injector; + + @Override + protected void setUp() throws Exception { + super.setUp(); + ValueStack stack = container.getInstance(ValueStackFactory.class).createValueStack(); + stack.push(new Bean()); + context = ActionContext.of(stack.getContext()).withContainer(container).withValueStack(stack).bind(); + injector = new WithLazyParams.LazyParamInjector(stack); + container.inject(injector); + } + + @Override + protected void tearDown() throws Exception { + ActionContext.clear(); + super.tearDown(); + } + + public void testResolvesExpressionsIntoTheHolder() { + Map params = new HashMap<>(); + params.put("name", "${label}"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getName()).isEqualTo("resolved-label"); + assertThat(holder.getUnresolvedCalls()).isEmpty(); + } + + public void testAppliesOgnlTypeConversion() { + Map params = new HashMap<>(); + params.put("size", "${limit}"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getSize()).isEqualTo(4096L); + } + + public void testPassesLiteralValuesThrough() { + Map params = new HashMap<>(); + params.put("name", "plain-text"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getName()).isEqualTo("plain-text"); + assertThat(holder.getUnresolvedCalls()).isEmpty(); + } + + public void testUnresolvableExpressionSkipsWriteAndNotifiesHolder() { + Holder seeded = new Holder(); + seeded.setName("seeded-value"); + + Map params = new HashMap<>(); + params.put("name", "${noSuchProperty}"); + + Holder holder = injector.resolveInto(seeded, params, context); + + assertThat(holder.getName()).isEqualTo("seeded-value"); + assertThat(holder.getUnresolvedCalls()).containsExactly("name"); + } + + public void testResolvesDisabledOntoDisableParams() { + Map params = new HashMap<>(); + params.put("disabled", "true"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.isDisabled()).isTrue(); + } + + public void testUnknownParamIsIgnoredWithoutFailingTheInvocation() { + Map params = new HashMap<>(); + params.put("noSuchParam", "whatever"); + + Holder holder = injector.resolveInto(new Holder(), params, context); + + assertThat(holder.getName()).isNull(); + assertThat(holder.getSize()).isNull(); + } +} From 31dc7f667aece368c0f9f8c03a81de8b3fcaa22b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 10:33:34 +0200 Subject: [PATCH 06/23] WW-5659 docs(core): correct isUnresolved javadoc and pin empty-value fail-closed behavior isUnresolved cannot distinguish a failed ${...} resolution from an expression that legitimately evaluates to an empty string; the parser gives no other signal. The previous javadoc wrongly claimed the raw template let it tell the two apart. Fix the javadoc to state the actual, intentional rule (fail-closed: treat both as unusable), and add a test pinning that a legitimately-empty expression is treated as unresolved rather than written. --- .../struts2/interceptor/WithLazyParams.java | 21 +++++++++++++------ .../interceptor/LazyParamInjectorTest.java | 14 +++++++++++++ 2 files changed, 29 insertions(+), 6 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 8c74b398df..f683ebb7c0 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java +++ b/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java @@ -91,9 +91,13 @@ public Interceptor injectParams(Interceptor interceptor, Map par /** * Resolves configured params into a per-invocation holder, leaving the interceptor untouched. *

- * A {@code ${...}} expression that cannot be resolved is not written: the holder keeps its - * seeded configuration value and is notified via {@link InterceptorParams#unresolved(String)}, - * so a broken expression cannot silently relax a validation policy. + * 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. * * @since 7.3.0 */ @@ -122,9 +126,14 @@ public

P resolveInto(P target, Map } /** - * {@link org.apache.struts2.util.OgnlTextParser} yields an empty string for an expression that - * does not resolve and gives no other signal, so the raw template is needed to tell that apart - * from a legitimately empty value. + * A {@code ${...}} param is treated as unresolved when its evaluated value is null or empty. + *

+ * {@link org.apache.struts2.util.OgnlTextParser} yields the same empty string both when an + * expression fails to resolve and when it resolves to a legitimately empty value — there is + * no way to tell the two apart from the parser's output alone. This method does not attempt + * 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. */ 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 8939c13134..46f5282f0f 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/LazyParamInjectorTest.java @@ -50,6 +50,7 @@ public static class Holder extends DisableParams { public static class Bean { public String getLabel() { return "resolved-label"; } public Long getLimit() { return 4096L; } + public String getBlank() { return ""; } } private ActionContext context; @@ -113,6 +114,19 @@ public void testUnresolvableExpressionSkipsWriteAndNotifiesHolder() { assertThat(holder.getUnresolvedCalls()).containsExactly("name"); } + public void testExpressionResolvingToEmptyIsTreatedAsUnresolved() { + Holder seeded = new Holder(); + seeded.setName("seeded-value"); + + Map params = new HashMap<>(); + params.put("name", "${blank}"); + + Holder holder = injector.resolveInto(seeded, params, context); + + assertThat(holder.getName()).isEqualTo("seeded-value"); + assertThat(holder.getUnresolvedCalls()).containsExactly("name"); + } + public void testResolvesDisabledOntoDisableParams() { Map params = new HashMap<>(); params.put("disabled", "true"); From af6e5fb81809dedacf29f7f05c9a8690c779627c Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 10:42:20 +0200 Subject: [PATCH 07/23] WW-5659 refactor(core): hold upload policy in one value object Introduce UploadPolicy (extends DisableParams) to consolidate the three loose maximumSize/allowedTypes/allowedExtensions fields on AbstractFileUploadInterceptor into a single config-time value object. acceptFile now takes the effective policy as an explicit parameter instead of reading interceptor-level state directly. Pure refactor, no behaviour change: the existing setters still mutate the shared singleton via configuredPolicy, and ActionFileUploadInterceptor copies it once per invocation via copyConfiguredPolicy() before calling acceptFile. This groundwork lets a later change route lazily-resolved per-request params into the copy instead of the singleton. --- .../AbstractFileUploadInterceptor.java | 43 +++++---- .../ActionFileUploadInterceptor.java | 4 +- .../struts2/interceptor/UploadPolicy.java | 95 +++++++++++++++++++ .../ActionFileUploadInterceptorTest.java | 61 +++++++++--- 4 files changed, 172 insertions(+), 31 deletions(-) create mode 100644 core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java diff --git a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java index c7cbb65c8d..f8529732bc 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java @@ -24,7 +24,6 @@ import org.apache.struts2.text.TextProviderFactory; import org.apache.struts2.inject.Container; import org.apache.struts2.inject.Inject; -import org.apache.struts2.util.TextParseUtil; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.dispatcher.LocalizedMessage; @@ -35,7 +34,6 @@ import java.text.NumberFormat; import java.util.Arrays; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Set; @@ -59,9 +57,7 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor public static final String STRUTS_MESSAGES_ERROR_CONTENT_TYPE_NOT_ALLOWED_KEY = "struts.messages.error.content.type.not.allowed"; public static final String STRUTS_MESSAGES_ERROR_FILE_EXTENSION_NOT_ALLOWED_KEY = "struts.messages.error.file.extension.not.allowed"; - private Long maximumSize; - private Set allowedTypesSet = Collections.emptySet(); - private Set allowedExtensionsSet = Collections.emptySet(); + private final UploadPolicy configuredPolicy = new UploadPolicy(); private ContentTypeMatcher matcher; private Container container; @@ -77,35 +73,48 @@ public void setContainer(Container container) { } /** - * Sets the allowed extensions + * Sets the allowed extensions. Applied at configuration time only; the effective policy for + * an invocation is a copy, see {@link ActionFileUploadInterceptor#newLazyParams()}. * * @param allowedExtensions A comma-delimited list of extensions */ public void setAllowedExtensions(String allowedExtensions) { - allowedExtensionsSet = TextParseUtil.commaDelimitedStringToSet(allowedExtensions); + configuredPolicy.setAllowedExtensions(allowedExtensions); } /** - * Sets the allowed mimetypes + * Sets the allowed mimetypes. Applied at configuration time only; the effective policy for + * an invocation is a copy, see {@link ActionFileUploadInterceptor#newLazyParams()}. * * @param allowedTypes A comma-delimited list of types */ public void setAllowedTypes(String allowedTypes) { - allowedTypesSet = TextParseUtil.commaDelimitedStringToSet(allowedTypes); + configuredPolicy.setAllowedTypes(allowedTypes); } /** - * Sets the maximum size of an uploaded file + * Sets the maximum size of an uploaded file. Applied at configuration time only; the + * effective policy for an invocation is a copy, see + * {@link ActionFileUploadInterceptor#newLazyParams()}. * * @param maximumSize The maximum size in bytes */ public void setMaximumSize(Long maximumSize) { - this.maximumSize = maximumSize; + configuredPolicy.setMaximumSize(maximumSize); + } + + /** + * @return an independent copy of the configured policy, to be resolved for one invocation + * @since 7.3.0 + */ + protected UploadPolicy copyConfiguredPolicy() { + return configuredPolicy.copy(); } /** * Override for added functionality. Checks if the proposed file is acceptable based on contentType and size. * + * @param policy - the effective upload policy for this invocation. * @param action - uploading action for message retrieval. * @param file - proposed upload file. * @param originalFilename - name of the file. @@ -113,7 +122,7 @@ public void setMaximumSize(Long maximumSize) { * @param inputName - inputName of the file. * @return true if the proposed file is acceptable by contentType and size. */ - protected boolean acceptFile(Object action, UploadedFile file, String originalFilename, String contentType, String inputName) { + protected boolean acceptFile(UploadPolicy policy, Object action, UploadedFile file, String originalFilename, String contentType, String inputName) { Set errorMessages = new HashSet<>(); ValidationAware validation = null; @@ -131,21 +140,21 @@ protected boolean acceptFile(Object action, UploadedFile file, String originalFi return false; } - if (maximumSize != null && maximumSize < file.length()) { + if (policy.getMaximumSize() != null && policy.getMaximumSize() < file.length()) { String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_FILE_TOO_LARGE_KEY, new String[]{ - inputName, originalFilename, file.getName(), "" + file.length(), getMaximumSizeStr(action) + inputName, originalFilename, file.getName(), "" + file.length(), getMaximumSizeStr(action, policy.getMaximumSize()) }); errorMessages.add(errMsg); LOG.warn(errMsg); } - if ((!allowedTypesSet.isEmpty()) && (!containsItem(allowedTypesSet, contentType))) { + if ((!policy.getAllowedTypes().isEmpty()) && (!containsItem(policy.getAllowedTypes(), contentType))) { String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_CONTENT_TYPE_NOT_ALLOWED_KEY, new String[]{ inputName, originalFilename, file.getName(), contentType }); errorMessages.add(errMsg); LOG.warn(errMsg); } - if ((!allowedExtensionsSet.isEmpty()) && (!hasAllowedExtension(allowedExtensionsSet, originalFilename))) { + if ((!policy.getAllowedExtensions().isEmpty()) && (!hasAllowedExtension(policy.getAllowedExtensions(), originalFilename))) { String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_FILE_EXTENSION_NOT_ALLOWED_KEY, new String[]{ inputName, originalFilename, file.getName(), contentType }); @@ -161,7 +170,7 @@ protected boolean acceptFile(Object action, UploadedFile file, String originalFi return errorMessages.isEmpty(); } - private String getMaximumSizeStr(Object action) { + private String getMaximumSizeStr(Object action, Long maximumSize) { return NumberFormat.getNumberInstance(getLocaleProvider(action).getLocale()).format(maximumSize); } diff --git a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java index 79d020a345..4aea0f7828 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java @@ -228,6 +228,8 @@ public String intercept(ActionInvocation invocation) throws Exception { return invocation.invoke(); } + UploadPolicy policy = copyConfiguredPolicy(); + applyValidation(action, multiWrapper); // bind allowed Files @@ -245,7 +247,7 @@ public String intercept(ActionInvocation invocation) throws Exception { } } else { for (UploadedFile uploadedFile : uploadedFiles) { - if (acceptFile(action, uploadedFile, uploadedFile.getOriginalName(), uploadedFile.getContentType(), inputName)) { + if (acceptFile(policy, action, uploadedFile, uploadedFile.getOriginalName(), uploadedFile.getContentType(), inputName)) { acceptedFiles.add(uploadedFile); } } diff --git a/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java new file mode 100644 index 0000000000..a9e4441576 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.interceptor; + +import org.apache.struts2.util.TextParseUtil; + +import java.util.Collections; +import java.util.Set; + +/** + * Per-invocation upload validation policy for {@link ActionFileUploadInterceptor}. + *

+ * A configured instance is held by the interceptor and copied for each invocation, so lazily + * resolved values never reach the shared interceptor. + * + * @since 7.3.0 + */ +public class UploadPolicy extends DisableParams { + + private Long maximumSize; + private Set allowedTypes = Collections.emptySet(); + private Set allowedExtensions = Collections.emptySet(); + + public UploadPolicy() { + } + + private UploadPolicy(UploadPolicy other) { + super(other); + this.maximumSize = other.maximumSize; + this.allowedTypes = other.allowedTypes; + this.allowedExtensions = other.allowedExtensions; + } + + /** + * @param allowedTypes a comma-delimited list of content types, or null for no restriction + */ + public void setAllowedTypes(String allowedTypes) { + this.allowedTypes = toSet(allowedTypes); + } + + /** + * @param allowedExtensions a comma-delimited list of extensions, or null for no restriction + */ + public void setAllowedExtensions(String allowedExtensions) { + this.allowedExtensions = toSet(allowedExtensions); + } + + /** + * @param maximumSize the maximum size in bytes, or null for no limit + */ + public void setMaximumSize(Long maximumSize) { + this.maximumSize = maximumSize; + } + + public Long getMaximumSize() { + return maximumSize; + } + + public Set getAllowedTypes() { + return allowedTypes; + } + + public Set getAllowedExtensions() { + return allowedExtensions; + } + + /** + * @return an independent copy, used to seed a per-invocation policy from the configured one + */ + public UploadPolicy copy() { + return new UploadPolicy(this); + } + + private static Set toSet(String commaDelimited) { + return commaDelimited == null + ? Collections.emptySet() + : TextParseUtil.commaDelimitedStringToSet(commaDelimited); + } +} diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java index 058dabe115..c45091d770 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -63,7 +63,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { public void testAcceptFileWithEmptyAllowedTypesAndExtensions() { // when allowed type is empty ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -75,7 +75,7 @@ public void testAcceptFileWithoutEmptyTypes() { // when file is of allowed types ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -83,7 +83,7 @@ public void testAcceptFileWithoutEmptyTypes() { // when file is not of allowed types validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/html", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/html", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -94,7 +94,7 @@ public void testAcceptFileWithWildcardContent() { interceptor.setAllowedTypes("text/*"); ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -102,7 +102,7 @@ public void testAcceptFileWithWildcardContent() { interceptor.setAllowedTypes("text/h*"); validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/plain", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/plain", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -114,7 +114,7 @@ public void testAcceptFileWithoutEmptyExtensions() { // when file is of allowed extensions ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -122,7 +122,7 @@ public void testAcceptFileWithoutEmptyExtensions() { // when file is not of allowed extensions validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/html", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/html", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -130,7 +130,7 @@ public void testAcceptFileWithoutEmptyExtensions() { interceptor.setAllowedExtensions(".txt,.lol"); validation = new ValidationAwareSupport(); - ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.lol", "text/plain", "inputName"); + ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.lol", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -142,7 +142,7 @@ public void testAcceptFileWithNoFile() { // when file is not of allowed types ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, null, "filename.html", "text/html", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, null, "filename.html", "text/html", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -159,7 +159,7 @@ public void testAcceptFileWithNoContent() { interceptor.setAllowedTypes("text/plain"); ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, createTestFile(null), "filename.html", "text/plain", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(null), "filename.html", "text/plain", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -183,7 +183,7 @@ public void testAcceptFileDoesNotMaterializeInMemoryUpload() { .withInputName("inputName") .build(); - boolean ok = interceptor.acceptFile(validation, file, "f.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, file, "f.txt", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.hasErrors()).isFalse(); @@ -203,7 +203,7 @@ public void testRejectedInMemoryUploadIsStillNotMaterialized() { .build(); // wrong content type -> rejected - boolean ok = interceptor.acceptFile(validation, file, "f.html", "text/html", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, file, "f.html", "text/html", "inputName"); assertThat(ok).isFalse(); assertThat(validation.hasErrors()).isTrue(); @@ -221,7 +221,7 @@ public void testAcceptFileWithMaxSize() throws Exception { File file = new File(new URI(url.toString())); assertThat(file).exists(); UploadedFile uploadedFile = StrutsUploadedFile.Builder.create(file).withContentType("text/html").withOriginalName("filename").build(); - boolean notOk = interceptor.acceptFile(validation, uploadedFile, "filename", "text/html", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, uploadedFile, "filename", "text/html", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -920,4 +920,39 @@ public void setMaxFileSize(Long maxFileSize) { } } + public void testUploadPolicyParsesAndCopies() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes("text/plain, text/html"); + policy.setAllowedExtensions(".txt,.html"); + policy.setMaximumSize(1024L); + policy.setDisabled("true"); + + UploadPolicy copy = policy.copy(); + + assertThat(copy.getAllowedTypes()).containsExactlyInAnyOrder("text/plain", "text/html"); + assertThat(copy.getAllowedExtensions()).containsExactlyInAnyOrder(".txt", ".html"); + assertThat(copy.getMaximumSize()).isEqualTo(1024L); + assertThat(copy.isDisabled()).isTrue(); + } + + public void testUploadPolicyCopyIsIndependentOfTheOriginal() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes("text/plain"); + + UploadPolicy copy = policy.copy(); + copy.setAllowedTypes("text/html"); + + assertThat(policy.getAllowedTypes()).containsExactly("text/plain"); + assertThat(copy.getAllowedTypes()).containsExactly("text/html"); + } + + public void testUploadPolicyTreatsNullAsNoRestriction() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes(null); + policy.setAllowedExtensions(null); + + assertThat(policy.getAllowedTypes()).isEmpty(); + assertThat(policy.getAllowedExtensions()).isEmpty(); + } + } From 84aca8cd5e76e137cb125bebb07f8dc7b0575c9b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 10:56:03 +0200 Subject: [PATCH 08/23] WW-5659 fix(core): resolve lazy interceptor params per invocation Co-Authored-By: deprrous --- .../struts2/DefaultActionInvocation.java | 53 +++-- .../ActionFileUploadInterceptor.java | 14 +- .../struts2/interceptor/WithLazyParams.java | 25 ++- .../ActionFileUploadInterceptorTest.java | 186 +++++++++++++++++- .../struts2/mock/MockLazyInterceptor.java | 48 ++++- 5 files changed, 291 insertions(+), 35 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java b/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java index 6885dd50d1..913590410c 100644 --- a/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java +++ b/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java @@ -40,6 +40,7 @@ import org.apache.struts2.util.ValueStackFactory; import java.util.ArrayList; +import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -258,17 +259,9 @@ public String invoke() throws Exception { if (interceptors.hasNext()) { final InterceptorMapping interceptorMapping = interceptors.next(); Interceptor interceptor = interceptorMapping.getInterceptor(); - if (interceptor instanceof WithLazyParams) { - Map params = interceptorMapping.getParams(); - - proxy.getConfig().getInterceptors().stream() - .filter(im -> im.getName().equals(interceptorMapping.getName())) - .findFirst() - .ifPresent(im -> params.putAll(im.getParams())); - - interceptor = lazyParamInjector.injectParams(interceptor, params, invocationContext); - } - if (interceptor instanceof ConditionalInterceptor conditionalInterceptor) { + if (interceptor instanceof WithLazyParams lazyInterceptor) { + resultCode = invokeWithLazyParams(lazyInterceptor, interceptorMapping); + } else if (interceptor instanceof ConditionalInterceptor conditionalInterceptor) { resultCode = executeConditional(conditionalInterceptor); } else { LOG.debug("Executing normal interceptor: {}", interceptorMapping.getName()); @@ -312,6 +305,44 @@ public String invoke() throws Exception { return resultCode; } + /** + * Resolves lazy params into a per-invocation holder and dispatches to the interceptor. + *

+ * {@link org.apache.struts2.interceptor.AbstractInterceptor} implements + * {@link ConditionalInterceptor}, so a lazy interceptor is normally conditional too; both the + * lazily resolved {@code disabled} flag and any custom {@code shouldIntercept} must be honoured + * here, because the single-argument {@code intercept} is not the entry point on this path. + */ + private

String invokeWithLazyParams( + WithLazyParams

lazyInterceptor, InterceptorMapping interceptorMapping) throws Exception { + P lazyParams = lazyParamInjector.resolveInto( + lazyInterceptor.newLazyParams(), mergedParams(interceptorMapping), invocationContext); + + if (lazyParams instanceof org.apache.struts2.interceptor.DisableParams disableParams && disableParams.isDisabled()) { + LOG.debug("Interceptor: {} is disabled for this invocation, skipping to next", interceptorMapping.getName()); + return this.invoke(); + } + if (lazyInterceptor instanceof ConditionalInterceptor conditionalInterceptor + && !conditionalInterceptor.shouldIntercept(this)) { + LOG.debug("Interceptor: {} is disabled, skipping to next", interceptorMapping.getName()); + return this.invoke(); + } + LOG.debug("Executing lazy params interceptor: {}", interceptorMapping.getName()); + return lazyInterceptor.intercept(this, lazyParams); + } + + /** + * @return a fresh map; the mapping's own param map is shared across requests and must not be mutated + */ + private Map mergedParams(InterceptorMapping interceptorMapping) { + Map merged = new HashMap<>(interceptorMapping.getParams()); + proxy.getConfig().getInterceptors().stream() + .filter(im -> im.getName().equals(interceptorMapping.getName())) + .findFirst() + .ifPresent(im -> merged.putAll(im.getParams())); + return merged; + } + protected String executeConditional(ConditionalInterceptor conditionalInterceptor) throws Exception { if (conditionalInterceptor.shouldIntercept(this)) { LOG.debug("Executing conditional interceptor: {}", conditionalInterceptor.getClass().getSimpleName()); diff --git a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java index 4aea0f7828..82ff798e3f 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java @@ -202,12 +202,22 @@ * @see UploadedFilesAware * @see AbstractFileUploadInterceptor */ -public class ActionFileUploadInterceptor extends AbstractFileUploadInterceptor implements WithLazyParams { +public class ActionFileUploadInterceptor extends AbstractFileUploadInterceptor implements WithLazyParams { protected static final Logger LOG = LogManager.getLogger(ActionFileUploadInterceptor.class); + @Override + public UploadPolicy newLazyParams() { + return copyConfiguredPolicy(); + } + @Override public String intercept(ActionInvocation invocation) throws Exception { + return intercept(invocation, newLazyParams()); + } + + @Override + public String intercept(ActionInvocation invocation, UploadPolicy policy) throws Exception { HttpServletRequest request = invocation.getInvocationContext().getServletRequest(); MultiPartRequestWrapper multiWrapper = request instanceof HttpServletRequestWrapper wrapper ? findMultipartRequestWrapper(wrapper) @@ -228,8 +238,6 @@ public String intercept(ActionInvocation invocation) throws Exception { return invocation.invoke(); } - UploadPolicy policy = copyConfiguredPolicy(); - applyValidation(action, multiWrapper); // bind allowed Files 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 f683ebb7c0..85d8e6d71a 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java +++ b/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java @@ -21,6 +21,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.ActionContext; +import org.apache.struts2.ActionInvocation; import org.apache.struts2.inject.Inject; import org.apache.struts2.ognl.OgnlUtil; import org.apache.struts2.util.TextParseUtil; @@ -48,7 +49,21 @@ * * @since 2.5.9 */ -public interface WithLazyParams { +public interface WithLazyParams

{ + + /** + * @return a fresh holder for one invocation, seeded from the configured values + * @since 7.3.0 + */ + P newLazyParams(); + + /** + * Invoked in place of {@link Interceptor#intercept(ActionInvocation)} when lazy params apply. + * + * @param lazyParams params resolved for this invocation only + * @since 7.3.0 + */ + String intercept(ActionInvocation invocation, P lazyParams) throws Exception; class LazyParamInjector { @@ -80,14 +95,6 @@ public void setOgnlUtil(OgnlUtil ognlUtil) { this.ognlUtil = ognlUtil; } - public Interceptor injectParams(Interceptor interceptor, Map params, ActionContext invocationContext) { - for (Map.Entry 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; - } - /** * Resolves configured params into a per-invocation holder, leaving the interceptor untouched. *

diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java index c45091d770..c2cf407190 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -34,6 +34,8 @@ import org.apache.struts2.mock.MockActionInvocation; import org.apache.struts2.mock.MockActionProxy; import org.apache.struts2.util.ClassLoaderUtil; +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.ValueStackFactory; import org.assertj.core.util.Files; import org.springframework.mock.web.MockHttpServletRequest; @@ -41,8 +43,16 @@ import java.net.URI; import java.net.URL; import java.nio.charset.StandardCharsets; +import java.util.HashMap; import java.util.List; import java.util.Locale; +import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import static org.assertj.core.api.Assertions.assertThat; @@ -536,14 +546,7 @@ private MultiPartRequestWrapper createMultipartRequestMaxStringLength() { } private MultiPartRequestWrapper createMultipartRequest(int maxsize, int maxfilesize, int maxfiles, int maxStringLength) { - JakartaMultiPartRequest jak = new JakartaMultiPartRequest(); - jak.setMaxSize(String.valueOf(maxsize)); - jak.setMaxFileSize(String.valueOf(maxfilesize)); - jak.setMaxFiles(String.valueOf(maxfiles)); - jak.setMaxStringLength(String.valueOf(maxStringLength)); - jak.setDefaultEncoding(StandardCharsets.UTF_8.name()); - - return new MultiPartRequestWrapper(jak, request, tempDir.getAbsolutePath(), new DefaultLocaleProvider()); + return createMultipartRequest(request, maxsize, maxfilesize, maxfiles, maxStringLength); } protected void setUp() throws Exception { @@ -955,4 +958,171 @@ public void testUploadPolicyTreatsNullAsNoRestriction() { assertThat(policy.getAllowedExtensions()).isEmpty(); } + /** + * Regression for WW-5659: two concurrent invocations resolving different policies must not + * see each other's values. Scenario contributed by @deprrous in GitHub PR #1815. + */ + public void testConcurrentDynamicPoliciesStayIsolatedPerRequest() throws Exception { + CoordinatedActionFileUploadInterceptor sharedInterceptor = new CoordinatedActionFileUploadInterceptor(); + container.inject(sharedInterceptor); + + MyDynamicFileUploadAction plainPolicyAction = new MyDynamicFileUploadAction(); + plainPolicyAction.setAllowedMimeTypes("text/plain"); + container.inject(plainPolicyAction); + + MyDynamicFileUploadAction htmlPolicyAction = new MyDynamicFileUploadAction(); + htmlPolicyAction.setAllowedMimeTypes("text/html"); + container.inject(htmlPolicyAction); + + ExecutorService executor = Executors.newFixedThreadPool(2); + try { + Future plainResult = executor.submit(() -> runUploadAttempt( + sharedInterceptor, plainPolicyAction, createUploadRequest("plain-policy.html", "text/html", htmlContent))); + + assertThat(sharedInterceptor.awaitFirstValidation()).isTrue(); + + Future htmlResult = executor.submit(() -> runUploadAttempt( + sharedInterceptor, htmlPolicyAction, createUploadRequest("html-policy.html", "text/html", htmlContent))); + + assertThat(htmlResult.get(10, TimeUnit.SECONDS)).isEqualTo("success"); + sharedInterceptor.releaseFirstValidation(); + assertThat(plainResult.get(10, TimeUnit.SECONDS)).isEqualTo("success"); + } finally { + sharedInterceptor.releaseFirstValidation(); + executor.shutdownNow(); + sharedInterceptor.destroy(); + } + + // the text/plain policy must have rejected the text/html upload despite the concurrent + // text/html invocation resolving a more permissive policy on the same interceptor + assertThat(plainPolicyAction.getUploadFiles()).isNull(); + assertThat(plainPolicyAction.getFieldErrors()).containsKey("file"); + + assertThat(htmlPolicyAction.hasFieldErrors()).isFalse(); + assertThat(htmlPolicyAction.getUploadFiles()).isNotNull().hasSize(1); + assertThat(htmlPolicyAction.getUploadFiles().get(0).getOriginalName()).isEqualTo("html-policy.html"); + } + + /** + * Regression for WW-5659: resolving an invocation's params must leave the interceptor + * singleton exactly as configured. + */ + public void testResolutionDoesNotMutateTheInterceptor() throws Exception { + ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); + container.inject(interceptor); + interceptor.setAllowedTypes("text/plain"); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes("text/html"); + container.inject(action); + + runUploadAttempt(interceptor, action, createUploadRequest("f.html", "text/html", htmlContent)); + + assertThat(interceptor.newLazyParams().getAllowedTypes()).containsExactly("text/plain"); + } + + /** + * Regression for WW-5659: a lazily resolved {@code disabled} must apply to one invocation only. + */ + public void testDisabledIsResolvedPerInvocation() throws Exception { + ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); + container.inject(interceptor); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes("text/plain"); + container.inject(action); + + UploadPolicy policy = interceptor.newLazyParams(); + policy.setDisabled("true"); + + assertThat(policy.isDisabled()).isTrue(); + assertThat(interceptor.newLazyParams().isDisabled()).isFalse(); + } + + private String runUploadAttempt(ActionFileUploadInterceptor actionFileUploadInterceptor, + MyDynamicFileUploadAction action, + MockHttpServletRequest uploadRequest) throws Exception { + MultiPartRequestWrapper multiPartRequest = createMultipartRequest(uploadRequest, -1, -1, 3, -1); + ValueStack valueStack = container.getInstance(ValueStackFactory.class).createValueStack(); + valueStack.push(action); + + ActionContext context = ActionContext.of(valueStack.getContext()) + .withContainer(container) + .withValueStack(valueStack) + .withServletRequest(multiPartRequest) + .bind(); + try { + MockActionInvocation invocation = new MockActionInvocation(); + invocation.setAction(action); + invocation.setResultCode("success"); + invocation.setInvocationContext(context); + + Map params = new HashMap<>(); + params.put("allowedTypes", "${allowedMimeTypes}"); + + WithLazyParams.LazyParamInjector injector = new WithLazyParams.LazyParamInjector(valueStack); + container.inject(injector); + UploadPolicy policy = injector.resolveInto(actionFileUploadInterceptor.newLazyParams(), params, context); + + return actionFileUploadInterceptor.intercept(invocation, policy); + } finally { + ActionContext.clear(); + } + } + + private MockHttpServletRequest createUploadRequest(String filename, String contentType, String content) { + MockHttpServletRequest uploadRequest = new MockHttpServletRequest(); + uploadRequest.setCharacterEncoding(StandardCharsets.UTF_8.name()); + uploadRequest.setMethod("POST"); + uploadRequest.addHeader("Content-type", "multipart/form-data; boundary=\"" + boundary + "\""); + uploadRequest.setContent((encodeTextFile(filename, contentType, content) + endLine + "--" + boundary + "--") + .getBytes(StandardCharsets.UTF_8)); + return uploadRequest; + } + + private MultiPartRequestWrapper createMultipartRequest(MockHttpServletRequest multipartRequest, int maxsize, int maxfilesize, int maxfiles, int maxStringLength) { + JakartaMultiPartRequest jak = new JakartaMultiPartRequest(); + jak.setMaxSize(String.valueOf(maxsize)); + jak.setMaxFileSize(String.valueOf(maxfilesize)); + jak.setMaxFiles(String.valueOf(maxfiles)); + jak.setMaxStringLength(String.valueOf(maxStringLength)); + jak.setDefaultEncoding(StandardCharsets.UTF_8.name()); + return new MultiPartRequestWrapper(jak, multipartRequest, tempDir.getAbsolutePath(), new DefaultLocaleProvider()); + } + + /** Pauses the first validation so a second invocation can overlap it. From PR #1815 by @deprrous. */ + private static final class CoordinatedActionFileUploadInterceptor extends ActionFileUploadInterceptor { + private final AtomicBoolean pauseFirstValidation = new AtomicBoolean(true); + private final CountDownLatch firstValidationEntered = new CountDownLatch(1); + private final CountDownLatch allowFirstValidationToContinue = new CountDownLatch(1); + + @Override + protected boolean acceptFile(UploadPolicy policy, Object action, UploadedFile file, String originalFilename, String contentType, String inputName) { + if (pauseFirstValidation.compareAndSet(true, false)) { + firstValidationEntered.countDown(); + awaitUnchecked(allowFirstValidationToContinue); + } + return super.acceptFile(policy, action, file, originalFilename, contentType, inputName); + } + + private boolean awaitFirstValidation() throws InterruptedException { + return firstValidationEntered.await(10, TimeUnit.SECONDS); + } + + private void releaseFirstValidation() { + allowFirstValidationToContinue.countDown(); + } + + private void awaitUnchecked(CountDownLatch latch) { + try { + if (!latch.await(10, TimeUnit.SECONDS)) { + throw new AssertionError("Timed out waiting for concurrent validation release"); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new AssertionError("Interrupted while waiting for concurrent validation release", e); + } + } + } + } diff --git a/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java b/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java index ae3aea3cc3..75d3d021a9 100644 --- a/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java +++ b/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java @@ -21,9 +21,35 @@ import org.apache.struts2.ActionInvocation; import org.apache.struts2.SimpleAction; import org.apache.struts2.interceptor.AbstractInterceptor; +import org.apache.struts2.interceptor.InterceptorParams; import org.apache.struts2.interceptor.WithLazyParams; -public class MockLazyInterceptor extends AbstractInterceptor implements WithLazyParams { +public class MockLazyInterceptor extends AbstractInterceptor implements WithLazyParams { + + /** + * Per-invocation holder, seeded from the configured values. + */ + public static class MockLazyParams implements InterceptorParams { + + private String foo = ""; + private String bar = ""; + + public void setFoo(String foo) { + this.foo = foo; + } + + public String getFoo() { + return foo; + } + + public void setBar(String bar) { + this.bar = bar; + } + + public String getBar() { + return bar; + } + } private String foo = ""; private String bar = ""; @@ -44,12 +70,26 @@ public String getBar() { return bar; } + @Override + public MockLazyParams newLazyParams() { + MockLazyParams params = new MockLazyParams(); + params.setFoo(foo); + params.setBar(bar); + return params; + } + + @Override public String intercept(ActionInvocation invocation) throws Exception { + return intercept(invocation, newLazyParams()); + } + + @Override + public String intercept(ActionInvocation invocation, MockLazyParams lazyParams) throws Exception { if (invocation.getAction() instanceof SimpleAction) { - ((SimpleAction) invocation.getAction()).setName(foo); + ((SimpleAction) invocation.getAction()).setName(lazyParams.getFoo()); // Only set blah if bar is configured (not empty) - if (bar != null && !bar.isEmpty()) { - ((SimpleAction) invocation.getAction()).setBlah(bar); + if (lazyParams.getBar() != null && !lazyParams.getBar().isEmpty()) { + ((SimpleAction) invocation.getAction()).setBlah(lazyParams.getBar()); } } return invocation.invoke(); From 73c7ec350e3cda786c541fc71d3c0e4ac6c8b24d Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 11:23:16 +0200 Subject: [PATCH 09/23] WW-5659 test(core): cover both lazy-params skip branches and per-invocation disabled The two skip branches in DefaultActionInvocation#invokeWithLazyParams had no coverage: deleting either left the whole suite green. The only tests reaching that method used LazyFoo/LazyFooWithStackParams, which declare no disabled param, and MockLazyParams did not extend DisableParams, so the holder branch was unreachable and shouldIntercept was always true. Make MockLazyParams extend DisableParams and add two action configs that isolate one branch each: - LazyFooLazilyDisabled passes disabled as an interceptor-ref param, so it reaches InterceptorMapping#getParams(), resolves onto the holder, and exercises the DisableParams branch. - LazyFooStaticallyDisabled sets disabled on the interceptor definition instead. InterceptorBuilder only puts interceptor-ref params into the mapping, so the holder never sees it and it can only be honoured through ConditionalInterceptor#shouldIntercept. Verified by deleting each branch in turn: each deletion fails exactly the one test that targets it, and no other. Also make testDisabledIsResolvedPerInvocation earn its name. It previously asserted only that newLazyParams() returns a fresh object, never resolving anything, and built a MyDynamicFileUploadAction it never used. It now routes two actions through a real LazyParamInjector#resolveInto of a disabled=${uploadDisabled} param and pins that one invocation's resolved flag survives the other's, and that neither reaches the interceptor singleton. --- .../struts2/DefaultActionInvocationTest.java | 53 +++++++++++++++++++ .../ActionFileUploadInterceptorTest.java | 47 +++++++++++++--- .../struts2/mock/MockLazyInterceptor.java | 7 +-- core/src/test/resources/xwork-sample.xml | 21 ++++++++ .../src/test/resources/xwork-test-default.xml | 5 ++ 5 files changed, 124 insertions(+), 9 deletions(-) diff --git a/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java b/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java index 6a8e9fc21c..398fa7f77b 100644 --- a/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java +++ b/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java @@ -42,6 +42,7 @@ import java.util.concurrent.TimeUnit; import static org.apache.struts2.ognl.OgnlUtilTest.createOgnlUtil; +import static org.assertj.core.api.Assertions.assertThat; /** @@ -423,6 +424,58 @@ public void testInvokeWithLazyParamsStackConfiguration() throws Exception { assertEquals("static value", action.getBlah()); } + /** + * Regression for WW-5659: a {@code disabled} param resolved lazily from the value stack must skip + * the interceptor for that invocation. It arrives through the interceptor mapping's params, so it + * lands on the per-invocation holder and is honoured there, never on the shared interceptor. + */ + public void testInvokeWithLazyParamsSkipsLazilyDisabledInterceptor() throws Exception { + HashMap params = new HashMap<>(); + params.put("blah", "true"); + + ActionContext extraContext = ActionContext.of() + .withParameters(HttpParameters.create(params).build()); + + DefaultActionInvocation defaultActionInvocation = new DefaultActionInvocation(extraContext.getContextMap(), true); + container.inject(defaultActionInvocation); + + ActionProxy actionProxy = actionProxyFactory.createActionProxy("", "LazyFooLazilyDisabled", null, extraContext.getContextMap()); + defaultActionInvocation.init(actionProxy); + defaultActionInvocation.invoke(); + + SimpleAction action = (SimpleAction) defaultActionInvocation.getAction(); + + // the rest of the stack still ran, so the params interceptor applied blah... + assertThat(action.getBlah()).isEqualTo("true"); + // ...but the lazy interceptor was skipped, so it never applied its foo param to the action + assertThat(action.getName()).isNull(); + } + + /** + * Regression for WW-5659: {@code disabled} configured on the interceptor definition never reaches + * the params holder, so it can only be honoured through {@link org.apache.struts2.interceptor.ConditionalInterceptor#shouldIntercept}. + * The lazy path must still consult it. + */ + public void testInvokeWithLazyParamsSkipsStaticallyDisabledInterceptor() throws Exception { + HashMap params = new HashMap<>(); + params.put("blah", "dynamic value"); + + ActionContext extraContext = ActionContext.of() + .withParameters(HttpParameters.create(params).build()); + + DefaultActionInvocation defaultActionInvocation = new DefaultActionInvocation(extraContext.getContextMap(), true); + container.inject(defaultActionInvocation); + + ActionProxy actionProxy = actionProxyFactory.createActionProxy("", "LazyFooStaticallyDisabled", null, extraContext.getContextMap()); + defaultActionInvocation.init(actionProxy); + defaultActionInvocation.invoke(); + + SimpleAction action = (SimpleAction) defaultActionInvocation.getAction(); + + assertThat(action.getBlah()).isEqualTo("dynamic value"); + assertThat(action.getName()).isNull(); + } + public void testInvokeWithAsyncManager() throws Exception { DefaultActionInvocation dai = new DefaultActionInvocation(new HashMap<>(), false); dai.stack = container.getInstance(ValueStackFactory.class).createValueStack(); diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java index c2cf407190..e9bca76ccd 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -888,6 +888,7 @@ public static class MyDynamicFileUploadAction extends ActionSupport implements U private String allowedMimeTypes; private String allowedExtensions; private Long maxFileSize; + private String uploadDisabled; @Override public void withUploadedFiles(List uploadedFiles) { @@ -921,6 +922,14 @@ public Long getMaxFileSize() { public void setMaxFileSize(Long maxFileSize) { this.maxFileSize = maxFileSize; } + + public String getUploadDisabled() { + return uploadDisabled; + } + + public void setUploadDisabled(String uploadDisabled) { + this.uploadDisabled = uploadDisabled; + } } public void testUploadPolicyParsesAndCopies() { @@ -1028,17 +1037,43 @@ public void testDisabledIsResolvedPerInvocation() throws Exception { ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); container.inject(interceptor); - MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); - action.setAllowedMimeTypes("text/plain"); - container.inject(action); + MyDynamicFileUploadAction disablingAction = new MyDynamicFileUploadAction(); + disablingAction.setUploadDisabled("true"); + container.inject(disablingAction); - UploadPolicy policy = interceptor.newLazyParams(); - policy.setDisabled("true"); + MyDynamicFileUploadAction enablingAction = new MyDynamicFileUploadAction(); + enablingAction.setUploadDisabled("false"); + container.inject(enablingAction); + + UploadPolicy disabledPolicy = resolveDisabled(interceptor, disablingAction); + UploadPolicy enabledPolicy = resolveDisabled(interceptor, enablingAction); - assertThat(policy.isDisabled()).isTrue(); + // the second resolution must not have cleared the first invocation's flag... + assertThat(disabledPolicy.isDisabled()).isTrue(); + assertThat(enabledPolicy.isDisabled()).isFalse(); + // ...and neither resolution may reach the shared interceptor assertThat(interceptor.newLazyParams().isDisabled()).isFalse(); } + private UploadPolicy resolveDisabled(ActionFileUploadInterceptor actionFileUploadInterceptor, + MyDynamicFileUploadAction action) { + ValueStack valueStack = container.getInstance(ValueStackFactory.class).createValueStack(); + valueStack.push(action); + + ActionContext context = ActionContext.of(valueStack.getContext()) + .withContainer(container) + .withValueStack(valueStack) + .bind(); + try { + WithLazyParams.LazyParamInjector injector = new WithLazyParams.LazyParamInjector(valueStack); + container.inject(injector); + return injector.resolveInto(actionFileUploadInterceptor.newLazyParams(), + Map.of("disabled", "${uploadDisabled}"), context); + } finally { + ActionContext.clear(); + } + } + private String runUploadAttempt(ActionFileUploadInterceptor actionFileUploadInterceptor, MyDynamicFileUploadAction action, MockHttpServletRequest uploadRequest) throws Exception { diff --git a/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java b/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java index 75d3d021a9..f875c043cc 100644 --- a/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java +++ b/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java @@ -21,15 +21,16 @@ import org.apache.struts2.ActionInvocation; import org.apache.struts2.SimpleAction; import org.apache.struts2.interceptor.AbstractInterceptor; -import org.apache.struts2.interceptor.InterceptorParams; +import org.apache.struts2.interceptor.DisableParams; import org.apache.struts2.interceptor.WithLazyParams; public class MockLazyInterceptor extends AbstractInterceptor implements WithLazyParams { /** - * Per-invocation holder, seeded from the configured values. + * Per-invocation holder, seeded from the configured values. Extends {@link DisableParams} so a + * lazily resolved {@code disabled} param applies to a single invocation. */ - public static class MockLazyParams implements InterceptorParams { + public static class MockLazyParams extends DisableParams { private String foo = ""; private String bar = ""; diff --git a/core/src/test/resources/xwork-sample.xml b/core/src/test/resources/xwork-sample.xml index 5bc189d821..5615e8ef2f 100644 --- a/core/src/test/resources/xwork-sample.xml +++ b/core/src/test/resources/xwork-sample.xml @@ -72,6 +72,27 @@ + + + + + + should not be applied + ${blah} + + + + + + + + + should not be applied + + + 17 23 diff --git a/core/src/test/resources/xwork-test-default.xml b/core/src/test/resources/xwork-test-default.xml index 31f92d9d66..87931c7d29 100644 --- a/core/src/test/resources/xwork-test-default.xml +++ b/core/src/test/resources/xwork-test-default.xml @@ -42,6 +42,11 @@ expectedFoo + + + true + From 9d66a7b9f44953c2adadee4d8f892fa486ecb507 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 11:31:30 +0200 Subject: [PATCH 10/23] WW-5659 fix(core): reject uploads when the policy cannot be resolved --- .../AbstractFileUploadInterceptor.java | 12 ++++++ .../struts2/interceptor/UploadPolicy.java | 21 ++++++++++ .../apache/struts2/struts-messages.properties | 4 ++ .../ActionFileUploadInterceptorTest.java | 38 +++++++++++++++++++ 4 files changed, 75 insertions(+) diff --git a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java index f8529732bc..dac450b450 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java @@ -56,6 +56,7 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor public static final String STRUTS_MESSAGES_INVALID_CONTENT_TYPE_KEY = "struts.messages.invalid.content.type"; public static final String STRUTS_MESSAGES_ERROR_CONTENT_TYPE_NOT_ALLOWED_KEY = "struts.messages.error.content.type.not.allowed"; public static final String STRUTS_MESSAGES_ERROR_FILE_EXTENSION_NOT_ALLOWED_KEY = "struts.messages.error.file.extension.not.allowed"; + public static final String STRUTS_MESSAGES_ERROR_UPLOAD_POLICY_UNRESOLVED_KEY = "struts.messages.error.upload.policy.unresolved"; private final UploadPolicy configuredPolicy = new UploadPolicy(); @@ -140,6 +141,17 @@ protected boolean acceptFile(UploadPolicy policy, Object action, UploadedFile fi return false; } + if (policy.isUnresolved()) { + String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_UPLOAD_POLICY_UNRESOLVED_KEY, new String[]{ + inputName, originalFilename, String.join(", ", policy.getUnresolvedParams()) + }); + if (validation != null) { + validation.addFieldError(inputName, errMsg); + } + LOG.warn(errMsg); + return false; + } + if (policy.getMaximumSize() != null && policy.getMaximumSize() < file.length()) { String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_FILE_TOO_LARGE_KEY, new String[]{ inputName, originalFilename, file.getName(), "" + file.length(), getMaximumSizeStr(action, policy.getMaximumSize()) diff --git a/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java index a9e4441576..08702dee48 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java +++ b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java @@ -21,6 +21,7 @@ import org.apache.struts2.util.TextParseUtil; import java.util.Collections; +import java.util.LinkedHashSet; import java.util.Set; /** @@ -36,6 +37,7 @@ public class UploadPolicy extends DisableParams { private Long maximumSize; private Set allowedTypes = Collections.emptySet(); private Set allowedExtensions = Collections.emptySet(); + private final Set unresolvedParams = new LinkedHashSet<>(); public UploadPolicy() { } @@ -45,6 +47,7 @@ private UploadPolicy(UploadPolicy other) { this.maximumSize = other.maximumSize; this.allowedTypes = other.allowedTypes; this.allowedExtensions = other.allowedExtensions; + this.unresolvedParams.addAll(other.unresolvedParams); } /** @@ -80,6 +83,24 @@ public Set getAllowedExtensions() { return allowedExtensions; } + /** + * A parameter that could not be resolved makes this policy unusable: the upload is rejected + * rather than validated against a partially-resolved policy, so a broken expression cannot + * silently relax validation. + */ + @Override + public void unresolved(String paramName) { + unresolvedParams.add(paramName); + } + + public boolean isUnresolved() { + return !unresolvedParams.isEmpty(); + } + + public Set getUnresolvedParams() { + return Collections.unmodifiableSet(unresolvedParams); + } + /** * @return an independent copy, used to seed a per-invocation policy from the configured one */ diff --git a/core/src/main/resources/org/apache/struts2/struts-messages.properties b/core/src/main/resources/org/apache/struts2/struts-messages.properties index 63514b000f..ecd99000bd 100644 --- a/core/src/main/resources/org/apache/struts2/struts-messages.properties +++ b/core/src/main/resources/org/apache/struts2/struts-messages.properties @@ -50,6 +50,10 @@ struts.messages.error.content.type.not.allowed=Content-Type not allowed: {0} "{1 # 2 - file name after uploading the file # 3 - content type of the file struts.messages.error.file.extension.not.allowed=File extension not allowed: {0} "{1}" "{2}" {3} +# 0 - input name +# 1 - original filename +# 2 - comma-delimited list of unresolved parameter names +struts.messages.error.upload.policy.unresolved=The upload validation policy could not be resolved, rejecting the file: {0} "{1}"; unresolved parameters: {2} # dedicated messages used to handle various problems with file upload - check {@link JakartaMultiPartRequest#parse(HttpServletRequest, String)} # params depend on exception being handled # FileUploadByteCountLimitException diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java index e9bca76ccd..ab31c3d18b 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -1160,4 +1160,42 @@ private void awaitUnchecked(CountDownLatch latch) { } } + public void testUnresolvedPolicyRejectsTheUpload() throws Exception { + ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); + container.inject(interceptor); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes(null); // ${allowedMimeTypes} will not resolve + container.inject(action); + + runUploadAttempt(interceptor, action, createUploadRequest("f.txt", "text/plain", plainContent)); + + assertThat(action.getUploadFiles()).isNull(); + assertThat(action.getFieldErrors()).containsKey("file"); + } + + public void testResolvedPolicyStillAcceptsTheUpload() throws Exception { + ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); + container.inject(interceptor); + + MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); + action.setAllowedMimeTypes("text/plain"); + container.inject(action); + + runUploadAttempt(interceptor, action, createUploadRequest("f.txt", "text/plain", plainContent)); + + assertThat(action.hasFieldErrors()).isFalse(); + assertThat(action.getUploadFiles()).isNotNull().hasSize(1); + } + + public void testUploadPolicyTracksUnresolvedParams() { + UploadPolicy policy = new UploadPolicy(); + assertThat(policy.isUnresolved()).isFalse(); + + policy.unresolved("allowedTypes"); + + assertThat(policy.isUnresolved()).isTrue(); + assertThat(policy.getUnresolvedParams()).containsExactly("allowedTypes"); + } + } From 83c52c9f077519d7f65ed0f7260568b9e31b6cfc Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 11:39:55 +0200 Subject: [PATCH 11/23] WW-5659 test(core): exercise real lazy param resolution in dynamic upload tests --- .../ActionFileUploadInterceptor.java | 13 ++++ .../ActionFileUploadInterceptorTest.java | 62 +++++++++++++------ ...5659-lazy-params-request-scoping-design.md | 26 +++----- 3 files changed, 67 insertions(+), 34 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java index 82ff798e3f..c898275f09 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java @@ -198,6 +198,19 @@ * } * * + *

+ * Dynamic parameters are resolved into a fresh {@link UploadPolicy} for each invocation, so the + * interceptor itself is never modified per request and concurrent uploads cannot observe each + * other's policy. An expression that cannot be resolved does not relax validation: the policy is + * marked unresolved and affected uploads are rejected. A lazily resolved {@code disabled} param + * only takes effect if the interceptor's params holder extends {@link DisableParams} — there is + * deliberately no fallback to the interceptor instance. Likewise, an interceptor that overrides + * {@link ConditionalInterceptor#shouldIntercept(ActionInvocation) shouldIntercept} to read its own + * lazily-injected fields would see only config-time values, since resolution never touches the + * interceptor; {@code ActionFileUploadInterceptor} does not override {@code shouldIntercept}, so + * this does not affect it. + *

+ * * @see WithLazyParams * @see UploadedFilesAware * @see AbstractFileUploadInterceptor diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java index ab31c3d18b..abdc127631 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -648,11 +648,10 @@ public void testDynamicParameterEvaluation() throws Exception { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - // Simulate WithLazyParams injection by manually setting the parameters - // In real execution, DefaultActionInvocation.invoke() would call LazyParamInjector - interceptor.setAllowedTypes(action.getAllowedMimeTypes()); - - interceptor.intercept(mai); + // Exercise the real resolution path: LazyParamInjector resolves ${allowedMimeTypes} + // into a fresh UploadPolicy instead of mutating the shared interceptor. + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, false, false); + interceptor.intercept(mai, policy); List files = action.getUploadFiles(); @@ -686,8 +685,8 @@ public void testDynamicParametersChangePerRequest() throws Exception { ActionContext.getContext().getValueStack().push(action1); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setAllowedTypes(action1.getAllowedMimeTypes()); - interceptor.intercept(mai1); + UploadPolicy policy1 = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, false, false); + interceptor.intercept(mai1, policy1); assertThat(action1.getUploadFiles()).isNotNull().hasSize(1); assertThat(action1.getUploadFiles().get(0).getContentType()).isEqualTo("text/plain"); @@ -715,8 +714,8 @@ public void testDynamicParametersChangePerRequest() throws Exception { ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); // Simulate new parameter evaluation for second request - interceptor.setAllowedTypes(action2.getAllowedMimeTypes()); - interceptor.intercept(mai2); + UploadPolicy policy2 = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, false, false); + interceptor.intercept(mai2, policy2); assertThat(action2.getUploadFiles()).isNotNull().hasSize(1); assertThat(action2.getUploadFiles().get(0).getContentType()).isEqualTo("text/html"); @@ -746,8 +745,8 @@ public void testDynamicExtensionValidation() throws Exception { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setAllowedExtensions(action.getAllowedExtensions()); - interceptor.intercept(mai); + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), false, true, false); + interceptor.intercept(mai, policy); List files = action.getUploadFiles(); @@ -785,8 +784,8 @@ public void testDynamicMaximumSizeValidation() throws Exception { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setMaximumSize(action.getMaxFileSize()); - interceptor.intercept(mai); + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), false, false, true); + interceptor.intercept(mai, policy); // File should be rejected due to size assertThat(action.hasFieldErrors()).isTrue(); @@ -819,9 +818,8 @@ public void testSecurityValidationWithDynamicParameters() throws Exception { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setAllowedTypes(action.getAllowedMimeTypes()); - interceptor.setAllowedExtensions(action.getAllowedExtensions()); - interceptor.intercept(mai); + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, true, false); + interceptor.intercept(mai, policy); List files = action.getUploadFiles(); @@ -856,8 +854,8 @@ public void testWildcardMatchingWithDynamicParameters() throws Exception { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setAllowedTypes(action.getAllowedMimeTypes()); - interceptor.intercept(mai); + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, false, false); + interceptor.intercept(mai, policy); List files = action.getUploadFiles(); @@ -1074,6 +1072,34 @@ private UploadPolicy resolveDisabled(ActionFileUploadInterceptor actionFileUploa } } + /** + * Resolves the given flags into a fresh {@link UploadPolicy} via the real + * {@link WithLazyParams.LazyParamInjector} path, using the already-bound {@code context} + * (and its ValueStack, with the action already pushed by the caller) rather than fabricating + * a new one. This mirrors what {@code DefaultActionInvocation} does at request time, so tests + * exercise the actual resolution instead of hand-calling a setter on the shared interceptor. + */ + private UploadPolicy injectDynamicUploadPolicy(ActionFileUploadInterceptor actionFileUploadInterceptor, + ActionContext context, + boolean includeAllowedTypes, + boolean includeAllowedExtensions, + boolean includeMaximumSize) { + Map params = new HashMap<>(); + if (includeAllowedTypes) { + params.put("allowedTypes", "${allowedMimeTypes}"); + } + if (includeAllowedExtensions) { + params.put("allowedExtensions", "${allowedExtensions}"); + } + if (includeMaximumSize) { + params.put("maximumSize", "${maxFileSize}"); + } + + WithLazyParams.LazyParamInjector injector = new WithLazyParams.LazyParamInjector(context.getValueStack()); + container.inject(injector); + return injector.resolveInto(actionFileUploadInterceptor.newLazyParams(), params, context); + } + private String runUploadAttempt(ActionFileUploadInterceptor actionFileUploadInterceptor, MyDynamicFileUploadAction action, MockHttpServletRequest uploadRequest) throws Exception { 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 20e8a7c962..397b79be37 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 @@ -290,19 +290,15 @@ Under this design: `unresolved(paramName)` is called. - A WARN is logged naming interceptor, param and expression. -`UploadPolicy.unresolved(param)` then has to decide whether the seeded value is usable. Two -cases exist, and they are distinguishable: - -- **A genuine static fallback.** The interceptor's own `` definition carried a - literal value for that param and the `` overrode it with an expression, - so the seed is a real value (e.g. `image/png`). That value applies and validation - proceeds normally. -- **No fallback.** The only configuration for that param is the expression itself, so - `buildInterceptor` seeded the holder with the literal `${...}` text — as a set containing - the string `"${uploadConfig.allowedMimeTypes}"`, which matches no content type. - -The rule: `unresolved(param)` marks the dimension unusable **only if** the seeded value for -that dimension still contains `${`. Otherwise a genuine static fallback exists and is used. +`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`. + +The seed-introspection alternative — honouring a seeded value that does not itself contain +`${` — was rejected during planning: it is not implementable deterministically, because +`maximumSize` is seeded as a `Long` and cannot carry a `${...}` literal, so the rule would behave +differently per parameter type. + A dimension marked unusable causes `acceptFile` to reject the file with a dedicated message rather than the opaque one produced by matching content types against literal `${...}` text. This needs a new bundle key — `struts.messages.error.upload.policy.unresolved` — added to @@ -324,9 +320,7 @@ silently never runs. New tests must follow the existing style. `newLazyParams()` still returns the configured values — the singleton was never written. - `disabled` request-scoping: concurrent invocations resolving different `disabled` values; assert only the intended one is skipped. -- Fail-closed: unresolved expression with no static fallback → rejected with the new - message; unresolved *with* a static fallback (literal on the `` definition, - expression on the ``) → the static value applies and validation proceeds. +- Fail-closed: unresolved expression → rejected with the new message. - `ConditionalInterceptor` interaction: a `WithLazyParams` interceptor that is also conditional still honours a custom `shouldIntercept`. - Migrate existing dynamic tests (`testDynamicParameterEvaluation` and friends) off the From 0cafec4d24cc751e554ef58c9850fa671ab2ab73 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 11:59:33 +0200 Subject: [PATCH 12/23] WW-5659 fix(core): mark params unusable when a lazy value cannot be applied MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../struts2/interceptor/WithLazyParams.java | 32 ++++++++++++++----- .../interceptor/LazyParamInjectorTest.java | 14 +++++++- ...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 85d8e6d71a..64cea04996 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 void setOgnlUtil(OgnlUtil ognlUtil) { /** * Resolves configured params into a per-invocation holder, leaving the interceptor untouched. *

- * 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. + * Every path that skips a write notifies the holder 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: + *

    + *
  • a {@code ${...}} expression that resolves to null or an empty value (see + * {@link #isUnresolved})
    • + *
  • 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
    • + *
+ * In both cases the holder keeps its seeded configuration value and a WARN is logged. + *

+ * 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

P resolveInto(P target, Map // 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

P resolveInto(P target, Map * 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. + *

+ * Partial resolution is not detected. 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 46f5282f0f..48ef39c566 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 static class Bean { 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 void testExpressionResolvingToEmptyIsTreatedAsUnresolved() { assertThat(holder.getUnresolvedCalls()).containsExactly("name"); } + public void testValueThatCannotBeConvertedSkipsWriteAndNotifiesHolder() { + Map 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 params = new HashMap<>(); params.put("disabled", "true"); @@ -136,7 +147,7 @@ public void testResolvesDisabledOntoDisableParams() { assertThat(holder.isDisabled()).isTrue(); } - public void testUnknownParamIsIgnoredWithoutFailingTheInvocation() { + public void testUnknownParamDoesNotFailTheInvocationButNotifiesHolder() { Map params = new HashMap<>(); params.put("noSuchParam", "whatever"); @@ -144,5 +155,6 @@ public void testUnknownParamIsIgnoredWithoutFailingTheInvocation() { 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 397b79be37..221dff23fe 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`. From ac4676db5daf7f453f993331b90dcc3e8c88846b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 11:59:41 +0200 Subject: [PATCH 13/23] WW-5659 chore(core): harden the policy sets and tidy the lazy params dispatch UploadPolicy handed out the mutable HashSet built by commaDelimitedStringToSet, which the copy constructor shares by reference with the configured policy, so a subclass overriding the protected acceptFile could have rewritten process-wide config from a request thread. The sets are unmodifiable now. Also document that unresolved() records `disabled` like any other param, so an unresolvable disabled expression leaves the interceptor enabled and rejects every upload; and in DefaultActionInvocation use normal imports for the params types, make the interceptor local final, word the three skip logs consistently and identify the interceptor by its mapping name, and note that the name-based param merge is inherited behaviour whose duplicate-ref handling is questionable. Co-Authored-By: Claude Opus 5 --- .../struts2/DefaultActionInvocation.java | 38 +++++++++++++++---- .../struts2/interceptor/UploadPolicy.java | 14 ++++++- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java b/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java index 913590410c..72a35afe5b 100644 --- a/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java +++ b/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java @@ -30,7 +30,9 @@ import org.apache.struts2.inject.Container; import org.apache.struts2.inject.Inject; import org.apache.struts2.interceptor.ConditionalInterceptor; +import org.apache.struts2.interceptor.DisableParams; import org.apache.struts2.interceptor.Interceptor; +import org.apache.struts2.interceptor.InterceptorParams; import org.apache.struts2.interceptor.PreResultListener; import org.apache.struts2.interceptor.WithLazyParams; import org.apache.struts2.ognl.OgnlUtil; @@ -258,11 +260,11 @@ public String invoke() throws Exception { if (asyncManager == null || !asyncManager.hasAsyncActionResult()) { if (interceptors.hasNext()) { final InterceptorMapping interceptorMapping = interceptors.next(); - Interceptor interceptor = interceptorMapping.getInterceptor(); + final Interceptor interceptor = interceptorMapping.getInterceptor(); if (interceptor instanceof WithLazyParams lazyInterceptor) { resultCode = invokeWithLazyParams(lazyInterceptor, interceptorMapping); } else if (interceptor instanceof ConditionalInterceptor conditionalInterceptor) { - resultCode = executeConditional(conditionalInterceptor); + resultCode = executeConditional(conditionalInterceptor, interceptorMapping.getName()); } else { LOG.debug("Executing normal interceptor: {}", interceptorMapping.getName()); resultCode = interceptor.intercept(this); @@ -313,18 +315,18 @@ public String invoke() throws Exception { * lazily resolved {@code disabled} flag and any custom {@code shouldIntercept} must be honoured * here, because the single-argument {@code intercept} is not the entry point on this path. */ - private

String invokeWithLazyParams( + private

String invokeWithLazyParams( WithLazyParams

lazyInterceptor, InterceptorMapping interceptorMapping) throws Exception { P lazyParams = lazyParamInjector.resolveInto( lazyInterceptor.newLazyParams(), mergedParams(interceptorMapping), invocationContext); - if (lazyParams instanceof org.apache.struts2.interceptor.DisableParams disableParams && disableParams.isDisabled()) { - LOG.debug("Interceptor: {} is disabled for this invocation, skipping to next", interceptorMapping.getName()); + if (lazyParams instanceof DisableParams disableParams && disableParams.isDisabled()) { + LOG.debug("Interceptor: {} is disabled by its lazily resolved params, skipping to next", interceptorMapping.getName()); return this.invoke(); } if (lazyInterceptor instanceof ConditionalInterceptor conditionalInterceptor && !conditionalInterceptor.shouldIntercept(this)) { - LOG.debug("Interceptor: {} is disabled, skipping to next", interceptorMapping.getName()); + LOG.debug("Interceptor: {} declined by shouldIntercept() on the lazy params path, skipping to next", interceptorMapping.getName()); return this.invoke(); } LOG.debug("Executing lazy params interceptor: {}", interceptorMapping.getName()); @@ -332,6 +334,13 @@ private

String invo } /** + * Merges the params declared on the interceptor-ref with those of the mapping being invoked. + *

+ * The name-based lookup is inherited behaviour, kept as-is: the mapping is normally the very one + * found by name, so the merge is a no-op, and when a stack references the same interceptor name + * twice with different params it merges the first mapping's params over the current one, which + * is questionable. Changing it is out of scope here. + * * @return a fresh map; the mapping's own param map is shared across requests and must not be mutated */ private Map mergedParams(InterceptorMapping interceptorMapping) { @@ -343,12 +352,25 @@ private Map mergedParams(InterceptorMapping interceptorMapping) return merged; } + /** + * @deprecated since 7.3.0, use {@link #executeConditional(ConditionalInterceptor, String)} so the + * interceptor is identified by its mapping name in the logs. + */ + @Deprecated protected String executeConditional(ConditionalInterceptor conditionalInterceptor) throws Exception { + return executeConditional(conditionalInterceptor, conditionalInterceptor.getClass().getSimpleName()); + } + + /** + * @param interceptorName the name of the interceptor mapping being invoked, used for logging + * @since 7.3.0 + */ + protected String executeConditional(ConditionalInterceptor conditionalInterceptor, String interceptorName) throws Exception { if (conditionalInterceptor.shouldIntercept(this)) { - LOG.debug("Executing conditional interceptor: {}", conditionalInterceptor.getClass().getSimpleName()); + LOG.debug("Executing conditional interceptor: {}", interceptorName); return conditionalInterceptor.intercept(this); } else { - LOG.debug("Interceptor: {} is disabled, skipping to next", conditionalInterceptor.getClass().getSimpleName()); + LOG.debug("Interceptor: {} declined by shouldIntercept(), skipping to next", interceptorName); return this.invoke(); } } diff --git a/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java index 08702dee48..5911a401e4 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java +++ b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java @@ -87,6 +87,13 @@ public Set getAllowedExtensions() { * A parameter that could not be resolved makes this policy unusable: the upload is rejected * rather than validated against a partially-resolved policy, so a broken expression cannot * silently relax validation. + *

+ * Any param name is recorded, {@code disabled} included. An unresolvable + * {@code ${...}} therefore leaves the interceptor enabled (the + * flag keeps its configured value) and marks this policy unusable, so every upload in + * that invocation is rejected. That is deliberate — a broken dispatch flag is a broken + * configuration, and refusing the upload is the safe reading — but it means a typo in + * {@code disabled} takes out upload validation rather than only the disabling. */ @Override public void unresolved(String paramName) { @@ -108,9 +115,14 @@ public UploadPolicy copy() { return new UploadPolicy(this); } + /** + * The resulting set is unmodifiable: the copy constructor shares it by reference with the + * configured policy, so every per-invocation policy that does not override the param would + * otherwise hand out a live handle on process-wide configuration. + */ private static Set toSet(String commaDelimited) { return commaDelimited == null ? Collections.emptySet() - : TextParseUtil.commaDelimitedStringToSet(commaDelimited); + : Collections.unmodifiableSet(TextParseUtil.commaDelimitedStringToSet(commaDelimited)); } } From eaca05d6229e47c8b088d38a3671abb48da4d9ac Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 12:47:43 +0200 Subject: [PATCH 14/23] WW-5659 fix(core): allowlist the lazy params holder for OGNL member access Moving lazy param resolution off the interceptor and onto a per-invocation InterceptorParams holder changed the OGNL target of the write. The interceptor is allowlisted at configuration time by XmlDocConfigurationProvider, because it is named in the configuration; the holder is named nowhere, so with the shipped default struts.allowlist.enable=true SecurityMemberAccess refused every setter, resolveInto's fail-closed handling marked every param unresolved, and ActionFileUploadInterceptor rejected every upload. Register the holder's own class hierarchy through ProviderAllowlist when the interceptor is built, keyed by the holder class so repeated builds collapse onto one entry. Only the holder's class, superclasses and interfaces are registered - the setter may be declared on any of them and SecurityMemberAccess checks both the target and the declaring class. Object is filtered out: it says nothing about the holder and is excluded by default anyway. No package is allowlisted. Also run Interceptor#init() before the holder is obtained, so newLazyParams() sees a fully initialised interceptor. Co-Authored-By: Claude Opus 5 --- .../factory/DefaultInterceptorFactory.java | 59 ++++++++++++++++++- 1 file changed, 57 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java b/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java index cd5bc09bd2..ee1a3d0155 100644 --- a/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java +++ b/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java @@ -22,14 +22,19 @@ import org.apache.logging.log4j.Logger; import org.apache.struts2.ObjectFactory; import org.apache.struts2.config.ConfigurationException; +import org.apache.struts2.config.ConfigurationUtil; import org.apache.struts2.config.entities.InterceptorConfig; import org.apache.struts2.inject.Inject; import org.apache.struts2.interceptor.Interceptor; +import org.apache.struts2.interceptor.InterceptorParams; import org.apache.struts2.interceptor.WithLazyParams; +import org.apache.struts2.ognl.ProviderAllowlist; import org.apache.struts2.util.reflection.ReflectionProvider; import java.util.HashMap; import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; /** * Default implementation @@ -40,6 +45,7 @@ public class DefaultInterceptorFactory implements InterceptorFactory { private ObjectFactory objectFactory; private ReflectionProvider reflectionProvider; + private ProviderAllowlist providerAllowlist; @Inject public void setObjectFactory(ObjectFactory objectFactory) { @@ -51,6 +57,11 @@ public void setReflectionProvider(ReflectionProvider reflectionProvider) { this.reflectionProvider = reflectionProvider; } + @Inject + public void setProviderAllowlist(ProviderAllowlist providerAllowlist) { + this.providerAllowlist = providerAllowlist; + } + public Interceptor buildInterceptor(InterceptorConfig interceptorConfig, Map interceptorRefParams) throws ConfigurationException { String interceptorClassName = interceptorConfig.getClassName(); Map thisInterceptorClassParams = interceptorConfig.getParams(); @@ -69,12 +80,15 @@ public Interceptor buildInterceptor(InterceptorConfig interceptorConfig, Map lazyInterceptor) { LOG.debug("Interceptor {} implements {} - expression parameters will be re-evaluated during action invocation", interceptorClassName, WithLazyParams.class.getName()); + allowlistLazyParamsHolder(lazyInterceptor); } - interceptor.init(); return interceptor; } catch (InstantiationException e) { cause = e; @@ -96,4 +110,45 @@ public Interceptor buildInterceptor(InterceptorConfig interceptorConfig, Map + * {@link WithLazyParams.LazyParamInjector#resolveInto} writes the resolved {@code ${...}} values + * onto the holder with OGNL. With {@code struts.allowlist.enable=true} (the default) that write + * is refused unless the holder's class is allowlisted, and the fail-closed handling in + * {@code resolveInto} would then mark every lazy param unresolved. The interceptor itself is + * allowlisted by {@code XmlDocConfigurationProvider.allowAndLoadClass} when its config is read; + * the holder is never named in any configuration, so it has to be registered here. + *

+ * Only the holder's own class hierarchy is registered - its class, its superclasses and the + * interfaces it implements - because the setter being written may be declared on any of them + * ({@code disabled} for instance is declared on {@link org.apache.struts2.interceptor.DisableParams}), + * and {@code SecurityMemberAccess} requires both the target class and the declaring class of the + * member to be allowlisted. {@link Object} is deliberately dropped from that set: it is a + * universal supertype that would say nothing about this holder, and it is excluded by default + * anyway ({@code struts.excludedClasses}), so registering it could only ever mislead a reader. + * No package is allowlisted and nothing beyond the hierarchy is added. + *

+ * The holder class is used as the registration key so repeated registrations - the factory is a + * prototype and several interceptors may share a holder type - collapse onto one entry. + */ + private void allowlistLazyParamsHolder(WithLazyParams lazyInterceptor) { + if (providerAllowlist == null) { + LOG.warn("No ProviderAllowlist available, cannot allowlist the lazy params holder of [{}];" + + " lazy params will fail to resolve if the OGNL allowlist is enabled", lazyInterceptor.getClass().getName()); + return; + } + InterceptorParams lazyParams = lazyInterceptor.newLazyParams(); + if (lazyParams == null) { + LOG.warn("Interceptor [{}] returned no lazy params holder, nothing to allowlist", lazyInterceptor.getClass().getName()); + return; + } + Class holderClass = lazyParams.getClass(); + Set> holderTypes = ConfigurationUtil.getAllClassTypes(holderClass).stream() + .filter(type -> type != Object.class) + .collect(Collectors.toSet()); + LOG.debug("Allowlisting lazy params holder [{}] and its supertypes {}", holderClass.getName(), holderTypes); + providerAllowlist.registerAllowlist(holderClass, holderTypes); + } + } From d77739b747d9f772434a3ed15d381d81d5f144c5 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 12:47:50 +0200 Subject: [PATCH 15/23] WW-5659 test(core): prove lazy params resolve with the OGNL allowlist enabled Every other core test runs with struts.allowlist.enable=false - StrutsTestCaseHelper turns it off by default and XWorkTestCaseHelper never loads default.properties - so no core test could see the holder being blocked by SecurityMemberAccess. Only the showcase DynamicFileUploadTest integration test exercised the production setting, which is why the regression reached CI. This test boots the dispatcher with the allowlist enforced and asserts both that the holder hierarchy is registered at configuration time and that a ${...} param actually lands on the policy rather than being reported unresolved. Reverting the registration in DefaultInterceptorFactory fails it with the same "Declaring class [UploadPolicy] ... is not allowlisted" warning seen in the showcase failure. Co-Authored-By: Claude Opus 5 --- .../interceptor/LazyParamsAllowlistTest.java | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 core/src/test/java/org/apache/struts2/interceptor/LazyParamsAllowlistTest.java diff --git a/core/src/test/java/org/apache/struts2/interceptor/LazyParamsAllowlistTest.java b/core/src/test/java/org/apache/struts2/interceptor/LazyParamsAllowlistTest.java new file mode 100644 index 0000000000..8e07147949 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/LazyParamsAllowlistTest.java @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.interceptor; + +import org.apache.struts2.ActionContext; +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.dispatcher.PrepareOperations; +import org.apache.struts2.ognl.ProviderAllowlist; +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.ValueStackFactory; + +import java.util.HashMap; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Regression for WW-5659: lazy interceptor params must still resolve when the OGNL allowlist is + * enforced. + *

+ * {@link WithLazyParams.LazyParamInjector} writes resolved values onto a per-invocation + * {@link InterceptorParams} holder with OGNL. The holder is never named in any configuration, so + * unlike the interceptor it is not allowlisted by + * {@code XmlDocConfigurationProvider.allowAndLoadClass}; without an explicit registration + * {@code SecurityMemberAccess} refuses the write and the fail-closed handling marks every param + * unresolved, which rejects every upload. + *

+ * Every other core test runs with {@code struts.allowlist.enable=false} (see + * {@code StrutsTestCaseHelper}), so this is the only core coverage of that production setting for + * this feature - the showcase {@code DynamicFileUploadTest} integration test is the other one. + */ +public class LazyParamsAllowlistTest extends StrutsInternalTestCase { + + /** + * Stands in for the action whose state drives the upload rules. Allowlisted explicitly below, + * as a real action class would be by the configuration provider that loads it. + */ + public static class UploadRules { + + public String getAllowedMimeTypes() { + return "text/plain"; + } + + public Long getMaxFileSize() { + return 2048L; + } + } + + @Override + protected void setUp() throws Exception { + PrepareOperations.clearDevModeOverride(); + Map params = new HashMap<>(); + // the shipped default, which StrutsTestCaseHelper otherwise turns off for tests + params.put(StrutsConstants.STRUTS_ALLOWLIST_ENABLE, "true"); + // only the value-stack root needs help; the params holder must be allowlisted by the framework + params.put(StrutsConstants.STRUTS_ALLOWLIST_CLASSES, UploadRules.class.getName()); + initDispatcher(params); + } + + public void testAllowlistIsActuallyEnforced() { + assertThat(container.getInstance(String.class, StrutsConstants.STRUTS_ALLOWLIST_ENABLE)).isEqualTo("true"); + } + + public void testParamsHolderHierarchyIsAllowlistedAtConfigurationTime() { + ProviderAllowlist providerAllowlist = container.getInstance(ProviderAllowlist.class); + + assertThat(providerAllowlist.getProviderAllowlist()).contains( + UploadPolicy.class, // the holder itself, the OGNL target + DisableParams.class, // declares setDisabled + InterceptorParams.class // declares unresolved + ); + } + + /** + * The regression proper: a {@code ${...}} param must reach the holder, not be swallowed by the + * allowlist and reported as unresolved. + */ + public void testLazyParamsResolveOntoTheHolderWithAllowlistEnabled() { + ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); + container.inject(interceptor); + + ValueStack valueStack = container.getInstance(ValueStackFactory.class).createValueStack(); + valueStack.push(new UploadRules()); + ActionContext context = ActionContext.of(valueStack.getContext()) + .withContainer(container) + .withValueStack(valueStack) + .bind(); + try { + WithLazyParams.LazyParamInjector injector = new WithLazyParams.LazyParamInjector(valueStack); + container.inject(injector); + + Map params = new HashMap<>(); + params.put("allowedTypes", "${allowedMimeTypes}"); + params.put("maximumSize", "${maxFileSize}"); + + UploadPolicy policy = injector.resolveInto(interceptor.newLazyParams(), params, context); + + assertThat(policy.getUnresolvedParams()).isEmpty(); + assertThat(policy.isUnresolved()).isFalse(); + assertThat(policy.getAllowedTypes()).containsExactly("text/plain"); + assertThat(policy.getMaximumSize()).isEqualTo(2048L); + } finally { + ActionContext.clear(); + } + } +} From bfb4fbaa5b79e07e00ce530a376fad4a97435e84 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 12:47:57 +0200 Subject: [PATCH 16/23] WW-5659 fix(core): stop an unresolvable disabled param from voiding the upload policy UploadPolicy#unresolved recorded every param name, disabled included, so an unresolvable ${...} marked the whole policy unusable and rejected every upload of the invocation. That is not a safe default: disabled is not a validation dimension. Its unresolved value is simply false, which leaves the interceptor running and the rest of the policy intact, so it cannot relax validation - recording it only invents a second failure mode. Exclude it from the tracking that gates isUnresolved(), via a new DisableParams#DISABLED_PARAM constant, and replace the javadoc that defended the old behaviour. A param that is a validation dimension still voids the policy, including when it fails alongside disabled. Co-Authored-By: Claude Opus 5 --- .../struts2/interceptor/DisableParams.java | 7 +++++ .../struts2/interceptor/UploadPolicy.java | 14 +++++---- .../ActionFileUploadInterceptorTest.java | 29 +++++++++++++++++++ 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/DisableParams.java b/core/src/main/java/org/apache/struts2/interceptor/DisableParams.java index aa34467879..860f4b520e 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/DisableParams.java +++ b/core/src/main/java/org/apache/struts2/interceptor/DisableParams.java @@ -29,6 +29,13 @@ */ public class DisableParams implements InterceptorParams { + /** + * Name of the parameter bound to {@link #setDisabled(String)}. + * + * @since 7.3.0 + */ + public static final String DISABLED_PARAM = "disabled"; + private boolean disabled; public DisableParams() { diff --git a/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java index 5911a401e4..50d647cde5 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java +++ b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java @@ -88,15 +88,17 @@ public Set getAllowedExtensions() { * rather than validated against a partially-resolved policy, so a broken expression cannot * silently relax validation. *

- * Any param name is recorded, {@code disabled} included. An unresolvable - * {@code ${...}} therefore leaves the interceptor enabled (the - * flag keeps its configured value) and marks this policy unusable, so every upload in - * that invocation is rejected. That is deliberate — a broken dispatch flag is a broken - * configuration, and refusing the upload is the safe reading — but it means a typo in - * {@code disabled} takes out upload validation rather than only the disabling. + * {@link DisableParams#DISABLED_PARAM} is the one exception and is not recorded. It is not a + * validation dimension: its unresolved value is simply {@code false}, which leaves the + * interceptor running and every other part of the policy intact, so it cannot relax validation. + * Recording it would let a broken {@code ${...}} reject every + * upload of the invocation, which is a failure mode of its own rather than a safe default. */ @Override public void unresolved(String paramName) { + if (DISABLED_PARAM.equals(paramName)) { + return; + } unresolvedParams.add(paramName); } diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java index abdc127631..d628a976d4 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -1224,4 +1224,33 @@ public void testUploadPolicyTracksUnresolvedParams() { assertThat(policy.getUnresolvedParams()).containsExactly("allowedTypes"); } + /** + * An unresolvable {@code disabled} cannot relax validation - its absent value is {@code false}, + * so the interceptor simply runs - and must not take the whole policy down with it. + */ + public void testUnresolvedDisabledDoesNotMakeThePolicyUnusable() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes("text/plain"); + + policy.unresolved(DisableParams.DISABLED_PARAM); + + assertThat(policy.isUnresolved()).isFalse(); + assertThat(policy.getUnresolvedParams()).isEmpty(); + assertThat(policy.isDisabled()).isFalse(); + assertThat(policy.getAllowedTypes()).containsExactly("text/plain"); + } + + /** + * ...but a validation dimension failing alongside it still does. + */ + public void testUnresolvedDisabledDoesNotMaskOtherUnresolvedParams() { + UploadPolicy policy = new UploadPolicy(); + + policy.unresolved(DisableParams.DISABLED_PARAM); + policy.unresolved("maximumSize"); + + assertThat(policy.isUnresolved()).isTrue(); + assertThat(policy.getUnresolvedParams()).containsExactly("maximumSize"); + } + } From 2be4f6bbc3f7e268dabbb07efa6e58e268cb10f0 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 13:00:59 +0200 Subject: [PATCH 17/23] WW-5659 docs(core): state what the lazy param injector actually did The two WARN messages in resolveInto claimed consequences the injector does not control. The ReflectionException branch said the params were 'marked unusable', but InterceptorParams.unresolved is a defaulted no-op, so only a holder that overrides it - UploadPolicy does - degrades at all. The unresolved-expression branch said the configured value was kept, which reads as a sensible fallback when it is normally the unevaluated ${...} literal applied at build time. Both now report only the injector's own actions: the value was not written and the holder was notified. The nuance about what the holder retains moves to the javadoc, where there is room to state it accurately. Co-Authored-By: Claude Opus 5 --- .../struts2/interceptor/InterceptorParams.java | 14 ++++++++++---- .../struts2/interceptor/WithLazyParams.java | 16 ++++++++++++---- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java b/core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java index 68cf3e2bae..2dc358086c 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java +++ b/core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java @@ -30,13 +30,19 @@ public interface InterceptorParams { /** - * Called when a {@code ${...}} parameter could not be resolved for the current invocation. - * The framework skips the write, leaving the seeded configuration value in place, and - * notifies the holder so it can decide how to degrade. + * Called when a configured parameter could not be applied for the current invocation - either a + * {@code ${...}} expression that did not resolve, or a value the holder's setter could not + * accept. The framework skips the write and notifies the holder so it can decide how to degrade. + *

+ * Whatever the holder was seeded with stays in place, but it is rarely a usable default: the + * seed comes from applying the raw configuration string at build time, so for a {@code ${...}} + * param it is the unevaluated literal, or nothing at all when that literal could not be + * converted to the property's type. A holder guarding a security-sensitive dimension should + * therefore treat this as a reason to fail closed rather than carry on. *

* The default implementation does nothing. * - * @param paramName name of the parameter that could not be resolved + * @param paramName name of the parameter that could not be applied */ default void unresolved(String paramName) { } 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 64cea04996..9a37fec69b 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java +++ b/core/src/main/java/org/apache/struts2/interceptor/WithLazyParams.java @@ -45,7 +45,9 @@ *

* The {@link Interceptor#init()} method is called after initial parameter setting, so interceptors * can rely on configured values during initialization. Expression parameters (containing ${...}) - * are re-evaluated at invocation time via {@link LazyParamInjector}. + * are re-evaluated at invocation time via {@link LazyParamInjector} and written into a fresh + * {@link InterceptorParams} holder, never back onto the interceptor, which is shared across + * requests and stays untouched after {@link Interceptor#init()}. * * @since 2.5.9 */ @@ -108,7 +110,13 @@ public void setOgnlUtil(OgnlUtil ognlUtil) { * {@code Long} property, which OGNL reports as a * {@link ReflectionException} during conversion * - * In both cases the holder keeps its seeded configuration value and a WARN is logged. + * In both cases the injector does nothing beyond skipping the write, notifying the holder and + * logging a WARN; what that means for the invocation is the holder's decision, since + * {@code unresolved} is a no-op by default. Whatever the holder was seeded with is left in + * place, but for a {@code ${...}} param that is not a usable default: the seed comes from + * applying the raw configuration string at build time, so it is either the unevaluated + * {@code ${...}} literal or, when the literal could not be converted to the property's type, + * nothing at all. *

* 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 @@ -124,7 +132,7 @@ public

P resolveInto(P target, Map 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", + LOG.warn("Param [{}] of [{}] could not be resolved from expression [{}] - the value was not written and the params holder was notified", paramName, target.getClass().getName(), rawValue); target.unresolved(paramName); continue; @@ -134,7 +142,7 @@ public

P resolveInto(P target, Map // 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", + LOG.warn("Param [{}] could not be applied to [{}] - the value was not written and the params holder was notified; check the interceptor configuration", paramName, target.getClass().getName(), e); target.unresolved(paramName); } From 1b709ec1928a32328138a98bc04d456e28fecd20 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 13:05:04 +0200 Subject: [PATCH 18/23] WW-5659 feat(core): reject unknown lazy interceptor params at configuration time A param name that no property on the params holder can accept was only noticed per request: resolveInto caught the ReflectionException, warned, and notified the holder - which for UploadPolicy means rejecting every upload of every request behind a WARN. The names are fully known when the configuration is parsed, so DefaultInterceptorFactory now fails with a ConfigurationException naming the interceptor, the param and the holder type. Only the interceptor-ref params are checked. InterceptorBuilder passes that same map on to the InterceptorMapping, and DefaultActionInvocation.mergedParams feeds it to resolveInto, so it is exactly the set that reaches the holder. Params on the definition are applied to the interceptor instance and never reach the mapping; checking them too would reject working config, on a definition being the obvious case. The runtime handling stays as defence in depth. ConfigurationException is now rethrown rather than swallowed by the generic catch, so the operator reads the param name instead of "Caught Exception while registering Interceptor class". Co-Authored-By: Claude Opus 5 --- .../factory/DefaultInterceptorFactory.java | 78 ++++++++- .../DefaultInterceptorFactoryTest.java | 149 ++++++++++++++++++ .../struts2/mock/MockLazyInterceptor.java | 14 ++ ...5659-lazy-params-request-scoping-design.md | 17 +- 4 files changed, 252 insertions(+), 6 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java diff --git a/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java b/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java index ee1a3d0155..13157fbcfa 100644 --- a/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java +++ b/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java @@ -29,8 +29,12 @@ import org.apache.struts2.interceptor.InterceptorParams; import org.apache.struts2.interceptor.WithLazyParams; import org.apache.struts2.ognl.ProviderAllowlist; +import org.apache.struts2.util.reflection.ReflectionException; import org.apache.struts2.util.reflection.ReflectionProvider; +import java.beans.IntrospectionException; +import java.lang.reflect.Field; +import java.lang.reflect.Modifier; import java.util.HashMap; import java.util.Map; import java.util.Set; @@ -86,10 +90,15 @@ public Interceptor buildInterceptor(InterceptorConfig interceptorConfig, Map lazyInterceptor) { LOG.debug("Interceptor {} implements {} - expression parameters will be re-evaluated during action invocation", interceptorClassName, WithLazyParams.class.getName()); - allowlistLazyParamsHolder(lazyInterceptor); + InterceptorParams lazyParams = lazyInterceptor.newLazyParams(); + validateLazyParamNames(interceptorConfig, lazyParams, interceptorRefParams); + allowlistLazyParamsHolder(lazyInterceptor, lazyParams); } return interceptor; + } catch (ConfigurationException e) { + // already carries its own message and location, don't bury it in a generic wrapper + throw e; } catch (InstantiationException e) { cause = e; message = "Unable to instantiate an instance of Interceptor class [" + interceptorClassName + "]."; @@ -110,6 +119,70 @@ public Interceptor buildInterceptor(InterceptorConfig interceptorConfig, Map + * Only the interceptor-ref params are checked, and that is exactly the set that reaches + * the holder. {@link org.apache.struts2.config.providers.InterceptorBuilder} passes the + * very map it hands to this factory on to the {@code InterceptorMapping} it creates, and + * {@code DefaultActionInvocation.mergedParams} feeds that map to + * {@link WithLazyParams.LazyParamInjector#resolveInto}. The params declared on the + * {@code } definition are deliberately not checked: they never reach the mapping, + * they are only applied to the interceptor instance a few lines above, and requiring them to + * exist on the holder too would reject working configurations - {@code } + * on an interceptor definition, for instance, is honoured through + * {@link org.apache.struts2.interceptor.AbstractInterceptor#shouldIntercept} alone. + *

+ * Without this check an unknown name is only noticed per request, where + * {@code resolveInto} logs a WARN and notifies the holder - which for + * {@link org.apache.struts2.interceptor.UploadPolicy} means rejecting every upload of every + * request. The names are fully known at configuration-parse time, so a typo belongs to startup. + * The runtime handling stays in place as defence in depth for holders built outside this factory. + *

+ * A param whose name is a compound OGNL expression (it contains a {@code .}) is not checked: + * only its leading segment would have to be readable rather than writable on the holder, which + * this simple property check cannot decide. Such names are left to the runtime path. + */ + private void validateLazyParamNames(InterceptorConfig interceptorConfig, InterceptorParams lazyParams, Map interceptorRefParams) { + if (lazyParams == null || interceptorRefParams == null || interceptorRefParams.isEmpty()) { + return; + } + Class holderClass = lazyParams.getClass(); + for (String paramName : interceptorRefParams.keySet()) { + if (paramName == null || paramName.indexOf('.') >= 0) { + LOG.debug("Skipping configuration-time check of compound lazy param name [{}] on holder [{}]", + paramName, holderClass.getName()); + continue; + } + if (!isWritableOnHolder(holderClass, paramName)) { + throw new ConfigurationException(String.format( + "Param [%s] of interceptor [%s] (%s) is not a writable property of its lazy params holder [%s]." + + " Params of a %s interceptor-ref are resolved onto that holder for each invocation," + + " so this one could never be applied - check the interceptor configuration for a typo.", + paramName, interceptorConfig.getName(), interceptorConfig.getClassName(), + holderClass.getName(), WithLazyParams.class.getSimpleName()), interceptorConfig); + } + } + } + + /** + * @return true when OGNL could write {@code paramName} on the holder, i.e. there is a setter for + * it anywhere in the holder's hierarchy, or failing that a public field of that name + */ + private boolean isWritableOnHolder(Class holderClass, String paramName) { + try { + if (reflectionProvider.getSetMethod(holderClass, paramName) != null) { + return true; + } + } catch (IntrospectionException | ReflectionException e) { + LOG.debug("Could not introspect setter [{}] on lazy params holder [{}], falling back to field lookup", + paramName, holderClass.getName(), e); + } + Field field = reflectionProvider.getField(holderClass, paramName); + return field != null && Modifier.isPublic(field.getModifiers()); + } + /** * Allowlists the params holder of a {@link WithLazyParams} interceptor for OGNL member access. *

@@ -132,13 +205,12 @@ public Interceptor buildInterceptor(InterceptorConfig interceptorConfig, Map lazyInterceptor) { + private void allowlistLazyParamsHolder(WithLazyParams lazyInterceptor, InterceptorParams lazyParams) { if (providerAllowlist == null) { LOG.warn("No ProviderAllowlist available, cannot allowlist the lazy params holder of [{}];" + " lazy params will fail to resolve if the OGNL allowlist is enabled", lazyInterceptor.getClass().getName()); return; } - InterceptorParams lazyParams = lazyInterceptor.newLazyParams(); if (lazyParams == null) { LOG.warn("Interceptor [{}] returned no lazy params holder, nothing to allowlist", lazyInterceptor.getClass().getName()); return; diff --git a/core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java b/core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java new file mode 100644 index 0000000000..b430255e0e --- /dev/null +++ b/core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java @@ -0,0 +1,149 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.factory; + +import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.config.ConfigurationException; +import org.apache.struts2.config.entities.InterceptorConfig; +import org.apache.struts2.interceptor.ActionFileUploadInterceptor; +import org.apache.struts2.interceptor.Interceptor; +import org.apache.struts2.interceptor.UploadPolicy; +import org.apache.struts2.mock.MockInterceptor; +import org.apache.struts2.mock.MockLazyInterceptor; + +import java.util.LinkedHashMap; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Covers the configuration-time validation of {@code WithLazyParams} interceptor params (WW-5659). + *

+ * Params of a lazy interceptor-ref are resolved onto a per-invocation params holder, so a name the + * holder cannot accept can never be applied. Before this check that was only noticed per request, + * where it costs a WARN and - for {@link UploadPolicy} - the rejection of every upload. The names + * are known when the configuration is parsed, so it fails there instead. + */ +public class DefaultInterceptorFactoryTest extends StrutsInternalTestCase { + + private InterceptorFactory factory; + + @Override + protected void setUp() throws Exception { + super.setUp(); + factory = container.getInstance(InterceptorFactory.class); + } + + private static Map params(String... keysAndValues) { + Map params = new LinkedHashMap<>(); + for (int i = 0; i < keysAndValues.length; i += 2) { + params.put(keysAndValues[i], keysAndValues[i + 1]); + } + return params; + } + + private static InterceptorConfig config(String name, Class clazz) { + return new InterceptorConfig.Builder(name, clazz.getName()).build(); + } + + public void testRefParamsWritableOnTheHolderAreAccepted() { + InterceptorConfig config = config("actionFileUpload", ActionFileUploadInterceptor.class); + + assertThatCode(() -> factory.buildInterceptor(config, params( + "allowedTypes", "${uploadConfig.allowedMimeTypes}", + "allowedExtensions", "${uploadConfig.allowedExtensions}", + "maximumSize", "${uploadConfig.maxFileSize}", + "disabled", "${uploadConfig.skip}"))).doesNotThrowAnyException(); + } + + public void testUnknownRefParamFailsConfiguration() { + InterceptorConfig config = config("actionFileUpload", ActionFileUploadInterceptor.class); + + assertThatThrownBy(() -> factory.buildInterceptor(config, params("maximumSizes", "1024"))) + .isInstanceOf(ConfigurationException.class) + .hasMessageContaining("maximumSizes") + .hasMessageContaining("actionFileUpload") + .hasMessageContaining(ActionFileUploadInterceptor.class.getName()) + .hasMessageContaining(UploadPolicy.class.getName()); + } + + /** + * The failure must not be buried by the factory's generic {@code catch (Exception)} wrapper, + * otherwise the operator reads "Caught Exception while registering Interceptor class" and has to + * dig through the cause chain to find the param name. + */ + public void testUnknownRefParamFailureIsNotRewrapped() { + InterceptorConfig config = config("actionFileUpload", ActionFileUploadInterceptor.class); + + assertThatThrownBy(() -> factory.buildInterceptor(config, params("nope", "x"))) + .isInstanceOf(ConfigurationException.class) + .hasNoCause(); + } + + /** + * Params declared on the {@code } definition are applied to the interceptor and + * never reach the {@code InterceptorMapping}, so they never reach the holder either. Validating + * them would reject working configurations. + */ + public void testInterceptorDefinitionParamsAreNotValidatedAgainstTheHolder() { + InterceptorConfig config = new InterceptorConfig.Builder("lazy", MockLazyInterceptor.class.getName()) + .addParam("interceptorOnly", "applied at build time") + .build(); + + Interceptor interceptor = factory.buildInterceptor(config, params()); + + assertThat(interceptor).isInstanceOf(MockLazyInterceptor.class); + assertThat(((MockLazyInterceptor) interceptor).getInterceptorOnly()).isEqualTo("applied at build time"); + } + + /** + * Same property, this time on the interceptor-ref: now it does reach the holder, and the holder + * has no such property. + */ + public void testSameParamOnTheRefIsValidated() { + InterceptorConfig config = config("lazy", MockLazyInterceptor.class); + + assertThatThrownBy(() -> factory.buildInterceptor(config, params("interceptorOnly", "x"))) + .isInstanceOf(ConfigurationException.class) + .hasMessageContaining("interceptorOnly") + .hasMessageContaining(MockLazyInterceptor.MockLazyParams.class.getName()); + } + + /** + * A compound name is an OGNL navigation expression, not a property of the holder, so this simple + * check cannot decide it and leaves it to the runtime path. + */ + public void testCompoundParamNameIsNotValidated() { + InterceptorConfig config = config("lazy", MockLazyInterceptor.class); + + assertThatCode(() -> factory.buildInterceptor(config, params("nested.foo", "x"))).doesNotThrowAnyException(); + } + + /** + * Non-lazy interceptors keep the lenient behaviour: their params go straight onto the instance + * and an unknown one is ignored, which is long-standing and out of scope here. + */ + public void testNonLazyInterceptorRefParamsAreNotValidated() { + InterceptorConfig config = config("test", MockInterceptor.class); + + assertThatCode(() -> factory.buildInterceptor(config, params("noSuchProperty", "x"))).doesNotThrowAnyException(); + } +} diff --git a/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java b/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java index f875c043cc..eda9c6e93b 100644 --- a/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java +++ b/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java @@ -54,6 +54,20 @@ public String getBar() { private String foo = ""; private String bar = ""; + private String interceptorOnly = ""; + + /** + * A property of the interceptor with no counterpart on {@link MockLazyParams}. Configuring it on + * the {@code } definition is legitimate - definition params are applied to the + * interceptor and never reach the holder - so the configuration-time check must not reject it. + */ + public void setInterceptorOnly(String interceptorOnly) { + this.interceptorOnly = interceptorOnly; + } + + public String getInterceptorOnly() { + return interceptorOnly; + } public void setFoo(String foo) { this.foo = foo; 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 221dff23fe..9f2566b976 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 @@ -304,9 +304,14 @@ exists to remove. There are three such paths: 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. + (`NoSuchPropertyException`) from a typo'd param name. This is now rejected at configuration + time: `DefaultInterceptorFactory` checks every *interceptor-ref* param of a `WithLazyParams` + interceptor against its holder type and throws `ConfigurationException` if it is not writable + there. Only the ref params are checked, because only those become + `InterceptorMapping#getParams()` and reach `resolveInto`; params on the `` + definition are applied to the interceptor instance and never reach the holder, so requiring + them on the holder would reject working configurations. The runtime notification stays as + defence in depth for holders and mappings built outside the factory. A new failure mode added to `resolveInto` later must be checked against this rule. @@ -328,6 +333,12 @@ This is a behaviour change for 7.2.x applications with a broken expression. Thos applications are currently running with that validation silently disabled, which is the reason to surface it. +The configuration-time param check is a behaviour change of its own and needs a release note: +an application carrying a latent typo in a `` param of a `WithLazyParams` +interceptor now fails to start instead of failing every affected request. That is the point — +the alternative is an outage that only shows up under upload traffic — but it is a startup +failure where there was none. + ## Testing `ActionFileUploadInterceptorTest` is JUnit 3 style — `protected void setUp()`, plain From abe677dad792345d28fa16350ba0aac34af9df44 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 13:06:26 +0200 Subject: [PATCH 19/23] WW-5659 refactor(core): drop the deprecated single-arg executeConditional The overload lost its last caller when the mapping name became available at the call site, so an existing subclass override would have compiled and then never run again - silently dead code, worse than a compile error. This branch already changes the protected acceptFile signature, so keeping the one-arg form for source compatibility was not consistent either. Covered by a test asserting the surviving two-arg form is the extension point and receives the mapping name. Co-Authored-By: Claude Opus 5 --- .../struts2/DefaultActionInvocation.java | 13 +++---- .../struts2/DefaultActionInvocationTest.java | 35 +++++++++++++++++++ 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java b/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java index 72a35afe5b..2ba0ad22f0 100644 --- a/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java +++ b/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java @@ -353,15 +353,10 @@ private Map mergedParams(InterceptorMapping interceptorMapping) } /** - * @deprecated since 7.3.0, use {@link #executeConditional(ConditionalInterceptor, String)} so the - * interceptor is identified by its mapping name in the logs. - */ - @Deprecated - protected String executeConditional(ConditionalInterceptor conditionalInterceptor) throws Exception { - return executeConditional(conditionalInterceptor, conditionalInterceptor.getClass().getSimpleName()); - } - - /** + * Replaces the single-argument form removed in 7.3.0. That one had no callers left once the + * mapping name became available here, so a subclass still overriding it would have gone quietly + * dead; removing it turns that into a compile error instead. + * * @param interceptorName the name of the interceptor mapping being invoked, used for logging * @since 7.3.0 */ diff --git a/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java b/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java index 398fa7f77b..6c355860a4 100644 --- a/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java +++ b/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java @@ -24,6 +24,7 @@ import org.apache.struts2.config.entities.ResultConfig; import org.apache.struts2.config.providers.XmlConfigurationProvider; import org.apache.struts2.dispatcher.HttpParameters; +import org.apache.struts2.interceptor.ConditionalInterceptor; import org.apache.struts2.interceptor.Interceptor; import org.apache.struts2.interceptor.WithLazyParams; import org.apache.struts2.mock.MockActionProxy; @@ -161,6 +162,40 @@ public void testInvokeWithDisabledInterceptors() throws Exception { assertTrue(defaultActionInvocation.isExecuted()); } + /** + * WW-5659: {@code executeConditional(ConditionalInterceptor)} was removed in 7.3.0 - it had no + * callers left, so a subclass still overriding it would have gone silently dead. The + * two-argument form is now the only extension point and must be handed the interceptor + * mapping's name, which is what the removed overload could not supply. + */ + public void testExecuteConditionalOverrideReceivesTheMappingName() throws Exception { + // given + List interceptorMappings = new ArrayList<>(); + MockInterceptor mockInterceptor = new MockInterceptor(); + mockInterceptor.setFoo("test1"); + mockInterceptor.setExpectedFoo("test1"); + interceptorMappings.add(new InterceptorMapping("namedInStack", mockInterceptor)); + + List observedNames = new ArrayList<>(); + DefaultActionInvocation defaultActionInvocation = new DefaultActionInvocationTester(interceptorMappings) { + @Override + protected String executeConditional(ConditionalInterceptor conditionalInterceptor, String interceptorName) throws Exception { + observedNames.add(interceptorName); + return super.executeConditional(conditionalInterceptor, interceptorName); + } + }; + container.inject(defaultActionInvocation); + defaultActionInvocation.stack = container.getInstance(ValueStackFactory.class).createValueStack(); + + // when + defaultActionInvocation.setResultCode(""); + defaultActionInvocation.invoke(); + + // then + assertThat(observedNames).containsExactly("namedInStack"); + assertThat(mockInterceptor.isExecuted()).isTrue(); + } + public void testInvokingExistingExecuteMethod() throws Exception { // given DefaultActionInvocation dai = new DefaultActionInvocation(ActionContext.getContext().getContextMap(), false); From 48aeac824bea0957a6f58a4688467d0d26c44f7f Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 13:07:01 +0200 Subject: [PATCH 20/23] WW-5659 fix(core): keep configuration order when merging lazy interceptor params mergedParams built a HashMap, so the order the configuration carries was discarded and the order params were applied to the per-invocation holder was whatever hashing produced. LinkedHashMap makes it deterministic and matches the LinkedHashMap the InterceptorBuilder already assembles. Co-Authored-By: Claude Opus 5 --- .../java/org/apache/struts2/DefaultActionInvocation.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java b/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java index 2ba0ad22f0..0a671648a5 100644 --- a/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java +++ b/core/src/main/java/org/apache/struts2/DefaultActionInvocation.java @@ -42,8 +42,8 @@ import org.apache.struts2.util.ValueStackFactory; import java.util.ArrayList; -import java.util.HashMap; import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.concurrent.Callable; @@ -341,10 +341,11 @@ private

String invokeWithLazyParams( * twice with different params it merges the first mapping's params over the current one, which * is questionable. Changing it is out of scope here. * - * @return a fresh map; the mapping's own param map is shared across requests and must not be mutated + * @return a fresh map preserving the configuration order, so params are applied to the holder + * deterministically; the mapping's own param map is shared across requests and must not be mutated */ private Map mergedParams(InterceptorMapping interceptorMapping) { - Map merged = new HashMap<>(interceptorMapping.getParams()); + Map merged = new LinkedHashMap<>(interceptorMapping.getParams()); proxy.getConfig().getInterceptors().stream() .filter(im -> im.getName().equals(interceptorMapping.getName())) .findFirst() From f2a54ff6f6f92ef4e7bbaec7d2c3c7caa4cb8e21 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 13:33:34 +0200 Subject: [PATCH 21/23] WW-5659 fix(core): keep interceptor params serializable Interceptor extends Serializable, so an interceptor holding its configured params as a field must hold something serializable. The fields UploadPolicy replaced were a Long and two HashSets, all serializable; the holder was not, which silently broke serialization of every file upload interceptor. Fix it on the contract rather than the field: InterceptorParams now extends Serializable, so every holder inherits the requirement. Marking the field transient would instead have dropped the configured policy on deserialization. Also renames four test locals that shadowed the interceptor field and drops a throws clause that could not be reached, both reported by SonarCloud. --- .../struts2/interceptor/DisableParams.java | 2 ++ .../interceptor/InterceptorParams.java | 8 ++++- .../struts2/interceptor/UploadPolicy.java | 2 ++ .../ActionFileUploadInterceptorTest.java | 34 +++++++++---------- 4 files changed, 28 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/DisableParams.java b/core/src/main/java/org/apache/struts2/interceptor/DisableParams.java index 860f4b520e..df016f72e7 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/DisableParams.java +++ b/core/src/main/java/org/apache/struts2/interceptor/DisableParams.java @@ -36,6 +36,8 @@ public class DisableParams implements InterceptorParams { */ public static final String DISABLED_PARAM = "disabled"; + private static final long serialVersionUID = 1L; + private boolean disabled; public DisableParams() { diff --git a/core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java b/core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java index 2dc358086c..bace8139ad 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java +++ b/core/src/main/java/org/apache/struts2/interceptor/InterceptorParams.java @@ -18,16 +18,22 @@ */ package org.apache.struts2.interceptor; +import java.io.Serializable; + /** * Contract for an object holding the parameters of a single interceptor. *

* Implementations are per-invocation value objects: the framework resolves configured * parameters into a fresh instance for each action invocation, so nothing is written back * onto the interceptor, which stays immutable after {@link Interceptor#init()}. + *

+ * Extends {@link Serializable} because an interceptor holding its configured params is itself + * {@link Interceptor serializable}; a non-serializable holder would silently break serialization + * of any interceptor that keeps one as a field. * * @since 7.3.0 */ -public interface InterceptorParams { +public interface InterceptorParams extends Serializable { /** * Called when a configured parameter could not be applied for the current invocation - either a diff --git a/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java index 50d647cde5..4d03f711a8 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java +++ b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java @@ -34,6 +34,8 @@ */ public class UploadPolicy extends DisableParams { + private static final long serialVersionUID = 1L; + private Long maximumSize; private Set allowedTypes = Collections.emptySet(); private Set allowedExtensions = Collections.emptySet(); diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java index d628a976d4..22f4484086 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -1015,25 +1015,25 @@ public void testConcurrentDynamicPoliciesStayIsolatedPerRequest() throws Excepti * singleton exactly as configured. */ public void testResolutionDoesNotMutateTheInterceptor() throws Exception { - ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); - container.inject(interceptor); - interceptor.setAllowedTypes("text/plain"); + ActionFileUploadInterceptor configuredInterceptor = new ActionFileUploadInterceptor(); + container.inject(configuredInterceptor); + configuredInterceptor.setAllowedTypes("text/plain"); MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); action.setAllowedMimeTypes("text/html"); container.inject(action); - runUploadAttempt(interceptor, action, createUploadRequest("f.html", "text/html", htmlContent)); + runUploadAttempt(configuredInterceptor, action, createUploadRequest("f.html", "text/html", htmlContent)); - assertThat(interceptor.newLazyParams().getAllowedTypes()).containsExactly("text/plain"); + assertThat(configuredInterceptor.newLazyParams().getAllowedTypes()).containsExactly("text/plain"); } /** * Regression for WW-5659: a lazily resolved {@code disabled} must apply to one invocation only. */ - public void testDisabledIsResolvedPerInvocation() throws Exception { - ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); - container.inject(interceptor); + public void testDisabledIsResolvedPerInvocation() { + ActionFileUploadInterceptor sharedInterceptor = new ActionFileUploadInterceptor(); + container.inject(sharedInterceptor); MyDynamicFileUploadAction disablingAction = new MyDynamicFileUploadAction(); disablingAction.setUploadDisabled("true"); @@ -1043,14 +1043,14 @@ public void testDisabledIsResolvedPerInvocation() throws Exception { enablingAction.setUploadDisabled("false"); container.inject(enablingAction); - UploadPolicy disabledPolicy = resolveDisabled(interceptor, disablingAction); - UploadPolicy enabledPolicy = resolveDisabled(interceptor, enablingAction); + UploadPolicy disabledPolicy = resolveDisabled(sharedInterceptor, disablingAction); + UploadPolicy enabledPolicy = resolveDisabled(sharedInterceptor, enablingAction); // the second resolution must not have cleared the first invocation's flag... assertThat(disabledPolicy.isDisabled()).isTrue(); assertThat(enabledPolicy.isDisabled()).isFalse(); // ...and neither resolution may reach the shared interceptor - assertThat(interceptor.newLazyParams().isDisabled()).isFalse(); + assertThat(sharedInterceptor.newLazyParams().isDisabled()).isFalse(); } private UploadPolicy resolveDisabled(ActionFileUploadInterceptor actionFileUploadInterceptor, @@ -1187,28 +1187,28 @@ private void awaitUnchecked(CountDownLatch latch) { } public void testUnresolvedPolicyRejectsTheUpload() throws Exception { - ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); - container.inject(interceptor); + ActionFileUploadInterceptor uploadInterceptor = new ActionFileUploadInterceptor(); + container.inject(uploadInterceptor); MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); action.setAllowedMimeTypes(null); // ${allowedMimeTypes} will not resolve container.inject(action); - runUploadAttempt(interceptor, action, createUploadRequest("f.txt", "text/plain", plainContent)); + runUploadAttempt(uploadInterceptor, action, createUploadRequest("f.txt", "text/plain", plainContent)); assertThat(action.getUploadFiles()).isNull(); assertThat(action.getFieldErrors()).containsKey("file"); } public void testResolvedPolicyStillAcceptsTheUpload() throws Exception { - ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); - container.inject(interceptor); + ActionFileUploadInterceptor uploadInterceptor = new ActionFileUploadInterceptor(); + container.inject(uploadInterceptor); MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); action.setAllowedMimeTypes("text/plain"); container.inject(action); - runUploadAttempt(interceptor, action, createUploadRequest("f.txt", "text/plain", plainContent)); + runUploadAttempt(uploadInterceptor, action, createUploadRequest("f.txt", "text/plain", plainContent)); assertThat(action.hasFieldErrors()).isFalse(); assertThat(action.getUploadFiles()).isNotNull().hasSize(1); From 4a1859f01a479e1fa42d69f5ab0faa98a1198ba5 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 15:52:22 +0200 Subject: [PATCH 22/23] WW-5659 test(core): hoist the params map out of the assertThatThrownBy lambdas Each lambda called both params(...) and buildInterceptor(...), so a throw from the helper would have satisfied the assertion just as well as one from the code under test. Building the map first leaves one throwing call per lambda. Reported by SonarCloud (java:S5778). --- .../factory/DefaultInterceptorFactoryTest.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java b/core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java index b430255e0e..a43d2819bb 100644 --- a/core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java +++ b/core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java @@ -77,7 +77,9 @@ public void testRefParamsWritableOnTheHolderAreAccepted() { public void testUnknownRefParamFailsConfiguration() { InterceptorConfig config = config("actionFileUpload", ActionFileUploadInterceptor.class); - assertThatThrownBy(() -> factory.buildInterceptor(config, params("maximumSizes", "1024"))) + Map refParams = params("maximumSizes", "1024"); + + assertThatThrownBy(() -> factory.buildInterceptor(config, refParams)) .isInstanceOf(ConfigurationException.class) .hasMessageContaining("maximumSizes") .hasMessageContaining("actionFileUpload") @@ -93,7 +95,9 @@ public void testUnknownRefParamFailsConfiguration() { public void testUnknownRefParamFailureIsNotRewrapped() { InterceptorConfig config = config("actionFileUpload", ActionFileUploadInterceptor.class); - assertThatThrownBy(() -> factory.buildInterceptor(config, params("nope", "x"))) + Map refParams = params("nope", "x"); + + assertThatThrownBy(() -> factory.buildInterceptor(config, refParams)) .isInstanceOf(ConfigurationException.class) .hasNoCause(); } @@ -121,7 +125,9 @@ public void testInterceptorDefinitionParamsAreNotValidatedAgainstTheHolder() { public void testSameParamOnTheRefIsValidated() { InterceptorConfig config = config("lazy", MockLazyInterceptor.class); - assertThatThrownBy(() -> factory.buildInterceptor(config, params("interceptorOnly", "x"))) + Map refParams = params("interceptorOnly", "x"); + + assertThatThrownBy(() -> factory.buildInterceptor(config, refParams)) .isInstanceOf(ConfigurationException.class) .hasMessageContaining("interceptorOnly") .hasMessageContaining(MockLazyInterceptor.MockLazyParams.class.getName()); From 83d2b61dda2f9ad6bab123219ff8340d76f8bad4 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 27 Jul 2026 16:08:28 +0200 Subject: [PATCH 23/23] WW-5659 fix(core): stop seeding the interceptor with raw lazy expressions A ${...} param is resolved per invocation into the params holder, so applying its raw text to the interceptor at configuration time only seeded an unevaluated literal - allowedTypes held "${uploadConfig.allowedMimeTypes}", matching no content type - or failed conversion outright for a typed property such as the Long maximumSize. Withhold those params at build time; static params still apply and still seed the holder, which is what a lazy param falling back has to fall back to. Also makes InterceptorParams.unresolved's javadoc about the retained value honest. Idea from @deprrous in GitHub PR #1815. Co-Authored-By: deprrous --- .../factory/DefaultInterceptorFactory.java | 24 +++++++++++++- .../DefaultInterceptorFactoryTest.java | 33 +++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java b/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java index 13157fbcfa..38e0ac741b 100644 --- a/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java +++ b/core/src/main/java/org/apache/struts2/factory/DefaultInterceptorFactory.java @@ -36,6 +36,7 @@ import java.lang.reflect.Field; import java.lang.reflect.Modifier; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; @@ -83,7 +84,13 @@ public Interceptor buildInterceptor(InterceptorConfig interceptorConfig, Map ? withoutLazyExpressions(params) : params, + interceptor); interceptor.init(); @@ -223,4 +230,19 @@ private void allowlistLazyParamsHolder(WithLazyParams lazyInterceptor, Interc providerAllowlist.registerAllowlist(holderClass, holderTypes); } + /** + * Drops the params carrying a {@code ${...}} expression, which {@link WithLazyParams} resolves per + * invocation into its params holder instead. + *

+ * The predicate matches {@code WithLazyParams.LazyParamInjector}'s, so a value treated as an + * expression at resolution time is the same one withheld here. + * + * @return the params to apply to the interceptor at configuration time + */ + private static Map withoutLazyExpressions(Map params) { + return params.entrySet().stream() + .filter(entry -> entry.getValue() == null || !entry.getValue().contains("${")) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, (first, second) -> second, LinkedHashMap::new)); + } + } diff --git a/core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java b/core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java index a43d2819bb..e837d41e4b 100644 --- a/core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java +++ b/core/src/test/java/org/apache/struts2/factory/DefaultInterceptorFactoryTest.java @@ -64,6 +64,39 @@ private static InterceptorConfig config(String name, Class clazz) { return new InterceptorConfig.Builder(name, clazz.getName()).build(); } + /** + * A {@code ${...}} param must not be applied to the interceptor at configuration time: the raw + * literal is meaningless as a policy value, and for the {@code Long} maximumSize it cannot even be + * converted. Idea from @deprrous in GitHub PR #1815. + */ + public void testLazyExpressionsDoNotSeedTheInterceptor() throws Exception { + InterceptorConfig config = config("actionFileUpload", ActionFileUploadInterceptor.class); + + ActionFileUploadInterceptor interceptor = (ActionFileUploadInterceptor) factory.buildInterceptor(config, params( + "allowedTypes", "${uploadConfig.allowedMimeTypes}", + "maximumSize", "${uploadConfig.maxFileSize}")); + + UploadPolicy configured = interceptor.newLazyParams(); + assertThat(configured.getAllowedTypes()).isEmpty(); + assertThat(configured.getMaximumSize()).isNull(); + } + + /** + * Static params, by contrast, are the interceptor's configured policy and must still seed it - they + * are what a per-invocation holder starts from, and what survives a lazy param failing to resolve. + */ + public void testStaticParamsStillSeedTheInterceptor() throws Exception { + InterceptorConfig config = config("actionFileUpload", ActionFileUploadInterceptor.class); + + ActionFileUploadInterceptor interceptor = (ActionFileUploadInterceptor) factory.buildInterceptor(config, params( + "allowedTypes", "text/plain,text/html", + "maximumSize", "2048")); + + UploadPolicy configured = interceptor.newLazyParams(); + assertThat(configured.getAllowedTypes()).containsExactlyInAnyOrder("text/plain", "text/html"); + assertThat(configured.getMaximumSize()).isEqualTo(2048L); + } + public void testRefParamsWritableOnTheHolderAreAccepted() { InterceptorConfig config = config("actionFileUpload", ActionFileUploadInterceptor.class);