[
https://issues.apache.org/jira/browse/WW-5288?focusedWorklogId=848501&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-848501
]
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 (use #equals instead of #isAssignableFrom)
Issue Time Tracking
-------------------
Worklog Id: (was: 848501)
Time Spent: 40m (was: 0.5h)
> 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: 40m
> 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)
