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

fix(*): protect calls to hasOwnProperty in public API #3331

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/content/error/ng/badname.ngdoc
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.
8 changes: 8 additions & 0 deletions docs/content/error/resource/badname.ngdoc
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.
23 changes: 13 additions & 10 deletions src/Angular.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,6 @@

////////////////////////////////////

/**
* hasOwnProperty may be overwritten by a property of the same name, or entirely
* absent from an object that does not inherit Object.prototype; this copy is
* used instead
*/
var hasOwnPropertyFn = Object.prototype.hasOwnProperty;
var hasOwnPropertyLocal = function(obj, key) {
return hasOwnPropertyFn.call(obj, key);
};

/**
* @ngdoc function
* @name angular.lowercase
Expand Down Expand Up @@ -691,6 +681,8 @@ function shallowCopy(src, dst) {
dst = dst || {};

for(var key in src) {
// shallowCopy is only ever called by $compile nodeLinkFn, which has control over src
// so we don't need to worry hasOwnProperty here
if (src.hasOwnProperty(key) && key.substr(0, 2) !== '$$') {
dst[key] = src[key];
}
Expand Down Expand Up @@ -1187,6 +1179,17 @@ function assertArgFn(arg, name, acceptArrayAnnotation) {
return arg;
}

/**
* throw error if the name given is hasOwnProperty
* @param {String} name the name to test
* @param {String} context the context in which the name is used, such as module or directive
*/
function assertNotHasOwnProperty(name, context) {
if (name === 'hasOwnProperty') {
throw ngMinErr('badname', "hasOwnProperty is not a valid {0} name", context);
}
}

/**
* Return the value accessible from the object by path. Any undefined traversals are ignored
* @param {Object} obj starting object
Expand Down
2 changes: 2 additions & 0 deletions src/auto/injector.js
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,7 @@ function createInjector(modulesToLoad) {
}

function provider(name, provider_) {
assertNotHasOwnProperty(name, 'service');
if (isFunction(provider_) || isArray(provider_)) {
provider_ = providerInjector.instantiate(provider_);
}
Expand All @@ -475,6 +476,7 @@ function createInjector(modulesToLoad) {
function value(name, value) { return factory(name, valueFn(value)); }

function constant(name, value) {
assertNotHasOwnProperty(name, 'constant');
providerCache[name] = value;
instanceCache[name] = value;
}
Expand Down
5 changes: 4 additions & 1 deletion src/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

function setupModuleLoader(window) {

var $injectorMinErr = minErr('$injector');

function ensure(obj, name, factory) {
return obj[name] || (obj[name] = factory());
}
Expand Down Expand Up @@ -68,12 +70,13 @@ function setupModuleLoader(window) {
* @returns {module} new module with the {@link angular.Module} api.
*/
return function module(name, requires, configFn) {
assertNotHasOwnProperty(name, 'module');
if (requires && modules.hasOwnProperty(name)) {
modules[name] = null;
}
return ensure(modules, name, function() {
if (!requires) {
throw minErr('$injector')('nomod', "Module '{0}' is not available! You either misspelled the module name " +
throw $injectorMinErr('nomod', "Module '{0}' is not available! You either misspelled the module name " +
"or forgot to load it. If registering a module ensure that you specify the dependencies as the second " +
"argument.", name);
}
Expand Down
4 changes: 4 additions & 0 deletions src/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ function $CompileProvider($provide) {
* @returns {ng.$compileProvider} Self for chaining.
*/
this.directive = function registerDirective(name, directiveFactory) {
assertNotHasOwnProperty(name, 'directive');
if (isString(name)) {
assertArg(directiveFactory, 'directiveFactory');
if (!hasDirectives.hasOwnProperty(name)) {
Expand Down Expand Up @@ -1175,6 +1176,9 @@ function $CompileProvider($provide) {
dst['class'] = (dst['class'] ? dst['class'] + ' ' : '') + value;
} else if (key == 'style') {
$element.attr('style', $element.attr('style') + ';' + value);
// `dst` will never contain hasOwnProperty as DOM parser won't let it.
// You will get an "InvalidCharacterError: DOM Exception 5" error if you
// have an attribute like "has-own-property" or "data-has-own-property", etc.
} else if (key.charAt(0) != '$' && !dst.hasOwnProperty(key)) {
dst[key] = value;
dstAttr[key] = srcAttr[key];
Expand Down
1 change: 1 addition & 0 deletions src/ng/controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ function $ControllerProvider() {
* annotations in the array notation).
*/
this.register = function(name, constructor) {
assertNotHasOwnProperty(name, 'controller');
if (isObject(name)) {
extend(controllers, name)
} else {
Expand Down
5 changes: 4 additions & 1 deletion src/ng/directive/form.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,12 @@ function FormController(element, attrs) {
* Input elements using ngModelController do this automatically when they are linked.
*/
form.$addControl = function(control) {
// Breaking change - before, inputs whose name was "hasOwnProperty" were quietly ignored
Copy link
Contributor

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.

// and not added to the scope. Now we throw an error.
assertNotHasOwnProperty(control.$name, 'input');
controls.push(control);

if (control.$name && !form.hasOwnProperty(control.$name)) {
if (control.$name) {
form[control.$name] = control;
}
};
Expand Down
2 changes: 2 additions & 0 deletions src/ng/directive/ngRepeat.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this ?

Copy link
Contributor

Choose a reason for hiding this comment

The 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];
Expand All @@ -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);
Expand Down
22 changes: 12 additions & 10 deletions src/ng/directive/select.js
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
Expand Down Expand Up @@ -148,10 +149,11 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
ngModelCtrl = ngModelCtrl_;
nullOption = nullOption_;
unknownOption = unknownOption_;
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The 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) {
Expand All @@ -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
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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 = [];
Expand All @@ -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;
}
Expand All @@ -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);
Expand Down Expand Up @@ -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 = {};
Expand Down Expand Up @@ -564,7 +566,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
}
}
}
}
};
}];

var optionDirective = ['$interpolate', function($interpolate) {
Expand Down Expand Up @@ -613,5 +615,5 @@ var optionDirective = ['$interpolate', function($interpolate) {
});
};
}
}
};
}];
28 changes: 23 additions & 5 deletions src/ng/parse.js
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -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];
}
Expand Down Expand Up @@ -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;
}

///////////////////////////////////
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm changing this to just reuturn parsedExpression, as I don't think there is any reason to parse it twice, right ?


case 'function':
return exp;

default:
return noop;
}
Expand Down
4 changes: 2 additions & 2 deletions src/ngMock/angular-mocks.js
Original file line number Diff line number Diff line change
Expand Up @@ -731,7 +731,7 @@ angular.mock.dump = function(object) {
offset = offset || ' ';
var log = [offset + 'Scope(' + scope.$id + '): {'];
for ( var key in scope ) {
if (scope.hasOwnProperty(key) && !key.match(/^(\$|this)/)) {
if (Object.prototype.hasOwnProperty.call(scope, key) && !key.match(/^(\$|this)/)) {
log.push(' ' + key + ': ' + angular.toJson(scope[key]));
}
}
Expand Down Expand Up @@ -1773,7 +1773,7 @@ angular.mock.clearDataCache = function() {
cache = angular.element.cache;

for(key in cache) {
if (cache.hasOwnProperty(key)) {
if (Object.prototype.hasOwnProperty.call(cache,key)) {
var handle = cache[key].handle;

handle && angular.element(handle.elem).off();
Expand Down
3 changes: 3 additions & 0 deletions src/ngResource/resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,9 @@ angular.module('ngResource', ['ng']).

var urlParams = self.urlParams = {};
forEach(url.split(/\W/), function(param){
if (param === 'hasOwnProperty') {
throw $resourceMinErr('badname', "hasOwnProperty is not a valid parameter name.");
}
if (!(new RegExp("^\\d+$").test(param)) && param && (new RegExp("(^|[^\\\\]):" + param + "(\\W|$)").test(url))) {
urlParams[param] = true;
}
Expand Down
16 changes: 15 additions & 1 deletion test/AngularSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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); });
Copy link
Contributor

Choose a reason for hiding this comment

The 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']);
});

Expand Down
Loading