Some options changes the definition of thenable

[mhofman]

The discussion during plenary made me realize that options 2 & 3 basically change the definition of what a thenable is for native promises, as it effectively excludes some objects with a callable then property, resulting in these objects to become the fulfillment value.

While I don't think the Promises/A+ spec explicitly defines a fulfillment value as being non-thenable, its Promise Resolution Procedure clearly state that values provided to a then's onFulfilled handler are recursively resolved, making it impossible for such values to become a fulfillment value of a spec compliant promise. Aka a "thenable" can misbehave and result in a thenable value fulfillment, but a spec compliant promise implementation must paper over that and not adopt that state.

As such I do not believe these approaches to be acceptable. By contrast, option 1, making Object.prototype exotic, does not cause that issue as it prevents plain objects from becoming thenable in the first place.

[bakkot]

As such I do not believe these approaches to be acceptable.

Say more about why? I don't see the relationship between this sentence and the preceding paragraphs.

[mhofman]

Options 2 & 3 would result in some native promises having fulfillment values that are thenables by the Promises/A+ spec definition of having a then callable property, making the JS native promise non compliant with the Promises/A+ specification itself.

[bakkot]

... OK? I don't see why that's a problem. What bad thing do you expect would happen as a result of that?

[mhofman]

Well arguably a better behaved promise implementation would paper over the indiscretions of the native promise implementation, so I supposed the concern is that end users of native promises would be surprised to find a thenable where they previously didn't. If native promises are the only implementation of promises in use, and are consistent when treating these thenables (aka that thenable never gets adopted by any native promise), then that would be the only surprise. I personally find that surprise unappetizing.

[mgaudet]

Given that the committee really was not particularly enthusiastic about (e.g. I expect to be blocked on) adding extra ticks, I think by definition any solution to this proposal must entail changing the definition of thenables, and producing situations where a promise resolves to a value which has a then property.

Given something like

Object.prototype.then = (o,r) => { o(42); };
Promise.resolve({}).then((x) => { console.log(x); }) // A promise fulfilled with 42, or [object Object] ? 

If we want to satisfy the restriction that a promise -never- resolves to an object with a then property [1], then we must make sure that Object.prototype never has the then property; This is is the making Object.prototype exotic plan. I don't love this; basically, I want to be able to generalize the solution beyond simply Object.prototype; e.g. for IDL types, being able to avoid this problem if someone sets then on an IDL prototype.

Otherwise, any solution (be it [[InternalProto]] or some other exclusion system) must always sometimes produce a value that has a then property as the value of a resolved promise.

Now: I am OK with modifying the definition of thenables, because I consider putting then on platform provided objects and expecting them to work to be deep antipatterns.

[1]: I seem to recall that modules already ignore the then property, so that's not true right now is it?

[mhofman]

While extra ticks were ruled out as an across the board change, I don't believe believe they were ruled out as targeted mitigations. There were multiple calls to avoid invasive spec changes and look at explicit safe handling operations for the web and spec to use. I personally consider making some objects with a then property non-thenable an invasive change.

As you mention, making Object.prototype exotic does remain an option for the common case of fresh born objects (which I also don't like for the record, but would remain amenable to)

I also want to remind that none of the thenable exclusion systems presented would have covered the case of CVE-2024-9680.

Finally, module namespace objects can definitely be thenable, and it is something actively used by the web ecosystem for dynamic import. What was decided is that for import defer, the deferred namespace object would not have a then property even if the module had such an export. This is in fact consistent with preventing thenable objects from coming to existence, not treating them specially.

[bakkot]

I personally consider making some objects with a then property non-thenable an invasive change.

Say more about why? It seems fine to me for the creator of an object to decide that it's not supposed to be Promise-like even if it does happen to end up with a then property. Indeed, objects which are not intended by their creator to be Promise-like ending up accidentally being treated as thenables is one of the more common footguns in the language, and people have fairly regularly suggested ways to opt out (Symbol.unthenable or whatever), which have died only because of the additional overhead.

I also want to remind that none of the thenable exclusion systems presented would have covered the case of CVE-2024-9680.

Why not? Animation instances are not intended to be thenables as far as I can tell, so marking Animation as [[InternalProto]] or whatever seems like it would have fixed that to me.

[mhofman]

It seems fine to me for the creator of an object to decide that it's not supposed to be Promise-like

But the language already allows you to do that in multiple ways: make the object non-extensible or define a non-configurable, non-writable then property with a value of undefined.

Animation instances are not intended to be thenables as far as I can tell, so marking Animation as [[InternalProto]] or whatever seems like it would have fixed that to me.

I may not fully understand this specific vulnerability, but wouldn't attaching a get then on animation instances have been sufficient to trigger the bug? And even if there was no way to do that in this specific case, I think thenable instances is still a potential vector that should be considered, and from what I understand [[InternalProto]] does not address it.

[bakkot]

But the language already allows you to do that in multiple ways: make the object non-extensible or define a non-configurable, non-writable then property with a value of undefined.

Some objects are expected to have more properties added to them over time. Some objects even quite reasonably expect to be able to use the string "then" to name one of their own properties, and only discover this is a problem when someone else wants to be able to return such an object from an async function.

may not fully understand this specific vulnerability, but wouldn't attaching a get then on animation instances have been sufficient to trigger the bug?

Ah, I missed that the objects were exposed to users before being used as the resolution value of a Promise. Yes, for this mitigation to address that the objects themselves would have to be marked as non-thenable. Which could be done, of course.

[jridgewell]

While I don't think the Promises/A+ spec explicitly defines a fulfillment value as being non-thenable, its Promise Resolution Procedure clearly state that values provided to a then's onFulfilled handler are recursively resolved, making it impossible for such values to become a fulfillment value of a spec compliant promise.

Note that it's possible to have a promise hold a thenable today:

const p = new Promise(r => {
    const thenable = {};
    r(thenable);
    thenable.then = (onFulfilled) => { onFulfilled(1); }
});

const thenable = await p;
console.assert(typeof thenable.then === 'function');

This is only possible in constructor form because the resolution code can be interleaved with user code immediately afterwards.

Also note that the A+ spec doesn't cover Promise constructors at all, we've just defined Promise(r => r(value)) to behave as if it's calling A+'s Promise Resolution Procedure.

Options 2 & 3 would result in some native promises having fulfillment values that are thenables by the Promises/A+ spec definition of having a then callable property, making the JS native promise non compliant with the Promises/A+ specification itself.

The A+ spec only defines how to resolve to a value that is returned from the onFullfilled/onRejected callbacks passed to a .then(). An conforming impl must run the Promise Resolution Procedure on the value in p2 = p.then(() => value) and p2 = p.then(null, () => value).

And you're right, in these 2 specific cases, it's impossible to have p2 hold a thenable, it'll always resolve to the thenable's eventual value.

If native promises are the only implementation of promises in use, and are consistent when treating these thenables (aka that thenable never gets adopted by any native promise), then that would be the only surprise. I personally find that surprise unappetizing.

I would also find this surprising if we did it with userland code. I do not think we should be updating our Promise to have a different behavior for a some arbitrary thenable, or even when resolving with one of our special thenables.

As a concrete example, let's discuss the the IteratorResult thenable CVE. Imagine we had Object.prototype.then = () => { exploit(); }. If the user were to new Promise(r => r({ value: foo })), they should still get the same behavior as today and trigger the exploit function. Or if they did p.then(() => ({ value: foo })). But we, as the spec and engine, should be able to safely resolve a promise to hold { value: foo }.

I also want to remind that none of the thenable exclusion systems presented would have covered the case of CVE-2024-9680.

Can you share code that would cause this exploit? I'm happy to receive it privately through Matrix, my email (my user name at google.com), or we could screen share. I don't know how to trigger this bug, so I don't know how we would protect against it.

---

If we want to satisfy the restriction that a promise -never- resolves to an object with a then property [1], then we must make sure that Object.prototype never has the then property; This is is the making Object.prototype exotic plan.

It's not just Object, but any thing in the prototype chain that could cause this.

Given that the committee really was not particularly enthusiastic about (e.g. I expect to be blocked on) adding extra ticks,

I will block on adding extra ticks to general object (non-thenable) resolution. It's too much of a performance penalty. But I agree with @mhofman:

While extra ticks were ruled out as an across the board change, I don't believe believe they were ruled out as targeted mitigations.

If we need to make targeted objects take an extra tick in limited circumstances, that's fine. But it can't be every resolve(obj) getting considerably slower.

[mhofman]

I also want to remind that none of the thenable exclusion systems presented would have covered the case of CVE-2024-9680.

Can you share code that would cause this exploit? I'm happy to receive it privately through Matrix, my email (my user name at google.com), or we could screen share. I don't know how to trigger this bug, so I don't know how we would protect against it.

My cursory reading of https://bugzilla.mozilla.org/show_bug.cgi?id=1923344 was that the issue stems from resolving a promise with the Animation instance, which means that if the instance was previously accessible to user code (which I thought was the case since remove seems to be an explicit operation), it could have an own get then (instead of having to rely on an inherited one). But even if that's not the case here, I think we should come up with a solution that addresses the risk of instances gaining an unexpected get then, not just their prototype chain.

[mhofman]

Some objects are expected to have more properties added to them over time. Some objects even quite reasonably expect to be able to use the string "then" to name one of their own properties, and only discover this is a problem when someone else wants to be able to return such an object from an async function.

Maybe people adding then properties to objects they don't create should discover earlier that they shouldn't do that.

String then properties are a protocol, whether we like it or not. If your object has it, it should implement the protocol. I don't care about people that want a then property but do not want to participate in the thenable protocol.

In the case of animation, we could very well have each instance have a non-configurable non-writable undefined then property to explicitly say they do not want to participate in the thenable protocol ever.

[mgaudet]

I have to shelve this for the weekend shortly, so I haven't read all the comments here. I do want to highlight one thing: We don't not have to come up with a bulletproof mitigation! A mitigation that addresses a good chunk of failures (perhaps tailored to newborn object resolve) is good enough from my mind.

[bakkot]

String then properties are a protocol, whether we like it or not. If your object has it, it should implement the protocol. I don't care about people that want a then property but do not want to participate in the thenable protocol.

Well, that's certainly a position one could hold. I disagree with it and see no reason why that position should bind us.