From ebdf019959594c6f2f2c5bcce655c91a1f36eb4c Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Thu, 23 Nov 2023 21:12:15 +1100 Subject: [PATCH 01/14] WW-5364 Modify XmlDocConfigurationProvider to be able to load into allowlist --- .../XmlDocConfigurationProvider.java | 59 ++++++++++++++----- 1 file changed, 43 insertions(+), 16 deletions(-) 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 0fcdac0ed..030bd470d 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,6 +45,7 @@ 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; @@ -64,6 +65,7 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.function.Consumer; +import java.util.stream.Stream; import static com.opensymphony.xwork2.util.TextParseUtil.commaDelimitedStringToSet; import static java.lang.Boolean.parseBoolean; @@ -135,6 +137,16 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid public void destroy() { } + protected Class allowAndLoadClass(String className) throws ClassNotFoundException { + Class clazz = loadClass(className); + List> superClasses = ClassUtils.getAllSuperclasses(clazz); + List> interfaces = ClassUtils.getAllInterfaces(clazz); + Stream.concat(superClasses.stream(), interfaces.stream()).forEach(c -> { + + }); + return clazz; + } + protected Class loadClass(String className) throws ClassNotFoundException { return objectFactory.getClassInstance(className); } @@ -197,7 +209,7 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid String name = child.getAttribute("name"); String impl = child.getAttribute("class"); try { - Class classImpl = loadClass(impl); + Class classImpl = ClassLoaderUtil.loadClass(impl, getClass()); if (BeanSelectionProvider.class.isAssignableFrom(classImpl)) { BeanSelectionProvider provider = (BeanSelectionProvider) classImpl.newInstance(); provider.register(containerBuilder, props); @@ -442,13 +454,8 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid Location location = DomHelper.getLocationObject(actionElement); - if (location == null) { - LOG.warn("Location null for {}", className); - } - - if (!className.isEmpty() && !verifyAction(className, name, location)) { - LOG.error("Unable to verify action [{}] with class [{}], from [{}]", name, className, location); - return; + if (!className.isEmpty()) { + verifyAction(className, name, location); } Map results; @@ -499,17 +506,18 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid */ @Deprecated protected boolean verifyAction(String className, String name, Location loc) { - return verifyAction(className, loc); + verifyAction(className, loc); + return true; } - protected boolean verifyAction(String className, Location loc) { + protected void verifyAction(String className, Location loc) { if (className.contains("{")) { LOG.debug("Action class [{}] contains a wildcard replacement value, so it can't be verified", className); - return true; + return; } try { + Class clazz = allowAndLoadClass(className); if (objectFactory.isNoArgConstructorRequired()) { - Class clazz = loadClass(className); if (!Modifier.isPublic(clazz.getModifiers())) { throw new ConfigurationException("Action class [" + className + "] is not public", loc); } @@ -530,7 +538,6 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid LOG.debug("Unable to verify action class [{}]", className, ex); throw new ConfigurationException(ex, loc); } - return true; } protected void addResultTypes(PackageConfig.Builder packageContext, Element element) { @@ -572,7 +579,7 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid protected Class verifyResultType(String className, Location loc) { try { - return loadClass(className); + return allowAndLoadClass(className); } catch (ClassNotFoundException | NoClassDefFoundError e) { LOG.warn("Result class [{}] doesn't exist ({}) at {}, ignoring", className, e.getClass().getSimpleName(), loc, e); } @@ -888,7 +895,12 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid NodeList defaultClassRefList = element.getElementsByTagName("default-class-ref"); if (defaultClassRefList.getLength() > 0) { Element defaultClassRefElement = (Element) defaultClassRefList.item(0); - packageContext.defaultClassRef(defaultClassRefElement.getAttribute("class")); + + String className = defaultClassRefElement.getAttribute("class"); + Location location = DomHelper.getLocationObject(defaultClassRefElement); + verifyAction(className, location); + + packageContext.defaultClassRef(className); } } @@ -927,10 +939,25 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid iterateChildrenByTagName( element, "interceptor", - interceptorElement -> context.addInterceptorConfig(buildInterceptorConfig(interceptorElement))); + interceptorElement -> { + String className = interceptorElement.getAttribute("class"); + Location location = DomHelper.getLocationObject(interceptorElement); + + verifyInterceptor(className, location); + + context.addInterceptorConfig(buildInterceptorConfig(interceptorElement)); + }); loadInterceptorStacks(element, context); } + protected void verifyInterceptor(String className, Location loc) { + try { + allowAndLoadClass(className); + } catch (ClassNotFoundException | NoClassDefFoundError e) { + LOG.warn("Interceptor class [{}] doesn't exist at {}, ignoring", className, loc, e); + } + } + protected InterceptorConfig buildInterceptorConfig(Element interceptorElement) { String interceptorName = interceptorElement.getAttribute("name"); String className = interceptorElement.getAttribute("class"); From 1d76bff95e10f36cdaa7ddeebf92a67834918841 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 24 Nov 2023 10:58:04 +1100 Subject: [PATCH 02/14] WW-5364 Make allowlist classloader specific --- .../xwork2/ognl/SecurityMemberAccess.java | 7 ++++--- .../com/opensymphony/xwork2/util/ConfigParseUtil.java | 11 +++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index c68423014..0fa118829 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -39,6 +39,7 @@ import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; +import static com.opensymphony.xwork2.util.ConfigParseUtil.toClassObjectsSet; import static com.opensymphony.xwork2.util.ConfigParseUtil.toClassesSet; import static com.opensymphony.xwork2.util.ConfigParseUtil.toNewClassesSet; import static com.opensymphony.xwork2.util.ConfigParseUtil.toNewPackageNamesSet; @@ -64,7 +65,7 @@ public class SecurityMemberAccess implements MemberAccess { private Set excludedPackageNames = emptySet(); private Set excludedPackageExemptClasses = emptySet(); private boolean enforceAllowlistEnabled = false; - private Set allowlistClasses = emptySet(); + private Set> allowlistClasses = emptySet(); private Set allowlistPackageNames = emptySet(); private boolean disallowProxyMemberAccess = false; private boolean disallowDefaultPackageAccess = false; @@ -199,7 +200,7 @@ public class SecurityMemberAccess implements MemberAccess { } protected boolean isClassAllowlisted(Class clazz) { - return allowlistClasses.contains(clazz.getName()) || isClassBelongsToPackages(clazz, allowlistPackageNames); + return allowlistClasses.contains(clazz) || isClassBelongsToPackages(clazz, allowlistPackageNames); } /** @@ -406,7 +407,7 @@ public class SecurityMemberAccess implements MemberAccess { @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_CLASSES, required = false) public void useAllowlistClasses(String commaDelimitedClasses) { - this.allowlistClasses = toClassesSet(commaDelimitedClasses); + this.allowlistClasses = toClassObjectsSet(commaDelimitedClasses); } @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false) diff --git a/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java b/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java index 53475b7a8..5003063e1 100644 --- a/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java @@ -40,6 +40,11 @@ public class ConfigParseUtil { return unmodifiableSet(classNames); } + public static Set> toClassObjectsSet(String newDelimitedClasses) throws ConfigurationException { + Set classNames = commaDelimitedStringToSet(newDelimitedClasses); + return unmodifiableSet(validateClasses(classNames, OgnlUtil.class.getClassLoader())); + } + public static Set toNewClassesSet(Set oldClasses, String newDelimitedClasses) throws ConfigurationException { Set classNames = commaDelimitedStringToSet(newDelimitedClasses); validateClasses(classNames, OgnlUtil.class.getClassLoader()); @@ -61,14 +66,16 @@ public class ConfigParseUtil { return unmodifiableSet(newPatterns); } - public static void validateClasses(Set classNames, ClassLoader validatingClassLoader) throws ConfigurationException { + public static Set> validateClasses(Set classNames, ClassLoader validatingClassLoader) throws ConfigurationException { + Set> classes = new HashSet<>(); for (String className : classNames) { try { - validatingClassLoader.loadClass(className); + classes.add(validatingClassLoader.loadClass(className)); } catch (ClassNotFoundException e) { throw new ConfigurationException("Cannot load class for exclusion/exemption configuration: " + className, e); } } + return classes; } public static Set toPackageNamesSet(String newDelimitedPackageNames) throws ConfigurationException { From 198812fe8b813e33bcbaab21d065cf976c7fe620 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 24 Nov 2023 19:15:40 +1100 Subject: [PATCH 03/14] WW-5364 Implement provider allowlist --- .../config/impl/DefaultConfiguration.java | 2 + .../StrutsDefaultConfigurationProvider.java | 2 + .../XmlDocConfigurationProvider.java | 27 +++++--- .../struts2/ognl/ProviderAllowlist.java | 61 +++++++++++++++++++ core/src/main/resources/struts-beans.xml | 1 + core/src/main/resources/struts-default.xml | 2 - 6 files changed, 83 insertions(+), 12 deletions(-) create mode 100644 core/src/main/java/org/apache/struts2/ognl/ProviderAllowlist.java diff --git a/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java b/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java index d0cbcef1c..39bb55f8f 100644 --- a/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java +++ b/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java @@ -107,6 +107,7 @@ import org.apache.struts2.conversion.StrutsConversionPropertiesProcessor; import org.apache.struts2.conversion.StrutsTypeConverterCreator; import org.apache.struts2.conversion.StrutsTypeConverterHolder; import org.apache.struts2.ognl.OgnlGuard; +import org.apache.struts2.ognl.ProviderAllowlist; import org.apache.struts2.ognl.StrutsOgnlGuard; import java.util.ArrayList; @@ -388,6 +389,7 @@ public class DefaultConfiguration implements Configuration { builder.factory(OgnlUtil.class, Scope.SINGLETON); builder.factory(SecurityMemberAccess.class, Scope.PROTOTYPE); builder.factory(OgnlGuard.class, StrutsOgnlGuard.class, Scope.SINGLETON); + builder.factory(ProviderAllowlist.class, Scope.SINGLETON); builder.factory(ValueSubstitutor.class, EnvsValueSubstitutor.class, Scope.SINGLETON); diff --git a/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java b/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java index 09eeb7c85..88a152141 100644 --- a/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java +++ b/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java @@ -123,6 +123,7 @@ import org.apache.struts2.dispatcher.Parameter; import org.apache.struts2.interceptor.exec.ExecutorProvider; import org.apache.struts2.interceptor.exec.StrutsExecutorProvider; import org.apache.struts2.ognl.OgnlGuard; +import org.apache.struts2.ognl.ProviderAllowlist; import org.apache.struts2.ognl.StrutsOgnlGuard; import org.apache.struts2.url.QueryStringBuilder; import org.apache.struts2.url.QueryStringParser; @@ -233,6 +234,7 @@ public class StrutsDefaultConfigurationProvider implements ConfigurationProvider .factory(OgnlUtil.class, Scope.SINGLETON) .factory(SecurityMemberAccess.class, Scope.PROTOTYPE) .factory(OgnlGuard.class, StrutsOgnlGuard.class, Scope.SINGLETON) + .factory(ProviderAllowlist.class, Scope.SINGLETON) .factory(CollectionConverter.class, Scope.SINGLETON) .factory(ArrayConverter.class, Scope.SINGLETON) .factory(DateConverter.class, Scope.SINGLETON) 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 030bd470d..2a342c521 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 @@ -49,6 +49,7 @@ 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 org.apache.struts2.ognl.ProviderAllowlist; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.w3c.dom.Node; @@ -94,8 +95,10 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid protected ObjectFactory objectFactory; protected Map dtdMappings = new HashMap<>(); protected Configuration configuration; + protected ProviderAllowlist providerAllowlist; protected boolean throwExceptionOnDuplicateBeans = true; protected ValueSubstitutor valueSubstitutor; + protected Set> allowlistClasses = new HashSet<>(); @Inject public void setObjectFactory(ObjectFactory objectFactory) { @@ -133,17 +136,22 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid this.configuration = configuration; } + private void registerAllowlist() { + providerAllowlist = configuration.getContainer().getInstance(ProviderAllowlist.class); + providerAllowlist.registerAllowlist(this, allowlistClasses); + } + @Override public void destroy() { + providerAllowlist.clearAllowlist(this); } protected Class allowAndLoadClass(String className) throws ClassNotFoundException { Class clazz = loadClass(className); + allowlistClasses.add(clazz); List> superClasses = ClassUtils.getAllSuperclasses(clazz); List> interfaces = ClassUtils.getAllInterfaces(clazz); - Stream.concat(superClasses.stream(), interfaces.stream()).forEach(c -> { - - }); + Stream.concat(superClasses.stream(), interfaces.stream()).forEach(allowlistClasses::add); return clazz; } @@ -181,6 +189,7 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid @Override public void register(ContainerBuilder containerBuilder, LocatableProperties props) throws ConfigurationException { + allowlistClasses.clear(); Map loadedBeans = new HashMap<>(); for (Document doc : documents) { iterateElementChildren(doc, child -> { @@ -315,7 +324,7 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid loadExtraConfiguration(doc); } - if (reloads.size() > 0) { + if (!reloads.isEmpty()) { reloadRequiredPackages(reloads); } @@ -324,6 +333,7 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid } declaredPackages.clear(); + registerAllowlist(); configuration = null; } @@ -524,10 +534,8 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid clazz.getConstructor(); } } catch (ClassNotFoundException e) { - LOG.debug("Class not found for action [{}]", className, e); - throw new ConfigurationException("Action class [" + className + "] not found", loc); + throw new ConfigurationException("Action class [" + className + "] not found", e, loc); } catch (NoSuchMethodException e) { - LOG.debug("No constructor found for action [{}]", className, e); throw new ConfigurationException("Action class [" + className + "] does not have a public no-arg constructor", e, loc); } catch (RuntimeException ex) { // Probably not a big deal, like request or session-scoped Spring beans that need a real request @@ -581,9 +589,8 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid try { return allowAndLoadClass(className); } catch (ClassNotFoundException | NoClassDefFoundError e) { - LOG.warn("Result class [{}] doesn't exist ({}) at {}, ignoring", className, e.getClass().getSimpleName(), loc, e); + throw new ConfigurationException("Result class [" + className + "] not found", e, loc); } - return null; } /** @@ -954,7 +961,7 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid try { allowAndLoadClass(className); } catch (ClassNotFoundException | NoClassDefFoundError e) { - LOG.warn("Interceptor class [{}] doesn't exist at {}, ignoring", className, loc, e); + throw new ConfigurationException("Interceptor class [" + className + "] not found", e, loc); } } diff --git a/core/src/main/java/org/apache/struts2/ognl/ProviderAllowlist.java b/core/src/main/java/org/apache/struts2/ognl/ProviderAllowlist.java new file mode 100644 index 000000000..82ebaabc3 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/ognl/ProviderAllowlist.java @@ -0,0 +1,61 @@ +/* + * 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 org.apache.struts2.ognl; + +import com.opensymphony.xwork2.config.ConfigurationProvider; + +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +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. + * + * @since 6.4.0 + */ +public class ProviderAllowlist { + + private final Map>> allowlistMap = new HashMap<>(); + private final Set> allowlistClasses = new HashSet<>(); + + public synchronized void registerAllowlist(ConfigurationProvider configurationProvider, Set> allowlist) { + Set> existingAllowlist = allowlistMap.get(configurationProvider); + if (existingAllowlist != null) { + clearAllowlist(configurationProvider); + } + this.allowlistMap.put(configurationProvider, new HashSet<>(allowlist)); + this.allowlistClasses.addAll(allowlist); + } + + public synchronized void clearAllowlist(ConfigurationProvider configurationProvider) { + Set> allowlist = allowlistMap.get(configurationProvider); + if (allowlist == null) { + return; + } + this.allowlistClasses.removeAll(allowlist); + this.allowlistMap.remove(configurationProvider); + } + + public Set> getProviderAllowlist() { + return unmodifiableSet(allowlistClasses); + } +} diff --git a/core/src/main/resources/struts-beans.xml b/core/src/main/resources/struts-beans.xml index 273b43b87..8c751c0c1 100644 --- a/core/src/main/resources/struts-beans.xml +++ b/core/src/main/resources/struts-beans.xml @@ -169,6 +169,7 @@ + diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index 90657630e..0fdcc2b37 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -44,8 +44,6 @@ - Date: Fri, 24 Nov 2023 19:24:10 +1100 Subject: [PATCH 04/14] WW-5364 Inject ProviderAllowlist into SecurityMemberAccess --- .../xwork2/ognl/SecurityMemberAccess.java | 15 ++++++++++----- .../xwork2/ognl/SecurityMemberAccessTest.java | 8 +++++++- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index 0fa118829..c7d941eeb 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -25,6 +25,7 @@ import org.apache.commons.lang3.BooleanUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.StrutsConstants; +import org.apache.struts2.ognl.ProviderAllowlist; import java.lang.reflect.AccessibleObject; import java.lang.reflect.Field; @@ -57,6 +58,7 @@ public class SecurityMemberAccess implements MemberAccess { private static final Logger LOG = LogManager.getLogger(SecurityMemberAccess.class); + private final ProviderAllowlist providerAllowlist; private final boolean allowStaticFieldAccess; private Set excludeProperties = emptySet(); private Set acceptProperties = emptySet(); @@ -71,8 +73,10 @@ public class SecurityMemberAccess implements MemberAccess { private boolean disallowDefaultPackageAccess = false; @Inject - public SecurityMemberAccess(@Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS) String allowStaticFieldAccess) { - this(BooleanUtils.toBoolean(allowStaticFieldAccess)); + public SecurityMemberAccess(@Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS) String allowStaticFieldAccess, @Inject ProviderAllowlist providerAllowlist) { + this.allowStaticFieldAccess = BooleanUtils.toBoolean(allowStaticFieldAccess); + useExcludedClasses(""); // Initialise default exclusions + this.providerAllowlist = providerAllowlist; } /** @@ -83,8 +87,7 @@ public class SecurityMemberAccess implements MemberAccess { * @param allowStaticFieldAccess if set to true static fields (constants) will be accessible */ public SecurityMemberAccess(boolean allowStaticFieldAccess) { - this.allowStaticFieldAccess = allowStaticFieldAccess; - useExcludedClasses(""); // Initialise default exclusions + this(String.valueOf(allowStaticFieldAccess), null); } @Override @@ -200,7 +203,9 @@ public class SecurityMemberAccess implements MemberAccess { } protected boolean isClassAllowlisted(Class clazz) { - return allowlistClasses.contains(clazz) || isClassBelongsToPackages(clazz, allowlistPackageNames); + return allowlistClasses.contains(clazz) + || (providerAllowlist != null && providerAllowlist.getProviderAllowlist().contains(clazz)) + || isClassBelongsToPackages(clazz, allowlistPackageNames); } /** diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java index ebbc797ee..90f120a07 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -24,6 +24,7 @@ import com.opensymphony.xwork2.test.TestBean2; import com.opensymphony.xwork2.util.Foo; import ognl.MemberAccess; import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.struts2.ognl.ProviderAllowlist; import org.junit.Before; import org.junit.Test; @@ -44,22 +45,27 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; public class SecurityMemberAccessTest { private Map context; private FooBar target; protected SecurityMemberAccess sma; + private ProviderAllowlist mockedProviderAllowlist; @Before public void setUp() throws Exception { context = new HashMap<>(); target = new FooBar(); + mockedProviderAllowlist = mock(ProviderAllowlist.class); assignNewSma(true); } protected void assignNewSma(boolean allowStaticFieldAccess) { - sma = new SecurityMemberAccess(allowStaticFieldAccess); + when(mockedProviderAllowlist.getProviderAllowlist()).thenReturn(new HashSet<>()); + sma = new SecurityMemberAccess(String.valueOf(allowStaticFieldAccess), mockedProviderAllowlist); } private T reflectField(String fieldName) throws IllegalAccessException { From ee442db9ed66f5da55664c36323d720868dd1459 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 24 Nov 2023 20:12:17 +1100 Subject: [PATCH 05/14] WW-5364 Enable allowlist for showcase --- apps/showcase/src/main/resources/struts.xml | 1 + core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java | 1 + 2 files changed, 2 insertions(+) diff --git a/apps/showcase/src/main/resources/struts.xml b/apps/showcase/src/main/resources/struts.xml index f73963de9..150b8e36d 100644 --- a/apps/showcase/src/main/resources/struts.xml +++ b/apps/showcase/src/main/resources/struts.xml @@ -33,6 +33,7 @@ + diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java index 62e635fbc..331ddcc02 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -856,6 +856,7 @@ public class OgnlUtil { } SecurityMemberAccess memberAccess = container.getInstance(SecurityMemberAccess.class); + memberAccess.useEnforceAllowlistEnabled(Boolean.FALSE.toString()); if (devMode) { if (!warnReported.get()) { From 39c3e332d7a0ea4fa51bb6e62f5ac170b0cc5072 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 24 Nov 2023 21:32:24 +1100 Subject: [PATCH 06/14] WW-5364 Add Struts components to allowlist --- apps/showcase/src/main/resources/struts.xml | 12 ++++++++++++ .../xwork2/ognl/SecurityMemberAccess.java | 14 ++++++++++++++ .../src/main/resources/struts-excluded-classes.xml | 12 ++++++++---- 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/apps/showcase/src/main/resources/struts.xml b/apps/showcase/src/main/resources/struts.xml index 150b8e36d..d59a198cd 100644 --- a/apps/showcase/src/main/resources/struts.xml +++ b/apps/showcase/src/main/resources/struts.xml @@ -35,6 +35,18 @@ + + diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index c7d941eeb..c4ddfbd24 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -58,6 +58,18 @@ public class SecurityMemberAccess implements MemberAccess { private static final Logger LOG = LogManager.getLogger(SecurityMemberAccess.class); + private static final Set ALLOWLIST_REQUIRED_PACKAGES = unmodifiableSet(new HashSet<>(Arrays.asList( + "org.apache.struts2.components", + "org.apache.struts2.views.jsp", + "com.opensymphony.xwork2.validator.validators" + ))); + + private static final Set> ALLOWLIST_REQUIRED_CLASSES = unmodifiableSet(new HashSet<>(Arrays.asList( + java.lang.Enum.class, + java.util.Date.class, + java.util.HashMap.class + ))); + private final ProviderAllowlist providerAllowlist; private final boolean allowStaticFieldAccess; private Set excludeProperties = emptySet(); @@ -204,7 +216,9 @@ public class SecurityMemberAccess implements MemberAccess { protected boolean isClassAllowlisted(Class clazz) { return allowlistClasses.contains(clazz) + || ALLOWLIST_REQUIRED_CLASSES.contains(clazz) || (providerAllowlist != null && providerAllowlist.getProviderAllowlist().contains(clazz)) + || isClassBelongsToPackages(clazz, ALLOWLIST_REQUIRED_PACKAGES) || isClassBelongsToPackages(clazz, allowlistPackageNames); } diff --git a/core/src/main/resources/struts-excluded-classes.xml b/core/src/main/resources/struts-excluded-classes.xml index 0d95f9c2c..f3f4f3f19 100644 --- a/core/src/main/resources/struts-excluded-classes.xml +++ b/core/src/main/resources/struts-excluded-classes.xml @@ -43,7 +43,8 @@ java.lang.System, java.lang.Thread, java.lang.ThreadGroup, - java.lang.ThreadLocal"/> + java.lang.ThreadLocal + "/> + java.lang.ThreadLocal + "/> @@ -98,7 +100,8 @@ org.wildfly.extension.undertow.deployment, org.yaml.snakeyaml, sun.misc, - sun.reflect"/> + sun.reflect + "/> + sun.reflect + "/> From 6657e01f9b45290677e532d3bb952fdb8cf97653 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 24 Nov 2023 22:11:27 +1100 Subject: [PATCH 07/14] WW-5364 Don't throw ConfigurationException on unloadable action or interceptor classes --- .../providers/XmlDocConfigurationProvider.java | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) 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 2a342c521..ccddb9efc 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 @@ -534,7 +534,11 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid clazz.getConstructor(); } } catch (ClassNotFoundException e) { - throw new ConfigurationException("Action class [" + className + "] not found", e, loc); + if (objectFactory.isNoArgConstructorRequired()) { + throw new ConfigurationException("Action class [" + className + "] not found", e, loc); + } + LOG.warn("Action class [" + className + "] not found"); + LOG.debug("Action class [" + className + "] not found", e); } catch (NoSuchMethodException e) { throw new ConfigurationException("Action class [" + className + "] does not have a public no-arg constructor", e, loc); } catch (RuntimeException ex) { @@ -543,8 +547,7 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid LOG.debug("Action verification cause", ex); } catch (Exception ex) { // Default to failing fast - LOG.debug("Unable to verify action class [{}]", className, ex); - throw new ConfigurationException(ex, loc); + throw new ConfigurationException("Unable to verify action class [" + className + "]", ex, loc); } } @@ -556,9 +559,7 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid Location loc = DomHelper.getLocationObject(resultTypeElement); Class clazz = verifyResultType(className, loc); - if (clazz == null) { - return; - } + String paramName = null; try { paramName = (String) clazz.getField("DEFAULT_PARAM").get(null); @@ -961,7 +962,8 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid try { allowAndLoadClass(className); } catch (ClassNotFoundException | NoClassDefFoundError e) { - throw new ConfigurationException("Interceptor class [" + className + "] not found", e, loc); + LOG.warn("Interceptor class [" + className + "] at location " + loc + " not found"); + LOG.debug("Interceptor class [" + className + "] not found", e); } } From d7df9ce9981558d230c40cd065d2c67aa110b657 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 24 Nov 2023 22:41:14 +1100 Subject: [PATCH 08/14] WW-5364 Replace some allowlist classes with packages --- apps/showcase/src/main/resources/struts.xml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/showcase/src/main/resources/struts.xml b/apps/showcase/src/main/resources/struts.xml index d59a198cd..a9fe3c9dc 100644 --- a/apps/showcase/src/main/resources/struts.xml +++ b/apps/showcase/src/main/resources/struts.xml @@ -33,15 +33,15 @@ - + + entry : BOOTSTRAP_CONSTANTS.entrySet()) { builder.constant(entry.getKey(), String.valueOf(entry.getValue())); @@ -397,6 +359,57 @@ public class DefaultConfiguration implements Configuration { return builder.create(true); } + public static ContainerBuilder bootstrapFactories(ContainerBuilder builder) { + return builder + // TODO: SpringObjectFactoryTest fails when these are SINGLETON + .factory(ObjectFactory.class, Scope.PROTOTYPE) + .factory(ActionFactory.class, DefaultActionFactory.class, Scope.PROTOTYPE) + .factory(ResultFactory.class, DefaultResultFactory.class, Scope.PROTOTYPE) + .factory(InterceptorFactory.class, DefaultInterceptorFactory.class, Scope.PROTOTYPE) + .factory(ValidatorFactory.class, DefaultValidatorFactory.class, Scope.PROTOTYPE) + .factory(ConverterFactory.class, StrutsConverterFactory.class, Scope.PROTOTYPE) + .factory(UnknownHandlerFactory.class, DefaultUnknownHandlerFactory.class, Scope.PROTOTYPE) + + .factory(FileManager.class, "system", DefaultFileManager.class, Scope.SINGLETON) + .factory(ReflectionProvider.class, OgnlReflectionProvider.class, Scope.SINGLETON) + .factory(ValueStackFactory.class, OgnlValueStackFactory.class, Scope.SINGLETON) + + .factory(XWorkConverter.class, Scope.SINGLETON) + .factory(XWorkBasicConverter.class, Scope.SINGLETON) + .factory(ConversionPropertiesProcessor.class, StrutsConversionPropertiesProcessor.class, Scope.SINGLETON) + .factory(ConversionFileProcessor.class, DefaultConversionFileProcessor.class, Scope.SINGLETON) + .factory(ConversionAnnotationProcessor.class, DefaultConversionAnnotationProcessor.class, Scope.SINGLETON) + .factory(TypeConverterCreator.class, StrutsTypeConverterCreator.class, Scope.SINGLETON) + .factory(TypeConverterHolder.class, StrutsTypeConverterHolder.class, Scope.SINGLETON) + + .factory(TextProvider.class, "system", DefaultTextProvider.class, Scope.SINGLETON) + .factory(LocalizedTextProvider.class, StrutsLocalizedTextProvider.class, Scope.SINGLETON) + .factory(TextProviderFactory.class, StrutsTextProviderFactory.class, Scope.SINGLETON) + .factory(LocaleProviderFactory.class, DefaultLocaleProviderFactory.class, Scope.SINGLETON) + .factory(TextParser.class, OgnlTextParser.class, Scope.SINGLETON) + + .factory(ObjectTypeDeterminer.class, DefaultObjectTypeDeterminer.class, Scope.SINGLETON) + .factory(PropertyAccessor.class, CompoundRoot.class.getName(), CompoundRootAccessor.class, Scope.SINGLETON) + + .factory(ExpressionCacheFactory.class, DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON) + .factory(BeanInfoCacheFactory.class, DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON) + .factory(OgnlUtil.class, Scope.SINGLETON) + .factory(SecurityMemberAccess.class, Scope.PROTOTYPE) + .factory(OgnlGuard.class, StrutsOgnlGuard.class, Scope.SINGLETON) + .factory(ProviderAllowlist.class, Scope.SINGLETON) + + .factory(ValueSubstitutor.class, EnvsValueSubstitutor.class, Scope.SINGLETON); + } + + public static ContainerBuilder bootstrapTypeConverters(ContainerBuilder builder) { + return builder + .factory(TypeConverter.class, StrutsConstants.STRUTS_CONVERTER_COLLECTION, CollectionConverter.class, Scope.SINGLETON) + .factory(TypeConverter.class, StrutsConstants.STRUTS_CONVERTER_ARRAY, ArrayConverter.class, Scope.SINGLETON) + .factory(TypeConverter.class, StrutsConstants.STRUTS_CONVERTER_DATE, DateConverter.class, Scope.SINGLETON) + .factory(TypeConverter.class, StrutsConstants.STRUTS_CONVERTER_NUMBER, NumberConverter.class, Scope.SINGLETON) + .factory(TypeConverter.class, StrutsConstants.STRUTS_CONVERTER_STRING, StringConverter.class, Scope.SINGLETON); + } + /** *

* This builds the internal runtime configuration used by Xwork for finding and configuring Actions from the diff --git a/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java b/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java index a4960ee92..20f3abce8 100644 --- a/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java +++ b/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java @@ -20,62 +20,24 @@ package com.opensymphony.xwork2.config.providers; import com.opensymphony.xwork2.ActionProxyFactory; import com.opensymphony.xwork2.DefaultActionProxyFactory; -import com.opensymphony.xwork2.DefaultLocaleProviderFactory; -import com.opensymphony.xwork2.DefaultTextProvider; import com.opensymphony.xwork2.DefaultUnknownHandlerManager; -import com.opensymphony.xwork2.FileManager; import com.opensymphony.xwork2.FileManagerFactory; -import com.opensymphony.xwork2.LocaleProviderFactory; -import com.opensymphony.xwork2.LocalizedTextProvider; -import com.opensymphony.xwork2.ObjectFactory; -import com.opensymphony.xwork2.StrutsTextProviderFactory; -import com.opensymphony.xwork2.TextProvider; -import com.opensymphony.xwork2.TextProviderFactory; import com.opensymphony.xwork2.UnknownHandlerManager; import com.opensymphony.xwork2.config.Configuration; import com.opensymphony.xwork2.config.ConfigurationException; import com.opensymphony.xwork2.config.ConfigurationProvider; import com.opensymphony.xwork2.config.impl.DefaultConfiguration; -import com.opensymphony.xwork2.conversion.ConversionAnnotationProcessor; -import com.opensymphony.xwork2.conversion.ConversionFileProcessor; -import com.opensymphony.xwork2.conversion.ConversionPropertiesProcessor; import com.opensymphony.xwork2.conversion.NullHandler; -import com.opensymphony.xwork2.conversion.ObjectTypeDeterminer; -import com.opensymphony.xwork2.conversion.TypeConverterCreator; -import com.opensymphony.xwork2.conversion.TypeConverterHolder; import com.opensymphony.xwork2.conversion.impl.ArrayConverter; import com.opensymphony.xwork2.conversion.impl.CollectionConverter; import com.opensymphony.xwork2.conversion.impl.DateConverter; -import com.opensymphony.xwork2.conversion.impl.DefaultConversionAnnotationProcessor; -import com.opensymphony.xwork2.conversion.impl.DefaultConversionFileProcessor; -import com.opensymphony.xwork2.conversion.impl.DefaultObjectTypeDeterminer; import com.opensymphony.xwork2.conversion.impl.InstantiatingNullHandler; import com.opensymphony.xwork2.conversion.impl.NumberConverter; import com.opensymphony.xwork2.conversion.impl.StringConverter; -import com.opensymphony.xwork2.conversion.impl.XWorkBasicConverter; -import com.opensymphony.xwork2.conversion.impl.XWorkConverter; -import com.opensymphony.xwork2.factory.ActionFactory; -import com.opensymphony.xwork2.factory.ConverterFactory; -import com.opensymphony.xwork2.factory.DefaultActionFactory; -import com.opensymphony.xwork2.factory.DefaultInterceptorFactory; -import com.opensymphony.xwork2.factory.DefaultResultFactory; -import com.opensymphony.xwork2.factory.DefaultUnknownHandlerFactory; -import com.opensymphony.xwork2.factory.InterceptorFactory; -import com.opensymphony.xwork2.factory.ResultFactory; -import com.opensymphony.xwork2.factory.StrutsConverterFactory; -import com.opensymphony.xwork2.factory.UnknownHandlerFactory; import com.opensymphony.xwork2.inject.ContainerBuilder; import com.opensymphony.xwork2.inject.Scope; -import com.opensymphony.xwork2.ognl.BeanInfoCacheFactory; -import com.opensymphony.xwork2.ognl.DefaultOgnlBeanInfoCacheFactory; -import com.opensymphony.xwork2.ognl.DefaultOgnlExpressionCacheFactory; -import com.opensymphony.xwork2.ognl.ExpressionCacheFactory; import com.opensymphony.xwork2.ognl.ObjectProxy; import com.opensymphony.xwork2.ognl.OgnlReflectionContextFactory; -import com.opensymphony.xwork2.ognl.OgnlReflectionProvider; -import com.opensymphony.xwork2.ognl.OgnlUtil; -import com.opensymphony.xwork2.ognl.OgnlValueStackFactory; -import com.opensymphony.xwork2.ognl.SecurityMemberAccess; import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor; import com.opensymphony.xwork2.ognl.accessor.HttpParametersPropertyAccessor; import com.opensymphony.xwork2.ognl.accessor.ObjectAccessor; @@ -94,17 +56,11 @@ import com.opensymphony.xwork2.security.DefaultNotExcludedAcceptedPatternsChecke import com.opensymphony.xwork2.security.ExcludedPatternsChecker; import com.opensymphony.xwork2.security.NotExcludedAcceptedPatternsChecker; import com.opensymphony.xwork2.util.CompoundRoot; -import com.opensymphony.xwork2.util.OgnlTextParser; import com.opensymphony.xwork2.util.PatternMatcher; -import com.opensymphony.xwork2.util.StrutsLocalizedTextProvider; -import com.opensymphony.xwork2.util.TextParser; -import com.opensymphony.xwork2.util.ValueStackFactory; import com.opensymphony.xwork2.util.WildcardHelper; -import com.opensymphony.xwork2.util.fs.DefaultFileManager; import com.opensymphony.xwork2.util.fs.DefaultFileManagerFactory; import com.opensymphony.xwork2.util.location.LocatableProperties; import com.opensymphony.xwork2.util.reflection.ReflectionContextFactory; -import com.opensymphony.xwork2.util.reflection.ReflectionProvider; import com.opensymphony.xwork2.validator.ActionValidatorManager; import com.opensymphony.xwork2.validator.AnnotationActionValidatorManager; import com.opensymphony.xwork2.validator.DefaultActionValidatorManager; @@ -114,16 +70,10 @@ import com.opensymphony.xwork2.validator.ValidatorFactory; import com.opensymphony.xwork2.validator.ValidatorFileParser; import ognl.MethodAccessor; import ognl.PropertyAccessor; -import org.apache.struts2.conversion.StrutsConversionPropertiesProcessor; -import org.apache.struts2.conversion.StrutsTypeConverterCreator; -import org.apache.struts2.conversion.StrutsTypeConverterHolder; import org.apache.struts2.dispatcher.HttpParameters; import org.apache.struts2.dispatcher.Parameter; import org.apache.struts2.interceptor.exec.ExecutorProvider; import org.apache.struts2.interceptor.exec.StrutsExecutorProvider; -import org.apache.struts2.ognl.OgnlGuard; -import org.apache.struts2.ognl.ProviderAllowlist; -import org.apache.struts2.ognl.StrutsOgnlGuard; import org.apache.struts2.url.QueryStringBuilder; import org.apache.struts2.url.QueryStringParser; import org.apache.struts2.url.StrutsQueryStringBuilder; @@ -163,97 +113,60 @@ public class StrutsDefaultConfigurationProvider implements ConfigurationProvider } @Override - public void register(ContainerBuilder builder, LocatableProperties props) - throws ConfigurationException { + public void register(ContainerBuilder builder, LocatableProperties props) throws ConfigurationException { - builder - .factory(ObjectFactory.class) - .factory(ActionFactory.class, DefaultActionFactory.class) - .factory(ResultFactory.class, DefaultResultFactory.class) - .factory(InterceptorFactory.class, DefaultInterceptorFactory.class) - .factory(com.opensymphony.xwork2.factory.ValidatorFactory.class, com.opensymphony.xwork2.factory.DefaultValidatorFactory.class) - .factory(ConverterFactory.class, StrutsConverterFactory.class) - .factory(UnknownHandlerFactory.class, DefaultUnknownHandlerFactory.class) + DefaultConfiguration.bootstrapFactories(builder) + .factory(FileManagerFactory.class, DefaultFileManagerFactory.class, Scope.SINGLETON) - .factory(ActionProxyFactory.class, DefaultActionProxyFactory.class, Scope.SINGLETON) - .factory(ObjectTypeDeterminer.class, DefaultObjectTypeDeterminer.class, Scope.SINGLETON) + .factory(ActionProxyFactory.class, DefaultActionProxyFactory.class, Scope.SINGLETON) - .factory(XWorkConverter.class, Scope.SINGLETON) - .factory(XWorkBasicConverter.class, Scope.SINGLETON) - .factory(ConversionPropertiesProcessor.class, StrutsConversionPropertiesProcessor.class, Scope.SINGLETON) - .factory(ConversionFileProcessor.class, DefaultConversionFileProcessor.class, Scope.SINGLETON) - .factory(ConversionAnnotationProcessor.class, DefaultConversionAnnotationProcessor.class, Scope.SINGLETON) - .factory(TypeConverterCreator.class, StrutsTypeConverterCreator.class, Scope.SINGLETON) - .factory(TypeConverterHolder.class, StrutsTypeConverterHolder.class, Scope.SINGLETON) + .factory(ValidatorFactory.class, DefaultValidatorFactory.class, Scope.SINGLETON) + .factory(ValidatorFileParser.class, DefaultValidatorFileParser.class, Scope.SINGLETON) + .factory(PatternMatcher.class, WildcardHelper.class, Scope.SINGLETON) - .factory(FileManager.class, "system", DefaultFileManager.class, Scope.SINGLETON) - .factory(FileManagerFactory.class, DefaultFileManagerFactory.class, Scope.SINGLETON) - .factory(ValueStackFactory.class, OgnlValueStackFactory.class, Scope.SINGLETON) - .factory(ValidatorFactory.class, DefaultValidatorFactory.class, Scope.SINGLETON) - .factory(ValidatorFileParser.class, DefaultValidatorFileParser.class, Scope.SINGLETON) - .factory(PatternMatcher.class, WildcardHelper.class, Scope.SINGLETON) - .factory(ReflectionProvider.class, OgnlReflectionProvider.class, Scope.SINGLETON) - .factory(ReflectionContextFactory.class, OgnlReflectionContextFactory.class, Scope.SINGLETON) + .factory(ReflectionContextFactory.class, OgnlReflectionContextFactory.class, Scope.SINGLETON) - .factory(PropertyAccessor.class, CompoundRoot.class.getName(), CompoundRootAccessor.class, Scope.SINGLETON) - .factory(PropertyAccessor.class, Object.class.getName(), ObjectAccessor.class, Scope.SINGLETON) - .factory(PropertyAccessor.class, Iterator.class.getName(), XWorkIteratorPropertyAccessor.class, Scope.SINGLETON) - .factory(PropertyAccessor.class, Enumeration.class.getName(), XWorkEnumerationAccessor.class, Scope.SINGLETON) + .factory(PropertyAccessor.class, Object.class.getName(), ObjectAccessor.class, Scope.SINGLETON) + .factory(PropertyAccessor.class, Iterator.class.getName(), XWorkIteratorPropertyAccessor.class, Scope.SINGLETON) + .factory(PropertyAccessor.class, Enumeration.class.getName(), XWorkEnumerationAccessor.class, Scope.SINGLETON) - .factory(UnknownHandlerManager.class, DefaultUnknownHandlerManager.class, Scope.SINGLETON) + .factory(UnknownHandlerManager.class, DefaultUnknownHandlerManager.class, Scope.SINGLETON) - // silly workarounds for ognl since there is no way to flush its caches - .factory(PropertyAccessor.class, List.class.getName(), XWorkListPropertyAccessor.class, Scope.SINGLETON) - .factory(PropertyAccessor.class, ArrayList.class.getName(), XWorkListPropertyAccessor.class, Scope.SINGLETON) - .factory(PropertyAccessor.class, HashSet.class.getName(), XWorkCollectionPropertyAccessor.class, Scope.SINGLETON) - .factory(PropertyAccessor.class, Set.class.getName(), XWorkCollectionPropertyAccessor.class, Scope.SINGLETON) - .factory(PropertyAccessor.class, HashMap.class.getName(), XWorkMapPropertyAccessor.class, Scope.SINGLETON) - .factory(PropertyAccessor.class, Map.class.getName(), XWorkMapPropertyAccessor.class, Scope.SINGLETON) - .factory(PropertyAccessor.class, Collection.class.getName(), XWorkCollectionPropertyAccessor.class, Scope.SINGLETON) - .factory(PropertyAccessor.class, ObjectProxy.class.getName(), ObjectProxyPropertyAccessor.class, Scope.SINGLETON) - .factory(PropertyAccessor.class, HttpParameters.class.getName(), HttpParametersPropertyAccessor.class, Scope.SINGLETON) - .factory(PropertyAccessor.class, Parameter.class.getName(), ParameterPropertyAccessor.class, Scope.SINGLETON) + // silly workarounds for ognl since there is no way to flush its caches + .factory(PropertyAccessor.class, List.class.getName(), XWorkListPropertyAccessor.class, Scope.SINGLETON) + .factory(PropertyAccessor.class, ArrayList.class.getName(), XWorkListPropertyAccessor.class, Scope.SINGLETON) + .factory(PropertyAccessor.class, HashSet.class.getName(), XWorkCollectionPropertyAccessor.class, Scope.SINGLETON) + .factory(PropertyAccessor.class, Set.class.getName(), XWorkCollectionPropertyAccessor.class, Scope.SINGLETON) + .factory(PropertyAccessor.class, HashMap.class.getName(), XWorkMapPropertyAccessor.class, Scope.SINGLETON) + .factory(PropertyAccessor.class, Map.class.getName(), XWorkMapPropertyAccessor.class, Scope.SINGLETON) + .factory(PropertyAccessor.class, Collection.class.getName(), XWorkCollectionPropertyAccessor.class, Scope.SINGLETON) + .factory(PropertyAccessor.class, ObjectProxy.class.getName(), ObjectProxyPropertyAccessor.class, Scope.SINGLETON) + .factory(PropertyAccessor.class, HttpParameters.class.getName(), HttpParametersPropertyAccessor.class, Scope.SINGLETON) + .factory(PropertyAccessor.class, Parameter.class.getName(), ParameterPropertyAccessor.class, Scope.SINGLETON) - .factory(MethodAccessor.class, Object.class.getName(), XWorkMethodAccessor.class, Scope.SINGLETON) - .factory(MethodAccessor.class, CompoundRoot.class.getName(), CompoundRootAccessor.class, Scope.SINGLETON) + .factory(MethodAccessor.class, Object.class.getName(), XWorkMethodAccessor.class, Scope.SINGLETON) + .factory(MethodAccessor.class, CompoundRoot.class.getName(), CompoundRootAccessor.class, Scope.SINGLETON) - .factory(TextParser.class, OgnlTextParser.class, Scope.SINGLETON) + .factory(NullHandler.class, Object.class.getName(), InstantiatingNullHandler.class, Scope.SINGLETON) + .factory(ActionValidatorManager.class, AnnotationActionValidatorManager.class, Scope.SINGLETON) + .factory(ActionValidatorManager.class, "no-annotations", DefaultActionValidatorManager.class, Scope.SINGLETON) - .factory(NullHandler.class, Object.class.getName(), InstantiatingNullHandler.class, Scope.SINGLETON) - .factory(ActionValidatorManager.class, AnnotationActionValidatorManager.class, Scope.SINGLETON) - .factory(ActionValidatorManager.class, "no-annotations", DefaultActionValidatorManager.class, Scope.SINGLETON) + .factory(CollectionConverter.class, Scope.SINGLETON) + .factory(ArrayConverter.class, Scope.SINGLETON) + .factory(DateConverter.class, Scope.SINGLETON) + .factory(NumberConverter.class, Scope.SINGLETON) + .factory(StringConverter.class, Scope.SINGLETON) - .factory(TextProvider.class, "system", DefaultTextProvider.class, Scope.SINGLETON) - .factory(LocalizedTextProvider.class, StrutsLocalizedTextProvider.class, Scope.SINGLETON) - .factory(TextProviderFactory.class, StrutsTextProviderFactory.class, Scope.SINGLETON) - .factory(LocaleProviderFactory.class, DefaultLocaleProviderFactory.class, Scope.SINGLETON) + .factory(ExcludedPatternsChecker.class, DefaultExcludedPatternsChecker.class, Scope.PROTOTYPE) + .factory(AcceptedPatternsChecker.class, DefaultAcceptedPatternsChecker.class, Scope.PROTOTYPE) + .factory(NotExcludedAcceptedPatternsChecker.class, DefaultNotExcludedAcceptedPatternsChecker.class, Scope.SINGLETON) - .factory(ExpressionCacheFactory.class, DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON) - .factory(BeanInfoCacheFactory.class, DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON) - .factory(OgnlUtil.class, Scope.SINGLETON) - .factory(SecurityMemberAccess.class, Scope.PROTOTYPE) - .factory(OgnlGuard.class, StrutsOgnlGuard.class, Scope.SINGLETON) - .factory(ProviderAllowlist.class, Scope.SINGLETON) - .factory(CollectionConverter.class, Scope.SINGLETON) - .factory(ArrayConverter.class, Scope.SINGLETON) - .factory(DateConverter.class, Scope.SINGLETON) - .factory(NumberConverter.class, Scope.SINGLETON) - .factory(StringConverter.class, Scope.SINGLETON) + .factory(QueryStringBuilder.class, StrutsQueryStringBuilder.class, Scope.SINGLETON) + .factory(QueryStringParser.class, StrutsQueryStringParser.class, Scope.SINGLETON) + .factory(UrlEncoder.class, StrutsUrlEncoder.class, Scope.SINGLETON) + .factory(UrlDecoder.class, StrutsUrlDecoder.class, Scope.SINGLETON) - .factory(ExcludedPatternsChecker.class, DefaultExcludedPatternsChecker.class, Scope.PROTOTYPE) - .factory(AcceptedPatternsChecker.class, DefaultAcceptedPatternsChecker.class, Scope.PROTOTYPE) - .factory(NotExcludedAcceptedPatternsChecker.class, DefaultNotExcludedAcceptedPatternsChecker.class - , Scope.SINGLETON) - - .factory(ValueSubstitutor.class, EnvsValueSubstitutor.class, Scope.SINGLETON) - - .factory(QueryStringBuilder.class, StrutsQueryStringBuilder.class, Scope.SINGLETON) - .factory(QueryStringParser.class, StrutsQueryStringParser.class, Scope.SINGLETON) - .factory(UrlEncoder.class, StrutsUrlEncoder.class, Scope.SINGLETON) - .factory(UrlDecoder.class, StrutsUrlDecoder.class, Scope.SINGLETON) - - .factory(ExecutorProvider.class, StrutsExecutorProvider.class, Scope.SINGLETON) - ; + .factory(ExecutorProvider.class, StrutsExecutorProvider.class, Scope.SINGLETON); for (Map.Entry entry : DefaultConfiguration.BOOTSTRAP_CONSTANTS.entrySet()) { props.setProperty(entry.getKey(), String.valueOf(entry.getValue())); From 5e33c7f2c4d27e9d468b3299a6028e61fcee7429 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Sun, 3 Dec 2023 23:22:06 +1100 Subject: [PATCH 10/14] WW-5343 Add unit test coverage for ProviderAllowlist --- .../struts2/ognl/ProviderAllowlist.java | 18 ++++- .../struts2/ognl/ProviderAllowlistTest.java | 70 +++++++++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/ognl/ProviderAllowlistTest.java 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 82ebaabc3..d10372885 100644 --- a/core/src/main/java/org/apache/struts2/ognl/ProviderAllowlist.java +++ b/core/src/main/java/org/apache/struts2/ognl/ProviderAllowlist.java @@ -34,8 +34,13 @@ import static java.util.Collections.unmodifiableSet; */ public class ProviderAllowlist { - private final Map>> allowlistMap = new HashMap<>(); - private final Set> allowlistClasses = new HashSet<>(); + private final Map>> allowlistMap; + private Set> allowlistClasses; + + public ProviderAllowlist() { + allowlistMap = new HashMap<>(); + reconstructAllowlist(); + } public synchronized void registerAllowlist(ConfigurationProvider configurationProvider, Set> allowlist) { Set> existingAllowlist = allowlistMap.get(configurationProvider); @@ -51,11 +56,18 @@ public class ProviderAllowlist { if (allowlist == null) { return; } - this.allowlistClasses.removeAll(allowlist); this.allowlistMap.remove(configurationProvider); + reconstructAllowlist(); } public Set> getProviderAllowlist() { return unmodifiableSet(allowlistClasses); } + + private void reconstructAllowlist() { + this.allowlistClasses = allowlistMap.values().stream().reduce(new HashSet<>(), (a, b) -> { + a.addAll(b); + return a; + }); + } } diff --git a/core/src/test/java/org/apache/struts2/ognl/ProviderAllowlistTest.java b/core/src/test/java/org/apache/struts2/ognl/ProviderAllowlistTest.java new file mode 100644 index 000000000..ce583aa6b --- /dev/null +++ b/core/src/test/java/org/apache/struts2/ognl/ProviderAllowlistTest.java @@ -0,0 +1,70 @@ +package org.apache.struts2.ognl; + +import com.opensymphony.xwork2.config.ConfigurationProvider; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; + +import java.util.HashSet; + +import static java.util.Arrays.asList; +import static org.assertj.core.api.Assertions.assertThat; + +public class ProviderAllowlistTest { + + @Rule + public final MockitoRule mockito = MockitoJUnit.rule(); + + private ProviderAllowlist providerAllowlist; + + @Mock + private ConfigurationProvider provider1; + + @Mock + private ConfigurationProvider provider2; + + @Before + public void setUp() throws Exception { + providerAllowlist = new ProviderAllowlist(); + } + + @Test + public void registerAllowlist() { + providerAllowlist.registerAllowlist(provider1, new HashSet<>(asList(String.class, Integer.class))); + providerAllowlist.registerAllowlist(provider2, 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))); + + 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.clearAllowlist(provider1); + + 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.clearAllowlist(provider1); + providerAllowlist.clearAllowlist(provider2); + + assertThat(providerAllowlist.getProviderAllowlist()).isEmpty(); + } +} From 16f822a2b2463b3cb1bbb1ff988b9c251b94b241 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Sun, 3 Dec 2023 23:36:10 +1100 Subject: [PATCH 11/14] WW-5343 Move JUnit4 test case into Struts-core --- .../xwork2/XWorkJUnit4TestCase.java | 90 +++++++++++++++++++ .../struts2/junit/XWorkJUnit4TestCase.java | 89 +----------------- .../apache/struts2/junit/XWorkTestCase.java | 4 + 3 files changed, 95 insertions(+), 88 deletions(-) create mode 100644 core/src/main/java/com/opensymphony/xwork2/XWorkJUnit4TestCase.java create mode 100644 plugins/junit/src/main/java/org/apache/struts2/junit/XWorkTestCase.java diff --git a/core/src/main/java/com/opensymphony/xwork2/XWorkJUnit4TestCase.java b/core/src/main/java/com/opensymphony/xwork2/XWorkJUnit4TestCase.java new file mode 100644 index 000000000..b22789cc2 --- /dev/null +++ b/core/src/main/java/com/opensymphony/xwork2/XWorkJUnit4TestCase.java @@ -0,0 +1,90 @@ +/* + * 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 com.opensymphony.xwork2; + +import com.opensymphony.xwork2.config.Configuration; +import com.opensymphony.xwork2.config.ConfigurationException; +import com.opensymphony.xwork2.config.ConfigurationManager; +import com.opensymphony.xwork2.config.ConfigurationProvider; +import com.opensymphony.xwork2.inject.Container; +import com.opensymphony.xwork2.inject.ContainerBuilder; +import com.opensymphony.xwork2.inject.Context; +import com.opensymphony.xwork2.inject.Factory; +import com.opensymphony.xwork2.inject.Scope; +import com.opensymphony.xwork2.test.StubConfigurationProvider; +import com.opensymphony.xwork2.util.XWorkTestCaseHelper; +import com.opensymphony.xwork2.util.location.LocatableProperties; +import org.junit.After; +import org.junit.Before; + +public abstract class XWorkJUnit4TestCase { + + protected ConfigurationManager configurationManager; + protected Configuration configuration; + protected Container container; + protected ActionProxyFactory actionProxyFactory; + + @Before + public void setUp() throws Exception { + configurationManager = XWorkTestCaseHelper.setUp(); + configuration = configurationManager.getConfiguration(); + container = configuration.getContainer(); + actionProxyFactory = container.getInstance(ActionProxyFactory.class); + } + + @After + public void tearDown() throws Exception { + XWorkTestCaseHelper.tearDown(configurationManager); + configurationManager = null; + configuration = null; + container = null; + actionProxyFactory = null; + } + + protected void loadConfigurationProviders(ConfigurationProvider... providers) { + configurationManager = XWorkTestCaseHelper.loadConfigurationProviders(configurationManager, providers); + configuration = configurationManager.getConfiguration(); + container = configuration.getContainer(); + actionProxyFactory = container.getInstance(ActionProxyFactory.class); + } + + protected void loadButAdd(final Class type, final Object impl) { + loadButAdd(type, Container.DEFAULT_NAME, impl); + } + + protected void loadButAdd(final Class type, final String name, final Object impl) { + loadConfigurationProviders(new StubConfigurationProvider() { + @Override + public void register(ContainerBuilder builder, + LocatableProperties props) throws ConfigurationException { + builder.factory(type, name, new Factory() { + public Object create(Context context) throws Exception { + return impl; + } + + @Override + public Class type() { + return impl.getClass(); + } + }, Scope.SINGLETON); + } + }); + } + +} diff --git a/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkJUnit4TestCase.java b/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkJUnit4TestCase.java index d36c3b0ad..03cf71e77 100644 --- a/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkJUnit4TestCase.java +++ b/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkJUnit4TestCase.java @@ -1,91 +1,4 @@ -/* - * 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 org.apache.struts2.junit; -import com.opensymphony.xwork2.ActionProxyFactory; -import com.opensymphony.xwork2.config.Configuration; -import com.opensymphony.xwork2.config.ConfigurationException; -import com.opensymphony.xwork2.config.ConfigurationManager; -import com.opensymphony.xwork2.config.ConfigurationProvider; -import com.opensymphony.xwork2.inject.Container; -import com.opensymphony.xwork2.inject.ContainerBuilder; -import com.opensymphony.xwork2.inject.Context; -import com.opensymphony.xwork2.inject.Factory; -import com.opensymphony.xwork2.inject.Scope; -import com.opensymphony.xwork2.test.StubConfigurationProvider; -import com.opensymphony.xwork2.util.XWorkTestCaseHelper; -import com.opensymphony.xwork2.util.location.LocatableProperties; -import org.junit.After; -import org.junit.Before; - -public abstract class XWorkJUnit4TestCase { - - protected ConfigurationManager configurationManager; - protected Configuration configuration; - protected Container container; - protected ActionProxyFactory actionProxyFactory; - - @Before - public void setUp() throws Exception { - configurationManager = XWorkTestCaseHelper.setUp(); - configuration = configurationManager.getConfiguration(); - container = configuration.getContainer(); - actionProxyFactory = container.getInstance(ActionProxyFactory.class); - } - - @After - public void tearDown() throws Exception { - XWorkTestCaseHelper.tearDown(configurationManager); - configurationManager = null; - configuration = null; - container = null; - actionProxyFactory = null; - } - - protected void loadConfigurationProviders(ConfigurationProvider... providers) { - configurationManager = XWorkTestCaseHelper.loadConfigurationProviders(configurationManager, providers); - configuration = configurationManager.getConfiguration(); - container = configuration.getContainer(); - actionProxyFactory = container.getInstance(ActionProxyFactory.class); - } - - protected void loadButAdd(final Class type, final Object impl) { - loadButAdd(type, Container.DEFAULT_NAME, impl); - } - - protected void loadButAdd(final Class type, final String name, final Object impl) { - loadConfigurationProviders(new StubConfigurationProvider() { - @Override - public void register(ContainerBuilder builder, - LocatableProperties props) throws ConfigurationException { - builder.factory(type, name, new Factory() { - public Object create(Context context) throws Exception { - return impl; - } - - @Override - public Class type() { - return impl.getClass(); - } - }, Scope.SINGLETON); - } - }); - } - +public abstract class XWorkJUnit4TestCase extends com.opensymphony.xwork2.XWorkJUnit4TestCase { } diff --git a/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkTestCase.java b/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkTestCase.java new file mode 100644 index 000000000..a0e1837b3 --- /dev/null +++ b/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkTestCase.java @@ -0,0 +1,4 @@ +package org.apache.struts2.junit; + +public abstract class XWorkTestCase extends com.opensymphony.xwork2.XWorkTestCase { +} From a26823386c5c9d54507bf8b770b2ce06ea2e7dc5 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 4 Dec 2023 21:48:22 +1100 Subject: [PATCH 12/14] WW-5343 Add integration tests for ConfigurationProvider populating ProviderAllowlist --- .../XmlDocConfigurationProvider.java | 6 +- ...onfigurationProviderOgnlAllowlistTest.java | 96 +++++++++++++++++++ .../providers/xwork-test-allowlist-2.xml | 41 ++++++++ .../config/providers/xwork-test-allowlist.xml | 48 ++++++++++ 4 files changed, 187 insertions(+), 4 deletions(-) create mode 100644 core/src/test/java/com/opensymphony/xwork2/config/providers/ConfigurationProviderOgnlAllowlistTest.java create mode 100644 core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowlist-2.xml create mode 100644 core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowlist.xml 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 ccddb9efc..bae553789 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 @@ -66,7 +66,6 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.function.Consumer; -import java.util.stream.Stream; import static com.opensymphony.xwork2.util.TextParseUtil.commaDelimitedStringToSet; import static java.lang.Boolean.parseBoolean; @@ -149,9 +148,8 @@ public abstract class XmlDocConfigurationProvider implements ConfigurationProvid protected Class allowAndLoadClass(String className) throws ClassNotFoundException { Class clazz = loadClass(className); allowlistClasses.add(clazz); - List> superClasses = ClassUtils.getAllSuperclasses(clazz); - List> interfaces = ClassUtils.getAllInterfaces(clazz); - Stream.concat(superClasses.stream(), interfaces.stream()).forEach(allowlistClasses::add); + allowlistClasses.addAll(ClassUtils.getAllSuperclasses(clazz)); + allowlistClasses.addAll(ClassUtils.getAllInterfaces(clazz)); return clazz; } diff --git a/core/src/test/java/com/opensymphony/xwork2/config/providers/ConfigurationProviderOgnlAllowlistTest.java b/core/src/test/java/com/opensymphony/xwork2/config/providers/ConfigurationProviderOgnlAllowlistTest.java new file mode 100644 index 000000000..4870ba8fc --- /dev/null +++ b/core/src/test/java/com/opensymphony/xwork2/config/providers/ConfigurationProviderOgnlAllowlistTest.java @@ -0,0 +1,96 @@ +package com.opensymphony.xwork2.config.providers; + +import com.opensymphony.xwork2.XWorkJUnit4TestCase; +import com.opensymphony.xwork2.config.ConfigurationProvider; +import org.apache.struts2.config.StrutsXmlConfigurationProvider; +import org.apache.struts2.ognl.ProviderAllowlist; +import org.junit.Before; +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +public class ConfigurationProviderOgnlAllowlistTest extends XWorkJUnit4TestCase { + + private final ConfigurationProvider testXml1 = new StrutsXmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-allowlist.xml"); + private final ConfigurationProvider testXml2 = new StrutsXmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-allowlist-2.xml"); + private ProviderAllowlist providerAllowlist; + + @Before + public void setUp() throws Exception { + loadConfigurationProviders(testXml1, testXml2); + providerAllowlist = container.getInstance(ProviderAllowlist.class); + } + + @Test + public void allowlist() throws Exception { + loadConfigurationProviders(testXml1, testXml2); + providerAllowlist = container.getInstance(ProviderAllowlist.class); + + assertThat(providerAllowlist.getProviderAllowlist()).containsExactlyInAnyOrder( + Class.forName("com.opensymphony.xwork2.interceptor.ValidationAware"), + Class.forName("com.opensymphony.xwork2.LocaleProvider"), + Class.forName("java.io.Serializable"), + Class.forName("com.opensymphony.xwork2.mock.MockResult"), + Class.forName("com.opensymphony.xwork2.interceptor.ConditionalInterceptor"), + Class.forName("com.opensymphony.xwork2.ActionSupport"), + Class.forName("com.opensymphony.xwork2.ActionChainResult"), + Class.forName("com.opensymphony.xwork2.TextProvider"), + Class.forName("org.apache.struts2.interceptor.NoOpInterceptor"), + Class.forName("com.opensymphony.xwork2.interceptor.Interceptor"), + Class.forName("java.lang.Object"), + Class.forName("com.opensymphony.xwork2.Validateable"), + Class.forName("com.opensymphony.xwork2.mock.MockInterceptor"), + Class.forName("com.opensymphony.xwork2.Action"), + Class.forName("com.opensymphony.xwork2.interceptor.AbstractInterceptor"), + Class.forName("com.opensymphony.xwork2.Result"), + Class.forName("com.opensymphony.xwork2.SimpleAction") + ); + } + + @Test + public void allowlist_1only() throws Exception { + loadConfigurationProviders(testXml1); + providerAllowlist = container.getInstance(ProviderAllowlist.class); + + assertThat(providerAllowlist.getProviderAllowlist()).containsExactlyInAnyOrder( + Class.forName("com.opensymphony.xwork2.interceptor.ValidationAware"), + Class.forName("com.opensymphony.xwork2.LocaleProvider"), + Class.forName("java.io.Serializable"), + Class.forName("com.opensymphony.xwork2.mock.MockResult"), + Class.forName("com.opensymphony.xwork2.interceptor.ConditionalInterceptor"), + Class.forName("com.opensymphony.xwork2.ActionSupport"), + Class.forName("com.opensymphony.xwork2.TextProvider"), + Class.forName("com.opensymphony.xwork2.interceptor.Interceptor"), + Class.forName("java.lang.Object"), + Class.forName("com.opensymphony.xwork2.Validateable"), + Class.forName("com.opensymphony.xwork2.mock.MockInterceptor"), + Class.forName("com.opensymphony.xwork2.Action"), + Class.forName("com.opensymphony.xwork2.interceptor.AbstractInterceptor"), + Class.forName("com.opensymphony.xwork2.Result"), + Class.forName("com.opensymphony.xwork2.SimpleAction") + ); + } + + @Test + public void allowlist_2only() throws Exception { + loadConfigurationProviders(testXml2); + providerAllowlist = container.getInstance(ProviderAllowlist.class); + + assertThat(providerAllowlist.getProviderAllowlist()).containsExactlyInAnyOrder( + Class.forName("com.opensymphony.xwork2.interceptor.ValidationAware"), + Class.forName("com.opensymphony.xwork2.LocaleProvider"), + Class.forName("java.io.Serializable"), + Class.forName("com.opensymphony.xwork2.interceptor.ConditionalInterceptor"), + Class.forName("com.opensymphony.xwork2.ActionSupport"), + Class.forName("com.opensymphony.xwork2.ActionChainResult"), + Class.forName("com.opensymphony.xwork2.TextProvider"), + Class.forName("org.apache.struts2.interceptor.NoOpInterceptor"), + Class.forName("com.opensymphony.xwork2.interceptor.Interceptor"), + Class.forName("java.lang.Object"), + Class.forName("com.opensymphony.xwork2.Validateable"), + Class.forName("com.opensymphony.xwork2.Action"), + Class.forName("com.opensymphony.xwork2.interceptor.AbstractInterceptor"), + Class.forName("com.opensymphony.xwork2.Result") + ); + } +} diff --git a/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowlist-2.xml b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowlist-2.xml new file mode 100644 index 000000000..f5e9b184d --- /dev/null +++ b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowlist-2.xml @@ -0,0 +1,41 @@ + + + + + + + + + + + + + + + + + + + + diff --git a/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowlist.xml b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowlist.xml new file mode 100644 index 000000000..1de061efd --- /dev/null +++ b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowlist.xml @@ -0,0 +1,48 @@ + + + + + + + + + + + + fooDefault + + + + + + + + + 18 + 24 + + + + + From 589219baacc364bb953adf31028c85aa31e416dd Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 4 Dec 2023 21:53:28 +1100 Subject: [PATCH 13/14] WW-5343 Add missing licenses --- ...ConfigurationProviderOgnlAllowlistTest.java | 18 ++++++++++++++++++ .../struts2/ognl/ProviderAllowlistTest.java | 18 ++++++++++++++++++ .../struts2/junit/XWorkJUnit4TestCase.java | 18 ++++++++++++++++++ .../apache/struts2/junit/XWorkTestCase.java | 18 ++++++++++++++++++ 4 files changed, 72 insertions(+) diff --git a/core/src/test/java/com/opensymphony/xwork2/config/providers/ConfigurationProviderOgnlAllowlistTest.java b/core/src/test/java/com/opensymphony/xwork2/config/providers/ConfigurationProviderOgnlAllowlistTest.java index 4870ba8fc..4fa4aad8b 100644 --- a/core/src/test/java/com/opensymphony/xwork2/config/providers/ConfigurationProviderOgnlAllowlistTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/config/providers/ConfigurationProviderOgnlAllowlistTest.java @@ -1,3 +1,21 @@ +/* + * 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 com.opensymphony.xwork2.config.providers; import com.opensymphony.xwork2.XWorkJUnit4TestCase; 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 ce583aa6b..baf91b9b5 100644 --- a/core/src/test/java/org/apache/struts2/ognl/ProviderAllowlistTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/ProviderAllowlistTest.java @@ -1,3 +1,21 @@ +/* + * 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 org.apache.struts2.ognl; import com.opensymphony.xwork2.config.ConfigurationProvider; diff --git a/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkJUnit4TestCase.java b/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkJUnit4TestCase.java index 03cf71e77..12acf283e 100644 --- a/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkJUnit4TestCase.java +++ b/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkJUnit4TestCase.java @@ -1,3 +1,21 @@ +/* + * 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 org.apache.struts2.junit; public abstract class XWorkJUnit4TestCase extends com.opensymphony.xwork2.XWorkJUnit4TestCase { diff --git a/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkTestCase.java b/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkTestCase.java index a0e1837b3..e64184ba0 100644 --- a/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkTestCase.java +++ b/plugins/junit/src/main/java/org/apache/struts2/junit/XWorkTestCase.java @@ -1,3 +1,21 @@ +/* + * 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 org.apache.struts2.junit; public abstract class XWorkTestCase extends com.opensymphony.xwork2.XWorkTestCase { From a7d273c1f0e4bd942c770f6697456cd2c263e3b3 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 4 Dec 2023 22:40:39 +1100 Subject: [PATCH 14/14] WW-5343 Make StrutsTestCase extend same package --- .../src/main/java/org/apache/struts2/junit/StrutsTestCase.java | 1 - 1 file changed, 1 deletion(-) diff --git a/plugins/junit/src/main/java/org/apache/struts2/junit/StrutsTestCase.java b/plugins/junit/src/main/java/org/apache/struts2/junit/StrutsTestCase.java index 608310838..f64a9966f 100644 --- a/plugins/junit/src/main/java/org/apache/struts2/junit/StrutsTestCase.java +++ b/plugins/junit/src/main/java/org/apache/struts2/junit/StrutsTestCase.java @@ -21,7 +21,6 @@ package org.apache.struts2.junit; import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.ActionProxy; import com.opensymphony.xwork2.ActionProxyFactory; -import com.opensymphony.xwork2.XWorkTestCase; import com.opensymphony.xwork2.config.Configuration; import org.apache.struts2.ServletActionContext; import org.apache.struts2.dispatcher.Dispatcher;