Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible solution to override mistake without a new integrity trait #4

Open
jridgewell opened this issue Dec 3, 2024 · 9 comments
Open

Comments

@jridgewell
Copy link
Member

jridgewell commented Dec 3, 2024

Based on the bug found in tc39/ecma262#1320 (comment):

  1. If a non-writable prototype property exists during o.foo = v
    1. Define a new property on o with { value: v, enumerable: true, writable: true, configurable: true } (like it's defining a new property without extending the parent)
    2. Note: It must be configurable to get past the delete statement when running getRawTag(new Uint8Array(0)).
  2. Extend Object.prototype.toString() special cases with typed arrays, DataView, ArrayBuffer, Map, WeakMap, Set, WeakSet, Promise, etc.
    1. Note: With the ability to write value[Symbol.toStringTag] = undefined, we must fix various builtins to still return the expected [object Foo] values.
    2. Note: This fixes lodash's isTypedArray, isArrayBuffer, etc, functions directly, and the getTag function indirectly by ensuring a few feature checks don't fail and cause a incomplete impl of getTag to be used.
@mhofman
Copy link
Member

mhofman commented Dec 3, 2024

To clarify, this is unilaterally fixing the override mistake, just with mitigations for some observed in-the-wild reliance of the override mistake currently silently failing in sloppy mode?

@jridgewell
Copy link
Member Author

jridgewell commented Dec 3, 2024

To clarify, this is unilaterally fixing the override mistake

Correct.

just with mitigations for some observed in-the-wild reliance of the override mistake

Correct.

currently silently failing in sloppy mode?

Incorrect. This fixes an assumption made in both sloppy and strict modes.

  • In sloppy, the value[Symbol.toStringTag] = undefined would silently fail, and toString.call(new Uint8Array(0)) would return [object Uint8Array] from the string tag.
  • In strict, value[Symbol.toStringTag] = undefined would throw and be swallowed by the surrounding try-catch. This would still return the [object Uint8Array] from the string tag.

With the new override behavior, this code would change. Since the new built-ins don't have special behavior in Object.prototype.toString, they would begin to return [object Object] when their own toStringTag is undefined.

@Jamesernator
Copy link

Jamesernator commented Dec 6, 2024

If the override mistake can be fixed unilaterally that is ideal, but if it's not web compatible, we can still fix it without changing the object model I've suggested in the past.

To summarize and simplify that suggestion, we would just have a new slot on ordinary objects, have a way to freeze and set that slot to true, and then OrdinarySetWithOwnDescriptor just performs the override mistake fix if that slot is set to true.

I called it harden, but it could just be an argument to freeze:

const o = Object.freeze({ x: 10 }, true);
// o is an object { [[AllowOverride]]: true, x: 10 }
const o2 = { __proto__: o };
// o has slot [[AllowOverride]] so the override mistake is disabled.
o2.x = 12;

As this doesn't add any proxy traps it should be very simple to implement (just a bit on ordinary objects so it's extremely lightweight).

@mhofman
Copy link
Member

mhofman commented Dec 6, 2024

@Jamesernator adding a new slot is effectively the equivalent of the overidable integrity trait we're suggesting. Why do you believe it wouldn't require a proxy trap?

@Jamesernator
Copy link

Jamesernator commented Dec 6, 2024

Why do you believe it wouldn't require a proxy trap?

Because it adds no more power than if ordinary objects just had a private field, it would essentially just change the current check:

a. If ownDesc.[[Writable]] is false, return false.

to:

a. If ownDesc.[[Writable]] is false
  a. If O does not have an [[AllowOverride]] slot or o.[[AllowOveride]] is false, return false.

Freezing would essentially be:

function freezeWithOverride(o) {
    // Prevent a communication channel if the object is already frozen
    if (Object.isFrozen(o)) {
        return;
    }
    if (#allowOverride in o) {
        o.#allowOverride = true;
    }
    Object.freeze(o);
}

@mhofman
Copy link
Member

mhofman commented Dec 9, 2024

The problem is that having no traps prevents a proxy the opportunity to reflect the state of the real target on its shadow target.

Imagine the following simplified pattern that is common in membranes:

const shadowToReal= new WeakMap();
const realToProxy = new WeakMap();

// Traps of the handler lookup the real target from the shadow in shadowToReal
// and reflect the shape of the real target onto the shadow as necessary to respect
// object invariants and according to any distortion that the membrane wants to apply.
const membraneHandler = {...};

const getProxyForTarget = (target) => {
  let proxy = realToProxy.get(target);
  if (proxy) return proxy;
  // Approximation of creating a bare shadow object of the same "kind"
  const shadow = typeof target === 'function' ?
    function() {} :
    Array.isArray(target) ?
      [] :
      { __proto__: null };
  shadowToReal.set(shadow, target);
  proxy = new Proxy(shadow, membraneHandler);
  realToProxy.set(target, proxy);
  return proxy;
};

Without "is overridable" and "make overridable" proxy traps, the following can't work:

const someTarget = {};
const proxy = getProxyForTarget(someTarget);
freezeWithOverride(someTarget);
stampPrivateField(proxy); // This should fail

const someOtherTarget = {};
const otherProxy = getProxyForTarget(someOtherTarget);
freezeWithOverride(otherProxy); // If distortions allow
stampPrivateField(someOtherTarget); // Should now fail

@Jamesernator
Copy link

Jamesernator commented Dec 13, 2024

Without "is overridable" and "make overridable" proxy traps, the following can't work:
stampPrivateField(proxy); // This should fail

I wasn't suggesting that the trap for preventing private field stamping can be removed. That is separate as it is actually a fundamental capability for objects. Unlike the override mistake, there is no proxy you can write to emulate such behaviour.

Though yes freezeWithOverride wouldn't pass through proxies in my suggestion, but I don't see this as any different from other builtin methods (e.g. Map.prototype.get) methods not passing through proxies either. The solution for making freezeWithOverride work through the membrane is the same as any other slot-checking builtin functions — you expose it through the membrane.

@mhofman
Copy link
Member

mhofman commented Dec 13, 2024

Ugh sorry I got my overrides mixed up. But I believe the same argument holds for the assign override mistake. I'll follow up with some examples.

@mhofman
Copy link
Member

mhofman commented Dec 27, 2024

Example:

const someTarget = { foo: 3 };
const proxy = getProxyForTarget(someTarget);
freezeWithOverride(someTarget);
const someDerivedObject = { __proto__: proxy };
someDerivedObject.foo = 4; // This should work if distortions allow

const someOtherTarget = { foo: 3};
const otherProxy = getProxyForTarget(someOtherTarget);
freezeWithOverride(otherProxy); // If distortions allow
const someOtherDerivedObject = { __proto__: someOtherTarget };
someOtherDerivedObject.foo = 4; // This should work

Though yes freezeWithOverride wouldn't pass through proxies in my suggestion, but I don't see this as any different from other builtin methods (e.g. Map.prototype.get) methods not passing through proxies either. The solution for making freezeWithOverride work through the membrane is the same as any other slot-checking builtin functions — you expose it through the membrane.

I think this is where I disagree. Allowing assignment override is an intrinsic aspect of the object (as denoted by the private slot you're adding to described that aspect). I don't see how you would "expose it through the membrane" without making it part of the MOP traps a proxy handler implement. Assignment does not currently trigger user code except for proxy traps, and we should not expand that unfortunate exception. Built-in functions do not do any slot checking on arguments, only on receivers, which is why membranes have to make sure to properly map the receiver when reflecting to the real target. The only intrinsic that brand checks its argument is Array.isArray() and that has special proxy logic that no-one wants to replicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants