fix: issue with array methods that re-assign indexes

[ahmedkandel]

see #44 for details

[salesforce-cla[bot]]

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

[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?