ajvincent / es-membrane

An ECMAScript implementation of a Membrane, allowing users to dynamically hide, override, or extend objects in JavaScript with controlled effects on the original objects.
ISC License
109 stars 13 forks source link

ModifyRulesAPI.prototype.filterOwnKeys's filter cannot handle missing ProxyMapping correctly #165

Closed ajvincent closed 6 years ago

ajvincent commented 6 years ago

In commit 0755e953 for the dogfood-0.9 branch, I added the following:

        if (!pMapping)
          return true;

This resulted in Membrane.prototype.buildMapping being exposed - which violates a private API constraint of the dogfood membrane. But when I reversed that to return false, revokeEverything() in ObjectGraphHandler.prototype no longer works: looking up properties of a revoked object graph works when it shouldn't.

Removing this bailout fails a bunch of asserts... and then fails the revokeEverything() tests.

The ownKeys filter must return true or false definitively. We hit this failure because in the dogfood membrane, we do not wrap the entire prototype chain of a membrane proxy. Specifically, we don't create ProxyMapping objects for Function.prototype or Object.prototype.

This is a serious design flaw.

ajvincent commented 6 years ago

Very specifically, this cannot be fixed in just the dogfood membrane: it can happen in a real-world configuration, particularly with the passthrough functions.

ajvincent commented 6 years ago

So in pseudocode, here's the problem:

function a() {}
function b() {}
b.prototype = new a();
b.prototype.__hidden__ = 1;

const M = new Membrane({
  passThroughFilter: function(obj) {
    return obj == a.prototype;
  });
const wetHandler = M.getHandlerByName("wet", {mustCreate: true});
const dryHandler = M.getHandlerByName("dry", {mustCreate: true});

// this function doesn't exist... remember, this is pseudo-code
wetHandler.setKeysFilter(function(value) {
  const keys = Reflect.ownKeys(value);
  if (value == b.prototype)
    keys.splice(keys.indexOf("__hidden__"), 1);
  return keys;
});

const B = M.convertArgumentToProxy(wetHandler, dryHandler, b);
const B_instance = new B();
// should B_instance.__hidden__ exist?

If the filter rejects at b.prototype, that doesn't say whether or not the filter at a.prototype should accept or reject. Now, I could just stop the propagation at b.prototype... but then, what about methods like Object.prototype.toString()? We really don't want to wrap Object.prototype in the membrane, especially not in the dogfood membrane.

ajvincent commented 6 years ago

After a little break, I think this is fundamentally a problem I caused by confusing "own keys" with "properties accessible by getter", in the filterOwnKeys code.

https://github.com/ajvincent/es-membrane/blob/f2649e85a74328c92b9533dcd8757f2a4416c048/source/ModifyRulesAPI.js#L317-L365

Issue #94 was just wrong. Filtering by an array of strings, a Set of strings, or a function is fine. Insisting that the prototype chain can alter the results of filterOwnKeys is not.

ajvincent commented 6 years ago

Fixed.