kusalk commented on code in PR #1237:
URL: https://github.com/apache/struts/pull/1237#discussion_r1977316681
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -510,9 +519,16 @@ protected StrutsParameter
getParameterAnnotation(AnnotatedElement element) {
return element.getAnnotation(StrutsParameter.class);
}
+ protected Class<?> ultimateClass(Object action) {
+ if (ProxyUtil.isProxy(action)) {
+ return ProxyUtil.ultimateTargetClass(action);
+ }
+ return action.getClass();
+ }
+
protected BeanInfo getBeanInfo(Object action) {
try {
- return Introspector.getBeanInfo(action.getClass());
+ return ognlUtil.getBeanInfo(ultimateClass(action));
Review Comment:
When I implemented this class, I forgot `OgnlUtil` already had a cached
variant of this capability. We are now using that, and resolving any proxies to
ensure annotation detection works as expected.
##########
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:
##########
@@ -212,16 +212,17 @@ protected boolean checkAllowlist(Object target, Member
member) {
return true;
}
+ Class<?> targetClass = target != null ? target.getClass() : null;
+
if (!disallowProxyObjectAccess && ProxyUtil.isProxy(target)) {
- // If `disallowProxyObjectAccess` is not set, allow resolving
Hibernate entities to their underlying
- // classes/members. This allows the allowlist capability to
continue working and offer some level of
+ // If `disallowProxyObjectAccess` is not set, allow resolving
Hibernate entities and Spring proxies to their
+ // underlying classes/members. This allows the allowlist
capability to continue working and still offer
// protection in applications where the developer has accepted the
risk of allowing OGNL access to Hibernate
- // entities. This is preferred to having to disable the allowlist
capability entirely.
- Object newTarget = ProxyUtil.getHibernateProxyTarget(target);
- if (newTarget != target) {
- logAllowlistHibernateEntity(target, newTarget);
- target = newTarget;
- member = ProxyUtil.resolveTargetMember(member, newTarget);
+ // entities and Spring proxies. This is preferred to having to
disable the allowlist capability entirely.
+ Class<?> newTargetClass = ProxyUtil.ultimateTargetClass(target);
Review Comment:
We replaced `#getHibernateProxyTarget` with `#ultimateTargetClass` which can
also resolve Spring proxies. This allows the OGNL allowlist to function in the
presence of Spring proxies in applications where
`struts.disallowProxyObjectAccess` has been reverted to `false`.
##########
core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java:
##########
Review Comment:
I added comments for all the other tests in this class as I realised I
didn't name them very well initially
##########
core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java:
##########
@@ -276,6 +384,27 @@ public void publicModelPojo() {
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Object.class,
Pojo.class));
}
+ /**
+ * Models of ModelDriven actions can be injected without any annotations
on the Action, even when the Action is
+ * proxied.
+ */
+ @Test
+ public void publicModelPojo_proxied() {
+ var proxyFactory = new ProxyFactory(new ModelAction());
Review Comment:
We use the Spring proxy factory to create a CGLIB proxy like would occur
when a transactional proxy is applied to a concrete Action class (like in the
original bug report)
--
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]
