[
https://issues.apache.org/jira/browse/WW-5350?focusedWorklogId=888829&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-888829
]
ASF GitHub Bot logged work on WW-5350:
--------------------------------------
Author: ASF GitHub Bot
Created on: 05/Nov/23 09:35
Start Date: 05/Nov/23 09:35
Worklog Time Spent: 10m
Work Description: kusalk commented on code in PR #780:
URL: https://github.com/apache/struts/pull/780#discussion_r1382538745
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
- final int memberModifiers = member.getModifiers();
- 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();
Review Comment:
Wanted to be a bit more deliberate about how we handle the arguments in case
OGNL behaviour changes
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
- final int memberModifiers = member.getModifiers();
- 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 (!memberClass.isAssignableFrom(targetClass)) {
- throw new IllegalArgumentException("Target does not match
member!");
+ if (target instanceof Class) { // Target may be of type Class for
static members
+ if (!member.getDeclaringClass().equals(target)) {
+ throw new IllegalArgumentException("Target class does not
match static member!");
Review Comment:
Throw an exception here because if this ceases to be the case, it means the
following logic may need changes
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
- final int memberModifiers = member.getModifiers();
- 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 (!memberClass.isAssignableFrom(targetClass)) {
- throw new IllegalArgumentException("Target does not match
member!");
+ if (target instanceof Class) { // Target may be of type Class for
static members
+ if (!member.getDeclaringClass().equals(target)) {
+ throw new IllegalArgumentException("Target class does not
match static member!");
+ }
+ target = null;
+ } else {
+ if (target != null &&
!member.getDeclaringClass().isAssignableFrom(target.getClass())) {
+ throw new IllegalArgumentException("Target does not match
member!");
+ }
}
- if (disallowProxyMemberAccess && ProxyUtil.isProxyMember(member,
target)) {
Review Comment:
Extract this into its own protected method for consistency
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
- final int memberModifiers = member.getModifiers();
- 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 (!memberClass.isAssignableFrom(targetClass)) {
- throw new IllegalArgumentException("Target does not match
member!");
+ if (target instanceof Class) { // Target may be of type Class for
static members
+ if (!member.getDeclaringClass().equals(target)) {
+ throw new IllegalArgumentException("Target class does not
match static member!");
+ }
+ target = null;
+ } else {
+ if (target != null &&
!member.getDeclaringClass().isAssignableFrom(target.getClass())) {
+ 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);
+ if (!checkProxyMemberAccess(target, member)) {
+ LOG.warn("Access to proxy is blocked! Member class [{}] of target
[{}], member [{}]", member.getDeclaringClass(), target, member);
return false;
}
- if (!checkPublicMemberAccess(memberModifiers)) {
+ if (!checkPublicMemberAccess(member)) {
LOG.warn("Access to non-public [{}] is blocked!", member);
return false;
}
- if (!checkStaticFieldAccess(member, memberModifiers)) {
+ if (!checkStaticFieldAccess(member)) {
LOG.warn("Access to static field [{}] is blocked!", member);
return false;
}
- // it needs to be before calling #checkStaticMethodAccess()
- if (checkEnumAccess(target, member)) {
- LOG.trace("Allowing access to enum: target [{}], member [{}]",
target, member);
- return true;
- }
-
- if (!checkStaticMethodAccess(member, memberModifiers)) {
+ if (!checkStaticMethodAccess(member)) {
LOG.warn("Access to static method [{}] is blocked!", member);
return false;
}
- if (isClassExcluded(memberClass)) {
- LOG.warn("Declaring class of member type [{}] is excluded!",
member);
+ if (!checkDefaultPackageAccess(target, member)) {
return false;
}
- if (targetClass != memberClass && isClassExcluded(targetClass)) {
- LOG.warn("Target class [{}] of target [{}] is excluded!",
targetClass, target);
+ if (!checkExclusionList(target, member)) {
return false;
}
- if (disallowDefaultPackageAccess) {
- if (targetClass.getPackage() == null ||
targetClass.getPackage().getName().isEmpty()) {
- LOG.warn("Class [{}] from the default package is excluded!",
targetClass);
- return false;
- }
- if (memberClass.getPackage() == null ||
memberClass.getPackage().getName().isEmpty()) {
- LOG.warn("Class [{}] from the default package is excluded!",
memberClass);
- return false;
- }
+ if (!isAcceptableProperty(propertyName)) {
+ return false;
}
- if (isPackageExcluded(targetClass)) {
+ return true;
+ }
+
+ /**
+ * @return {@code true} if member access is allowed
+ */
+ protected boolean checkExclusionList(Object target, Member member) {
Review Comment:
Extracted all the exclusion list checks into this method
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
- final int memberModifiers = member.getModifiers();
- 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 (!memberClass.isAssignableFrom(targetClass)) {
- throw new IllegalArgumentException("Target does not match
member!");
+ if (target instanceof Class) { // Target may be of type Class for
static members
+ if (!member.getDeclaringClass().equals(target)) {
+ throw new IllegalArgumentException("Target class does not
match static member!");
+ }
+ target = null;
+ } else {
+ if (target != null &&
!member.getDeclaringClass().isAssignableFrom(target.getClass())) {
+ 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);
+ if (!checkProxyMemberAccess(target, member)) {
+ LOG.warn("Access to proxy is blocked! Member class [{}] of target
[{}], member [{}]", member.getDeclaringClass(), target, member);
return false;
}
- if (!checkPublicMemberAccess(memberModifiers)) {
Review Comment:
Tried to standardise around just passing the object and/or member to these
protected methods
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
- final int memberModifiers = member.getModifiers();
- 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 (!memberClass.isAssignableFrom(targetClass)) {
- throw new IllegalArgumentException("Target does not match
member!");
+ if (target instanceof Class) { // Target may be of type Class for
static members
+ if (!member.getDeclaringClass().equals(target)) {
+ throw new IllegalArgumentException("Target class does not
match static member!");
+ }
+ target = null;
+ } else {
+ if (target != null &&
!member.getDeclaringClass().isAssignableFrom(target.getClass())) {
+ 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);
+ if (!checkProxyMemberAccess(target, member)) {
+ LOG.warn("Access to proxy is blocked! Member class [{}] of target
[{}], member [{}]", member.getDeclaringClass(), target, member);
return false;
}
- if (!checkPublicMemberAccess(memberModifiers)) {
+ if (!checkPublicMemberAccess(member)) {
LOG.warn("Access to non-public [{}] is blocked!", member);
return false;
}
- if (!checkStaticFieldAccess(member, memberModifiers)) {
+ if (!checkStaticFieldAccess(member)) {
LOG.warn("Access to static field [{}] is blocked!", member);
return false;
}
- // it needs to be before calling #checkStaticMethodAccess()
Review Comment:
This is essentially an exemption to `checkStaticMethodAccess` so I moved it
within that method - we shouldn't be relying on method call order here
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
- final int memberModifiers = member.getModifiers();
- 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 (!memberClass.isAssignableFrom(targetClass)) {
- throw new IllegalArgumentException("Target does not match
member!");
+ if (target instanceof Class) { // Target may be of type Class for
static members
+ if (!member.getDeclaringClass().equals(target)) {
+ throw new IllegalArgumentException("Target class does not
match static member!");
+ }
+ target = null;
+ } else {
+ if (target != null &&
!member.getDeclaringClass().isAssignableFrom(target.getClass())) {
+ 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);
+ if (!checkProxyMemberAccess(target, member)) {
+ LOG.warn("Access to proxy is blocked! Member class [{}] of target
[{}], member [{}]", member.getDeclaringClass(), target, member);
return false;
}
- if (!checkPublicMemberAccess(memberModifiers)) {
+ if (!checkPublicMemberAccess(member)) {
LOG.warn("Access to non-public [{}] is blocked!", member);
return false;
}
- if (!checkStaticFieldAccess(member, memberModifiers)) {
Review Comment:
Removed redundant argument
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
- final int memberModifiers = member.getModifiers();
- 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 (!memberClass.isAssignableFrom(targetClass)) {
- throw new IllegalArgumentException("Target does not match
member!");
+ if (target instanceof Class) { // Target may be of type Class for
static members
+ if (!member.getDeclaringClass().equals(target)) {
+ throw new IllegalArgumentException("Target class does not
match static member!");
+ }
+ target = null;
+ } else {
+ if (target != null &&
!member.getDeclaringClass().isAssignableFrom(target.getClass())) {
+ 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);
+ if (!checkProxyMemberAccess(target, member)) {
+ LOG.warn("Access to proxy is blocked! Member class [{}] of target
[{}], member [{}]", member.getDeclaringClass(), target, member);
return false;
}
- if (!checkPublicMemberAccess(memberModifiers)) {
+ if (!checkPublicMemberAccess(member)) {
LOG.warn("Access to non-public [{}] is blocked!", member);
return false;
}
- if (!checkStaticFieldAccess(member, memberModifiers)) {
+ if (!checkStaticFieldAccess(member)) {
LOG.warn("Access to static field [{}] is blocked!", member);
return false;
}
- // it needs to be before calling #checkStaticMethodAccess()
- if (checkEnumAccess(target, member)) {
- LOG.trace("Allowing access to enum: target [{}], member [{}]",
target, member);
- return true;
- }
-
- if (!checkStaticMethodAccess(member, memberModifiers)) {
+ if (!checkStaticMethodAccess(member)) {
LOG.warn("Access to static method [{}] is blocked!", member);
return false;
}
- if (isClassExcluded(memberClass)) {
- LOG.warn("Declaring class of member type [{}] is excluded!",
member);
+ if (!checkDefaultPackageAccess(target, member)) {
return false;
}
- if (targetClass != memberClass && isClassExcluded(targetClass)) {
- LOG.warn("Target class [{}] of target [{}] is excluded!",
targetClass, target);
+ if (!checkExclusionList(target, member)) {
return false;
}
- if (disallowDefaultPackageAccess) {
- if (targetClass.getPackage() == null ||
targetClass.getPackage().getName().isEmpty()) {
- LOG.warn("Class [{}] from the default package is excluded!",
targetClass);
- return false;
- }
- if (memberClass.getPackage() == null ||
memberClass.getPackage().getName().isEmpty()) {
- LOG.warn("Class [{}] from the default package is excluded!",
memberClass);
- return false;
- }
+ if (!isAcceptableProperty(propertyName)) {
+ return false;
}
- if (isPackageExcluded(targetClass)) {
+ return true;
+ }
+
+ /**
+ * @return {@code true} if member access is allowed
+ */
+ protected boolean checkExclusionList(Object target, Member member) {
+ Class<?> memberClass = member.getDeclaringClass();
+ if (isClassExcluded(memberClass)) {
+ LOG.warn("Declaring class of member type [{}] is excluded!",
memberClass);
+ return false;
+ }
+ if (isPackageExcluded(memberClass)) {
LOG.warn("Package [{}] of target class [{}] of target [{}] is
excluded!",
- targetClass.getPackage(),
- targetClass,
+ memberClass.getPackage(),
+ memberClass,
target);
return false;
}
+ if (target == null || target.getClass() == memberClass) {
+ return true;
+ }
+ Class<?> targetClass = target.getClass();
+ if (isClassExcluded(targetClass)) {
+ LOG.warn("Target class [{}] of target [{}] is excluded!",
targetClass, target);
+ return false;
+ }
+ if (isPackageExcluded(targetClass)) {
+ LOG.warn("Package [{}] of member [{}] are excluded!",
targetClass.getPackage(), member);
+ return false;
+ }
+ return true;
+ }
- if (targetClass != memberClass && isPackageExcluded(memberClass)) {
- LOG.warn("Package [{}] of member [{}] are excluded!",
memberClass.getPackage(), member);
+ /**
+ * @return {@code true} if member access is allowed
+ */
+ protected boolean checkDefaultPackageAccess(Object target, Member member) {
+ if (!disallowDefaultPackageAccess) {
+ return true;
+ }
+ Class<?> memberClass = member.getDeclaringClass();
+ if (memberClass.getPackage() == null ||
memberClass.getPackage().getName().isEmpty()) {
+ LOG.warn("Class [{}] from the default package is excluded!",
memberClass);
+ return false;
+ }
+ if (target == null || target.getClass() == memberClass) {
+ return true;
+ }
+ Class<?> targetClass = target.getClass();
+ if (targetClass.getPackage() == null ||
targetClass.getPackage().getName().isEmpty()) {
+ LOG.warn("Class [{}] from the default package is excluded!",
targetClass);
return false;
}
+ return true;
+ }
- return isAcceptableProperty(propertyName);
+ /**
+ * @return {@code true} if member access is allowed
+ */
+ protected boolean checkProxyMemberAccess(Object target, Member member) {
+ return !(disallowProxyMemberAccess && ProxyUtil.isProxyMember(member,
target));
}
/**
* Check access for static method (via modifiers).
- *
+ * <p>
* Note: For non-static members, the result is always true.
*
- * @param member
- * @param memberModifiers
- *
- * @return
+ * @return {@code true} if member access is allowed
*/
- protected boolean checkStaticMethodAccess(Member member, int
memberModifiers) {
- return !Modifier.isStatic(memberModifiers) || member instanceof Field;
+ protected boolean checkStaticMethodAccess(Member member) {
+ if (checkEnumAccess(member)) {
+ LOG.trace("Exempting Enum#values from static method check: class
[{}]", member.getDeclaringClass());
+ return true;
+ }
+ return member instanceof Field || !isStatic(member);
+ }
+
+ private static boolean isStatic(Member member) {
+ return Modifier.isStatic(member.getModifiers());
}
/**
* Check access for static field (via modifiers).
* <p>
* Note: For non-static members, the result is always true.
*
- * @param member
- * @param memberModifiers
- * @return
+ * @return {@code true} if member access is allowed
*/
- protected boolean checkStaticFieldAccess(Member member, int
memberModifiers) {
- if (Modifier.isStatic(memberModifiers) && member instanceof Field) {
- return allowStaticFieldAccess;
- } else {
+ protected boolean checkStaticFieldAccess(Member member) {
+ if (allowStaticFieldAccess) {
Review Comment:
Bypass computation if static field access is permitted
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
- final int memberModifiers = member.getModifiers();
- 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 (!memberClass.isAssignableFrom(targetClass)) {
- throw new IllegalArgumentException("Target does not match
member!");
+ if (target instanceof Class) { // Target may be of type Class for
static members
+ if (!member.getDeclaringClass().equals(target)) {
+ throw new IllegalArgumentException("Target class does not
match static member!");
+ }
+ target = null;
Review Comment:
Set to null as there is no more useful information to extract here, and it
simplifies the checks/logic to follow
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
- final int memberModifiers = member.getModifiers();
- 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 (!memberClass.isAssignableFrom(targetClass)) {
- throw new IllegalArgumentException("Target does not match
member!");
+ if (target instanceof Class) { // Target may be of type Class for
static members
+ if (!member.getDeclaringClass().equals(target)) {
+ throw new IllegalArgumentException("Target class does not
match static member!");
+ }
+ target = null;
+ } else {
+ if (target != null &&
!member.getDeclaringClass().isAssignableFrom(target.getClass())) {
+ 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);
+ if (!checkProxyMemberAccess(target, member)) {
+ LOG.warn("Access to proxy is blocked! Member class [{}] of target
[{}], member [{}]", member.getDeclaringClass(), target, member);
return false;
}
- if (!checkPublicMemberAccess(memberModifiers)) {
+ if (!checkPublicMemberAccess(member)) {
LOG.warn("Access to non-public [{}] is blocked!", member);
return false;
}
- if (!checkStaticFieldAccess(member, memberModifiers)) {
+ if (!checkStaticFieldAccess(member)) {
LOG.warn("Access to static field [{}] is blocked!", member);
return false;
}
- // it needs to be before calling #checkStaticMethodAccess()
- if (checkEnumAccess(target, member)) {
- LOG.trace("Allowing access to enum: target [{}], member [{}]",
target, member);
- return true;
- }
-
- if (!checkStaticMethodAccess(member, memberModifiers)) {
+ if (!checkStaticMethodAccess(member)) {
LOG.warn("Access to static method [{}] is blocked!", member);
return false;
}
- if (isClassExcluded(memberClass)) {
- LOG.warn("Declaring class of member type [{}] is excluded!",
member);
+ if (!checkDefaultPackageAccess(target, member)) {
return false;
}
- if (targetClass != memberClass && isClassExcluded(targetClass)) {
- LOG.warn("Target class [{}] of target [{}] is excluded!",
targetClass, target);
+ if (!checkExclusionList(target, member)) {
return false;
}
- if (disallowDefaultPackageAccess) {
Review Comment:
Extracted this into its own method
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -104,126 +105,164 @@ public void restore(Map context, Object target, Member
member, String propertyNa
public boolean isAccessible(Map context, Object target, Member member,
String propertyName) {
LOG.debug("Checking access for [target: {}, member: {}, property:
{}]", target, member, propertyName);
- final int memberModifiers = member.getModifiers();
- 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 (!memberClass.isAssignableFrom(targetClass)) {
- throw new IllegalArgumentException("Target does not match
member!");
+ if (target instanceof Class) { // Target may be of type Class for
static members
+ if (!member.getDeclaringClass().equals(target)) {
+ throw new IllegalArgumentException("Target class does not
match static member!");
+ }
+ target = null;
+ } else {
+ if (target != null &&
!member.getDeclaringClass().isAssignableFrom(target.getClass())) {
+ 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);
+ if (!checkProxyMemberAccess(target, member)) {
+ LOG.warn("Access to proxy is blocked! Member class [{}] of target
[{}], member [{}]", member.getDeclaringClass(), target, member);
return false;
}
- if (!checkPublicMemberAccess(memberModifiers)) {
+ if (!checkPublicMemberAccess(member)) {
LOG.warn("Access to non-public [{}] is blocked!", member);
return false;
}
- if (!checkStaticFieldAccess(member, memberModifiers)) {
+ if (!checkStaticFieldAccess(member)) {
LOG.warn("Access to static field [{}] is blocked!", member);
return false;
}
- // it needs to be before calling #checkStaticMethodAccess()
- if (checkEnumAccess(target, member)) {
- LOG.trace("Allowing access to enum: target [{}], member [{}]",
target, member);
- return true;
- }
-
- if (!checkStaticMethodAccess(member, memberModifiers)) {
+ if (!checkStaticMethodAccess(member)) {
LOG.warn("Access to static method [{}] is blocked!", member);
return false;
}
- if (isClassExcluded(memberClass)) {
- LOG.warn("Declaring class of member type [{}] is excluded!",
member);
+ if (!checkDefaultPackageAccess(target, member)) {
return false;
}
- if (targetClass != memberClass && isClassExcluded(targetClass)) {
- LOG.warn("Target class [{}] of target [{}] is excluded!",
targetClass, target);
+ if (!checkExclusionList(target, member)) {
return false;
}
- if (disallowDefaultPackageAccess) {
- if (targetClass.getPackage() == null ||
targetClass.getPackage().getName().isEmpty()) {
- LOG.warn("Class [{}] from the default package is excluded!",
targetClass);
- return false;
- }
- if (memberClass.getPackage() == null ||
memberClass.getPackage().getName().isEmpty()) {
- LOG.warn("Class [{}] from the default package is excluded!",
memberClass);
- return false;
- }
+ if (!isAcceptableProperty(propertyName)) {
+ return false;
}
- if (isPackageExcluded(targetClass)) {
+ return true;
+ }
+
+ /**
+ * @return {@code true} if member access is allowed
+ */
+ protected boolean checkExclusionList(Object target, Member member) {
+ Class<?> memberClass = member.getDeclaringClass();
+ if (isClassExcluded(memberClass)) {
+ LOG.warn("Declaring class of member type [{}] is excluded!",
memberClass);
+ return false;
+ }
+ if (isPackageExcluded(memberClass)) {
LOG.warn("Package [{}] of target class [{}] of target [{}] is
excluded!",
- targetClass.getPackage(),
- targetClass,
+ memberClass.getPackage(),
+ memberClass,
target);
return false;
}
+ if (target == null || target.getClass() == memberClass) {
+ return true;
+ }
+ Class<?> targetClass = target.getClass();
+ if (isClassExcluded(targetClass)) {
+ LOG.warn("Target class [{}] of target [{}] is excluded!",
targetClass, target);
+ return false;
+ }
+ if (isPackageExcluded(targetClass)) {
+ LOG.warn("Package [{}] of member [{}] are excluded!",
targetClass.getPackage(), member);
+ return false;
+ }
+ return true;
+ }
- if (targetClass != memberClass && isPackageExcluded(memberClass)) {
- LOG.warn("Package [{}] of member [{}] are excluded!",
memberClass.getPackage(), member);
+ /**
+ * @return {@code true} if member access is allowed
+ */
+ protected boolean checkDefaultPackageAccess(Object target, Member member) {
+ if (!disallowDefaultPackageAccess) {
+ return true;
+ }
+ Class<?> memberClass = member.getDeclaringClass();
+ if (memberClass.getPackage() == null ||
memberClass.getPackage().getName().isEmpty()) {
+ LOG.warn("Class [{}] from the default package is excluded!",
memberClass);
+ return false;
+ }
+ if (target == null || target.getClass() == memberClass) {
+ return true;
+ }
+ Class<?> targetClass = target.getClass();
+ if (targetClass.getPackage() == null ||
targetClass.getPackage().getName().isEmpty()) {
+ LOG.warn("Class [{}] from the default package is excluded!",
targetClass);
return false;
}
+ return true;
+ }
- return isAcceptableProperty(propertyName);
+ /**
+ * @return {@code true} if member access is allowed
+ */
+ protected boolean checkProxyMemberAccess(Object target, Member member) {
+ return !(disallowProxyMemberAccess && ProxyUtil.isProxyMember(member,
target));
}
/**
* Check access for static method (via modifiers).
- *
+ * <p>
* Note: For non-static members, the result is always true.
*
- * @param member
- * @param memberModifiers
- *
- * @return
+ * @return {@code true} if member access is allowed
*/
- protected boolean checkStaticMethodAccess(Member member, int
memberModifiers) {
- return !Modifier.isStatic(memberModifiers) || member instanceof Field;
+ protected boolean checkStaticMethodAccess(Member member) {
+ if (checkEnumAccess(member)) {
+ LOG.trace("Exempting Enum#values from static method check: class
[{}]", member.getDeclaringClass());
+ return true;
+ }
+ return member instanceof Field || !isStatic(member);
+ }
+
+ private static boolean isStatic(Member member) {
+ return Modifier.isStatic(member.getModifiers());
}
/**
* Check access for static field (via modifiers).
* <p>
* Note: For non-static members, the result is always true.
*
- * @param member
- * @param memberModifiers
- * @return
+ * @return {@code true} if member access is allowed
*/
- protected boolean checkStaticFieldAccess(Member member, int
memberModifiers) {
- if (Modifier.isStatic(memberModifiers) && member instanceof Field) {
- return allowStaticFieldAccess;
- } else {
+ protected boolean checkStaticFieldAccess(Member member) {
+ if (allowStaticFieldAccess) {
return true;
}
+ return !(member instanceof Field) || !isStatic(member);
}
/**
* Check access for public members (via modifiers)
- * <p>
- * Returns true if-and-only-if the member is public.
*
- * @param memberModifiers
- * @return
+ * @return {@code true} if member access is allowed
*/
- protected boolean checkPublicMemberAccess(int memberModifiers) {
- return Modifier.isPublic(memberModifiers);
+ protected boolean checkPublicMemberAccess(Member member) {
+ return Modifier.isPublic(member.getModifiers());
}
- protected boolean checkEnumAccess(Object target, Member member) {
- if (target instanceof Class) {
- final Class<?> clazz = (Class<?>) target;
- return Enum.class.isAssignableFrom(clazz) &&
member.getName().equals("values");
- }
- return false;
+ /**
+ * @return {@code true} if member access is allowed
+ */
+ protected boolean checkEnumAccess(Member member) {
+ return member.getDeclaringClass().isEnum()
+ && isStatic(member)
+ && member instanceof Method
+ && member.getName().equals("values")
+ && ((Method) member).getParameterCount() == 0;
Review Comment:
Closed a minor hole where another static `values` method could also be
exempted
Issue Time Tracking
-------------------
Worklog Id: (was: 888829)
Time Spent: 0.5h (was: 20m)
> Implement optional strict class/package allowlist for OGNL
> ----------------------------------------------------------
>
> Key: WW-5350
> URL: https://issues.apache.org/jira/browse/WW-5350
> Project: Struts 2
> Issue Type: Improvement
> Components: Core
> Reporter: Kusal Kithul-Godage
> Priority: Minor
> Fix For: 6.4.0
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> I think this will be more useful than WW-5345
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
