feat($interpolate): escape interpolated expressions

[caitp]

By default, the escaped interpolation start and end symbols are {{{{ and }}}}, respectively.

These symbols will be replaced with the proper interpolation start and end symbols during interpolation, without parsing the expression.

Fixes #5601

These tests might be made better, and the implementation might be improved. Comments welcome :3

[caitp]

Would this need to be documented? As an internal function, given the name its use seems pretty clear

[lgalfaso]

I have some doubts that the patch as-is works as expected in the real world when the startSymbol or endSymbol are changed to something else other than '{{' or '}}'. The reason for this is that when compiling a template from a directive, this is renormalized first before the text is interpolated. E.g. If you change the startSymbol to '[[' and the template from a directive is '<div>This is my template {{{{please do not escape this}}}}</div>' then when the denormalization kicks in the template will be changed to '<div>This is my template [[[[please do not escape this}}}}</div>'. From this point on, I think you can guess why this will break

[caitp]

You mean in conjunction with the HTML compiler, yes it starts to get tricky there.

Write some tests and try to break it, for sure. We have a few weeks before this is even considered for merging in.

[caitp]

Okay, this has finally been rebased and the bitrot is fixed! The changes to $interpolate made it a bit harder to make this work, and it might kill all of the performance benefits that you'd otherwise get =(

The escaping algorithm has been made more complicated now out of necessity, so I think it's likely broken in some cases, and probably needs some more robust testing.

@lgalfaso, @shahata, @btford, whoever else, PTAL and see if you can break it when you have a chance, alright?

[lgalfaso]

Sorry, I had to read the entire #5601 before understanding the why of this PR. Truth be told, the more I read about this issue, the more I think this PR is an overkill. If the idea is to be able to handle

<%= user.profile %>

in a safe way, then there is a much simpler solution to this. Create a function (say ngEscape) that takes a string and return the same string with the following modifications

• changes '}' for '\\}'
• changes "'" for "\\'"
• changes "\\" for "\\\\"
• other changes might be needed

Then write the template as

{{'<%= ngEscape(user.profile) %>'}}

Just to be clear, there might be cases where there might be a need for {{{{ and }}}}, but if the only reason is #5601, then I think that adding this to the core is an overkill

[caitp]

I don't think so @lgalfaso --- The issue is that we're trying to avoid script injection, and anything within {{ ... }} can be evaluated as script (while it's questionable how exploitable this is, it's still good to avoid it). But you can't really just replace all \\} or \\{, because their meaning depends on context.

[lgalfaso]

@caitp let me read the original issue once again and then will go to the PR

As a side note the idea was to create a javascript string with {{'<%= ngEscape(user.profile) %>'}} and make use that "\{" is a valid javascript string

[caitp]

The idea is that the web application can upload arbitrary data, which later an be rendered by angular, thus allowing a form of script injection. The server can easily escape these by replacing double curlies with quadruple-curlies (for example), thus mitigating problems and showing the expected text. Otherwise angular throws and renders nothing

[lgalfaso]

@caitp I am still trying to understand some of the cases, is the idea that the server will replace {{ by {{{{ and optionally }} by }}}} ? If this is the case, then is my expectation that

expect($interpolate('{{{{{{{{abcdef}}}}}}')($rootScope)).toBe('{{{{abcdef}}}}');
expect($interpolate('{{{{{{{{abcdef}}}}}}}}')($rootScope)).toBe('{{{{abcdef}}}}');

correct?

[caitp]

I dunno, I am exhausted and will have to look at it again tomorrow

[shahata]

I think you should add unit tests for setting startSymbol and endSymbol with escaped

[shahata]

@caitp I think that the main problem with this PR is that you are expecting the escaped interpolation symbol (aka quadruple-curlies) to end at some point, assuming the attacker was nice enough to close them. I mean, if for example I have something like this in my server side template:

<div><%= unsafeVariable %> {{abc}}</div>

An attacker can put something like {{whatever and mess stuff up even if it is escaped to {{{{whatever. He will probably won't be able to execute code like this (except for exception handlers maybe), but he will succeed in breaking the page layout and generally have the user see something unexpected, which is undesirable.

Actually, I believe that this code can be much simpler if we do not expect quadruple-curlies to end and just take them in as regular string and replace them back to double-curlies

[shahata]

This is a bit confusing. Could be just else if you move the above (!escaped || escapedEnd < startIndex || escapedStart > endIndex + endSymbolLength) into a nested condition.

[IgorMinar]

it seems that most of the complexity in this change is coming from the requirement that the interpolation markers might be a substring of the escaped markers. Is that right?

If that's the case, then wouldn't things be significantly easier if we said that that can't be the case? Once the interpolation and escaped markers are easy to distinguish then we could run interpolation as is today and then run through all separators and see if we can find the escaped markers there and replace them with interpolation markers.

[caitp]

@IgorMinar I rewrote this using (non-greedy) regular expressions instead. The CL is still a bit complicated, but it removes all of the hacks, pretty much. It would be better to criticize the updated one

[IgorMinar]

this got implemented as e3f78c1