WW-5534 Allow @StrutsParameter recognition and OGNL allowlist for Spring proxies#1237
Conversation
2df54ea to
b075b67
Compare
| return true; | ||
| } | ||
|
|
||
| private void logAllowlistHibernateEntity(Object original, Object resolved) { |
There was a problem hiding this comment.
I've decided to remove this warning as it's a tad aggressive. We've documented best practices in the migration guide and if users wish to still utilise Hibernate entities then that's up to them
24d6242 to
edda3e4
Compare
| protected BeanInfo getBeanInfo(Object action) { | ||
| try { | ||
| return Introspector.getBeanInfo(action.getClass()); | ||
| return ognlUtil.getBeanInfo(ultimateClass(action)); |
There was a problem hiding this 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.
There was a problem hiding this comment.
I added comments for all the other tests in this class as I realised I didn't name them very well initially
| */ | ||
| @Test | ||
| public void publicModelPojo_proxied() { | ||
| var proxyFactory = new ProxyFactory(new ModelAction()); |
There was a problem hiding this 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)
| 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); |
There was a problem hiding this 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.
edda3e4 to
85d69c5
Compare
| /** | ||
| * When the allowlist is enabled and proxy object access is allowed, Spring proxies should be allowlisted based | ||
| * on their underlying target object. Class allowlisting should work as expected. | ||
| */ | ||
| @Test | ||
| public void classInclusion_springProxy_allowProxyObjectAccess() throws Exception { | ||
| SpringService proxyObject = newSpringService(); | ||
| Method proxyMethod = proxyObject.getClass().getMethod("doSomething"); | ||
|
|
||
| sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString()); | ||
| sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString()); | ||
| sma.useAllowlistClasses(SpringServiceImpl.class.getName()); | ||
|
|
||
| assertTrue(sma.checkAllowlist(proxyObject, proxyMethod)); | ||
| } | ||
|
|
There was a problem hiding this comment.
There is a dedicated SecurityMemberAccessProxyTest and maybe it would be good to move proxy related test cases there as well.
lukaszlenart
left a comment
There was a problem hiding this comment.
Left one comment, looks good!
85d69c5 to
433c483
Compare
|
WW-5534
This change fixes
@StrutsParameterdetection for Spring proxied Actions. It additionally adds optional support for resolving Spring proxies when enforcing the OGNL allowlist, alongside the existing support for resolving Hibernate proxies.