Skip to content

Commit ef90b3b

Browse files
kusalkkusalk
authored
Merge pull request #1220 from apache/WW-5525-proxyutil-npe-67
6.7: WW-5525 Fix NPE in ProxyUtil for SecurityMemberAccess originating static members
2 parents 9b04437 + d35ec15 commit ef90b3b

File tree

5 files changed

+124
-2
lines changed

5 files changed

+124
-2
lines changed

‎core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,9 @@ public void restore(Map context, Object target, Member member, String propertyNa
160160
public boolean isAccessible(Map context, Object target, Member member, String propertyName) {
161161
LOG.debug("Checking access for [target: {}, member: {}, property: {}]", target, member, propertyName);
162162

163+
if (member == null) {
164+
throw new IllegalArgumentException("Member cannot be null!");
165+
}
163166
if (target != null) {
164167
// Special case: Target is a Class object but not Class.class
165168
if (Class.class.equals(target.getClass()) && !Class.class.equals(target)) {
@@ -228,7 +231,7 @@ protected boolean checkAllowlist(Object target, Member member) {
228231
return true;
229232
}
230233

231-
if (!disallowProxyObjectAccess && target != null && ProxyUtil.isProxy(target)) {
234+
if (!disallowProxyObjectAccess && ProxyUtil.isProxy(target)) {
232235
// If `disallowProxyObjectAccess` is not set, allow resolving Hibernate entities to their underlying
233236
// classes/members. This allows the allowlist capability to continue working and offer some level of
234237
// protection in applications where the developer has accepted the risk of allowing OGNL access to Hibernate

‎core/src/main/java/com/opensymphony/xwork2/util/ProxyUtil.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ public static Class<?> ultimateTargetClass(Object candidate) {
8181
* @param object the object to check
8282
*/
8383
public static boolean isProxy(Object object) {
84+
if (object == null) return false;
8485
Class<?> clazz = object.getClass();
8586
Boolean flag = isProxyCache.get(clazz);
8687
if (flag != null) {
@@ -121,7 +122,7 @@ public static boolean isProxyMember(Member member, Object object) {
121122
*/
122123
public static boolean isHibernateProxy(Object object) {
123124
try {
124-
return HibernateProxy.class.isAssignableFrom(object.getClass());
125+
return object != null && HibernateProxy.class.isAssignableFrom(object.getClass());
125126
} catch (NoClassDefFoundError ignored) {
126127
return false;
127128
}

‎core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1233,6 +1233,34 @@ public void testOgnlValueStackFromOgnlValueStackFactoryAllStaticAccess() throws
12331233
assertNull("accessed private field (result not null) ?", accessedValue);
12341234
}
12351235

1236+
public void testFindValueWithConstructorAndProxyChecks() {
1237+
Map<String, String> properties = new HashMap<>();
1238+
properties.put(StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, Boolean.TRUE.toString());
1239+
properties.put(StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, Boolean.TRUE.toString());
1240+
loadButSet(properties);
1241+
refreshContainerFields();
1242+
1243+
String value = "test";
1244+
String ognlResult = (String) vs.findValue(
1245+
"new com.opensymphony.xwork2.ognl.OgnlValueStackTest$ValueHolder('" + value + "').value", String.class);
1246+
1247+
assertEquals(value, ognlResult);
1248+
}
1249+
1250+
@SuppressWarnings({"unused"})
1251+
public static class ValueHolder {
1252+
// See testFindValueWithConstructorAndProxyChecks
1253+
private final String value;
1254+
1255+
public ValueHolder(String value) {
1256+
this.value = value;
1257+
}
1258+
1259+
public String getValue() {
1260+
return value;
1261+
}
1262+
}
1263+
12361264
static class BadJavaBean {
12371265
private int count;
12381266
private int count2;

‎plugins/spring/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import java.util.Map;
3232

3333
import static org.junit.Assert.assertFalse;
34+
import static org.junit.Assert.assertThrows;
3435
import static org.junit.Assert.assertTrue;
3536

3637
public class SecurityMemberAccessProxyTest extends XWorkJUnit4TestCase {
@@ -87,4 +88,91 @@ public void allowAllProxyAccess() {
8788
assertTrue(sma.isAccessible(context, proxy.getAction(), proxyObjectProxyMember, ""));
8889
assertTrue(sma.isAccessible(context, proxy.getAction(), proxyObjectNonProxyMember, ""));
8990
}
91+
92+
@Test
93+
public void nullTargetAndTargetAndMemberNotAllowed() {
94+
sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString());
95+
sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString());
96+
assertTrue(sma.isAccessible(context, null, proxyObjectProxyMember, ""));
97+
}
98+
99+
@Test
100+
public void nullTargetAndTargetAllowedAndMemberNotAllowed() {
101+
sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString());
102+
sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString());
103+
assertTrue(sma.isAccessible(context, null, proxyObjectProxyMember, ""));
104+
}
105+
106+
@Test
107+
public void nullTargetAndTargetAndMemberAllowed() {
108+
sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString());
109+
sma.useDisallowProxyMemberAccess(Boolean.FALSE.toString());
110+
assertTrue(sma.isAccessible(context, null, proxyObjectProxyMember, ""));
111+
}
112+
113+
@Test
114+
public void nullMemberAndTargetAndMemberNotAllowed() {
115+
sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString());
116+
sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString());
117+
Object action = proxy.getAction();
118+
assertThrows("Member cannot be null!", IllegalArgumentException.class,
119+
() -> sma.isAccessible(context, action, null, ""));
120+
}
121+
122+
@Test
123+
public void nullMemberAndTargetAllowedAndMemberNotAllowed() {
124+
sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString());
125+
sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString());
126+
Object action = proxy.getAction();
127+
assertThrows("Member cannot be null!", IllegalArgumentException.class,
128+
() -> sma.isAccessible(context, action, null, ""));
129+
}
130+
131+
@Test
132+
public void nullMemberAndTargetNotAllowedAndMemberAllowed() {
133+
sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString());
134+
sma.useDisallowProxyMemberAccess(Boolean.FALSE.toString());
135+
Object action = proxy.getAction();
136+
assertThrows("Member cannot be null!", IllegalArgumentException.class,
137+
() -> sma.isAccessible(context, action, null, ""));
138+
}
139+
140+
@Test
141+
public void nullTargetAndMemberAndTargetAndMemberNotAllowed() {
142+
sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString());
143+
sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString());
144+
assertThrows("Member cannot be null!", IllegalArgumentException.class,
145+
() -> sma.isAccessible(context, null, null, ""));
146+
}
147+
148+
@Test
149+
public void nullTargetAndMemberAndTargetNotAllowedAndMemberAllowed() {
150+
sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString());
151+
sma.useDisallowProxyMemberAccess(Boolean.FALSE.toString());
152+
assertThrows("Member cannot be null!", IllegalArgumentException.class,
153+
() -> sma.isAccessible(context, null, null, ""));
154+
}
155+
156+
@Test
157+
public void nullTargetAndMemberAndTargetAllowedAndMemberNotAllowed() {
158+
sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString());
159+
sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString());
160+
assertThrows("Member cannot be null!", IllegalArgumentException.class,
161+
() -> sma.isAccessible(context, null, null, ""));
162+
}
163+
164+
@Test
165+
public void nullTargetAndMemberAndTargetAndMemberAllowed() {
166+
sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString());
167+
sma.useDisallowProxyMemberAccess(Boolean.FALSE.toString());
168+
assertThrows("Member cannot be null!", IllegalArgumentException.class,
169+
() -> sma.isAccessible(context, null, null, ""));
170+
}
171+
172+
@Test
173+
public void nullPropertyName() {
174+
sma.useDisallowProxyMemberAccess(Boolean.FALSE.toString());
175+
Object action = proxy.getAction();
176+
assertTrue(sma.isAccessible(context, action, proxyObjectProxyMember, null));
177+
}
90178
}

‎plugins/spring/src/test/java/com/opensymphony/xwork2/spring/SpringProxyUtilTest.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ public void setUp() throws Exception {
4646
}
4747

4848
public void testIsProxy() throws Exception {
49+
assertFalse(ProxyUtil.isProxy(null));
50+
4951
Object simpleAction = appContext.getBean("simple-action");
5052
assertFalse(ProxyUtil.isProxy(simpleAction));
5153

0 commit comments

Comments
 (0)