WW-5534 Proper fix ModelDriven parameter injection and allowlisting

Author: kusalk

apps/showcase/src/test/java/it/org/apache/struts2/showcase/ModelDrivenTest.java

@@ -0,0 +1,63 @@
+/*
+ * 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 it.org.apache.struts2.showcase;
+
+import org.htmlunit.WebClient;
+import org.htmlunit.html.HtmlForm;
+import org.htmlunit.html.HtmlPage;
+import org.htmlunit.html.HtmlSubmitInput;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class ModelDrivenTest {
+
+    private WebClient webClient;
+
+    @Before
+    public void setUp() throws Exception {
+        webClient = new WebClient();
+    }
+
+    @After
+    public void tearDown() throws Exception {
+        webClient.close();
+    }
+
+    @Test
+    public void submit() throws Exception {
+        HtmlPage page = webClient.getPage(ParameterUtils.getBaseUrl() + "/modelDriven/modelDriven.action");
+        HtmlForm form = page.getForms().get(0);
+
+        form.getInputByName("name").setValue("Johannes");
+        form.getInputByName("age").setValue("21");
+        form.getInputByName("bustedBefore").setChecked(true);
+        form.getTextAreaByName("description").setText("Deals bugs");
+
+        HtmlSubmitInput button = form.getInputByValue("Submit");
+        page = button.click();
+
+        assertThat(page.getElementById("name").asNormalizedText()).isEqualTo("Johannes");
+        assertThat(page.getElementById("age").asNormalizedText()).isEqualTo("21");
+        assertThat(page.getElementById("bustedBefore").asNormalizedText()).isEqualTo("true");
+        assertThat(page.getElementById("description").asNormalizedText()).isEqualTo("Deals bugs");
+    }
+}

core/src/main/java/org/apache/struts2/ModelDriven.java

@@ -18,8 +18,6 @@
  */
 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.
@@ -36,7 +34,6 @@ public interface ModelDriven<T> {
      *
      * @return the model
      */
-    @StrutsParameter(depth = Integer.MAX_VALUE)
     T getModel();
 
 }

core/src/main/java/org/apache/struts2/components/Debug.java

@@ -18,18 +18,17 @@
  */
 package org.apache.struts2.components;
 
-import org.apache.commons.lang3.ClassUtils;
+import org.apache.struts2.StrutsException;
+import org.apache.struts2.dispatcher.PrepareOperations;
 import org.apache.struts2.inject.Inject;
 import org.apache.struts2.ognl.ThreadAllowlist;
 import org.apache.struts2.util.CompoundRoot;
 import org.apache.struts2.util.ValueStack;
 import org.apache.struts2.util.reflection.ReflectionProvider;
-import jakarta.servlet.http.HttpServletRequest;
-import jakarta.servlet.http.HttpServletResponse;
-import org.apache.struts2.StrutsException;
-import org.apache.struts2.dispatcher.PrepareOperations;
 import org.apache.struts2.views.annotations.StrutsTag;
 
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
 import java.io.Writer;
 import java.util.ArrayList;
 import java.util.Iterator;
@@ -94,9 +93,7 @@ private void allowList(CompoundRoot root) {
     }
 
     private void allowListClass(Object o) {
-        threadAllowlist.allowClass(o.getClass());
-        ClassUtils.getAllSuperclasses(o.getClass()).forEach(threadAllowlist::allowClass);
-        ClassUtils.getAllInterfaces(o.getClass()).forEach(threadAllowlist::allowClass);
+        threadAllowlist.allowClassHierarchy(o.getClass());
     }
 
     @Override

core/src/main/java/org/apache/struts2/components/IteratorComponent.java

@@ -18,12 +18,12 @@
  */
 package org.apache.struts2.components;
 
-import org.apache.struts2.inject.Inject;
-import org.apache.struts2.util.ValueStack;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.apache.struts2.inject.Inject;
 import org.apache.struts2.ognl.ThreadAllowlist;
 import org.apache.struts2.util.MakeIterator;
+import org.apache.struts2.util.ValueStack;
 import org.apache.struts2.views.annotations.StrutsTag;
 import org.apache.struts2.views.annotations.StrutsTagAttribute;
 import org.apache.struts2.views.jsp.IteratorStatus;
@@ -307,7 +307,7 @@ public boolean start(Writer writer) {
             stack.push(currentValue);
 
             if (currentValue != null) {
-                threadAllowlist.allowClass(currentValue.getClass());
+                threadAllowlist.allowClassHierarchy(currentValue.getClass());
             }
 
             String var = getVar();

core/src/main/java/org/apache/struts2/interceptor/ExceptionMappingInterceptor.java

@@ -215,9 +215,9 @@ public String intercept(ActionInvocation invocation) throws Exception {
                 HttpParameters parameters = HttpParameters.create(mappingParams).build();
                 invocation.getInvocationContext().withParameters(parameters);
                 result = mappingConfig.getResult();
-                ExceptionHolder holder = new ExceptionHolder(e);
-                threadAllowlist.allowClass(holder.getClass());
-                threadAllowlist.allowClass(e.getClass());
+                var holder = new ExceptionHolder(e);
+                threadAllowlist.allowClassHierarchy(ExceptionHolder.class);
+                threadAllowlist.allowClassHierarchy(e.getClass());
                 publishException(invocation, holder);
             } else {
                 throw e;

core/src/main/java/org/apache/struts2/interceptor/ModelDrivenInterceptor.java

@@ -20,6 +20,8 @@
 
 import org.apache.struts2.ActionInvocation;
 import org.apache.struts2.ModelDriven;
+import org.apache.struts2.inject.Inject;
+import org.apache.struts2.ognl.ThreadAllowlist;
 import org.apache.struts2.util.CompoundRoot;
 import org.apache.struts2.util.ValueStack;
 
@@ -79,20 +81,27 @@
 public class ModelDrivenInterceptor extends AbstractInterceptor {
 
     protected boolean refreshModelBeforeResult = false;
+    private ThreadAllowlist threadAllowlist;
 
     public void setRefreshModelBeforeResult(boolean val) {
         this.refreshModelBeforeResult = val;
     }
 
+    @Inject
+    public void setThreadAllowlist(ThreadAllowlist threadAllowlist) {
+        this.threadAllowlist = threadAllowlist;
+    }
+
     @Override
     public String intercept(ActionInvocation invocation) throws Exception {
         Object action = invocation.getAction();
 
-        if (action instanceof ModelDriven modelDriven) {
+        if (action instanceof ModelDriven<?> modelDriven) {
             ValueStack stack = invocation.getStack();
             Object model = modelDriven.getModel();
-            if (model !=  null) {
-            	stack.push(model);
+            if (model != null) {
+                stack.push(model);
+                threadAllowlist.allowClassHierarchy(model.getClass());
             }
             if (refreshModelBeforeResult) {
                 invocation.addPreResultListener(new RefreshModelBeforeResult(modelDriven, model));

core/src/main/java/org/apache/struts2/interceptor/debugging/DebuggingInterceptor.java

@@ -18,8 +18,6 @@
  */
 package org.apache.struts2.interceptor.debugging;
 
-import jakarta.servlet.http.HttpServletResponse;
-import org.apache.commons.lang3.ClassUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.struts2.ActionContext;
@@ -38,6 +36,7 @@
 import org.apache.struts2.views.freemarker.FreemarkerManager;
 import org.apache.struts2.views.freemarker.FreemarkerResult;
 
+import jakarta.servlet.http.HttpServletResponse;
 import java.beans.BeanInfo;
 import java.beans.Introspector;
 import java.beans.PropertyDescriptor;
@@ -273,9 +272,7 @@ public String intercept(ActionInvocation inv) throws Exception {
 
     private void allowListClass(Object o) {
         if (o != null) {
-            threadAllowlist.allowClass(o.getClass());
-            ClassUtils.getAllSuperclasses(o.getClass()).forEach(threadAllowlist::allowClass);
-            ClassUtils.getAllInterfaces(o.getClass()).forEach(threadAllowlist::allowClass);
+            threadAllowlist.allowClassHierarchy(o.getClass());
         }
     }
 

core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java

@@ -19,7 +19,6 @@
 package org.apache.struts2.interceptor.parameter;
 
 import org.apache.commons.lang3.BooleanUtils;
-import org.apache.commons.lang3.ClassUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.struts2.ActionContext;
@@ -358,9 +357,8 @@ 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 org.apache.struts2.ModelDriven#getModel)
-            return hasValidAnnotatedMember("model", action, paramDepth + 1);
+            LOG.debug("Model driven Action detected, exempting from @StrutsParameter annotation requirement");
+            return true;
         }
 
         if (requireAnnotationsTransitionMode && paramDepth == 0) {
@@ -447,15 +445,13 @@ protected void allowlistParameterizedTypeArg(Type genericType) {
     }
 
     protected void allowlistParamType(Type paramType) {
-        if (paramType instanceof Class) {
-            allowlistClass((Class<?>) paramType);
+        if (paramType instanceof Class<?> clazz) {
+            allowlistClass(clazz);
         }
     }
 
     protected void allowlistClass(Class<?> clazz) {
-        threadAllowlist.allowClass(clazz);
-        ClassUtils.getAllSuperclasses(clazz).forEach(threadAllowlist::allowClass);
-        ClassUtils.getAllInterfaces(clazz).forEach(threadAllowlist::allowClass);
+        threadAllowlist.allowClassHierarchy(clazz);
     }
 
     protected boolean hasValidAnnotatedField(Object action, String fieldName, long paramDepth) {
@@ -527,10 +523,11 @@ protected Class<?> ultimateClass(Object action) {
     }
 
     protected BeanInfo getBeanInfo(Object action) {
+        Class<?> actionClass = ultimateClass(action);
         try {
-            return ognlUtil.getBeanInfo(ultimateClass(action));
+            return ognlUtil.getBeanInfo(actionClass);
         } catch (IntrospectionException e) {
-            LOG.warn("Error introspecting Action {} for parameter injection validation", action.getClass(), e);
+            LOG.warn("Error introspecting Action {} for parameter injection validation", actionClass, e);
             return null;
         }
     }

core/src/main/java/org/apache/struts2/ognl/ThreadAllowlist.java

@@ -18,6 +18,8 @@
  */
 package org.apache.struts2.ognl;
 
+import org.apache.commons.lang3.ClassUtils;
+
 import java.util.HashSet;
 import java.util.Set;
 
@@ -34,6 +36,15 @@ public class ThreadAllowlist {
 
     private final ThreadLocal<Set<Class<?>>> allowlist = new ThreadLocal<>();
 
+    /**
+     * @since 7.1.0
+     */
+    public void allowClassHierarchy(Class<?> clazz) {
+        allowClass(clazz);
+        ClassUtils.getAllSuperclasses(clazz).forEach(this::allowClass);
+        ClassUtils.getAllInterfaces(clazz).forEach(this::allowClass);
+    }
+
     public void allowClass(Class<?> clazz) {
         if (allowlist.get() == null) {
             allowlist.set(new HashSet<>());

core/src/test/java/org/apache/struts2/interceptor/ModelDrivenInterceptorTest.java

@@ -20,16 +20,19 @@
 
 import com.mockobjects.dynamic.ConstraintMatcher;
 import com.mockobjects.dynamic.Mock;
-import org.apache.struts2.action.Action;
 import org.apache.struts2.ActionContext;
 import org.apache.struts2.ActionInvocation;
 import org.apache.struts2.ActionSupport;
 import org.apache.struts2.ModelDriven;
 import org.apache.struts2.XWorkTestCase;
+import org.apache.struts2.action.Action;
+import org.apache.struts2.ognl.ThreadAllowlist;
 import org.apache.struts2.util.ValueStack;
 
 import java.util.Date;
 
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
 
 /**
  * @author $Author$
@@ -40,6 +43,7 @@ public class ModelDrivenInterceptorTest extends XWorkTestCase {
     Action action;
     Mock mockActionInvocation;
     ModelDrivenInterceptor modelDrivenInterceptor;
+    ThreadAllowlist threadAllowlist;
     Object model;
     PreResultListener preResultListener;
     ValueStack stack;
@@ -55,6 +59,7 @@ public void testModelDrivenGetsPushedOntoStack() throws Exception {
 
         Object topOfStack = stack.pop();
         assertEquals("our model should be on the top of the stack", model, topOfStack);
+        verify(threadAllowlist).allowClassHierarchy(model.getClass());
     }
 
     private void setupRefreshModelBeforeResult() {
@@ -167,6 +172,8 @@ protected void setUp() throws Exception {
         super.setUp();
         mockActionInvocation = new Mock(ActionInvocation.class);
         modelDrivenInterceptor = new ModelDrivenInterceptor();
+        threadAllowlist = mock(ThreadAllowlist.class);
+        modelDrivenInterceptor.setThreadAllowlist(threadAllowlist);
         stack = ActionContext.getContext().getValueStack();
         model = new Date(); // any object will do
     }

core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java

@@ -381,7 +381,6 @@ public void publicModelPojo() {
 
         testParameter(action, "name", true);
         testParameter(action, "name.nested", true);
-        assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Object.class, Pojo.class));
     }
 
     /**
@@ -402,10 +401,9 @@ public void publicModelPojo_proxied() {
 
         testParameter(proxiedAction, "name", true);
         testParameter(proxiedAction, "name.nested", true);
-        assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Object.class, Pojo.class));
     }
 
-    static class FieldAction {
+    public static class FieldAction {
         @StrutsParameter
         private String privateStr;
 
@@ -436,7 +434,7 @@ static class FieldAction {
         public Map<String, Pojo> publicPojoMapDepthTwo;
     }
 
-    static class MethodAction {
+    public static class MethodAction {
 
         @StrutsParameter
         private void setPrivateStr(String str) {
@@ -489,14 +487,14 @@ public Map<String, Pojo> getPublicPojoMapDepthTwo() {
         }
     }
 
-    static class ModelAction implements ModelDriven<Pojo> {
+    public static class ModelAction implements ModelDriven<Pojo> {
 
         @Override
         public Pojo getModel() {
             return new Pojo();
         }
     }
 
-    static class Pojo {
+    public static class Pojo {
     }
 }