diff options
| -rw-r--r-- | src/ng/compile.js | 12 | ||||
| -rw-r--r-- | src/ng/interpolate.js | 24 | ||||
| -rw-r--r-- | test/ng/directive/booleanAttrsSpec.js | 28 | ||||
| -rw-r--r-- | test/ng/interpolateSpec.js | 24 |
4 files changed, 82 insertions, 6 deletions
diff --git a/src/ng/compile.js b/src/ng/compile.js index 91844f45..caa4e8ce 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1157,6 +1157,16 @@ function $CompileProvider($provide) { } + function isTrustedContext(node, attrNormalizedName) { + // maction[xlink:href] can source SVG. It's not limited to <maction>. + if (attrNormalizedName == "xlinkHref" || + (nodeName_(node) != "IMG" && (attrNormalizedName == "src" || + attrNormalizedName == "ngSrc"))) { + return true; + } + } + + function addAttrInterpolateDirective(node, directives, value, name) { var interpolateFn = $interpolate(value, true); @@ -1177,7 +1187,7 @@ function $CompileProvider($provide) { // we need to interpolate again, in case the attribute value has been updated // (e.g. by another directive's compile function) - interpolateFn = $interpolate(attr[name], true); + interpolateFn = $interpolate(attr[name], true, isTrustedContext(node, name)); // if attribute was updated so that there is no interpolation going on we don't want to // register any observers diff --git a/src/ng/interpolate.js b/src/ng/interpolate.js index 51f4630d..8e94fe24 100644 --- a/src/ng/interpolate.js +++ b/src/ng/interpolate.js @@ -1,5 +1,7 @@ 'use strict'; +var $interpolateMinErr = minErr('$interpolate'); + /** * @ngdoc object * @name ng.$interpolateProvider @@ -82,6 +84,12 @@ function $InterpolateProvider() { * @param {boolean=} mustHaveExpression if set to true then the interpolation string must have * embedded expression in order to return an interpolation function. Strings with no * embedded expression will return null for the interpolation function. + * @param {boolean=} isTrustedContext when true, requires that the interpolation string does not + * contain any concatenations - i.e. the interpolation string is a single expression. + * Interpolations for *[src] and *[ng-src] (except IMG, since itwhich sanitizes its value) + * pass true for this parameter. This helps avoid hunting through the template code to + * figure out of some iframe[src], object[src], etc. was interpolated with a concatenation + * that ended up introducing a XSS. * @returns {function(context)} an interpolation function which is used to compute the interpolated * string. The function has these parameters: * @@ -89,7 +97,7 @@ function $InterpolateProvider() { * against. * */ - function $interpolate(text, mustHaveExpression) { + function $interpolate(text, mustHaveExpression, isTrustedContext) { var startIndex, endIndex, index = 0, @@ -121,6 +129,18 @@ function $InterpolateProvider() { length = 1; } + // Concatenating expressions makes it hard to reason about whether some combination of concatenated + // values are unsafe to use and could easily lead to XSS. By requiring that a single + // expression be used for iframe[src], object[src], etc., we ensure that the value that's used + // is assigned or constructed by some JS code somewhere that is more testable or make it + // obvious that you bound the value to some user controlled value. This helps reduce the load + // when auditing for XSS issues. + if (isTrustedContext && parts.length > 1) { + throw $interpolateMinErr('noconcat', + "Error while interpolating: {0}\nYou may not use multiple expressions when " + + "interpolating this expression.", text); + } + if (!mustHaveExpression || hasInterpolation) { concat.length = length; fn = function(context) { @@ -139,7 +159,7 @@ function $InterpolateProvider() { return concat.join(''); } catch(err) { - var newErr = minErr('$interpolate')('interr', "Can't interpolate: {0}\n{1}", text, err.toString()); + var newErr = $interpolateMinErr('interr', "Can't interpolate: {0}\n{1}", text, err.toString()); $exceptionHandler(newErr); } }; diff --git a/test/ng/directive/booleanAttrsSpec.js b/test/ng/directive/booleanAttrsSpec.js index aa48afff..f2ea003c 100644 --- a/test/ng/directive/booleanAttrsSpec.js +++ b/test/ng/directive/booleanAttrsSpec.js @@ -89,19 +89,41 @@ describe('boolean attr directives', function() { describe('ngSrc', function() { it('should interpolate the expression and bind to src', inject(function($compile, $rootScope) { - var element = $compile('<div ng-src="some/{{id}}"></div>')($rootScope); + var element = $compile('<div ng-src="{{id}}"></div>')($rootScope); $rootScope.$digest(); - expect(element.attr('src')).toEqual('some/'); + expect(element.attr('src')).toBeUndefined(); $rootScope.$apply(function() { $rootScope.id = 1; }); - expect(element.attr('src')).toEqual('some/1'); + expect(element.attr('src')).toEqual('1'); dealoc(element); })); + describe('isTrustedContext', function() { + it('should NOT interpolate a multi-part expression for non-img src attribute', inject(function($compile, $rootScope) { + expect(function() { + var element = $compile('<div ng-src="some/{{id}}"></div>')($rootScope); + dealoc(element); + }).toThrow( + "[$interpolate:noconcat] Error while interpolating: some/{{id}}\nYou may not use " + + "multiple expressions when interpolating this expression."); + })); + + it('should interpolate a multi-part expression for regular attributes', inject(function($compile, $rootScope) { + var element = $compile('<div foo="some/{{id}}"></div>')($rootScope); + $rootScope.$digest(); + expect(element.attr('foo')).toBe('some/'); + $rootScope.$apply(function() { + $rootScope.id = 1; + }); + expect(element.attr('foo')).toEqual('some/1'); + })); + + }); + if (msie) { it('should update the element property as well as the attribute', inject( function($compile, $rootScope) { diff --git a/test/ng/interpolateSpec.js b/test/ng/interpolateSpec.js index 454d81aa..7569c0e2 100644 --- a/test/ng/interpolateSpec.js +++ b/test/ng/interpolateSpec.js @@ -149,6 +149,29 @@ describe('$interpolate', function() { }); + describe('isTrustedContext', function() { + it('should NOT interpolate a multi-part expression when isTrustedContext is true', inject(function($interpolate) { + var isTrustedContext = true; + expect(function() { + $interpolate('constant/{{var}}', true, isTrustedContext); + }).toThrow( + "[$interpolate:noconcat] Error while interpolating: constant/{{var}}\nYou may not use " + + "multiple expressions when interpolating this expression."); + expect(function() { + $interpolate('{{foo}}{{bar}}', true, isTrustedContext); + }).toThrow( + "[$interpolate:noconcat] Error while interpolating: {{foo}}{{bar}}\nYou may not use " + + "multiple expressions when interpolating this expression."); + })); + + it('should interpolate a multi-part expression when isTrustedContext is false', inject(function($interpolate) { + expect($interpolate('some/{{id}}')()).toEqual('some/'); + expect($interpolate('some/{{id}}')({id: 1})).toEqual('some/1'); + expect($interpolate('{{foo}}{{bar}}')({foo: 1, bar: 2})).toEqual('12'); + })); + }); + + describe('startSymbol', function() { beforeEach(module(function($interpolateProvider) { @@ -199,4 +222,5 @@ describe('$interpolate', function() { expect($interpolate.endSymbol()).toBe('))'); })); }); + }); |