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

Commit

Permalink
fix(*): protect calls to hasOwnProperty in public API
Browse files Browse the repository at this point in the history
Objects received from outside AngularJS may have had their `hasOwnProperty`
method overridden with something else. In cases where we can do this without
incurring a performance penalty we call directly on Object.prototype.hasOwnProperty
to ensure that we use the correct method.

Also, we have some internal hash objects, where the keys for the map are provided
from outside AngularJS. In such cases we either prevent `hasOwnProperty` from
being used as a key or provide some other way of preventing our objects from
having their `hasOwnProperty` overridden.

BREAKING CHANGE: Inputs with name equal to "hasOwnProperty" are not allowed inside
form or ngForm directives.

Before, inputs whose name was "hasOwnProperty" were quietly ignored and not added
to the scope.  Now a badname exception is thrown.

Using "hasOwnProperty" for an input name would be very unusual and bad practice.
Either do not include such an input in a `form` or `ngForm` directive or change
the name of the input.

Closes #3331
  • Loading branch information
petebacondarwin authored and vojtajina committed Oct 7, 2013
1 parent fb99f54 commit 7a586e5
Show file tree
Hide file tree
Showing 24 changed files with 196 additions and 17 deletions.
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
// 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');
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
4 changes: 3 additions & 1 deletion 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 @@ -152,6 +153,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {


self.addOption = function(value) {
assertNotHasOwnProperty(value, '"option value"');
optionsMap[value] = true;

if (ngModelCtrl.$viewValue == value) {
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
22 changes: 20 additions & 2 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 @@ -1036,14 +1045,23 @@ function $ParseProvider() {
return function(exp) {
var lexer = new Lexer($sniffer.csp);
var parser = new Parser(lexer, $filter, $sniffer.csp);
var parsedExpression;

switch (typeof exp) {
case 'string':
if (cache.hasOwnProperty(exp)) {
return cache[exp];
}

return cache[exp] = parser.parse(exp, false);
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 parsedExpression;

case 'function':
return exp;
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
14 changes: 14 additions & 0 deletions test/AngularSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,20 @@ 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 = [];
Expand Down
18 changes: 18 additions & 0 deletions test/auto/injectorSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,24 @@ describe('injector', function() {
});

describe('$provide', function() {

it('should throw an exception if we try to register a service called "hasOwnProperty"', function() {
createInjector([function($provide) {
expect(function() {
$provide.provider('hasOwnProperty', function() { });
}).toThrowMinErr('ng', 'badname');
}]);
});

it('should throw an exception if we try to register a constant called "hasOwnProperty"', function() {
createInjector([function($provide) {
expect(function() {
$provide.constant('hasOwnProperty', {});
}).toThrowMinErr('ng', 'badname');
}]);
});


describe('constant', function() {
it('should create configuration injectable constants', function() {
var log = [];
Expand Down
6 changes: 6 additions & 0 deletions test/loaderSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,4 +72,10 @@ describe('module loader', function() {
"or forgot to load it. If registering a module ensure that you specify the dependencies as the second " +
"argument.");
});

it('should complain if a module is called "hasOwnProperty', function() {
expect(function() {
window.angular.module('hasOwnProperty', []);
}).toThrowMinErr('ng','badname', "hasOwnProperty is not a valid module name");
});
});
9 changes: 9 additions & 0 deletions test/ng/compileSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,15 @@ describe('$compile', function() {
expect(log).toEqual('pre1; pre2; post2; post1');
});
});

it('should throw an exception if a directive is called "hasOwnProperty"', function() {
module(function() {
expect(function() {
directive('hasOwnProperty', function() { });
}).toThrowMinErr('ng','badname', "hasOwnProperty is not a valid directive name");
});
inject(function($compile) {});
});
});


Expand Down
7 changes: 7 additions & 0 deletions test/ng/controllerSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@ describe('$controller', function() {
expect(scope.foo).toBe('bar');
expect(ctrl instanceof FooCtrl).toBe(true);
});


it('should throw an exception if a controller is called "hasOwnProperty"', function () {
expect(function() {
$controllerProvider.register('hasOwnProperty', function($scope) {});
}).toThrowMinErr('ng', 'badname', "hasOwnProperty is not a valid controller name");
});
});


Expand Down
12 changes: 12 additions & 0 deletions test/ng/directive/formSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,18 @@ describe('form', function() {
});


it('should throw an exception if an input has name="hasOwnProperty"', function() {
doc = jqLite(
'<form name="form">'+
'<input name="hasOwnProperty" ng-model="some" />'+
'<input name="other" ng-model="someOther" />'+
'</form>');
expect(function() {
$compile(doc)(scope);
}).toThrowMinErr('ng', 'badname');
});


describe('preventing default submission', function() {

it('should prevent form submission', function() {
Expand Down
8 changes: 8 additions & 0 deletions test/ng/directive/ngRepeatSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,14 @@ describe('ngRepeat', function() {
});


it("should throw an exception if 'track by' evaluates to 'hasOwnProperty'", function() {
scope.items = {age:20};
$compile('<div ng-repeat="(key, value) in items track by \'hasOwnProperty\'"></div>')(scope);
scope.$digest();
expect($exceptionHandler.errors.shift().message).toMatch(/ng:badname/);
});


it('should track using build in $id function', function() {
element = $compile(
'<ul>' +
Expand Down
Loading

0 comments on commit 7a586e5

Please sign in to comment.