fix: issue with array methods that re-assign indexes

[ahmedkandel]

see #44 for details

[salesforce-cla]

Thanks for the contribution! Before we can merge this, we need @ahmedkandel to sign the Salesforce.com Contributor License Agreement.

[caridy]

Ok, this change does makes sense, but I think it should be a lot more generic. Let me loop more folks into this because IIRC there are some history around this. /cc @ravijayaramappa @davidturissini

I believe defineProperty is doing the right thing by unwrapping the value in the descriptor when setting it into the original target. The set trap should do the same all the time by just simply unwrap the value before setting that up. The question is, what are the implications of such generalization.

value = unwrapProxy(value);
if (oldValue !== value) {
    originalTarget[key] = value;
    valueMutated(originalTarget, key);
} else {...

[ahmedkandel]

unwrapping the value all the time will cause some issues:

1. It will allow injecting read-only proxy to an array to make it read-write:

const membrane = new ObservableMembrane();

const o = { foo: 'bar' };

const readOnly = membrane.getReadOnlyProxy(o);
const writeAndRead = membrane.getProxy({baz: []});

writeAndRead.baz.push(readOnly);
writeAndRead.baz[0].foo = 'qux';

writeAndRead.baz[0].foo will return 'qux' not 'bar'

2. Pushing a proxy to the array will not be allowed and will get unwrapped:

const membrane = new ObservableMembrane();

const o = {
    x: 2,
    y: [
        {z:1}
    ]
};

const p = membrane.getProxy(o);

p.y.push(membrane.getProxy({z: 2}));

p.y[1] will yield an object but not a proxy

3. If the original value of an array item is already a proxy for some use-cases the unwrapping will transform the original value when doing (shift, unshift, ...).

That's why I had to narrow the scope to array items re-assignment. Also by checking originalTarget.includes(unwrapProxy(value)) we make sure we conserve a proxy value if it does exist.

[caridy]

@ahmedkandel thanks for bringing this up, now I remember a lot more about this. Ok, let me try to articulate my position on this:

1. I don't think this is an array specific issue, I think it is a fundamental issue, it also affects objects of those objects have functions that carry mutation similars to those done by the array intrinsics.

2. yes, readOnly proxies have particular semantics that have to be preserved, should not be that hard IMO.

Now, let me state the problem, the way I see it:

Lets use a code of colors, which I find easier to understand, in this library we have two colors: yellow and blue:

• readOnly proxies and readAndWrite proxies should be considered yellow objects and arrays controlled by this library.
• any other object in general (object, functions, arrays, etc.) should be consider blue.
• yellow objects can be readOnly (RO) or readAndWrite (RW).

Operations to control by this membrane for RW:

a. Reflect.set(yellowRWObject, key, blueValue) as a result, a blueValue should be set into the target of the yellowRWObject (that target is another blueValue). (no unwrapping needed)
b. Reflect.set(yellowRWObject, key, yellowROValue) as a result, a yellowROValue should be set into the target of the yellowRWObject (that target is another blueValue). I call this a leaking escape hatch, which is a necessary evil to avoid making a RO value a RW value by all means. (no unwrapping needed)
c. Reflect.set(yellowRWObject, key, yellowRWValue) as a result, the target of the yellowRWValue should be set into the target of the yellowRWObject (that target is another blueValue). (unwrapping needed)

Operations to control by this membrane for RO Objects are not that important because they just throw for all cases.

What all this tell us, is that the only condition that needs to be added is for the case where a RO value is about to be set into a RW value as a property, in which case no unwrapping should be done (case b above), while for the rest, should require unwrapping attempts.

In terms of the code, let me adjust my proposal:

value = unwrapProxyOnlyIfReadAndWriteProxyIsGiven(value);
if (oldValue !== value) {
    originalTarget[key] = value;
    valueMutated(originalTarget, key);
} else {...

We can either create that new method (with a better name), or reuse something else. This will also have to be changed in unwrapDescriptor method, which is used by defineProperty trap to accommodate that as well since it is faulty as well today.

[ahmedkandel]

Exactly @caridy, I was thinking about this implementation with a little change in (case c above).

When setting yellowRWValue, maybe the user has the intention to set a proxy yellowValue, not blueVlaue described in my previous comment (case 2 above). While I think it is rare and we can clarify in the documentation that it is not allowed to set() proxy values to a property as it will get unwarped. Also because get() will always return a proxy whenever the value is observable "membrane is applicable to an entire object graph" so no need for the user to do it by himself.

In this case, generalizing value unwrapping in ReactiveProxyHandler . set() and defineProperty() traps except for read-only proxy values yellowROValue is a better fix.

We can add a new method to ReactiveMembrane maybe we name it safeUnwrapValue that return unwrappedValue if not a yellowROValue.

Another approach is to add a condition to ReadOnlyHandler . get() trap

if (key === isReadOnlyProxy) {
    return true;
}

Then we can check it later if (value[isReadOnlyProxy]) ...

I think the new method approach is better. I will commit it so we could start discussing it.

[ahmedkandel]

I think we have another issue Not directly related to this fix but we can try to fix it too:

currently, the membrane unwrapProxy() method does not check if the proxy is read-only or not so it could be used to leak the proxy and make it read-write as in the following example:

const membrane = new ObservableMembrane();

const readOnly = membrane.getReadOnlyProxy({ foo: 'bar' });
const writeAndRead = membrane.getProxy({});

writeAndRead.x = membrane.unwrapProxy(readOnly);
writeAndRead.x.foo = 'baz';

So we can change the way unwrapProxy() method is working to check if the proxy is read-only and return the proxy instead of the original target. then we can use unwrapProxy() method in set() and defineProperty() traps, This will fix both issues.

@caridy what do you think?

[ahmedkandel]

@caridy /cc @ravijayaramappa @davidturissini, Can you please update if this implementation is ok?

Thanks.

[caridy]

I think we have another issue Not directly related to this fix but we can try to fix it too:

currently, the membrane unwrapProxy() method does not check if the proxy is read-only or not so it could be used to leak the proxy and make it read-write as in the following example:

const membrane = new ObservableMembrane();

const readOnly = membrane.getReadOnlyProxy({ foo: 'bar' });
const writeAndRead = membrane.getProxy({});

writeAndRead.x = membrane.unwrapProxy(readOnly);
writeAndRead.x.foo = 'baz';

So we can change the way unwrapProxy() method is working to check if the proxy is read-only and return the proxy instead of the original target. then we can use unwrapProxy() method in set() and defineProperty() traps, This will fix both issues.

@caridy what do you think?

I'm missing something here @ahmedkandel. Btw, apologies for the delay... trying to clean things up this week. The only way you can unwrap a proxy is if you give them access to such functionality. In, LWC for example, only the core functionality of the platform has access to unwrap, the rest, component authors and such do not have access to it, they don't even have access to the membrane instance itself. Anyhow, feel free to open a separate issue to discuss this in details.

[caridy]

As for this PR specifically, I believe it was fixed by PR #49, @ahmedkandel can you double check and close this one?