add expression max length functionality for security enhancement

[yasserzamani]

No description provided.

[JCgH4164838Gh792C124B5]

Hi Yasser.

This PR seems like a reasonable approach to me, and it provides an opt-in/opt-out mechanism (:smile:).

Beyond the two suggestions provided, it LGTM. 👍

[JCgH4164838Gh792C124B5]

Maybe helpful to identify when the feature was added:

 *
 * @since 3.1.26
 *

[JCgH4164838Gh792C124B5]

You might want to declare it volatile, to ensure visibility of any change to all threads:
private static volatile Integer expressionMaxLength;

[yasserzamani]

Glad to hear you like it! It seems you cherish opt-in/opt-out mechanism :) -- it makes me remember airplanes consoles :) I added since tag but not volatile as I think we don't expect enormous updates of this variable (it's likely to being called once on application startup via a singleton object) P.S. This PR is a follow up for https://mail-archives.apache.org/mod_mbox/struts-dev/201909.mbox/%3C000001d56b9b%24522b4cd0%24f681e670%24%40apache.org%3E

[lukaszlenart]

I wonder if we should add this into OGNL 3.1.x, 3.2.x contains large changes around security.

[yasserzamani]

Yes please because I would import this functionality into Struts 2.5 which uses OGNL 3.1 series. We need it in Struts to fix our security list recent last reports. We can think about this functionality and OGNL 3.2 relationship in future though.

[lukaszlenart]

The case is that those changes can impact users, also introducing a lot of those improvements in 2.5 can confuse users - that's why I wanted push them into 2.6

[yasserzamani]

Well, I think at ognl side we can merge this because it doesn't have any impact but enables its users who are interested in this functionality to drop its jar in classpath and call that method in their apps -- e.g. struts users also can do these. At Struts 2.5 side we can include this as well but disabled by default or with a large number e.g. 512 as default. Do these work? I think so but if really there aren't any way, then I can close this PR and remove this functionality completely from my PR at Struts 2.5 side :)

[lukaszlenart]

It's not the case with changes itself but with versioning schema which doesn't reflect those changes - it's like having major improvements and still using inappropriate version number eg.: instead of Struts 2.5 use Struts 2.0.250 ;)

I'm ok with the current change but in the future I would opt to target the latest version for new improvements instead :)

[JCgH4164838Gh792C124B5]

Hello @yasserzamani and @lukaszlenart .

It looks like this change was already merged before I was not to respond back concerning the "call once on application startup" considerations.

Even if the expressionMaxLength is only planned to be set once (during application startup), since the setter is public it will be valid to call it at any time (e.g. from a framework or an application calling a framework). If that happens then different threads could behave differently if it is just a "plain Integer" due to seeing different state for expressionMaxLength.

Here are some ways we could avoid that condition:

1. Remove the applyExpressionMaxLength() method and make expressionMaxLength final and assign it's value using a static initializer block (less flexible but no synchronized or volatile required).
2. Make expressionMaxLength volatile.
3. Make expressionMaxLength an AtomicInteger.
4. Make applyExpressionMaxLength() synchronized and use a synchronized getter for expressionMaxLength.

There's trade-offs for any of those, but 1) is similar to strategies used in OgnlRuntime to perform a "once-only" initialization of state.

What do you think ?

[yasserzamani]

I think it's not crucial -- it's not designated for such usage so I personally don't worry if users get exception when they put it under race condition! (despite it's public static modifier, rationally application main thread would decide on it and set or disable it during app startup (I didn't make more code to check these though)). So I think we can have readability (shorter code) and performance (avoid JVM volatile checking burden) instance of validating user not rational and expected usage. Albeit we can fix anything in future if users liked this feature :)