Merge pull request #1072 from apache/fix/WW-5468-modeldriven-2

WW-5468 Exempt ModelDriven Actions from @StrutsParameter requirement
apache · Nov 1, 2024 · 7cdcd84 · 7cdcd84
1 parent 8566c14
commit 7cdcd84
Show file tree

Hide file tree

Showing 17 changed files with 281 additions and 248 deletions.
diff --git a/apps/showcase/src/main/java/org/apache/struts2/showcase/modelDriven/ModelDrivenAction.java b/apps/showcase/src/main/java/org/apache/struts2/showcase/modelDriven/ModelDrivenAction.java
@@ -42,7 +42,7 @@ public String execute() throws Exception {
 	}
 
 	@Override
-	public Object getModel() {
+	public Gangster getModel() {
 		return new Gangster();
 	}
 }
diff --git a/core/src/main/java/org/apache/struts2/ModelDriven.java b/core/src/main/java/org/apache/struts2/ModelDriven.java
@@ -18,6 +18,8 @@
  */
 package org.apache.struts2;
 
+import org.apache.struts2.interceptor.parameter.StrutsParameter;
+
 /**
  * ModelDriven Actions provide a model object to be pushed onto the ValueStack
  * in addition to the Action itself, allowing a FormBean type approach like Struts.
@@ -28,9 +30,13 @@ public interface ModelDriven<T> {
 
     /**
      * Gets the model to be pushed onto the ValueStack instead of the Action itself.
+     * <p>
+     * Please be aware that all setters and getters of every depth on the object returned by this method are available
+     * for user parameter injection!
      *
      * @return the model
      */
+    @StrutsParameter(depth = Integer.MAX_VALUE)
     T getModel();
 
 }
diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
@@ -35,6 +35,7 @@
 import org.apache.commons.lang3.ClassUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.apache.struts2.ModelDriven;
 import org.apache.struts2.StrutsConstants;
 import org.apache.struts2.action.NoParameters;
 import org.apache.struts2.action.ParameterNameAware;
@@ -348,7 +349,15 @@ protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) {
         }
 
         long paramDepth = name.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count();
+
+        if (action instanceof ModelDriven<?> && !ActionContext.getContext().getValueStack().peek().equals(action)) {
+            LOG.debug("Model driven Action detected, exempting from @StrutsParameter annotation requirement and OGNL allowlisting model type");
+            // (Exempted by annotation on com.opensymphony.xwork2.ModelDriven#getModel)
+            return hasValidAnnotatedMember("model", action, paramDepth + 1);
+        }
+
         if (requireAnnotationsTransitionMode && paramDepth == 0) {
+            LOG.debug("Annotation transition mode enabled, exempting non-nested parameter [{}] from @StrutsParameter annotation requirement", name);
             return true;
         }
 
@@ -365,6 +374,8 @@ protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) {
      * save computation by checking this last.
      */
     protected boolean hasValidAnnotatedMember(String rootProperty, Object action, long paramDepth) {
+        LOG.debug("Checking Action [{}] for a matching, correctly annotated member for property [{}]",
+                action.getClass().getSimpleName(), rootProperty);
         BeanInfo beanInfo = getBeanInfo(action);
         if (beanInfo == null) {
             return hasValidAnnotatedField(action, rootProperty, paramDepth);
@@ -399,7 +410,7 @@ protected boolean hasValidAnnotatedPropertyDescriptor(Object action, PropertyDes
         }
         if (getPermittedInjectionDepth(relevantMethod) < paramDepth) {
             String logMessage = format(
-                    "Parameter injection for method [%s] on action [%s] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.",
+                    "Parameter injection for method [%s] on Action [%s] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.",
                     relevantMethod.getName(),
                     relevantMethod.getDeclaringClass().getName());
             if (devMode) {
@@ -409,8 +420,10 @@ protected boolean hasValidAnnotatedPropertyDescriptor(Object action, PropertyDes
             }
             return false;
         }
+        LOG.debug("Success: Matching annotated method [{}] found for property [{}] of depth [{}] on Action [{}]",
+                relevantMethod.getName(), propDesc.getName(), paramDepth, action.getClass().getSimpleName());
         if (paramDepth >= 1) {
-            allowlistClass(relevantMethod.getReturnType());
+            allowlistClass(propDesc.getPropertyType());
         }
         if (paramDepth >= 2) {
             allowlistReturnTypeIfParameterized(relevantMethod);
@@ -447,19 +460,23 @@ protected void allowlistClass(Class<?> clazz) {
     }
 
     protected boolean hasValidAnnotatedField(Object action, String fieldName, long paramDepth) {
+        LOG.debug("No matching annotated method found for property [{}] of depth [{}] on Action [{}], now also checking for public field",
+                fieldName, paramDepth, action.getClass().getSimpleName());
         Field field;
         try {
             field = action.getClass().getDeclaredField(fieldName);
         } catch (NoSuchFieldException e) {
+            LOG.debug("Matching field for property [{}] not found on Action [{}]", fieldName, action.getClass().getSimpleName());
             return false;
         }
         if (!Modifier.isPublic(field.getModifiers())) {
+            LOG.debug("Matching field [{}] is not public on Action [{}]", field.getName(), action.getClass().getSimpleName());
             return false;
         }
         if (getPermittedInjectionDepth(field) < paramDepth) {
             String logMessage = format(
-                    "Parameter injection for field [%s] on action [%s] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.",
-                    fieldName,
+                    "Parameter injection for field [%s] on Action [%s] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.",
+                    field.getName(),
                     action.getClass().getName());
             if (devMode) {
                 notifyDeveloperOfError(LOG, action, logMessage);
@@ -468,6 +485,8 @@ protected boolean hasValidAnnotatedField(Object action, String fieldName, long p
             }
             return false;
         }
+        LOG.debug("Success: Matching annotated public field [{}] found for property of depth [{}] on Action [{}]",
+                field.getName(), paramDepth, action.getClass().getSimpleName());
         if (paramDepth >= 1) {
             allowlistClass(field.getType());
         }
@@ -629,7 +648,7 @@ protected boolean isAccepted(String paramName) {
         if (!result.isAccepted()) {
             if (devMode) {
                 LOG.warn("Parameter [{}] didn't match accepted pattern [{}]! See Accepted / Excluded patterns at\n" +
-                                "https://struts.apache.org/security/#accepted--excluded-patterns",
+                         "https://struts.apache.org/security/#accepted--excluded-patterns",
                         paramName, result.getAcceptedPattern());
             } else {
                 LOG.debug("Parameter [{}] didn't match accepted pattern [{}]!", paramName, result.getAcceptedPattern());
@@ -644,8 +663,8 @@ protected boolean isExcluded(String paramName) {
         if (result.isExcluded()) {
             if (devMode) {
                 LOG.warn("Parameter [{}] matches excluded pattern [{}]! See Accepted / Excluded patterns at\n" +
-                        "https://struts.apache.org/security/#accepted--excluded-patterns",
-                    paramName, result.getExcludedPattern());
+                         "https://struts.apache.org/security/#accepted--excluded-patterns",
+                        paramName, result.getExcludedPattern());
             } else {
                 LOG.debug("Parameter [{}] matches excluded pattern [{}]!", paramName, result.getExcludedPattern());
             }
@@ -663,8 +682,8 @@ protected boolean isParamValueExcluded(String value) {
             if (excludedValuePattern.matcher(value).matches()) {
                 if (devMode) {
                     LOG.warn("Parameter value [{}] matches excluded pattern [{}]! See Accepting/Excluding parameter values at\n" +
-                            "https://struts.apache.org/core-developers/parameters-interceptor#excluding-parameter-values",
-                        value, excludedValuePatterns);
+                             "https://struts.apache.org/core-developers/parameters-interceptor#excluding-parameter-values",
+                            value, excludedValuePatterns);
                 } else {
                     LOG.debug("Parameter value [{}] matches excluded pattern [{}]", value, excludedValuePattern);
                 }
@@ -686,8 +705,8 @@ protected boolean isParamValueAccepted(String value) {
         }
         if (devMode) {
             LOG.warn("Parameter value [{}] didn't match accepted pattern [{}]! See Accepting/Excluding parameter values at\n" +
-                    "https://struts.apache.org/core-developers/parameters-interceptor#excluding-parameter-values",
-                value, acceptedValuePatterns);
+                     "https://struts.apache.org/core-developers/parameters-interceptor#excluding-parameter-values",
+                    value, acceptedValuePatterns);
         } else {
             LOG.debug("Parameter value [{}] was not accepted!", value);
         }
@@ -757,7 +776,7 @@ public void setAcceptedValuePatterns(String commaDelimitedPatterns) {
             LOG.debug("Sets accepted value patterns to [{}], note this may impact the safety of your application!", patterns);
         } else {
             LOG.warn("Replacing accepted patterns [{}] with [{}], be aware that this may impact safety of your application!",
-                acceptedValuePatterns, patterns);
+                    acceptedValuePatterns, patterns);
         }
         acceptedValuePatterns = new HashSet<>(patterns.size());
         try {
@@ -782,7 +801,7 @@ public void setExcludedValuePatterns(String commaDelimitedPatterns) {
             LOG.debug("Setting excluded value patterns to [{}]", patterns);
         } else {
             LOG.warn("Replacing excluded value patterns [{}] with [{}], be aware that this may impact safety of your application!",
-                excludedValuePatterns, patterns);
+                    excludedValuePatterns, patterns);
         }
         excludedValuePatterns = new HashSet<>(patterns.size());
         try {

diff --git a/core/src/test/java/com/opensymphony/xwork2/ModelDrivenAction.java b/core/src/test/java/com/opensymphony/xwork2/ModelDrivenAction.java
@@ -44,9 +44,8 @@ public String getFoo() {
     /**
      * @return the model to be pushed onto the ValueStack after the Action itself
      */
-    @StrutsParameter(depth = 2)
     @Override
-    public Object getModel() {
+    public TestBean getModel() {
         return model;
     }
 }
diff --git a/core/src/test/java/com/opensymphony/xwork2/ModelDrivenAnnotationAction.java b/core/src/test/java/com/opensymphony/xwork2/ModelDrivenAnnotationAction.java
@@ -44,9 +44,8 @@ public String getFoo() {
     /**
      * @return the model to be pushed onto the ValueStack after the Action itself
      */
-    @StrutsParameter(depth = 2)
     @Override
-    public Object getModel() {
+    public AnnotatedTestBean getModel() {
         return model;
     }
 }
diff --git a/core/src/test/java/com/opensymphony/xwork2/interceptor/ModelDrivenInterceptorTest.java b/core/src/test/java/com/opensymphony/xwork2/interceptor/ModelDrivenInterceptorTest.java
@@ -178,7 +178,7 @@ protected void tearDown() throws Exception {
     }
 
 
-    public class ModelDrivenAction extends ActionSupport implements ModelDriven {
+    public class ModelDrivenAction extends ActionSupport implements ModelDriven<Object> {
 
         @Override
         public Object getModel() {

diff --git a/core/src/test/java/com/opensymphony/xwork2/test/ModelDrivenAction2.java b/core/src/test/java/com/opensymphony/xwork2/test/ModelDrivenAction2.java
@@ -19,7 +19,6 @@
 package com.opensymphony.xwork2.test;
 
 import com.opensymphony.xwork2.ModelDrivenAction;
-import org.apache.struts2.interceptor.parameter.StrutsParameter;
 
 
 /**
@@ -35,9 +34,8 @@ public class ModelDrivenAction2 extends ModelDrivenAction {
     /**
      * @return the model to be pushed onto the ValueStack after the Action itself
      */
-    @StrutsParameter(depth = 3)
     @Override
-    public Object getModel() {
+    public TestBean2 getModel() {
         return model;
     }
 }
diff --git a/core/src/test/java/com/opensymphony/xwork2/test/ModelDrivenAnnotationAction2.java b/core/src/test/java/com/opensymphony/xwork2/test/ModelDrivenAnnotationAction2.java
@@ -19,7 +19,6 @@
 package com.opensymphony.xwork2.test;
 
 import com.opensymphony.xwork2.ModelDrivenAnnotationAction;
-import org.apache.struts2.interceptor.parameter.StrutsParameter;
 
 
 /**
@@ -36,9 +35,8 @@ public class ModelDrivenAnnotationAction2 extends ModelDrivenAnnotationAction {
     /**
      * @return the model to be pushed onto the ValueStack after the Action itself
      */
-    @StrutsParameter(depth = 3)
     @Override
-    public Object getModel() {
+    public AnnotationTestBean2 getModel() {
         return model;
     }
 }
diff --git a/core/src/test/java/com/opensymphony/xwork2/test/subtest/NullModelDrivenAction.java b/core/src/test/java/com/opensymphony/xwork2/test/subtest/NullModelDrivenAction.java
@@ -19,6 +19,7 @@
 package com.opensymphony.xwork2.test.subtest;
 
 import com.opensymphony.xwork2.ModelDrivenAction;
+import com.opensymphony.xwork2.TestBean;
 
 /**
  * Extends ModelDrivenAction to return a null model.
@@ -31,7 +32,7 @@ public class NullModelDrivenAction extends ModelDrivenAction {
      * @return the model to be pushed onto the ValueStack instead of the Action itself
      */
     @Override
-    public Object getModel() {
+    public TestBean getModel() {
         return null;
     }
 }
diff --git a/core/src/test/java/com/opensymphony/xwork2/validator/VisitorValidatorModelAction.java b/core/src/test/java/com/opensymphony/xwork2/validator/VisitorValidatorModelAction.java
@@ -19,7 +19,7 @@
 package com.opensymphony.xwork2.validator;
 
 import com.opensymphony.xwork2.ModelDriven;
-import org.apache.struts2.interceptor.parameter.StrutsParameter;
+import com.opensymphony.xwork2.TestBean;
 
 
 /**
@@ -33,9 +33,8 @@ public class VisitorValidatorModelAction extends VisitorValidatorTestAction impl
     /**
      * @return the model to be pushed onto the ValueStack instead of the Action itself
      */
-    @StrutsParameter(depth = 2)
     @Override
-    public Object getModel() {
+    public TestBean getModel() {
         return getBean();
     }
 }
diff --git a/...src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java b/...src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java
@@ -18,8 +18,12 @@
  */
 package org.apache.struts2.interceptor.parameter;
 
+import com.opensymphony.xwork2.ActionContext;
+import com.opensymphony.xwork2.ModelDriven;
+import com.opensymphony.xwork2.StubValueStack;
 import com.opensymphony.xwork2.security.AcceptedPatternsChecker;
 import com.opensymphony.xwork2.security.NotExcludedAcceptedPatternsChecker;
+import com.opensymphony.xwork2.util.ValueStack;
 import org.apache.commons.lang3.ClassUtils;
 import org.apache.struts2.dispatcher.HttpParameters;
 import org.apache.struts2.dispatcher.Parameter;
@@ -63,6 +67,7 @@ public void setUp() throws Exception {
     @After
     public void tearDown() throws Exception {
         threadAllowlist.clearAllowlist();
+        ActionContext.clear();
     }
 
     private void testParameter(Object action, String paramName, boolean shouldContain) {
@@ -80,7 +85,7 @@ private void testParameter(Object action, String paramName, boolean shouldContai
         }
     }
 
-    private Set<Class<?>> getParentClasses(Class<?> ...clazzes) {
+    private Set<Class<?>> getParentClasses(Class<?>... clazzes) {
         Set<Class<?>> set = new HashSet<>();
         for (Class<?> clazz : clazzes) {
             set.add(clazz);
@@ -258,8 +263,21 @@ public void publicStrNotAnnotatedMethod_transitionMode() {
         testParameter(new MethodAction(), "publicStrNotAnnotated", true);
     }
 
+    @Test
+    public void publicModelPojo() {
+        ModelAction action = new ModelAction();
+
+        // Emulate ModelDrivenInterceptor running previously
+        ValueStack valueStack = new StubValueStack();
+        valueStack.push(action.getModel());
+        ActionContext.of().withValueStack(valueStack).bind();
+
+        testParameter(action, "name", true);
+        testParameter(action, "name.nested", true);
+        assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Object.class, Pojo.class));
+    }
 
-    class FieldAction {
+    static class FieldAction {
         @StrutsParameter
         private String privateStr;
 
@@ -275,7 +293,7 @@ class FieldAction {
         public Pojo publicPojoDepthZero;
 
         @StrutsParameter(depth = 1)
-        public Pojo publicPojoDepthOne ;
+        public Pojo publicPojoDepthOne;
 
         @StrutsParameter(depth = 2)
         public Pojo publicPojoDepthTwo;
@@ -290,7 +308,7 @@ class FieldAction {
         public Map<String, Pojo> publicPojoMapDepthTwo;
     }
 
-    class MethodAction {
+    static class MethodAction {
 
         @StrutsParameter
         private void setPrivateStr(String str) {
@@ -343,6 +361,14 @@ public Map<String, Pojo> getPublicPojoMapDepthTwo() {
         }
     }
 
-    class Pojo {
+    static class ModelAction implements ModelDriven<Pojo> {
+
+        @Override
+        public Pojo getModel() {
+            return new Pojo();
+        }
+    }
+
+    static class Pojo {
     }
 }