fix double evaluations

[yasserzamani]

address known issues reported at https://securitylab.github.com/research/apache-struts-double-evaluation/

[coveralls]

Coverage increased (+0.2%) to 47.553% when pulling e7834d4 on fix/double_evaluations_2_5 into 8d0382c on struts-2-5-x.

[lukaszlenart]

I have a huge concern if such a big change should be introduced in a minor version like 2.5.27 - I'm ok with having it in 2.6 as this is natural to break backward comaptibility in a major version release. Users can be heavily impacted by these changes and won't be able update their apps.

[yasserzamani]

Thanks @lukaszlenart I see. For me that has spent plenty of time on it, I remember it was obviously safe changes, but please let me double check, I'll let you know.

P.S. actually I like these changes for 2.5 because they improve Struts to behave better in currently publicly known double evaluation reports. It also contains small yet safe other improvements. Please consider it as an ordinary PR that somebody like to improve Struts. And please let me know any possibility of backward-compatibility issue.

[yasserzamani]

@lukaszlenart I double checked and couldn't find any. Maybe to make it simpler just skip tests files review. Then you'll see obvious safe and nice changes. Test coverage is also increased 0.2%!

[lukaszlenart]

Tests are the most important in this change as changing existing tests means you probably introduced breaking changes as users can depend on the buggy implementation.

[yasserzamani]

@lukaszlenart I double checked and host of them are additions or just java language improvements. Existing tests' functional changes are:

-assertEquals(bean.escape("hello!world"), "hello!world");
-assertEquals(bean.escape("hello!@#$%^&*()world"), "hello!@#$%^&*()world");
+assertEquals(bean.escape("hello!world"), "hello_world");
+assertEquals(bean.escape("hello!@#$%^&*()world"), "hello__________world");

which isn't a breaking changes because it's an Struts' internal change.

-tag.setId("cb.bc");
+tag.setId("cb['\".\"'] = bc(){};//");

and consequently

-<td class="tdLabel"><label for="cb.bc" class="label">mylabel:</label></td>
+<td class="tdLabel"><label for="cb['&quot;.&quot;']=bc(){};//" class="label">mylabel:</label></td>
...
-function autoPopulate_cb_bc(targetElement) {
+function autoPopulate_cb__________bc_______(targetElement) {
-<input type="text" name="foo" value="hello" id="cb.bc"/><br/>
-<select onChange="autoPopulate_cb_bc(this);">
+<input type="text" name="foo" value="hello" id="cb['&quot;.&quot;']=bc(){};//"/><br/>
+<select onChange="autoPopulate_cb__________bc_______(this);">

which isn't a breaking changes because it's an Struts' internal change.

-        <input type="radio" name="myMap['name']" id="myMap_'name'_"value=""/>
-        <label for="myMap_'name'_">N/A</label>
-        <input type="radio" name="myMap['name']" id="myMap_'name'_Opt." value="Opt."/>
-        <label for="myMap_'name'_Opt.">Opt.</label>
-        <input type="radio" name="myMap['name']" id="myMap_'name'_Std." checked="checked" value="Std."/>
-        <label for="myMap_'name'_Std.">Std.</label>
+       <input type="radio" name="myMap['name']" id="myMap__name__"value=""/>
+       <label for="myMap__name__">N/A</label>
+       <input type="radio" name="myMap['name']" id="myMap__name__Opt." value="Opt."/>
+       <label for="myMap__name__Opt.">Opt.</label>
+       <input type="radio" name="myMap['name']" id="myMap__name__Std." checked="checked" value="Std."/>
+       <label for="myMap__name__Std.">Std.</label>

which isn't a breaking changes because it's an Struts' internal change.

BTW can I merge at my own risk and responsibility? @apache/struts-committers

[lukaszlenart]

@yasserzamani me and @aleksandr-m expressed our concerns with introducing this change into the 2.5.x branch and you still ignore our votes and pushing your opinion. This not the the Apache way of collaborating.

I have a suggestion, we can release the current state of the 2.5.x branch as Struts 2.5.27 and then introduce your change and release 2.5.28 which will contain only your change. I think this is the safest option we have.

[yasserzamani]

I think its philosophy is respectful, honest, technical-based interaction not concern-based and technically we haven't any reason I think.

But at bottom as per respectful OK 👍 I keep this PR open for the second next release :) thanks a lot for your contribution and comments!

[lukaszlenart]

My technical reason is very simply: this PR will be mixed in with other changes and if users start complain that something doesn't work after upgrade it can be very hard for us to figure out what's the core issue - this change or the other changes. If you want to you can prepare a strict security fix based on a STRUTS_2_5_26 tag and then release it separately. From my experience mixing security fixes with bug fixes isn't a good idea.

[aleksandr-m]

@yasserzamani Why dynamicAttributes type is changed?

[yasserzamani]

@aleksandr-m thanks for asking! Now by merging upstream into this branch by me, a test of TextField has two dynamic parameters set (one added by Lukasz). Then I saw that tests are passing with jdk7 and are failing with jdk8 and newer. Then I realized that different JDKs return different order for HashMap.entrySet(). So I changed to LinkedHashMap which keeps order to avoid a workaround like click here. Furthermore I think it's nice to keep dynamic attributes order same as added by user. And at bottom, LinkedHashMap is also a Map so I think it shouldn't be a breaking change.

[yasserzamani]

@aleksandr-m LinkedHashMap didn't help either. I just reverted it and instead I fixed (improved) the corresponding test to be able to verify against any of multiple possibilities.

[lukaszlenart]

LGTM 👍