Object.freezing proxies should freeze or throw?

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Object.freezing proxies should freeze or throw?

Raul-Sebastian Mihăilă
Initially posted on github (https://github.com/tc39/ecma262/issues/652), but I think the mailing list is more appropriate.

Usually methods on Object that change some internal state of the objects/properties of the objects either succeed or throw. However, Object.freeze can fail to freeze proxies without throwing:

```js
      const target = Object.seal({x: 2});
      const proxy = new Proxy(target, {
        defineProperty() {
          return true;
        },

        set(target, prop, value) {
          target[prop] = value;
        }
      });

      Object.freeze(proxy);
      console.log(proxy.x); // 2
      proxy.x = 100;
      console.log(proxy.x); // 100
```

Should this be allowed?

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Tom Van Cutsem-3
Good catch. This appears to be a genuine spec bug.

First, I thought that the problem simply was due to sloppy-mode silent failures, as a call to Object.isFrozen revealed that the proxy is not actually frozen after the call to Object.freeze. When your script is run in strict mode, it fails:

on Chrome: Uncaught TypeError: 'set' on proxy: trap returned falsish for property 'x'
on Firefox: TypeError: proxy set handler returned false for property '"x"'

However, this error can easily be avoided by adding a `return true` to the set trap. At this point, your example still runs in strict mode, which worried me.

The culprit is not that the proxy's "x" property can still be updated, but rather that Object.freeze(proxy) doesn't actually freeze the proxy yet still returns without throwing. Thus, a shorter testcase of the problem is:

      "use strict";
      const target = Object.seal({x: 2});
      const proxy = new Proxy(target, {
        defineProperty() {
          return true;
        }
      });
      Object.freeze(proxy);
      console.log(Object.isFrozen(proxy)); // expected true, but is false

Tracing through the ES2015 spec, the spec 'call stack' is roughly:

19.1.2.5 Object.freeze ( O )
calls 7.3.14 SetIntegrityLevel (O, level = "frozen")
 calls 7.3.7 DefinePropertyOrThrow (O, P = "x", desc = {configurable: false, writable: false})
  calls 9.5.6 [[DefineOwnProperty]] (P, Desc)
   calls 9.1.6.2 IsCompatiblePropertyDescriptor (Extensible, Desc, Current)
    calls 9.1.6.3 ValidateAndApplyPropertyDescriptor (O, P, extensible, Desc, current)
     where O = undefined, P = undefined, extensible = false,
                Desc = {configurable: false, writable: false},
                current = { value: 2, writable: true, enumerable: true, configurable: false }

The ValidateAndApplyPropertyDescriptor, when called via IsCompatiblePropertyDescriptor, essentially checks whether it is legal to update an existing property descriptor `current` to a new state described by `Desc`, without actually performing the update.

Tracing through the details of that algorithm, it turns out the above property descriptors are indeed compatible. It is legal to update a data property {writable: true, configurable: false} to {writable: false, configurable: false}. IIRC, this is somewhat of an exception as it is the only state transition allowed on a non-configurable data property.

Because IsCompatiblePropertyDescriptor returns true, the [[DefineOwnProperty]] call on the proxy completes successfully, signaling to SetIntegrityLevel that the property appears to be successfully updated to {configurable: false, writable: false} (even though it didn't in this case, as the proxy's "defineProperty" trap simply returned `true` without actually performing the update).

The quick fix appears to be to change the SetIntegrityLevel algorithm as follows, by updating step 10:

10. If O is a Proxy, return TestIntegrityLevel(O, "frozen"), otherwise return true.

With this change, the call to SetIntegrityLevel ends with a call to TestIntegrityLevel, which will notice that O is in fact not frozen and thus cause the call to Object.freeze to fail.

To summarize: the error here is not with the proxy invariant checks (the proxy is still unfrozen), but rather that because of the exceptional allowed state transfer from writable,non-configurable to non-writable,non-configurable, SetIntegrityLevel fails to detect that the proxy object being frozen in fact remains unfrozen.

Cheers,
Tom

2016-08-06 21:09 GMT+02:00 Raul-Sebastian Mihăilă <[hidden email]>:
Initially posted on github (https://github.com/tc39/ecma262/issues/652), but I think the mailing list is more appropriate.

Usually methods on Object that change some internal state of the objects/properties of the objects either succeed or throw. However, Object.freeze can fail to freeze proxies without throwing:

```js
      const target = Object.seal({x: 2});
      const proxy = new Proxy(target, {
        defineProperty() {
          return true;
        },

        set(target, prop, value) {
          target[prop] = value;
        }
      });

      Object.freeze(proxy);
      console.log(proxy.x); // 2
      proxy.x = 100;
      console.log(proxy.x); // 100
```

Should this be allowed?

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss



_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Raul-Sebastian Mihăilă
Would be a good idea to do the test for any kind of exotic objects (including host defined ones), not only for proxies?

On Sun, Aug 7, 2016 at 4:33 PM, Tom Van Cutsem <[hidden email]> wrote:
Good catch. This appears to be a genuine spec bug.

First, I thought that the problem simply was due to sloppy-mode silent failures, as a call to Object.isFrozen revealed that the proxy is not actually frozen after the call to Object.freeze. When your script is run in strict mode, it fails:

on Chrome: Uncaught TypeError: 'set' on proxy: trap returned falsish for property 'x'
on Firefox: TypeError: proxy set handler returned false for property '"x"'

However, this error can easily be avoided by adding a `return true` to the set trap. At this point, your example still runs in strict mode, which worried me.

The culprit is not that the proxy's "x" property can still be updated, but rather that Object.freeze(proxy) doesn't actually freeze the proxy yet still returns without throwing. Thus, a shorter testcase of the problem is:

      "use strict";
      const target = Object.seal({x: 2});
      const proxy = new Proxy(target, {
        defineProperty() {
          return true;
        }
      });
      Object.freeze(proxy);
      console.log(Object.isFrozen(proxy)); // expected true, but is false

Tracing through the ES2015 spec, the spec 'call stack' is roughly:

19.1.2.5 Object.freeze ( O )
calls 7.3.14 SetIntegrityLevel (O, level = "frozen")
 calls 7.3.7 DefinePropertyOrThrow (O, P = "x", desc = {configurable: false, writable: false})
  calls 9.5.6 [[DefineOwnProperty]] (P, Desc)
   calls 9.1.6.2 IsCompatiblePropertyDescriptor (Extensible, Desc, Current)
    calls 9.1.6.3 ValidateAndApplyPropertyDescriptor (O, P, extensible, Desc, current)
     where O = undefined, P = undefined, extensible = false,
                Desc = {configurable: false, writable: false},
                current = { value: 2, writable: true, enumerable: true, configurable: false }

The ValidateAndApplyPropertyDescriptor, when called via IsCompatiblePropertyDescriptor, essentially checks whether it is legal to update an existing property descriptor `current` to a new state described by `Desc`, without actually performing the update.

Tracing through the details of that algorithm, it turns out the above property descriptors are indeed compatible. It is legal to update a data property {writable: true, configurable: false} to {writable: false, configurable: false}. IIRC, this is somewhat of an exception as it is the only state transition allowed on a non-configurable data property.

Because IsCompatiblePropertyDescriptor returns true, the [[DefineOwnProperty]] call on the proxy completes successfully, signaling to SetIntegrityLevel that the property appears to be successfully updated to {configurable: false, writable: false} (even though it didn't in this case, as the proxy's "defineProperty" trap simply returned `true` without actually performing the update).

The quick fix appears to be to change the SetIntegrityLevel algorithm as follows, by updating step 10:

10. If O is a Proxy, return TestIntegrityLevel(O, "frozen"), otherwise return true.

With this change, the call to SetIntegrityLevel ends with a call to TestIntegrityLevel, which will notice that O is in fact not frozen and thus cause the call to Object.freeze to fail.

To summarize: the error here is not with the proxy invariant checks (the proxy is still unfrozen), but rather that because of the exceptional allowed state transfer from writable,non-configurable to non-writable,non-configurable, SetIntegrityLevel fails to detect that the proxy object being frozen in fact remains unfrozen.

Cheers,
Tom

2016-08-06 21:09 GMT+02:00 Raul-Sebastian Mihăilă <[hidden email]>:
Initially posted on github (https://github.com/tc39/ecma262/issues/652), but I think the mailing list is more appropriate.

Usually methods on Object that change some internal state of the objects/properties of the objects either succeed or throw. However, Object.freeze can fail to freeze proxies without throwing:

```js
      const target = Object.seal({x: 2});
      const proxy = new Proxy(target, {
        defineProperty() {
          return true;
        },

        set(target, prop, value) {
          target[prop] = value;
        }
      });

      Object.freeze(proxy);
      console.log(proxy.x); // 2
      proxy.x = 100;
      console.log(proxy.x); // 100
```

Should this be allowed?

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss




_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Mark S. Miller-2
Hi Raul, yes it would. The invariants apply to all objects.

If someone would like to write up the spec bug and fix, I'll happily represent and advance it at upcoming meetings.


On Sun, Aug 7, 2016 at 6:49 AM, Raul-Sebastian Mihăilă <[hidden email]> wrote:
Would be a good idea to do the test for any kind of exotic objects (including host defined ones), not only for proxies?

On Sun, Aug 7, 2016 at 4:33 PM, Tom Van Cutsem <[hidden email]> wrote:
Good catch. This appears to be a genuine spec bug.

First, I thought that the problem simply was due to sloppy-mode silent failures, as a call to Object.isFrozen revealed that the proxy is not actually frozen after the call to Object.freeze. When your script is run in strict mode, it fails:

on Chrome: Uncaught TypeError: 'set' on proxy: trap returned falsish for property 'x'
on Firefox: TypeError: proxy set handler returned false for property '"x"'

However, this error can easily be avoided by adding a `return true` to the set trap. At this point, your example still runs in strict mode, which worried me.

The culprit is not that the proxy's "x" property can still be updated, but rather that Object.freeze(proxy) doesn't actually freeze the proxy yet still returns without throwing. Thus, a shorter testcase of the problem is:

      "use strict";
      const target = Object.seal({x: 2});
      const proxy = new Proxy(target, {
        defineProperty() {
          return true;
        }
      });
      Object.freeze(proxy);
      console.log(Object.isFrozen(proxy)); // expected true, but is false

Tracing through the ES2015 spec, the spec 'call stack' is roughly:

19.1.2.5 Object.freeze ( O )
calls 7.3.14 SetIntegrityLevel (O, level = "frozen")
 calls 7.3.7 DefinePropertyOrThrow (O, P = "x", desc = {configurable: false, writable: false})
  calls 9.5.6 [[DefineOwnProperty]] (P, Desc)
   calls 9.1.6.2 IsCompatiblePropertyDescriptor (Extensible, Desc, Current)
    calls 9.1.6.3 ValidateAndApplyPropertyDescriptor (O, P, extensible, Desc, current)
     where O = undefined, P = undefined, extensible = false,
                Desc = {configurable: false, writable: false},
                current = { value: 2, writable: true, enumerable: true, configurable: false }

The ValidateAndApplyPropertyDescriptor, when called via IsCompatiblePropertyDescriptor, essentially checks whether it is legal to update an existing property descriptor `current` to a new state described by `Desc`, without actually performing the update.

Tracing through the details of that algorithm, it turns out the above property descriptors are indeed compatible. It is legal to update a data property {writable: true, configurable: false} to {writable: false, configurable: false}. IIRC, this is somewhat of an exception as it is the only state transition allowed on a non-configurable data property.

Because IsCompatiblePropertyDescriptor returns true, the [[DefineOwnProperty]] call on the proxy completes successfully, signaling to SetIntegrityLevel that the property appears to be successfully updated to {configurable: false, writable: false} (even though it didn't in this case, as the proxy's "defineProperty" trap simply returned `true` without actually performing the update).

The quick fix appears to be to change the SetIntegrityLevel algorithm as follows, by updating step 10:

10. If O is a Proxy, return TestIntegrityLevel(O, "frozen"), otherwise return true.

With this change, the call to SetIntegrityLevel ends with a call to TestIntegrityLevel, which will notice that O is in fact not frozen and thus cause the call to Object.freeze to fail.

To summarize: the error here is not with the proxy invariant checks (the proxy is still unfrozen), but rather that because of the exceptional allowed state transfer from writable,non-configurable to non-writable,non-configurable, SetIntegrityLevel fails to detect that the proxy object being frozen in fact remains unfrozen.

Cheers,
Tom

2016-08-06 21:09 GMT+02:00 Raul-Sebastian Mihăilă <[hidden email]>:
Initially posted on github (https://github.com/tc39/ecma262/issues/652), but I think the mailing list is more appropriate.

Usually methods on Object that change some internal state of the objects/properties of the objects either succeed or throw. However, Object.freeze can fail to freeze proxies without throwing:

```js
      const target = Object.seal({x: 2});
      const proxy = new Proxy(target, {
        defineProperty() {
          return true;
        },

        set(target, prop, value) {
          target[prop] = value;
        }
      });

      Object.freeze(proxy);
      console.log(proxy.x); // 2
      proxy.x = 100;
      console.log(proxy.x); // 100
```

Should this be allowed?

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss




_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss




--
    Cheers,
    --MarkM

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Allen Wirfs-Brock
In reply to this post by Tom Van Cutsem-3
Tom,

I.m not sure that I agree with your conclusion and your fix.  I agree that there doesn’t appear to be a bug with the implementation of any of the currently specified proxy  invariant checks and hence the problem is internal to Object.freeze.  But Object.freeze is not a “MOP level” operation.  If is just a function, just like any function that a ES programmer might write, that manipulates an object using the MOP and assumes that the objet essential invariants are maintained.  Testing an object to see if it is a Proxy is not an operation that is available to ES code via the MOP.  If we respecify Object.freeze to make such a test we are using special powers that would not be available to an ES programer who wanted to build a similar operation.

I think the real problem is actually is with the invariants.  Note that [[Set]] has two invariants that are predicated by the "previously observed” state of a property’s configurable and writable attributes.  Now “observed” has always been a trickle concept to translate into the specification of the actual Proxy internal method. Rather than keeping track of what has been observed we tried to ensure that inconsistent states are unobservable. 

In the case of Proxy [[DefineOwnProperty]], it seems to me that returning true on a call that sets a property to non-configurable or non-configurable+non-writable is a form of observation. If the corresponding target property has not been set by the handler to the requested value, then [[DefineOwnProperty]] should not be allowed to return true. The problem with the current spec. is that before allowing true to be returned it only checks that the requested value of configurable has been set in the target. It seems to me that if the target property is non-configurable, then it should also ensure that a request to set writable to false has set actually set writable to false on the target property.  If not, it should throw.

Allen   




On Aug 7, 2016, at 6:33 AM, Tom Van Cutsem <[hidden email]> wrote:

Good catch. This appears to be a genuine spec bug.

First, I thought that the problem simply was due to sloppy-mode silent failures, as a call to Object.isFrozen revealed that the proxy is not actually frozen after the call to Object.freeze. When your script is run in strict mode, it fails:

on Chrome: Uncaught TypeError: 'set' on proxy: trap returned falsish for property 'x'
on Firefox: TypeError: proxy set handler returned false for property '"x"'

However, this error can easily be avoided by adding a `return true` to the set trap. At this point, your example still runs in strict mode, which worried me.

The culprit is not that the proxy's "x" property can still be updated, but rather that Object.freeze(proxy) doesn't actually freeze the proxy yet still returns without throwing. Thus, a shorter testcase of the problem is:

      "use strict";
      const target = Object.seal({x: 2});
      const proxy = new Proxy(target, {
        defineProperty() {
          return true;
        }
      });
      Object.freeze(proxy);
      console.log(Object.isFrozen(proxy)); // expected true, but is false

Tracing through the ES2015 spec, the spec 'call stack' is roughly:

19.1.2.5 Object.freeze ( O )
calls 7.3.14 SetIntegrityLevel (O, level = "frozen")
 calls 7.3.7 DefinePropertyOrThrow (O, P = "x", desc = {configurable: false, writable: false})
  calls 9.5.6 [[DefineOwnProperty]] (P, Desc)
   calls 9.1.6.2 IsCompatiblePropertyDescriptor (Extensible, Desc, Current)
    calls 9.1.6.3 ValidateAndApplyPropertyDescriptor (O, P, extensible, Desc, current)
     where O = undefined, P = undefined, extensible = false,
                Desc = {configurable: false, writable: false},
                current = { value: 2, writable: true, enumerable: true, configurable: false }

The ValidateAndApplyPropertyDescriptor, when called via IsCompatiblePropertyDescriptor, essentially checks whether it is legal to update an existing property descriptor `current` to a new state described by `Desc`, without actually performing the update.

Tracing through the details of that algorithm, it turns out the above property descriptors are indeed compatible. It is legal to update a data property {writable: true, configurable: false} to {writable: false, configurable: false}. IIRC, this is somewhat of an exception as it is the only state transition allowed on a non-configurable data property.

Because IsCompatiblePropertyDescriptor returns true, the [[DefineOwnProperty]] call on the proxy completes successfully, signaling to SetIntegrityLevel that the property appears to be successfully updated to {configurable: false, writable: false} (even though it didn't in this case, as the proxy's "defineProperty" trap simply returned `true` without actually performing the update).

The quick fix appears to be to change the SetIntegrityLevel algorithm as follows, by updating step 10:

10. If O is a Proxy, return TestIntegrityLevel(O, "frozen"), otherwise return true.

With this change, the call to SetIntegrityLevel ends with a call to TestIntegrityLevel, which will notice that O is in fact not frozen and thus cause the call to Object.freeze to fail.

To summarize: the error here is not with the proxy invariant checks (the proxy is still unfrozen), but rather that because of the exceptional allowed state transfer from writable,non-configurable to non-writable,non-configurable, SetIntegrityLevel fails to detect that the proxy object being frozen in fact remains unfrozen.

Cheers,
Tom

2016-08-06 21:09 GMT+02:00 Raul-Sebastian Mihăilă <[hidden email]>:
Initially posted on github (https://github.com/tc39/ecma262/issues/652), but I think the mailing list is more appropriate.

Usually methods on Object that change some internal state of the objects/properties of the objects either succeed or throw. However, Object.freeze can fail to freeze proxies without throwing:

```js
      const target = Object.seal({x: 2});
      const proxy = new Proxy(target, {
        defineProperty() {
          return true;
        },

        set(target, prop, value) {
          target[prop] = value;
        }
      });

      Object.freeze(proxy);
      console.log(proxy.x); // 2
      proxy.x = 100;
      console.log(proxy.x); // 100
```

Should this be allowed?

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss


_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss


_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

RE: Object.freezing proxies should freeze or throw?

doodad-js Admin

Testing an object to see if it is a Proxy is not an operation that is available to ES code via the MOP”

 

By the way, why such a test is not available? I think that everything should be testable, but it’s not the case actually.


_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Claude Pache
In reply to this post by Allen Wirfs-Brock
Here is another test case, with [[GetOwnPropertyDescriptor]]:

```js
var target = Object.seal({x: 2});
var proxy = new Proxy(target, {
    getOwnPropertyDescriptor(o, p) {
        var d = Reflect.getOwnPropertyDescriptor(o, p)
        if (d && 'writable' in d)
            d.writable = false
        return d
    }
});

Object.getOwnPropertyDescriptor(proxy, 'x'); // { value: 2, writable: false, configurable: false }

Object.defineProperty(proxy, 'x', { value: 3 })

Object.getOwnPropertyDescriptor(proxy, 'x'); // { value: 3, writable: false, configurable: false }
```

IMHO, the most robust fix is the following: Both the [[DefineOwnProperty]] and the [[GetOwnPropertyDescriptor]] contain the following check:

* If IsCompatiblePropertyDescriptor(extensibleTarget, resultDesc, targetDesc) is false, throw a TypeError exception.

I think there should be also the following check (except when targetDesc is undefined, in which case another appropriate check is used):

* If IsCompatiblePropertyDescriptor(extensibleTarget, targetDesc, resultDesc) is false, throw a TypeError exception.

That would replace the weaker adhoc check about the [[Configurable]] attribute (search for settingConfigFalse) in those algorithms.

(Alternatively, in order to avoid double checks, define an AreBothWaysCompatiblePropertyDescriptors(extensibleTarget, desc1, desc2) abstract operation.)

—Claude

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Claude Pache

> Le 8 août 2016 à 11:02, Claude Pache <[hidden email]> a écrit :
>
> Here is another test case, with [[GetOwnPropertyDescriptor]]:
>
> ```js
> var target = Object.seal({x: 2});
> var proxy = new Proxy(target, {
>    getOwnPropertyDescriptor(o, p) {
>        var d = Reflect.getOwnPropertyDescriptor(o, p)
>        if (d && 'writable' in d)
>            d.writable = false
>        return d
>    }
> });
>
> Object.getOwnPropertyDescriptor(proxy, 'x'); // { value: 2, writable: false, configurable: false }
>
> Object.defineProperty(proxy, 'x', { value: 3 })
>
> Object.getOwnPropertyDescriptor(proxy, 'x'); // { value: 3, writable: false, configurable: false }
> ```
>
> IMHO, the most robust fix is the following: Both the [[DefineOwnProperty]] and the [[GetOwnPropertyDescriptor]] contain the following check:
>
> * If IsCompatiblePropertyDescriptor(extensibleTarget, resultDesc, targetDesc) is false, throw a TypeError exception.
>
> I think there should be also the following check (except when targetDesc is undefined, in which case another appropriate check is used):
>
> * If IsCompatiblePropertyDescriptor(extensibleTarget, targetDesc, resultDesc) is false, throw a TypeError exception.
>
> That would replace the weaker adhoc check about the [[Configurable]] attribute (search for settingConfigFalse) in those algorithms.
>
> (Alternatively, in order to avoid double checks, define an AreBothWaysCompatiblePropertyDescriptors(extensibleTarget, desc1, desc2) abstract operation.)
>
> —Claude
>

Looking closer, it seems that using IsCompatiblePropertyDescriptor is in fact an overkill, because we probably don’t want the special case of conditionally mutable [[Writable]] attribute for nonconfigurable properties.

That is, I think that the following condition must hold: If either the target has a nonconfigurable property, or the proxy claims to have a nonconfigurable property, then every attribute of the property descriptor claimed by the proxy must be identical to the corresponding attribute of the property descriptor of the target.

—Claude


_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Tom Van Cutsem-3
Claude's additional example is indeed evidence that Object.freeze is not to blame, but rather that the invariant checks of [[GetOwnPropertyDescriptor]] and [[DefineOwnProperty]] are too weak. The culprit is, as far as I can tell, that we re-used the state transitions allowed by DefineOwnProperty, which allows the transition from non-configurable,writable (-c+w) to non-configurable,non-writable (-c-w) and which is unwanted here, as Claude rightfully concludes [to better understand the state transitions involved, see MarkM's lucid state transition chart of property attributes <http://wiki.ecmascript.org/doku.php?id=es3.1:attribute_states>. I wish this diagram were in the spec btw, once you know how to read it, it is significantly easier to understand than the DefineOwnProperty algorithm itself]

I like the simplicity of Claude's suggestion, but I think that check is too tight. Technically if the descriptors are both -c+w data descriptors, their value attributes need not be identical to honor the spec invariants. Instead I would propose to tackle the problem head-on and explicitly disallow a proxy from reporting a -c-w property if the corresponding target property is -c+w. We already have a similar check in place in precisely those two algorithms to test if the configurable attribute matches. It is just a matter of tightening that check to also verify the writable attribute.

This would entail the following changes to the ES2015 spec (new or modified text in bold):


9.5.5 [[GetOwnProperty]] (P)
  ...
  22. If resultDesc.[[Configurable]] is false, then
    a. If targetDesc is undefined or targetDesc.[[Configurable]] is true, then
      i. Throw a TypeError exception.
    b. If resultDesc.[[Writable]] is false and targetDesc.[[Writable]] is true, then
      i. Throw a TypeError exception.
      
NOTE [[GetOwnProperty]] for proxy objects enforces the following invariants:

  ...
  * A property cannot be reported as non-configurable, non-writable if it exists as a non-configurable, writable own property on the target object.


9.5.6 [[DefineOwnProperty]] (P, Desc)
  ...
  20. Else targetDesc is not undefined,
    a.  If IsCompatiblePropertyDescriptor(extensibleTarget, Desc , targetDesc) is false, throw a TypeError exception.
    b. If settingConfigFalse is true
      i. If targetDesc.[[Configurable]] is true, throw a TypeError exception.
      ii. If Desc.[[Writable]] is false and targetDesc.[[Writable]] is true, throw a TypeError exception.
      
NOTE [[DefineOwnProperty]] for proxy objects enforces the following invariants:

  ...
  * A property cannot be successfully set to non-configurable, non-writable if the corresponding own property of the target object is non-configurable, writable.

WDYT?

Regards,
Tom

2016-08-08 15:37 GMT+02:00 Claude Pache <[hidden email]>:

> Le 8 août 2016 à 11:02, Claude Pache <[hidden email]> a écrit :
>
> Here is another test case, with [[GetOwnPropertyDescriptor]]:
>
> ```js
> var target = Object.seal({x: 2});
> var proxy = new Proxy(target, {
>    getOwnPropertyDescriptor(o, p) {
>        var d = Reflect.getOwnPropertyDescriptor(o, p)
>        if (d && 'writable' in d)
>            d.writable = false
>        return d
>    }
> });
>
> Object.getOwnPropertyDescriptor(proxy, 'x'); // { value: 2, writable: false, configurable: false }
>
> Object.defineProperty(proxy, 'x', { value: 3 })
>
> Object.getOwnPropertyDescriptor(proxy, 'x'); // { value: 3, writable: false, configurable: false }
> ```
>
> IMHO, the most robust fix is the following: Both the [[DefineOwnProperty]] and the [[GetOwnPropertyDescriptor]] contain the following check:
>
> * If IsCompatiblePropertyDescriptor(extensibleTarget, resultDesc, targetDesc) is false, throw a TypeError exception.
>
> I think there should be also the following check (except when targetDesc is undefined, in which case another appropriate check is used):
>
> * If IsCompatiblePropertyDescriptor(extensibleTarget, targetDesc, resultDesc) is false, throw a TypeError exception.
>
> That would replace the weaker adhoc check about the [[Configurable]] attribute (search for settingConfigFalse) in those algorithms.
>
> (Alternatively, in order to avoid double checks, define an AreBothWaysCompatiblePropertyDescriptors(extensibleTarget, desc1, desc2) abstract operation.)
>
> —Claude
>

Looking closer, it seems that using IsCompatiblePropertyDescriptor is in fact an overkill, because we probably don’t want the special case of conditionally mutable [[Writable]] attribute for nonconfigurable properties.

That is, I think that the following condition must hold: If either the target has a nonconfigurable property, or the proxy claims to have a nonconfigurable property, then every attribute of the property descriptor claimed by the proxy must be identical to the corresponding attribute of the property descriptor of the target.

—Claude




_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Mark S. Miller-2


On Mon, Aug 8, 2016 at 1:35 PM, Tom Van Cutsem <[hidden email]> wrote:
Claude's additional example is indeed evidence that Object.freeze is not to blame, but rather that the invariant checks of [[GetOwnPropertyDescriptor]] and [[DefineOwnProperty]] are too weak. The culprit is, as far as I can tell, that we re-used the state transitions allowed by DefineOwnProperty, which allows the transition from non-configurable,writable (-c+w) to non-configurable,non-writable (-c-w) and which is unwanted here, as Claude rightfully concludes [to better understand the state transitions involved, see MarkM's lucid state transition chart of property attributes <http://wiki.ecmascript.org/doku.php?id=es3.1:attribute_states>. I wish this diagram were in the spec btw, once you know how to read it, it is significantly easier to understand than the DefineOwnProperty algorithm itself]

I would like to see it in the spec too.
 

I like the simplicity of Claude's suggestion, but I think that check is too tight. Technically if the descriptors are both -c+w data descriptors, their value attributes need not be identical to honor the spec invariants. Instead I would propose to tackle the problem head-on and explicitly disallow a proxy from reporting a -c-w property if the corresponding target property is -c+w. We already have a similar check in place in precisely those two algorithms to test if the configurable attribute matches. It is just a matter of tightening that check to also verify the writable attribute.

This would entail the following changes to the ES2015 spec (new or modified text in bold):


9.5.5 [[GetOwnProperty]] (P)
  ...
  22. If resultDesc.[[Configurable]] is false, then
    a. If targetDesc is undefined or targetDesc.[[Configurable]] is true, then
      i. Throw a TypeError exception.
    b. If resultDesc.[[Writable]] is false and targetDesc.[[Writable]] is true, then
      i. Throw a TypeError exception.
      
NOTE [[GetOwnProperty]] for proxy objects enforces the following invariants:

  ...
  * A property cannot be reported as non-configurable, non-writable if it exists as a non-configurable, writable own property on the target object.


9.5.6 [[DefineOwnProperty]] (P, Desc)
  ...
  20. Else targetDesc is not undefined,
    a.  If IsCompatiblePropertyDescriptor(extensibleTarget, Desc , targetDesc) is false, throw a TypeError exception.
    b. If settingConfigFalse is true
      i. If targetDesc.[[Configurable]] is true, throw a TypeError exception.
      ii. If Desc.[[Writable]] is false and targetDesc.[[Writable]] is true, throw a TypeError exception.
      
NOTE [[DefineOwnProperty]] for proxy objects enforces the following invariants:

  ...
  * A property cannot be successfully set to non-configurable, non-writable if the corresponding own property of the target object is non-configurable, writable.

WDYT?

Regards,
Tom

2016-08-08 15:37 GMT+02:00 Claude Pache <[hidden email]>:

> Le 8 août 2016 à 11:02, Claude Pache <[hidden email]> a écrit :
>
> Here is another test case, with [[GetOwnPropertyDescriptor]]:
>
> ```js
> var target = Object.seal({x: 2});
> var proxy = new Proxy(target, {
>    getOwnPropertyDescriptor(o, p) {
>        var d = Reflect.getOwnPropertyDescriptor(o, p)
>        if (d && 'writable' in d)
>            d.writable = false
>        return d
>    }
> });
>
> Object.getOwnPropertyDescriptor(proxy, 'x'); // { value: 2, writable: false, configurable: false }
>
> Object.defineProperty(proxy, 'x', { value: 3 })
>
> Object.getOwnPropertyDescriptor(proxy, 'x'); // { value: 3, writable: false, configurable: false }
> ```
>
> IMHO, the most robust fix is the following: Both the [[DefineOwnProperty]] and the [[GetOwnPropertyDescriptor]] contain the following check:
>
> * If IsCompatiblePropertyDescriptor(extensibleTarget, resultDesc, targetDesc) is false, throw a TypeError exception.
>
> I think there should be also the following check (except when targetDesc is undefined, in which case another appropriate check is used):
>
> * If IsCompatiblePropertyDescriptor(extensibleTarget, targetDesc, resultDesc) is false, throw a TypeError exception.
>
> That would replace the weaker adhoc check about the [[Configurable]] attribute (search for settingConfigFalse) in those algorithms.
>
> (Alternatively, in order to avoid double checks, define an AreBothWaysCompatiblePropertyDescriptors(extensibleTarget, desc1, desc2) abstract operation.)
>
> —Claude
>

Looking closer, it seems that using IsCompatiblePropertyDescriptor is in fact an overkill, because we probably don’t want the special case of conditionally mutable [[Writable]] attribute for nonconfigurable properties.

That is, I think that the following condition must hold: If either the target has a nonconfigurable property, or the proxy claims to have a nonconfigurable property, then every attribute of the property descriptor claimed by the proxy must be identical to the corresponding attribute of the property descriptor of the target.

—Claude






--
    Cheers,
    --MarkM

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Tom Van Cutsem-3
In reply to this post by Tom Van Cutsem-3
I just realized: since the tightened test is reading Desc.[[Writable]] but Desc can by any descriptor, we probably need to first check whether Desc is a DataDescriptor and disregard the writability test otherwise.

2016-08-08 22:35 GMT+02:00 Tom Van Cutsem <[hidden email]>:
Claude's additional example is indeed evidence that Object.freeze is not to blame, but rather that the invariant checks of [[GetOwnPropertyDescriptor]] and [[DefineOwnProperty]] are too weak. The culprit is, as far as I can tell, that we re-used the state transitions allowed by DefineOwnProperty, which allows the transition from non-configurable,writable (-c+w) to non-configurable,non-writable (-c-w) and which is unwanted here, as Claude rightfully concludes [to better understand the state transitions involved, see MarkM's lucid state transition chart of property attributes <http://wiki.ecmascript.org/doku.php?id=es3.1:attribute_states>. I wish this diagram were in the spec btw, once you know how to read it, it is significantly easier to understand than the DefineOwnProperty algorithm itself]

I like the simplicity of Claude's suggestion, but I think that check is too tight. Technically if the descriptors are both -c+w data descriptors, their value attributes need not be identical to honor the spec invariants. Instead I would propose to tackle the problem head-on and explicitly disallow a proxy from reporting a -c-w property if the corresponding target property is -c+w. We already have a similar check in place in precisely those two algorithms to test if the configurable attribute matches. It is just a matter of tightening that check to also verify the writable attribute.

This would entail the following changes to the ES2015 spec (new or modified text in bold):


9.5.5 [[GetOwnProperty]] (P)
  ...
  22. If resultDesc.[[Configurable]] is false, then
    a. If targetDesc is undefined or targetDesc.[[Configurable]] is true, then
      i. Throw a TypeError exception.
    b. If resultDesc.[[Writable]] is false and targetDesc.[[Writable]] is true, then
      i. Throw a TypeError exception.
      
NOTE [[GetOwnProperty]] for proxy objects enforces the following invariants:

  ...
  * A property cannot be reported as non-configurable, non-writable if it exists as a non-configurable, writable own property on the target object.


9.5.6 [[DefineOwnProperty]] (P, Desc)
  ...
  20. Else targetDesc is not undefined,
    a.  If IsCompatiblePropertyDescriptor(extensibleTarget, Desc , targetDesc) is false, throw a TypeError exception.
    b. If settingConfigFalse is true
      i. If targetDesc.[[Configurable]] is true, throw a TypeError exception.
      ii. If Desc.[[Writable]] is false and targetDesc.[[Writable]] is true, throw a TypeError exception.
      
NOTE [[DefineOwnProperty]] for proxy objects enforces the following invariants:

  ...
  * A property cannot be successfully set to non-configurable, non-writable if the corresponding own property of the target object is non-configurable, writable.

WDYT?

Regards,
Tom

2016-08-08 15:37 GMT+02:00 Claude Pache <[hidden email]>:

> Le 8 août 2016 à 11:02, Claude Pache <[hidden email]> a écrit :
>
> Here is another test case, with [[GetOwnPropertyDescriptor]]:
>
> ```js
> var target = Object.seal({x: 2});
> var proxy = new Proxy(target, {
>    getOwnPropertyDescriptor(o, p) {
>        var d = Reflect.getOwnPropertyDescriptor(o, p)
>        if (d && 'writable' in d)
>            d.writable = false
>        return d
>    }
> });
>
> Object.getOwnPropertyDescriptor(proxy, 'x'); // { value: 2, writable: false, configurable: false }
>
> Object.defineProperty(proxy, 'x', { value: 3 })
>
> Object.getOwnPropertyDescriptor(proxy, 'x'); // { value: 3, writable: false, configurable: false }
> ```
>
> IMHO, the most robust fix is the following: Both the [[DefineOwnProperty]] and the [[GetOwnPropertyDescriptor]] contain the following check:
>
> * If IsCompatiblePropertyDescriptor(extensibleTarget, resultDesc, targetDesc) is false, throw a TypeError exception.
>
> I think there should be also the following check (except when targetDesc is undefined, in which case another appropriate check is used):
>
> * If IsCompatiblePropertyDescriptor(extensibleTarget, targetDesc, resultDesc) is false, throw a TypeError exception.
>
> That would replace the weaker adhoc check about the [[Configurable]] attribute (search for settingConfigFalse) in those algorithms.
>
> (Alternatively, in order to avoid double checks, define an AreBothWaysCompatiblePropertyDescriptors(extensibleTarget, desc1, desc2) abstract operation.)
>
> —Claude
>

Looking closer, it seems that using IsCompatiblePropertyDescriptor is in fact an overkill, because we probably don’t want the special case of conditionally mutable [[Writable]] attribute for nonconfigurable properties.

That is, I think that the following condition must hold: If either the target has a nonconfigurable property, or the proxy claims to have a nonconfigurable property, then every attribute of the property descriptor claimed by the proxy must be identical to the corresponding attribute of the property descriptor of the target.

—Claude





_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Claude Pache
In reply to this post by Claude Pache
Given a Proxy that pretends to be in state A while its target is observably in state B, and assuming that the target satisfies the Invariants of the Essential Internal Methods [6.1.7.3], I claim that, in order to force the Proxy to satisfy those Invariants, it is necessary and sufficient to check that the two following conditions hold:

* it is legal for an object to pass from state A to state B; and,
* it is legal for an object to pass from state B to state A.

[6.1.7.3]: https://tc39.github.io/ecma262/#sec-invariants-of-the-essential-internal-methods


Because I am too lazy to write the proof just now, I cowardly leave it as an exercice to the reader. Meanwhile, that principle may be used to audit the robustness of the Proxy specification. I have found the following bug in Proxy.[[Delete]]() by applying the above principle to:

* state A: nonexistent property on a nonextensible object;
* state B: existent own property on a nonextensible object.

Resurrection of a successfully deleted property on a nonextensible object:

```js
var target = Object.preventExtensions({ x: 1 })
var proxy = new Proxy(target, {
    deleteProperty() { return true }
})

Object.isExtensible(proxy) // false
delete proxy.x // true
proxy.hasOwnProperty('x') // true
```

After a first scan, I haven't found other bugs in the essential methods of Proxy, than that one and the missing nonconfigurable-but-writable check in [[GetOwnPropertyDescriptor]] and [[DefineOwnProperty]] already mentioned in that thread.

I plan to propose a minimal patch (i.e., just adding the missing checks) in a few days.

—Claude


_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Claude Pache
In reply to this post by Tom Van Cutsem-3

Le 8 août 2016 à 22:35, Tom Van Cutsem <[hidden email]> a écrit :

Claude's additional example is indeed evidence that Object.freeze is not to blame, but rather that the invariant checks of [[GetOwnPropertyDescriptor]] and [[DefineOwnProperty]] are too weak. The culprit is, as far as I can tell, that we re-used the state transitions allowed by DefineOwnProperty, which allows the transition from non-configurable,writable (-c+w) to non-configurable,non-writable (-c-w) and which is unwanted here, as Claude rightfully concludes [to better understand the state transitions involved, see MarkM's lucid state transition chart of property attributes <http://wiki.ecmascript.org/doku.php?id=es3.1:attribute_states>. I wish this diagram were in the spec btw, once you know how to read it, it is significantly easier to understand than the DefineOwnProperty algorithm itself]

I like the simplicity of Claude's suggestion, but I think that check is too tight. Technically if the descriptors are both -c+w data descriptors, their value attributes need not be identical to honor the spec invariants. Instead I would propose to tackle the problem head-on and explicitly disallow a proxy from reporting a -c-w property if the corresponding target property is -c+w. We already have a similar check in place in precisely those two algorithms to test if the configurable attribute matches. It is just a matter of tightening that check to also verify the writable attribute.

This would entail the following changes to the ES2015 spec (new or modified text in bold):


9.5.5 [[GetOwnProperty]] (P)
  ...
  22. If resultDesc.[[Configurable]] is false, then
    a. If targetDesc is undefined or targetDesc.[[Configurable]] is true, then
      i. Throw a TypeError exception.
    b. If resultDesc.[[Writable]] is false and targetDesc.[[Writable]] is true, then
      i. Throw a TypeError exception.
      
NOTE [[GetOwnProperty]] for proxy objects enforces the following invariants:

  ...
  * A property cannot be reported as non-configurable, non-writable if it exists as a non-configurable, writable own property on the target object.


9.5.6 [[DefineOwnProperty]] (P, Desc)
  ...
  20. Else targetDesc is not undefined,
    a.  If IsCompatiblePropertyDescriptor(extensibleTarget, Desc , targetDesc) is false, throw a TypeError exception.
    b. If settingConfigFalse is true
      i. If targetDesc.[[Configurable]] is true, throw a TypeError exception.
      ii. If Desc.[[Writable]] is false and targetDesc.[[Writable]] is true, throw a TypeError exception.

Rather:

  b (unchanged). If settingConfigFalse is true and targetDesc.[[Configurable]] is true, throw a TypeError exception. 
  c. If targetDesc.[[Configurable]] is false, then
    i. If targetDesc.[[Writable]] is true and Desc.[[Writable]] is false, throw a TypeError exception.

because we need the additional check on [[Writable]] when the property is nonconfigurable ("targetDesc.[[Configurable]] is false"), not only when the property is made nonconfigurable ("settingConfigFalse is true").

—Claude


      
NOTE [[DefineOwnProperty]] for proxy objects enforces the following invariants:

  ...
  * A property cannot be successfully set to non-configurable, non-writable if the corresponding own property of the target object is non-configurable, writable.

WDYT?

Regards,
Tom


_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Raul-Sebastian Mihăilă
In reply to this post by Claude Pache
Claude, I don't see how non-extensibility and deleting properties are connected.


```js
var target = Object.preventExtensions({ x: 1 })
var proxy = new Proxy(target, {
    deleteProperty() { return true }
})

Object.isExtensible(proxy) // false
delete proxy.x // true
proxy.hasOwnProperty('x') // true
```




_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Claude Pache
Le 9 août 2016 à 18:58, Raul-Sebastian Mihăilă <[hidden email]> a écrit :

Claude, I don't see how non-extensibility and deleting properties are connected.


The issue is not deleting per se. The issue is that a property appears to be non-existent (because successfully deleted), and later existent again, which should not be allowed on non-extensible objects.

—Claude


```js
var target = Object.preventExtensions({ x: 1 })
var proxy = new Proxy(target, {
    deleteProperty() { return true }
})

Object.isExtensible(proxy) // false
delete proxy.x // true
proxy.hasOwnProperty('x') // true
```





_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

RE: Object.freezing proxies should freeze or throw?

doodad-js Admin
In reply to this post by Claude Pache
I think too much validation is not a good idea. Let the proxy lie. If you don't, what is the purpose of Proxy? I have a case where I wanted to force every new property to be read-only through a Proxy so that, once created, they can no longer change. But I get "TypeError" because of such a validation:

```js
"use strict";
const proxy = new Proxy({}, {
        set(target, property, value, receiver) {
                if (Object.prototype.hasOwnProperty.call(target, property)) return false;
                Object.defineProperty(target, property, {configurable: false, enumerable: true, writable: false, value: value});
                return true;
        },
        defineProperty(target, property, descriptor) {
                if (Object.prototype.hasOwnProperty.call(target, property)) return false;
                descriptor = Object.assign({}, descriptor, {configurable: false, enumerable: true});
                if (!descriptor.get && !descriptor.set) descriptor.writable = false;
                Object.defineProperty(target, property, descriptor);
                return true;
        },
});

proxy.a = 1;
proxy.a = 2; // TypeError: 'set' on proxy: trap returned falsish for property 'a'

Object.defineProperty(proxy, 'b', {value: 3});
Object.defineProperty(proxy, 'b', {value: 4}); // TypeError: 'defineProperty' on proxy: trap returned falsish for property 'b'
```

-----Original Message-----
From: Claude Pache [mailto:[hidden email]]
Sent: Tuesday, August 09, 2016 8:44 AM
To: es-discuss <[hidden email]>
Cc: Mark S. Miller <[hidden email]>; Raul-Sebastian Mihăilă <[hidden email]>
Subject: Re: Object.freezing proxies should freeze or throw?

Given a Proxy that pretends to be in state A while its target is observably in state B, and assuming that the target satisfies the Invariants of the Essential Internal Methods [6.1.7.3], I claim that, in order to force the Proxy to satisfy those Invariants, it is necessary and sufficient to check that the two following conditions hold:

* it is legal for an object to pass from state A to state B; and,
* it is legal for an object to pass from state B to state A.

[6.1.7.3]: https://tc39.github.io/ecma262/#sec-invariants-of-the-essential-internal-methods


Because I am too lazy to write the proof just now, I cowardly leave it as an exercice to the reader. Meanwhile, that principle may be used to audit the robustness of the Proxy specification. I have found the following bug in Proxy.[[Delete]]() by applying the above principle to:

* state A: nonexistent property on a nonextensible object;
* state B: existent own property on a nonextensible object.

Resurrection of a successfully deleted property on a nonextensible object:

```js
var target = Object.preventExtensions({ x: 1 }) var proxy = new Proxy(target, {
    deleteProperty() { return true }
})

Object.isExtensible(proxy) // false
delete proxy.x // true
proxy.hasOwnProperty('x') // true
```

After a first scan, I haven't found other bugs in the essential methods of Proxy, than that one and the missing nonconfigurable-but-writable check in [[GetOwnPropertyDescriptor]] and [[DefineOwnProperty]] already mentioned in that thread.

I plan to propose a minimal patch (i.e., just adding the missing checks) in a few days.

—Claude




_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Mark S. Miller
On Tue, Aug 9, 2016 at 10:58 AM, doodad-js Admin <[hidden email]> wrote:
I think too much validation is not a good idea. Let the proxy lie. If you don't, what is the purpose of Proxy?


Of course, we can still agree that too much, by definition, is not a good idea ;).


 
I have a case where I wanted to force every new property to be read-only through a Proxy so that, once created, they can no longer change. But I get "TypeError" because of such a validation:

```js
"use strict";
const proxy = new Proxy({}, {
        set(target, property, value, receiver) {
                if (Object.prototype.hasOwnProperty.call(target, property)) return false;
                Object.defineProperty(target, property, {configurable: false, enumerable: true, writable: false, value: value});
                return true;
        },
        defineProperty(target, property, descriptor) {
                if (Object.prototype.hasOwnProperty.call(target, property)) return false;
                descriptor = Object.assign({}, descriptor, {configurable: false, enumerable: true});
                if (!descriptor.get && !descriptor.set) descriptor.writable = false;
                Object.defineProperty(target, property, descriptor);
                return true;
        },
});

proxy.a = 1;
proxy.a = 2; // TypeError: 'set' on proxy: trap returned falsish for property 'a'

Object.defineProperty(proxy, 'b', {value: 3});
Object.defineProperty(proxy, 'b', {value: 4}); // TypeError: 'defineProperty' on proxy: trap returned falsish for property 'b'

I don't understand the purpose of this code. From your description, don't you want these TypeErrors? Could you show a hypothetical test case that, if passed, demonstrates what you are actually trying to accomplish?


 
```

-----Original Message-----
From: Claude Pache [mailto:[hidden email]]
Sent: Tuesday, August 09, 2016 8:44 AM
To: es-discuss <[hidden email]>
Cc: Mark S. Miller <[hidden email]>; Raul-Sebastian Mihăilă <[hidden email]>
Subject: Re: Object.freezing proxies should freeze or throw?

Given a Proxy that pretends to be in state A while its target is observably in state B, and assuming that the target satisfies the Invariants of the Essential Internal Methods [6.1.7.3], I claim that, in order to force the Proxy to satisfy those Invariants, it is necessary and sufficient to check that the two following conditions hold:

* it is legal for an object to pass from state A to state B; and,
* it is legal for an object to pass from state B to state A.

[6.1.7.3]: https://tc39.github.io/ecma262/#sec-invariants-of-the-essential-internal-methods


Because I am too lazy to write the proof just now, I cowardly leave it as an exercice to the reader. Meanwhile, that principle may be used to audit the robustness of the Proxy specification. I have found the following bug in Proxy.[[Delete]]() by applying the above principle to:

* state A: nonexistent property on a nonextensible object;
* state B: existent own property on a nonextensible object.

Resurrection of a successfully deleted property on a nonextensible object:

```js
var target = Object.preventExtensions({ x: 1 }) var proxy = new Proxy(target, {
    deleteProperty() { return true }
})

Object.isExtensible(proxy) // false
delete proxy.x // true
proxy.hasOwnProperty('x') // true
```

After a first scan, I haven't found other bugs in the essential methods of Proxy, than that one and the missing nonconfigurable-but-writable check in [[GetOwnPropertyDescriptor]] and [[DefineOwnProperty]] already mentioned in that thread.

I plan to propose a minimal patch (i.e., just adding the missing checks) in a few days.

—Claude




_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss



--
  Cheers,
  --MarkM

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

RE: Object.freezing proxies should freeze or throw?

doodad-js Admin

“Of course, we can still agree that too much, by definition, is not a good idea ;)”

 

Thanks, that was my point. But my example was not appropriated. It occurs that I’ve misread MDN:

 

Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/handler/set

“If the following invariants are violated, the proxy will throw a TypeError:”

But just below :

 

In strict mode, a false return value from the set handler will throw a TypeError exception.”

 

 


_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Claude Pache
In reply to this post by Claude Pache
By reviewing more carefully the Proxy internal methods, I have found the following glitch in [[OwnPropertyKeys]]. 

Testcase:

```js
var base = Object.freeze({x: 1})
var target = new Proxy(base, {
    ownKeys(o) {
        return ['x', 'x']
    }
})

Object.keys(target) // ['x', 'x'] // ok; according to ECMA262, Chrome, and Safari TP; buggy in Firefox

var proxy = new Proxy(target, {
    ownKeys(o) {
        return ['x']
    }
})

Object.keys(proxy) // !!! throws in ECMA262, Chrome, and Safari TP; should be fixed so that it returns ['x']
```

The issue is in step 17:

  17. Repeat, for each key that is an element of targetNonconfigurableKeys,
        a. If key is not an element of uncheckedResultKeys, throw a TypeError exception.
        b. Remove all occurrences of key from uncheckedResultKeys.

because targetNonconfigurableKeys contains a list with twice the value 'x', and, at the second iteration, a TypeError is thrown in step a.

The solution is simple: step 14 should guard against duplicate keys when constructing targetNonconfigurableKeys and targetConfigurableKeys. Additional text marked in bold:

  14. Repeat, for each element key of targetKeys,
      I. If key is not already an element of targetNonconfigurableKeys or targetConfigurableKeys, then 
            a. Let desc be ? target.[[GetOwnProperty]](key).
            b. If desc is not undefined and desc.[[Configurable]] is false, then
                  i. Append key as an element of targetNonconfigurableKeys.
            c. Else,
                ii. Append key as an element of targetConfigurableKeys.


―Claude


_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Object.freezing proxies should freeze or throw?

Tom Van Cutsem-3
In reply to this post by Claude Pache
Thanks Claude for your careful review, and for trying to articulate a more general principle behind the invariant checks. The lack of such crisp principles makes it (too) difficult to verify whether all necessary checks are in place.

To clarify, in the case of "update" MOP operations such as defineProperty and deleteProperty, your "state B" (the observable state of the target), presumably is the state of the target *after* the proxy trap has executed, i.e. state B should already reflect the update.

Both the original issue and your new test case with deleteProperty are cases where the proxy pretends to have altered the target but has in fact not. In a later interaction, the proxy reverts to the original state. This violates a client's expectations about when state transitions occur.

Interestingly, the opposite problem, where a proxy does alter the target as requested, but then reports that the update failed, is allowed, even though this technically also violates a client's expectations about what state transitions have occurred. But a failed update leads to a TypeError anyway.

At this point, we should probably iron out the details of the fix in a GitHub issue or on bugs.ecmascript.org.

Cheers,
Tom

2016-08-09 14:44 GMT+02:00 Claude Pache <[hidden email]>:
Given a Proxy that pretends to be in state A while its target is observably in state B, and assuming that the target satisfies the Invariants of the Essential Internal Methods [6.1.7.3], I claim that, in order to force the Proxy to satisfy those Invariants, it is necessary and sufficient to check that the two following conditions hold:

* it is legal for an object to pass from state A to state B; and,
* it is legal for an object to pass from state B to state A.

[6.1.7.3]: https://tc39.github.io/ecma262/#sec-invariants-of-the-essential-internal-methods


Because I am too lazy to write the proof just now, I cowardly leave it as an exercice to the reader. Meanwhile, that principle may be used to audit the robustness of the Proxy specification. I have found the following bug in Proxy.[[Delete]]() by applying the above principle to:

* state A: nonexistent property on a nonextensible object;
* state B: existent own property on a nonextensible object.

Resurrection of a successfully deleted property on a nonextensible object:

```js
var target = Object.preventExtensions({ x: 1 })
var proxy = new Proxy(target, {
    deleteProperty() { return true }
})

Object.isExtensible(proxy) // false
delete proxy.x // true
proxy.hasOwnProperty('x') // true
```

After a first scan, I haven't found other bugs in the essential methods of Proxy, than that one and the missing nonconfigurable-but-writable check in [[GetOwnPropertyDescriptor]] and [[DefineOwnProperty]] already mentioned in that thread.

I plan to propose a minimal patch (i.e., just adding the missing checks) in a few days.

—Claude




_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
12