Discuss
  • Subclassing ES6 objects with ES5 syntax.

Subclassing ES6 objects with ES5 syntax.

# C. Scott Ananian (9 years ago)

Is there any way to access new.target using ES5 syntax?

It appears that the "correct" way to create a subclass using ES5 syntax is:

function MyPromise(executor) {
  var self = Reflect.construct(Promise, [executor], new.target);
  return self;
}
Object.setPrototypeOf(MyPromise, Promise);

But since new.target isn't accessible, we have to do something like:

function MyPromise(executor) {
  var self = Reflect.construct(Promise, [executor], MyPromise); // <-- THIS
  return self;
}
Object.setPrototypeOf(MyPromise, Promise);

which works for only a single level of subclassing. That is, it allows us to create and instantiate MyPromise, but now nobody can subclass MyPromise. That's too bad.

Is there any way around this? --scott

ps. Use case: My prfun package on npm subclasses Promise in order to add all the useful utility helpers without stomping on the global Promise object. I'd like to do so in a way which is compatible with both native ES6 promises (if they are available) and properly-written ES5 shims.

# Erik Arvidsson (9 years ago)

new.target is available in functions.

# Caitlin Potter (9 years ago)

So, why exactly is SuperCall forbidden in function bodies? If people work around it with Reflect.construct anyways, it just seems nice to use SuperCall instead — at least if new.target is defined

# Claude Pache (9 years ago)

Le 24 avr. 2015 à 03:25, Caitlin Potter <caitpotter88 at gmail.com> a écrit :

So, why exactly is SuperCall forbidden in function bodies? If people work around it with Reflect.construct anyways, it just seems nice to use SuperCall instead — at least if new.target is defined

Note that super(...) will try to initialise the this binding—which won’t work in plain functions.

# Alex Kocharin (9 years ago)

An HTML attachment was scrubbed... URL: esdiscuss/attachments/20150424/0db86bd0/attachment

# Andrea Giammarchi (9 years ago)

Not exactly ... if it's an Array, as example, the moment you slice/map/splice/concat, etc will return an instanceof Array, not an instance of whatever you have sublcassed.

# Alex Kocharin (9 years ago)

An HTML attachment was scrubbed... URL: esdiscuss/attachments/20150424/4b35544b/attachment

# Andrea Giammarchi (9 years ago)

So you should do the same with Promise methods but then you'll see overall a quite consistent performance drop when using all these subclasses.

We've been trying for about 10 years now ( dean.edwards.name/weblog/2006/11/hooray ) and yet we don't have a working and performing solution, which I guess is why somebody still pollutes global prototypes, everything becomes easier, including the ability to still use common literals to define objects or lists.

Oh well :-)

Best

# C. Scott Ananian (9 years ago)

On Fri, Apr 24, 2015 at 10:46 AM, Andrea Giammarchi < andrea.giammarchi at gmail.com> wrote:

So you should do the same with Promise methods but then you'll see overall a quite consistent performance drop when using all these subclasses.

Well, not exactly. The Promise methods are all written to use the correct constructor (perhaps a (welcome) artifact of the fact that they were spec'ed before this change in instantiation mechanism?):

$ iojs
> var P2 = function P2(x) { var self = new Promise(x);

Object.setPrototypeOf(self, P2.prototype); return self; }
undefined
> Object.setPrototypeOf(P2, Promise);
[Function: P2]
>   P2.prototype = Object.create(Promise.prototype);
{}
>   P2.prototype.constructor = P2;
{}
> p = P2.resolve(5);
{}
> p instanceof Promise

true
> p instanceof P2

true
> p2 = p.then(function(){return 6; });
{}
> p2 instanceof P2

true

It's true that the Object.setPrototypeOf inside the constructor is quite unfortunate for performance. :( But at least you don't have to repeat that inside each instance method.

Does anyone have a better idea for creating an ES6-compatible subclass using ES5 syntax, without resorting to Object.setPrototypeOf when every instance is constructed?

# C. Scott Ananian (9 years ago)

On Fri, Apr 24, 2015 at 11:31 AM, C. Scott Ananian <ecmascript at cscott.net>

wrote:

On Fri, Apr 24, 2015 at 10:46 AM, Andrea Giammarchi < andrea.giammarchi at gmail.com> wrote:

So you should do the same with Promise methods but then you'll see overall a quite consistent performance drop when using all these subclasses.

Well, not exactly. The Promise methods are all written to use the correct constructor (perhaps a (welcome) artifact of the fact that they were spec'ed before this change in instantiation mechanism?):

Hm. Well, almost: Promise.resolve uses the hidden [[PromiseConstructor]] property, which is initialized from new.target at construction time. So we need to use a native Reflect.construct to ensure that is set correctly (or just work around it by overriding Promise.resolve while we wait for engines to implement ES6 class semantics).

This begs the question: what, exactly, is Promise.resolve accomplishing by using a different mechanism for determining the "class of the constructor" than every other promise-related method. In ecmascript#2513 the answer given was "for allowing tamper-proof casting via private slots." But in the code given previously, I've used Object.setPrototypeOf to effectively change the type of the object -- but [[PromiseConstructor]] doesn't follow along. Seems a little ineffectual, although perhaps if you care about "tamper-proof casting" you've already disabled access to Object.setPrototypeOf?

I think I'd rather see Promise.resolve changed to use this.constructor instead of this.[[PromiseConstructor]], like every other Promise-related method. Can someone who feels strongly otherwise give me the use case for [[PromiseConstructor]] existing?

# Domenic Denicola (9 years ago)

From: es-discuss [mailto:es-discuss-bounces at mozilla.org] On Behalf Of C. Scott Ananian

But in the code given previously, I've used Object.setPrototypeOf to effectively change the type of the object -- but [[PromiseConstructor]] doesn't follow along.

That is exactly the kind of tampering that the unforgable [[PromiseConstructor]] mechanism is meant to prevent.

# C. Scott Ananian (9 years ago)

Inadvertently moved discussion off-list; requoting on list:

On Sat, Apr 25, 2015 at 4:33 PM, Domenic Denicola <d at domenic.me> wrote:

From: es-discuss [mailto:es-discuss-bounces at mozilla.org] On Behalf Of C. Scott Ananian

But in the code given previously, I've used Object.setPrototypeOf to effectively change the type of the object -- but [[PromiseConstructor]] doesn't follow along.

That is exactly the kind of tampering that the unforgable [[PromiseConstructor]] mechanism is meant to prevent.

Sure, but why? If it is now quacking like a duck, returning it from GoosePromise.resolve as if it were still a goose seems wrong.

Because then you could break invariants by doing e.g. var syncThenable = { then(f) { f(5); }; syncThenable.constructor = Promise.

Oh, I don't think looking at the constructor is right, either. It seems you should perhaps be looking at the object's prototype chain instead, like instanceof does.

The invariant I expect to be preserved is that GoosePromise.resolve(x) gives me a GoosePromise back, regardless of what sort of x I give it. But instead if I give it an x which used to be a goose but is now a duck, I get back a duck instead of a goose.

And this is a real use case: my prfun package creates a BoundPromise subclass, but because I'm doing it ES5 style without access to new.target, Promise.resolve(x) returns a BoundPromise (and BoundPromise.resolve(x) returns a Promise) because everything "was once a Promise".

This particular case will probably be less frequent once we can do "real" subclassing in JavaScript engines... but it still seems to indicate to me that this basic idea is broken. What things "once were" seems to be some weird property with no resemblance to what they are now.

# C. Scott Ananian (9 years ago)

On Sat, Apr 25, 2015 at 5:03 PM, Domenic Denicola <d at domenic.me> wrote:

There needs to be an unforgable brand property such that only objects created with new XPromise() pass XPromise.resolve. It is not a use case to allow building an object ES5 style in pieces to pass the brand check. Such objects should be coerced to real promises.

But can you give me an actual use case, please? The ostensible security property is pretty moot given Object.setPrototypeOf.

In this case, the promises are "real promises". They were created with new Promise. But then the prototype was changed. So in effect I've forged the brand -- they are not actually Promise any more, they are something else.

And I can forge this arbitrarily using Reflect.construct, which lets me set new.target (and thus [[PromiseConstructor]]`) to absolutely anything I want.

If the brand property is not actually unforgeable, what use is it? It seems like some broken piece of an idea that doesn't actually work.

But maybe it does work and serve a purpose -- that's why I'm asking: can we point to someone who is actually using this functionality for a specific purpose? And is it working for them?

# Kevin Smith (9 years ago)

I think I'd rather see Promise.resolve changed to use this.constructor instead of this.[[PromiseConstructor]], like every other Promise-related method. Can someone who feels strongly otherwise give me the use case for [[PromiseConstructor]] existing?

I'll give it a shot.

"Promise.resolve", executed on a constructor C, with argument x, should only return x if x was created by C.[[Construct]]. Otherwise it should create an instance of C which is resolved with x.

If we used "x.constructor" to determine the actual constructor, then someone could just change the "constructor" property for x and fool someone who wrote "C.resolve(x)" and expected to get an instance of C back.

It would be an unreliable, unsound conversion function.

Proper subclassing requires subclassing support from the engine.

# C. Scott Ananian (9 years ago)

On Sat, Apr 25, 2015 at 6:58 PM, Kevin Smith <zenparsing at gmail.com> wrote:

I think I'd rather see Promise.resolve changed to use this.constructor

instead of this.[[PromiseConstructor]], like every other Promise-related method. Can someone who feels strongly otherwise give me the use case for [[PromiseConstructor]] existing?

I shouldn't have written this.constructor here. I think looking at the object's prototype would be preferable.

"Promise.resolve", executed on a constructor C, with argument x, should only return x if x was created by C.[[Construct]]. Otherwise it should create an instance of C which is resolved with x.

"was created by C.[[Construct]] with NewTarget equal to C" is roughly how it is written currently. But I think "should only return x if x is an instanceof C" (or "if the prototype of C is exactly C.prototype") would be the most logical behavior, since:

If we used "x.constructor" to determine the actual constructor, then someone could just change the "constructor" property for x and fool someone who wrote "C.resolve(x)" and expected to get an instance of C back.

It would be an unreliable, unsound conversion function.

I don't think this argument holds water since:

x = Reflect.construct(Promise, x, C);

is another fine way to fool someone who wrote "C.resolve(x)" and expected to get an instance of C back.

What's more, I can then do:

Object.setPrototype(x, SomeArbitraryThing);

and still fool C.resolve, and now x behaves nothing like a Promise (much less a C).

So we've got an unreliable, unsound conversion function. What is it good for?

If our conversion is inherently "unsound", we might as well have it preserve duck typing. If it's currently quacking like a duck (that is, x.prototype = Duck.prototype), then let's just call it a duck and be done with it. Adding complexity to try to "preserve soundness" isn't actually working.

Again: but maybe I'm wrong. Can someone with a security-sensitive application explain how Promise.resolve is actually the right thing for them?

# Domenic Denicola (9 years ago)

It's possible Reflect.construct has introduced a security hole that was not present before the recent instantiation reform. Hopefully Mark can comment more.

# C. Scott Ananian (9 years ago)

On Sun, Apr 26, 2015 at 12:56 AM, Domenic Denicola <d at domenic.me> wrote:

It's possible Reflect.construct has introduced a security hole that was not present before the recent instantiation reform. Hopefully Mark can comment more.

Note that, even without Reflect.construct:

x = new C(); // C some subclass of Promise.
Object.setPrototypeOf(x, SomeArbitaryThing);
y = C.resolve(x); // doesn't actually return an instanceof C

seems somewhat surprising. If there are security properties involved, the shortcut taken by Promise.resolve just seems a bit dodgy. Better just to say y = new C(function(r) { r(x); }) (and take the slight performance hit) if your security mechanism absolutely requires y to be a C with no funny business. --scott

ps. For what it's worth, my prfun package has what I think is a more normal use-case for Promise.resolve -- I define a Promise subclass with extra utility methods: try, reduce, map, done, and so on. The return values from all of these methods, and for the standard then, catch, all, etc methods, preserve the instance type, so I know that all of the utility methods will continue to be available as I chain promises. But at an API boundary, when x comes from some outside source, I use x = PrFunPromise.resolve(x) to ensure that the utility methods are available on x.

I don't need any special tamper-proofing here, so a straightforward implementation of Promise.resolve would work fine. Checking whether x.__proto__ == PrFunPromise.prototype would be ok. But right now [[PromiseConstructor]] is giving me mild fits because v8 has implemented enough of the Promise spec to be storing [[PromiseConstructor]], but not enough of the ES6 class mechanism for me to actually set new.target and [[PromiseConstructor]] correctly in my PrFunPromise subclass. This will eventually resolve itself when the engines can implement Reflect.construct and/or full subclass support. But it led me to question what [[PromiseConstructor]] was supposed to be doing in the first place, and whether it was doing it successfully.

# Kevin Smith (9 years ago)

x = Reflect.construct(Promise, x, C);

is another fine way to fool someone who wrote "C.resolve(x)" and expected to get an instance of C back.

Thanks for pointing this out. I believe the ability to use an arbitrary newTarget parameter for Reflect.construct is breaking the intent of Promise.resolve. Using an arbitrary "newTarget" is also problematic for the private fields proposal.

It seems to me that Reflect.construct has been given a capability that is not otherwise expressible with ES6 syntax, and that gap is problematic.

Maybe I've missed some context though. Mark, Allen, any thoughts?

Looking over the Reflect namespace, I also see that Reflect.get and Reflect.set have been given powers not expressible with syntax: the receiver does not have to be a prototype parent of the target.

# Claude Pache (9 years ago)

Le 26 avr. 2015 à 00:58, Kevin Smith <zenparsing at gmail.com> a écrit :

If we used "x.constructor" to determine the actual constructor, then someone could just change the "constructor" property for x and fool someone who wrote "C.resolve(x)" and expected to get an instance of C back.

Note that if you want to protect yourself against tampering the constructor property, you should seriously consider to protect yourself against tampering the then property. That means that you should at the very least execute preventExtensions on your promise anyway.

# Tom Van Cutsem (9 years ago)

2015-04-28 5:01 GMT+02:00 Kevin Smith <zenparsing at gmail.com>:

Looking over the Reflect namespace, I also see that Reflect.get and Reflect.set have been given powers not expressible with syntax: the receiver does not have to be a prototype parent of the target.

Did you mean "the receiver does not have to be a prototype [child] of the target"? I would expect that to be the common case. Anyway, with proto / setPrototypeOf as part of the language, two objects being in a parent-child prototype relationship is not an invariant you can always count on. Additionally, if the property is an accessor, the receiver argument is passed as the this binding into the accessor. But it's easy enough to call getOwnPropertyDescriptor, extract the getter or setter, and call it with a this-binding that also isn't in a prototype relation with the object holding the accessor. So, for Reflect.{get|set} at least, I don't see an issue.

# Allen Wirfs-Brock (9 years ago)

On Apr 27, 2015, at 8:01 PM, Kevin Smith wrote:

x = Reflect.construct(Promise, x, C);

is another fine way to fool someone who wrote "C.resolve(x)" and expected to get an instance of C back.

Thanks for pointing this out. I believe the ability to use an arbitrary newTarget parameter for Reflect.construct is breaking the intent of Promise.resolve. Using an arbitrary "newTarget" is also problematic for the private fields proposal.

It seems to me that Reflect.construct has been given a capability that is not otherwise expressible with ES6 syntax, and that gap is problematic.

Maybe I've missed some context though. Mark, Allen, any thoughts?

Note that the exact same kind of circumvention could have been accomplished with the Promise design prior to instantiation reform adding the 3rd argument to Construct:

x = C[Symbol.create]().apply(Promise, x);

As to "breaking the intent of Promise.resolve", we have to look closely at what that intent actually is and whether "intent" is equivalent to "guarantee". Apparently the intent of Foo.resolve is to coerce the argument value to a the same type of promise object as produced by the Foo constructor (assuming that Foo is a constructor that inherits from Promise). But "same type" is a very elusive concept of in JS. Is an object that is initially instantiated by Foo still of the "same type" as other objects created by Foo if it has been mutated that such that it no longer has Foo.prototype on its prototype chain and/or shares no properties in common with the other objects created using Foo. If an object created by Foo still the "same type" if additional properties are added to it. What if addiotnal properties are added via subclassing. Why is an instance of a subclass of Foo the "same type" as an instance of Foo. If not, why not? Finally, note Foo.resolve may or may not be the same method as Promise.resolve, so whatever definition of "same type" that is used by Promise.resolve is not necessarily the definition used by Foo.resolve.

So, ES6 Promises reflect a specific set of design decisions, including a specific definition of "same type" that appears to exist solely for use by Promise.resolve. All that design guarantees is that the object has an certain specific internal slot whose value is tested in a specific way by Promise.resolve. It doesn't guarantee that the object is a well-promise or that it has any specific other characteristics that are expected of Foo objects. It's hard to extrapolate from that specific design to the underlying intent of the designer.

This is the nature of JS objects. There are very few guarantees about anything. If you want guarantees you need to lock things down and probably add code to test for the guaranteed conditions. If you want guarantees, you probably aren't going to use polymeric then-ables or any other form of OO polymorphism. but, it's the developers choice.

Looking over the Reflect namespace, I also see that Reflect.get and Reflect.set have been given powers not expressible with syntax: the receiver does not have to be a prototype parent of the target.

The Receiver argument to these Reflect.get/set/construct is necessary for many Proxy handler use cases.

These are simply a reification of the ES MOP api and an important use case of them is to define exotic objects whose behaviors differ from what can be directly expressed using ES language level syntax.

It's a trade-up, if you want to allow ES programmer to express things beyond that the syntactic language supports you can't limit that expression to what the syntax allows.

# C. Scott Ananian (9 years ago)

On Tue, Apr 28, 2015 at 2:41 PM, Allen Wirfs-Brock <allen at wirfs-brock.com>

wrote:

So, ES6 Promises reflect a specific set of design decisions, including a specific definition of "same type" that appears to exist solely for use by Promise.resolve. All that design guarantees is that the object has an certain specific internal slot whose value is tested in a specific way by Promise.resolve. It doesn't guarantee that the object is a well-promise or that it has any specific other characteristics that are expected of Foo objects. It's hard to extrapolate from that specific design to the underlying intent of the designer.

I think this is exactly my point. I understand what the original intent was, but the result is (it seems to me) a wart. Promise.resolve now uses its own idiosyncratic definition for "same type" which isn't shared by any other JavaScript objects. What's more, it's not clear that its idiosyncratic definition actually accomplishes what it was meant to.

It seems to me that this is an incomplete feature that should have been dropped from the spec. I apologize for not noticing this sooner.

Folks who want some sort of "tamper-proof" Promises should probably build them in user-space on top of the other abstractions which are provided. --scott

ps. I note that core-js/babel have already implemented the fix I proposed above, of looking at the prototype chain instead of [[PromiseConstructor]]: zloirock/core-js/blob/v0.9.4/modules/es6.promise.js#L194

# Kevin Smith (9 years ago)

So, ES6 Promises reflect a specific set of design decisions, including a specific definition of "same type" that appears to exist solely for use by Promise.resolve. All that design guarantees is that the object has an certain specific internal slot whose value is tested in a specific way by Promise.resolve. It doesn't guarantee that the object is a well-promise or that it has any specific other characteristics that are expected of Foo objects. It's hard to extrapolate from that specific design to the underlying intent of the designer.

I don't see any rational intent for the current design other than:

Only return x if x was constructed by C.

The current semantic is:

Only return x if x was constructed by C, or any code with a reference to C arbitrarily chose that it should appear to have been constructed by C.

Allowing an object to pass a nominal type test for a constructor, where the object was not initialized by that constructor seems bad.

What is the use case for third argument to Reflect.construct?

# Allen Wirfs-Brock (9 years ago)

On Apr 28, 2015, at 1:08 PM, Kevin Smith wrote:

What is the use case for third argument to Reflect.construct?

The primary use case is emulating the super() call semantics in a Proxy construct trap handler.

# Brendan Eich (9 years ago)

C. Scott Ananian wrote:

It seems to me that this is an incomplete feature that should have been dropped from the spec.

Seems like.

I apologize for not noticing this sooner.

No need -- thanks for finding it.

Domenic, Kevin: the concern about Reflect.construct seems misplaced, but in any event, the issue C. Scott raises wants addressing on its own. WDYT?

# Kevin Smith (9 years ago)

Domenic, Kevin: the concern about Reflect.construct seems misplaced, but in any event, the issue C. Scott raises wants addressing on its own. WDYT?

Yeah, sorry for dwelling on Reflect.construct so much (it's in my mind for other reasons).

So what would the ideal Promise.resolve semantics do? I'm not sure, maybe use SpeciesConstructor instead of [[PromiseConstructor]]?

# C. Scott Ananian (9 years ago)

On Wed, Apr 29, 2015 at 1:06 AM, Brendan Eich <brendan at mozilla.org> wrote:

Kevin Smith wrote:

So what would the ideal Promise.resolve semantics do? I'm not sure, maybe use SpeciesConstructor instead of [[PromiseConstructor]]?

This removes the wart in my view, has no less integrity. C. Scott?

Making this concrete, here would be the new text for 25.4.4.5 Promise.resolve(x):

The resolve function returns either a new promise resolved with the passed argument, or the argument itself if the argument is a promise produced by this constructor.

  1. Let C be the this value.
  2. If IsPromise(x) is true, a. Let constructor be the value of SpeciesConstructor(x, %Promise%)
b. If SameValue(constructor, C) is true, return x.
  1. If Type(C) is not Object, throw a TypeError exception.
  2. Let S be Get(C, @@species).
  3. ReturnIfAbrupt(S).
  4. If S is neither undefined nor null, let C be S.
  5. Let promiseCapability be NewPromiseCapability(C)

[...remainer elided...]

Step 2a is the only change. (It was previously "Let constructor be the value of x's [[PromiseConstructor]] internal slot.")

Thinking this through, the purpose of the @@species property, from the spec: "Promise prototype methods normally use their this object’s constructor to create a derived object. However, a subclass constructor may over-ride that default behaviour by redefining its @@species property."

So with this change, P.resolve() is guaranteed to return an object whose constructor property is P.@@species, whether it takes the shortcut in step 2 or not. That seems pleasantly less warty.

The alternative (implemented in core-js) is:

  1. If IsPromise(x) is true, a. Let proto be the value of x.[[Prototype]]
b. If SameValue(proto, GetPrototypeFromConstructor(C,

"%PromisePrototype%")) is true, return x.

But that seems to break the expected behavior of @@species. That is, a promise subclass P1 with P1.@@species == P2 would shortcut objects of type P1 through P1.resolve() but return objects of type P2 if the shortcut wasn't taken. (Forgive the looseness of the word "type" here.)

That seems somewhat surprising. But I don't have a concrete use case in mind for setting Promise[@@species]. Perhaps someone else has one, and can say what behavior they'd prefer to see?

Lacking such a justification, the first alternative (using [[SpeciesConstructor]]) seems to be more consistent with the rest of the spec and less surprising. So that has my vote.

# Allen Wirfs-Brock (9 years ago)

On Apr 29, 2015, at 8:44 AM, C. Scott Ananian wrote:

On Wed, Apr 29, 2015 at 1:06 AM, Brendan Eich <brendan at mozilla.org> wrote: Kevin Smith wrote: So what would the ideal Promise.resolve semantics do? I'm not sure, maybe use SpeciesConstructor instead of [[PromiseConstructor]]?

This removes the wart in my view, has no less integrity. C. Scott?

Making this concrete, here would be the new text for 25.4.4.5 Promise.resolve(x): The resolve function returns either a new promise resolved with the passed argument, or the argument itself if the argument is a promise produced by this constructor.

  1. Let C be the this value.
  2. If IsPromise(x) is true, a. Let constructor be the value of SpeciesConstructor(x, %Promise%) b. If SameValue(constructor, C) is true, return x.
  3. If Type(C) is not Object, throw a TypeError exception.
  4. Let S be Get(C, @@species).
  5. ReturnIfAbrupt(S).
  6. If S is neither undefined nor null, let C be S.
  7. Let promiseCapability be NewPromiseCapability(C) [...remainer elided...]

Step 2a is the only change. (It was previously "Let constructor be the value of x's [[PromiseConstructor]] internal slot.")

But SpeciesConstructor (or any access to x's @@species) goes through x.constructor and my recollection is that the motivation for adding [[PromiseConstructor]] was that 'constructor' was not sufficiently tamper proof. From that perspective, it seems that SpeciesConstructor is actually worse than just accessing x.constructor.

Also, in a private message Mark Miller mentioned that the primarily security invariant he's concerned about really relates to the behavior of the then method of the object returned by Promise.resolve(x). Neither testing construct or SpeciesConstructor really tells you anything about then. It seems that the root problem here is trying to apply nominal type based reasoning to JS.

# Mark S. Miller (9 years ago)

I have not responded on list yet because I haven't yet been able to find the time to absorb this thread. But since Allen mentioned it, what I wrote was:

The invariant I am interested in:

In a realm where we (the trusted defender who runs first) make Promise defensive as follows

  • Freeze everything primordial, as SES does

  • Make a DefensivePromise subclass of Promise that differs minimally, hopefully only by ensuring that its instances are frozen.

  • "Promise = DefensivePromise;" do "Promise" below refers to DefensivePromise

  • Freezing whitelisted global properties, as SES currently does for ES5 globals, but for ES6 including "Promise"

then it must be the case that

Promise.resolve(anything).then(anycallback)

for an anything provided by a potential attacker, when executed in the middle of a turn does not call callback during that turn. If it calls anycallback at all, it calls it back as a later turn, i.e., in a later turn starting from an empty stack.

# C. Scott Ananian (9 years ago)

On Wed, Apr 29, 2015 at 12:49 PM, Allen Wirfs-Brock <allen at wirfs-brock.com> wrote:

Also, in a private message Mark Miller mentioned that the primarily security invariant he's concerned about really relates to the behavior of the then method of the object returned by Promise.resolve(x). Neither testing construct or SpeciesConstructor really tells you anything about then. It seems that the root problem here is trying to apply nominal type based reasoning to JS.

I agree that is the root problem. That is why [[PromiseConstructor]] is misguided, as (in their own way) is testing x.[[Prototype]] and testing x.then. We could imagine rewriting step 2 to test all three of these. And then maybe we should think about proxies as well! But rather than add some complicated test to try to do something which doesn't actually accomplish its purpose, better to use the simple/expected/consistent thing, which seems (at this point) to be SpeciesConstructor. And that seems to make setting Promise.@@species "work right" as well, for what that's worth.

# C. Scott Ananian (9 years ago)

On Wed, Apr 29, 2015 at 1:00 PM, Mark S. Miller <erights at google.com> wrote:

The invariant I am interested in:

In a realm where we (the trusted defender who runs first) make Promise defensive as follows

  • Freeze everything primordial, as SES does

  • Make a DefensivePromise subclass of Promise that differs minimally, hopefully only by ensuring that its instances are frozen.

  • "Promise = DefensivePromise;" do "Promise" below refers to DefensivePromise

  • Freezing whitelisted global properties, as SES currently does for ES5 globals, but for ES6 including "Promise"

then it must be the case that

Promise.resolve(anything).then(anycallback)

for an anything provided by a potential attacker, when executed in the middle of a turn does not call callback during that turn. If it calls anycallback at all, it calls it back as a later turn, i.e., in a later turn starting from an empty stack.

How about:

var goodPromises = new WeakSet();
class DefensivePromise {
  constructor(x) {
    super(x);
    Object.freeze(x);
    // check this.constructor here if you are paranoid.
    goodPromises.add(this);
  }
  resolve(x) {
    if (goodPromises.has(x)) {
      return super.resolve(x);
    }
    return new DefensivePromise(function(r){r(x);});
  }
}

Doesn't seem like this needs special support in the Promise spec.

Note that the goodPromises set won't be fooled by passing in DefensivePromise as new.target to Promise without actually running the DefensivePromise constructor.

# Mark S. Miller (9 years ago)

On Wed, Apr 29, 2015 at 10:26 AM, C. Scott Ananian <ecmascript at cscott.net> wrote:

How about:

Isn't this still vulnerable to the Promise.resolve attack? IIUC, this attack enables the attacker to cause this.constructor to lie, so how would checking it help?

# Mark S. Miller (9 years ago)

I think your approach is on the right track. How about the following?

Anyone see a way to attack it?

const goodPromises = new WeakSet();
class DefensivePromise {
  constructor(x) {
    super(x);
    if (new.target === DefensivePromise) {
      Object.freeze(this);
      goodPromises.add(this);
    }
  }
  static resolve(x) {
    if (goodPromises.has(x)) {
      return x;  // should be equiv to super.resolve(x);
    }
    return new DefensivePromise(r => {r(x)});
  }
}

Note a few typos fixed and a few simplifications, all of which you should double check.

# C. Scott Ananian (9 years ago)

On Wed, Apr 29, 2015 at 1:37 PM, Mark S. Miller <erights at google.com> wrote:

Isn't this still vulnerable to the Promise.resolve attack? IIUC, this attack enables the attacker to cause this.constructor to lie, so how would checking it help?

Because I only allow DefensivePromise.resolve to check this.constructor (via the call to super.resolve) if I know that this.constructor is not lying (because I froze this.constructor before adding it to goodPromises). All other promises get wrapped in a new DefensivePromise, which should enforce the desired "not on this turn" invariant.

In this particular example, I'm not exporting DefensivePromise, just using it internally to sanitize "anything provided by a potential attacker" (while preserving efficiency if the attacker is in good faith giving my promises back to me). If you want to export DefensivePromise and allow malicious code to subclass it, then you should perform some additional checks after freezing and before adding to goodPromises, perhaps something like:

  constructor(x) {
    super(x);
    Object.freeze(this);
    if (this.constructor==DefensivePromise && this.then ===
DefensivePromise.prototype.then && /*) {
      goodPromises.add(this);
    }
  }

You probably need to make the test a little bit more careful, in case then is a malicious getter or something like that. But my point is that you can do all this in userland, you don't need to expose its complexity or bake any of it into the Promise spec.

# C. Scott Ananian (9 years ago)

On Wed, Apr 29, 2015 at 2:07 PM, Mark S. Miller <erights at google.com> wrote:

I think your approach is on the right track. How about the following?

Assuming that you don't export DefensivePromise to the attacker, this is fine. Otherwise, I think this is still vulnerable to Reflect.construct lying about new.target:

class BadPromise extends DefensivePromise {
  then(r) { r(); r(); }
}
var bp = Reflect.construct(BadPromise, DefensivePromise);

Since it's Promise.then you care about, I think the approach in my previous message (where then is tested directly) is preferable.

# Mark S. Miller (9 years ago)

I do indeed need to expose DefensivePromise under the global name "Promise", replacing the builtin. Other than itself being frozen and making frozen promises, it should in all other ways conform exactly to the promise spec while still guaranteeing this invariant.

  constructor(x) {
    super(x);
    Object.freeze(this);
    if (this.constructor==DefensivePromise && this.then ===
DefensivePromise.prototype.then && /*) {
      goodPromises.add(this);
    }
  }

You probably need to make the test a little bit more careful, in case then is a malicious getter or something like that.

Attack:

class BadPromise {}

BadPromise.prototype.constructor = DefensivePromise;

const bp = Reflect.construct(DefensivePromise, [], BadPromise);

// Passed your checks, and so bp is frozen and added to goodPromises.
// But IIUC bp.[[Prototype]] is BadPromise.prototype.

BadPromise.prototype.then = r => {r(), r()};  // borrowed from your attack.

bp is now an attack object which can be used in a plan interference attack, by inappropriately synchronously calling r. I also like the point implicit in your attack that I also need the invariant that

DefensivePromise.resolve(anything).then(anycallback)

should all callback at most once.

But my point is that you can do all this in userland, you don't need to expose its complexity or bake any of it into the Promise spec.

I suspect you are correct. But let's reserve judgement until you, I, or someone succeeds at writing userland code immune to attack.

# Mark S. Miller (9 years ago)

On Wed, Apr 29, 2015 at 11:12 AM, C. Scott Ananian <ecmascript at cscott.net> wrote:

Assuming that you don't export DefensivePromise to the attacker, this is fine. Otherwise, I think this is still vulnerable to Reflect.construct lying about new.target:

Clever. Yes, this attack works.

Since it's Promise.then you care about, I think the approach in my previous message (where then is tested directly) is preferable.

As demonstrated, vulnerable to TOCTTOU.

# Kevin Smith (9 years ago)
const goodPromises = new WeakSet();
class DefensivePromise {
  constructor(x) {
    super(x);
    if (new.target === DefensivePromise) {
      Object.freeze(this);
      goodPromises.add(this);
    }
  }
  static resolve(x) {
    if (goodPromises.has(x)) {
      return x;  // should be equiv to super.resolve(x);
    }
    return new DefensivePromise(r => {r(x)});
  }
}

Basically you can't rely on new.target to mean what you think it means, in the face of Reflect.construct.

Maybe this?

constructor(x) {
    super(x);
    // At this point you know that "this" is a Promise, but you don't
    // know if the prototype is set correctly, so:
    if (Object.getPrototypeOf(this) === DefensivePromise.prototype)
        gooPromises.add(Object.freeze(this));
}
# C. Scott Ananian (9 years ago)
  constructor(x) {
    super(x);
    Object.defineProperties(this, { then: { value: this.then }});
    Object.freeze(this);
    if (this.constructor==DefensivePromise && this.then ===
DefensivePromise.prototype.then) {
      goodPromises.add(this);
    }
  }

Getting closer, I hope!

I also like the point implicit in your attack that I also need the invariant that

DefensivePromise.resolve(anything).then(anycallback)

should all callback at most once.

I believe this is already taken care of by the ES6 spec once you make it into NewPromiseCapability. At least I wrote tests for es6-shim once upon a time purporting to show this.

# C. Scott Ananian (9 years ago)

On Wed, Apr 29, 2015 at 3:09 PM, Allen Wirfs-Brock <allen at wirfs-brock.com> wrote:

 class DefensivePromise extends Promise {
  constructor(x) {
    super(x);
    if (new.target === DefensivePromise) {

I'm assuming this test is just to be subclass friendly and allow subclasses to freeze later?

      Object.freeze(this);
    }
  }
  static resolve(x) {
    switch (true) {
       default:

I guess a do { } while(false); would work as well?

          // assuming frozen primordial
          if (x.constructor !== DefensivePromise) break;  //just a quick exit, doesn't guarantee much
          if (!Object.isFrozen(x)) break;
          If (Object.getOwnPropertyDescriptor(x, 'then')) break;
          //a more subclass friendly approach would walk x's [[Prototype]] chain to ensure that the correct 'then' is inherited from frozen prototypes
          if (Object.getPrototypeOf(x) !== DefensivePromise.prototype) break;
          //Assert: x.then === Promise.prototype.then, now and forever after
          return x;
     }
     // must be called on a subclass of DefensivePromise, so we don't need to enforce the 'then' invariant
     If  (x.constructor ===  this) return x;   //in which case a constructor check is good enough

^^ this is a mistake right? I think this line doesn't belong.

     return new this(r => {r(x)});
   }
 }
Object.freeze(DefensivePromise);
Object.freeze(DefensivePromise.prototype);

It's not clear what the x.constructor test is still doing in your implementation.

But, regardless of the details of our implementations, can we agree that "tamper proof" promises don't seem to need the [[PromiseConstructor]] property?

# Allen Wirfs-Brock (9 years ago)

On Apr 29, 2015, at 12:24 PM, C. Scott Ananian wrote:

It's not clear what the x.constructor test is still doing in your implementation.

Do you mean the first or second occurrence?

But, regardless of the details of our implementations, can we agree that "tamper proof" promises don't seem to need the [[PromiseConstructor]] property?

I agree with you, but I'm not the Promise champion.

Regardless, too late for ES2015. It will have to proposed as an ES2016 change.

# C. Scott Ananian (9 years ago)

On Wed, Apr 29, 2015 at 3:32 PM, Allen Wirfs-Brock <allen at wirfs-brock.com> wrote:

Do you mean the first or second occurrence?

The second occurrence. At that point all we know about x is that it's not something safe. We shouldn't be looking at constructor in that case.

I agree with you, but I'm not the Promise champion.

Regardless, too late for ES2015. It will have to proposed as an ES2016 change.

True, but since subclassable Promises haven't been implemented in the major engines yet, still in time to get everyone to agree to implement the new semantics before use "in the wild" locks in the warts of ES2015.

If we get consensus, I can certainly ensure that es6-shim, core-js, and babel implement the "ES2016" semantics.

# Mark S. Miller (9 years ago)

On Wed, Apr 29, 2015 at 12:40 PM, C. Scott Ananian <ecmascript at cscott.net> wrote:

If we get consensus,

I want to sleep on it, but at the moment I am convinced. Thanks for pushing on this!

I can certainly ensure that es6-shim, core-js, and babel implement the "ES2016" semantics.

Excellent.

# Allen Wirfs-Brock (9 years ago)

On Apr 29, 2015, at 12:40 PM, C. Scott Ananian wrote:

The second occurrence. At that point all we know about x is that it's not something safe. We shouldn't be looking at constructor in that case.

the last two lines of the above resolve method should be replaced with:

return super.resolve(x)

x.construct === this is the test that I propose Promise.resolve should make instead of testing [[PromiseConstructor]] There should probably also be another test that SpeciesConstructor(x)===this

If we get consensus, I can certainly ensure that es6-shim, core-js, and babel implement the "ES2016" semantics.

I wouldn't jump the gun until we actually have TC39 consensus on a proposal.

What we've shown is that the [[PromiseConstructor]] test was never sufficient to guarantee the invariant that Mark is concerned about and also that there probably isn't a generic check we can do that would satisfy his requirements. He has do it himself like shown above. But the specified [[PromiseConstructor]] test doesn't really hurt anything. It places some slight limitations on what subclasses can do, but probably doesn't interfere with anything an actual subclass is likely to do. It seems like a restriction than can be relaxed in the future without anybody (or than conformance tests) noticing.

# C. Scott Ananian (9 years ago)

On Wed, Apr 29, 2015 at 4:16 PM, Allen Wirfs-Brock <allen at wirfs-brock.com> wrote:

the last two lines of the above resolve method should be replaced with:

return super.resolve(x)

x.construct === this is the test that I propose Promise.resolve should make instead of testing [[PromiseConstructor]] There should probably also be another test that SpeciesConstructor(x)===this

This is still not correct.

You can't call super.resolve(x) safely unless you know that this.constructor is trustworthy. If you've bailed out of the switch, you don't know that.

A correct implementation just does return new this(r => { r(x); }) after the switch.

The "another test" that you propose seems to be exactly the fix which Brandon proposed and I codified, to wit:

2. If IsPromise(x) is true,
    a. Let constructor be the value of SpeciesConstructor(x, %Promise%)
    b. If SameValue(constructor, C) is true, return x.

I wouldn't jump the gun until we actually have TC39 consensus on a proposal.

Mark, after you've slept on this and assuming you haven't changed your mind, would you be willing to be the TC39 champion? I'm not a participant in that process.

What we've shown is that the [[PromiseConstructor]] test was never sufficient to guarantee the invariant that Mark is concerned about and also that there probably isn't a generic check we can do that would satisfy his requirements. He has do it himself like shown above. But the specified [[PromiseConstructor]] test doesn't really hurt anything. It places some slight limitations on what subclasses can do, but probably doesn't interfere with anything an actual subclass is likely to do. It seems like a restriction than can be relaxed in the future without anybody (or than conformance tests) noticing.

Well, the [[PromiseConstructor]] test is breaking prfun, es6-shim and core-js right now, which is why I noticed it in the first place. (v8 seems to have implemented the [[PromiseConstructor]] internal property even though they don't have new.target implemented, so it's always hard-wired to Promise.) And babel and core-js have implemented non-standard semantics for Promise.resolve as a result.

In particular, with the current state of Promise.resolve, I can't subclass Promise in a meaningful way using ES5 syntax (see, the thread title turns relevant again!). So I have a vested interest in getting the semantics fixed so that people can start using Promise subclasses (like prfun does) instead of writing their "extra" Promise helpers directly on the Promise global object, while we're all waiting for ES6 syntax to make it to the mainstream.

(And when I say "getting the semantics fixed" what I really mean is "consensus among TC39" so that shim libraries can implement the correct semantics as early adopters, with assurance the browser engines will follow.)

Since I believe the goal is to eventually introduce some new methods on the global Promise in ES7, I think this list would have a keen interest in ensuring that libraries which stomp on the global Promise (due to lack of an alternative) don't take root.

# Mark S. Miller (9 years ago)

On Wed, Apr 29, 2015 at 2:36 PM, C. Scott Ananian <ecmascript at cscott.net> wrote:

Mark, after you've slept on this and assuming you haven't changed your mind, would you be willing to be the TC39 champion? I'm not a participant in that process.

Yes.

# Allen Wirfs-Brock (9 years ago)

On Apr 29, 2015, at 2:44 PM, Mark S. Miller wrote:

On Wed, Apr 29, 2015 at 2:36 PM, C. Scott Ananian <ecmascript at cscott.net> wrote: [...] Mark, after you've slept on this and assuming you haven't changed your mind, would you be willing to be the TC39 champion? I'm not a participant in that process.

Yes.

probably need to make sure that Domenic is in the loop.

# Brendan Eich (9 years ago)

Allen Wirfs-Brock wrote:

Regardless, too late for ES2015. It will have to proposed as an ES2016 change.

Could we errata quickly? Implementations should not ship the bug.

# Brendan Eich (9 years ago)

Allen Wirfs-Brock wrote:

But the specified [[PromiseConstructor]] test doesn't really hurt anything.

I've heard that before!

It's a wart implementations can avoid shipping, to avoid Murphy's-Law compatibility constraints growing on top of it. You don't want a wart at all, certainly not with ugly MLcc atop it! :-|

# Allen Wirfs-Brock (9 years ago)

On Apr 29, 2015, at 2:36 PM, C. Scott Ananian wrote:

This is still not correct.

You can't call super.resolve(x) safely unless you know that this.constructor is trustworthy. If you've bailed out of the switch, you don't know that.

you're correct. Let's go with:

class DefensivePromise extends Promise {
  constructor(x) {
    super(x);
    if (new.target === DefensivePromise) {
      Object.freeze(this);
    }
  }
  static resolve(x) {
    if (this===DefensivePromise) {
       switch (true) {
          default:
             // assuming frozen primordial
             if (x.constructor !== DefensivePromise) break;  //just a quick exit, doesn't guarantee much
             if (!Object.isFrozen(x)) break;
             If (Object.getOwnPropertyDescriptor(x, 'then')) break;
             //a more subclass friendly approach would walk x's [[Prototype]] chain to ensure that the correct 'then' is inherited from frozen prototypes
             if (Object.getPrototypeOf(x) !== DefensivePromise.prototype) break;
             //Assert: x.then === Promise.prototype.then, now and forever after
             return x;
        };
       return new this(r => {r(x)});
    }
    // must be called on a subclass of DefensivePromise, so we don't need to enforce the DefensivePromise 'then' invariant 
    return super.resolve(x);
    }
 }
Object.freeze(DefensivePromise);
Object.freeze(DefensivePromise.prototype);

A correct implementation just does return new this(r => { r(x); }) after the switch.

The "another test" that you propose seems to be exactly the fix which Brandon proposed and I codified, to wit:

2. If IsPromise(x) is true,
    a. Let constructor be the value of SpeciesConstructor(x, %Promise%)
    b. If SameValue(constructor, C) is true, return x.

That can't right. x could be an object that is not a C but whose SpeciesConstructor is C. In that case, a new C promise on x still has to be created. But your code would just return x. I would use a 'construtor' test in step 2. But if 'constructor' is not C and SpeciesConstructor is also not C then an extra level of C wrapping is needed.

# C. Scott Ananian (9 years ago)

On Wed, Apr 29, 2015 at 6:32 PM, Allen Wirfs-Brock <allen at wirfs-brock.com> wrote:

That can't right. x could be an object that is not a C but whose SpeciesConstructor is C. In that case, a new C promise on x still has to be created. But your code would just return x. I would use a 'construtor' test in step 2. But if 'constructor' is not C and SpeciesConstructor is also not C then an extra level of C wrapping is needed.

Let's call the Brendan proposal above the "early test" proposal.

It seems to me that if P.@@species is Q then P.resolve should always return an object x with x.constructor==Q, which I don't think any of the proposed tweaks yet do. So here's that variant, which I'll call the "late test" proposal:

Promise.resolve(x)

  1. Let C be the this value.
  2. If Type(C) is not Object, throw a TypeError exception.
  3. Let S be Get(C, @@species).
  4. ReturnIfAbrupt(S).
  5. If S is neither undefined nor null, let C be S.
  6. If IsPromise(x) is true, a. Let constructor be the value of Get(x, "constructor"). b. ReturnIfAbrupt(constructor) c. If SameValue(constructor, C) is true, return x.
  7. Let promiseCapability be NewPromiseCapability(C).
  8. ReturnIfAbrupt(promiseCapability).
  9. Let resolveResult be Call(promiseCapability.[[Resolve]], undefined, «x»).
  10. ReturnIfAbrupt(resolveResult).
  11. Return promiseCapability.[[Promise]].

This moves the test down from step 2 to step 6, and ensures that we test x.constructor against the same species that we would use to create the capability. The returned promise thus always has a consistent value for its constructor property.

It's hard for me to determine which of these is actually preferred, although both are more consistent that the current [[PromiseConstructor]] semantics.

It would help if the ES2015 proposal actually had any objects where C[Symbol.species] !== C, but it does not appear to. So let's look at the smalltalk usage of species.

To quote an arbitrarily-chosen smalltalk glossary ( www.mimuw.edu.pl/~sl/teaching/00_01/Delfin_EC/Glossary.htm):

species: The generic class of an object. This is normally the same as the class of an object, otherwise it is a class which describes the generic group of classes to which a particular class belongs. Note that this does not necessarily have to be class from the same branch of the hierarchy.

With this meaning for @@species, when one sets MyPromise.constructor[Symbol.species] = Promise; then one is saying that even though this is a MyPromise it's still generically a Promise (of the Promise species). Then the "early test" Promise.resolve is entirely consistent in passing through anything whose species is Promise. The "late test" semantics are a bit harder to describe in terms of species (even though they seem more consistent if you are just looking at the constructor property of the result).

The smalltalk motivating example usually given for species is the equivalent of:

WeakArray.constructor[Symbol.species] = Array;

so that the result of WeakArray.filter(...) isn't itself a weak array and so won't mysteriously disappear in the hands of the caller. Moving this example to Promises we'd have:

WeakPromise.constructor[Symbol.species] = Promise;

and WeakPromise would hold a weak reference to some value, but it wouldn't cause the promise returned by Promise#then to also be a weak reference.

Moving from the reference domain to the time domain, we can imagine that:

TimeoutPromise.constructor[Symbol.species] = Promise;

could also be used to ensure that a given promise is rejected if not resolved within a certain time period, but the "timeout" functionality is not intended to be inherited by promises chained with then.

The "early test" and "late test" proposals give different results and interpretations for:

p = Promise.resolve(weakPromise);
p = Promise.resolve(timeoutPromise);

In the "early test" proposal, both the weakPromise and the timeoutPromise are both already of the Promise species, and so don't need to be recast. In the "late test" proposal these would both be recast to new Promises -- possibly creating a new strong reference to the value in the weakPromise case.

On the other hand, both "early test" and "late test" give awkward semantics for:

pp = WeakPromise.resolve(p);
pp = TimeoutPromise.resolve(p);

Presumably these invocations are intended to ensure that pp holds a weak reference to the value or times out, respectively, even if p is already a generic Promise ("belongs to the Promise species").

But both "early test" and "late test" semantics potentially return generic Promise objects here, although the "early test" semantics do pass through a WeakPromise/TimeoutPromise if it is passed as an argument.

I think what this is saying is that Promise.resolve is more like a constructor than an instance method, and it shouldn't consult @@species at all.

So here's one more proposal. We'll call this the "no species" proposal:

Promise.resolve(x)

  1. Let C be the this value.
  2. If IsPromise(x) is true, a. Let constructor be the value of Get(x, "constructor"). b. ReturnIfAbrupt(constructor) c. If SameValue(constructor, C) is true, return x.
  3. Let promiseCapability be NewPromiseCapability(C).
  4. ReturnIfAbrupt(promiseCapability).
  5. Let resolveResult be Call(promiseCapability.[[Resolve]], undefined, «x»).
  6. ReturnIfAbrupt(resolveResult).
  7. Return promiseCapability.[[Promise]].

At this point I think "no species" is the correct thing to do. It also happens to be the simplest. If someone can come up with some alternative use cases for Promise.@@species I could perhaps be persuaded to change my mind.

# Mark Miller (9 years ago)

Just to close the loop on this, regarding the main point I remain convinced and would be happy to champion. Again, thanks for pushing on this.

More later....