Fix backwards-compatibility issue introduced with PR#59/#60

[JCgH4164838Gh792C124B5]

Fix backwards-compatibility issue introduced with PR#59/#60

• Made access to non-public static fields an optional opt-in feature (control flag via context).
• Restored original behaviour when accessing public static fields (the default behaviour).
• Updated unit tests to test proper behaviour with both original (default) and opt-in behaviour.

[lukaszlenart]

I'm not sure if this a good solution, OGNL shouldn't have such control flags and as far I recall this will be the first flag ever.

[JCgH4164838Gh792C124B5]

Hello @lukaszlenart .

If using a control flag for the behaviour is not desirable, what do you think about a "partial return" to the original behaviour when dealing with public static fields ?
Basically attempt c.getField() first (which can only succeed for public static fields), then if that fails attempt @hengyunabc 's methodology for non-public fields (which performs the access checks).

Such a scheme would have a couple of advantages:

• avoids a control flag being introduced.
• retains backwards compatibility (3.1.18 and earlier) for public static fields (which should be "safe" to access values for as they are declared public anyway). Should allow for better "drop-in" compatibility with existing applications.
• retains access control for non-public static fields.

Tradeoffs:

• slightly more efficient access for public static fields (probably the majority of use cases).
• slight more expensive access for non-public static fields (initial public-only check will fail).

I could amend this commit to match the above so that you can take a look.

Would you like this PR to be an example of that strategy instead ?

[JCgH4164838Gh792C124B5]

Hello.
It didn't take too long to remove the control flag and implement the alternate backward-compatible strategy described above. The previous commit was amended to achieve this.
Let me know if it seems better-suited.

[lukaszlenart]

Nice work, looks far better 👍
@hengyunabc & @aleksandr-m what do you think? Looks good to me.

[hengyunabc]

Nice work, looks far better 👍
@hengyunabc & @aleksandr-m what do you think? Looks good to me.

I tested it, it is ok 👍 .

[aleksandr-m]

Why change this?

[JCgH4164838Gh792C124B5]

Hello @aleksandr-m .
It was just a minor optimization (equivalent to one already used in OgnlRuntime) to replace two calls to f.getModifiers():
.. if (!Modifier.isStatic(f.getModifiers())) / result = Modifier.isFinal(f.getModifiers());
with a single call/assignment for the field modifiers fModifiers = f.getModifiers():
.. if (!Modifier.isStatic(fModifiers)) / result = Modifier.isFinal(fModifiers).

[aleksandr-m]

getModifiers() is just a getter it is ok to use it directly. If you want to keep fModifiers then declaration and assignment can be moved to the same line, currently it looks very odd.

[JCgH4164838Gh792C124B5]

Hello @aleksandr-m .
Thanks for the feed-back and suggestion.
The declaration/assignment for fModifiers has been moved to the same line in both modules (and the declaration of one variable moved closer to its usage point). Hopefully it looks a little cleaner now ?

[aleksandr-m]

Looks good. 👍

[lukaszlenart]

Ok, no objections so let's merge and release a new version :)

[lukaszlenart]

What do you think about putting this back in OGNL 3.2.x (checking access to static fields) and add an additional flag to Struts 2.6 to enable/disable such checking in SecureMemberAccess?

[JCgH4164838Gh792C124B5]

Hello @lukaszlenart .

I guess the question for OGNL 3.2.x is whether it is preferable to:

1. Always allow access to public static fields (as with 3.1.x) ... or ...
2. Always perform access checks (for both public and non-public static fields).

Choice 1) would be more consistent (as 3.2.7/earlier also provided that behaviour), but choice 2) would provide more "fine-grained" control over access. I'll put together a PR matching choice 1 for now and it can be accepted/rejected depending on what direction you (and others) think it should go.

An additional flag or flag(s) in Struts 2.6 to enable/disable checks in SecureMemberAccess might be a good feature, but I wonder if for it to be effective it might need to be backed with a separate inclusion/exclusion set just for the fields (since method/field access might not line up 1-to-1) ?

[lukaszlenart]

I opt for 2) - I prefer to control access, we can then consider adding a flag or not.

[JCgH4164838Gh792C124B5]

Hello @lukaszlenart .

OK, sounds good. In that case doesn't that mean that the current access check flows for the OGNL 3.2.x code should remain in place (as with 3.2.8+), if I'm understanding correctly ?

If so, then maybe the only things that could change would be updates to ASTStaticField and OgnlRuntime that restore the type-of-exception thrown if the field is non-static (throw OgnlException instead of NoSuchFieldException). That might be unnecessary if the behaviour for access to public static fields has changed anyway...
Also to provide the original exception type would still require retrieving the field twice (once directly, to get modifiers, then if static, again to enforce access checks) ... unless I'm missing something ?

[lukaszlenart]

Yes, you are right. I would like to keep the checks in place in case of OGNL 3.2.x.

I think throwing NoSuchFieldException exception gives us more control how to control such case (e.g. we can report this as a missing property in Struts), OgnlException is too broad.