Skip to content

Commit e84f0d2

Browse files
committed
Review pass
1 parent 681009b commit e84f0d2

2 files changed

Lines changed: 18 additions & 20 deletions

File tree

README.md

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ unexpected.
2828
## Why is this a problem?
2929

3030
The most concrete one is security vulnerabilities. We must be ever-vigilant about
31-
this compatability supporting feature in all standard work, and throughout
31+
this compatibility supporting feature in all standard work, and throughout
3232
the web-platform. Failure to do so has the consequence of possible exploitation:
3333

3434
- [CVE-2024-43357](https://github.com/tc39/ecma262/security/advisories/GHSA-g38c-wh3c-5h9r) on the specification.
@@ -37,9 +37,9 @@ the web-platform. Failure to do so has the consequence of possible exploitation:
3737
- [CVE-2024-9086](https://www.welivesecurity.com/en/eset-research/romcom-exploits-firefox-and-windows-zero-days-in-the-wild/)
3838
- Others not disclosed here.
3939

40-
The reason this particular issue is fingered for causing security vulnerabities is
40+
The reason this particular issue is fingered for causing security vulnerabilities is
4141
that it adds many paths for user code execution which otherwise don't exist, and
42-
is not always obviously a possbility.
42+
is not always obviously a possibility.
4343

4444
Of particular danger is where specification authors think of newborn objects of known
4545
types as known quantities, only to call `Promise.resolve` on them. At this point
@@ -51,22 +51,23 @@ exist purely to work out the expected behaviour [for someone breaking `then`](ht
5151

5252
## How do I propose we fix this?
5353

54-
I'd like to propose we add a "Safe Resolve" resolve operation. It is functionally identical to
55-
[`PromiseResolve`](https://tc39.es/ecma262/#sec-promise-resolve) except there is a pre-step where
56-
we check for the conditions under which we could run user-code. If we cannot run any user code, we simply
57-
tail-call into `PromiseResolve`. If we *could* run user-code, we instead enqueue a new job whose
58-
responsibility is to call into `PromiseResolve`, while also putting the promise into a 'parked' state
59-
such that any future resolutions are ignored (Thank you very much to Mark Miller for catching this
60-
requirement in TG3 review discussion).
54+
I'd like to propose we add a "SafeResolve" resolve operation, which resolves a promise after
55+
checking for the possibility of running user code. If we cannot run any user code, we simply
56+
tail-call into the promise capability's `[[Resolve]]` operation. If we *could* run user-code,
57+
we instead enqueue a new job whose responsibility is to resolve the promise while also latching
58+
the promise in the same way the regular resolve functions do, such that any future resolutions
59+
are ignored (Thank you very much to Mark Miller for catching this requirement in TG3 review discussion).
6160

6261
The next step is to decide how to consume this. There is interest from the Mozilla DOM to explore
6362
using this to replace [the steps for resolving a promise in WebIDL](https://webidl.spec.whatwg.org/#resolve)
6463
and more generally powering all the promise resolution code in Mozilla's DOM. This would help make
6564
C++ code safer by making promise resolution into an operation that never runs script, which
6665
simplifies the reasoning required when implementing code.
6766

68-
Exposing this to user-code is a non-goal of this specific proposal, but can be done as a followup
69-
proposal eventually
67+
Specification discussion about WebIDL consumption is happening at [whatwg/webidl #1584](https://github.com/whatwg/webidl/issues/1584)
68+
69+
**Exposing this to user-code is a non-goal of this specific proposal**, but can be done as a followup
70+
proposal eventually.
7071

7172
## Is this a bulletproof fix?
7273

@@ -108,7 +109,7 @@ The vast majority of tests (as expected) pass.
108109

109110
#### Unexpected Pass:
110111

111-
1. [/fetch/api/response/response-body-read-task-handling.html](https://searchfox.org/firefox-main/source/testing/web-platform/tests/fetch/api/response/response-body-read-task-handling.html) \- This test is using `then` to get insight into execution order. The test no longer tests what it thinks it is testing anymore; however the test \-also- was created to address [this kind of thennable issue](https://bugzilla.mozilla.org/show_bug.cgi?id=1612308).
112+
1. [/fetch/api/response/response-body-read-task-handling.html](https://searchfox.org/firefox-main/source/testing/web-platform/tests/fetch/api/response/response-body-read-task-handling.html) \- This test is using `then` to get insight into execution order. The test no longer tests what it thinks it is testing anymore; however the test \-also- was created to address [this kind of thenable issue](https://bugzilla.mozilla.org/show_bug.cgi?id=1612308).
112113

113114
#### Test Failures
114115

spec.emu

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,8 @@
44
<script src="./spec.js"></script>
55
<pre class="metadata">
66
title: SafePromiseResolve
7-
stage: 1
8-
contributors: Matthew Gaudet
7+
stage: 2
8+
contributors: Matthew Gaudet, Justin Ridgewell
99
</pre>
1010

1111
<emu-clause id="sec-safe-resolve-promise">
@@ -107,9 +107,6 @@ contributors: Matthew Gaudet
107107
1. Perform HostEnqueuePromiseJob(_job_.[[Job]], _job_.[[Realm]]).
108108
1. Return ~unused~.
109109
</emu-alg>
110-
<emu-note>
111-
<p>This overlaps heavily with the Abstract Closure inside CreateResolvingFunctions. Later work could resolve this duplication</p>
112-
</emu-note>
113110
</emu-clause>
114111

115112
<emu-clause id="sec-newpromiseresolvethenablejob" type="abstract operation" oldids="sec-promiseresolvethenablejob">
@@ -147,7 +144,7 @@ contributors: Matthew Gaudet
147144
_promiseToResolve_: a Promise,
148145
_thenable_: an Object,
149146
_then_: a JobCallback Record or a function object,
150-
): either a normal completion containing *undefined* or a throw completion
147+
): either a normal completion containing an ECMAScript language value or a throw completion
151148
</h1>
152149
<dl class="header">
153150
</dl>
@@ -175,7 +172,7 @@ contributors: Matthew Gaudet
175172
</dl>
176173
<emu-alg>
177174
1. Let _promiseOrEmpty_ be the Record { [[Value]]: _toResolve_ }.
178-
1. Let _resolveSteps_ be a new Abstract Closure with parameters (_resolution_<ins>, _doSafeResolve_</ins>) that captures _promiseOrEmpty_ and performs the following steps when called:
175+
1. Let _resolveSteps_ be a new Abstract Closure with parameters (_resolution_) that captures _promiseOrEmpty_ and performs the following steps when called:
179176
1. If _promiseOrEmpty_.[[Value]] is ~empty~, return *undefined*.
180177
1. Let _promise_ be _promiseOrEmpty_.[[Value]].
181178
1. Set _promiseOrEmpty_.[[Value]] to ~empty~.

0 commit comments

Comments
 (0)