This is an automated email from the ASF dual-hosted git repository.
kusal pushed a commit to branch WW-5534-model-driven-struts-param
in repository https://gitbox.apache.org/repos/asf/struts.git
commit 59e6baade0ec6ded5e0f42d7633b041578df20cf
Author: Kusal Kithul-Godage <g...@kusal.io>
AuthorDate: Thu Mar 6 23:50:44 2025 +1100
WW-5534 Proper fix ModelDriven parameter injection and allowlisting
---
.../apache/struts2/showcase/ModelDrivenTest.java | 63 ++++++++++++++++++++++
.../main/java/org/apache/struts2/ModelDriven.java | 3 --
.../java/org/apache/struts2/components/Debug.java | 13 ++---
.../struts2/components/IteratorComponent.java | 6 +--
.../interceptor/ExceptionMappingInterceptor.java | 6 +--
.../interceptor/ModelDrivenInterceptor.java | 15 ++++--
.../debugging/DebuggingInterceptor.java | 7 +--
.../parameter/ParametersInterceptor.java | 19 +++----
.../org/apache/struts2/ognl/ThreadAllowlist.java | 11 ++++
.../parameter/StrutsParameterAnnotationTest.java | 10 ++--
10 files changed, 111 insertions(+), 42 deletions(-)
diff --git
a/apps/showcase/src/test/java/it/org/apache/struts2/showcase/ModelDrivenTest.java
b/apps/showcase/src/test/java/it/org/apache/struts2/showcase/ModelDrivenTest.java
new file mode 100644
index 000000000..b3656fc7c
--- /dev/null
+++
b/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");
+ }
+}
diff --git a/core/src/main/java/org/apache/struts2/ModelDriven.java
b/core/src/main/java/org/apache/struts2/ModelDriven.java
index 993a6f5eb..dd21eb2f7 100644
--- a/core/src/main/java/org/apache/struts2/ModelDriven.java
+++ b/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();
}
diff --git a/core/src/main/java/org/apache/struts2/components/Debug.java
b/core/src/main/java/org/apache/struts2/components/Debug.java
index 702f9e32b..65c210fa0 100644
--- a/core/src/main/java/org/apache/struts2/components/Debug.java
+++ b/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 @@ public class Debug extends UIBean {
}
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
diff --git
a/core/src/main/java/org/apache/struts2/components/IteratorComponent.java
b/core/src/main/java/org/apache/struts2/components/IteratorComponent.java
index f45452ddf..c31f02486 100644
--- a/core/src/main/java/org/apache/struts2/components/IteratorComponent.java
+++ b/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 class IteratorComponent extends ContextBean {
stack.push(currentValue);
if (currentValue != null) {
- threadAllowlist.allowClass(currentValue.getClass());
+ threadAllowlist.allowClassHierarchy(currentValue.getClass());
}
String var = getVar();
diff --git
a/core/src/main/java/org/apache/struts2/interceptor/ExceptionMappingInterceptor.java
b/core/src/main/java/org/apache/struts2/interceptor/ExceptionMappingInterceptor.java
index b64bd81e1..acc906500 100644
---
a/core/src/main/java/org/apache/struts2/interceptor/ExceptionMappingInterceptor.java
+++
b/core/src/main/java/org/apache/struts2/interceptor/ExceptionMappingInterceptor.java
@@ -215,9 +215,9 @@ public class ExceptionMappingInterceptor extends
AbstractInterceptor {
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;
diff --git
a/core/src/main/java/org/apache/struts2/interceptor/ModelDrivenInterceptor.java
b/core/src/main/java/org/apache/struts2/interceptor/ModelDrivenInterceptor.java
index fb8cb2dd5..5bdb295ee 100644
---
a/core/src/main/java/org/apache/struts2/interceptor/ModelDrivenInterceptor.java
+++
b/core/src/main/java/org/apache/struts2/interceptor/ModelDrivenInterceptor.java
@@ -20,6 +20,8 @@ package org.apache.struts2.interceptor;
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 @@ import org.apache.struts2.util.ValueStack;
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));
diff --git
a/core/src/main/java/org/apache/struts2/interceptor/debugging/DebuggingInterceptor.java
b/core/src/main/java/org/apache/struts2/interceptor/debugging/DebuggingInterceptor.java
index 1fa9f7b18..ed8b2c922 100644
---
a/core/src/main/java/org/apache/struts2/interceptor/debugging/DebuggingInterceptor.java
+++
b/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.util.reflection.ReflectionProvider;
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 class DebuggingInterceptor extends
AbstractInterceptor {
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());
}
}
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
index 4cd1c3faa..32cffc291 100644
---
a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
+++
b/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 @@ public class ParametersInterceptor extends
MethodFilterInterceptor {
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 @@ public class ParametersInterceptor extends
MethodFilterInterceptor {
}
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 @@ public class ParametersInterceptor extends
MethodFilterInterceptor {
}
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;
}
}
diff --git a/core/src/main/java/org/apache/struts2/ognl/ThreadAllowlist.java
b/core/src/main/java/org/apache/struts2/ognl/ThreadAllowlist.java
index e07cae64e..9fda5734b 100644
--- a/core/src/main/java/org/apache/struts2/ognl/ThreadAllowlist.java
+++ b/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<>());
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java
b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java
index 500574d34..54125fdef 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java
@@ -381,7 +381,6 @@ public class StrutsParameterAnnotationTest {
testParameter(action, "name", true);
testParameter(action, "name.nested", true);
-
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Object.class,
Pojo.class));
}
/**
@@ -402,10 +401,9 @@ public class StrutsParameterAnnotationTest {
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 @@ public class StrutsParameterAnnotationTest {
public Map<String, Pojo> publicPojoMapDepthTwo;
}
- static class MethodAction {
+ public static class MethodAction {
@StrutsParameter
private void setPrivateStr(String str) {
@@ -489,7 +487,7 @@ public class StrutsParameterAnnotationTest {
}
}
- static class ModelAction implements ModelDriven<Pojo> {
+ public static class ModelAction implements ModelDriven<Pojo> {
@Override
public Pojo getModel() {
@@ -497,6 +495,6 @@ public class StrutsParameterAnnotationTest {
}
}
- static class Pojo {
+ public static class Pojo {
}
}
