Discuss
  • Nuking misleading properties in `Object.getOwnPropertyDescriptor`

Nuking misleading properties in `Object.getOwnPropertyDescriptor`

# Nathan Wall (12 years ago)

Given that defineProperty uses properties on the prototype of the descriptor[1] and getOwnPropertyDescriptor returns an object which inherits from Object.prototype, the following use-case is volatile:

function copy(from, to) {         for (let name of Object.getOwnPropertyNames(from))             Object.defineProperty(to, name,                 Object.getOwnPropertyDescriptor(from, name));     }

If a third party script happens to add get, set, or value to Object.prototype the copy function breaks.

Object.prototype.get = function() { };

var x = { foo: 1 };     var y = { };     copy(x, y); // TypeError: property descriptors must not specify a value or                 // be writable when a getter or setter has been specified

It's misleading that you can't trust getOwnPropertyDescriptor to give you a descriptor which you can use to define a property with defineProperty.

A script which doesn't use defineProperty could very well define a get property on Object.prototype, unknowingly breaking other scripts which do use defineProperty. This is a hazard in the language.

I think one of the following would be an appropriate solution:

(1) "Nuke" the special properties (get, set, and value when any of them is not defined) on a descriptor returned by getOwnPropertyDescriptor which shouldn't be inherited through the descriptor's prototype. By "nuke" them, I mean specify that they be defined as undefined, much like callee and caller in strict mode. (I don't think strict mode is required in this case, though, since I highly doubt anyone could be relying on this behavior.)

(2) Use the introduction of Reflect.defineProperty as an opportunity to "fix" this function to only inspect own properties on the property descriptor so that the descriptor's prototype doesn't matter. This is similar to the addition of Number.isNaN to fix the broken isNaN.

Nathan

[1] esdiscuss/2012-November/026705

# Nathan Wall (12 years ago)

(1) "Nuke" the special properties (get, set, and value when any of them is not defined) on a descriptor returned by getOwnPropertyDescriptor which shouldn't be inherited through the descriptor's prototype. By "nuke" them, I mean specify that they be defined as undefined, much like callee and caller in strict mode. (I don't think strict mode is required in this case, though, since I highly doubt anyone could be relying on this behavior.)

Naturally, defineProperty would also have to be rewritten a little to ignore properties set to undefined. If get, set, and value are all set to undefined, then value would be assumed to be undefined. In addition, writable would also have to be nuked for accessor descriptors.  I think it's workable?

Nathan

# Andrea Giammarchi (12 years ago)

for safe definition and more, have a look into redefine WebReflection/redefine#redefinejs

br

# Herby Vojčík (12 years ago)

Yes, this was in this list some three-four months ago. There were answers like "JS is this way dynamic, playing with Object.prototype hurts".

I proposed that return value from Object.getOwnPropertyDescriptor should inherit from well well-known frozen object with all the default set. Back, then no-one seemed to be interested.

# Andrea Giammarchi (12 years ago)

there was another discussion here, is not just getOwnPropertyDescriptor problem: esdiscuss/2012-November/026705

# Tom Van Cutsem (12 years ago)

2013/3/10 Nathan Wall <nathan.wall at live.com>

Given that defineProperty uses properties on the prototype of the descriptor[1] and getOwnPropertyDescriptor returns an object which inherits from Object.prototype, the following use-case is volatile:

function copy(from, to) {
    for (let name of Object.getOwnPropertyNames(from))
        Object.defineProperty(to, name,
            Object.getOwnPropertyDescriptor(from, name));
}

If a third party script happens to add get, set, or value to Object.prototype the copy function breaks.

To my mind, the blame for the breakage lies with Object.prototype being mutated by the third-party script, not with property descriptors inheriting from Object.prototype. Thus, a fix for the breakage should address that directly, rather than tweaking the design of property descriptors, IMHO.

(2) Use the introduction of Reflect.defineProperty as an opportunity to

"fix" this function to only inspect own properties on the property descriptor so that the descriptor's prototype doesn't matter. This is similar to the addition of Number.isNaN to fix the broken isNaN.

Apart from the fact that Reflect.defineProperty returns a boolean success value rather than throwing or returning normally, I would not be in favor of changing the semantics of the Reflect.* functions much from the Object.* functions. Your suggestion is reasonable, but I wonder if the inconsistent behavior it introduces isn't going to cause more confusion than the problem it intends to fix in the first place.

# David Bruant (12 years ago)

Le 12/03/2013 16:45, Tom Van Cutsem a écrit :

Hi Nathan,

2013/3/10 Nathan Wall <nathan.wall at live.com <mailto:nathan.wall at live.com>>

Given that `defineProperty` uses properties on the prototype of
the descriptor[1] and `getOwnPropertyDescriptor` returns an object
which inherits from `Object.prototype`, the following use-case is
volatile:

    function copy(from, to) {
        for (let name of Object.getOwnPropertyNames(from))
            Object.defineProperty(to, name,
                Object.getOwnPropertyDescriptor(from, name));
    }

If a third party script happens to add `get`, `set`, or `value` to
`Object.prototype` the `copy` function breaks.

To my mind, the blame for the breakage lies with Object.prototype being mutated by the third-party script, not with property descriptors inheriting from Object.prototype. Thus, a fix for the breakage should address that directly, rather than tweaking the design of property descriptors, IMHO.

I agree.

As Object.prototype-jacking threats are discussed more and more recently, I'd like to take a step back and "meta-discuss" JavaScript threats.

Currently, by default, any script that run can mutate the environment it is executed in (it can be fixed by sandboxing with things like Caja [1] and soon the module loader API used with proxies [2], but even then, there could be leaks of native built-ins). The first (security) decision any JavaScript application should make would be to freeze all built-ins like SES [3][4] does. (In the future, it could even make sense to add a CSP [5] directive for that) If necessary, the application can first enhance the environment by adding polyfills/libraries and such, but that's pretty much the only thing that's acceptable to run before freezing everything.

Given that freezing all built-ins (after polyfills) is a reasonable thing to do, I think JavaScript threat should be considered serious only if applicable assuming the environment is already frozen. It naturally rules out threats related to property descriptors inheriting from Object.prototype or anything looking like "what if an attacker switches Array.prototype.push and Array.prototype.pop?"

David

[1] code.google.com/p/google-caja [2] harmony:module_loaders [3] code.google.com/p/es-lab/wiki/SecureEcmaScript [4] code.google.com/p/es-lab/source/browse/#svn%2Ftrunk%2Fsrc%2Fses [5] dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html

# Nathan Wall (12 years ago)

David Bruant wrote:

Tom Van Cutsem wrote:

To my mind, the blame for the breakage lies with Object.prototype  being mutated by the third-party script, not with property descriptors  inheriting from Object.prototype. Thus, a fix for the breakage should  address that directly, rather than tweaking the design of property  descriptors, IMHO.  I agree.    The first (security) decision any JavaScript application should make  would be to freeze all built-ins like SES [3][4] does. (In the future,  it could even make sense to add a CSP [5] directive for that)  If necessary, the application can first enhance the environment by  adding polyfills/libraries and such, but that's pretty much the only  thing that's acceptable to run before freezing everything.

Hey David and Tom.  This is good advice for application authors, but I don't work at the application level; I write libraries.  I don't want to freeze everything because I want to leave the environment open to monkey-patching and shimming by other libraries and the application authors. So this isn't an option for me.

"what if an attacker switches Array.prototype.push and Array.prototype.pop?"

These are issues that are easy to address by using stored late-bound function references rather than methods and array-likes instead of true arrays.

var push = Function.prototype.call.bind(Array.prototype.push),         arrayLike = Object.create(null);     arrayLike.length = 0;     push(arrayLike, 'item-1');

As long as the environment is correct when my script initializes, I get all methods I need to use stored inside my library's closure. Freezing isn't needed.

It's also possible to write around the defineProperty problem by converting the descriptor into a prototype-less object. However, I actually encountered some performance problems with this. I was able to improve the performance by only dropping the prototype when necessary (as long as get, set, value or writable haven't been added to Object.prototype, it's not necessary). However, as a matter of principle, my argument is that Object.getOwnPropertyDescriptor should, at the bare minimum, return a descriptor that can be known to work in Object.defineProperty.  If Object.defineProperty doesn't accept it, then you getOwnPropertyDescriptor didn't really give me a valid descriptor.

I think that this behavior (1) limits the creativity of developers to define properties like Object.prototype.get, (2) is a potential stumbling block, (3) has no real benefit -- really, there's not anything positive about this behavior, and (4) forces developers who want to support Object.prototype.get to add an extra layer of cleaning before using defineProperty.

Nathan

# Mark S. Miller (12 years ago)

The one qualification everyone should be aware of is that if they simply freeze these themselves, rather than using the tamper-proofing abstractions defined by SES[1][2], then they will suffer from the override mistake[3] and conventional code such as the following will break:

function Point(x, y) { this.x = x; this.y = y; } Point.prototype.toString = function() { return '<' + x + ',' + y + '>'; };

This normal looking assignment to Point.prototype.toString fails because Point.prototype inherits from Object.prototype, on which toString is a non-writable non-configurable data property.

[1] code.google.com/p/google-caja/source/browse/trunk/src/com/google/caja/ses/repairES5.js#466 [2] code.google.com/p/google-caja/source/browse/trunk/src/com/google/caja/ses/startSES.js#869 [3] strawman:fixing_override_mistake

# Mark S. Miller (12 years ago)

On Wed, Mar 13, 2013 at 8:26 AM, Nathan Wall <nathan.wall at live.com> wrote:

David Bruant wrote:

Tom Van Cutsem wrote:

To my mind, the blame for the breakage lies with Object.prototype being mutated by the third-party script, not with property descriptors inheriting from Object.prototype. Thus, a fix for the breakage should address that directly, rather than tweaking the design of property descriptors, IMHO. I agree.

The first (security) decision any JavaScript application should make would be to freeze all built-ins like SES [3][4] does. (In the future, it could even make sense to add a CSP [5] directive for that) If necessary, the application can first enhance the environment by adding polyfills/libraries and such, but that's pretty much the only thing that's acceptable to run before freezing everything.

Hey David and Tom. This is good advice for application authors, but I don't work at the application level; I write libraries. I don't want to freeze everything because I want to leave the environment open to monkey-patching and shimming by other libraries and the application authors. So this isn't an option for me.

"what if an attacker switches Array.prototype.push and Array.prototype.pop?"

These are issues that are easy to address by using stored late-bound function references rather than methods and array-likes instead of true arrays.

var push = Function.prototype.call.bind(Array.prototype.push),
    arrayLike = Object.create(null);
arrayLike.length = 0;
push(arrayLike, 'item-1');

As long as the environment is correct when my script initializes, I get all methods I need to use stored inside my library's closure. Freezing isn't needed.

That's correct. I've written a bit of code like that myself. At first, for those used to JS or even just conventional oo, the style needed is very counter-intuitive and awkward -- it goes against the grain of what the language tries to make convenient. But amazingly, it is possible, and one even gets used to it after a while.

We need a name for your use case. It is definitely distinct from the normal JS library writing practice, which implicitly assumes that the primordials haven't been too corrupted but does nothing to detect if this is true. And distinct from the opposite extreme -- the SES library practice, which may rely on the primordials having been made incorruptible. Amusingly, these extremes resemble each other much more than they resemble your middle ground.

For your use case, your analysis of the defineProperty problem is indeed valid. To date, few if any language design decisions have purposely been made to accommodate this use case, although some language changes have helped anyway. I think perhaps the shortest path towards anything like the defineProperty reform you seek is to argue first why this use case is important.

# David Bruant (12 years ago)

Le 13/03/2013 16:26, Nathan Wall a écrit :

David Bruant wrote:

Tom Van Cutsem wrote:

To my mind, the blame for the breakage lies with Object.prototype being mutated by the third-party script, not with property descriptors inheriting from Object.prototype. Thus, a fix for the breakage should address that directly, rather than tweaking the design of property descriptors, IMHO. I agree.

The first (security) decision any JavaScript application should make would be to freeze all built-ins like SES [3][4] does. (In the future, it could even make sense to add a CSP [5] directive for that) If necessary, the application can first enhance the environment by adding polyfills/libraries and such, but that's pretty much the only thing that's acceptable to run before freezing everything. Hey David and Tom. This is good advice for application authors, but I don't work at the application level; I write libraries. I don't want to freeze everything because I want to leave the environment open to monkey-patching and shimming by other libraries and the application authors. So this isn't an option for me.

Interesting. As a library author, in theory, you don't know in which environment your code is going to be executed (that is maybe the environment has been modified or not), so I think one assumption has to be made: Either you assume the library runs in a non-changing environment (whether the client has frozen itself or just decided not to change anything) or you assume it is a battlefield where anything can happen and try to capture a reference to all available built-ins at first load as well as being defensive against potential changes to the environment (like Object.prototype in your case).

"what if an attacker switches Array.prototype.push and Array.prototype.pop?" These are issues that are easy to address by using stored late-bound function references rather than methods and array-likes instead of true arrays.

 var push = Function.prototype.call.bind(Array.prototype.push),
     arrayLike = Object.create(null);
 arrayLike.length = 0;
 push(arrayLike, 'item-1');

As long as the environment is correct when my script initializes, I get all methods I need to use stored inside my library's closure. Freezing isn't needed.

It's also possible to write around the defineProperty problem by converting the descriptor into a prototype-less object. However, I actually encountered some performance problems with this. I was able to improve the performance by only dropping the prototype when necessary (as long as get, set, value or writable haven't been added to Object.prototype, it's not necessary). However, as a matter of principle, my argument is that Object.getOwnPropertyDescriptor should, at the bare minimum, return a descriptor that can be known to work in Object.defineProperty. If Object.defineProperty doesn't accept it, then you getOwnPropertyDescriptor didn't really give me a valid descriptor.

I think that this behavior (1) limits the creativity of developers to define properties like Object.prototype.get

I don't think we should consider adding a Object.prototype.get property as creativity. For instance, proxies have an optional "get" trap, so things can become confusing quickly. Other properties also have other meanings for other objects. For instance JSON.parse creates objects with Object.prototype as [[Prototype]], so custom Object.prototype properties may be confusing in "myData.someProp" cases. It's possible, but annoying to prefix every [[Get]] with hasOwnProperty checks. If someone tries to polyfill Array.prototype.contains, they may test 'contains' in Array.prototype If Object.prototype.contains is defined, this can mistakenly return true. etc. Writing defensive code is a serious relearning of how to write for the language.

# David Bruant (12 years ago)

Le 13/03/2013 16:49, Mark S. Miller a écrit :

On Wed, Mar 13, 2013 at 8:26 AM, Nathan Wall <nathan.wall at live.com <mailto:nathan.wall at live.com>> wrote:

David Bruant wrote:
> Tom Van Cutsem wrote:
> > To my mind, the blame for the breakage lies with
`Object.prototype`
> > being mutated by the third-party script, not with property
descriptors
> > inheriting from Object.prototype. Thus, a fix for the breakage
should
> > address that directly, rather than tweaking the design of
property
> > descriptors, IMHO.
> I agree.
>
> The first (security) decision any JavaScript application should
make
> would be to freeze all built-ins like SES [3][4] does. (In the
future,
> it could even make sense to add a CSP [5] directive for that)
> If necessary, the application can first enhance the environment by
> adding polyfills/libraries and such, but that's pretty much the
only
> thing that's acceptable to run before freezing everything.

Hey David and Tom.  This is good advice for application authors,
but I don't work at the application level; I write libraries.  I
don't want to freeze everything because I want to leave the
environment open to monkey-patching and shimming by other
libraries and the application authors. So this isn't an option for me.

> "what if an attacker switches Array.prototype.push and
Array.prototype.pop?"

These are issues that are easy to address by using stored
late-bound function references rather than methods and array-likes
instead of true arrays.

    var push = Function.prototype.call.bind(Array.prototype.push),
        arrayLike = Object.create(null);
    arrayLike.length = 0;
    push(arrayLike, 'item-1');

As long as the environment is correct when my script initializes,
I get all methods I need to use stored inside my library's
closure. Freezing isn't needed.

That's correct. I've written a bit of code like that myself. At first, for those used to JS or even just conventional oo, the style needed is very counter-intuitive and awkward -- it goes against the grain of what the language tries to make convenient. But amazingly, it is possible, and one even gets used to it after a while.

Would it be easier to teach everyone to freeze their built-ins or to (re)write their code using this style?

We need a name for your use case. It is definitely distinct from the normal JS library writing practice, which implicitly assumes that the primordials haven't been too corrupted but does nothing to detect if this is true.

Given what you're describing as normal library writing practice, teaching everyone to freeze their built-ins sounds like a safer bet in the path of acceptability and making the web overall safer.

In my opinion, to a large extent, using a library and messing around with built-ins the library might be using is a recipe for disaster. This practice shouldn't be encouraged.

# Tom Van Cutsem (12 years ago)

[+Allen]

2013/3/13 Nathan Wall <nathan.wall at live.com>

However, as a matter of principle, my argument is that Object.getOwnPropertyDescriptor should, at the bare minimum, return a descriptor that can be known to work in Object.defineProperty. If Object.defineProperty doesn't accept it, then you getOwnPropertyDescriptor didn't really give me a valid descriptor.

I think that this behavior (1) limits the creativity of developers to define properties like Object.prototype.get, (2) is a potential stumbling block, (3) has no real benefit -- really, there's not anything positive about this behavior, and (4) forces developers who want to support Object.prototype.get to add an extra layer of cleaning before using defineProperty.

While the monkey-patching of Object.prototype ("don't do that!") is still the culprit, I agree that it would have been better if defineProperty looked only at "own" properties of the descriptor. I almost always think of descriptors as "records" rather than "objects". Similarly, perhaps Object.getOwnPropertyDescriptor should have returned descriptors whose [[prototype]] was null.

It's true that Reflect.getOwnPropertyDescriptor and Reflect.defineProperty give us a chance to fix this. I'm just worried that these differences will bite developers that will assume that these methods are identical to the Object.* versions.

I'd like to hear Allen's opinion on this issue.

# Herby Vojčík (12 years ago)

Tom Van Cutsem wrote:

[+Allen]

2013/3/13 Nathan Wall <nathan.wall at live.com <mailto:nathan.wall at live.com>>

However, as a matter of principle, my argument is that
`Object.getOwnPropertyDescriptor` should, at the bare minimum,
return a descriptor that can be known to work in
`Object.defineProperty`.  If `Object.defineProperty` doesn't accept
it, then you `getOwnPropertyDescriptor` didn't really give me a
valid descriptor.

I think that this behavior (1) limits the creativity of developers
to define properties like `Object.prototype.get`, (2) is a potential
stumbling block, (3) has no real benefit -- really, there's not
anything positive about this behavior, and (4) forces developers who
want to support `Object.prototype.get` to add an extra layer of
cleaning before using `defineProperty`.

While the monkey-patching of Object.prototype ("don't do that!") is still the culprit, I agree that it would have been better if defineProperty looked only at "own" properties of the descriptor. I

No, there are legitimate uses of Object.create(descriptorTemplate) with descriptors.

# David Bruant (12 years ago)

Le 14/03/2013 08:51, Tom Van Cutsem a écrit :

[+Allen]

2013/3/13 Nathan Wall <nathan.wall at live.com <mailto:nathan.wall at live.com>>

However, as a matter of principle, my argument is that
`Object.getOwnPropertyDescriptor` should, at the bare minimum,
return a descriptor that can be known to work in
`Object.defineProperty`.  If `Object.defineProperty` doesn't
accept it, then you `getOwnPropertyDescriptor` didn't really give
me a valid descriptor.

I think that this behavior (1) limits the creativity of developers
to define properties like `Object.prototype.get`, (2) is a
potential stumbling block, (3) has no real benefit -- really,
there's not anything positive about this behavior, and (4) forces
developers who want to support `Object.prototype.get` to add an
extra layer of cleaning before using `defineProperty`.

While the monkey-patching of Object.prototype ("don't do that!") is still the culprit, I agree that it would have been better if defineProperty looked only at "own" properties of the descriptor.

In a previous message, Brandon Benvie mentioned he uses inheritance to reuse a property descriptor [1] (I think there was another quote of him, but I can't find it now). I can imagine it's a used pattern.

I almost always think of descriptors as "records" rather than "objects". Similarly, perhaps Object.getOwnPropertyDescriptor should have returned descriptors whose [[prototype]] was null.

It's true that Reflect.getOwnPropertyDescriptor and Reflect.defineProperty give us a chance to fix this. I'm just worried that these differences will bite developers that will assume that these methods are identical to the Object.* versions.

I doubt differences would be a good idea.

Maybe an idea would be for Object.defineProperty to call Attributes.@@iterate is user-defined so that a user can restrict what property descriptor properties are being traversed. If that's too heavy of a refactoring, maybe an ES6 map could be accepted as the 3rd argument of Object.defineProperty (with maps semantics, not objects semantics). This way, one could write the copy function as:

 function copy(from, to) {
     for (let name of Object.getOwnPropertyNames(from)){
         let desc = Object.getOwnPropertyDescriptor(from, name);
         desc[@iterator] = ownIterator; // is that the proper

syntax? I'm a bit lost :-/ Object.defineProperty(to, name, new Map(desc)); } }

ownIterator only iterates over own properties as its name indicates, so the Map will only list that. The extra map allocation isn't that big of a deal since it is very short-lived. It could be shared and cleared across iterations if necessary.

Nathan, how do you feel about such a solution?

David

[1] esdiscuss/2012-November/026081

# Brandon Benvie (12 years ago)

On 3/14/2013 2:12 AM, David Bruant wrote:

Le 14/03/2013 08:51, Tom Van Cutsem a écrit :

[+Allen]

2013/3/13 Nathan Wall <nathan.wall at live.com <mailto:nathan.wall at live.com>>

However, as a matter of principle, my argument is that
`Object.getOwnPropertyDescriptor` should, at the bare minimum,
return a descriptor that can be known to work in
`Object.defineProperty`.  If `Object.defineProperty` doesn't
accept it, then you `getOwnPropertyDescriptor` didn't really give
me a valid descriptor.

I think that this behavior (1) limits the creativity of
developers to define properties like `Object.prototype.get`, (2)
is a potential stumbling block, (3) has no real benefit --
really, there's not anything positive about this behavior, and
(4) forces developers who want to support `Object.prototype.get`
to add an extra layer of cleaning before using `defineProperty`.

While the monkey-patching of Object.prototype ("don't do that!") is still the culprit, I agree that it would have been better if defineProperty looked only at "own" properties of the descriptor. In a previous message, Brandon Benvie mentioned he uses inheritance to reuse a property descriptor [1] (I think there was another quote of him, but I can't find it now). I can imagine it's a used pattern.

I also mentioned I thought it was unlikely to be commonly used, since I've never seen it used besides some of my own code (which exists in a couple libraries used by few or just me). I think, though, the other thing I mentioned that you're thinking of is that methods on the Descriptor class prototypes can be useful. Here's a simple version that demonstrates potential utility:

const fields = new Set(['enumerable', 'configurable', 'writable', 'value', 'get', 'set', 'key']);

class Descriptor extends null { constructor(desc){ if (desc) { for (let [key, value] of items(desc)) { if (fields.has(key) && value === undefined || Object.prototype[key] !== value) { this[key] = value; } } } } hide(){ this.enumerable = false } lock(){ this.configurable = false; } define(object, key = this.key){ Object.defineProperty(object, key, this); } /** etc **/ }

class AccessorDescriptor extends Descriptor { setOn(object, value){ if (typeof this.set === 'function') { return call(this.set, object, value) !== false; } return false; } getOn(object){ if (typeof this.get === 'function') { return call(this.get, object); } } /** etc **/ }

# David Bruant (12 years ago)

Le 14/03/2013 17:01, Brandon Benvie a écrit :

I also mentioned I thought it was unlikely to be commonly used, since I've never seen it used besides some of my own code (which exists in a couple libraries used by few or just me).

Sincere apologies on missing an important part of your quote (I remember there was another message than the one I quoted, but I've been unable to find it) :-/

# Andrea Giammarchi (12 years ago)

this is an excellent "Point" Mark, I wish I mentioned this earlier in redefine.js

For already panicing developers sake, the problem addressed by Mark can be easily solved going explicitly over the property definition so that this won't work:

Object.freeze(Object.prototype); function Point(x, y) { this.x = x; this.y = y; } Point.prototype.toString = function() { return '<' + this.x + ',' + this.y + '>'; };

alert(new Point(1, 2)); // [object Object]

but this will: Object.defineProperty( Point.prototype, 'toString', {value: function() { return '<' + this.x + ',' + this.y + '>'; } });

alert(new Point(1, 2)); // <1,2>

br