From 699786e190ee0d12e72d43615d4ad2d9ada5bb76 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 28 Aug 2023 13:43:18 +1000 Subject: [PATCH 1/9] WW-5341 Refactor SecurityMemberAccess methods for reuse --- .../xwork2/ognl/SecurityMemberAccess.java | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 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 5db52639e5..fada135c22 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -220,34 +220,32 @@ protected boolean isPackageExcluded(Class targetClass, Class memberClass) "Parameters should never be null - if member is static, targetClass should be the same as memberClass."); } - Set> classesToCheck = new HashSet<>(); - classesToCheck.add(targetClass); - classesToCheck.add(memberClass); - + List> classesToCheck = Arrays.asList(targetClass, memberClass); for (Class clazz : classesToCheck) { - if (!isExcludedPackageExempt(clazz) && (isExcludedPackageNamePatterns(clazz) || isExcludedPackageNames(clazz))) { + if (!excludedPackageExemptClasses.contains(clazz) && (isExcludedPackageNames(clazz) || isExcludedPackageNamePatterns(clazz))) { return true; } } return false; } - protected String toPackageName(Class clazz) { + public static String toPackageName(Class clazz) { if (clazz.getPackage() == null) { return ""; - } else { - return clazz.getPackage().getName(); } + return clazz.getPackage().getName(); } protected boolean isExcludedPackageNamePatterns(Class clazz) { - String packageName = toPackageName(clazz); - return excludedPackageNamePatterns.stream().anyMatch(pattern -> pattern.matcher(packageName).matches()); + return excludedPackageNamePatterns.stream().anyMatch(pattern -> pattern.matcher(toPackageName(clazz)).matches()); } protected boolean isExcludedPackageNames(Class clazz) { - String packageName = toPackageName(clazz); - List packageParts = Arrays.asList(packageName.split("\\.")); + return isExcludedPackageNamesStatic(clazz, excludedPackageNames); + } + + public static boolean isExcludedPackageNamesStatic(Class clazz, Set excludedPackageNames) { + List packageParts = Arrays.asList(toPackageName(clazz).split("\\.")); for (int i = 0; i < packageParts.size(); i++) { String parentPackage = String.join(".", packageParts.subList(0, i + 1)); if (excludedPackageNames.contains(parentPackage)) { @@ -261,10 +259,6 @@ protected boolean isClassExcluded(Class clazz) { return excludedClasses.contains(clazz); } - protected boolean isExcludedPackageExempt(Class clazz) { - return excludedPackageExemptClasses.contains(clazz); - } - protected boolean isAcceptableProperty(String name) { return name == null || ((!isExcluded(name)) && isAccepted(name)); } From d6fcfd9c275c289f8ee5bcfb53f93b6980f2c19c Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 28 Aug 2023 22:19:06 +1000 Subject: [PATCH 2/9] WW-5341 Clean up SecurityMemberAccess#restore --- .../xwork2/ognl/SecurityMemberAccess.java | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 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 fada135c22..48d36a4fda 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -84,16 +84,17 @@ public Object setup(Map context, Object target, Member member, String propertyNa @Override public void restore(Map context, Object target, Member member, String propertyName, Object state) { - if (state != null) { - final AccessibleObject accessible = (AccessibleObject) member; - final boolean stateBoolean = ((Boolean) state).booleanValue(); // Using twice (avoid unboxing) - if (!stateBoolean) { - accessible.setAccessible(stateBoolean); - } else { - throw new IllegalArgumentException("Improper restore state [" + stateBoolean + "] for target [" + target + - "], member [" + member + "], propertyName [" + propertyName + "]"); - } + if (state == null) { + return; + } + if ((Boolean) state) { + throw new IllegalArgumentException(format( + "Improper restore state [true] for target [{0}], member [{1}], propertyName [{2}]", + target, + member, + propertyName)); } + ((AccessibleObject) member).setAccessible(false); } @Override From c57015749dd6af2a00693edef4395bb1cb279c9e Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 28 Aug 2023 22:23:49 +1000 Subject: [PATCH 3/9] WW-5341 Further refactor of OgnlUtil and SecurityMemberAccess to store excluded classes as Strings --- .../opensymphony/xwork2/ognl/OgnlUtil.java | 128 ++++++++---------- .../xwork2/ognl/SecurityMemberAccess.java | 24 ++-- .../xwork2/ognl/OgnlUtilStrutsTest.java | 4 +- .../xwork2/ognl/OgnlUtilTest.java | 85 ++++-------- .../xwork2/ognl/SecurityMemberAccessTest.java | 100 +++++++------- 5 files changed, 146 insertions(+), 195 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 005d17eba7..bbfdd72bbd 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -25,6 +25,7 @@ import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor; import com.opensymphony.xwork2.util.CompoundRoot; import com.opensymphony.xwork2.util.reflection.ReflectionException; +import jdk.internal.reflect.Reflection; import ognl.ClassResolver; import ognl.Ognl; import ognl.OgnlContext; @@ -43,7 +44,6 @@ import java.beans.PropertyDescriptor; import java.lang.reflect.Method; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -53,6 +53,8 @@ import java.util.regex.PatternSyntaxException; import static com.opensymphony.xwork2.util.TextParseUtil.commaDelimitedStringToSet; +import static java.util.Collections.emptySet; +import static java.util.Collections.unmodifiableSet; import static java.util.stream.Collectors.toSet; import static org.apache.commons.lang3.StringUtils.strip; @@ -78,15 +80,15 @@ public class OgnlUtil { private boolean enableExpressionCache = true; private boolean enableEvalExpression; - private Set> excludedClasses; + private Set excludedClasses; private Set excludedPackageNamePatterns; private Set excludedPackageNames; - private Set> excludedPackageExemptClasses; + private Set excludedPackageExemptClasses; - private Set> devModeExcludedClasses; + private Set devModeExcludedClasses; private Set devModeExcludedPackageNamePatterns; private Set devModeExcludedPackageNames; - private Set> devModeExcludedPackageExemptClasses; + private Set devModeExcludedPackageExemptClasses; private Container container; private boolean allowStaticFieldAccess = true; @@ -126,18 +128,18 @@ public OgnlUtil( if (ognlBeanInfoCacheFactory == null) { throw new IllegalArgumentException("BeanInfoCacheFactory parameter cannot be null"); } - excludedClasses = Collections.unmodifiableSet(new HashSet<>()); - excludedPackageNamePatterns = Collections.unmodifiableSet(new HashSet<>()); - excludedPackageNames = Collections.unmodifiableSet(new HashSet<>()); - excludedPackageExemptClasses = Collections.unmodifiableSet(new HashSet<>()); + excludedClasses = emptySet(); + excludedPackageNamePatterns = emptySet(); + excludedPackageNames = emptySet(); + excludedPackageExemptClasses = emptySet(); - devModeExcludedClasses = Collections.unmodifiableSet(new HashSet<>()); - devModeExcludedPackageNamePatterns = Collections.unmodifiableSet(new HashSet<>()); - devModeExcludedPackageNames = Collections.unmodifiableSet(new HashSet<>()); - devModeExcludedPackageExemptClasses = Collections.unmodifiableSet(new HashSet<>()); + devModeExcludedClasses = emptySet(); + devModeExcludedPackageNamePatterns = emptySet(); + devModeExcludedPackageNames = emptySet(); + devModeExcludedPackageExemptClasses = emptySet(); - this.expressionCache = ognlExpressionCacheFactory.buildOgnlCache(); - this.beanInfoCache = ognlBeanInfoCacheFactory.buildOgnlCache(); + expressionCache = ognlExpressionCacheFactory.buildOgnlCache(); + beanInfoCache = ognlBeanInfoCacheFactory.buildOgnlCache(); } @Inject @@ -176,101 +178,87 @@ protected void setEnableEvalExpression(String evalExpression) { @Inject(value = StrutsConstants.STRUTS_EXCLUDED_CLASSES, required = false) protected void setExcludedClasses(String commaDelimitedClasses) { - Set> excludedClasses = new HashSet<>(); - excludedClasses.addAll(this.excludedClasses); - excludedClasses.addAll(parseClasses(commaDelimitedClasses)); - this.excludedClasses = Collections.unmodifiableSet(excludedClasses); + excludedClasses = toNewClassesSet(excludedClasses, commaDelimitedClasses); } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required = false) protected void setDevModeExcludedClasses(String commaDelimitedClasses) { - Set> excludedClasses = new HashSet<>(); - excludedClasses.addAll(this.devModeExcludedClasses); - excludedClasses.addAll(parseClasses(commaDelimitedClasses)); - this.devModeExcludedClasses = Collections.unmodifiableSet(excludedClasses); + devModeExcludedClasses = toNewClassesSet(devModeExcludedClasses, commaDelimitedClasses); } - private Set> parseClasses(String commaDelimitedClasses) { - Set classNames = commaDelimitedStringToSet(commaDelimitedClasses); - Set> classes = new HashSet<>(); + private static Set toNewClassesSet(Set oldClasses, String newDelimitedClasses) throws ConfigurationException { + Set classNames = commaDelimitedStringToSet(newDelimitedClasses); + validateClasses(classNames, Reflection.getCallerClass().getClassLoader()); + Set excludedClasses = new HashSet<>(oldClasses); + excludedClasses.addAll(classNames); + return unmodifiableSet(excludedClasses); + } + + private static void validateClasses(Set classNames, ClassLoader validatingClassLoader) throws ConfigurationException { for (String className : classNames) { try { - classes.add(Class.forName(className)); + validatingClassLoader.loadClass(className); } catch (ClassNotFoundException e) { throw new ConfigurationException("Cannot load class for exclusion/exemption configuration: " + className, e); } } - return classes; } @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) protected void setExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { - Set excludedPackageNamePatterns = new HashSet<>(); - excludedPackageNamePatterns.addAll(this.excludedPackageNamePatterns); - excludedPackageNamePatterns.addAll(parseExcludedPackageNamePatterns(commaDelimitedPackagePatterns)); - this.excludedPackageNamePatterns = Collections.unmodifiableSet(excludedPackageNamePatterns); + excludedPackageNamePatterns = toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns); } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) protected void setDevModeExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { - Set excludedPackageNamePatterns = new HashSet<>(); - excludedPackageNamePatterns.addAll(this.devModeExcludedPackageNamePatterns); - excludedPackageNamePatterns.addAll(parseExcludedPackageNamePatterns(commaDelimitedPackagePatterns)); - this.devModeExcludedPackageNamePatterns = Collections.unmodifiableSet(excludedPackageNamePatterns); + devModeExcludedPackageNamePatterns = toNewPatternsSet(devModeExcludedPackageNamePatterns, commaDelimitedPackagePatterns); } - private Set parseExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { - try { - return commaDelimitedStringToSet(commaDelimitedPackagePatterns) - .stream().map(Pattern::compile).collect(toSet()); - } catch (PatternSyntaxException e) { - throw new ConfigurationException( - "Excluded package name patterns could not be parsed due to invalid regex: " + commaDelimitedPackagePatterns, e); + private static Set toNewPatternsSet(Set oldPatterns, String newDelimitedPatterns) throws ConfigurationException { + Set patterns = commaDelimitedStringToSet(newDelimitedPatterns); + Set newPatterns = new HashSet<>(oldPatterns); + for (String pattern: patterns) { + try { + newPatterns.add(Pattern.compile(pattern)); + } catch (PatternSyntaxException e) { + throw new ConfigurationException("Excluded package name patterns could not be parsed due to invalid regex: " + pattern, e); + } } + return unmodifiableSet(newPatterns); } @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, required = false) protected void setExcludedPackageNames(String commaDelimitedPackageNames) { - Set excludedPackageNames = new HashSet<>(); - excludedPackageNames.addAll(this.excludedPackageNames); - excludedPackageNames.addAll(parseExcludedPackageNames(commaDelimitedPackageNames)); - this.excludedPackageNames = Collections.unmodifiableSet(excludedPackageNames); + excludedPackageNames = toNewPackageNamesSet(excludedPackageNames, commaDelimitedPackageNames); } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES, required = false) protected void setDevModeExcludedPackageNames(String commaDelimitedPackageNames) { - Set excludedPackageNames = new HashSet<>(); - excludedPackageNames.addAll(this.devModeExcludedPackageNames); - excludedPackageNames.addAll(parseExcludedPackageNames(commaDelimitedPackageNames)); - this.devModeExcludedPackageNames = Collections.unmodifiableSet(excludedPackageNames); + devModeExcludedPackageNames = toNewPackageNamesSet(devModeExcludedPackageNames, commaDelimitedPackageNames); + } + + private static Set toNewPackageNamesSet(Set oldPackageNames, String newDelimitedPackageNames) throws ConfigurationException { + Set packageNames = commaDelimitedStringToSet(newDelimitedPackageNames) + .stream().map(s -> strip(s, ".")).collect(toSet()); + if (packageNames.stream().anyMatch(s -> s.matches("(.*?)\\s(.*?)"))) { + throw new ConfigurationException("Excluded package names could not be parsed due to erroneous whitespace characters: " + newDelimitedPackageNames); + } + Set newPackageNames = new HashSet<>(oldPackageNames); + newPackageNames.addAll(packageNames); + return unmodifiableSet(newPackageNames); } @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) public void setExcludedPackageExemptClasses(String commaDelimitedClasses) { - Set> excludedPackageExemptClasses = new HashSet<>(); - excludedPackageExemptClasses.addAll(this.excludedPackageExemptClasses); - excludedPackageExemptClasses.addAll(parseClasses(commaDelimitedClasses)); - this.excludedPackageExemptClasses = Collections.unmodifiableSet(excludedPackageExemptClasses); + excludedPackageExemptClasses = toNewClassesSet(excludedPackageExemptClasses, commaDelimitedClasses); } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) public void setDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) { - Set> excludedPackageExemptClasses = new HashSet<>(); - excludedPackageExemptClasses.addAll(this.devModeExcludedPackageExemptClasses); - excludedPackageExemptClasses.addAll(parseClasses(commaDelimitedClasses)); - this.devModeExcludedPackageExemptClasses = Collections.unmodifiableSet(excludedPackageExemptClasses); - } - - private Set parseExcludedPackageNames(String commaDelimitedPackageNames) { - Set parsedSet = commaDelimitedStringToSet(commaDelimitedPackageNames) - .stream().map(s -> strip(s, ".")).collect(toSet()); - if (parsedSet.stream().anyMatch(s -> s.matches("(.*?)\\s(.*?)"))) { - throw new ConfigurationException("Excluded package names could not be parsed due to erroneous whitespace characters: " + commaDelimitedPackageNames); - } - return parsedSet; + devModeExcludedPackageExemptClasses = toNewClassesSet(devModeExcludedPackageExemptClasses, commaDelimitedClasses); } - public Set> getExcludedClasses() { + public Set getExcludedClasses() { return excludedClasses; } @@ -282,7 +270,7 @@ public Set getExcludedPackageNames() { return excludedPackageNames; } - public Set> getExcludedPackageExemptClasses() { + public Set getExcludedPackageExemptClasses() { return excludedPackageExemptClasses; } 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 48d36a4fda..f65c322b8c 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -35,8 +35,10 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; +import static java.text.MessageFormat.format; import static java.util.Collections.emptySet; import static java.util.Collections.unmodifiableSet; +import static java.util.stream.Collectors.toSet; /** * Allows access decisions to be made on the basis of whether a member is static or not. @@ -49,10 +51,10 @@ public class SecurityMemberAccess implements MemberAccess { private final boolean allowStaticFieldAccess; private Set excludeProperties = emptySet(); private Set acceptProperties = emptySet(); - private Set> excludedClasses = emptySet(); + private Set excludedClasses = emptySet(); private Set excludedPackageNamePatterns = emptySet(); private Set excludedPackageNames = emptySet(); - private Set> excludedPackageExemptClasses = emptySet(); + private Set excludedPackageExemptClasses = emptySet(); private boolean disallowProxyMemberAccess; /** @@ -223,7 +225,7 @@ protected boolean isPackageExcluded(Class targetClass, Class memberClass) List> classesToCheck = Arrays.asList(targetClass, memberClass); for (Class clazz : classesToCheck) { - if (!excludedPackageExemptClasses.contains(clazz) && (isExcludedPackageNames(clazz) || isExcludedPackageNamePatterns(clazz))) { + if (!excludedPackageExemptClasses.contains(clazz.getName()) && (isExcludedPackageNames(clazz) || isExcludedPackageNamePatterns(clazz))) { return true; } } @@ -257,7 +259,7 @@ public static boolean isExcludedPackageNamesStatic(Class clazz, Set e } protected boolean isClassExcluded(Class clazz) { - return excludedClasses.contains(clazz); + return excludedClasses.contains(clazz.getName()); } protected boolean isAcceptableProperty(String name) { @@ -322,14 +324,14 @@ public void useAcceptProperties(Set acceptedProperties) { */ @Deprecated public void setExcludedClasses(Set> excludedClasses) { - useExcludedClasses(excludedClasses); + useExcludedClasses(excludedClasses.stream().map(Class::getName).collect(toSet())); } - public void useExcludedClasses(Set> excludedClasses) { - Set> newExcludedClasses = new HashSet<>(excludedClasses); - newExcludedClasses.add(Object.class); + public void useExcludedClasses(Set excludedClasses) { + Set newExcludedClasses = new HashSet<>(excludedClasses); + newExcludedClasses.add(Object.class.getName()); if (!allowStaticFieldAccess) { - newExcludedClasses.add(Class.class); + newExcludedClasses.add(Class.class.getName()); } this.excludedClasses = unmodifiableSet(newExcludedClasses); } @@ -363,10 +365,10 @@ public void useExcludedPackageNames(Set excludedPackageNames) { */ @Deprecated public void setExcludedPackageExemptClasses(Set> excludedPackageExemptClasses) { - this.excludedPackageExemptClasses = excludedPackageExemptClasses; + useExcludedPackageExemptClasses(excludedPackageExemptClasses.stream().map(Class::getName).collect(toSet())); } - public void useExcludedPackageExemptClasses(Set> excludedPackageExemptClasses) { + public void useExcludedPackageExemptClasses(Set excludedPackageExemptClasses) { this.excludedPackageExemptClasses = excludedPackageExemptClasses; } diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilStrutsTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilStrutsTest.java index c644bfed4c..0eaf517a41 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilStrutsTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilStrutsTest.java @@ -35,8 +35,8 @@ public void testDefaultExcludes() { ognlUtil.setExcludedClasses(""); ognlUtil.setExcludedPackageNames(""); ognlUtil.setExcludedPackageNamePatterns(""); - assertTrue(ognlUtil.getExcludedClasses().size() > 0); - assertTrue(ognlUtil.getExcludedPackageNames().size() > 0); + assertFalse(ognlUtil.getExcludedClasses().isEmpty()); + assertFalse(ognlUtil.getExcludedPackageNames().isEmpty()); try { ognlUtil.getExcludedClasses().clear(); diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java index 0cafd799b9..7368898868 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java @@ -903,7 +903,7 @@ public void testBeanMapExpressions() throws OgnlException, NoSuchMethodException assertEquals(foo.getTitle(), expression); SecurityMemberAccess sma = (SecurityMemberAccess) ((OgnlContext) context).getMemberAccess(); - assertFalse(sma.isAccessible(context, sma, sma.getClass().getDeclaredMethod("setExcludedClasses", Set.class), "excludedClasses")); + assertFalse(sma.isAccessible(context, sma, sma.getClass().getDeclaredMethod("useExcludedClasses", Set.class), "excludedClasses")); } public void testNullProperties() { @@ -1366,7 +1366,7 @@ public void testDefaultOgnlUtilExclusionsAlternateConstructorPopulated() { } public void testOgnlUtilExcludedAdditivity() { - Set> excludedClasses; + Set excludedClasses; Set excludedPackageNamePatterns; Iterator excludedPackageNamePatternsIterator; Set excludedPackageNames; @@ -1376,17 +1376,17 @@ public void testOgnlUtilExcludedAdditivity() { excludedClasses = ognlUtil.getExcludedClasses(); assertNotNull("initial excluded classes null?", excludedClasses); assertEquals("initial excluded classes size not 2 after adds?", 2, excludedClasses.size()); - assertTrue("String not in exclusions?", excludedClasses.contains(String.class)); - assertTrue("Integer not in exclusions?", excludedClasses.contains(Integer.class)); + assertTrue("String not in exclusions?", excludedClasses.contains(String.class.getName())); + assertTrue("Integer not in exclusions?", excludedClasses.contains(Integer.class.getName())); ognlUtil.setExcludedClasses("java.lang.Boolean,java.lang.Double"); internalTestOgnlUtilExclusionsImmutable(ognlUtil); excludedClasses = ognlUtil.getExcludedClasses(); assertNotNull("updated excluded classes null?", excludedClasses); assertEquals("updated excluded classes size not 4 after adds?", 4, excludedClasses.size()); - assertTrue("String not in exclusions?", excludedClasses.contains(String.class)); - assertTrue("Integer not in exclusions?", excludedClasses.contains(Integer.class)); - assertTrue("String not in exclusions?", excludedClasses.contains(Boolean.class)); - assertTrue("Integer not in exclusions?", excludedClasses.contains(Double.class)); + assertTrue("String not in exclusions?", excludedClasses.contains(String.class.getName())); + assertTrue("Integer not in exclusions?", excludedClasses.contains(Integer.class.getName())); + assertTrue("String not in exclusions?", excludedClasses.contains(Boolean.class.getName())); + assertTrue("Integer not in exclusions?", excludedClasses.contains(Double.class.getName())); ognlUtil.setExcludedPackageNamePatterns("fakepackage1.*,fakepackage2.*"); internalTestOgnlUtilExclusionsImmutable(ognlUtil); @@ -1617,7 +1617,7 @@ public void testApplyExpressionMaxLength() { } private void internalTestInitialEmptyOgnlUtilExclusions(OgnlUtil ognlUtilParam) { - Set> excludedClasses = ognlUtilParam.getExcludedClasses(); + Set excludedClasses = ognlUtilParam.getExcludedClasses(); assertNotNull("parameter (default) exluded classes null?", excludedClasses); assertTrue("parameter (default) exluded classes not empty?", excludedClasses.isEmpty()); @@ -1632,65 +1632,34 @@ private void internalTestInitialEmptyOgnlUtilExclusions(OgnlUtil ognlUtilParam) private void internalTestOgnlUtilExclusionsImmutable(OgnlUtil ognlUtilParam) { Pattern somePattern = Pattern.compile("SomeRegexPattern"); - Set> excludedClasses = ognlUtilParam.getExcludedClasses(); - assertNotNull("parameter exluded classes null?", excludedClasses); - try { - excludedClasses.clear(); - fail("parameter excluded classes modifiable?"); - } catch (UnsupportedOperationException uoe) { - // Expected failure - } - try { - excludedClasses.add(Integer.class); - fail("parameter excluded classes modifiable?"); - } catch (UnsupportedOperationException uoe) { - // Expected failure - } - try { - excludedClasses.remove(Integer.class); - fail("parameter excluded classes modifiable?"); - } catch (UnsupportedOperationException uoe) { - // Expected failure - } - - Set excludedPackageNamePatterns = ognlUtilParam.getExcludedPackageNamePatterns(); - assertNotNull("parameter exluded package name patterns null?", excludedPackageNamePatterns); - try { - excludedPackageNamePatterns.clear(); - fail("parameter excluded package name patterns modifiable?"); - } catch (UnsupportedOperationException uoe) { - // Expected failure - } - try { - excludedPackageNamePatterns.add(somePattern); - fail("parameter excluded package name patterns modifiable?"); - } catch (UnsupportedOperationException uoe) { - // Expected failure - } - try { - excludedPackageNamePatterns.remove(somePattern); - fail("parameter excluded package name patterns modifiable?"); - } catch (UnsupportedOperationException uoe) { - // Expected failure - } + assertImmutability(ognlUtilParam.getExcludedClasses(), Integer.class.getName()); + assertImmutability(ognlUtilParam.getExcludedPackageNamePatterns(), somePattern); + assertImmutability(ognlUtilParam.getExcludedPackageNames(), "somepackagename"); + assertImmutability(ognlUtilParam.getExcludedPackageExemptClasses(), Integer.class.getName()); + } - Set excludedPackageNames = ognlUtilParam.getExcludedPackageNames(); - assertNotNull("parameter exluded package names null?", excludedPackageNames); + public static void assertImmutability(Collection collection, T item) { + assertNotNull("Collection is null", collection); try { - excludedPackageNames.clear(); - fail("parameter excluded package names modifiable?"); + if (!collection.isEmpty()) { + collection.clear(); + fail("Collection could be cleared"); + } } catch (UnsupportedOperationException uoe) { // Expected failure } try { - excludedPackageNames.add("somepackagename"); - fail("parameter excluded package names modifiable?"); + collection.add(item); + fail("Collection could be added to"); } catch (UnsupportedOperationException uoe) { // Expected failure } try { - excludedPackageNames.remove("somepackagename"); - fail("parameter excluded package names modifiable?"); + int prevSize = collection.size(); + collection.remove(item); + if (prevSize != collection.size()) { + fail("Collection could be removed from"); + } } catch (UnsupportedOperationException uoe) { // Expected failure } 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 1a6d9697d1..b7b1bce091 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -25,7 +25,6 @@ import java.lang.reflect.Field; import java.lang.reflect.Member; import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -33,6 +32,7 @@ import java.util.Set; import java.util.regex.Pattern; +import static java.util.Collections.singletonList; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -73,8 +73,8 @@ public void testClassExclusion() throws Exception { String propertyName = "stringField"; Member member = FooBar.class.getDeclaredMethod(formGetterName(propertyName)); - Set> excluded = new HashSet<>(); - excluded.add(FooBar.class); + Set excluded = new HashSet<>(); + excluded.add(FooBar.class.getName()); sma.useExcludedClasses(excluded); // when @@ -116,8 +116,8 @@ public void testInterfaceInheritanceExclusion() throws Exception { String propertyName = "barLogic"; Member member = BarInterface.class.getMethod(propertyName); - Set> excluded = new HashSet<>(); - excluded.add(BarInterface.class); + Set excluded = new HashSet<>(); + excluded.add(BarInterface.class.getName()); sma.useExcludedClasses(excluded); // when @@ -133,8 +133,8 @@ public void testMiddleOfInheritanceExclusion1() throws Exception { String propertyName = "fooLogic"; Member member = FooBar.class.getMethod(propertyName); - Set> excluded = new HashSet<>(); - excluded.add(BarInterface.class); + Set excluded = new HashSet<>(); + excluded.add(BarInterface.class.getName()); sma.useExcludedClasses(excluded); // when @@ -150,8 +150,8 @@ public void testMiddleOfInheritanceExclusion2() throws Exception { String propertyName = "barLogic"; Member member = BarInterface.class.getMethod(propertyName); - Set> excluded = new HashSet<>(); - excluded.add(BarInterface.class); + Set excluded = new HashSet<>(); + excluded.add(BarInterface.class.getName()); sma.useExcludedClasses(excluded); // when @@ -167,8 +167,8 @@ public void testMiddleOfInheritanceExclusion3() throws Exception { String propertyName = "barLogic"; Member member = BarInterface.class.getMethod(propertyName); - Set> excluded = new HashSet<>(); - excluded.add(FooInterface.class); + Set excluded = new HashSet<>(); + excluded.add(FooInterface.class.getName()); sma.useExcludedClasses(excluded); // when @@ -202,8 +202,8 @@ public void testPackageExclusionExemption() throws Exception { excluded.add(Pattern.compile("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*")); sma.useExcludedPackageNamePatterns(excluded); - Set> allowed = new HashSet<>(); - allowed.add(FooBar.class); + Set allowed = new HashSet<>(); + allowed.add(FooBar.class.getName()); sma.useExcludedPackageExemptClasses(allowed); String propertyName = "stringField"; @@ -240,8 +240,8 @@ public void testPackageNameExclusionExemption() throws Exception { excluded.add(FooBar.class.getPackage().getName()); sma.useExcludedPackageNames(excluded); - Set> allowed = new HashSet<>(); - allowed.add(FooBar.class); + Set allowed = new HashSet<>(); + allowed.add(FooBar.class.getName()); sma.useExcludedPackageExemptClasses(allowed); String propertyName = "stringField"; @@ -262,8 +262,8 @@ public void testPackageNameExclusionExemption2() throws Exception { sma.useExcludedPackageNames(excluded); // Exemption must exist for both classes (target and member) if they both match a banned package - Set> allowed = new HashSet<>(); - allowed.add(BarInterface.class); + Set allowed = new HashSet<>(); + allowed.add(BarInterface.class.getName()); sma.useExcludedPackageExemptClasses(allowed); String propertyName = "barLogic"; @@ -284,9 +284,9 @@ public void testPackageNameExclusionExemption3() throws Exception { sma.useExcludedPackageNames(excluded); // Exemption must exist for both classes (target and member) if they both match a banned package - Set> allowed = new HashSet<>(); - allowed.add(BarInterface.class); - allowed.add(FooBar.class); + Set allowed = new HashSet<>(); + allowed.add(BarInterface.class.getName()); + allowed.add(FooBar.class.getName()); sma.useExcludedPackageExemptClasses(allowed); String propertyName = "barLogic"; @@ -344,7 +344,7 @@ public void testAccessEnum() throws Exception { @Test public void testAccessStaticMethod() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); // when Member method = StaticTester.class.getMethod("sayHello"); @@ -357,7 +357,7 @@ public void testAccessStaticMethod() throws Exception { @Test public void testAccessStaticField() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); // when Member method = StaticTester.class.getField("MAX_VALUE"); @@ -371,7 +371,7 @@ public void testAccessStaticField() throws Exception { public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); // when Member method = StaticTester.class.getField("MAX_VALUE"); @@ -383,7 +383,7 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // public static final test // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); // when method = StaticTester.class.getField("MIN_VALUE"); @@ -395,7 +395,7 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // package static test // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); // when method = StaticTester.getFieldByName("PACKAGE_STRING"); @@ -407,7 +407,7 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // package final static test // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); // when method = StaticTester.getFieldByName("FINAL_PACKAGE_STRING"); @@ -419,7 +419,7 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // protected static test // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); // when method = StaticTester.getFieldByName("PROTECTED_STRING"); @@ -431,7 +431,7 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // protected final static test // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); // when method = StaticTester.getFieldByName("FINAL_PROTECTED_STRING"); @@ -443,7 +443,7 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // private static test // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); // when method = StaticTester.getFieldByName("PRIVATE_STRING"); @@ -455,7 +455,7 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // private final static test // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); // when method = StaticTester.getFieldByName("FINAL_PRIVATE_STRING"); @@ -469,7 +469,6 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { public void testBlockedStaticFieldWhenFlagIsFalse() throws Exception { // given assignNewSma(false); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); // when Member method = StaticTester.class.getField("MAX_VALUE"); @@ -481,7 +480,6 @@ public void testBlockedStaticFieldWhenFlagIsFalse() throws Exception { // public static final test // given assignNewSma(false); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); // when method = StaticTester.class.getField("MIN_VALUE"); @@ -493,7 +491,6 @@ public void testBlockedStaticFieldWhenFlagIsFalse() throws Exception { // package static test // given assignNewSma(false); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); // when method = StaticTester.getFieldByName("PACKAGE_STRING"); @@ -505,7 +502,6 @@ public void testBlockedStaticFieldWhenFlagIsFalse() throws Exception { // package final static test // given assignNewSma(false); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); // when method = StaticTester.getFieldByName("FINAL_PACKAGE_STRING"); @@ -517,7 +513,6 @@ public void testBlockedStaticFieldWhenFlagIsFalse() throws Exception { // protected static test // given assignNewSma(false); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); // when method = StaticTester.getFieldByName("PROTECTED_STRING"); @@ -529,7 +524,6 @@ public void testBlockedStaticFieldWhenFlagIsFalse() throws Exception { // protected final static test // given assignNewSma(false); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); // when method = StaticTester.getFieldByName("FINAL_PROTECTED_STRING"); @@ -541,7 +535,6 @@ public void testBlockedStaticFieldWhenFlagIsFalse() throws Exception { // private static test // given assignNewSma(false); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); // when method = StaticTester.getFieldByName("PRIVATE_STRING"); @@ -553,7 +546,6 @@ public void testBlockedStaticFieldWhenFlagIsFalse() throws Exception { // private final static test // given assignNewSma(false); - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); // when method = StaticTester.getFieldByName("FINAL_PRIVATE_STRING"); @@ -566,7 +558,7 @@ public void testBlockedStaticFieldWhenFlagIsFalse() throws Exception { @Test public void testBlockedStaticFieldWhenClassIsExcluded() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(Arrays.asList(Class.class, StaticTester.class))); + sma.useExcludedClasses(new HashSet<>(Arrays.asList(Class.class.getName(), StaticTester.class.getName()))); // when Member method = StaticTester.class.getField("MAX_VALUE"); @@ -579,7 +571,7 @@ public void testBlockedStaticFieldWhenClassIsExcluded() throws Exception { @Test public void testBlockStaticMethodAccess() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); // when Member method = StaticTester.class.getMethod("sayHello"); @@ -592,7 +584,7 @@ public void testBlockStaticMethodAccess() throws Exception { @Test public void testBlockAccessIfClassIsExcluded() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); // when Member method = Class.class.getMethod("getClassLoader"); @@ -605,7 +597,7 @@ public void testBlockAccessIfClassIsExcluded() throws Exception { @Test public void testBlockAccessIfClassIsExcluded_2() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(ClassLoader.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(ClassLoader.class.getName()))); // when Member method = ClassLoader.class.getMethod("loadClass", String.class); @@ -619,7 +611,7 @@ public void testBlockAccessIfClassIsExcluded_2() throws Exception { @Test public void testAllowAccessIfClassIsNotExcluded() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(ClassLoader.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(ClassLoader.class.getName()))); // when Member method = Class.class.getMethod("getClassLoader"); @@ -632,7 +624,7 @@ public void testAllowAccessIfClassIsNotExcluded() throws Exception { @Test public void testIllegalArgumentExceptionExpectedForTargetMemberMismatch() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(Collections.singletonList(Class.class))); + sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); // when Member method = ClassLoader.class.getMethod("loadClass", String.class); @@ -669,12 +661,12 @@ public void testAccessPrimitiveDoubleWithNames() throws Exception { sma.useExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("ognl.,javax.")); - Set> excluded = new HashSet<>(); - excluded.add(Object.class); - excluded.add(Runtime.class); - excluded.add(System.class); - excluded.add(Class.class); - excluded.add(ClassLoader.class); + Set excluded = new HashSet<>(); + excluded.add(Object.class.getName()); + excluded.add(Runtime.class.getName()); + excluded.add(System.class.getName()); + excluded.add(Class.class.getName()); + excluded.add(ClassLoader.class.getName()); sma.useExcludedClasses(excluded); String propertyName = "doubleValue"; @@ -737,8 +729,8 @@ public void testAccessPrimitiveDoubleWithPackageRegExs() throws Exception { @Test public void testAccessMemberAccessIsAccessible() throws Exception { // given - Set> excluded = new HashSet<>(); - excluded.add(ognl.MemberAccess.class); + Set excluded = new HashSet<>(); + excluded.add(ognl.MemberAccess.class.getName()); sma.useExcludedClasses(excluded); String propertyName = "excludedClasses"; @@ -755,8 +747,8 @@ public void testAccessMemberAccessIsAccessible() throws Exception { @Test public void testAccessMemberAccessIsBlocked() throws Exception { // given - Set> excluded = new HashSet<>(); - excluded.add(SecurityMemberAccess.class); + Set excluded = new HashSet<>(); + excluded.add(SecurityMemberAccess.class.getName()); sma.useExcludedClasses(excluded); String propertyName = "excludedClasses"; From a9666272eb270ca836e537c90252d4e1f232ecb1 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 29 Aug 2023 09:21:01 +1000 Subject: [PATCH 4/9] WW-5341 Move proxy check to be first --- .../com/opensymphony/xwork2/ognl/SecurityMemberAccess.java | 5 +++++ 1 file changed, 5 insertions(+) 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 f65c322b8c..e75805d71f 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -111,6 +111,11 @@ public boolean isAccessible(Map context, Object target, Member member, String pr throw new IllegalArgumentException("Target does not match member!"); } + if (disallowProxyMemberAccess && ProxyUtil.isProxyMember(member, target)) { + LOG.warn("Access to proxy is blocked! Target class [{}] of target [{}], member [{}]", targetClass, target, member); + return false; + } + if (!checkPublicMemberAccess(memberModifiers)) { LOG.warn("Access to non-public [{}] is blocked!", member); return false; From 1bbcc17c747210eaea27a0e9182971b7bca075d0 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 29 Aug 2023 09:21:30 +1000 Subject: [PATCH 5/9] WW-5341 Split package exclusion check --- .../xwork2/ognl/SecurityMemberAccess.java | 28 +++++-------------- .../xwork2/ognl/SecurityMemberAccessTest.java | 6 ++-- .../test/ExternalSecurityMemberAccess.java | 4 +-- 3 files changed, 12 insertions(+), 26 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 e75805d71f..3e1c69d5db 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -152,19 +152,16 @@ public boolean isAccessible(Map context, Object target, Member member, String pr LOG.warn("The use of the default (unnamed) package is discouraged!"); } - if (isPackageExcluded(targetClass, memberClass)) { - LOG.warn( - "Package [{}] of target class [{}] of target [{}] or package [{}] of member [{}] are excluded!", + if (isPackageExcluded(targetClass)) { + LOG.warn("Package [{}] of target class [{}] of target [{}] is excluded!", targetClass.getPackage(), targetClass, - target, - memberClass.getPackage(), - member); + target); return false; } - if (disallowProxyMemberAccess && ProxyUtil.isProxyMember(member, target)) { - LOG.warn("Access to proxy is blocked! Target class [{}] of target [{}], member [{}]", targetClass, target, member); + if (isPackageExcluded(memberClass)) { + LOG.warn("Package [{}] of member [{}] are excluded!", memberClass.getPackage(), member); return false; } @@ -222,19 +219,8 @@ protected boolean checkEnumAccess(Object target, Member member) { return false; } - protected boolean isPackageExcluded(Class targetClass, Class memberClass) { - if (targetClass == null || memberClass == null) { - throw new IllegalArgumentException( - "Parameters should never be null - if member is static, targetClass should be the same as memberClass."); - } - - List> classesToCheck = Arrays.asList(targetClass, memberClass); - for (Class clazz : classesToCheck) { - if (!excludedPackageExemptClasses.contains(clazz.getName()) && (isExcludedPackageNames(clazz) || isExcludedPackageNamePatterns(clazz))) { - return true; - } - } - return false; + protected boolean isPackageExcluded(Class clazz) { + return !excludedPackageExemptClasses.contains(clazz.getName()) && (isExcludedPackageNames(clazz) || isExcludedPackageNamePatterns(clazz)); } public static String toPackageName(Class clazz) { 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 b7b1bce091..15a9885c9e 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -309,7 +309,7 @@ public void testDefaultPackageExclusion() throws Exception { Class clazz = Class.forName("PackagelessAction"); // when - boolean actual = sma.isPackageExcluded(clazz, clazz); + boolean actual = sma.isPackageExcluded(clazz); // then assertFalse("default package is excluded!", actual); @@ -325,7 +325,7 @@ public void testDefaultPackageExclusion2() throws Exception { Class clazz = Class.forName("PackagelessAction"); // when - boolean actual = sma.isPackageExcluded(clazz, clazz); + boolean actual = sma.isPackageExcluded(clazz); // then assertTrue("default package isn't excluded!", actual); @@ -768,7 +768,7 @@ public void testPackageNameExclusionAsCommaDelimited() { sma.useExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("java.lang")); // when - boolean actual = sma.isPackageExcluded(String.class, String.class); + boolean actual = sma.isPackageExcluded(String.class); // then assertTrue("package java.lang. is accessible!", actual); diff --git a/core/src/test/java/com/test/ExternalSecurityMemberAccess.java b/core/src/test/java/com/test/ExternalSecurityMemberAccess.java index 41183fa39e..a53f1736bf 100644 --- a/core/src/test/java/com/test/ExternalSecurityMemberAccess.java +++ b/core/src/test/java/com/test/ExternalSecurityMemberAccess.java @@ -27,7 +27,7 @@ class ExternalSecurityMemberAccess extends SecurityMemberAccess { } @Override - public boolean isPackageExcluded(Class targetClass, Class memberClass) { - return super.isPackageExcluded(targetClass, memberClass); + public boolean isPackageExcluded(Class clazz) { + return super.isPackageExcluded(clazz); } } From a352132f00baf771c25451281758f6764c277787 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 29 Aug 2023 09:36:12 +1000 Subject: [PATCH 6/9] WW-5341 Clean up OgnlUtilTest --- .../opensymphony/xwork2/ognl/OgnlUtil.java | 3 +- .../xwork2/ognl/OgnlUtilTest.java | 65 ++++++++++--------- 2 files changed, 35 insertions(+), 33 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 bbfdd72bbd..6b90bd6741 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -102,8 +102,7 @@ public class OgnlUtil { @Deprecated public OgnlUtil() { // Instantiate default Expression and BeanInfo caches (factories must be non-null). - this(new DefaultOgnlExpressionCacheFactory<>(), - new DefaultOgnlBeanInfoCacheFactory<>()); + this(new DefaultOgnlExpressionCacheFactory<>(), new DefaultOgnlBeanInfoCacheFactory<>()); } /** diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java index 7368898868..3dbcbdafd6 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java @@ -238,7 +238,9 @@ public void testMethodExpressionIsCachedIrrespectiveOfItsExecutionStatus() throw // Method expression which executes with success try { ognlUtil.callMethod("getBar()", context, foo); - assertTrue("Successfully executed method expression must have been cached", ognlUtil.expressionCacheSize() == 1); + assertEquals("Successfully executed method expression must have been cached", + 1, + ognlUtil.expressionCacheSize()); } catch (Exception ex) { fail("Method expression execution should have succeeded here. Exception: " + ex); } @@ -541,7 +543,7 @@ public void testIncudeExcludes() { assertEquals(b1.getSomethingElse(), b2.getSomethingElse()); // id properties did not - assertEquals(b2.getId(), new Long(2)); + assertEquals(b2.getId(), Long.valueOf(2)); } @@ -959,8 +961,8 @@ public void testGetBeanMap() throws Exception { assertNotNull(beans); assertEquals(22, beans.size()); assertEquals("Hello Santa", beans.get("title")); - assertEquals(new Long("123"), beans.get("ALong")); - assertEquals(new Integer("44"), beans.get("number")); + assertEquals(Long.valueOf("123"), beans.get("ALong")); + assertEquals(Integer.valueOf("44"), beans.get("number")); assertEquals(bar, beans.get("bar")); assertEquals(Boolean.TRUE, beans.get("useful")); } @@ -971,7 +973,7 @@ public void testGetBeanMapNoReadMethod() throws Exception { Map beans = ognlUtil.getBeanMap(bar); assertEquals(2, beans.size()); - assertEquals(new Integer("1"), beans.get("id")); + assertEquals(Integer.valueOf("1"), beans.get("id")); assertEquals("There is no read method for bar", beans.get("bar")); } @@ -1345,13 +1347,13 @@ public void testDefaultOgnlUtilExclusions() { public void testDefaultOgnlUtilAlternateConstructorArguments() { // Code coverage test for the OgnlUtil alternate constructor method, and verify expected behaviour. try { - OgnlUtil basicOgnlUtil = new OgnlUtil(new DefaultOgnlExpressionCacheFactory(), null); + OgnlUtil basicOgnlUtil = new OgnlUtil(new DefaultOgnlExpressionCacheFactory<>(), null); fail("null beanInfoCacheFactory should result in exception"); } catch (IllegalArgumentException iaex) { // expected result } try { - OgnlUtil basicOgnlUtil = new OgnlUtil(null, new DefaultOgnlBeanInfoCacheFactory, BeanInfo>()); + OgnlUtil basicOgnlUtil = new OgnlUtil(null, new DefaultOgnlBeanInfoCacheFactory<>()); fail("null expressionCacheFactory should result in exception"); } catch (IllegalArgumentException iaex) { // expected result @@ -1359,7 +1361,8 @@ public void testDefaultOgnlUtilAlternateConstructorArguments() { } public void testDefaultOgnlUtilExclusionsAlternateConstructorPopulated() { - OgnlUtil basicOgnlUtil = new OgnlUtil(new DefaultOgnlExpressionCacheFactory(), new DefaultOgnlBeanInfoCacheFactory, BeanInfo>()); + OgnlUtil basicOgnlUtil = new OgnlUtil(new DefaultOgnlExpressionCacheFactory<>(), + new DefaultOgnlBeanInfoCacheFactory<>()); internalTestInitialEmptyOgnlUtilExclusions(basicOgnlUtil); internalTestOgnlUtilExclusionsImmutable(basicOgnlUtil); @@ -1734,7 +1737,7 @@ public void testGetExcludedClasses() { public void testGetExcludedClassesAlternateConstructorPopulated() { // Getter should return an immutable collection - OgnlUtil util = new OgnlUtil(new DefaultOgnlExpressionCacheFactory(), new DefaultOgnlBeanInfoCacheFactory, BeanInfo>()); + OgnlUtil util = new OgnlUtil(new DefaultOgnlExpressionCacheFactory<>(), new DefaultOgnlBeanInfoCacheFactory<>()); util.setExcludedClasses("java.lang.Runtime,java.lang.ProcessBuilder,java.net.URL"); assertEquals(util.getExcludedClasses().size(), 3); try { @@ -1762,7 +1765,7 @@ public void testGetExcludedPackageNamePatterns() { public void testGetExcludedPackageNamePatternsAlternateConstructorPopulated() { // Getter should return an immutable collection - OgnlUtil util = new OgnlUtil(new DefaultOgnlExpressionCacheFactory(), new DefaultOgnlBeanInfoCacheFactory, BeanInfo>()); + OgnlUtil util = new OgnlUtil(new DefaultOgnlExpressionCacheFactory<>(), new DefaultOgnlBeanInfoCacheFactory<>()); util.setExcludedPackageNamePatterns("java.lang."); assertEquals(util.getExcludedPackageNamePatterns().size(), 1); try { @@ -1779,24 +1782,24 @@ public void testOgnlUtilDefaultCacheClass() throws OgnlException { assertEquals("Initial evictionLimit did not match initial value", 2, defaultCache.getEvictionLimit()); defaultCache.setEvictionLimit(3); assertEquals("Updated evictionLimit did not match updated value", 3, defaultCache.getEvictionLimit()); - String lookupResult = defaultCache.get(Integer.valueOf(0)); + String lookupResult = defaultCache.get(0); assertNull("Lookup of empty cache returned non-null value ?", lookupResult); - defaultCache.put(Integer.valueOf(0), "Zero"); - lookupResult = defaultCache.get(Integer.valueOf(0)); + defaultCache.put(0, "Zero"); + lookupResult = defaultCache.get(0); assertEquals("Retrieved value does not match put value ?", "Zero", lookupResult); - defaultCache.put(Integer.valueOf(1), "One"); - defaultCache.put(Integer.valueOf(2), "Two"); + defaultCache.put(1, "One"); + defaultCache.put(2, "Two"); assertEquals("Default cache not size evictionlimit after adding three values ?", defaultCache.getEvictionLimit(), defaultCache.size()); - lookupResult = defaultCache.get(Integer.valueOf(2)); + lookupResult = defaultCache.get(2); assertEquals("Retrieved value does not match put value ?", "Two", lookupResult); - defaultCache.put(Integer.valueOf(3), "Three"); + defaultCache.put(3, "Three"); assertEquals("Default cache not size zero after an add that exceeded the evection limit ?", 0, defaultCache.size()); - lookupResult = defaultCache.get(Integer.valueOf(0)); + lookupResult = defaultCache.get(0); assertNull("Lookup of value 0 (should have been evicted with everything) returned non-null value ?", lookupResult); - lookupResult = defaultCache.get(Integer.valueOf(3)); + lookupResult = defaultCache.get(3); assertNull("Lookup of value 3 (should have been evicted with everything) returned non-null value ?", lookupResult); - defaultCache.putIfAbsent(Integer.valueOf(2), "Two"); - lookupResult = defaultCache.get(Integer.valueOf(2)); + defaultCache.putIfAbsent(2, "Two"); + lookupResult = defaultCache.get(2); assertEquals("Retrieved value does not match put value ?", "Two", lookupResult); defaultCache.clear(); assertEquals("Default cache not empty after clear ?", 0, defaultCache.size()); @@ -1807,22 +1810,22 @@ public void testOgnlUtilLRUCacheClass() throws OgnlException { assertEquals("Initial evictionLimit did not match initial value", 2, lruCache.getEvictionLimit()); lruCache.setEvictionLimit(3); assertEquals("Updated evictionLimit did not match updated value", 3, lruCache.getEvictionLimit()); - String lookupResult = lruCache.get(Integer.valueOf(0)); + String lookupResult = lruCache.get(0); assertNull("Lookup of empty cache returned non-null value ?", lookupResult); - lruCache.put(Integer.valueOf(0), "Zero"); - lookupResult = lruCache.get(Integer.valueOf(0)); + lruCache.put(0, "Zero"); + lookupResult = lruCache.get(0); assertEquals("Retrieved value does not match put value ?", "Zero", lookupResult); - lruCache.put(Integer.valueOf(1), "One"); - lruCache.put(Integer.valueOf(2), "Two"); + lruCache.put(1, "One"); + lruCache.put(2, "Two"); assertEquals("LRU cache not size evictionlimit after adding three values ?", lruCache.getEvictionLimit(), lruCache.size()); - lookupResult = lruCache.get(Integer.valueOf(2)); + lookupResult = lruCache.get(2); assertEquals("Retrieved value does not match put value ?", "Two", lookupResult); - lruCache.put(Integer.valueOf(3), "Three"); + lruCache.put(3, "Three"); assertEquals("LRU cache not size evictionlimit after adding values ?", lruCache.getEvictionLimit(), lruCache.size()); - lookupResult = lruCache.get(Integer.valueOf(0)); + lookupResult = lruCache.get(0); assertNull("Lookup of value 0 (should have dropped off LRU cache) returned non-null value ?", lookupResult); - lruCache.putIfAbsent(Integer.valueOf(2), "Two"); - lookupResult = lruCache.get(Integer.valueOf(2)); + lruCache.putIfAbsent(2, "Two"); + lookupResult = lruCache.get(2); assertEquals("Retrieved value does not match put value ?", "Two", lookupResult); lruCache.clear(); assertEquals("LRU cache not empty after clear ?", 0, lruCache.size()); From 7c1c674a023d30fcb50c8ff6e57455641208ba94 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 29 Aug 2023 09:44:50 +1000 Subject: [PATCH 7/9] WW-5341 Add unit test for excluded pattern validation --- .../test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java index 3dbcbdafd6..8694d97213 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java @@ -1763,6 +1763,10 @@ public void testGetExcludedPackageNamePatterns() { } } + public void testInvalidExcludedPackageNamePatterns() { + assertThrows(ConfigurationException.class, () -> ognlUtil.setExcludedPackageNamePatterns("[")); + } + public void testGetExcludedPackageNamePatternsAlternateConstructorPopulated() { // Getter should return an immutable collection OgnlUtil util = new OgnlUtil(new DefaultOgnlExpressionCacheFactory<>(), new DefaultOgnlBeanInfoCacheFactory<>()); From a755c30cad882939a58adf083fa91fc10e8dc53a Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 30 Aug 2023 12:43:30 +1000 Subject: [PATCH 8/9] WW-5341 Fix default ClassLoader --- core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java | 3 +-- 1 file changed, 1 insertion(+), 2 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 6b90bd6741..8f2872669c 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -25,7 +25,6 @@ import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor; import com.opensymphony.xwork2.util.CompoundRoot; import com.opensymphony.xwork2.util.reflection.ReflectionException; -import jdk.internal.reflect.Reflection; import ognl.ClassResolver; import ognl.Ognl; import ognl.OgnlContext; @@ -187,7 +186,7 @@ protected void setDevModeExcludedClasses(String commaDelimitedClasses) { private static Set toNewClassesSet(Set oldClasses, String newDelimitedClasses) throws ConfigurationException { Set classNames = commaDelimitedStringToSet(newDelimitedClasses); - validateClasses(classNames, Reflection.getCallerClass().getClassLoader()); + validateClasses(classNames, OgnlUtil.class.getClassLoader()); Set excludedClasses = new HashSet<>(oldClasses); excludedClasses.addAll(classNames); return unmodifiableSet(excludedClasses); From bc85d35a2928d74bdcd409e2ab5b3fd8b189e967 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 30 Aug 2023 13:03:30 +1000 Subject: [PATCH 9/9] WW-5341 Make validation more efficient --- core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 8f2872669c..b0dc173978 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -238,7 +238,7 @@ protected void setDevModeExcludedPackageNames(String commaDelimitedPackageNames) private static Set toNewPackageNamesSet(Set oldPackageNames, String newDelimitedPackageNames) throws ConfigurationException { Set packageNames = commaDelimitedStringToSet(newDelimitedPackageNames) .stream().map(s -> strip(s, ".")).collect(toSet()); - if (packageNames.stream().anyMatch(s -> s.matches("(.*?)\\s(.*?)"))) { + if (packageNames.stream().anyMatch(s -> Pattern.compile("\\s").matcher(s).find())) { throw new ConfigurationException("Excluded package names could not be parsed due to erroneous whitespace characters: " + newDelimitedPackageNames); } Set newPackageNames = new HashSet<>(oldPackageNames);