Skip to content

Instantly share code, notes, and snippets.

@Jamesernator

Jamesernator/README.md

Last active December 6, 2024 02:00
Show Gist options
  • Save Jamesernator/dc9831f9c13d17ac9a75d1fe25a2e015 to your computer and use it in GitHub Desktop.
Save Jamesernator/dc9831f9c13d17ac9a75d1fe25a2e015 to your computer and use it in GitHub Desktop.
Download ZIP
Harden
Raw
README.md

See this comment for a revised version.

Harden - Fixing the override mistake

The override mistake

The override mistake is a problem introduced with ES5's ability to mark properties as non-writable. When a property on the prototype of an object is marked non-writable, it makes the property unaddable on the object.

A notable example of this is if we freeze (which marks all properties as non-writable) on Object.prototype.

Object.freeze(Object.prototype);

When we do this, it becomes impossible to set toString() on any objects that don't already have a .toString().

e.g. the following fails:

"use strict";
Object.freeze(Object.prototype);
const newObject = {};
newObject.toString = () => "Some string"; // TypeError

This is of particular concern to the SES proposal, which needs to freeze all builtins in order to guarantee security properties.

Solution: Overridable writable properties

In this proposal we propose a new descriptor member called overridable, this member may only be present when writable is set to false.

When overridable is present, non-writable properties on the prototype will be ignored when attempting to set them.

For example the following will print 10:

"use strict";
const proto = {};
Object.defineProperty(proto, 'example', { value: 5, writable: false, overridable: true });
const object = { __proto__: proto };
object.example = 10;
console.log(object.example);

Object.harden

For SES, we need to be able to able to freeze entire objects, but without creating the problems of the override mistake. As such a new function will be provided called Object.harden, this method acts just like Object.freeze, except it sets overridable: true on all descriptors (that are configurable).

For example, the following will just work now:

"use strict";
Object.harden(Object.prototype);
const object = {};
object.toString = () => "Hello world!";
console.log(object.toString()); // Hello world!

QUESTION: Should this simply be an option to freeze rather than a new method?

Hazard: Existing code relying on descriptors

If we expose overridable: boolean as a new property on descriptors returned from Object.getOwnPropertyDescriptor(s), will existing code break?

If so, should we add a new method to get overridable to Object (e.g. Object.getIsOverridable(object, name)), or should we expose overridable only when it is set to true.

@ljharb
Copy link

ljharb commented Sep 21, 2020

In this proposal we propose a new descriptor member called overridable, this member may only be present when writable is set to true.

Do you mean, only when set to false? Either way, I'd expect it to be allowed to be false explicitly, otherwise conditionally setting it to a boolean value gets a bit awkward.

How would/could this proposal be polyfilled?

@Jamesernator
Copy link
Author

Do you mean, only when set to false?

Yes, fixed.

Either way, I'd expect it to be allowed to be false explicitly, otherwise conditionally setting it to a boolean value gets a bit awkward.

Yes, setting to false should be fine, although I'd imagine setting to true only being allowed with writable: false (similar to how get/set are exclusive with value).

How would/could this proposal be polyfilled?

I'm not sure it could be polyfilled faithfully, similar to how simply fixing the override mistake itself can't be polyfilled.

@erights
Copy link

erights commented Sep 22, 2020

I like the general idea. However, some specifics need adjustment. Some thoughts.

We are not proposing harden be added to Object. Rather we're proposing that it be global and enabled by lockdown(). Once you start using harden you tend to call it from so many places that it needs to be said as short as possible. (If there were a word as clear with fewer letters, I'd be tempted to use that instead.) We also use harden to transitively freeze the primordials, which would work well with this proposal. Currently, we only repair the override mistake for the properties on the whitelist at https://github.com/Agoric/SES-shim/blob/master/packages/ses/src/enablements.js . With this proposal, all properties of all primordials would be override-mistake-safe in this manner, removing the need for the ad hoc whitelist.

Be aware that we're about to change the semantics of harden in ways that should not affect this proposal. Currently, harden transitively walks own properties, which it would continue to do. But it only does a correctness check on [[Prototype]] --- that after this harden completes, all hardened objects would inherit only from hardened objects. This complexity was motivated when we wanted harden to be useful outside of SES, which turned out to be a waste of time and effort, and was hard to explain. With harden only being enabled by lockdown we're going to return to the simpler semantics that it also transitively walks the [[Prototype]] chain upwards, hardening all the objects that hardened objects would inherit from, rather than just checking that they would be hardened.

Adding this protection against the override mistake to the semantics of a lockdown / harden proposal makes a lot of sense. However, we would normally not shim the pervasive protection from the override mistake, as that turns out to be expensive. The shim would likely continue the whitelist approach we're currently using. It works extremely well.

I am not in favor of adding a new descriptor member. Regarding whether we need to adjust the object property model at all, I gotta ask: Is it within the exotic behaviors allowed under the object invariants for an object to represent that it has a non-writable, non-configurable data property foo while still allowing an object that inherits from that exotic object to override it by assignment. We could test this by trying to write a proxy that does exactly that. If so, then harden should cause that change to the behaviors of objects that it freezes --- but only objects that it freezes. Any objects that are already frozen should not have this behavior changed by harden.

Whether and how to provide any reflective access to this, such as getIsOverridable, depends on the answer to the prior question. But some such reflective query is plausible.

@erights
Copy link

erights commented Sep 22, 2020

Answering your question at tc39/ecma262#1320 (comment)

@erights I've written a strawman gist for an Object.harden which allows non-writable properties to optionally be overridable. If this something you would be interesting in championing I would be happy to write up a full proposal with spec text.

Yes! I'd love to work with you on advancing such a proposal. Please!

@erights
Copy link

erights commented Sep 22, 2020 •
edited
Loading

@ljharb writes:

How would/could this proposal be polyfilled?

@Jamesernator correctly replies

I'm not sure it could be polyfilled faithfully, similar to how simply fixing the override mistake itself can't be polyfilled.

But to clarify, the override mistake can be polyfilled unfaithfully but practically, and is by Agoric, Salesforce, MetaMask, Node, Google, Bloomberg and others. Most started with either the Agoric version or the earlier version from the Google Caja project. The current Agoric version is at https://github.com/Agoric/SES-shim/blob/master/packages/ses/src/enable-property-overrides.js

It is unfaithful only in that the properties visibly change from data properties to accessor properties. This will break very little code, and so is a practical shim for most purposes --- as shown by the wide use of this technique.

@ljharb
Copy link

ljharb commented Sep 22, 2020

@erights unfortunately it does break es-abstract's GetIntrinsic, which is used by a number of my shims, and relies on the spec mandating that data properties be data properties. See ljharb/object.assign#79 for some context.

@Jamesernator
Copy link
Author

Jamesernator commented Sep 22, 2020 •
edited
Loading

I gotta ask: Is it within the exotic behaviors allowed under the object invariants for an object to represent that it has a non-writable, non-configurable data property foo while still allowing an object that inherits from that exotic object to override it by assignment.

The short answer is no. Basically you're forced to return false from the set trap (as per the invariants of [[Set]]). Although strictly speaking you can actually still use defineProperty within the set trap to set the value. It's just that in strict mode the assignment will still result in an error (despite being been set).

e.g.:

"use strict";
const o = {};
Object.defineProperty(o, 'toString', { value: () => "base value"});
const p = new Proxy(o, {
    set(target, prop, value, receiver) {
        const normalSet = Reflect.set(target, prop, value, receiver);
        if (!normalSet && !Reflect.has(receiver, prop)) {
            Reflect.defineProperty(receiver, prop, {
                value,
                writable: true,
                enumerable: true,
                configurable: true,
            });
        }
        return normalSet;
    },
});

const a = { __proto__: p };
try {
    a.toString = () => "overriden value"; // TypeError as set must return false
} catch {
    console.log(a.toString()); // Still works however
}

It is somewhat surprising to me that the invariant of [[Set]](Prop, Value, Receiver) requires a return of false in such a case, regardless of whether or not the Receiver is different to the target or not.

I am not in favor of adding a new descriptor member. Regarding whether we need to adjust the object property model at all

I understand the concern of adding a new descriptor member. Strictly speaking because the override mistake is actually a problem with the definition of "ordinary objects" (by way of [OrdinarySetWithOwnPropertyDescriptor]), we don't actually need to add it to the object meta-model. We only need to find a way for ordinary objects to mark their properties as overridable.

As a suggestion we could add a new slot [[OverridableProperties]] which is a set of all own properties which may be overridable. OrdinarySetWithOwnPropertyDescriptor would check it's passed property as to whether it is in this set before deciding to ignore [[Writable]] on inherited descriptors.

I'd imagine some API like Object.setOverridable(object, property) that adds the property to the [[OverridableProperties]] list. For non-ordinary objects this would either do nothing or throw an error (TODO: Investigate side-channels introduced by calling Object.setOverridable).

We'd need to take care to ensure there's no way to add a property to [[OverridableProperties]] if the property isn't configurable. We'd also need to take care to ensure all relevant builtin non-ordinary objects have this slot (e.g. %Object.prototype%).

@Jamesernator
Copy link
Author

Jamesernator commented Feb 17, 2021 •
edited
Loading

Based on the above comment, this is a possible candidate for v2:

Harden - Fixing the override mistake

The override mistake

The override mistake is a problem introduced with ES5's ability to mark properties as non-writable.
When a property on the prototype of an object is marked non-writable, it makes the property unaddable on the object.

A notable example of this is if we freeze (which marks all properties as non-writable) on Object.prototype.

Object.freeze(Object.prototype);

When we do this, it becomes impossible to set toString() on any objects that don't already have a .toString().

e.g. the following fails:

"use strict";
Object.freeze(Object.prototype);
const newObject = {};
newObject.toString = () => "Some string"; // TypeError

This is of particular concern to the SES proposal, which needs to freeze all builtins in order to guarantee security properties.

Solution: Add [[OverridableProperties]] internal slot, and set it during harden

The override mistake as it turns out is not a fundmental rule in the meta-object model, but rather a consequence of how implementations of [[Set]] handle non-writable descriptors. In particular OrdinarySetWithOwnDescriptor returns false if the descriptor is non-writable, regardless of whether the receiver could be written to or not.

A previous pull request showed how the override mistake may be turned off entirely, however this turns out to not be web compatible. What the PR does show us though is that we could change OrdinarySetWithOwnDescriptor to have an OPT-IN mechanism for fixing the override mistake.

As such this proposal proposes a single new internal slot on ordinary objects [[OverridableProperties]], this slot is a list of properties that are allowed to be overriden in OrdinarySetWithOwnDescriptor. In order to populate this list, we add a new global method harden, this function performs Object.freeze on the object and populates [[OverridableProperties]] with all properties that use a value (not get/set) descriptor.

For example the following will print 10:

"use strict";
const proto = { x: 5, [Symbol.iterator]() {}, get foo() { return 12 } };
harden(proto);
const object = { __proto__: proto };
object.example = 10;
console.log(object.example); // 10
object[Symbol.iterator] = function*() { yield 1; }
console.log([...object]); // [1]

In this example, once proto is hardened it's [[OverridableProperties]] will contain "x" and Symbol.iterator, this allows them to be overriden. This would be accomplished effectively the same as in the previous pull request to fix the override mistake, except we check first that the property P is within [[OverridableProperties]].

FAQ

What does harden do on exotic objects?

This is TBD, but either simply behaving like Object.freeze or throwing an error are two possible strategies.

What about [[OverridableProperties]] on builtin exotic objects?

This is TBD.

What about [[OveriddableProperties]] on other exotic objects?

We don't propose changing those objects in this proposal, most objects in typical environments are already ordinary objects so they will automatically gain this behaviour. If there are any signficant host exotic objects that need this behaviour they will need to be changed by the host.

Can we override individual properties, rather than all or nothing?

This is also TBD, the proposal currently only does all properties as this is probably the most common case (e.g. in SES lockdown will simply call harden on all transitively reachable objects). If there are use cases for individual properties, or excluding properties this might be open to change.

@brusshamilton
Copy link

Why have an extra slot for [[OverridableProperties]] instead of adding an 'overridable' property to the descriptor of the prototype? Object.freeze could leave the overridable property to false (triggering current behavior) and "harden" or whatever could set it as true.

Then the modification to OrdinarySetWithOwnDescriptor would just be to move the check at 2.a to after 2.c and change it to "if ownDesc.[[Writable]] is false and (existingDescriptor is not undefined or ownDesc.[[Overridable]] is false), return false"

@Jamesernator
Copy link
Author

Jamesernator commented Jun 16, 2022 •
edited
Loading

Why have an extra slot for [[OverridableProperties]] instead of adding an 'overridable' property to the descriptor of the prototype? Object.freeze could leave the overridable property to false (triggering current behavior) and "harden" or whatever could set it as true.

That's essentially the proposal I suggested in the initial post, but this changes all objects not just ones with [[OverridableProperties]]. As mentioned by @erights :

I am not in favor of adding a new descriptor member. Regarding whether we need to adjust the object property model at all, I gotta ask: Is it within the exotic behaviors allowed under the object invariants for an object to represent that it has a non-writable, non-configurable data property foo while still allowing an object that inherits from that exotic object to override it by assignment.

And the answer to the query given there, is actually yes, the current object model already permits "override" behaviour as can proved by creating a proxy that does so:

const frozenProto = Object.freeze({
    toString() {
        return "[Object object]";
    }
});

const obj = {
    __proto__: frozenProto,
};

const override = new Proxy(obj, {
    // This is overly simplified as it doesn't actually invoke
    // getters/setters on the prototype, but the above proposal
    // is a bit more specific with the behaviour here
    set() {
        return true;
    },
});

// Works
override.toString = () => "override";
// Throws an error, this is the "override mistake"
obj.toString = () => "obj";

Hence, we can fix the override mistake without changing the object model, we just need to change the behaviour of [[Set]] for "Ordinary objects" to allow setting the property (under appropriate conditions).

And this is most easily accomplished by having an [[OverridableProperties]] internal slot or something vaguely similar. Hence the second suggested proposal I made above.

@Jamesernator
Copy link
Author

I should point out one reason something like [[OveridableProperties]] is needed on the prototype rather than just unconditionally returning true in cases where there's an unwritable property on the prototype is that this changes the behaviour of existing code (and that exact solution has been previously proposed and rejected).

Rather by having Object.harden or whatever as an opt-in, existing code will be unchanged, but new code can still freely opt-in to overridable properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment