Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Commit

Permalink
fix(ng-bind-html): watch string value instead of wrapper
Browse files Browse the repository at this point in the history
Ref: #4045

I have this sinking feeling that support this use case sort of
encourages binding to function that blindly trust some html.  For now,
I'm fixing the issue while I think about the use cases some more.

In the case of a function that performs any non-trivial work before
wrapping the value (e.g. the showdown filter in issue #3980, or the
binding to a simply wrapper function in issue #3932 if it did anything
meaty), this fix makes it "work" - but performance is going to suck -
you should bind to some other thing on scope that watches the actual
source and adjusts itself when that changes (e.g. the showdown filter.)
For the case of the wrapper in #3932, if one isn't performing
sanitization or some such thing - then you the developer has insight
into why that value is safe in that particular context - and it should
be available simply by name and not as a result of a function taking any
arbitrary input to make auditing of security a little saner.

Closes #3932, #3980
  • Loading branch information
chirayuk committed Sep 20, 2013
1 parent 3ed094d commit e2068ad
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 3 deletions.
10 changes: 7 additions & 3 deletions src/ng/directive/ngBind.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,15 @@ var ngBindTemplateDirective = ['$interpolate', function($interpolate) {
* @element ANY
* @param {expression} ngBindHtml {@link guide/expression Expression} to evaluate.
*/
var ngBindHtmlDirective = ['$sce', function($sce) {
var ngBindHtmlDirective = ['$sce', '$parse', function($sce, $parse) {
return function(scope, element, attr) {
element.addClass('ng-binding').data('$binding', attr.ngBindHtml);
scope.$watch(attr.ngBindHtml, function ngBindHtmlWatchAction(value) {
element.html($sce.getTrustedHtml(value) || '');

var parsed = $parse(attr.ngBindHtml);
function getStringValue() { return (parsed(scope) || '').toString(); }

scope.$watch(getStringValue, function ngBindHtmlWatchAction(value) {
element.html($sce.getTrustedHtml(parsed(scope)) || '');
});
};
}];
12 changes: 12 additions & 0 deletions test/ng/directive/ngBindSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,18 @@ describe('ngBind*', function() {
expect(angular.lowercase(element.html())).toEqual('<div onclick="">hello</div>');
}));

it('should watch the string value to avoid infinite recursion', inject(function($rootScope, $compile, $sce) {
// Ref: https://github.com/angular/angular.js/issues/3932
// If the binding is a function that creates a new value on every call via trustAs, we'll
// trigger an infinite digest if we don't take care of it.
element = $compile('<div ng-bind-html="getHtml()"></div>')($rootScope);
$rootScope.getHtml = function() {
return $sce.trustAsHtml('<div onclick="">hello</div>');
};
$rootScope.$digest();
expect(angular.lowercase(element.html())).toEqual('<div onclick="">hello</div>');
}));

describe('when $sanitize is available', function() {
beforeEach(function() { module('ngSanitize'); });

Expand Down

2 comments on commit e2068ad

@rzschech
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do a similar thing to ng-include's $sce.parseAsResourceUrl

@chirayuk
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed for the current version (1.2 milestone.)

The HTML path is different because $sce.getTrustedHtml(value) in Angular core will sanitize the HTML to make it trusted if it wasn't a wrapped/blessed value. Sanitizing is expensive which is why I implemented an optimization in commit 068d861. I'll do this for ng-include to support an alternate $sce service not from Angular core that has an expensive $sce.getTrustedResourceUrl implementation. Since it's not a pressing or upcoming problem, I'd rather think about the general approach a bit more before pushing on this pattern.

Please sign in to comment.