Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Investigate potential `__proto__` issues. #226

Closed
jdalton opened this Issue · 14 comments

3 participants

@jdalton
Owner

Investigate potential __proto__ issues. //cc @webreflection

@jdalton
Owner
var a = {};a.__proto__ = [];Object.keys(a);
// => [] which is expected.
@WebReflection
var a = Object.create(null);a.__proto__ = [];Object.keys(a);
// => ["__proto__"]

which is able to screw clone/copy operations being a bomb in the system able to break all instances

check this in Canary, Firefox, Nightly, or node.js

@jdalton
Owner
var a = Object.create(null);a.__proto__=[];Object.keys(a);
// => ['__proto__']; in Opera (the correct behavior).
// Safari/Chrome/Firefox are currently [], though Chrome has it fixed in Canary
@WebReflection

precisely

@jdalton
Owner

which is able to screw clone/copy operations being a bomb in the system able to break all instances

This is the desired behavior, not a bomb. On an object with a null [[Prototype]], __proto__ is just a regular property. I'll look into making current Chrome and others consistent.

Also, how awesome is it that we both typed the exact same example code within seconds of each other. Wonder twin powers!

@jdalton
Owner

Ok, so since detecting if __proto__ has been manually added to an object with a null [[Prototype]] is impossible-ish on current non-ES6 compliant browsers, because __proto__ is still active on the object, I'm going to punt on addressing it. However, I believe addressing it would be trivial & have minimal perf impact if I could detect it.

To reiterate the behavior of var a=Object.create(null);a.__proto__=[];Object.keys(a); //['__proto__'] is desired.

@jdalton jdalton closed this
@sharifmacky

//['__proto__'] is desired

Sorry, is that correct?

Looking at http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf

(15.2.3.5) Object.create: 3. Set the [[Prototype]] internal property of obj to O

Shouldn't it still be enumerable:false (similarly of the es6 draft)

@jdalton
Owner

@sharifmacky ? ES6 draft has Object.prototype.__proto__ defined. When you create an object via Object.create(null) it has a null internal [[Prototype]] value and so does not inherit from Object.prototype. Any properties added to the object by assignment will be enumerable by default. When the __proto__ property is added to an object with a null [[Prototype]] it's just a regular property w/o any special abilities.

@sharifmacky

Sorry of coarse, so Object.create(null) will give you a new object with [[Prototype]] = null but no __proto__. I was thinking that [[Prototype]] and __proto__ would be the same... My mistake ;-)

Thanks.

@WebReflection

@jdalton the issue is not that Object.create(null) do not inherit __proto__ from Object.prototype and it threat it as a generic key, which is indeed expected and desired, the issue is that most mobile browsers have same issue Chrome has and the update there won't be available any soon.

Why This Is A Problem
Every method that should iterate over a generic object cannot work anymore as expected in a consistent way unless a check against key === '__proto__' is performed per each key before acting.

_.extend() is just an example, if the source object is a Dictionary (read Object.create(null)), and the __proto__ is a key, and the target object is a generic object that can have inheritance changed because of this key, which is undesired and never a problem until now thanks to non-enumerability of that key even in dictionaries, what should happen?

If Lo-Dash wants to be consistent with the known behavior:

  • each _.extend(target, source) call should verify what kind of objects those are
  • if target is Dictionary and source is Dictionary, copy everything without problems
  • if target is Object, and source is Dictionary, do not copy the possible __proto__ key preserving inheritance integrity and currently known behavior
  • if target is Object, and source is Object, and the Object.prototype descriptor of __proto__ is not the native one or it has been simply deleted, act as Dictionary & Dictionary, copy everything

Thanks TC39, We Gonna Have Bad Time
Are you willing to make a basic method such copy or extend aware of all possible scenarios introduced by the __proto__ concept which is configurable, then rewritable, potentially made enumerable if reconfigured, and not present in Dictionary only for the 50% of browsers out there?
As far as I know, IE11 is implementing that property in an even different way so, brace yourself, disaster and fear oriented development is coming due a property in the middle that is causing an exception to the standard that nobody really wants.

What To Do To Change This
AFAIK Allen confirmed that TC39 is aware of all these problems but it does not believe that libraries will drop its usage.

Of course libraries will not drop it until there is a standard able to replace same behavior.
In order to make TC39 change its mind we need to use a code that is relying into a non existent yet standard

var setPrototypeOf = Object.setPrototypeOf || function (o, p) {
  o.__proto__ = p;
  return o;
};

This method preserves easiness and semantic, allowing {__proto__:obj} behavior via Object.setPrototypeOf({}, obj) and once widely adopted could make it in favor of deprecating and removing from any spec the current __proto__ keyword ... which is a bomb in the system.

@jdalton
Owner

the issue is not that Object.create(null) do not inherit __proto__ from Object.prototype and it threat it as a generic key, which is indeed expected and desired, the issue is that most mobile browsers have same issue Chrome has and the update there won't be available any soon.

I'm not concerned w/ what the current non-spec'ed behavior is (we can't control that), I'm glad the spec'ed behavior is clearing up these issues.

Why This Is A Problem
Every method that should iterate over a generic object cannot work anymore as expected in a consistent way unless a check against key === 'proto' is performed per each key before acting.

Cannot work anymore? This has been an issue with __proto__ since the beginning and there hasn't been a pain point across libs that I'm aware of. The double underscore prefix/postfix does a good job of avoiding accidental issues.

Thanks TC39, We Gonna Have Bad Time

No, thanks to the TC39 __proto__ is getting better/more-consistent, we'll have a good time :D

If Lo-Dash wants to be consistent with the known behavior...

I've already explored this in my previous comment.
I may also simply prefix key names I use for data objects, in cachedContains for example.

As far as I know, IE11 is implementing that property in an even different way so, brace yourself, disaster and fear oriented development is coming due a property in the middle that is causing an exception to the standard that nobody really wants.

Let's not speculate on IE11 please.

This method preserves easiness and semantic, allowing {__proto__:obj} behavior via Object.setPrototypeOf({}, obj) and once widely adopted could make it in favor of deprecating and removing from any spec the current __proto__ keyword ... which is a bomb in the system

__proto__ is already used and a de facto standard. Standardizing is the right answer to clear up existing inconsistencies.

@WebReflection

using Object.setPrototypeOf(), and everybody seems to agree on it, would solve this mess once for everyone without granting future problems for a property name, regardless which one is it, able to potentially destroy services.

The usage of a global Object method would be explicit and avoid everything we are discussing here .. so the point is, as I've written in my post, being stubborn for something that easy to solve nobody wants to.

So here my question: would you use Object.setPrototypeOf(obj, proto) if spec'd and if __proto__ will disappear?
I believe simply yes because there is no reason to not do it, IMHO, while there are tons of problems going forward with this messed up property.

@jdalton
Owner

using Object.setPrototypeOf(), and everybody seems to agree on it, would solve this mess once for everyone without granting future problems for a property name, regardless which one is it, able to potentially destroy services.

The sky is not falling. If it was a real pain point setPrototypeOf may have won out. As it is __proto__ getting standardized is a win, dealing w/ its current inconsistencies is a minor inconvenience.

@jdalton
Owner

_.extend() is just an example, if the source object is a Dictionary (read Object.create(null)), and the __proto__ is a key, and the target object is a generic object that can have inheritance changed because of this key, which is undesired and never a problem until now thanks to non-enumerability of that key even in dictionaries, what should happen?

I think that's a feature. If the source object has an enumerable __proto__ it may be some mixin or options object that is designed to set properties of the destination object.

So the only issue I can see is with our _.memoize and cachedContains data objects, which I'll address by prefixing keys. Methods like _.groupBy and _.countBy will be solved by convention.

@jdalton jdalton referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@jdalton jdalton referenced this issue from a commit
@jdalton jdalton Add `keyPrefix` to avoid issues with `__proto__`. [closes #226]
Former-commit-id: 55dee782acdd5e28229b1fcb7587424d3fdfd445
fd3b62e
@jdalton jdalton referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@jdalton jdalton referenced this issue from a commit
@jdalton jdalton Add `keyPrefix` to avoid issues with `__proto__`. [closes #226]
Former-commit-id: 55dee782acdd5e28229b1fcb7587424d3fdfd445
a835329
@jdalton jdalton referenced this issue from a commit
@jdalton jdalton Add `keyPrefix` to avoid issues with `__proto__`. [closes #226]
Former-commit-id: 55dee782acdd5e28229b1fcb7587424d3fdfd445
6c25905
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.