WW-5440 Fix OGNL allowlist compat with Convention plugin

This commit is contained in:
Kusal Kithul-Godage
2024-07-13 23:08:46 +10:00
parent 8b4c188d0f
commit cb62cb4e9a
10 changed files with 250 additions and 63 deletions
+6
View File
@@ -137,6 +137,12 @@
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>net.sourceforge.htmlunit</groupId>
<artifactId>htmlunit</artifactId>
@@ -0,0 +1,88 @@
/*
* 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 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");
}
}
@@ -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;
}
}
@@ -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;
}
@@ -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);
}
@@ -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();
}
@@ -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) {
}
}
}
@@ -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();
}
@@ -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 (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);
if (disableActionScanning) {
return;
}
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 (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) {
@@ -18,9 +18,21 @@
*/
package org.apache.struts2.convention;
import com.opensymphony.xwork2.*;
import com.opensymphony.xwork2.ActionChainResult;
import com.opensymphony.xwork2.ActionContext;
import com.opensymphony.xwork2.FileManager;
import com.opensymphony.xwork2.FileManagerFactory;
import com.opensymphony.xwork2.ObjectFactory;
import com.opensymphony.xwork2.Result;
import com.opensymphony.xwork2.config.Configuration;
import com.opensymphony.xwork2.config.entities.*;
import com.opensymphony.xwork2.config.entities.ActionConfig;
import com.opensymphony.xwork2.config.entities.ExceptionMappingConfig;
import com.opensymphony.xwork2.config.entities.InterceptorConfig;
import com.opensymphony.xwork2.config.entities.InterceptorMapping;
import com.opensymphony.xwork2.config.entities.InterceptorStackConfig;
import com.opensymphony.xwork2.config.entities.PackageConfig;
import com.opensymphony.xwork2.config.entities.ResultConfig;
import com.opensymphony.xwork2.config.entities.ResultTypeConfig;
import com.opensymphony.xwork2.config.impl.DefaultConfiguration;
import com.opensymphony.xwork2.factory.DefaultInterceptorFactory;
import com.opensymphony.xwork2.factory.DefaultResultFactory;
@@ -32,12 +44,20 @@ import com.opensymphony.xwork2.util.fs.DefaultFileManager;
import com.opensymphony.xwork2.util.fs.DefaultFileManagerFactory;
import com.opensymphony.xwork2.util.reflection.ReflectionException;
import junit.framework.TestCase;
import org.apache.commons.lang3.StringUtils;
import org.apache.struts2.convention.actions.DefaultResultPathAction;
import org.apache.struts2.convention.actions.NoAnnotationAction;
import org.apache.struts2.convention.actions.Skip;
import org.apache.struts2.convention.actions.action.*;
import org.apache.struts2.convention.actions.action.ActionNameAction;
import org.apache.struts2.convention.actions.action.ActionNamesAction;
import org.apache.struts2.convention.actions.action.ClassLevelAnnotationAction;
import org.apache.struts2.convention.actions.action.ClassLevelAnnotationDefaultMethodAction;
import org.apache.struts2.convention.actions.action.ClassLevelAnnotationsAction;
import org.apache.struts2.convention.actions.action.ClassLevelAnnotationsDefaultMethodAction;
import org.apache.struts2.convention.actions.action.ClassNameAction;
import org.apache.struts2.convention.actions.action.SingleActionNameAction;
import org.apache.struts2.convention.actions.action.TestAction;
import org.apache.struts2.convention.actions.action.TestExtends;
import org.apache.struts2.convention.actions.allowedmethods.ClassLevelAllowedMethodsAction;
import org.apache.struts2.convention.actions.allowedmethods.PackageLevelAllowedMethodsAction;
import org.apache.struts2.convention.actions.allowedmethods.sub.PackageLevelAllowedMethodsChildAction;
@@ -61,7 +81,15 @@ import org.apache.struts2.convention.actions.parentpackage.ClassLevelParentPacka
import org.apache.struts2.convention.actions.parentpackage.PackageLevelParentPackageAction;
import org.apache.struts2.convention.actions.parentpackage.sub.ClassLevelParentPackageChildAction;
import org.apache.struts2.convention.actions.parentpackage.sub.PackageLevelParentPackageChildAction;
import org.apache.struts2.convention.actions.result.*;
import org.apache.struts2.convention.actions.result.ActionLevelResultAction;
import org.apache.struts2.convention.actions.result.ActionLevelResultsAction;
import org.apache.struts2.convention.actions.result.ActionLevelResultsNamesAction;
import org.apache.struts2.convention.actions.result.ClassLevelResultAction;
import org.apache.struts2.convention.actions.result.ClassLevelResultsAction;
import org.apache.struts2.convention.actions.result.GlobalResultAction;
import org.apache.struts2.convention.actions.result.GlobalResultOverrideAction;
import org.apache.struts2.convention.actions.result.InheritedResultExtends;
import org.apache.struts2.convention.actions.result.OverrideResultAction;
import org.apache.struts2.convention.actions.resultpath.ClassLevelResultPathAction;
import org.apache.struts2.convention.actions.resultpath.PackageLevelResultPathAction;
import org.apache.struts2.convention.actions.skip.Index;
@@ -69,15 +97,24 @@ import org.apache.struts2.convention.actions.transactions.TransNameAction;
import org.apache.struts2.convention.annotation.Action;
import org.apache.struts2.convention.annotation.Actions;
import org.apache.struts2.convention.dontfind.DontFindMeAction;
import org.apache.struts2.ognl.ProviderAllowlist;
import org.apache.struts2.result.ServletDispatcherResult;
import org.easymock.EasyMock;
import javax.servlet.ServletContext;
import java.net.MalformedURLException;
import java.util.*;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import static org.apache.struts2.convention.ReflectionTools.getAnnotation;
import static org.easymock.EasyMock.*;
import static org.easymock.EasyMock.checkOrder;
import static org.easymock.EasyMock.createStrictMock;
import static org.easymock.EasyMock.expect;
import static org.easymock.EasyMock.verify;
/**
* <p>
@@ -388,6 +425,7 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
fileManagerFactory.setFileManager(new DefaultFileManager());
builder.setFileManagerFactory(fileManagerFactory);
builder.setPackageLocatorsBase("org.apache.struts2.convention.actions");
builder.setProviderAllowlist(new ProviderAllowlist());
builder.buildActionConfigs();
verify(resultMapBuilder);