fix(resolve): protect against reentrancy attack

[michaelfig]

Closes #9

Note that the test for isNormalPromiseThen does not check that the Promise.prototype is frozen as this is always true under SES and too strict outside of SES (tries assimilating all Promises). It also checks if p is frozen only when under SES, as it is too strict outside of SES (tries assimilating platform Promises).

Please review carefully. There is no rush.

[erights]

Outside of SES, if we're not checking isFrozen(promiseProto) and we are checking gopd(promiseProto, 'then').value === originalThen, we should also check that this data property is non-writable, non-configurable.

[michaelfig]

I found that using harden() anywhere in eventual-send meant that we are taking a stand in hardening HandledPromises. Going all the way, I instead harden things that are returned by HandledPromise.resolve which simplifies the test and makes it behave more consistently inside and outside SES. This helped ERTP pass all its unit tests again.

I hope I made an acceptable tradeoff.

If that's not acceptable, then we need a much more convincing story of what we should actually harden in eventual-send.

[michaelfig]

@erights PTAL.

[Chris-Hibbert]

Sorry, but I'm not qualified to review this.

[erights]

Object.setPrototypeOf(HandledPromise, Promise);

My apologies! I saw the comment, and interpreted it as being about setting HandledPromise.prototype to be Promise.prototype, which we do in fact need. However, I see this was always true, now with the line

HandledPromise.prototype = promiseProto;

which was never turned off. Regarding the line that I mistakenly asked you to change, I actually have no strong feeling one way or the other. I'm happy with what you changed it to, and with making the corresponding change to the spec. But I'd also be happy without it. Feel free to revert this portion of this PR before merging, or not, as you wish.

Sorry for the confusion!

[michaelfig]

After using agoric-labs/tape-xs#3 I discovered that the test suite under XS is hanging with the following output:

# E method calls
ok 1 return is a thenable
 lin_xs_cli: main() returned a promise; entering event loop
ok 2 method call works
# E shortcuts
ok 3 method call works
ok 4 anonymous method works
ok 5 property get
# chained properties
ok 6 should be equivalent
ok 7 should be equivalent
# E.resolve is always asynchronous
ok 8 should be identical

[michaelfig]

@dckc, the hanging test is at an awaited promise

[michaelfig]

@dckc Aha! I'm narrowing down on the problem, and it looks like a legit portability bug in this PR.

[michaelfig]

It's the override mistake. Under XS, I cannot override the .then of a promise because Promise.prototype.then is frozen!

That's actually a good thing. This prevents having to assimilate promises under XS.

I've updated the tests accordingly. Will retry.

[erights]

That's actually a good thing. This prevents having to assimilate promises under XS.

The override mistake only protects against override by assignment. It doesn't prevent override by defineProperty, and so does not provide any actual safety against intentional override.

[michaelfig]

That's actually a good thing. This prevents having to assimilate promises under XS.

The override mistake only protects against override by assignment. It doesn't prevent override by defineProperty, and so does not provide any actual safety against intentional override.

Oh, okay. I'll rewrite the test accordingly.

[michaelfig]

Bingo! The workaround of the override mistake using Object.defineProperty shows proper assimilation behaviour under both Node and XS.

Still, it was interesting to reveal Moddable-OpenSource/moddable#322

[erights]

LGTM