This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5631-chaining-require-annotations in repository https://gitbox.apache.org/repos/asf/struts.git
commit 5560633d378496f102e4711dc4afb86caa484698 Author: Lukasz Lenart <[email protected]> AuthorDate: Wed May 27 08:13:23 2026 +0200 WW-5631 docs(chaining): add design spec and implementation plan Opt-in @StrutsParameter enforcement for ChainingInterceptor (struts.chaining.requireAnnotations, default false). Co-Authored-By: Claude Opus 4.7 <[email protected]> --- ...ning-interceptor-strutsparameter-enforcement.md | 669 +++++++++++++++++++++ ...terceptor-strutsparameter-enforcement-design.md | 164 +++++ 2 files changed, 833 insertions(+) diff --git a/docs/superpowers/plans/2026-05-27-WW-5631-chaining-interceptor-strutsparameter-enforcement.md b/docs/superpowers/plans/2026-05-27-WW-5631-chaining-interceptor-strutsparameter-enforcement.md new file mode 100644 index 000000000..8ee363e79 --- /dev/null +++ b/docs/superpowers/plans/2026-05-27-WW-5631-chaining-interceptor-strutsparameter-enforcement.md @@ -0,0 +1,669 @@ +# WW-5631 ChainingInterceptor opt-in @StrutsParameter enforcement — 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:** Add an opt-in flag `struts.chaining.requireAnnotations` (default `false`) that makes `ChainingInterceptor` only copy a property to the target action when that property is authorized by `@StrutsParameter`, reusing the existing `ParameterAuthorizer`. + +**Architecture:** When the flag is on, before each `reflectionProvider.copy(...)` call, `ChainingInterceptor` introspects the target action's writable properties via `OgnlUtil.getBeanInfo`, asks `ParameterAuthorizer.isAuthorized(name, action, action)` for each, and adds rejected names to the excludes set passed to `copy`. If the target cannot be introspected, it fails closed (skips the copy for that object). When the flag is off, behavior and cost are unchanged. + +**Tech Stack:** Java, Struts container DI (`@Inject`), OGNL (`OgnlUtil`), JUnit 3-style `XWorkTestCase` + `com.mockobjects.dynamic.Mock` (matching the existing test file). + +**Reference spec:** `docs/superpowers/specs/2026-05-27-chaining-interceptor-strutsparameter-enforcement-design.md` + +--- + +## File Structure + +- **Modify** `core/src/main/java/org/apache/struts2/StrutsConstants.java` — add `STRUTS_CHAINING_REQUIRE_ANNOTATIONS` constant. +- **Modify** `core/src/main/resources/org/apache/struts2/default.properties` — add `struts.chaining.requireAnnotations=false`. +- **Modify** `core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java` — inject `ParameterAuthorizer`, `OgnlUtil`, and the new flag; add gating logic; update JavaDoc. +- **Create** `core/src/test/java/org/apache/struts2/interceptor/AnnotatedChainingAction.java` — test fixture: action with an `@StrutsParameter`-annotated `managerApproved` property. +- **Create** `core/src/test/java/org/apache/struts2/interceptor/UnannotatedChainingAction.java` — test fixture: action with an unannotated `managerApproved` property. +- **Modify** `core/src/test/java/org/apache/struts2/interceptor/ChainingInterceptorTest.java` — add enforcement tests. + +--- + +## Task 1: Add the configuration constant + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/StrutsConstants.java:734-736` + +- [ ] **Step 1: Add the constant next to the existing chaining constants** + +In `StrutsConstants.java`, the existing block is: + +```java + public static final String STRUTS_CHAINING_COPY_ERRORS = "struts.chaining.copyErrors"; + public static final String STRUTS_CHAINING_COPY_FIELD_ERRORS = "struts.chaining.copyFieldErrors"; + public static final String STRUTS_CHAINING_COPY_MESSAGES = "struts.chaining.copyMessages"; +``` + +Add a fourth line immediately after `STRUTS_CHAINING_COPY_MESSAGES`: + +```java + public static final String STRUTS_CHAINING_COPY_ERRORS = "struts.chaining.copyErrors"; + public static final String STRUTS_CHAINING_COPY_FIELD_ERRORS = "struts.chaining.copyFieldErrors"; + public static final String STRUTS_CHAINING_COPY_MESSAGES = "struts.chaining.copyMessages"; + public static final String STRUTS_CHAINING_REQUIRE_ANNOTATIONS = "struts.chaining.requireAnnotations"; +``` + +- [ ] **Step 2: Compile to verify the constant is valid** + +Run: `mvn -q -pl core compile -DskipAssembly` +Expected: BUILD SUCCESS (no compilation errors). + +- [ ] **Step 3: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/StrutsConstants.java +git commit -m "WW-5631 feat(chaining): add struts.chaining.requireAnnotations constant" +``` + +--- + +## Task 2: Add the default property (off by default) + +**Files:** +- Modify: `core/src/main/resources/org/apache/struts2/default.properties:258` + +- [ ] **Step 1: Add the default entry after the transition-mode line** + +The existing block ends at: + +```properties +### Whether to drop @StrutsParameter annotation requirement on simple setter methods +### Useful for transitioning legacy applications, but highly recommended to set to false as soon as possible! +struts.parameters.requireAnnotations.transitionMode=false +``` + +Add immediately after `struts.parameters.requireAnnotations.transitionMode=false`: + +```properties +struts.parameters.requireAnnotations.transitionMode=false + +### Whether ChainingInterceptor enforces @StrutsParameter on the target action when copying properties. +### Opt-in hardening; default false preserves legacy chaining behaviour. Only has effect when +### struts.parameters.requireAnnotations is also enabled. +struts.chaining.requireAnnotations=false +``` + +- [ ] **Step 2: Verify the file parses (build picks up resources)** + +Run: `mvn -q -pl core process-resources -DskipAssembly` +Expected: BUILD SUCCESS. + +- [ ] **Step 3: Commit** + +```bash +git add core/src/main/resources/org/apache/struts2/default.properties +git commit -m "WW-5631 feat(chaining): default struts.chaining.requireAnnotations=false" +``` + +--- + +## Task 3: Add test fixture action classes + +These give the tests an annotated and an unannotated target. Both expose the same property so the +only difference is the annotation. + +**Files:** +- Create: `core/src/test/java/org/apache/struts2/interceptor/AnnotatedChainingAction.java` +- Create: `core/src/test/java/org/apache/struts2/interceptor/UnannotatedChainingAction.java` + +- [ ] **Step 1: Create the annotated fixture** + +Create `core/src/test/java/org/apache/struts2/interceptor/AnnotatedChainingAction.java`: + +```java +/* + * 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.action.Action; +import org.apache.struts2.interceptor.parameter.StrutsParameter; + +/** + * Test fixture: target/source action whose {@code managerApproved} property is annotated with + * {@link StrutsParameter}. Used by {@link ChainingInterceptorTest}. + */ +public class AnnotatedChainingAction implements Action { + + private boolean managerApproved; + + public boolean getManagerApproved() { + return managerApproved; + } + + @StrutsParameter + public void setManagerApproved(boolean managerApproved) { + this.managerApproved = managerApproved; + } + + @Override + public String execute() { + return SUCCESS; + } +} +``` + +- [ ] **Step 2: Create the unannotated fixture** + +Create `core/src/test/java/org/apache/struts2/interceptor/UnannotatedChainingAction.java`: + +```java +/* + * 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.action.Action; + +/** + * Test fixture: target action whose {@code managerApproved} property is NOT annotated with + * {@code @StrutsParameter}. Used by {@link ChainingInterceptorTest}. + */ +public class UnannotatedChainingAction implements Action { + + private boolean managerApproved; + + public boolean getManagerApproved() { + return managerApproved; + } + + public void setManagerApproved(boolean managerApproved) { + this.managerApproved = managerApproved; + } + + @Override + public String execute() { + return SUCCESS; + } +} +``` + +- [ ] **Step 3: Compile test sources to verify fixtures** + +Run: `mvn -q -pl core test-compile -DskipAssembly` +Expected: BUILD SUCCESS. + +- [ ] **Step 4: Commit** + +```bash +git add core/src/test/java/org/apache/struts2/interceptor/AnnotatedChainingAction.java \ + core/src/test/java/org/apache/struts2/interceptor/UnannotatedChainingAction.java +git commit -m "WW-5631 test(chaining): add annotated/unannotated chaining fixtures" +``` + +--- + +## Task 4: Write the failing enforcement tests + +We add the tests before the implementation. They will fail to compile (new setters/behavior absent) +until Task 5. The tests build a real `StrutsParameterAuthorizer` so annotation semantics are exercised +faithfully. + +**Files:** +- Modify: `core/src/test/java/org/apache/struts2/interceptor/ChainingInterceptorTest.java` + +- [ ] **Step 1: Add imports** + +At the top of `ChainingInterceptorTest.java`, after the existing +`import org.apache.struts2.util.ValueStack;` line, add: + +```java +import org.apache.struts2.interceptor.parameter.StrutsParameterAuthorizer; +import org.apache.struts2.ognl.OgnlUtil; +import org.apache.struts2.util.ProxyService; +``` + +- [ ] **Step 2: Add a helper to build an authorizer with chosen flags** + +Add this private helper method inside the `ChainingInterceptorTest` class (e.g. just above the +`setUp()` method at line 152): + +```java + private StrutsParameterAuthorizer buildAuthorizer(boolean requireAnnotations, boolean transitionMode) { + StrutsParameterAuthorizer authorizer = new StrutsParameterAuthorizer(); + authorizer.setOgnlUtil(container.getInstance(OgnlUtil.class)); + authorizer.setProxyService(container.getInstance(ProxyService.class)); + authorizer.setRequireAnnotations(String.valueOf(requireAnnotations)); + authorizer.setRequireAnnotationsTransitionMode(String.valueOf(transitionMode)); + return authorizer; + } + + private void enableChainingEnforcement(boolean requireAnnotations, boolean transitionMode) { + interceptor.setParameterAuthorizer(buildAuthorizer(requireAnnotations, transitionMode)); + interceptor.setRequireAnnotations("true"); + } +``` + +- [ ] **Step 3: Add the test methods** + +Add the following test methods inside the `ChainingInterceptorTest` class (e.g. after +`testActionErrorsCanBeAddedAfterChain`): + +```java + public void testFlagOffCopiesUnannotatedProperty() throws Exception { + AnnotatedChainingAction source = new AnnotatedChainingAction(); + source.setManagerApproved(true); + UnannotatedChainingAction target = new UnannotatedChainingAction(); + mockInvocation.matchAndReturn("getAction", target); + stack.push(source); + stack.push(target); + + // requireAnnotations flag defaults to false -> legacy behaviour + interceptor.intercept(invocation); + + assertTrue("legacy chaining should copy the property when flag is off", target.getManagerApproved()); + } + + public void testFlagOnSkipsUnannotatedProperty() throws Exception { + AnnotatedChainingAction source = new AnnotatedChainingAction(); + source.setManagerApproved(true); + UnannotatedChainingAction target = new UnannotatedChainingAction(); + mockInvocation.matchAndReturn("getAction", target); + stack.push(source); + stack.push(target); + + enableChainingEnforcement(true, false); + interceptor.intercept(invocation); + + assertFalse("unannotated target property must NOT be copied when enforcement is on", + target.getManagerApproved()); + } + + public void testFlagOnCopiesAnnotatedProperty() throws Exception { + AnnotatedChainingAction source = new AnnotatedChainingAction(); + source.setManagerApproved(true); + AnnotatedChainingAction target = new AnnotatedChainingAction(); + mockInvocation.matchAndReturn("getAction", target); + stack.push(source); + stack.push(target); + + enableChainingEnforcement(true, false); + interceptor.intercept(invocation); + + assertTrue("annotated target property should be copied when enforcement is on", + target.getManagerApproved()); + } + + public void testTransitionModeCopiesNonNestedUnannotatedProperty() throws Exception { + AnnotatedChainingAction source = new AnnotatedChainingAction(); + source.setManagerApproved(true); + UnannotatedChainingAction target = new UnannotatedChainingAction(); + mockInvocation.matchAndReturn("getAction", target); + stack.push(source); + stack.push(target); + + // transition mode exempts depth-0 (non-nested) properties from the annotation requirement + enableChainingEnforcement(true, true); + interceptor.intercept(invocation); + + assertTrue("transition mode should copy depth-0 property without annotation", + target.getManagerApproved()); + } + + public void testRequireAnnotationsFalseIsNoOp() throws Exception { + AnnotatedChainingAction source = new AnnotatedChainingAction(); + source.setManagerApproved(true); + UnannotatedChainingAction target = new UnannotatedChainingAction(); + mockInvocation.matchAndReturn("getAction", target); + stack.push(source); + stack.push(target); + + // chaining flag on, but global requireAnnotations off -> authorizer authorizes everything + interceptor.setParameterAuthorizer(buildAuthorizer(false, false)); + interceptor.setRequireAnnotations("true"); + interceptor.intercept(invocation); + + assertTrue("when global requireAnnotations is off, enforcement is a no-op", + target.getManagerApproved()); + } +``` + +- [ ] **Step 4: Run the new tests to confirm they fail (compile failure expected first)** + +Run: `mvn -q -pl core test -DskipAssembly -Dtest=ChainingInterceptorTest` +Expected: FAILURE — compilation error `cannot find symbol: method setParameterAuthorizer` / `setRequireAnnotations` on `ChainingInterceptor` (these are added in Task 5). + +- [ ] **Step 5: Commit the failing tests** + +```bash +git add core/src/test/java/org/apache/struts2/interceptor/ChainingInterceptorTest.java +git commit -m "WW-5631 test(chaining): add failing @StrutsParameter enforcement tests" +``` + +--- + +## Task 5: Implement enforcement in ChainingInterceptor + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java` + +- [ ] **Step 1: Add imports** + +In `ChainingInterceptor.java`, add these imports alongside the existing ones (after +`import org.apache.struts2.util.reflection.ReflectionProvider;` and in the `java.*` block): + +```java +import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; +import org.apache.struts2.ognl.OgnlUtil; + +import java.beans.BeanInfo; +import java.beans.IntrospectionException; +import java.beans.PropertyDescriptor; +import java.util.Set; +``` + +- [ ] **Step 2: Add fields and injection setters** + +After the existing field block (the `private ProxyService proxyService;` line, around line 137), add: + +```java + private boolean requireAnnotations = false; + private ParameterAuthorizer parameterAuthorizer; + private OgnlUtil ognlUtil; +``` + +After the existing `setProxyService` setter (around line 147), add: + +```java + @Inject + public void setParameterAuthorizer(ParameterAuthorizer parameterAuthorizer) { + this.parameterAuthorizer = parameterAuthorizer; + } + + @Inject + public void setOgnlUtil(OgnlUtil ognlUtil) { + this.ognlUtil = ognlUtil; + } + + @Inject(value = StrutsConstants.STRUTS_CHAINING_REQUIRE_ANNOTATIONS, required = false) + public void setRequireAnnotations(String requireAnnotations) { + this.requireAnnotations = "true".equalsIgnoreCase(requireAnnotations); + } +``` + +- [ ] **Step 3: Gate the copy in `copyStack`** + +Replace the existing `copyStack` method (lines 174-188): + +```java + private void copyStack(ActionInvocation invocation, CompoundRoot root) { + List<Object> list = prepareList(root); + Map<String, Object> ctxMap = invocation.getInvocationContext().getContextMap(); + for (Object object : list) { + if (!shouldCopy(object)) { + continue; + } + Object action = invocation.getAction(); + Class<?> editable = null; + if (proxyService.isProxy(action)) { + editable = proxyService.ultimateTargetClass(action); + } + reflectionProvider.copy(object, action, ctxMap, prepareExcludes(), includes, editable); + } + } +``` + +with: + +```java + private void copyStack(ActionInvocation invocation, CompoundRoot root) { + List<Object> list = prepareList(root); + Map<String, Object> ctxMap = invocation.getInvocationContext().getContextMap(); + for (Object object : list) { + if (!shouldCopy(object)) { + continue; + } + Object action = invocation.getAction(); + Class<?> editable = null; + if (proxyService.isProxy(action)) { + editable = proxyService.ultimateTargetClass(action); + } + Collection<String> copyExcludes = prepareExcludes(); + if (requireAnnotations) { + Class<?> targetClass = editable != null ? editable : action.getClass(); + BeanInfo beanInfo = getTargetBeanInfo(targetClass); + if (beanInfo == null) { + // Fail closed: cannot prove which properties are annotated, so copy nothing. + LOG.warn("Chaining: unable to introspect target [{}]; skipping property copy " + + "(struts.chaining.requireAnnotations enabled)", targetClass.getName()); + continue; + } + copyExcludes = excludeUnauthorizedProperties(copyExcludes, beanInfo, targetClass, action); + } + reflectionProvider.copy(object, action, ctxMap, copyExcludes, includes, editable); + } + } + + /** + * Returns the excludes to use for the copy: the base excludes unioned with the names of all + * writable target properties that are not authorized by {@code @StrutsParameter}. + */ + private Collection<String> excludeUnauthorizedProperties(Collection<String> baseExcludes, + BeanInfo beanInfo, Class<?> targetClass, Object action) { + Set<String> merged = new HashSet<>(); + if (baseExcludes != null) { + merged.addAll(baseExcludes); + } + for (PropertyDescriptor descriptor : beanInfo.getPropertyDescriptors()) { + if (descriptor.getWriteMethod() == null) { + continue; + } + String name = descriptor.getName(); + if (!parameterAuthorizer.isAuthorized(name, action, action)) { + LOG.warn("Chaining: property [{}] not copied to [{}] because it is not annotated with @StrutsParameter", + name, targetClass.getName()); + merged.add(name); + } + } + return merged; + } + + private BeanInfo getTargetBeanInfo(Class<?> targetClass) { + try { + return ognlUtil.getBeanInfo(targetClass); + } catch (IntrospectionException e) { + LOG.warn("Error introspecting Action {} for chaining @StrutsParameter enforcement", targetClass, e); + return null; + } + } +``` + +- [ ] **Step 4: Run the enforcement tests to confirm they pass** + +Run: `mvn -q -pl core test -DskipAssembly -Dtest=ChainingInterceptorTest` +Expected: PASS — all tests in `ChainingInterceptorTest` green, including the five new methods. + +- [ ] **Step 5: Run the config-based chaining test to confirm no regression** + +Run: `mvn -q -pl core test -DskipAssembly -Dtest=ChainingInterceptorWithConfigTest` +Expected: PASS. + +- [ ] **Step 6: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java +git commit -m "WW-5631 feat(chaining): enforce @StrutsParameter on target when opted in" +``` + +--- + +## Task 6: Add the proxied-target and includes regression tests + +Covers the proxy-class resolution branch and confirms `includes` still filters when enforcement +is on (excludes take precedence in `OgnlUtil.copy`). + +**Files:** +- Modify: `core/src/test/java/org/apache/struts2/interceptor/ChainingInterceptorTest.java` + +- [ ] **Step 1: Add the includes-interaction test** + +Add inside `ChainingInterceptorTest`: + +```java + public void testEnforcementStillFiltersWithIncludesConfigured() throws Exception { + AnnotatedChainingAction source = new AnnotatedChainingAction(); + source.setManagerApproved(true); + UnannotatedChainingAction target = new UnannotatedChainingAction(); + mockInvocation.matchAndReturn("getAction", target); + stack.push(source); + stack.push(target); + + interceptor.setIncludes("managerApproved"); + enableChainingEnforcement(true, false); + interceptor.intercept(invocation); + + assertFalse("unauthorized property must be excluded even when listed in includes", + target.getManagerApproved()); + } +``` + +- [ ] **Step 2: Add the proxied-target test (stub ProxyService)** + +This verifies the `editable`/`targetClass` resolution uses the ultimate class for proxied actions. +Add inside `ChainingInterceptorTest`: + +```java + public void testEnforcementResolvesProxiedTargetClass() throws Exception { + AnnotatedChainingAction source = new AnnotatedChainingAction(); + source.setManagerApproved(true); + final UnannotatedChainingAction target = new UnannotatedChainingAction(); + mockInvocation.matchAndReturn("getAction", target); + stack.push(source); + stack.push(target); + + // Stub ProxyService to report the action as a proxy whose ultimate class is the target class. + interceptor.setProxyService(new ProxyService() { + @Override + public boolean isProxy(Object o) { + return true; + } + + @Override + public Class<?> ultimateTargetClass(Object o) { + return UnannotatedChainingAction.class; + } + }); + enableChainingEnforcement(true, false); + interceptor.intercept(invocation); + + assertFalse("proxied unannotated target property must NOT be copied", target.getManagerApproved()); + } +``` + +> NOTE: If `ProxyService` declares methods beyond `isProxy` and `ultimateTargetClass`, implement them +> as no-ops/defaults to satisfy the interface. Check the interface at +> `core/src/main/java/org/apache/struts2/util/ProxyService.java` before writing this stub; if it has +> additional abstract methods, add minimal implementations returning the obvious default +> (`false`/`null`/the argument). + +- [ ] **Step 3: Run the tests** + +Run: `mvn -q -pl core test -DskipAssembly -Dtest=ChainingInterceptorTest` +Expected: PASS — all methods green. + +- [ ] **Step 4: Commit** + +```bash +git add core/src/test/java/org/apache/struts2/interceptor/ChainingInterceptorTest.java +git commit -m "WW-5631 test(chaining): cover includes interaction and proxied target" +``` + +--- + +## Task 7: Update ChainingInterceptor JavaDoc + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java:62-71` + +- [ ] **Step 1: Document the new constant in the class JavaDoc** + +The existing JavaDoc lists the chaining constants: + +```java + * <ul> + * <li>struts.chaining.copyErrors - set to true to copy Action Errors</li> + * <li>struts.chaining.copyFieldErrors - set to true to copy Field Errors</li> + * <li>struts.chaining.copyMessages - set to true to copy Action Messages</li> + * </ul> +``` + +Replace it with: + +```java + * <ul> + * <li>struts.chaining.copyErrors - set to true to copy Action Errors</li> + * <li>struts.chaining.copyFieldErrors - set to true to copy Field Errors</li> + * <li>struts.chaining.copyMessages - set to true to copy Action Messages</li> + * <li>struts.chaining.requireAnnotations - set to true to only copy properties whose target + * Action member is annotated with {@code @StrutsParameter} (opt-in, default false). When the + * target cannot be introspected, no properties are copied (fail closed).</li> + * </ul> +``` + +- [ ] **Step 2: Compile to verify JavaDoc is well-formed** + +Run: `mvn -q -pl core compile -DskipAssembly` +Expected: BUILD SUCCESS. + +- [ ] **Step 3: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java +git commit -m "WW-5631 docs(chaining): document struts.chaining.requireAnnotations" +``` + +--- + +## Task 8: Full module verification + +- [ ] **Step 1: Run the full core interceptor test package** + +Run: `mvn -q -pl core test -DskipAssembly -Dtest='ChainingInterceptor*,ParametersInterceptorTest'` +Expected: PASS — chaining and parameters interceptor tests all green (confirms no cross-impact with the shared `ParameterAuthorizer`). + +- [ ] **Step 2: Confirm the working tree is clean** + +Run: `git status` +Expected: nothing to commit, working tree clean (all changes committed across Tasks 1-7). + +--- + +## Self-Review Notes + +- **Spec coverage:** constant (T1), default off (T2), `ParameterAuthorizer` reuse + `isAuthorized(name, action, action)` (T5), fail-closed on null BeanInfo (T5), WARN per rejected property (T5), transition mode (T4), proxied target (T6), includes interaction (T6), JavaDoc (T7). All spec sections mapped. +- **Type consistency:** new methods `setParameterAuthorizer`, `setOgnlUtil`, `setRequireAnnotations`, `excludeUnauthorizedProperties`, `getTargetBeanInfo`, and test helpers `buildAuthorizer`/`enableChainingEnforcement` are referenced consistently across tasks. +- **Out of scope (noted in spec):** public-field gating — `reflectionProvider.copy` walks bean property descriptors (setters), so enforcement gates on write-method properties; no `ReflectionProvider`/`OgnlUtil` API changes; main branch only. diff --git a/docs/superpowers/specs/2026-05-27-chaining-interceptor-strutsparameter-enforcement-design.md b/docs/superpowers/specs/2026-05-27-chaining-interceptor-strutsparameter-enforcement-design.md new file mode 100644 index 000000000..6b214f6bb --- /dev/null +++ b/docs/superpowers/specs/2026-05-27-chaining-interceptor-strutsparameter-enforcement-design.md @@ -0,0 +1,164 @@ +# WW-5631 ChainingInterceptor — opt-in `@StrutsParameter` enforcement + +**Jira:** [WW-5631](https://issues.apache.org/jira/browse/WW-5631) +**Date:** 2026-05-27 +**Status:** Design approved +**Target branch:** `main` (7.2.0-SNAPSHOT) +**Type:** Hardening enhancement (opt-in). **Not** a security fix — see Background. + +## Background + +A privately reported scenario observed that `ChainingInterceptor` copies request-influenced +property values from an earlier action into an unannotated setter on a later chained action, +even when `struts.parameters.requireAnnotations=true`. The reporter framed this as a bypass of +the `@StrutsParameter` annotation boundary. + +After analysis this is **not** considered a framework vulnerability: + +- `@StrutsParameter` gates exactly one channel — the binding of untrusted HTTP request + parameters to action properties via `ParametersInterceptor`. Its boundary is "request input + reaching action state." +- `ChainingInterceptor` is, by its documented contract, a property copier between actions the + application has explicitly wired together (the developer chooses the `chain` result, names the + target, and includes the `chain` interceptor in the target's stack). The value arriving at the + target is an internal state transfer the application opted into — not a request parameter. +- This is not a regression introduced by `@StrutsParameter`. Before the annotation existed, every + public setter was writable from both request parameters and chaining. The annotation tightened + the request-parameter channel; it did not redefine chaining as being under its control. +- The secure pattern is to not carry authorization/trust state across action chains, to avoid + action chaining (already discouraged), and to use `Unchainable` / `excludes` where chaining is + required. + +The report is therefore closed as "works as designed." Independently, some users assume +`@StrutsParameter` is a universal annotation gate. To support a stricter posture for those who +want it, we add an **opt-in, default-off** flag that makes `ChainingInterceptor` honour +`@StrutsParameter` on the target action using the same authorization service that +`ParametersInterceptor` (and the JSON/REST channels) already use. + +## Goals + +- Provide an opt-in way to make action chaining respect `@StrutsParameter` on the target action. +- Reuse the existing `ParameterAuthorizer` so semantics (requireAnnotations, transition mode, + depth) stay identical to the parameter-injection channel. +- Zero behavior change and zero overhead when the flag is off (default). +- Confine the change to `ChainingInterceptor` plus one constant and one default property. + +## Non-goals + +- Changing the default behavior of action chaining. +- Treating this as a security release / CVE. +- Per-interceptor-ref granularity (global constant only for now). +- Back-porting to `support/6.x` (main only). +- Modifying `ReflectionProvider` / `OgnlUtil` public API. + +## Configuration + +| Key | Default | Meaning | +| --- | --- | --- | +| `struts.chaining.requireAnnotations` | `false` | When `true`, `ChainingInterceptor` only copies properties whose target member is authorized by `@StrutsParameter` (per the active `requireAnnotations` / `transitionMode` rules). | + +- New constant: `StrutsConstants.STRUTS_CHAINING_REQUIRE_ANNOTATIONS = "struts.chaining.requireAnnotations"`. +- New entry in `core/src/main/resources/org/apache/struts2/default.properties`: + `struts.chaining.requireAnnotations=false`. +- The flag only has an effect in combination with the target action's annotations; it does not + itself enable/disable `struts.parameters.requireAnnotations`. Enforcement uses + `ParameterAuthorizer`, which already reads `struts.parameters.requireAnnotations` and + `struts.parameters.requireAnnotations.transitionMode`. In practice, if + `struts.parameters.requireAnnotations=false`, the authorizer authorizes everything and the chain + flag is a no-op — which is the intended, consistent behavior. + +## Component design + +### `ChainingInterceptor` (modified) + +New injected collaborators: + +- `ParameterAuthorizer parameterAuthorizer` — injected by interface (`@Inject`). Already bound as + a singleton (`DefaultConfiguration` factory binding; `StrutsBeanSelectionProvider` alias). +- `OgnlUtil ognlUtil` — injected to obtain `BeanInfo` of the target action for enumerating + writable property names. +- `boolean requireAnnotations` — injected via + `@Inject(value = STRUTS_CHAINING_REQUIRE_ANNOTATIONS, required = false)`. + +Modified flow in `copyStack(...)`: + +``` +for each object on the stack to copy (existing loop): + if !shouldCopy(object): continue + action = invocation.getAction() + editable = proxy? proxyService.ultimateTargetClass(action) : null + + excludesForCopy = prepareExcludes() // existing errors/messages handling + user excludes + if requireAnnotations: + excludesForCopy = union(excludesForCopy, rejectedTargetProperties(action, editable)) + reflectionProvider.copy(object, action, ctxMap, excludesForCopy, includes, editable) +``` + +`rejectedTargetProperties(action, editable)`: + +1. Resolve `targetClass` = `editable != null ? editable : action.getClass()`. +2. Obtain `BeanInfo` via `ognlUtil.getBeanInfo(targetClass)`. + - **Fail closed:** if `BeanInfo` is `null` (introspection failure), return the set of *all* + candidate writable property names so nothing is copied for this object, and emit a single + WARN. Rationale: the user explicitly opted into stricter enforcement; when we cannot prove a + property is annotated, we do not copy it. +3. Enumerate candidate writable members: + - `PropertyDescriptor`s with a non-null `getWriteMethod()`. + - Public fields on the target class (mirrors `StrutsParameterAuthorizer`'s field fallback). +4. For each candidate property `name`, call + `parameterAuthorizer.isAuthorized(name, action, action)` (depth-0, target == action so the + ModelDriven exemption branch is intentionally not taken — chaining copies onto the action + object itself, not a model). +5. Names that are **not** authorized are added to the rejected set. Emit one WARN per rejected + name including the target action class and property name. + +When `requireAnnotations` is false, none of the above runs — behavior and cost are unchanged. + +### Logging + +- Per rejected property: `WARN` (always, not only devMode) — + e.g. `"Chaining: property [{}] not copied to [{}] because it is not annotated with @StrutsParameter"`. +- BeanInfo failure: single `WARN` describing fail-closed behavior for that target. +- Volume is bounded by the number of writable properties on the target action; acceptable. + +## Data flow / boundary notes + +- Target of every copy is `invocation.getAction()` (or its ultimate class for proxies). Annotation + checks are performed against that same object, so `isAuthorized(name, action, action)` is the + correct call shape; `target == action` keeps the ModelDriven exemption inactive. +- `includes` semantics are preserved: excludes take precedence in `OgnlUtil.copy`, so adding + unauthorized names to excludes correctly removes them even if `includes` is configured. +- `Unchainable` objects are skipped before this logic runs (unchanged). + +## Testing + +Extend `core/.../interceptor/ChainingInterceptorTest` (and `ChainingInterceptorWithConfigTest` +where container wiring is needed): + +1. **Flag off (default):** unannotated target property is copied — legacy behavior preserved. +2. **Flag on + annotated target:** annotated property is copied. +3. **Flag on + unannotated target:** property is *not* copied; WARN emitted. +4. **Flag on + transition mode on:** depth-0 (non-nested) property copied without annotation + (consistency with `ParametersInterceptor`). +5. **Flag on + `requireAnnotations=false` globally:** flag is a no-op, everything copied. +6. **Flag on + proxied target action:** enforcement resolves the ultimate target class correctly. +7. **Flag on + BeanInfo introspection failure (simulated):** nothing copied for that object, + WARN emitted (fail-closed). +8. **Flag on + `includes` configured:** unauthorized names still excluded. + +## Documentation + +- Update `ChainingInterceptor` JavaDoc interceptor-parameters/notes section to document + `struts.chaining.requireAnnotations` and the fail-closed behavior. +- Add a "scope of `@StrutsParameter`" clarification to the security docs (tracked separately): + it gates request-parameter binding; chaining is an opt-in internal transfer; this flag exists + for users who want chaining to honour the annotation too. + +## Rollout + +- Default-off means no migration impact. Opt-in users should test their chains, as legitimately + chained properties on the target action will need `@StrutsParameter` once enabled. + +## Open questions + +- None outstanding. (Fail mode = fail closed; granularity = global constant only; both confirmed.)
