fix($sce): make trustAs watchable

[rodyhaddad]

068d861 made changes to ngBindHtml, making it watch attr.ngBindHtml instead of $sce.parseAsHtml(attr.ngBindHtml). (landed in 1.2.0rc2)

Since the attr.ngBindHtml expressions returned the result of calling $sce.trustAs, we would get a [$rootScope:infdig] error (#3932, #3980), because .trustAs always returns a new object.

This PR solves the problem by caching the trustedValues returned by $sce.trustAs

[mary-poppins]

Thanks for the PR!

• Contributor signed CLA now or in the past
  • If you just signed, leave a comment here with your real name
• PR's commit messages follow the commit message format

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!

[rodyhaddad]

CLA signed as Rodric Haddad

[btford]

Can you squash your commits?

[rodyhaddad]

@btford like that?

[btford]

Yes, thanks.

@chirayuk, can you please look at this? Does it seem reasonable?

[chirayuk]

I'll take a look at it tonight. Thanks @btford

[chirayuk]

tl;dr:  I understand the problem and will think about it tomorrow and commit a fix.

I just took a quick look at this PR now.

trustAs used incorrectly weakens security so I want people to think use it with care. It's my opinion that using it directly in template code is a mistake. It makes auditing for security issues harder.

The way I was hoping it would be used to is that one would have a watcher on the raw value and when that changed, you'd generate the trustAs wrapped value after ensuring that it was actually safe.  The idea is that by explicitly setting up the watcher, the watched expression is also explicitly available (i.e. not a passed in parameter.)  It would then be obvious just by looking at the code if the field/expression being watched was user controlled, a trusted field returned from the server, etc.  If one uses it as a filter / defines a function that takes an arbitrary input and wraps it as trusted, it makes it harder to audit for security issues – one now has to look up all those callers in addition to just grepping for trustAs in the codebase.  That said, the current version isn't great and it's definitely being used in the context of a filter (issue #3980) or wrapper (issue #3932).  So I want to think a bit about this and figure out a solution (tomorrow, not tonight).

ps:  As to the solution in this PR – it's one way to go. (However, sadly, the cache grows unbounded.  In addition, the check to ensure that the input is a string should be moved up since the new code bypasses that check.)

[chirayuk]

I'm not ready to add an unbounded no-eviction cache to trustAs just yet.  For now, I've committed a fix that should solve the problem of using this with ng-bind-html. Do you have cases where you need trustAs to be $watchable?

[chirayuk]

FYI – the fix I referred to is in e2068ad

[rodyhaddad]

Do you have cases where you need trustAs to be $watchable?

Nope.
I was trying to make ngBindHtml work in 1.2.0rc2 without changing its code (since you modified it for performance reasons), which produced a solution that wasn't ideal. Yours is a lot better, and solves the two issues without much to worry about.
Awesome. Closing this PR 👍