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

Conversation

petebacondarwin
Copy link
Member

Objects received from outside AngularJS may have had their hasOwnProperty
method overridden with something else.
Also, we may be using an object internally to store/cache a hash-map, where
the keys for the map are provided from outside AngularJS.

In such cases we must either call directly against
Object.prototype.hasOwnProperty or provide some other way of preventing
our objects from having hasOwnProperty overridden.

Closes #2141

@petebacondarwin
Copy link
Member Author

@IgorMinar - OK, so I think this is now ready to be reviewed. Can you take a look when you have a moment?

@@ -0,0 +1,4 @@
@ngdoc error
@name $injector:badcnst
@fullName hasOwnProperty cannot be used as a module name
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean hasOwnProperty cannot be used as a constant name? This looks like copypasta.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ksheedlo - tut tut, Pete, copy and paste!

@petebacondarwin
Copy link
Member Author

@IgorMinar - updated the commit. Can you take another look?

@ghost ghost assigned IgorMinar and vojtajina Oct 2, 2013
@@ -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 ;-)

@vojtajina
Copy link
Contributor

@petebacondarwin I think this looks good. Can you

  • fix the little style issues
  • separate style corrections into a separate style: commit (optional, but I would do it ;-))

Regards separating the style corrections. I think this is very useful for reviewing - if there's a commit style I just skim it, pretty much ignore it. This commit contains a lot of stuff like this, which is just a distraction when reviewing the core change of this PR. Also, when bisecting git history, again, it's easier to find issues. (last time I was using bisect for finding the compiler issue and it was very helpful).


Little side note:
In the future (once we don't have to worry about stupid non ES5 browsers), we should use Object.create(null) (or even better ES6 maps) for our internal hash maps. That is:

  • DI instance/providers cache
  • register of directives, controllers
  • options map in <select>
  • parser cache

@petebacondarwin
Copy link
Member Author

@vojtajina - thanks for reviewing. Will update over the weekend.

@petebacondarwin
Copy link
Member Author

@vojtajina - I have updated according to your comments. Can you let me know if it is not good to merge.

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 angular#3331
@@ -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.

@vojtajina
Copy link
Contributor

@petebacondarwin Great job! Thanks for splitting it up.

I rebased it on the latest master and did a couple of small changes (mentioned in my comments). Waiting for build and then merging...

jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
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 angular#3331
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
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 angular#3331
@petebacondarwin petebacondarwin deleted the hasOwnPropertyFn branch February 11, 2014 09:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants