GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — lib: implement safe alternatives to `Promise` static methods by aduh95 · Pull Request #43728 · nodejs/node

Conversation

@aduh95

@aduh95 aduh95 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji JakobJingleheimer reacted with hooray emoji
All reactions
  • 👍 1 reaction
  • 🎉 1 reaction
@aduh95
lib: implement safe alternatives to Promise static methods
63f9abf
@nodejs-github-bot

nodejs-github-bot commented Jul 8, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/modules
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-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
@aduh95
fixup! lib: implement safe alternatives to Promise static methods
418864e
@aduh95 aduh95 changed the title lib: make primordials Promise static methods safe @aduh95 lib: implement safe alternatives to Promise static methods Jul 8, 2022
@aduh95 aduh95 changed the title @aduh95 lib: implement safe alternatives to Promise static methods lib: implement safe alternatives to Promise static methods Jul 8, 2022
benjamingr
benjamingr approved these changes Jul 8, 2022
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Qard
Qard approved these changes Jul 8, 2022
View reviewed changes
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Jul 8, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
legendecas
legendecas approved these changes Jul 8, 2022
View reviewed changes
@github-actions github-actions Bot mentioned this pull request Jul 9, 2022
CI Reliability 2022-07-09 nodejs/reliability#319
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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Jul 10, 2022
CI Reliability 2022-07-10 nodejs/reliability#320
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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Jul 10, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 10, 2022
@nodejs-github-bot
nodejs-github-bot merged commit 3fb5784 into nodejs:main Jul 10, 2022
@nodejs-github-bot

nodejs-github-bot commented Jul 10, 2022

Copy link
Copy Markdown
Collaborator

Landed in 3fb5784
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 deleted the safer-promise-statics-2 branch July 10, 2022 13:24
This was referenced Jul 10, 2022
lib: make primordials Promise static methods safe #38843
Closed
repl: use SafePromiseAll and SafePromiseRace #43758
Merged
@MoLow MoLow mentioned this pull request Jul 10, 2022
test_runner: fix it concurrency #43757
Merged
@aduh95 aduh95 mentioned this pull request Jul 10, 2022
doc: improve documentation for safe Promise statics alternatives #43759
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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@targos targos mentioned this pull request Jul 12, 2022
v18.6.0 release proposal #43789
Merged
@F3n67u F3n67u mentioned this pull request Jul 12, 2022
Investigate flaky pummel/test-http-upload-timeout #43800
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
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 <[email protected]>
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 <[email protected]>
@targos targos mentioned this pull request Aug 3, 2022
v16.17.0 release proposal #44098
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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
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 <[email protected]>
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
fix(@nguniversal/builders): address method Promise.prototype.then called on incompatible receiver error angular/universal#2879
Merged
[zone.js] Node > 16.17.1: Method Promise.prototype.then called on incompatible receiver angular/angular#47872
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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Qard Qard Qard approved these changes
@benjamingr benjamingr benjamingr approved these changes
@legendecas legendecas legendecas approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@aduh95 @nodejs-github-bot @benjamingr @Qard @legendecas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.