-
Notifications
You must be signed in to change notification settings - Fork 27.5k
fix(*): protect calls to hasOwnProperty in public API #3331
Conversation
@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 |
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.
Do you mean hasOwnProperty cannot be used as a constant
name? This looks like copypasta.
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.
@ksheedlo - tut tut, Pete, copy and paste!
@IgorMinar - updated the commit. Can you take another look? |
@@ -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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I got it ;-)
@petebacondarwin I think this looks good. Can you
Regards separating the style corrections. I think this is very useful for reviewing - if there's a commit Little side note:
|
@vojtajina - thanks for reviewing. Will update over the weekend. |
@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 |
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.
@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... |
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
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
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 preventingour objects from having hasOwnProperty overridden.
Closes #2141