Skip to content

lib: implement safe alternatives to Promise static methods - #43728

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
aduh95:safer-promise-statics-2
Jul 10, 2022
Merged

lib: implement safe alternatives to Promise static methods#43728
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
aduh95:safer-promise-statics-2

Conversation

@aduh95

@aduh95aduh95 commented Jul 8, 2022

Copy link
Copy Markdown
Contributor

Alternative to #38843, but restricted to the changes related to the ESM implementation since it's easier to get consensus regarding whether we want tamperproofness on that part of the code base. /cc @nodejs/modules @nodejs/loaders

Promise static methods that iterate over the provided argument (%Promise.all%, %Promise.any%, ...) look up the then property over each promise to support promise subclassing. This PR is introducing SafePromiseAll, SafePromiseAny, etc. that take an array, safely iterate over it, and wrap each promise in a SafePromise (whose prototype is not accessible from userland) and wrap the resulting SafePromise in a classic Promise to make the operation transparent from userland.

@nodejs-github-bot

nodejs-github-bot commented Jul 8, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-botnodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Jul 8, 2022
@aduh95aduh95 changed the title lib: make primordials Promise static methods safe@aduh95 lib: implement safe alternatives to Promise static methodsJul 8, 2022
@aduh95aduh95 changed the title @aduh95 lib: implement safe alternatives to Promise static methodslib: implement safe alternatives to Promise static methodsJul 8, 2022
benjamingr
benjamingr approved these changes Jul 8, 2022
View reviewed changes

@benjamingrbenjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an improvement so LGTM.

Will leave another comment on the concept

@benjamingr

benjamingr commented Jul 8, 2022

Copy link
Copy Markdown
Member

Actually I see this is already scoped to discussing primordials in modules so I guess that's fine and no need to re-hash the "they make contributions harder, introduce perf regressions, confuse new contributors" etc discussions.

I just want to add that tamper-proofedness requires formal proof to be meaningful IMO (which afaik we haven't done), turning off known side channels (namely the JIT and concurrent GC) etc.

Qard
Qard approved these changes Jul 8, 2022
View reviewed changes
@aduh95aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 8, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2022
@nodejs-github-bot

nodejs-github-bot commented Jul 8, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45181/

@github-actionsgithub-actionsBot mentioned this pull request Jul 9, 2022
Open
21 tasks
@nodejs-github-bot

nodejs-github-bot commented Jul 9, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45199/

@nodejs-github-bot

nodejs-github-bot commented Jul 9, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45223/

@nodejs-github-bot

nodejs-github-bot commented Jul 9, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45227/

@nodejs-github-bot

nodejs-github-bot commented Jul 9, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45230/

@github-actionsgithub-actionsBot mentioned this pull request Jul 10, 2022
Open
20 tasks
@nodejs-github-bot

nodejs-github-bot commented Jul 10, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45255/

@nodejs-github-bot

nodejs-github-bot commented Jul 10, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45263/

@aduh95aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 10, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 10, 2022
@nodejs-github-bot
nodejs-github-bot merged commit 3fb5784 into nodejs:mainJul 10, 2022
@nodejs-github-bot

nodejs-github-bot commented Jul 10, 2022

Copy link
Copy Markdown
Collaborator

Landed in 3fb5784

@aduh95
aduh95 deleted the safer-promise-statics-2 branch July 10, 2022 13:24
This was referenced Jul 10, 2022
Closed
Merged
@MoLowMoLow mentioned this pull request Jul 10, 2022
Merged
@aduh95aduh95 mentioned this pull request Jul 10, 2022
Merged
targos pushed a commit that referenced this pull request Jul 12, 2022
@aduh95@targos
lib: implement safe alternatives to Promise static methods
dc484b6
PR-URL: #43728
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@targostargos mentioned this pull request Jul 12, 2022
Merged
@F3n67uF3n67u mentioned this pull request Jul 12, 2022
Closed
nodejs-github-bot pushed a commit that referenced this pull request Jul 25, 2022
@aduh95
doc: improve documentation for safe Promise statics alternatives
81a68b3
PR-URL: #43759
Refs: #43728
Reviewed-By: Feng Yu <F3n67u@outlook.com>
danielleadams pushed a commit that referenced this pull request Jul 26, 2022
@aduh95@danielleadams
doc: improve documentation for safe Promise statics alternatives
c789c0f
PR-URL: #43759
Refs: #43728
Reviewed-By: Feng Yu <F3n67u@outlook.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@aduh95@targos
lib: implement safe alternatives to Promise static methods
26d63f0
PR-URL: #43728
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@aduh95@targos
doc: improve documentation for safe Promise statics alternatives
122864f
PR-URL: #43759
Refs: #43728
Reviewed-By: Feng Yu <F3n67u@outlook.com>
targos pushed a commit that referenced this pull request Aug 1, 2022
@aduh95@targos
doc: improve documentation for safe Promise statics alternatives
bd0e463
PR-URL: #43759
Refs: #43728
Reviewed-By: Feng Yu <F3n67u@outlook.com>
@targostargos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@aduh95@guangwong
lib: implement safe alternatives to Promise static methods
794187c
PR-URL: nodejs/node#43728
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@aduh95@guangwong
doc: improve documentation for safe Promise statics alternatives
5343578
PR-URL: nodejs/node#43759
Refs: nodejs/node#43728
Reviewed-By: Feng Yu <F3n67u@outlook.com>
alan-agius4 added a commit to alan-agius4/universal that referenced this pull request Oct 26, 2022
@alan-agius4
fix(@nguniversal/builders): address method Promise.prototype.then cal…
1fd43f4
…led on incompatible receiver error
Prerendering does not work when using Node.js versions that contain the following commit nodejs/node#43728 due to `loadesm` is now using `SafePromiseAll` which is not handled properly by zone.js.
With this chage we introduce a workaround until it's fix upstream.
This was referenced Oct 26, 2022
Merged
Closed
alan-agius4 added a commit to alan-agius4/universal that referenced this pull request Oct 26, 2022
@alan-agius4
fix(@nguniversal/builders): address method Promise.prototype.then cal…
8e8fc81
…led on incompatible receiver error
Prerendering does not work when using Node.js versions that contain the following commit nodejs/node#43728 due to `loadesm` is now using `SafePromiseAll` which is not handled properly by zone.js.
With this chage we introduce a workaround until it's fix upstream.
alan-agius4 added a commit to alan-agius4/universal that referenced this pull request Oct 27, 2022
@alan-agius4
fix(@nguniversal/builders): address method Promise.prototype.then cal…
ac180a1
…led on incompatible receiver error
Prerendering does not work when using Node.js versions that contain the following commit nodejs/node#43728 due to `loadesm` is now using `SafePromiseAll` which is not handled properly by zone.js.
alan-agius4 added a commit to alan-agius4/universal that referenced this pull request Oct 27, 2022
@alan-agius4
fix(@nguniversal/builders): address method Promise.prototype.then cal…
bc69d62
…led on incompatible receiver error
Prerendering does not work when using Node.js versions that contain the following commit nodejs/node#43728 due to `loadesm` is now using `SafePromiseAll` which is not handled properly by zone.js.
alan-agius4 added a commit to angular/universal that referenced this pull request Oct 27, 2022
@alan-agius4
fix(@nguniversal/builders): address method Promise.prototype.then cal…
b7dbc25
…led on incompatible receiver error
Prerendering does not work when using Node.js versions that contain the following commit nodejs/node#43728 due to `loadesm` is now using `SafePromiseAll` which is not handled properly by zone.js.
alan-agius4 added a commit to angular/universal that referenced this pull request Oct 27, 2022
@alan-agius4
fix(@nguniversal/builders): address method Promise.prototype.then cal…
8f47c59
…led on incompatible receiver error
Prerendering does not work when using Node.js versions that contain the following commit nodejs/node#43728 due to `loadesm` is now using `SafePromiseAll` which is not handled properly by zone.js.
(cherry picked from commit b7dbc25)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QardQardQard approved these changes
@benjamingrbenjamingrbenjamingr approved these changes
@legendecaslegendecaslegendecas approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.esmIssues and PRs related to the ECMAScript Modules implementation.needs-ciPRs that need a full CI run.vmIssues and PRs related to the vm subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@aduh95@nodejs-github-bot@benjamingr@Qard@legendecas