[ 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)