diff --git a/src/ng/directive/ngEventDirs.js b/src/ng/directive/ngEventDirs.js index a5c30af98..9f1bbacc2 100644 --- a/src/ng/directive/ngEventDirs.js +++ b/src/ng/directive/ngEventDirs.js @@ -52,7 +52,11 @@ forEach( ngEventDirectives[directiveName] = ['$parse', '$rootScope', function($parse, $rootScope) { return { compile: function($element, attr) { - var fn = $parse(attr[directiveName]); + // We expose the powerful $event object on the scope that provides access to the Window, + // etc. that isn't protected by the fast paths in $parse. We explicitly request better + // checks at the cost of speed since event handler expressions are not executed as + // frequently as regular change detection. + var fn = $parse(attr[directiveName], /* expensiveChecks */ true); return function ngEventHandler(scope, element) { element.on(eventName, function(event) { var callback = function() { diff --git a/src/ng/parse.js b/src/ng/parse.js index 85191539c..ec58b9cc4 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -876,7 +876,8 @@ function setter(obj, path, setValue, fullExp, options) { return setValue; } -var getterFnCache = {}; +var getterFnCacheDefault = {}; +var getterFnCacheExpensive = {}; function isPossiblyDangerousMemberName(name) { return name == 'constructor'; @@ -896,11 +897,12 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp, options) { var eso = function(o) { return ensureSafeObject(o, fullExp); }; - var eso0 = isPossiblyDangerousMemberName(key0) ? eso : identity; - var eso1 = isPossiblyDangerousMemberName(key1) ? eso : identity; - var eso2 = isPossiblyDangerousMemberName(key2) ? eso : identity; - var eso3 = isPossiblyDangerousMemberName(key3) ? eso : identity; - var eso4 = isPossiblyDangerousMemberName(key4) ? eso : identity; + var expensiveChecks = options.expensiveChecks; + var eso0 = (expensiveChecks || isPossiblyDangerousMemberName(key0)) ? eso : identity; + var eso1 = (expensiveChecks || isPossiblyDangerousMemberName(key1)) ? eso : identity; + var eso2 = (expensiveChecks || isPossiblyDangerousMemberName(key2)) ? eso : identity; + var eso3 = (expensiveChecks || isPossiblyDangerousMemberName(key3)) ? eso : identity; + var eso4 = (expensiveChecks || isPossiblyDangerousMemberName(key4)) ? eso : identity; return !options.unwrapPromises ? function cspSafeGetter(scope, locals) { @@ -1006,6 +1008,8 @@ function getterFnWithExtraArgs(fn, fullExpression) { } function getterFn(path, options, fullExp) { + var expensiveChecks = options.expensiveChecks; + var getterFnCache = (expensiveChecks ? getterFnCacheExpensive : getterFnCacheDefault); // Check whether the cache has this getter already. // We can use hasOwnProperty directly on the cache because we ensure, // see below, that the cache never stores a path called 'hasOwnProperty' @@ -1037,7 +1041,10 @@ function getterFn(path, options, fullExp) { } } else { var code = 'var p;\n'; - var needsEnsureSafeObject = false; + if (expensiveChecks) { + code += 's = eso(s, fe);\nl = eso(l, fe);\n'; + } + var needsEnsureSafeObject = expensiveChecks; forEach(pathKeys, function(key, index) { ensureSafeMemberName(key, fullExp); var lookupJs = (index @@ -1045,7 +1052,7 @@ function getterFn(path, options, fullExp) { ? 's' // but if we are first then we check locals first, and if so read it first : '((l&&l.hasOwnProperty("' + key + '"))?l:s)') + '["' + key + '"]'; - var wrapWithEso = isPossiblyDangerousMemberName(key); + var wrapWithEso = expensiveChecks || isPossiblyDangerousMemberName(key); if (wrapWithEso) { lookupJs = 'eso(' + lookupJs + ', fe)'; needsEnsureSafeObject = true; @@ -1139,12 +1146,14 @@ function getterFn(path, options, fullExp) { * service. */ function $ParseProvider() { - var cache = {}; + var cacheDefault = {}; + var cacheExpensive = {}; var $parseOptions = { csp: false, unwrapPromises: false, - logPromiseWarnings: true + logPromiseWarnings: true, + expensiveChecks: false }; @@ -1231,6 +1240,12 @@ function $ParseProvider() { this.$get = ['$filter', '$sniffer', '$log', function($filter, $sniffer, $log) { $parseOptions.csp = $sniffer.csp; + var $parseOptionsExpensive = { + csp: $parseOptions.csp, + unwrapPromises: $parseOptions.unwrapPromises, + logPromiseWarnings: $parseOptions.logPromiseWarnings, + expensiveChecks: true + }; promiseWarning = function promiseWarningFn(fullExp) { if (!$parseOptions.logPromiseWarnings || promiseWarningCache.hasOwnProperty(fullExp)) return; @@ -1239,18 +1254,20 @@ function $ParseProvider() { 'Automatic unwrapping of promises in Angular expressions is deprecated.'); }; - return function(exp) { + return function(exp, expensiveChecks) { var parsedExpression; switch (typeof exp) { case 'string': + var cache = (expensiveChecks ? cacheExpensive : cacheDefault); if (cache.hasOwnProperty(exp)) { return cache[exp]; } - var lexer = new Lexer($parseOptions); - var parser = new Parser(lexer, $filter, $parseOptions); + var parseOptions = expensiveChecks ? $parseOptionsExpensive : $parseOptions; + var lexer = new Lexer(parseOptions); + var parser = new Parser(lexer, $filter, parseOptions); parsedExpression = parser.parse(exp); if (exp !== 'hasOwnProperty') { diff --git a/test/ng/directive/ngEventDirsSpec.js b/test/ng/directive/ngEventDirsSpec.js index 71e89343d..7c06e21ed 100644 --- a/test/ng/directive/ngEventDirsSpec.js +++ b/test/ng/directive/ngEventDirsSpec.js @@ -82,6 +82,25 @@ describe('event directives', function() { }); + describe('security', function() { + it('should allow access to the $event object', inject(function($rootScope, $compile) { + var scope = $rootScope.$new(); + element = $compile('')(scope); + element.triggerHandler('click'); + expect(scope.e.target).toBe(element[0]); + })); + + it('should block access to DOM nodes (e.g. exposed via $event)', inject(function($rootScope, $compile) { + var scope = $rootScope.$new(); + element = $compile('')(scope); + expect(function() { + element.triggerHandler('click'); + }).toThrowMinErr( + '$parse', 'isecdom', 'Referencing DOM nodes in Angular expressions is disallowed! ' + + 'Expression: e = $event.target'); + })); + }); + describe('blur', function() { describe('call the listener asynchronously during $apply', function() { diff --git a/test/ng/parseSpec.js b/test/ng/parseSpec.js index faac169c1..ad6126e9c 100644 --- a/test/ng/parseSpec.js +++ b/test/ng/parseSpec.js @@ -3,9 +3,10 @@ describe('parser', function() { beforeEach(function() { - /* global getterFnCache: true, promiseWarningCache: true */ + /* global getterFnCacheDefault: true, getterFnCacheExpensive: true, promiseWarningCache: true */ // clear caches - getterFnCache = {}; + getterFnCacheDefault = {}; + getterFnCacheExpensive = {}; promiseWarningCache = {}; }); @@ -1067,7 +1068,6 @@ describe('parser', function() { expect(count).toBe(1); }); - it('should call the function once when it is not part of the context', function() { var count = 0; scope.fn = function() { @@ -1078,6 +1078,20 @@ describe('parser', function() { expect(count).toBe(1); }); + describe('expensiveChecks', function() { + it('should block access to window object even when aliased', inject(function($parse, $window) { + scope.foo = {w: $window}; + // This isn't blocked for performance. + expect(scope.$eval($parse('foo.w'))).toBe($window); + // Event handlers use the more expensive path for better protection since they expose + // the $event object on the scope. + expect(function() { + scope.$eval($parse('foo.w', true)); + }).toThrowMinErr( + '$parse', 'isecwindow', 'Referencing the Window in Angular expressions is disallowed! ' + + 'Expression: foo.w'); + })); + }); it('should call the function once when it is part of the context on assignments', function() { var count = 0;