kusalk commented on code in PR #1201:
URL: https://github.com/apache/struts/pull/1201#discussion_r1957677603
##########
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:
##########
@@ -157,44 +157,44 @@ public boolean isAccessible(Map context, Object target,
Member member, String pr
}
}
- if (!checkProxyObjectAccess(target)) {
+ if (target != null && !checkProxyObjectAccess(target)) {
Review Comment:
This is redundant after https://github.com/apache/struts/pull/1214 - if we
wanted to keep it as an additional check it might be more consistent to move it
inside `#checkProxyObjectAccess`
##########
core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java:
##########
@@ -649,6 +649,7 @@ public void testBlockedStaticFieldWhenFlagIsFalse() throws
Exception {
@Test
public void testBlockedStaticFieldWhenClassIsExcluded() throws Exception {
// given
+ assignNewSma(false);
Review Comment:
By adding this, the test doesn't test the behaviour it's meant to check -
the point of this test is to ensure the exclusion list is enforced for static
fields even when static fields aren't blocked as a whole
##########
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:
##########
@@ -185,15 +185,15 @@ public boolean isAccessible(Map context, Object target,
Member member, String pr
return false;
}
- if (!checkDefaultPackageAccess(target, member)) {
+ if (target != null && !checkDefaultPackageAccess(target, member)) {
Review Comment:
Bug - prevents blocking of default package for static members and
constructors
##########
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:
##########
@@ -185,15 +185,15 @@ public boolean isAccessible(Map context, Object target,
Member member, String pr
return false;
}
- if (!checkDefaultPackageAccess(target, member)) {
+ if (target != null && !checkDefaultPackageAccess(target, member)) {
return false;
}
- if (!checkExclusionList(target, member)) {
+ if (target != null && !checkExclusionList(target, member)) {
return false;
}
- if (!checkAllowlist(target, member)) {
+ if (target != null && !checkAllowlist(target, member)) {
Review Comment:
Bug - prevents enforcement of allowlist for static members and constructors
##########
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:
##########
@@ -160,12 +160,12 @@ public boolean isAccessible(Map context, Object target,
Member member, String pr
}
}
- if (!checkProxyObjectAccess(target)) {
+ if (target != null && !checkProxyObjectAccess(target)) {
LOG.warn("Access to proxy is blocked! Target [{}], proxy class
[{}]", target, target.getClass().getName());
return false;
}
- if (!checkProxyMemberAccess(target, member)) {
+ if (target != null && !checkProxyMemberAccess(target, member)) {
Review Comment:
This is redundant after https://github.com/apache/struts/pull/1214 but could
also present a bug if we encountered a proxied static member or constructor
##########
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:
##########
@@ -185,15 +185,15 @@ public boolean isAccessible(Map context, Object target,
Member member, String pr
return false;
}
- if (!checkDefaultPackageAccess(target, member)) {
+ if (target != null && !checkDefaultPackageAccess(target, member)) {
return false;
}
- if (!checkExclusionList(target, member)) {
+ if (target != null && !checkExclusionList(target, member)) {
Review Comment:
Bug - prevents enforcement of exclusion list for static members and
constructors
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
