Disable expressionMaxLength by default for Struts 2.5.x.

[JCgH4164838Gh792C124B5]

Disable struts.ognl.expressionMaxLength by default for Struts 2.5.x.

• Commented out struts.ognl.expressionMaxLength line in default.properties and provided in-place comments about its usage.
• Changed OgnlValueStack.handleOgnlException() methods to output error instead of warn for failures to evaluate expressions due to security constraints.
• Updated existing unit tests to compensate for change in default behaviour.
• Added a unit test to confirm default behaviour for struts.ognl.expressionMaxLength is disabled.

[coveralls]

Coverage remained the same at 47.069% when pulling b10e18d on JCgH4164838Gh792C124B5:local_25x_CfgChg2 into fdfeb32 on apache:struts-2-5-x.

[JCgH4164838Gh792C124B5]

Hello Apache Struts Team.

This PR is an attempt to address concerns raised on the Struts Dev List concerning struts.ognl.expressionMaxLength introduction and usage in Struts 2.5.21. It was applied to the current 2.5.x branch (2.5.22-SNAPSHOT) but I'm hoping that the maintainers/committers can apply it to 2.5.21 fairly easily ... assuming there is consensus on the changes.

@lukaszlenart and @yasserzamani , please take a look at this PR and see if you think it is an appropriate response to the concerns raised on the Dev list.

Note: I don't know if either of you think there should also be a minimum length check applied to the property set in OgnlUtil as well and refuse setting lower than it (maybe something in the 128 to 200 range) ?

[JCgH4164838Gh792C124B5]

Hello.

Coveralls complains that code coverage has fallen ... but I don't see it myself. If there's something extra we should add in the unit tests for OgnlValueStack please let me know.

[lukaszlenart]

LGTM 👍

[lukaszlenart]

👏

[yasserzamani]

Thank you so much! It just is missing a unit test to ensure failure when user applies a max length e.g. testFailOnExpressionLongerThan2 - coveralls is sad because of this also (for instance see here).

I think this test should look like this:

    public void testFailOnExpressionLongerThan2() {
        loadConfigurationProviders(new StubConfigurationProvider() {
            @Override
            public void register(ContainerBuilder builder,
                                 LocatableProperties props) throws ConfigurationException {
                props.setProperty(StrutsConstants.STRUTS_OGNL_EXPRESSION_MAX_LENGTH, "2");
            }
        });
//the rest of code should be same as my previous testFailOnTooLongExpressionWithDefaultProperties
}

[JCgH4164838Gh792C124B5]

Hello Apache Struts Team.

The PR has been updated with a new unit test suggested by Y. Zamani (code coverage) that seems to make Coveralls happier. 😄

A new minimum maxlength limit (128, applied as a constant) was also added since comments in the Dev List pointed out that setting a really low value does not make much sense either. The unit tests were updated to reflect that and test it as well.

Please let me know if you think applying a minimum maxlength limit makes sense ? If it does make sense but you think a different value (than 128) should be used, just let me know what value you think it should be and I can update the PR to reflect that.

[yasserzamani]

Thank you @JCgH4164838Gh792C124B5 but IMHO it is a not needed complexity. Furthermore I think there are no reason to restrict user to set a low value because maybe his/her app may not exceed that value really. It isn't Struts business. If so why we don't restrict user to set a high value which is more rational than this?

[JCgH4164838Gh792C124B5]

Hello @yasserzamani. Thanks for the feedback.

I could not envision a Struts scenario where a really low value (e.g. 0) would make sense and very low values are more likely to produce failed expressions in even a typical application. The other concern was that a "bad actor" might somehow find a way to abuse the setting in the future and cause a sort of service denial in an application (by setting a very low maximum expression length).

On the other hand putting a lower limit restricts the flexibility for configuration by a developer. I could simply adjust the lower-bound constant to 0 for now (which will allow any non-negative value to be set) and the mechanism would be there to enforce a lower-limit in the future if circumstances call for it. Please let me know if that would be OK in your view ?

[lukaszlenart]

I have some concerns that adding that minimum maxlength can really confuse users - they will have to know that there is a limit in which they have to operate. And I'm concern that users won't use such functionality at all IMO.

[JCgH4164838Gh792C124B5]

Hello @lukaszlenart and @yasserzamani .

As requested the "minimum maxlength" idea has been removed, returning to the original logic by Yasser. The only change to OgnlUtil in this PR now is that applyExpressionMaxLength() will log an error instead of a warn when setting the value fails (as per justification in the commit comment).

I think the PR now has everything that was requested. Let me know if it is OK now or more changes are needed.

[lukaszlenart]

LGTM 👍