-
Notifications
You must be signed in to change notification settings - Fork 27.5k
fix(*): protect calls to hasOwnProperty in public API #3331
Changes from all commits
72bd60b
e9618a7
6ff52a7
94246ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
@ngdoc error | ||
@name ng:badname | ||
@fullName Bad `hasOwnProperty` Name | ||
@description | ||
|
||
Occurs when you try to use the name `hasOwnProperty` in a context where it is not allow. | ||
Generally, a name cannot be `hasOwnProperty` because it is used, internally, on a object | ||
and allowing such a name would break lookups on this object. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
@ngdoc error | ||
@name $resource:badname | ||
@fullName Cannot use hasOwnProperty as a parameter name | ||
@description | ||
|
||
Occurs when you try to use the name `hasOwnProperty` as a name of a parameter. | ||
Generally, a name cannot be `hasOwnProperty` because it is used, internally, on a object | ||
and allowing such a name would break lookups on this object. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -305,6 +305,7 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) { | |
key = (collection === collectionKeys) ? index : collectionKeys[index]; | ||
value = collection[key]; | ||
trackById = trackByIdFn(key, value, index); | ||
assertNotHasOwnProperty(trackById, '`track by` id'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nevermind, I got it ;-) |
||
if(lastBlockMap.hasOwnProperty(trackById)) { | ||
block = lastBlockMap[trackById] | ||
delete lastBlockMap[trackById]; | ||
|
@@ -327,6 +328,7 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) { | |
|
||
// remove existing items | ||
for (key in lastBlockMap) { | ||
// lastBlockMap is our own object so we don't need to use special hasOwnPropertyFn | ||
if (lastBlockMap.hasOwnProperty(key)) { | ||
block = lastBlockMap[key]; | ||
$animate.leave(block.elements); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
'use strict'; | ||
|
||
var ngOptionsMinErr = minErr('ngOptions'); | ||
/** | ||
* @ngdoc directive | ||
* @name ng.directive:select | ||
|
@@ -148,10 +149,11 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { | |
ngModelCtrl = ngModelCtrl_; | ||
nullOption = nullOption_; | ||
unknownOption = unknownOption_; | ||
} | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Btw, next time - please commit these "style" fixes as a separate commit. |
||
|
||
|
||
self.addOption = function(value) { | ||
assertNotHasOwnProperty(value, '"option value"'); | ||
optionsMap[value] = true; | ||
|
||
if (ngModelCtrl.$viewValue == value) { | ||
|
@@ -177,12 +179,12 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { | |
$element.prepend(unknownOption); | ||
$element.val(unknownVal); | ||
unknownOption.prop('selected', true); // needed for IE | ||
} | ||
}; | ||
|
||
|
||
self.hasOption = function(value) { | ||
return optionsMap.hasOwnProperty(value); | ||
} | ||
}; | ||
|
||
$scope.$on('$destroy', function() { | ||
// disable unknown option so that we don't do work when the whole select is being destroyed | ||
|
@@ -300,7 +302,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { | |
var match; | ||
|
||
if (! (match = optionsExp.match(NG_OPTIONS_REGEXP))) { | ||
throw minErr('ngOptions')('iexp', | ||
throw ngOptionsMinErr('iexp', | ||
"Expected expression in form of '_select_ (as _label_)? for (_key_,)?_value_ in _collection_' but got '{0}'. Element: {1}", | ||
optionsExp, startingTag(selectElement)); | ||
} | ||
|
@@ -339,7 +341,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { | |
var optionGroup, | ||
collection = valuesFn(scope) || [], | ||
locals = {}, | ||
key, value, optionElement, index, groupIndex, length, groupLength; | ||
key, value, optionElement, index, groupIndex, length, groupLength, trackIndex; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, this is a correct fix, but should not be part of this commit. |
||
|
||
if (multiple) { | ||
value = []; | ||
|
@@ -354,7 +356,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { | |
key = optionElement.val(); | ||
if (keyName) locals[keyName] = key; | ||
if (trackFn) { | ||
for (var trackIndex = 0; trackIndex < collection.length; trackIndex++) { | ||
for (trackIndex = 0; trackIndex < collection.length; trackIndex++) { | ||
locals[valueName] = collection[trackIndex]; | ||
if (trackFn(scope, locals) == key) break; | ||
} | ||
|
@@ -373,7 +375,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { | |
value = null; | ||
} else { | ||
if (trackFn) { | ||
for (var trackIndex = 0; trackIndex < collection.length; trackIndex++) { | ||
for (trackIndex = 0; trackIndex < collection.length; trackIndex++) { | ||
locals[valueName] = collection[trackIndex]; | ||
if (trackFn(scope, locals) == key) { | ||
value = valueFn(scope, locals); | ||
|
@@ -446,7 +448,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { | |
optionGroupNames.push(optionGroupName); | ||
} | ||
if (multiple) { | ||
selected = selectedSet.remove(trackFn ? trackFn(scope, locals) : valueFn(scope, locals)) != undefined; | ||
selected = selectedSet.remove(trackFn ? trackFn(scope, locals) : valueFn(scope, locals)) !== undefined; | ||
} else { | ||
if (trackFn) { | ||
var modelCast = {}; | ||
|
@@ -564,7 +566,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { | |
} | ||
} | ||
} | ||
} | ||
}; | ||
}]; | ||
|
||
var optionDirective = ['$interpolate', function($interpolate) { | ||
|
@@ -613,5 +615,5 @@ var optionDirective = ['$interpolate', function($interpolate) { | |
}); | ||
}; | ||
} | ||
} | ||
}; | ||
}]; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -290,6 +290,7 @@ Lexer.prototype = { | |
text: ident | ||
}; | ||
|
||
// OPERATORS is our own object so we don't need to use special hasOwnPropertyFn | ||
if (OPERATORS.hasOwnProperty(ident)) { | ||
token.fn = OPERATORS[ident]; | ||
token.json = OPERATORS[ident]; | ||
|
@@ -938,6 +939,9 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp) { | |
} | ||
|
||
function getterFn(path, csp, fullExp) { | ||
// 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' | ||
if (getterFnCache.hasOwnProperty(path)) { | ||
return getterFnCache[path]; | ||
} | ||
|
@@ -986,7 +990,12 @@ function getterFn(path, csp, fullExp) { | |
fn.toString = function() { return code; }; | ||
} | ||
|
||
return getterFnCache[path] = fn; | ||
// Only cache the value if it's not going to mess up the cache object | ||
// This is more performant that using Object.prototype.hasOwnProperty.call | ||
if (path !== 'hasOwnProperty') { | ||
getterFnCache[path] = fn; | ||
} | ||
return fn; | ||
} | ||
|
||
/////////////////////////////////// | ||
|
@@ -1034,19 +1043,28 @@ function $ParseProvider() { | |
var cache = {}; | ||
this.$get = ['$filter', '$sniffer', function($filter, $sniffer) { | ||
return function(exp) { | ||
var lexer, parser, parsedExpression; | ||
switch (typeof exp) { | ||
case 'string': | ||
if (cache.hasOwnProperty(exp)) { | ||
return cache[exp]; | ||
} | ||
|
||
var lexer = new Lexer($sniffer.csp); | ||
var parser = new Parser(lexer, $filter, $sniffer.csp); | ||
return cache[exp] = parser.parse(exp, false); | ||
lexer = new Lexer($sniffer.csp); | ||
parser = new Parser(lexer, $filter, $sniffer.csp); | ||
parsedExpression = parser.parse(exp, false); | ||
|
||
if (exp !== 'hasOwnProperty') { | ||
// Only cache the value if it's not going to mess up the cache object | ||
// This is more performant that using Object.prototype.hasOwnProperty.call | ||
cache[exp] = parsedExpression; | ||
} | ||
|
||
return parser.parse(exp, false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm changing this to just |
||
|
||
case 'function': | ||
return exp; | ||
|
||
default: | ||
return noop; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -417,11 +417,25 @@ describe('angular', function() { | |
}); | ||
|
||
|
||
it('should not break if obj is an array we override hasOwnProperty', function() { | ||
var obj = []; | ||
obj[0] = 1; | ||
obj[1] = 2; | ||
obj.hasOwnProperty = null; | ||
var log = []; | ||
forEach(obj, function(value, key) { | ||
log.push(key + ':' + value); | ||
}); | ||
expect(log).toEqual(['0:1', '1:2']); | ||
}); | ||
|
||
|
||
|
||
it('should handle JQLite and jQuery objects like arrays', function() { | ||
var jqObject = jqLite("<p><span>s1</span><span>s2</span></p>").find("span"), | ||
log = []; | ||
|
||
forEach(jqObject, function(value, key) { log.push(key + ':' + value.innerHTML)}); | ||
forEach(jqObject, function(value, key) { log.push(key + ':' + value.innerHTML); }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be not part of this commit ;-) |
||
expect(log).toEqual(['0:s1', '1:s2']); | ||
}); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm removing this comment, as I don't think it's useful to keep it here. You mentioned it in the commit msg, that's the right place.