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

Commit 4d0614f

Browse files
committed
fix($parse, events): prevent accidental misuse of properties on $event
Closes #9969
1 parent 756640f commit 4d0614f

File tree

4 files changed

+71
-17
lines changed

4 files changed

+71
-17
lines changed

src/ng/directive/ngEventDirs.js

+5-1
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,11 @@ forEach(
5252
ngEventDirectives[directiveName] = ['$parse', '$rootScope', function($parse, $rootScope) {
5353
return {
5454
compile: function($element, attr) {
55-
var fn = $parse(attr[directiveName]);
55+
// We expose the powerful $event object on the scope that provides access to the Window,
56+
// etc. that isn't protected by the fast paths in $parse. We explicitly request better
57+
// checks at the cost of speed since event handler expressions are not executed as
58+
// frequently as regular change detection.
59+
var fn = $parse(attr[directiveName], /* expensiveChecks */ true);
5660
return function ngEventHandler(scope, element) {
5761
element.on(eventName, function(event) {
5862
var callback = function() {

src/ng/parse.js

+30-13
Original file line numberDiff line numberDiff line change
@@ -876,7 +876,8 @@ function setter(obj, path, setValue, fullExp, options) {
876876
return setValue;
877877
}
878878

879-
var getterFnCache = {};
879+
var getterFnCacheDefault = {};
880+
var getterFnCacheExpensive = {};
880881

881882
function isPossiblyDangerousMemberName(name) {
882883
return name == 'constructor';
@@ -896,11 +897,12 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp, options) {
896897
var eso = function(o) {
897898
return ensureSafeObject(o, fullExp);
898899
};
899-
var eso0 = isPossiblyDangerousMemberName(key0) ? eso : identity;
900-
var eso1 = isPossiblyDangerousMemberName(key1) ? eso : identity;
901-
var eso2 = isPossiblyDangerousMemberName(key2) ? eso : identity;
902-
var eso3 = isPossiblyDangerousMemberName(key3) ? eso : identity;
903-
var eso4 = isPossiblyDangerousMemberName(key4) ? eso : identity;
900+
var expensiveChecks = options.expensiveChecks;
901+
var eso0 = (expensiveChecks || isPossiblyDangerousMemberName(key0)) ? eso : identity;
902+
var eso1 = (expensiveChecks || isPossiblyDangerousMemberName(key1)) ? eso : identity;
903+
var eso2 = (expensiveChecks || isPossiblyDangerousMemberName(key2)) ? eso : identity;
904+
var eso3 = (expensiveChecks || isPossiblyDangerousMemberName(key3)) ? eso : identity;
905+
var eso4 = (expensiveChecks || isPossiblyDangerousMemberName(key4)) ? eso : identity;
904906

905907
return !options.unwrapPromises
906908
? function cspSafeGetter(scope, locals) {
@@ -1006,6 +1008,8 @@ function getterFnWithExtraArgs(fn, fullExpression) {
10061008
}
10071009

10081010
function getterFn(path, options, fullExp) {
1011+
var expensiveChecks = options.expensiveChecks;
1012+
var getterFnCache = (expensiveChecks ? getterFnCacheExpensive : getterFnCacheDefault);
10091013
// Check whether the cache has this getter already.
10101014
// We can use hasOwnProperty directly on the cache because we ensure,
10111015
// see below, that the cache never stores a path called 'hasOwnProperty'
@@ -1037,15 +1041,18 @@ function getterFn(path, options, fullExp) {
10371041
}
10381042
} else {
10391043
var code = 'var p;\n';
1040-
var needsEnsureSafeObject = false;
1044+
if (expensiveChecks) {
1045+
code += 's = eso(s, fe);\nl = eso(l, fe);\n';
1046+
}
1047+
var needsEnsureSafeObject = expensiveChecks;
10411048
forEach(pathKeys, function(key, index) {
10421049
ensureSafeMemberName(key, fullExp);
10431050
var lookupJs = (index
10441051
// we simply dereference 's' on any .dot notation
10451052
? 's'
10461053
// but if we are first then we check locals first, and if so read it first
10471054
: '((l&&l.hasOwnProperty("' + key + '"))?l:s)') + '["' + key + '"]';
1048-
var wrapWithEso = isPossiblyDangerousMemberName(key);
1055+
var wrapWithEso = expensiveChecks || isPossiblyDangerousMemberName(key);
10491056
if (wrapWithEso) {
10501057
lookupJs = 'eso(' + lookupJs + ', fe)';
10511058
needsEnsureSafeObject = true;
@@ -1139,12 +1146,14 @@ function getterFn(path, options, fullExp) {
11391146
* service.
11401147
*/
11411148
function $ParseProvider() {
1142-
var cache = {};
1149+
var cacheDefault = {};
1150+
var cacheExpensive = {};
11431151

11441152
var $parseOptions = {
11451153
csp: false,
11461154
unwrapPromises: false,
1147-
logPromiseWarnings: true
1155+
logPromiseWarnings: true,
1156+
expensiveChecks: false
11481157
};
11491158

11501159

@@ -1231,6 +1240,12 @@ function $ParseProvider() {
12311240

12321241
this.$get = ['$filter', '$sniffer', '$log', function($filter, $sniffer, $log) {
12331242
$parseOptions.csp = $sniffer.csp;
1243+
var $parseOptionsExpensive = {
1244+
csp: $parseOptions.csp,
1245+
unwrapPromises: $parseOptions.unwrapPromises,
1246+
logPromiseWarnings: $parseOptions.logPromiseWarnings,
1247+
expensiveChecks: true
1248+
};
12341249

12351250
promiseWarning = function promiseWarningFn(fullExp) {
12361251
if (!$parseOptions.logPromiseWarnings || promiseWarningCache.hasOwnProperty(fullExp)) return;
@@ -1239,18 +1254,20 @@ function $ParseProvider() {
12391254
'Automatic unwrapping of promises in Angular expressions is deprecated.');
12401255
};
12411256

1242-
return function(exp) {
1257+
return function(exp, expensiveChecks) {
12431258
var parsedExpression;
12441259

12451260
switch (typeof exp) {
12461261
case 'string':
12471262

1263+
var cache = (expensiveChecks ? cacheExpensive : cacheDefault);
12481264
if (cache.hasOwnProperty(exp)) {
12491265
return cache[exp];
12501266
}
12511267

1252-
var lexer = new Lexer($parseOptions);
1253-
var parser = new Parser(lexer, $filter, $parseOptions);
1268+
var parseOptions = expensiveChecks ? $parseOptionsExpensive : $parseOptions;
1269+
var lexer = new Lexer(parseOptions);
1270+
var parser = new Parser(lexer, $filter, parseOptions);
12541271
parsedExpression = parser.parse(exp);
12551272

12561273
if (exp !== 'hasOwnProperty') {

test/ng/directive/ngEventDirsSpec.js

+19
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,25 @@ describe('event directives', function() {
8282

8383
});
8484

85+
describe('security', function() {
86+
it('should allow access to the $event object', inject(function($rootScope, $compile) {
87+
var scope = $rootScope.$new();
88+
element = $compile('<button ng-click="e = $event">BTN</button>')(scope);
89+
element.triggerHandler('click');
90+
expect(scope.e.target).toBe(element[0]);
91+
}));
92+
93+
it('should block access to DOM nodes (e.g. exposed via $event)', inject(function($rootScope, $compile) {
94+
var scope = $rootScope.$new();
95+
element = $compile('<button ng-click="e = $event.target">BTN</button>')(scope);
96+
expect(function() {
97+
element.triggerHandler('click');
98+
}).toThrowMinErr(
99+
'$parse', 'isecdom', 'Referencing DOM nodes in Angular expressions is disallowed! ' +
100+
'Expression: e = $event.target');
101+
}));
102+
});
103+
85104
describe('blur', function() {
86105

87106
describe('call the listener asynchronously during $apply', function() {

test/ng/parseSpec.js

+17-3
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,10 @@
33
describe('parser', function() {
44

55
beforeEach(function() {
6-
/* global getterFnCache: true, promiseWarningCache: true */
6+
/* global getterFnCacheDefault: true, getterFnCacheExpensive: true, promiseWarningCache: true */
77
// clear caches
8-
getterFnCache = {};
8+
getterFnCacheDefault = {};
9+
getterFnCacheExpensive = {};
910
promiseWarningCache = {};
1011
});
1112

@@ -1067,7 +1068,6 @@ describe('parser', function() {
10671068
expect(count).toBe(1);
10681069
});
10691070

1070-
10711071
it('should call the function once when it is not part of the context', function() {
10721072
var count = 0;
10731073
scope.fn = function() {
@@ -1078,6 +1078,20 @@ describe('parser', function() {
10781078
expect(count).toBe(1);
10791079
});
10801080

1081+
describe('expensiveChecks', function() {
1082+
it('should block access to window object even when aliased', inject(function($parse, $window) {
1083+
scope.foo = {w: $window};
1084+
// This isn't blocked for performance.
1085+
expect(scope.$eval($parse('foo.w'))).toBe($window);
1086+
// Event handlers use the more expensive path for better protection since they expose
1087+
// the $event object on the scope.
1088+
expect(function() {
1089+
scope.$eval($parse('foo.w', true));
1090+
}).toThrowMinErr(
1091+
'$parse', 'isecwindow', 'Referencing the Window in Angular expressions is disallowed! ' +
1092+
'Expression: foo.w');
1093+
}));
1094+
});
10811095

10821096
it('should call the function once when it is part of the context on assignments', function() {
10831097
var count = 0;

0 commit comments

Comments
 (0)