Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WW-4973] Upgrades to OGNL 3.2.8 #258

Merged
merged 8 commits into from
Oct 31, 2018
Merged

[WW-4973] Upgrades to OGNL 3.2.8 #258

merged 8 commits into from
Oct 31, 2018

Conversation

lukaszlenart
Copy link
Member

Refs WW-4973

@lukaszlenart
Copy link
Member Author

@apache/struts-committers I opened this PR to review a new approach in OGNL to allow control access to static fields. Till now OGNL didn't support this so you could access any static field. As from OGNL 3.2.8 a check was added if access to the static field is allowed, but there is one thing: in such case target is null. It is possible to pass a class as the target but then it will be narrowed to Class class (target.getClass() will return Class).

I assumed it is better to use null in this case. Opinions?

@coveralls
Copy link

coveralls commented Oct 27, 2018

Coverage Status

Coverage increased (+0.02%) to 46.385% when pulling e84373c on lukaszlenart:WW-4973 into 6f56e14 on apache:master.

Class targetClass = target.getClass();

// target can be null in case of accessing static fields, since OGNL 3.2.8
Class targetClass = target != null ? target.getClass() : member.getDeclaringClass();
Class memberClass = member.getDeclaringClass();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a small improvement you could switch L:86 and L:87, declare Class memberClass as final and reuse memberClass within the ternary expression.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, done :)

@yasserzamani
Copy link
Member

if target could be null, I see it also is passed to ProxyUtil.isProxyMember which should be fixed also to accept null without exception.

I assumed it is better to use null in this case. Opinions?

Did you mean in OGNL? because I see Struts doesn't call it directly anywhere so no control to pass null or class as target.

@lukaszlenart
Copy link
Member Author

Yes, it is in OGNL itself, when a call to a static field is performed from an expression
https://github.com/jkuhnert/ognl/blob/master/src/main/java/ognl/OgnlRuntime.java#L2066

@lukaszlenart
Copy link
Member Author

@yasserzamani I have fixed ProxyUtil but to be honest I have no idea how to write a test for that :(

@yasserzamani
Copy link
Member

yasserzamani commented Oct 30, 2018 via email

@lukaszlenart
Copy link
Member Author

@yasserzamani I have added two more tests but one isn't too realistic ;-)

@yasserzamani
Copy link
Member

Thanks! I rethought and requested a pull. Access to proxy should be blocked either if static access is allowed or not. This is fixed there. As our supported proxy detection classes don't have any static field, no need to test :)

@yasserzamani yasserzamani merged commit b82db24 into apache:master Oct 31, 2018
@lukaszlenart lukaszlenart deleted the WW-4973 branch October 31, 2018 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants