This is an automated email from the ASF dual-hosted git repository.
kusal pushed a commit to branch WW-5440-convention
in repository https://gitbox.apache.org/repos/asf/struts.git
commit 611933c60b06837938c6c4067db4b6c8a45ab9bb
Author: Kusal Kithul-Godage <g...@kusal.io>
AuthorDate: Sat Jul 13 23:08:46 2024 +1000
WW-5440 Fix OGNL allowlist compat with Convention plugin
---
apps/showcase/pom.xml | 6 ++
.../apache/struts2/showcase/ConventionTest.java | 70 ++++++++++++++++++++++
.../xwork2/config/ConfigurationUtil.java | 13 +++-
.../providers/XmlDocConfigurationProvider.java | 5 +-
.../org/apache/struts2/ognl/ProviderAllowlist.java | 35 ++++++++---
.../apache/struts2/ognl/ProviderAllowlistTest.java | 27 ++++-----
.../convention/ClasspathConfigurationProvider.java | 11 +++-
.../convention/ClasspathPackageProvider.java | 11 ++--
.../PackageBasedActionConfigBuilder.java | 61 ++++++++++++-------
9 files changed, 185 insertions(+), 54 deletions(-)
diff --git a/apps/showcase/pom.xml b/apps/showcase/pom.xml
index 3ea6e3dc8..e1ad28097 100644
--- a/apps/showcase/pom.xml
+++ b/apps/showcase/pom.xml
@@ -143,6 +143,12 @@
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.assertj</groupId>
+ <artifactId>assertj-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+
<!-- BeanValidation Example -->
<dependency>
<groupId>org.hibernate.validator</groupId>
diff --git
a/apps/showcase/src/test/java/it/org/apache/struts2/showcase/ConventionTest.java
b/apps/showcase/src/test/java/it/org/apache/struts2/showcase/ConventionTest.java
new file mode 100644
index 000000000..d42973bfb
--- /dev/null
+++
b/apps/showcase/src/test/java/it/org/apache/struts2/showcase/ConventionTest.java
@@ -0,0 +1,70 @@
+package it.org.apache.struts2.showcase;
+
+import com.gargoylesoftware.htmlunit.WebClient;
+import com.gargoylesoftware.htmlunit.html.HtmlForm;
+import com.gargoylesoftware.htmlunit.html.HtmlPage;
+import com.gargoylesoftware.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 ConventionTest {
+
+ private WebClient webClient;
+
+ @Before
+ public void setUp() throws Exception {
+ webClient = new WebClient();
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ webClient.close();
+ }
+
+ @Test
+ public void listPeople() throws Exception {
+ HtmlPage page = webClient.getPage(ParameterUtils.getBaseUrl() +
"/person/list-people.action");
+
+ assertThat(page.asNormalizedText()).contains(
+ "3\tAlexandru\tPapesco\n" +
+ "4\tJay\tBoss\n" +
+ "5\tRainer\tHermanos\n"
+ );
+ }
+
+ @Test
+ public void editPeople() throws Exception {
+ HtmlPage page = webClient.getPage(ParameterUtils.getBaseUrl() +
"/person/edit-person.action");
+ HtmlForm form = page.getForms().get(0);
+
+ form.getInputByName("persons(1).name").setValue("Lukasz");
+ form.getInputByName("persons(1).lastName").setValue("Lenart");
+ form.getInputByName("persons(2).name").setValue("Kusal");
+ form.getInputByName("persons(2).lastName").setValue("Kithul-Godage");
+
+ HtmlSubmitInput button = form.getInputByValue("Save all persons");
+ page = button.click();
+
+ assertThat(page.asNormalizedText()).contains(
+ "1\tLukasz\tLenart\n" +
+ "2\tKusal\tKithul-Godage\n"
+ );
+ }
+
+ @Test
+ public void createPerson() throws Exception {
+ HtmlPage page = webClient.getPage(ParameterUtils.getBaseUrl() +
"/person/new-person!input.action");
+ HtmlForm form = page.getForms().get(0);
+
+ form.getInputByName("person.name").type("Lukasz");
+ form.getInputByName("person.lastName").type("Lenart");
+
+ HtmlSubmitInput button = form.getInputByValue("Create person");
+ page = button.click();
+
+ assertThat(page.asNormalizedText()).contains("6\tLukasz\tLenart\n");
+ }
+}
diff --git
a/core/src/main/java/com/opensymphony/xwork2/config/ConfigurationUtil.java
b/core/src/main/java/com/opensymphony/xwork2/config/ConfigurationUtil.java
index b85042688..06381c5b0 100644
--- a/core/src/main/java/com/opensymphony/xwork2/config/ConfigurationUtil.java
+++ b/core/src/main/java/com/opensymphony/xwork2/config/ConfigurationUtil.java
@@ -19,18 +19,21 @@
package com.opensymphony.xwork2.config;
import com.opensymphony.xwork2.config.entities.PackageConfig;
+import org.apache.commons.lang3.ClassUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import java.util.ArrayList;
import java.util.Collections;
+import java.util.HashSet;
import java.util.List;
+import java.util.Set;
import java.util.StringTokenizer;
/**
* ConfigurationUtil
- *
+ *
* @author Jason Carreira Created May 23, 2003 11:22:49 PM
*/
public class ConfigurationUtil {
@@ -83,4 +86,12 @@ public class ConfigurationUtil {
return parents;
}
+
+ public static Set<Class<?>> getAllClassTypes(Class<?> clazz) {
+ HashSet<Class<?>> classes = new HashSet<>();
+ classes.add(clazz);
+ classes.addAll(ClassUtils.getAllSuperclasses(clazz));
+ classes.addAll(ClassUtils.getAllInterfaces(clazz));
+ return classes;
+ }
}
diff --git
a/core/src/main/java/com/opensymphony/xwork2/config/providers/XmlDocConfigurationProvider.java
b/core/src/main/java/com/opensymphony/xwork2/config/providers/XmlDocConfigurationProvider.java
index 6de202460..d874f850f 100644
---
a/core/src/main/java/com/opensymphony/xwork2/config/providers/XmlDocConfigurationProvider.java
+++
b/core/src/main/java/com/opensymphony/xwork2/config/providers/XmlDocConfigurationProvider.java
@@ -45,7 +45,6 @@ import
com.opensymphony.xwork2.util.location.LocatableProperties;
import com.opensymphony.xwork2.util.location.Location;
import com.opensymphony.xwork2.util.location.LocationUtils;
import org.apache.commons.lang3.BooleanUtils;
-import org.apache.commons.lang3.ClassUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
@@ -149,9 +148,7 @@ public abstract class XmlDocConfigurationProvider
implements ConfigurationProvid
protected Class<?> allowAndLoadClass(String className) throws
ClassNotFoundException {
Class<?> clazz = loadClass(className);
- allowlistClasses.add(clazz);
- allowlistClasses.addAll(ClassUtils.getAllSuperclasses(clazz));
- allowlistClasses.addAll(ClassUtils.getAllInterfaces(clazz));
+ allowlistClasses.addAll(ConfigurationUtil.getAllClassTypes(clazz));
return clazz;
}
diff --git a/core/src/main/java/org/apache/struts2/ognl/ProviderAllowlist.java
b/core/src/main/java/org/apache/struts2/ognl/ProviderAllowlist.java
index d10372885..93f7506b7 100644
--- a/core/src/main/java/org/apache/struts2/ognl/ProviderAllowlist.java
+++ b/core/src/main/java/org/apache/struts2/ognl/ProviderAllowlist.java
@@ -28,13 +28,14 @@ import java.util.Set;
import static java.util.Collections.unmodifiableSet;
/**
- * Allows {@link ConfigurationProvider}s to register classes that should be
allowed to be used in OGNL expressions.
+ * Allows registration of classes that should be allowed to be used in OGNL
expressions, using a key to identify the
+ * source of the allowlist.
*
* @since 6.4.0
*/
public class ProviderAllowlist {
- private final Map<ConfigurationProvider, Set<Class<?>>> allowlistMap;
+ private final Map<Object, Set<Class<?>>> allowlistMap;
private Set<Class<?>> allowlistClasses;
public ProviderAllowlist() {
@@ -42,24 +43,40 @@ public class ProviderAllowlist {
reconstructAllowlist();
}
- public synchronized void registerAllowlist(ConfigurationProvider
configurationProvider, Set<Class<?>> allowlist) {
- Set<Class<?>> existingAllowlist =
allowlistMap.get(configurationProvider);
+ public synchronized void registerAllowlist(Object key, Set<Class<?>>
allowlist) {
+ Set<Class<?>> existingAllowlist = allowlistMap.get(key);
if (existingAllowlist != null) {
- clearAllowlist(configurationProvider);
+ clearAllowlist(key);
}
- this.allowlistMap.put(configurationProvider, new HashSet<>(allowlist));
+ this.allowlistMap.put(key, new HashSet<>(allowlist));
this.allowlistClasses.addAll(allowlist);
}
- public synchronized void clearAllowlist(ConfigurationProvider
configurationProvider) {
- Set<Class<?>> allowlist = allowlistMap.get(configurationProvider);
+ /**
+ * @deprecated since 6.6.0, use {@link #registerAllowlist(Object, Set)}
+ */
+ @Deprecated
+ public synchronized void registerAllowlist(ConfigurationProvider
configurationProvider, Set<Class<?>> allowlist) {
+ registerAllowlist((Object) configurationProvider, allowlist);
+ }
+
+ public synchronized void clearAllowlist(Object key) {
+ Set<Class<?>> allowlist = allowlistMap.get(key);
if (allowlist == null) {
return;
}
- this.allowlistMap.remove(configurationProvider);
+ this.allowlistMap.remove(key);
reconstructAllowlist();
}
+ /**
+ * @deprecated since 6.6.0, use {@link #clearAllowlist(Object)}
+ */
+ @Deprecated
+ public synchronized void clearAllowlist(ConfigurationProvider
configurationProvider) {
+ clearAllowlist((Object) configurationProvider);
+ }
+
public Set<Class<?>> getProviderAllowlist() {
return unmodifiableSet(allowlistClasses);
}
diff --git
a/core/src/test/java/org/apache/struts2/ognl/ProviderAllowlistTest.java
b/core/src/test/java/org/apache/struts2/ognl/ProviderAllowlistTest.java
index baf91b9b5..2584d16f4 100644
--- a/core/src/test/java/org/apache/struts2/ognl/ProviderAllowlistTest.java
+++ b/core/src/test/java/org/apache/struts2/ognl/ProviderAllowlistTest.java
@@ -18,7 +18,6 @@
*/
package org.apache.struts2.ognl;
-import com.opensymphony.xwork2.config.ConfigurationProvider;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
@@ -39,10 +38,10 @@ public class ProviderAllowlistTest {
private ProviderAllowlist providerAllowlist;
@Mock
- private ConfigurationProvider provider1;
+ private Object key1;
@Mock
- private ConfigurationProvider provider2;
+ private Object key2;
@Before
public void setUp() throws Exception {
@@ -51,37 +50,37 @@ public class ProviderAllowlistTest {
@Test
public void registerAllowlist() {
- providerAllowlist.registerAllowlist(provider1, new
HashSet<>(asList(String.class, Integer.class)));
- providerAllowlist.registerAllowlist(provider2, new
HashSet<>(asList(Double.class, Integer.class)));
+ providerAllowlist.registerAllowlist(key1, new
HashSet<>(asList(String.class, Integer.class)));
+ providerAllowlist.registerAllowlist(key2, new
HashSet<>(asList(Double.class, Integer.class)));
assertThat(providerAllowlist.getProviderAllowlist()).containsExactlyInAnyOrder(String.class,
Integer.class, Double.class);
}
@Test
public void registerAllowlist_twice() {
- providerAllowlist.registerAllowlist(provider1, new
HashSet<>(asList(String.class, Integer.class)));
- providerAllowlist.registerAllowlist(provider1, new
HashSet<>(asList(Double.class, Integer.class)));
+ providerAllowlist.registerAllowlist(key1, new
HashSet<>(asList(String.class, Integer.class)));
+ providerAllowlist.registerAllowlist(key1, new
HashSet<>(asList(Double.class, Integer.class)));
assertThat(providerAllowlist.getProviderAllowlist()).containsExactlyInAnyOrder(Integer.class,
Double.class);
}
@Test
public void clearAllowlist() {
- providerAllowlist.registerAllowlist(provider1, new
HashSet<>(asList(String.class, Integer.class)));
- providerAllowlist.registerAllowlist(provider2, new
HashSet<>(asList(Double.class, Integer.class)));
+ providerAllowlist.registerAllowlist(key1, new
HashSet<>(asList(String.class, Integer.class)));
+ providerAllowlist.registerAllowlist(key2, new
HashSet<>(asList(Double.class, Integer.class)));
- providerAllowlist.clearAllowlist(provider1);
+ providerAllowlist.clearAllowlist(key1);
assertThat(providerAllowlist.getProviderAllowlist()).containsExactlyInAnyOrder(Integer.class,
Double.class);
}
@Test
public void clearAllowlist_both() {
- providerAllowlist.registerAllowlist(provider1, new
HashSet<>(asList(String.class, Integer.class)));
- providerAllowlist.registerAllowlist(provider2, new
HashSet<>(asList(Double.class, Integer.class)));
+ providerAllowlist.registerAllowlist(key1, new
HashSet<>(asList(String.class, Integer.class)));
+ providerAllowlist.registerAllowlist(key2, new
HashSet<>(asList(Double.class, Integer.class)));
- providerAllowlist.clearAllowlist(provider1);
- providerAllowlist.clearAllowlist(provider2);
+ providerAllowlist.clearAllowlist(key1);
+ providerAllowlist.clearAllowlist(key2);
assertThat(providerAllowlist.getProviderAllowlist()).isEmpty();
}
diff --git
a/plugins/convention/src/main/java/org/apache/struts2/convention/ClasspathConfigurationProvider.java
b/plugins/convention/src/main/java/org/apache/struts2/convention/ClasspathConfigurationProvider.java
index f3d873322..71ac094cf 100644
---
a/plugins/convention/src/main/java/org/apache/struts2/convention/ClasspathConfigurationProvider.java
+++
b/plugins/convention/src/main/java/org/apache/struts2/convention/ClasspathConfigurationProvider.java
@@ -36,7 +36,7 @@ import org.apache.struts2.dispatcher.DispatcherListener;
* </p>
*/
public class ClasspathConfigurationProvider implements ConfigurationProvider,
DispatcherListener {
- private ActionConfigBuilder actionConfigBuilder;
+ private final ActionConfigBuilder actionConfigBuilder;
private boolean devMode;
private boolean reload;
private boolean listeningToDispatcher;
@@ -59,6 +59,7 @@ public class ClasspathConfigurationProvider implements
ConfigurationProvider, Di
/**
* Not used.
*/
+ @Override
public void destroy() {
if (this.listeningToDispatcher) {
Dispatcher.removeDispatcherListener(this);
@@ -71,6 +72,7 @@ public class ClasspathConfigurationProvider implements
ConfigurationProvider, Di
*
* @param configuration configuration
*/
+ @Override
public void init(Configuration configuration) {
if (devMode && reload && !listeningToDispatcher) {
//this is the only way I found to be able to get added to to
ConfigurationProvider list
@@ -88,6 +90,7 @@ public class ClasspathConfigurationProvider implements
ConfigurationProvider, Di
*
* @throws ConfigurationException in case of configuration errors
*/
+ @Override
public void register(ContainerBuilder containerBuilder,
LocatableProperties locatableProperties)
throws ConfigurationException {
}
@@ -97,20 +100,24 @@ public class ClasspathConfigurationProvider implements
ConfigurationProvider, Di
*
* @throws ConfigurationException in case of configuration errors
*/
+ @Override
public void loadPackages() throws ConfigurationException {
}
/**
* @return true if devMode, reload and actionConfigBuilder.needsReload()
*/
+ @Override
public boolean needsReload() {
return devMode && reload && actionConfigBuilder.needsReload();
}
+ @Override
public void dispatcherInitialized(Dispatcher du) {
du.getConfigurationManager().addContainerProvider(this);
}
+ @Override
public void dispatcherDestroyed(Dispatcher du) {
}
-}
\ No newline at end of file
+}
diff --git
a/plugins/convention/src/main/java/org/apache/struts2/convention/ClasspathPackageProvider.java
b/plugins/convention/src/main/java/org/apache/struts2/convention/ClasspathPackageProvider.java
index 7b314c05b..3ffec8488 100644
---
a/plugins/convention/src/main/java/org/apache/struts2/convention/ClasspathPackageProvider.java
+++
b/plugins/convention/src/main/java/org/apache/struts2/convention/ClasspathPackageProvider.java
@@ -18,11 +18,11 @@
*/
package org.apache.struts2.convention;
-import com.opensymphony.xwork2.config.PackageProvider;
import com.opensymphony.xwork2.config.Configuration;
import com.opensymphony.xwork2.config.ConfigurationException;
-import com.opensymphony.xwork2.inject.Inject;
+import com.opensymphony.xwork2.config.PackageProvider;
import com.opensymphony.xwork2.inject.Container;
+import com.opensymphony.xwork2.inject.Inject;
/**
* <p>
@@ -34,20 +34,23 @@ import com.opensymphony.xwork2.inject.Container;
* </p>
*/
public class ClasspathPackageProvider implements PackageProvider {
- private ActionConfigBuilder actionConfigBuilder;
+ private final ActionConfigBuilder actionConfigBuilder;
@Inject
public ClasspathPackageProvider(Container container) {
this.actionConfigBuilder =
container.getInstance(ActionConfigBuilder.class,
container.getInstance(String.class,
ConventionConstants.CONVENTION_ACTION_CONFIG_BUILDER));
}
+ @Override
public void init(Configuration configuration) throws
ConfigurationException {
}
+ @Override
public boolean needsReload() {
- return actionConfigBuilder.needsReload();
+ return actionConfigBuilder.needsReload();
}
+ @Override
public void loadPackages() throws ConfigurationException {
actionConfigBuilder.buildActionConfigs();
}
diff --git
a/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java
b/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java
index 460cab49e..71331d4ef 100644
---
a/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java
+++
b/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java
@@ -24,6 +24,7 @@ import com.opensymphony.xwork2.FileManagerFactory;
import com.opensymphony.xwork2.ObjectFactory;
import com.opensymphony.xwork2.config.Configuration;
import com.opensymphony.xwork2.config.ConfigurationException;
+import com.opensymphony.xwork2.config.ConfigurationUtil;
import com.opensymphony.xwork2.config.entities.ActionConfig;
import com.opensymphony.xwork2.config.entities.ExceptionMappingConfig;
import com.opensymphony.xwork2.config.entities.InterceptorMapping;
@@ -57,6 +58,7 @@ import
org.apache.struts2.convention.annotation.ExceptionMappings;
import org.apache.struts2.convention.annotation.Namespace;
import org.apache.struts2.convention.annotation.Namespaces;
import org.apache.struts2.convention.annotation.ParentPackage;
+import org.apache.struts2.ognl.ProviderAllowlist;
import java.io.IOException;
import java.lang.reflect.Method;
@@ -125,6 +127,9 @@ public class PackageBasedActionConfigBuilder implements
ActionConfigBuilder {
private FileManager fileManager;
private ClassFinderFactory classFinderFactory;
+ private final Set<Class<?>> allowlistClasses = new HashSet<>();
+ private ProviderAllowlist providerAllowlist;
+
/**
* Constructs actions based on a list of packages.
*
@@ -167,6 +172,11 @@ public class PackageBasedActionConfigBuilder implements
ActionConfigBuilder {
this.devMode = BooleanUtils.toBoolean(mode);
}
+ @Inject
+ public void setProviderAllowlist(ProviderAllowlist providerAllowlist) {
+ this.providerAllowlist = providerAllowlist;
+ }
+
/**
* @param reload Reload configuration when classes change. Defaults to
"false" and should not be used
* in production.
@@ -345,33 +355,38 @@ public class PackageBasedActionConfigBuilder implements
ActionConfigBuilder {
* annotation which is used to control the parent package for a specific
action. Lastly, the
* {@link ResultMapBuilder} is used to create ResultConfig instances of
the action.
*/
+ @Override
public void buildActionConfigs() {
+ allowlistClasses.clear();
+
//setup reload class loader based on dev settings
initReloadClassLoader();
- if (!disableActionScanning) {
- if (actionPackages == null && packageLocators == null) {
- throw new ConfigurationException("At least a list of action
packages or action package locators " +
- "must be given using one of the properties
[struts.convention.action.packages] or " +
- "[struts.convention.package.locators]");
- }
+ if (disableActionScanning) {
+ return;
+ }
- if (LOG.isTraceEnabled()) {
- LOG.trace("Loading action configurations");
- if (actionPackages != null) {
- LOG.trace("Actions being loaded from action packages: {}",
(Object[]) actionPackages);
- }
- if (packageLocators != null) {
- LOG.trace("Actions being loaded using package locator's:
{}", (Object[]) packageLocators);
- }
- if (excludePackages != null) {
- LOG.trace("Excluding actions from packages: {}",
(Object[]) excludePackages);
- }
- }
+ if (actionPackages == null && packageLocators == null) {
+ throw new ConfigurationException("At least a list of action
packages or action package locators " +
+ "must be given using one of the properties
[struts.convention.action.packages] or " +
+ "[struts.convention.package.locators]");
+ }
- Set<Class<?>> classes = findActions();
- buildConfiguration(classes);
+ if (LOG.isTraceEnabled()) {
+ LOG.trace("Loading action configurations");
+ if (actionPackages != null) {
+ LOG.trace("Actions being loaded from action packages: {}",
(Object[]) actionPackages);
+ }
+ if (packageLocators != null) {
+ LOG.trace("Actions being loaded using package locator's: {}",
(Object[]) packageLocators);
+ }
+ if (excludePackages != null) {
+ LOG.trace("Excluding actions from packages: {}", (Object[])
excludePackages);
+ }
}
+
+ Set<Class<?>> classes = findActions();
+ buildConfiguration(classes);
}
protected ClassLoaderInterface getClassLoaderInterface() {
@@ -765,7 +780,10 @@ public class PackageBasedActionConfigBuilder implements
ActionConfigBuilder {
} else if (actionAnnotation != null)
createActionConfig(defaultPackageConfig, actionClass,
defaultActionName, methodName, actionAnnotation, allowedMethods);
}
+
+
allowlistClasses.addAll(ConfigurationUtil.getAllClassTypes(actionClass));
}
+ providerAllowlist.registerAllowlist(this, allowlistClasses);
buildIndexActions(packageConfigs);
@@ -1153,10 +1171,13 @@ public class PackageBasedActionConfigBuilder implements
ActionConfigBuilder {
}
}
+ @Override
public void destroy() {
loadedFileUrls.clear();
+ providerAllowlist.clearAllowlist(this);
}
+ @Override
public boolean needsReload() {
if (devMode && reload) {
for (String url : loadedFileUrls) {
