Fix 2226: restore props defined on prototype chain by deleting

[fatso83]

Purpose (TL;DR) - mandatory

Issue #2226 shows that we somehow handle props defined on the prototype chain in the wrong manner. This has mostly gone unnoticed, until someone tried stubbing a prop defined on a prototype that had configurable: false. Doing a restore() will fail in this case.

Background (Problem in detail) - optional

Up until now, we have simply defined a new property on the object, regardless of where the prop actually "lives". This is fine and does not cause an issue in itself. The problem is that when we restore, we try to restore/recreate the property descriptor onto the object, regardless of that object is the original source of that descriptor.

Truth be told, it is not that the "source" does not match: the basic problem here is that we try to redefine a property that we have said is non-configurable, but that is just a symptom of the underlying problem, which is that we do not take the source into consideration at creation/restore phases.

The fix here is quite simple, as can be seen:

1. Create a marker on (an extended version of) the property descriptor that tells us if the object comes from the prototype chain or if it is owned by the object itself.
2. Use this fact when creating the prop descriptor: we need to have configurable:true to be able to redefine and/or delete this prop from the object later on
3. Use this fact when restoring. If this prop never "belonged" to us, we can safely just delete it.

Details that need to be fixed for clarity (now moved into #2238)

I will try to create a different issue for this, but there are several things I found out while researching this that needs fixing.

Why do we not always set configurable: true?
Looking at the git log I found #1456 and #1462 which basically changed true to use false if that attribute was already set to false. Why does that make a difference? Well, as long as the prop is writable Object.defineProperty will only throw if configurable is false AND any of the other fields change their value. So we can do Object.defineProperty(obj, prop, {value:42}) on a non-configurable prop, as the other fields will be left alone.

Why do we tread enumerable differently than configurable?
Given the above, reasoning, why do we always set enumerable to true? Should it now be subjected to the same handling as configurable? Yes it should have been treated in the same manner and in fact, trying to do stub.value(42) on a prop that is neither configurable nor enumerable will throw in Sinon today. As should be apparent from the above section, this does not need to happen! We only need to keep properties the same, if they exist, otherwise, use sensible (permissive) default values.

Given that, I think we need to create a separate issue to cover that. A basic fix is to make our own defineProps that does this:

var getObjectDescriptor = require('./get-object-descriptor');
var defaultPropValues = Object.freeze({configurable: true, enumerable: true, writable: true});

// reuse existing values, if they exist
function defineProperty(obj, prop, newAttributes){
    var originalDescriptor = getObjectDescriptor(obj, prop);
    var descriptor = Object.assign({}, defaultPropValues, originalDescriptor || {}, newAttributes);
    Object.defineProperty(object, prop, descriptor);
}

The reason we need to merge in defaultProps is that all of the listed ones are false by default when using Object.defineProperty while we usually want them to be true (like they are in normal assignment operations).

How to verify - mandatory

The original issue had a small code listing that would blow up when run. It works now:

$ node test.js
0
1
0

$ cat test.js
const sinon = require("./lib/sinon");
const mongoose = require("mongoose");

console.log(mongoose.connection.readyState);

sinon.stub(mongoose.connection, "readyState").value(1);

console.log(mongoose.connection.readyState);

sinon.restore();

console.log(mongoose.connection.readyState);

Checklist for author

• npm run lint passes
• References to standard library functions are cached.

[mroderick]

A very thorough investigation and a very easy to follow pull request (considering the complexity) 🥇 💯

Would you mind folding the first commit into the one that moves the tests around, so we don't have any stray ES6 code in a commit?

[mroderick]

Suggested change

	referee.assert.isUndefined(ownPropertyDescriptor2)
	assert.isUndefined(ownPropertyDescriptor2)

[mroderick]

I don't think this needs a check for object. Without object, the function won't work anyway.

If anything, the function should throw a TypeError when object is falsy.

Considering the name isOwn, I would expect this to be a boolean value. In its current form, it will be an object (a property descriptor).

Suggested change

	var isOwn = object && Object.getOwnPropertyDescriptor(object, property);
	var isOwn = Boolean(Object.getOwnPropertyDescriptor(object, property));

[fatso83]

You need object. Without it all tests crashed. The loop below takes it into consideration, but I have to do it too.

The other I can fix.

[fatso83]

I seem to have been momentarily high. What you say make sense. I just remember having it fail. I will inspect again.

[fatso83]

Looked again, after seeing (again) it failing when removing object: it is indeed needed. The loop never starts when it is undefined, protected by proto being undefined and returns undefined in that case.

[mantoni]

Very nice indeed! Thank you for putting so much thought and time into this 🙏

[mroderick]

sinon@9.0.1