Subclassing ES6 objects with ES5 syntax.
new.target is available in functions.
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
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.
An HTML attachment was scrubbed... URL: esdiscuss/attachments/20150424/0db86bd0/attachment
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.
An HTML attachment was scrubbed... URL: esdiscuss/attachments/20150424/4b35544b/attachment
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
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?
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?
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.
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.
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()
passXPromise.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?
I think I'd rather see
Promise.resolve
changed to usethis.constructor
instead ofthis.[[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.
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 usethis.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?
It's possible Reflect.construct has introduced a security hole that was not present before the recent instantiation reform. Hopefully Mark can comment more.
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.
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.
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.
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.
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.
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
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?
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.
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?
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]]?
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.
- Let C be the this value.
- If IsPromise(x) is true, a. Let constructor be the value of SpeciesConstructor(x, %Promise%)
b. If SameValue(constructor, C) is true, return x.
- If Type(C) is not Object, throw a TypeError exception.
- Let S be Get(C, @@species).
- ReturnIfAbrupt(S).
- If S is neither undefined nor null, let C be S.
- 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:
- 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.
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.
- Let C be the this value.
- If IsPromise(x) is true, a. Let constructor be the value of SpeciesConstructor(x, %Promise%) b. If SameValue(constructor, C) is true, return x.
- If Type(C) is not Object, throw a TypeError exception.
- Let S be Get(C, @@species).
- ReturnIfAbrupt(S).
- If S is neither undefined nor null, let C be S.
- 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.
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.
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 byPromise.resolve(x)
. Neither testingconstruct
or SpeciesConstructor really tells you anything aboutthen
. 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.
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.
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?
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.
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.
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.
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.
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 (wherethen
is tested directly) is preferable.
As demonstrated, vulnerable to TOCTTOU.
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));
}
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.
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?
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.
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.
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.
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.
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.
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.
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.
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.
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! :-|
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 theswitch
, 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.
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)
- Let C be the this value.
- If Type(C) is not Object, throw a TypeError exception.
- Let S be Get(C, @@species).
- ReturnIfAbrupt(S).
- If S is neither undefined nor null, let C be S.
- 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.
- Let promiseCapability be NewPromiseCapability(C).
- ReturnIfAbrupt(promiseCapability).
- Let resolveResult be Call(promiseCapability.[[Resolve]], undefined, «x»).
- ReturnIfAbrupt(resolveResult).
- 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 Promise
s 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 Promise
s -- 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)
- Let C be the this value.
- 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.
- Let promiseCapability be NewPromiseCapability(C).
- ReturnIfAbrupt(promiseCapability).
- Let resolveResult be Call(promiseCapability.[[Resolve]], undefined, «x»).
- ReturnIfAbrupt(resolveResult).
- 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.
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....
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:
But since
new.target
isn't accessible, we have to do something like: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 subclassesPromise
in order to add all the useful utility helpers without stomping on the globalPromise
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.