Bug 57136 - EL Parser escaping dollar sign not ${ or ${...}
EL Parser escaping dollar sign not ${ or ${...}
Status: RESOLVED FIXED
Product: Tomcat 7
Classification: Unclassified
Component: Jasper
trunk
PC All
: P2 normal (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
: 58567 58706 (view as bug list)
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2014-10-24 05:16 UTC by Arthur Fiedler
Modified: 2015-12-09 11:30 UTC (History)
2 users (show)



Attachments
test.war - Sample web application demonstrating two variants of EL escaping in attributes of custom tags (213.07 KB, application/octet-stream)
2015-11-19 09:58 UTC, Konstantin Kolinko
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Arthur Fiedler 2014-10-24 05:16:19 UTC
In the EL 3.0 spec it says...
    Alternatively, the escape characters \$ and \# can be used to escape what would
otherwise be treated as an eval-expression. Given the literal-expressions:
        \${exprA}
        \#{exprB}
    The resulting values would again be the strings ${exprA} and #{exprB}.

I believe that means it should not escape lets say "Price: \\$500.00" it should instead output "Price: \$500.00"(it currently would output "Price: $500.00") but should instead escape "Price: \\${500.00}" and the output of that would be "Price: ${500.00}".

Normally this wouldn't be run into, but I happened to run into this issue passing javascript code through the EL processor, that happens to have some escaped dollar signs inside some regex strings.

It maybe a little more acceptable to use \${ and \#{ as the escapes, as that is a better indication that there is about to be an expression
Comment 1 Konstantin Kolinko 2014-10-24 07:12:06 UTC
1. Sample code to reproduce the issue = ?
(Are you using the API directly, a JSP page, or what?)

2. Is it observable with the current 8.0.14? What is your version of Tomcat?
Comment 2 Mark Thomas 2014-10-24 07:20:05 UTC
The spec says that \$ is the escape, not \${.

There are multiple sets of escaping rules at play in a JSP page and things can get complex quickly.

If you have a test case that demonstrates non-compliance with the spec then please feel free to re-open this issue with that test case attached and we'll take a look.
Comment 3 Arthur Fiedler 2014-10-24 07:26:02 UTC
Here is an example, and yes the current version of tomcat 8 trunk (downloaded after 8.0.14)

    public static void main(String[] args) {

        ELProcessor elp = new ELProcessor();
        ELContext elContext = elp.getELManager().getELContext();
        ValueExpression ve = ELManager.getExpressionFactory().createValueExpression(elContext, "Pri\\ce: \\$500.00 \\${true ? 'test':''} ${true ? 'test2':''}", String.class);
        Object ret = ve.getValue(elContext);
        System.out.println(ret);
    }

I use the code similar to this. If you guys deem this in-fact a bug then it would be in generated parser definition
Comment 4 Arthur Fiedler 2014-10-24 07:34:06 UTC
Mark,
The language in the spec is confusing at best, as I read it as saying the escape characters \$ would be used if it was otherwise to be an expression, $500 would not be an expression, but ${...} would. 

I do agree though this is a trivial matter, I just wanted to post it in case. I had already provided a workaround by automating my input translation adding an extra \\ before an escaped $ or # when { does not follow. The specific method I'm running into this is not using JSP, its direct similar to the example provided
Comment 5 Mark Thomas 2014-10-24 08:03:45 UTC
Test cases that avoid JSPs and go directly to the ELProcessor are good since they avoid a lot of the complexities. The only catch is that you still have to use Java escaping for the String (which you have in your example above).

I've re-read that section of the EL spec and I see what you mean. That language actually seems pretty clear although it would be better if it said the escape was "\$}" or "\#{". In this type of case, I usually look at section 1.24 for clarity/confirmation. Unfortunately that doesn't help. It is clear that "\$500" is a literal expression but it offers to guidance as to whether it evaluates to "$500" or "\$500".

I took a look at the previous EL spec and there is some helpful language there.

<quote>
LiteralExpression::= (LiteralComponent)* ([$#])?

i.e. a string of any characters that doesn't include ${ or #{ unless escaped by \${ or \#{
</quote>

The 2.2 spec also include the language about using \$ or \# as the escape.

Given all of this, I think you are right and "\$500" should be evaluated as "\$500" not "$500".

I've converted your example to a test case and I'll tae a look at a fix.
Comment 6 Arthur Fiedler 2014-10-24 08:21:39 UTC
Sounds good! Thanks for taking the extra time to investigate further
Comment 7 Mark Thomas 2014-10-24 16:30:25 UTC
Yuk. That wasn't fun. We really need some API changes in the EL spec to allow us to reduce the number of times we have to parse EL expressions.

This is fixed in 8.0.x and will be included in 8.0.15 onwards.

Assuming there are no regressions reported, I'll back port this to 7.0.x as well.
Comment 8 Arthur Fiedler 2014-10-24 20:18:54 UTC
Mark, I happened to check the svn source to see what changed and I noticed it said you modified AstLiteralExpression.java on Mon Apr 28 23:08:14 2014 UTC, both 8.0.x and 7.0.x, changing \${ escape to \$, I'm not sure what prompted that original change, but just wanted to remind you in case there was something being overlooked here.
Comment 9 Mark Thomas 2014-10-24 20:51:33 UTC
(In reply to Arthur Fiedler from comment #8)
> Mark, I happened to check the svn source to see what changed and I noticed
> it said you modified AstLiteralExpression.java on Mon Apr 28 23:08:14 2014
> UTC, both 8.0.x and 7.0.x, changing \${ escape to \$, I'm not sure what
> prompted that original change, but just wanted to remind you in case there
> was something being overlooked here.

The driver for that change was consistency between the various bits of code that parse EL while I was working on another EL issue.
Comment 10 Mark Thomas 2015-02-09 10:31:00 UTC
No reports of regressions. Fixed in 7.0.x for 7.0.60 onwards.
Comment 11 Chatellier 2015-08-31 10:55:31 UTC
Hi,

I think we are experiencing a regression on this.

We are using regular expression pattern with angularJs in our JSPs.
As doc says:
"Only when the EL is enabled for a page (see Section JSP.3.3.2, “Deactivating
EL Evaluation”), a literal $ can be quoted by \$, and a literal # can be quoted
by \#. This is not required but is useful for quoting EL expressions.

So we are using following source code:
ng-pattern="/^([1-9]+)\$/"

this should be replaced by JSP parser with:
ng-pattern="/^([1-9]+)$/"

But, this your fix, the code stay as this client side:
ng-pattern="/^([1-9]+)\$/"

So, according to spec, this seems to be a regression.
Comment 12 Mark Thomas 2015-09-03 20:50:51 UTC
The problem here is that both the EL and the JSP specs apply and they say different things.

The JSP specs says (assuming EL is enabled) '\$' is an escape for a literal '$'.
The EL spec says '\${' is an escape for a literal '${'.

The JSP spec has been in maintenance mode for 2.2 onwards whereas the EL spec has been in active development - hence why more wieght was given the EL view rather than the JSP one.

Having re-read both specs again, I'm no clearer in my own mind what the right behaviour is. Either the JSP style escaping or the EL style could be viewed as correct. It may even be possible (I need to think it through some more) to have EL use '\${' as an escape for '${' while JSP uses '\$' as an escape for '$'.

I'm reluctant to change this again without clarification from the JSP maintenance lead and the EL spec expert group. I'll leave this open in the NEEDINFO state and raised a bug over in the EL EG.
Comment 13 Mark Thomas 2015-09-03 21:05:36 UTC
https://java.net/jira/browse/UEL-42
Comment 14 Mark Thomas 2015-09-04 15:48:45 UTC
I've been mulling this over some more and I think - regardless of the feedback from the EG - we are going to have to make the escaping for JSP configurable. The EG feedback is just going to determine what the default is.

I'm concerned about the complexity making this configurable will create but the more I think about this the more I think we need to simply so we don't inconvenience one group of users over another.
Comment 15 Arthur Fiedler 2015-09-08 05:57:00 UTC
I've been thinking about this also, and me personally I think the JSP spec should be corrected to use escape \${ as the standard, this does not effect me but I keep thinking about javascript like jquery being output via jsp (not the best place but maybe its a dynamic library) and every time the user would need to double escape a $ in a regex expression, or \$(...) jquery... 

Seems like a lot of work to make it configurable and then users that need to be aware of that configuration may or may not even find it. It's a tough call
Comment 16 Mark Thomas 2015-09-08 19:02:31 UTC
Every time I research this bug some more I seem to be changing my position slightly. I'm currently leaning towards the view that there was a regression in this fix and that '\$' should have remained an escape for '$" within a JSP page. Only when processing standalone EL is '\${' an escape for '${'. Note that this means that within an EL expression in a JSP apge '\$' would be treated as a literal \$.

A careful reading of the JSP spec makes clear that the JSP spec is responsible for parsing the outer '${' and '}' of an EL expression within a JSP page and the EL spec is responsible for parsing what appears between them. (See the production for ELExpressionBody).

I plan to look at a patch for trunk to address this. Once I have that we'll see what the dev list thinks of it and back-porting it.
Comment 17 Mark Thomas 2015-09-09 09:28:45 UTC
I think I have found another, related, problem in attribute values.

The problem I described in comment #16 (where I was thinking about template text) also applies to attribute values. Specifically, consider the following:
<tags:echo echo="16-${'\\$'}" />

Currently, Tomcat applied attribute value escaping to the whole value. This is not correct. The production in the JSP spec for QuotedChar (which is used in attribute values) means that attribute escaping only applies outside the EL. Inside the EL is handled by ELEXpressionBody which delegates to the EL spec.

I am currently extending the EL test cases to take all of this into account before looking at what fixes are required.
Comment 18 Mark Thomas 2015-09-10 13:18:45 UTC
I've committed a fix for this to trunk. I'll give it a few days for folks to review it before back-porting.
Comment 19 Mark Thomas 2015-09-22 08:57:40 UTC
Fix for the regression applied to 8.0.x for 8.0.27 onwards.
Comment 20 Mark Thomas 2015-09-22 11:25:17 UTC
Back-ported the fix to 7.0.x for 7.0.65 onwards.

I've also back-ported a fix to EL parsing to 8.0.x and 7.0.x.
Comment 21 Konstantin Kolinko 2015-10-18 22:57:25 UTC
Reviewing r1704572 and r1704576 (the fix for 7.0.x)

(In reply to Mark Thomas from comment #17)
> I think I have found another, related, problem in attribute values.
> 
> The problem I described in comment #16 (where I was thinking about template
> text) also applies to attribute values. Specifically, consider the following:
> <tags:echo echo="16-${'\\$'}" />
> 
> Currently, Tomcat applied attribute value escaping to the whole value. This
> is not correct. The production in the JSP spec for QuotedChar (which is used
> in attribute values) means that attribute escaping only applies outside the
> EL. Inside the EL is handled by ELEXpressionBody which delegates to the EL
> spec.
> 

I agree with this assessment. I was reading the current as well as older versions of JSP and EL specifications - back to JSP 2.0/Tomcat 5.5.

The chapter "JSP.1.6 Quoting and Escape Conventions" has explicit section on "Quoting in EL Expressions" and it does not say that any additional quoting rules from surrounding context are applied to it. So I agree that when parsing an attribute, "Quoting in Attributes" rules are applied until a ${, then "Quoting in EL Expressions" rules are applied until a } is read.

Historically, EL expressions were introduced in JSTL 1.0 (JSR-052). At that time the expressions were evaluated by tags themselves. So an attribute of JSP tag was escaped as a whole, and unescaping it gives an EL expression.

Effectively if EL expression uses quoting (e.g. ${'\\'}) including it as attribute value requires double quoting <c10:out value="${'\\\\'}" />  This works when using JSTL 1.0 (xmlns:c10="http://java.sun.com/jstl/core") and EL evaluation in JSPs is disabled. So the EL is evaluated by the tag library.

If using JSTL 1.1 (xmlns:c="http://java.sun.com/jsp/jstl/core") and EL is enabled, it becomes <c:out value="${'\\'}" />

I see no mention of quoting changes, but for backwards compatibility there is an explicit option, <%@page isELIgnored="true">.



My concern is that the behaviour where double escaping in attributes was required has been there for many years.
E.g. Eclipse IDE cannot parse /tomcat-7.0.x/test/webapp-3.0/el-method.jsp file and flags errors on the following line,

    <tags:echo echo="00-${testBeanA["bean"].sayHello('JUnit')}" />

I think there would better be a flag in Jasper to enable old behaviour of attribute parsing. In the old behaviour mode the attribute will be parsed and unescaped as a whole.

I think related change in r1704572 is in o.a.jasper.compiler.AttributeParser.

I am REOPENING to discuss introduction of such configuration option. (Though may be better to reorganize into a separate BZ issue).
Comment 22 Violeta Georgieva 2015-11-02 12:55:32 UTC
*** Bug 58567 has been marked as a duplicate of this bug. ***
Comment 23 Mark Thomas 2015-11-02 13:03:27 UTC
*** Bug 58567 has been marked as a duplicate of this bug. ***
Comment 24 Yoni Amir 2015-11-02 13:44:23 UTC
Hello,
I opened bug 58567, and Mark resolved it as a duplicate of this bug.
I'd like to add to this discussion that I am suffering from the regression introduced in 8.0.27 and I didn't even try to escape the $ sign. I have quotes in the expression, something like this:

<c:if test="${(myAccountEmail != null) && (myAccountEmail != \"\")}">

I think that considering this issue as minor and just waiting for an enhancement is a bit weak, and will hurt users.
Thanks,
Yoni
Comment 25 Mark Thomas 2015-11-05 08:46:13 UTC
Also see this thread on the dev list:
http://tomcat.markmail.org/thread/uzo65gf572s636ly
Comment 26 Mark Thomas 2015-11-05 20:02:47 UTC
A configuration option has been added to the JSP Servlet.
Comment 27 Yoni Amir 2015-11-05 21:43:21 UTC
Thank you for providing a configuration option for this. Any chance to elaborate what the option name is, and where (and if) it will be documented?
I apologize if these are trivial questions, I am not sure where to find this information.
Thanks,
Yoni
Comment 28 Violeta Georgieva 2015-11-06 07:53:01 UTC
(In reply to Yoni Amir from comment #27)
> Thank you for providing a configuration option for this. Any chance to
> elaborate what the option name is, and where (and if) it will be documented?
> I apologize if these are trivial questions, I am not sure where to find this
> information.
> Thanks,
> Yoni

Here it is
https://ci.apache.org/projects/tomcat/tomcat9/docs/jasper-howto.html
strictQuoteEscaping
Comment 29 Mark Thomas 2015-11-06 08:15:46 UTC
That isn't the option you want. You want this one:
quoteAttributeEL

Thanks for the reminder, I'd forgotten to document it.
Comment 30 Yoni Amir 2015-11-09 13:33:29 UTC
Hi,
Do you know if this configuration option, quoteAttributeEL, will be available only in tomcat 8.0.29, or also in Tomcat 7.0.66 (or maybe a later 7.0.xx)?
Thanks,
Yoni
Comment 31 Konstantin Kolinko 2015-11-19 09:58:43 UTC
Created attachment 33282 [details]
test.war - Sample web application demonstrating two variants of EL escaping in attributes of custom tags

Sample web application to demonstrate two variants of EL escaping in attributes of a custom tag.

One of them is expected to work, other is expected to fail (with a compilation failure). This is used to test what is the actual behaviour.

It uses EL expression from Comment 24.
Comment 32 Konstantin Kolinko 2015-11-26 12:26:03 UTC
(In reply to Mark Thomas from comment #25)
> Also see this thread on the dev list:
> http://tomcat.markmail.org/thread/uzo65gf572s636ly
> ("On escaping of EL in attributes (BZ 57136)" thread on dev@)

As result of the above,

1. The quoteAttributeEL option has been backported to Tomcat 7 as well
2. Its default value was changed to be "true", restoring old behaviour.

This will be in Tomcat 7.0.66, 8.0.30, 9.0.0.M2 onwards.
Comment 33 Mark Thomas 2015-12-09 10:53:44 UTC
*** Bug 58706 has been marked as a duplicate of this bug. ***
Comment 34 Mark Thomas 2015-12-09 11:30:37 UTC
*** Bug 58706 has been marked as a duplicate of this bug. ***