You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by lu...@apache.org on 2023/02/28 07:26:20 UTC
[struts] branch master updated: WW-5268 Implement ability to specify exempt classes for package exclusions
This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/master by this push:
new 5fcee890d WW-5268 Implement ability to specify exempt classes for package exclusions
new 6a27f460c Merge pull request #660 from atlassian/WW-5268-excluded-package-exemptions
5fcee890d is described below
commit 5fcee890d593d6eb5034e836bb3cf9e565d7061d
Author: Kusal Kithul-Godage <gi...@kusal.io>
AuthorDate: Wed Feb 15 00:34:42 2023 +1100
WW-5268 Implement ability to specify exempt classes for package exclusions
---
.../com/opensymphony/xwork2/ognl/OgnlUtil.java | 49 ++++++++--
.../opensymphony/xwork2/ognl/OgnlValueStack.java | 8 +-
.../xwork2/ognl/SecurityMemberAccess.java | 30 ++++--
.../java/org/apache/struts2/StrutsConstants.java | 3 +
.../xwork2/ognl/SecurityMemberAccessTest.java | 107 +++++++++++++++++++--
5 files changed, 174 insertions(+), 23 deletions(-)
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 c5430491b..827cda11a 100644
--- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
+++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
@@ -26,7 +26,13 @@ import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor;
import com.opensymphony.xwork2.util.CompoundRoot;
import com.opensymphony.xwork2.util.TextParseUtil;
import com.opensymphony.xwork2.util.reflection.ReflectionException;
-import ognl.*;
+import ognl.ClassResolver;
+import ognl.Ognl;
+import ognl.OgnlContext;
+import ognl.OgnlException;
+import ognl.OgnlRuntime;
+import ognl.SimpleNode;
+import ognl.TypeConverter;
import org.apache.commons.lang3.BooleanUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
@@ -37,7 +43,12 @@ import java.beans.IntrospectionException;
import java.beans.Introspector;
import java.beans.PropertyDescriptor;
import java.lang.reflect.Method;
-import java.util.*;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.regex.Pattern;
@@ -66,10 +77,12 @@ public class OgnlUtil {
private Set<Class<?>> excludedClasses;
private Set<Pattern> excludedPackageNamePatterns;
private Set<String> excludedPackageNames;
+ private Set<Class<?>> excludedPackageExemptClasses;
private Set<Class<?>> devModeExcludedClasses;
private Set<Pattern> devModeExcludedPackageNamePatterns;
private Set<String> devModeExcludedPackageNames;
+ private Set<Class<?>> devModeExcludedPackageExemptClasses;
private Container container;
private boolean allowStaticFieldAccess = true;
@@ -112,10 +125,12 @@ public class OgnlUtil {
excludedClasses = Collections.unmodifiableSet(new HashSet<>());
excludedPackageNamePatterns = Collections.unmodifiableSet(new HashSet<>());
excludedPackageNames = Collections.unmodifiableSet(new HashSet<>());
+ excludedPackageExemptClasses = Collections.unmodifiableSet(new HashSet<>());
devModeExcludedClasses = Collections.unmodifiableSet(new HashSet<>());
devModeExcludedPackageNamePatterns = Collections.unmodifiableSet(new HashSet<>());
devModeExcludedPackageNames = Collections.unmodifiableSet(new HashSet<>());
+ devModeExcludedPackageExemptClasses = Collections.unmodifiableSet(new HashSet<>());
this.expressionCache = ognlExpressionCacheFactory.buildOgnlCache();
this.beanInfoCache = ognlBeanInfoCacheFactory.buildOgnlCache();
@@ -159,7 +174,7 @@ public class OgnlUtil {
protected void setExcludedClasses(String commaDelimitedClasses) {
Set<Class<?>> excludedClasses = new HashSet<>();
excludedClasses.addAll(this.excludedClasses);
- excludedClasses.addAll(parseExcludedClasses(commaDelimitedClasses));
+ excludedClasses.addAll(parseClasses(commaDelimitedClasses));
this.excludedClasses = Collections.unmodifiableSet(excludedClasses);
}
@@ -167,11 +182,11 @@ public class OgnlUtil {
protected void setDevModeExcludedClasses(String commaDelimitedClasses) {
Set<Class<?>> excludedClasses = new HashSet<>();
excludedClasses.addAll(this.devModeExcludedClasses);
- excludedClasses.addAll(parseExcludedClasses(commaDelimitedClasses));
+ excludedClasses.addAll(parseClasses(commaDelimitedClasses));
this.devModeExcludedClasses = Collections.unmodifiableSet(excludedClasses);
}
- private Set<Class<?>> parseExcludedClasses(String commaDelimitedClasses) {
+ private Set<Class<?>> parseClasses(String commaDelimitedClasses) {
Set<String> classNames = TextParseUtil.commaDelimitedStringToSet(commaDelimitedClasses);
Set<Class<?>> classes = new HashSet<>();
@@ -179,7 +194,7 @@ public class OgnlUtil {
try {
classes.add(Class.forName(className));
} catch (ClassNotFoundException e) {
- throw new ConfigurationException("Cannot load excluded class: " + className, e);
+ throw new ConfigurationException("Cannot load class for exclusion/exemption configuration: " + className, e);
}
}
@@ -229,6 +244,22 @@ public class OgnlUtil {
this.devModeExcludedPackageNames = Collections.unmodifiableSet(excludedPackageNames);
}
+ @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false)
+ public void setExcludedPackageExemptClasses(String commaDelimitedClasses) {
+ Set<Class<?>> excludedPackageExemptClasses = new HashSet<>();
+ excludedPackageExemptClasses.addAll(this.excludedPackageExemptClasses);
+ excludedPackageExemptClasses.addAll(parseClasses(commaDelimitedClasses));
+ this.excludedPackageExemptClasses = Collections.unmodifiableSet(excludedPackageExemptClasses);
+ }
+
+ @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false)
+ public void setDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) {
+ Set<Class<?>> excludedPackageExemptClasses = new HashSet<>();
+ excludedPackageExemptClasses.addAll(this.devModeExcludedPackageExemptClasses);
+ excludedPackageExemptClasses.addAll(parseClasses(commaDelimitedClasses));
+ this.devModeExcludedPackageExemptClasses = Collections.unmodifiableSet(excludedPackageExemptClasses);
+ }
+
private Set<String> parseExcludedPackageNames(String commaDelimitedPackageNames) {
return TextParseUtil.commaDelimitedStringToSet(commaDelimitedPackageNames);
}
@@ -245,6 +276,10 @@ public class OgnlUtil {
return excludedPackageNames;
}
+ public Set<Class<?>> getExcludedPackageExemptClasses() {
+ return excludedPackageExemptClasses;
+ }
+
@Inject
protected void setContainer(Container container) {
this.container = container;
@@ -847,10 +882,12 @@ public class OgnlUtil {
memberAccess.setExcludedClasses(devModeExcludedClasses);
memberAccess.setExcludedPackageNamePatterns(devModeExcludedPackageNamePatterns);
memberAccess.setExcludedPackageNames(devModeExcludedPackageNames);
+ memberAccess.setExcludedPackageExemptClasses(devModeExcludedPackageExemptClasses);
} else {
memberAccess.setExcludedClasses(excludedClasses);
memberAccess.setExcludedPackageNamePatterns(excludedPackageNamePatterns);
memberAccess.setExcludedPackageNames(excludedPackageNames);
+ memberAccess.setExcludedPackageExemptClasses(excludedPackageExemptClasses);
}
return Ognl.createDefaultContext(root, memberAccess, resolver, defaultConverter);
diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
index 1b874fc44..938ed477a 100644
--- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
+++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java
@@ -29,7 +29,12 @@ import com.opensymphony.xwork2.util.CompoundRoot;
import com.opensymphony.xwork2.util.MemberAccessValueStack;
import com.opensymphony.xwork2.util.ValueStack;
import com.opensymphony.xwork2.util.reflection.ReflectionContextState;
-import ognl.*;
+import ognl.MethodFailedException;
+import ognl.NoSuchPropertyException;
+import ognl.Ognl;
+import ognl.OgnlContext;
+import ognl.OgnlException;
+import ognl.PropertyAccessor;
import org.apache.commons.lang3.BooleanUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
@@ -87,6 +92,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS
securityMemberAccess.setExcludedClasses(ognlUtil.getExcludedClasses());
securityMemberAccess.setExcludedPackageNamePatterns(ognlUtil.getExcludedPackageNamePatterns());
securityMemberAccess.setExcludedPackageNames(ognlUtil.getExcludedPackageNames());
+ securityMemberAccess.setExcludedPackageExemptClasses(ognlUtil.getExcludedPackageExemptClasses());
securityMemberAccess.setDisallowProxyMemberAccess(ognlUtil.isDisallowProxyMemberAccess());
}
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 6dcdb5a1b..0b03f9226 100644
--- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java
+++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java
@@ -47,6 +47,7 @@ public class SecurityMemberAccess implements MemberAccess {
private Set<Class<?>> excludedClasses = Collections.emptySet();
private Set<Pattern> excludedPackageNamePatterns = Collections.emptySet();
private Set<String> excludedPackageNames = Collections.emptySet();
+ private Set<Class<?>> excludedPackageExemptClasses = Collections.emptySet();
private boolean disallowProxyMemberAccess;
/**
@@ -117,23 +118,23 @@ public class SecurityMemberAccess implements MemberAccess {
}
final Class<?> memberClass = member.getDeclaringClass();
+ // target can be null in case of accessing static fields, since OGNL 3.2.8
+ final Class<?> targetClass = Modifier.isStatic(memberModifiers) ? memberClass : target.getClass();
if (isClassExcluded(memberClass)) {
LOG.warn("Declaring class of member type [{}] is excluded!", member);
return false;
}
- // target can be null in case of accessing static fields, since OGNL 3.2.8
- final Class<?> targetClass = Modifier.isStatic(memberModifiers) ? memberClass : target.getClass();
-
- if (isPackageExcluded(targetClass.getPackage(), memberClass.getPackage())) {
- LOG.warn("Package [{}] of target class [{}] of target [{}] or package [{}] of member [{}] are excluded!", targetClass.getPackage(), targetClass,
- target, memberClass.getPackage(), member);
+ if (isClassExcluded(targetClass)) {
+ LOG.warn("Target class [{}] of target [{}] is excluded!", targetClass, target);
return false;
}
- if (isClassExcluded(targetClass)) {
- LOG.warn("Target class [{}] of target [{}] is excluded!", targetClass, target);
+ if (!isClassExcludedPackageExempt(targetClass) && !isClassExcludedPackageExempt(memberClass)
+ && isPackageExcluded(targetClass.getPackage(), memberClass.getPackage())) {
+ LOG.warn("Package [{}] of target class [{}] of target [{}] or package [{}] of member [{}] are excluded!",
+ targetClass.getPackage(), targetClass, target, memberClass.getPackage(), member);
return false;
}
@@ -234,6 +235,15 @@ public class SecurityMemberAccess implements MemberAccess {
return false;
}
+ protected boolean isClassExcludedPackageExempt(Class<?> clazz) {
+ for (Class<?> excludedPackageExemptClass : excludedPackageExemptClasses) {
+ if (clazz.isAssignableFrom(excludedPackageExemptClass)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
protected boolean isAcceptableProperty(String name) {
return name == null || ((!isExcluded(name)) && isAccepted(name));
}
@@ -287,6 +297,10 @@ public class SecurityMemberAccess implements MemberAccess {
this.excludedPackageNames = excludedPackageNames;
}
+ public void setExcludedPackageExemptClasses(Set<Class<?>> excludedPackageExemptClasses) {
+ this.excludedPackageExemptClasses = excludedPackageExemptClasses;
+ }
+
public void setDisallowProxyMemberAccess(boolean disallowProxyMemberAccess) {
this.disallowProxyMemberAccess = disallowProxyMemberAccess;
}
diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java
index 9a1920f02..8d44b9837 100644
--- a/core/src/main/java/org/apache/struts2/StrutsConstants.java
+++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java
@@ -414,11 +414,14 @@ public final class StrutsConstants {
public static final String STRUTS_EXCLUDED_CLASSES = "struts.excludedClasses";
public static final String STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS = "struts.excludedPackageNamePatterns";
public static final String STRUTS_EXCLUDED_PACKAGE_NAMES = "struts.excludedPackageNames";
+ public static final String STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES = "struts.excludedPackageExemptClasses";
/** Comma delimited set of excluded classes and package names which cannot be accessed via expressions in devMode */
public static final String STRUTS_DEV_MODE_EXCLUDED_CLASSES = "struts.devMode.excludedClasses";
public static final String STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS = "struts.devMode.excludedPackageNamePatterns";
public static final String STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES = "struts.devMode.excludedPackageNames";
+ public static final String STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES = "struts.devMode.excludedPackageExemptClasses";
+
/** Dedicated services to check if passed string is excluded/accepted */
public static final String STRUTS_EXCLUDED_PATTERNS_CHECKER = "struts.excludedPatterns.checker";
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 19abfae09..6bb9dcf94 100644
--- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java
+++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java
@@ -47,7 +47,7 @@ public class SecurityMemberAccessTest extends TestCase {
SecurityMemberAccess sma = new SecurityMemberAccess(true);
String propertyName = "stringField";
- Member member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1));
+ Member member = FooBar.class.getMethod(formGetterName(propertyName));
// when
boolean accessible = sma.isAccessible(context, target, member, propertyName);
@@ -61,7 +61,7 @@ public class SecurityMemberAccessTest extends TestCase {
SecurityMemberAccess sma = new SecurityMemberAccess(true);
String propertyName = "stringField";
- Member member = FooBar.class.getDeclaredMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1));
+ Member member = FooBar.class.getDeclaredMethod(formGetterName(propertyName));
Set<Class<?>> excluded = new HashSet<>();
excluded.add(FooBar.class);
@@ -138,6 +138,24 @@ public class SecurityMemberAccessTest extends TestCase {
assertTrue("fooLogic() from FooInterface isn't accessible!!!", accessible);
}
+ public void testMiddleOfInheritanceExclusion2() throws Exception {
+ // given
+ SecurityMemberAccess sma = new SecurityMemberAccess(true);
+
+ String propertyName = "barLogic";
+ Member member = BarInterface.class.getMethod(propertyName);
+
+ Set<Class<?>> excluded = new HashSet<>();
+ excluded.add(BarInterface.class);
+ sma.setExcludedClasses(excluded);
+
+ // when
+ boolean accessible = sma.isAccessible(context, target, member, propertyName);
+
+ // then
+ assertFalse("barLogic() from BarInterface is accessible!!!", accessible);
+ }
+
public void testMiddleOfInheritanceExclusion3() throws Exception {
// given
SecurityMemberAccess sma = new SecurityMemberAccess(true);
@@ -145,6 +163,10 @@ public class SecurityMemberAccessTest extends TestCase {
String propertyName = "barLogic";
Member member = BarInterface.class.getMethod(propertyName);
+ Set<Class<?>> excluded = new HashSet<>();
+ excluded.add(FooInterface.class);
+ sma.setExcludedClasses(excluded);
+
// when
boolean accessible = sma.isAccessible(context, target, member, propertyName);
@@ -179,7 +201,7 @@ public class SecurityMemberAccessTest extends TestCase {
sma.setExcludedPackageNamePatterns(excluded);
String propertyName = "stringField";
- Member member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1));
+ Member member = FooBar.class.getMethod(formGetterName(propertyName));
// when
boolean actual = sma.isAccessible(context, target, member, propertyName);
@@ -188,6 +210,28 @@ public class SecurityMemberAccessTest extends TestCase {
assertFalse("stringField is accessible!", actual);
}
+ public void testPackageExclusionExemption() throws Exception {
+ // given
+ SecurityMemberAccess sma = new SecurityMemberAccess(false);
+
+ Set<Pattern> excluded = new HashSet<>();
+ excluded.add(Pattern.compile("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*"));
+ sma.setExcludedPackageNamePatterns(excluded);
+
+ Set<Class<?>> allowed = new HashSet<>();
+ allowed.add(FooBar.class);
+ sma.setExcludedPackageExemptClasses(allowed);
+
+ String propertyName = "stringField";
+ Member member = FooBar.class.getMethod(formGetterName(propertyName));
+
+ // when
+ boolean actual = sma.isAccessible(context, target, member, propertyName);
+
+ // then
+ assertTrue("stringField isn't accessible!", actual);
+ }
+
public void testPackageNameExclusion() throws Exception {
// given
SecurityMemberAccess sma = new SecurityMemberAccess(true);
@@ -197,7 +241,7 @@ public class SecurityMemberAccessTest extends TestCase {
sma.setExcludedPackageNames(excluded);
String propertyName = "stringField";
- Member member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1));
+ Member member = FooBar.class.getMethod(formGetterName(propertyName));
// when
boolean actual = sma.isAccessible(context, target, member, propertyName);
@@ -206,6 +250,51 @@ public class SecurityMemberAccessTest extends TestCase {
assertFalse("stringField is accessible!", actual);
}
+
+ public void testPackageNameExclusionExemption() throws Exception {
+ // given
+ SecurityMemberAccess sma = new SecurityMemberAccess(false);
+
+ Set<String> excluded = new HashSet<>();
+ excluded.add(FooBar.class.getPackage().getName());
+ sma.setExcludedPackageNames(excluded);
+
+ Set<Class<?>> allowed = new HashSet<>();
+ allowed.add(FooBar.class);
+ sma.setExcludedPackageExemptClasses(allowed);
+
+ String propertyName = "stringField";
+ Member member = FooBar.class.getMethod(formGetterName(propertyName));
+
+ // when
+ boolean actual = sma.isAccessible(context, target, member, propertyName);
+
+ // then
+ assertTrue("stringField isn't accessible!", actual);
+ }
+
+ public void testPackageNameExclusionExemptionInheritance() throws Exception {
+ // given
+ SecurityMemberAccess sma = new SecurityMemberAccess(false);
+
+ Set<String> excluded = new HashSet<>();
+ excluded.add(FooBar.class.getPackage().getName());
+ sma.setExcludedPackageNames(excluded);
+
+ Set<Class<?>> allowed = new HashSet<>();
+ allowed.add(BarInterface.class);
+ sma.setExcludedPackageExemptClasses(allowed);
+
+ String propertyName = "barLogic";
+ Member member = BarInterface.class.getMethod(propertyName);
+
+ // when
+ boolean actual = sma.isAccessible(context, target, member, propertyName);
+
+ // then
+ assertTrue("barLogic isn't accessible!", actual);
+ }
+
public void testDefaultPackageExclusion() {
// given
SecurityMemberAccess sma = new SecurityMemberAccess(true);
@@ -429,7 +518,7 @@ public class SecurityMemberAccessTest extends TestCase {
sma.setExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("java.lang.,ognl,javax"));
String propertyName = "intField";
- Member member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1));
+ Member member = FooBar.class.getMethod(formGetterName(propertyName));
// when
boolean accessible = sma.isAccessible(context, target, member, propertyName);
@@ -473,7 +562,7 @@ public class SecurityMemberAccessTest extends TestCase {
// given
propertyName = "intField";
- member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1));
+ member = FooBar.class.getMethod(formGetterName(propertyName));
// when
accessible = sma.isAccessible(context, target, member, propertyName);
@@ -482,7 +571,7 @@ public class SecurityMemberAccessTest extends TestCase {
// given
propertyName = "doubleField";
- member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1));
+ member = FooBar.class.getMethod(formGetterName(propertyName));
// when
accessible = sma.isAccessible(context, target, member, propertyName);
@@ -547,7 +636,6 @@ public class SecurityMemberAccessTest extends TestCase {
// given
SecurityMemberAccess sma = new SecurityMemberAccess(true);
-
sma.setExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("java.lang."));
// when
@@ -558,6 +646,9 @@ public class SecurityMemberAccessTest extends TestCase {
assertTrue("package java.lang. is accessible!", actual);
}
+ private static String formGetterName(String propertyName) {
+ return "get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1);
+ }
}
class FooBar implements FooBarInterface {