Bug 56265 - Unexpected escaping in the values of dynamic tag attributes containing EL expressions
Unexpected escaping in the values of dynamic tag attributes containing EL exp...
Status: RESOLVED FIXED
Product: Tomcat 7
Classification: Unclassified
Component: Jasper
7.0.52
PC All
: P2 regression (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2014-03-14 19:53 UTC by Konstantin Kolinko
Modified: 2014-03-19 15:10 UTC (History)
0 users



Attachments
test.war (207.80 KB, application/octet-stream)
2014-03-14 19:58 UTC, Konstantin Kolinko
Details
2014-03-15_56265_tc8_v1.patch (728 bytes, patch)
2014-03-15 00:54 UTC, Konstantin Kolinko
Details | Diff
2014-03-18_56265_tc8_v2.patch (10.96 KB, patch)
2014-03-18 11:26 UTC, Konstantin Kolinko
Details | Diff
Proposed patch v3 (9.59 KB, patch)
2014-03-18 15:03 UTC, Mark Thomas
Details | Diff
Proposed patch v4 (12.51 KB, patch)
2014-03-18 20:43 UTC, Mark Thomas
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Konstantin Kolinko 2014-03-14 19:53:26 UTC
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(&#39;Hello &#39;World&#39;!&#39;)"
        data-test="window.alert(&#39;Hello &#39;World&#39;!&#39;)"
        data-test2="window.alert(&#39;Hello World!&#39;)"
tomcat 7.0.52 output:
<form onclick="window.alert(&#39;Hello &#39;World&#39;!&#39;)"
        data-test="window.alert(&amp;#039;Hello &#39;World&#39;!&amp;#039;)"
        data-test2="window.alert(&#39;Hello World!&#39;)"

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]
Comment 1 Konstantin Kolinko 2014-03-14 19:58:44 UTC
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(&#039;Hello 'World'!&#039;)]
[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.
Comment 2 Konstantin Kolinko 2014-03-14 20:01:22 UTC
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(&#039;Hello ${world}!&#039;)", 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(&#039;Hello ${world}!&#039;)" ,
Comment 3 Konstantin Kolinko 2014-03-15 00:24:25 UTC
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(&#039;Hello ${world}!&#039;)"

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.
Comment 4 Konstantin Kolinko 2014-03-15 00:54:51 UTC
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.
Comment 5 Konstantin Kolinko 2014-03-18 11:26:12 UTC
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).
Comment 6 Mark Thomas 2014-03-18 15:03:17 UTC
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.
Comment 7 Konstantin Kolinko 2014-03-18 18:17:33 UTC
(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.
Comment 8 Mark Thomas 2014-03-18 20:43:59 UTC
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.
Comment 9 Mark Thomas 2014-03-19 12:27:10 UTC
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.
Comment 10 Mark Thomas 2014-03-19 12:43:10 UTC
Fixed in 8.0.x for 8.0.4 onwards.
Comment 11 Mark Thomas 2014-03-19 15:10:27 UTC
This has also been fixed in 7.0.x for 7.0.53 onwards and in 6.0.x for 6.0.40 onwards.