feat($interpolate): enable escaping interpolated expressions

[caitp]

Previously, Angular would offer no proper mechanism to reveal attempted script
injection attacks when users would add expressions which may be compiled by
angular.

This CL enables web servers to escape escaped expressions by replacing
interpolation start and end markers with escpaed values (which by default are
{{{{ and }}}}, respectively).

This also allows the application to render the content of the expression
without rendering just the result of the expression.

Closes #5601

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#7511)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[caitp]

re-implementation of earlier patch for escaping expressions, which relies on non-greedy regular expressions rather than the awkward unescaping mechanism implemented previously, so it's much less janky.

It also makes the concatenation routine slightly speedier by doing less work inside it, and depending less on whacky math

[caitp]

So, this does look like a crazy amount of code, but a lot of that was just added to avoid breaking existing tests which depend on metadata like separators (which, since it's exposed, would be a breaking change to remove, I guess). However, all of that work happens during compilation. Runtime should be a bit quicker simply due to performing less work

[caitp]

escaping regexp operators, so that you don't get unexpected surprises/syntax errors with markers like [[/]] or (((/)))

[IgorMinar]

I have the same concern about this PR as I voiced at #5628 (comment)

I think it's important to keep in mind that this is a non-mainstream feature, so complicating interpolation significantly to support it is not the right approach to tackle it.

I'd rather simplify the problem to supporting \{\{ and \}\} as the default escape markers and putting a constraint on the custom escape markers to not contain the interpolation markers. Then we should be able to handle this issue with with simple code.

[caitp]

I don't think this really complicates $interpolation, what it does is changes the way expressions are built (and the way they are built is not necessarily more complicated). In effect, it actually simplifies runtime calls to the interpolation function.

It may even be possible to get a performance benefit from using regular expressions instead of messing around with indexOf() to parse expressions.

That said, for use cases where you do want to prevent a form of script insertion, having such a constraint will more likely just crash applications that depend on it, rather than provide an effective way around script insertion, because backends are not likely to do real work to correctly escape user data.

It's up to you, and I admit that there's definitely a limit to how exploitable angular expressions are, but I don't think this is a bad way to do this. It's simple and elegant, and hack-free.

[shahata]

@caitp - this PR has the same problem I mentioned in #5628 (comment). $parse will throw an exception in case $interpolate('{{{{foo{{foo}}')({foo: 'Hello'}) is called since you expect the escaped symbol to always close (I expect it to return {{fooHello). Can you explain why is it important to look for the closing braces in escaped symbols?

This is really what makes the implementation more complicated in my opinion... (as you can see, my suggested implementation in #7496 is literally 6 lines of code)

More over, if the interpolation symbol will never be a substring of the escaped interpolation symbol as @IgorMinar suggests, you won't need to do anything at all. Only unescape the string before pushing it into separators.

[caitp]

@shahata: edited to correct, yes, you will run into problems there, but that's okay, it's not a scenario which is likely to actually happen

[shahata]

@caitp - The test throws, for sure, I tested before posting my comment:

    it('should not really care about end symbols when escaping', inject(function($interpolate) {
      expect($interpolate('{{{{foo{{foo}}')(obj)).toBe('{{fooHello');
    }));

And still, I don't understand why the need to look for the closing escaped bracket instead of just treating each escaped symbol individually.

[caitp]

It's not important, @shahata.

[shahata]

I'm really sorry, I'm having a hard time understanding why. When you inject an escaped string into a template you don't want it to ever throw an exception, you expect the interpreter to eat it no matter what since it should be treated as an ordinary string. Also, I don't understand why it is not likely to happen. We are talking about an attacker trying to send interpolation symbols to create an XSS attack, he is just as likely to try that as he is likely to try anything else.

[caitp]

It's probably not worth writing hacks to get around cases where people let garbage into the template. Since the backend is responsible for correctly escaping things, it's quite simple for them to correct it, by simply ensuring that escaped start strings are always handled correctly (for instance, by appending an escaped end marker to the end of the text node or attribute, if necessary)

[shahata]

I think it is the other way around, the code is much more complicated because it expect all escaped start symbol to have a matching escaped end symbol. And it's not simple at all to add a missing escaped end symbol in the server side in my opinion. The server escaping should be a simple search & replace.

[shahata]

Sorry, I'm falling asleep on the keyboard here :) Hope to continue this tomorrow, we can chat about it in Gitter if you like.

[caitp]

Lose the concept of it being a simple search and replace, because it isn't. Context matters in this grammar, and if you drop it, you break the expression. This causes problems when you need to mix escaped and unescaped content (essentially making it impossible, which is a problem for server-side rendering).

We will be discussing this more, but the idea that a simple search and replace is adequate here is simply nonsense, it breaks the use-case which was requested on the original issue

[shahata]

Would be great if you can give an example. Maybe this is what I'm missing and will solve my confusion.

[caitp]

Suppose you have <h2>{{user.name}}: <%= userProfile %></h2> in your server-side template. The same element can have both interpolated and escaped expressions. So the rendered output might look like this for example: <h2>{{user.name}}: {{{{some{{IllicitAction}}}}}}</h2>

Now, the client can't just replace {{{{ with {{ and ignore it, because the context matters. What you end up with when you do that is <h2>{{user.name}}: {{some{{IllicitAction}}}}</h2>. If you're not aware of the context when unescaping this, you end up with a broken expression which will throw.

So you have to be aware of context, blind string replacement just doesn't work here. Likewise, the server needs to also be responsible and ensure that escaped expressions are properly closed.

Now, for interpolation, parseFns aren't actually wrapped in try/catch blocks (they could be, but currently aren't, and it would likely be a fair bit more expensive if they were), so if parsing fails, this will usually happen during $compile, and you potentially break compilation of the app.

If you don't break compilation of the app, you potentially allow evaluation of escaped expressions if you aren't aware of context, and that's not good either. It's more complicated than a simple search/replace.

[caitp]

It is possible to write a hack to support unclosed escaped expressions, I just don't necessarily think it's worth it. It's a few extra lines of code and it's a hack

[caitp]

There, a hack to fix that. I think it's still debatable whether that hack is really needed, but I stand by what I say, it's not really adequate to just do a string search/replace. It would be great if it were, but it isn't.

[shahata]

Suppose you have <h2>{{user.name}}: <%= userProfile %></h2> in your server-side template. The same element can have both interpolated and escaped expressions. So the rendered output might look like this for example: <h2>{{user.name}}: {{{{some{{IllicitAction}}}}}}</h2>

How will I get such output if the server does a search & replace for all {{ with {{{{? Let's say the that the server reads the userProfile from some DB which has a list of user profiles we got from an online form or something. The userProfile value might be {{some{{IllicitAction}}}} and then the rendered value will be: <h2>{{user.name}}: {{{{some{{{{IllicitAction}}}}}}}}</h2> or the userProfile value might be some{{IllicitAction}} and then the rendered value will be: <h2>{{user.name}}: some{{{{IllicitAction}}}}</h2> but we should never get a rendered value which has double curlies injected instead of <%= userProfile %> if we do a simple search and replace... I'm terribly sorry, but I still don't get it.

[caitp]

I'm not sure how many times I can say this to you before it sinks in, "context matters", search and replace is inadequate for that reason

[shahata]

Okay, I give up then. This issue is far from important enough to me to go to this kind of tones.

[caitp]

Closing in favour of #7517