You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by ku...@apache.org on 2023/11/05 09:13:50 UTC
(struts) branch WW-5350-allowlist updated (ff81928d8 -> 39787947f)
This is an automated email from the ASF dual-hosted git repository.
kusal pushed a change to branch WW-5350-allowlist
in repository https://gitbox.apache.org/repos/asf/struts.git
discard ff81928d8 WW-5350 Refactor SecurityMemberAccess (changes semantics)
new 82647959b WW-5350 Refactor SecurityMemberAccess
new 39787947f WW-5350 Fix static member test
This update added new revisions after undoing existing revisions.
That is to say, some revisions that were in the old version of the
branch are not in the new version. This situation occurs
when a user --force pushes a change and generates a repository
containing something like this:
* -- * -- B -- O -- O -- O (ff81928d8)
\
N -- N -- N refs/heads/WW-5350-allowlist (39787947f)
You should already have received notification emails for all of the O
revisions, and so the following emails describe only the N revisions
from the common base, B.
Any revisions marked "omit" are not gone; other references still
refer to them. Any revisions marked "discard" are gone forever.
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../xwork2/ognl/SecurityMemberAccess.java | 62 ++++++++++++----------
.../interceptor/ParametersInterceptorTest.java | 2 +-
.../xwork2/ognl/SecurityMemberAccessTest.java | 22 ++++++--
3 files changed, 54 insertions(+), 32 deletions(-)
(struts) 01/02: WW-5350 Refactor SecurityMemberAccess
Posted by ku...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
kusal pushed a commit to branch WW-5350-allowlist
in repository https://gitbox.apache.org/repos/asf/struts.git
commit 82647959b2a7db068bbb219fd07128e6b32fdc5b
Author: Kusal Kithul-Godage <gi...@kusal.io>
AuthorDate: Fri Nov 3 18:10:57 2023 +1100
WW-5350 Refactor SecurityMemberAccess
---
.../xwork2/ognl/SecurityMemberAccess.java | 168 +++++++++++++--------
.../xwork2/ognl/SecurityMemberAccessTest.java | 16 +-
2 files changed, 120 insertions(+), 64 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 232c33e5a..6301ac1a3 100644
--- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java
+++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java
@@ -26,6 +26,7 @@ import org.apache.logging.log4j.Logger;
import java.lang.reflect.AccessibleObject;
import java.lang.reflect.Field;
import java.lang.reflect.Member;
+import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.Arrays;
import java.util.HashSet;
@@ -104,89 +105,130 @@ public class SecurityMemberAccess implements MemberAccess {
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());
}
/**
@@ -194,36 +236,33 @@ public class SecurityMemberAccess implements MemberAccess {
* <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;
}
protected boolean isPackageExcluded(Class<?> clazz) {
@@ -260,8 +299,11 @@ public class SecurityMemberAccess implements MemberAccess {
return excludedClasses.contains(clazz.getName());
}
+ /**
+ * @return {@code true} if member access is allowed
+ */
protected boolean isAcceptableProperty(String name) {
- return name == null || ((!isExcluded(name)) && isAccepted(name));
+ return name == null || !isExcluded(name) && isAccepted(name);
}
protected boolean isAccepted(String paramName) {
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 f38a7dce6..830cdbb2c 100644
--- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java
+++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java
@@ -351,6 +351,16 @@ public class SecurityMemberAccessTest {
assertTrue("Access to enums is blocked!", actual);
}
+ @Test
+ public void testAccessEnum_alternateValues() throws Exception {
+ // when
+ Member alternateValues = MyValues.class.getMethod("values", String.class);
+ boolean actual = sma.isAccessible(context, MyValues.class, alternateValues, null);
+
+ // then
+ assertFalse("Access to unrelated #values method not blocked!", actual);
+ }
+
@Test
public void testAccessStaticMethod() throws Exception {
// given
@@ -875,7 +885,11 @@ interface FooBarInterface extends FooInterface, BarInterface {
}
enum MyValues {
- ONE, TWO, THREE
+ ONE, TWO, THREE;
+
+ public static MyValues[] values(String notUsed) {
+ return new MyValues[] {ONE, TWO, THREE};
+ }
}
class StaticTester {
(struts) 02/02: WW-5350 Fix static member test
Posted by ku...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
kusal pushed a commit to branch WW-5350-allowlist
in repository https://gitbox.apache.org/repos/asf/struts.git
commit 39787947f181550e9f96f562df9494f1932abcc6
Author: Kusal Kithul-Godage <gi...@kusal.io>
AuthorDate: Sun Nov 5 19:49:25 2023 +1100
WW-5350 Fix static member test
---
.../opensymphony/xwork2/interceptor/ParametersInterceptorTest.java | 2 +-
.../java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java b/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java
index 2e7a0be16..f1d1fe9f0 100644
--- a/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java
+++ b/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java
@@ -350,7 +350,7 @@ public class ParametersInterceptorTest extends XWorkTestCase {
//then
assertEquals("This is blah", ((SimpleAction) proxy.getAction()).getBlah());
Field field = ReflectionContextState.class.getField("DENY_METHOD_EXECUTION");
- boolean allowStaticFieldAccess = ((OgnlContext) stack.getContext()).getMemberAccess().isAccessible(stack.getContext(), proxy.getAction(), field, "");
+ boolean allowStaticFieldAccess = ((OgnlContext) stack.getContext()).getMemberAccess().isAccessible(stack.getContext(), ReflectionContextState.class, field, "");
assertFalse(allowStaticFieldAccess);
}
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 830cdbb2c..7ffc47b9f 100644
--- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java
+++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java
@@ -368,7 +368,7 @@ public class SecurityMemberAccessTest {
// when
Member method = StaticTester.class.getMethod("sayHello");
- boolean actual = sma.isAccessible(context, Class.class, method, null);
+ boolean actual = sma.isAccessible(context, StaticTester.class, method, null);
// then
assertFalse("Access to static method is not blocked!", actual);
@@ -595,7 +595,7 @@ public class SecurityMemberAccessTest {
// when
Member method = StaticTester.class.getMethod("sayHello");
- boolean actual = sma.isAccessible(context, Class.class, method, null);
+ boolean actual = sma.isAccessible(context, StaticTester.class, method, null);
// then
assertFalse("Access to static isn't blocked!", actual);
@@ -704,7 +704,7 @@ public class SecurityMemberAccessTest {
member = System.class.getMethod(propertyName, int.class);
// when
- accessible = sma.isAccessible(context, target, member, propertyName);
+ accessible = sma.isAccessible(context, System.class, member, propertyName);
// then
assertFalse(accessible);