[ https://issues.apache.org/jira/browse/WW-5288?focusedWorklogId=848500&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-848500 ]
ASF GitHub Bot logged work on WW-5288: -------------------------------------- Author: ASF GitHub Bot Created on: 02/Mar/23 04:09 Start Date: 02/Mar/23 04:09 Worklog Time Spent: 10m Work Description: kusalk commented on code in PR #664: URL: https://github.com/apache/struts/pull/664#discussion_r1122577982 ########## core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java: ########## @@ -197,51 +208,61 @@ protected boolean checkEnumAccess(Object target, Member member) { return false; } - protected boolean isPackageExcluded(Package targetPackage, Package memberPackage) { - if (targetPackage == null || memberPackage == null) { - LOG.warn("The use of the default (unnamed) package is discouraged!"); + 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."); } - String targetPackageName = targetPackage == null ? "" : targetPackage.getName(); - String memberPackageName = memberPackage == null ? "" : memberPackage.getName(); + Set<Class<?>> classesToCheck = new HashSet<>(); + classesToCheck.add(targetClass); + classesToCheck.add(memberClass); - for (Pattern pattern : excludedPackageNamePatterns) { - if (pattern.matcher(targetPackageName).matches() || pattern.matcher(memberPackageName).matches()) { + for (Class<?> clazz : classesToCheck) { + if (!isExcludedPackageExempt(clazz) && (isExcludedPackageNamePatterns(clazz) || isExcludedPackageNames(clazz))) { return true; } } + return false; + } - targetPackageName = targetPackageName + "."; - memberPackageName = memberPackageName + "."; + protected String toPackageName(Class<?> clazz) { + if (clazz.getPackage() == null) { + return ""; + } else { + return clazz.getPackage().getName(); + } + } - for (String packageName : excludedPackageNames) { - if (targetPackageName.startsWith(packageName) || memberPackageName.startsWith(packageName)) { + protected boolean isExcludedPackageNamePatterns(Class<?> clazz) { + String packageName = toPackageName(clazz); + for (Pattern pattern : excludedPackageNamePatterns) { + if (pattern.matcher(packageName).matches()) { return true; } } - return false; } - protected boolean isClassExcluded(Class<?> clazz) { - if (clazz == Object.class || (clazz == Class.class && !allowStaticFieldAccess)) { - return true; - } - for (Class<?> excludedClass : excludedClasses) { - if (clazz.isAssignableFrom(excludedClass)) { + protected boolean isExcludedPackageNames(Class<?> clazz) { + String suffixedPackageName = toPackageName(clazz) + "."; + for (String excludedPackageName : excludedPackageNames) { + if (suffixedPackageName.startsWith(excludedPackageName)) { return true; } } return false; } - protected boolean isClassExcludedPackageExempt(Class<?> clazz) { - for (Class<?> excludedPackageExemptClass : excludedPackageExemptClasses) { - if (clazz.isAssignableFrom(excludedPackageExemptClass)) { - return true; - } + protected boolean isClassExcluded(Class<?> clazz) { + if (clazz == Object.class || (clazz == Class.class && !allowStaticFieldAccess)) { + return true; } - return false; + return excludedClasses.stream().anyMatch(clazz::isAssignableFrom); + } + + protected boolean isExcludedPackageExempt(Class<?> clazz) { + return excludedPackageExemptClasses.stream().anyMatch(clazz::equals); Review Comment: Do not exempt superclasses Issue Time Tracking ------------------- Worklog Id: (was: 848500) Time Spent: 0.5h (was: 20m) > Make excluded package exemption logic more strict > ------------------------------------------------- > > Key: WW-5288 > URL: https://issues.apache.org/jira/browse/WW-5288 > Project: Struts 2 > Issue Type: Improvement > Components: Core > Reporter: Kusal Kithul-Godage > Priority: Minor > Fix For: 6.2.0 > > Time Spent: 0.5h > Remaining Estimate: 0h > > Following on from the discussion in the comments on WW-5268 - exempting > classes from excluded packages should only be done if unavoidable. > Given this, I realised we should make the exemption logic more strict to > prevent incorrect use and inadvertent exempting of more OGNL expressions than > intended. > * Currently, the exempted classes also match against superclasses. This is > unnecessary and we can match against only the specific class. > * Currently, an exemption against either the target or member class suffices. > This can be made more strict by requiring an exemption for the class which > matches the excluded package specifically, which could be either or both. > * The JavaDoc for the options should be very explicit in what each > configuration option achieves to prevent incorrect uses. -- This message was sent by Atlassian Jira (v8.20.10#820010)