Copy own __proto__ safely

[mnespor]

• When the src object has an own proto property, avoid
  modifying the result object's prototype

[TehShrike]

O hai @mnespor!

This change takes deepmerge farther down the path that I'm trying to walk it back from in e.g. #152 – I want to make deepmerge more explicitly only concerned with deep merging properties on vanilla objects.

deepmerge has historically been unclear on exactly what properties it should recurse down into, but starting with I think the defaulting of the clone option to true, it's been consistently moving towards cloning plain objects.

Once #152 is implemented, cloned properties will never even have a prototype with any meaningful properties of its own!

[TehShrike]

I'm tempted to go in the direction of "only assign new values if the target doesn't have the property, OR if the target hasOwnProperty and that property is enumerable" and just not copy any values where that isn't the case

[mnespor]

I like that a lot. It moves in the vanilla object direction and it protects from JSON.parse __proto__ skulduggery.

Should a change like that point at the v5 branch?

[TehShrike]

Checking for undefined isn't technically correct, since it would incorrectly return true in this case:

Object.defineProperty({}, key, {
	configurable: true,
	enumerable: false,
	writable: true,
	value: undefined
})

The most correct check I can think of would be !(key in target).

[TehShrike]

No need to point this at the v5 branch, this will be a good feature release.

[TehShrike]

:-o can in ever throw?

[mnespor]

I found out it can, specifically on the test case that replaces a plain string with a nested object

…

On Oct 4, 2019, at 10:16 AM, Josh Duff ***@***.***> wrote:  :-o can in ever throw? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[TehShrike]

Would something like (typeof target[key] !== 'object' || key in target) be correct?

[mnespor]

Unsure. Depends on what you'd like it to do when target is a function?

[TehShrike]

Now I'm leaning towards renaming propertyIsPlain to something along the lines of isSafeToSetProperty, and checking Object.isFrozen(target) before checking for key in target.

(If target was frozen, isSafeToSetProperty would return false.)

Downside: this would throw a TypeError in ES5 environments if you tried to merge onto a string.

---

After thinking about it some more, I'm getting nervous about breaking the case where someone passes in custom isMergeableObject and customMerge functions that do custom merging for strings.

Maybe the test function should be changed to propertyIsUnsafe, and it should only return true when !(target.hasOwnProperty(key) && target.propertyIsEnumerable(key)).

It could return true in every other case, even if target is frozen or a primitive.

---

I just realized that propertyIsPlain would currently throw for targets that didn't have Object.prototype in their prototype chain, e.g. objects created with Object.create(null).

If we switch to Object.prototype.hasOwnProperty.call(target, key) we avoid that error, and it looks like it returns false instead of throwing an error if target is a string!

[TehShrike]

Wanna switch the function to propertyIsUnsafe with Object.prototype.hasOwnProperty.call + Object.prototype.propertyIsEnumerable.call?

[mnespor]

Definitely.

I noticed that !(target.hasOwnProperty(key) && target.propertyIsEnumerable(key)) allowed prototype poisoning again, since __proto__ wasn't an own property on the target object. How about target && target[key] && !Object.propertyIsEnumerable.call(target, key)? It behaves similarly while protecting target.

[TehShrike]

How about target && target[key] && !Object.propertyIsEnumerable.call(target, key)

I don't think we need to check for target being truthy at this part of the code.

That "is unsafe" check would incorrectly return false in the case where target[key] is a non-enumerable property with a falsey value.

I noticed that !(target.hasOwnProperty(key) && target.propertyIsEnumerable(key)) allowed prototype poisoning again, since __proto__ wasn't an own property on the target object

Oh right... we only need to check for hasOwnProperty at all if the property exists somewhere up the prototype chain, and we only need to check propertyIsEnumerable if the property exists on target.

And since in is the only real way to see if a property exists up the prototype chain, I guess we're stuck with it.

Spelling it out for my sanity – a property is unsafe to touch if:

• it exists on the target object, or up the target's prototype chain, but is not enumerable
• it exists up the target's prototype chain

So, unsafe when key in target && (!target.hasOwnProperty(key) || !target.propertyIsEnumerable(key)) >de morgan> key in target && !(target.hasOwnProperty(key) && target.propertyIsEnumerable(key))

So I think I've talked myself back to something like what you had before my last comment.

---

A property is also unsafe if the target object is frozen, right? I think I was confusing merging string values with merging string properties.

[mnespor]

For a few minutes tonight, I considered frozen target objects unsafe to merge. However, that caused test case 'replace simple key with nested object in target' to fail. It recurses until it calls mergeObject('value1', { subkey1: 'subvalue1', subkey2: 'subvalue2' }, options). That's safe, I think. It generates a new destination, loaded with subkey1 and subkey2. I suggest omitting the frozen check.

[TehShrike]

Published as 4.2.0 🌿

[oliviertassinari]

I want to make deepmerge more explicitly only concerned with deep merging properties on vanilla objects.

@TehShrike It's exactly why we are using this dependency for in Material-UI. +1 for going down this direction, especially if it can save bytes.