WW-5350 Refactor SecurityMemberAccess #780
Conversation
| 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(); |
There was a problem hiding this comment.
Wanted to be a bit more deliberate about how we handle the arguments in case OGNL behaviour changes
| } | ||
|
|
||
| if (disallowProxyMemberAccess && ProxyUtil.isProxyMember(member, target)) { |
There was a problem hiding this comment.
Extract this into its own protected method for consistency
| LOG.warn("Access to non-public [{}] is blocked!", member); | ||
| return false; | ||
| } | ||
|
|
||
| if (!checkStaticFieldAccess(member, memberModifiers)) { |
There was a problem hiding this comment.
Removed redundant argument
| return false; | ||
| } | ||
|
|
||
| if (!checkPublicMemberAccess(memberModifiers)) { |
There was a problem hiding this comment.
Tried to standardise around just passing the object and/or member to these protected methods
| /** | ||
| * @return {@code true} if member access is allowed | ||
| */ | ||
| protected boolean checkExclusionList(Object target, Member member) { |
There was a problem hiding this comment.
Extracted all the exclusion list checks into this method
| return false; | ||
| } | ||
|
|
||
| if (disallowDefaultPackageAccess) { |
There was a problem hiding this comment.
Extracted this into its own method
| LOG.warn("Access to static field [{}] is blocked!", member); | ||
| return false; | ||
| } | ||
|
|
||
| // it needs to be before calling #checkStaticMethodAccess() |
There was a problem hiding this comment.
This is essentially an exemption to checkStaticMethodAccess so I moved it within that method - we shouldn't be relying on method call order here
| && isStatic(member) | ||
| && member instanceof Method | ||
| && member.getName().equals("values") | ||
| && ((Method) member).getParameterCount() == 0; |
There was a problem hiding this comment.
Closed a minor hole where another static values method could also be exempted
| return allowStaticFieldAccess; | ||
| } else { | ||
| protected boolean checkStaticFieldAccess(Member member) { | ||
| if (allowStaticFieldAccess) { |
There was a problem hiding this comment.
Bypass computation if static field access is permitted
kusalk
force-pushed
the
WW-5350-allowlist
branch
from
de5d341
to
c85d7eb
Compare
| throw new IllegalArgumentException("Target does not match member!"); | ||
| if (target != null) { | ||
| // Special case: Target is a Class object but not Class.class | ||
| if (Class.class.equals(target.getClass()) && !Class.class.equals(target)) { |
There was a problem hiding this comment.
Validate and throw exceptions if arguments are not what we expect as the following logic depends on these assumptions to be accurate
|
@lukaszlenart I'm going to make it extensible as part of WW-5343 so that applications can add even stronger checks if they so desire - so will leave it not |
|
SonarCloud Quality Gate failed.
|
WW-5350
See comments.
I didn't deprecate methods for which I've changed the signature as from what I can tell
SecurityMemberAccessis not considered public API (no reasonable situation in which a Struts applications would need to override or instantiate the class themselves, nor is it a defined extensible bean) (WW-5343 aims to make it a configurable bean and thus public API).