The following was reported on the users list, "double xmlEscape in dynamic attributes in 7.0.52" http://marc.info/?t=139479709800007&r=1&w=2 [quote] Hi, I have several custom jspx tags with dynamic attributes that worked well up to Tomcat 7.0.47, but they do not work properly on Tomcat 7.0.52. Same problems occur also when using Spring form tags (I suspect that other libraries would have same problem, but I didn't test them). sample (data-test[2] is dynamic attribute, onclick is static): <c:set var="world" value="'World'"></c:set> <sf:form onclick="window.alert('Hello ${world}!')" data-test="window.alert('Hello ${world}!')" data-test2="window.alert('Hello World!')" tomcat 7.0.47 output: <form onclick="window.alert('Hello 'World'!')" data-test="window.alert('Hello 'World'!')" data-test2="window.alert('Hello World!')" tomcat 7.0.52 output: <form onclick="window.alert('Hello 'World'!')" data-test="window.alert(&#039;Hello 'World'!&#039;)" data-test2="window.alert('Hello World!')" If there is EL used in dynamic attribute (data-test), non-EL part of that attribute is escaped twice, EL part is escaped only once. Tomcat 7.0.47 would escape everything just once. Everything works as before if static attribute is used (onclick) or there is no EL in dynamic attribute (data-test2). [/quote]
Created attachment 31388 [details] test.war A simple web application that reproduces this issue. Steps to reproduce on 7.0.52: 1. Deploy 2. Access http://localhost:8080/test/ 3. Actual: --- [data-test]: [window.alert('Hello 'World'!')] [data-test2]: [window.alert('Hello World!')] --- Expected: The following rows, in any order: --- [data-test]: [window.alert('Hello 'World'!')] [data-test2]: [window.alert('Hello World!')] --- Tomcat 7.0.42 does not have this issue, produces the "Expected" output.
In 7.0.52 the text of index_jspx.java contains: [[[ private boolean _jspx_meth_my_005fmytag_005f0(javax.servlet.jsp.PageContext _jspx_page_context) throws java.lang.Throwable { javax.servlet.jsp.PageContext pageContext = _jspx_page_context; javax.servlet.jsp.JspWriter out = _jspx_page_context.getOut(); // my:mytag org.apache.jsp.tag.webmytag_tagx _jspx_th_my_005fmytag_005f0 = (new org.apache.jsp.tag.webmytag_tagx()); _jsp_instancemanager.newInstance(_jspx_th_my_005fmytag_005f0); _jspx_th_my_005fmytag_005f0.setJspContext(_jspx_page_context); // /index.jspx(27,3) null _jspx_th_my_005fmytag_005f0.setDynamicAttribute(null, "data-test2", "window.alert('Hello World!')"); // /index.jspx(27,3) null _jspx_th_my_005fmytag_005f0.setDynamicAttribute(null, "data-test", (java.lang.Object) org.apache.jasper.runtime.PageContextImpl.proprietaryEvaluate("window.alert('Hello ${world}!')", java.lang.Object.class, (javax.servlet.jsp.PageContext)_jspx_page_context, null, false)); _jspx_th_my_005fmytag_005f0.doTag(); _jsp_instancemanager.destroyInstance(_jspx_th_my_005fmytag_005f0); return false; } ]]] So I wonder why is it "window.alert('Hello ${world}!')" ,
Thus far: 1. It is reproducible with the current trunk The following line numbers are from debugging the reproducer with current trunk @1577714. 2. 'setDynamicAttribute(' is written out at Generator$GenerateVisitor.generateSetters(...) at Generator.java L3098 At this point attrs[i].getValue() is already "window.alert('Hello ${world}!')" 3. The escaping happens when creating Node.JspAttribute. That is in Validator$ValidateVisitor.getJspAttribute(...) at Validator.java L1379 The code there originates from r1539173 Apparently the goal of r1539173 was to apply xml escaping to attributes in UninterpretedTag nodes, but it was applied to any tag attributes containing EL expressions.
Created attachment 31390 [details] 2014-03-15_56265_tc8_v1.patch Patch for this issue. Essentially it limits the effect of r1539173 to uninterpreted tags only. There is no JUnit testcase for this issue yet.
Created attachment 31400 [details] 2014-03-18_56265_tc8_v2.patch A better solution, with tests. This covers this bug and xmlescaping in attributes of bug 55198. https://issues.apache.org/bugzilla/show_bug.cgi?id=55198 XmlEscaping in bodies of bug 55198 is not covered, not investigated (nobody raised that besides me in discussion of bug 55198). Mark had compatibility concerns. https://issues.apache.org/bugzilla/show_bug.cgi?id=55198#c6 It is possible to introduce an option to make xmlescaping in attributes optional (a TODO comment in the patch).
Created attachment 31401 [details] Proposed patch v3 I've been looking at v2 of the patch. I think I found a couple of bugs in the testBug56265() - The method wasn't annotated with @Test so the test did not execute - bug56265.jsp assigned a value to a request attribute named 'text' but tried to use an attribute named 'world'. - The test was expected value [1] to be unescaped but I think it should have been expecting it to be escaped I've attached v3 of the patch for comment that - assuming my analysis of v2 is correct - addresses the issues I found and adds a few more tests. The fix part of the patch is now a lot simpler.
(In reply to Mark Thomas from comment #6) > Created attachment 31401 [details] > Proposed patch v3 > > I've been looking at v2 of the patch. > > I think I found a couple of bugs in the testBug56265() > - The method wasn't annotated with @Test so the test did not execute > - bug56265.jsp assigned a value to a request attribute named 'text' but > tried to use an attribute named 'world'. My bad. I have to fix it. > - The test was expected value [1] to be unescaped but I think it should have > been expecting it to be escaped Wrong. The tag bug56265.tagx print the values as <jsp:text>[${e.key}]: [${e.value}]</jsp:text> Those are the attribute values how they were provided by XML parser. They are unescaped ones. > I've attached v3 of the patch for comment that - assuming my analysis of v2 > is correct - addresses the issues I found and adds a few more tests. The fix > part of the patch is now a lot simpler. Wrong. Escaping discussed in bug 55198 shall be applied to template content of the document - to attributes of Node.UninterpretedTag only (and to textual content in the bodies if we extend this). There is no reason to apply xmlescaping in the dozen of other cases when getAttribute(...) method is called. There is also "if (pageInfo.isELIgnored())" branch that shall be handled in the same way as an attribute that does not contain EL expressions. My patch covers that, though I have not wrote a test case.
Created attachment 31402 [details] Proposed patch v4 I have updated my previous patch to follow the Validator changes in v2 but still with the test fixes from v3. I'm not 100% confident this patch is right yet. I want to spend more time reviewing it but I've put it here in case it is useful.
I've spent the morning reviewing this issue and the various related issues. I'm now convinced that Konstantin's v2 patch was correct and that the test changes in the v4 are also correct. I will therefore be applying the v4 patch (with a few extra comments) shortly and then looking at back-porting it.
Fixed in 8.0.x for 8.0.4 onwards.
This has also been fixed in 7.0.x for 7.0.53 onwards and in 6.0.x for 6.0.40 onwards.