Skip to content

Commit

Permalink
fix($parse, events): prevent accidental misuse of properties on $event
Browse files Browse the repository at this point in the history
  • Loading branch information
@chirayuk
chirayuk committed Nov 7, 2014
1 parent e676d64 commit e057a9a
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 24 deletions.
6 changes: 5 additions & 1 deletion src/ng/directive/ngEventDirs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,11 @@ forEach(
return {
restrict: 'A',
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], /* interceptorFn */ null, /* expensiveChecks */ true);
return function ngEventHandler(scope, element) {
element.on(eventName, function(event) {
var callback = function() {
Expand Down
53 changes: 33 additions & 20 deletions src/ng/parse.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -852,7 +852,8 @@ function setter(obj, path, setValue, fullExp) {
return setValue;
}

var getterFnCache = createMap();
var getterFnCacheDefault = createMap();
var getterFnCacheExpensive = createMap();

function isPossiblyDangerousMemberName(name) {
return name == 'constructor';
Expand All @@ -863,7 +864,7 @@ function isPossiblyDangerousMemberName(name) {
* - http://jsperf.com/angularjs-parse-getter/4
* - http://jsperf.com/path-evaluation-simplified/7
*/
function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp) {
function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp, expensiveChecks) {
ensureSafeMemberName(key0, fullExp);
ensureSafeMemberName(key1, fullExp);
ensureSafeMemberName(key2, fullExp);
Expand All @@ -872,11 +873,11 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp) {
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 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 function cspSafeGetter(scope, locals) {
var pathVal = (locals && locals.hasOwnProperty(key0)) ? locals : scope;
Expand Down Expand Up @@ -911,23 +912,25 @@ function getterFnWithEnsureSafeObject(fn, fullExpression) {
}

function getterFn(path, options, fullExp) {
var expensiveChecks = options.expensiveChecks;
var getterFnCache = (expensiveChecks ? getterFnCacheExpensive : getterFnCacheDefault);
var fn = getterFnCache[path];

if (fn) return fn;


var pathKeys = path.split('.'),
pathKeysLength = pathKeys.length;

// http://jsperf.com/angularjs-parse-getter/6
if (options.csp) {
if (pathKeysLength < 6) {
fn = cspSafeGetterFn(pathKeys[0], pathKeys[1], pathKeys[2], pathKeys[3], pathKeys[4], fullExp);
fn = cspSafeGetterFn(pathKeys[0], pathKeys[1], pathKeys[2], pathKeys[3], pathKeys[4], fullExp, expensiveChecks);
} else {
fn = function cspSafeGetter(scope, locals) {
var i = 0, val;
do {
val = cspSafeGetterFn(pathKeys[i++], pathKeys[i++], pathKeys[i++], pathKeys[i++],
pathKeys[i++], fullExp)(scope, locals);
pathKeys[i++], fullExp, expensiveChecks)(scope, locals);

locals = undefined; // clear after first iteration
scope = val;
Expand All @@ -937,15 +940,18 @@ function getterFn(path, options, fullExp) {
}
} else {
var code = '';
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
// we simply dereference 's' on any .dot notation
? 's'
// but if we are first then we check locals first, and if so read it first
: '((l&&l.hasOwnProperty("' + key + '"))?l:s)') + '.' + key;
if (isPossiblyDangerousMemberName(key)) {
if (expensiveChecks || isPossiblyDangerousMemberName(key)) {
lookupJs = 'eso(' + lookupJs + ', fe)';
needsEnsureSafeObject = true;
}
Expand Down Expand Up @@ -1030,15 +1036,20 @@ function getValueOf(value) {
* service.
*/
function $ParseProvider() {
var cache = createMap();
var cacheDefault = createMap();
var cacheExpensive = createMap();

var $parseOptions = {
csp: false
};


this.$get = ['$filter', '$sniffer', function($filter, $sniffer) {
$parseOptions.csp = $sniffer.csp;
var $parseOptions = {
csp: $sniffer.csp,
expensiveChecks: false
},
$parseOptionsExpensive = {
csp: $sniffer.csp,
expensiveChecks: true
};

function wrapSharedExpression(exp) {
var wrapped = exp;
Expand All @@ -1055,13 +1066,14 @@ function $ParseProvider() {
return wrapped;
}

return function $parse(exp, interceptorFn) {
return function $parse(exp, interceptorFn, expensiveChecks) {
var parsedExpression, oneTime, cacheKey;

switch (typeof exp) {
case 'string':
cacheKey = exp = exp.trim();

var cache = (expensiveChecks ? cacheExpensive : cacheDefault);
parsedExpression = cache[cacheKey];

if (!parsedExpression) {
Expand All @@ -1070,8 +1082,9 @@ function $ParseProvider() {
exp = exp.substring(2);
}

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 (parsedExpression.constant) {
Expand Down
19 changes: 19 additions & 0 deletions test/ng/directive/ngEventDirsSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,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('<button ng-click="e = $event">BTN</button>')(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('<button ng-click="e = $event.target">BTN</button>')(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() {
Expand Down
24 changes: 21 additions & 3 deletions test/ng/parseSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
describe('parser', function() {

beforeEach(function() {
/* global getterFnCache: true */
// clear cache
getterFnCache = createMap();
/* global getterFnCacheDefault: true */
/* global getterFnCacheExpensive: true */
// clear caches
getterFnCacheDefault = createMap();
getterFnCacheExpensive = createMap();
});


Expand Down Expand Up @@ -783,6 +785,22 @@ describe('parser', function() {
'Expression: foo["bar"]');

});

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', null, true));
}).toThrowMinErr(
'$parse', 'isecwindow', 'Referencing the Window in Angular expressions is disallowed! ' +
'Expression: foo.w');

}));
});
});

describe('Function prototype functions', function() {
Expand Down

0 comments on commit e057a9a

Please sign in to comment.