lib: make safe primordials Promise methods by aduh95 · Pull Request #38650 · nodejs/node

aduh95 · 2021-05-12T10:29:21Z

catch and finally methods on %Promise.prototype% looks up the then property of the promise instance, making it at risk of prototype pollution.

Refs: https://tc39.es/ecma262/#sec-promise.prototype.catch

test/parallel/test-primordials-promise.js

nodejs-github-bot · 2021-05-12T16:00:38Z

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

`catch` and `finally` methods on %Promise.prototype% looks up the `then` property of the instance, making it at risk of prototype pollution.

nodejs-github-bot · 2021-05-15T21:20:37Z

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

nodejs-github-bot · 2021-05-15T22:16:31Z

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

nodejs-github-bot · 2021-05-16T14:23:35Z

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

nodejs-github-bot · 2021-05-16T16:07:31Z

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

nodejs-github-bot · 2021-05-17T18:14:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38165/
ARM CI: https://ci.nodejs.org/job/node-test-commit-arm-fanned/20490/

mcollina · 2021-05-18T09:29:23Z

lib/internal/per_context/primordials.js

+  new Promise((a, b) =>
+    new SafePromise((a, b) => PromisePrototypeThen(thisPromise, a, b))
+      .finally(onFinally)
+      .then(a, b)


I think this creates too many promises to be usable in any hot code paths. I would recommend not adding this.

Aren't promises antithetical to hot code path since they are asynchronous? For info, PromisePrototypeFinally is only used in fs/promises, timers/promises, and run_main currently. Would you prefer if I added a lint rule to discourage its use so it doesn't end up in a hot code path?

I'm not sure if this allocates 3 or 4 promises instead of 1.
Using this specific code will only create problems.

I'd also be more in favor of discouraging the use of the catch and finally methods in core:

catch can be easily replaced with then(undefined, fn)

finally may be replaceable with async functions

I've removed used of PromisePrototypeFinally where it was possible, and rename the function to SafePromisePrototypeFinally to highlight the fact it is not a simple bridge to the actual primordial method. I plan to add it to the list of "problematic" primordials to avoid in hot code path in #38635.

mcollina

lgtm

nodejs-github-bot · 2021-05-18T14:44:23Z

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

nodejs-github-bot · 2021-05-18T23:20:21Z

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

`catch` and `finally` methods on %Promise.prototype% looks up the `then` property of the instance, making it at risk of prototype pollution. PR-URL: #38650 Refs: https://tc39.es/ecma262/#sec-promise.prototype.catch Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell · 2021-05-19T16:22:01Z

Landed in 2eeb4e1

danielleadams · 2021-05-31T19:34:09Z

@aduh95 Do you mind backporting this? It broke the build when pulling into v16.x-staging. Thank you!

`catch` and `finally` methods on %Promise.prototype% looks up the `then` property of the instance, making it at risk of prototype pollution. PR-URL: nodejs#38650 Refs: https://tc39.es/ecma262/#sec-promise.prototype.catch Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`catch` and `finally` methods on %Promise.prototype% looks up the `then` property of the instance, making it at risk of prototype pollution. PR-URL: #38650 Backport-PR-URL: #38878 Refs: https://tc39.es/ecma262/#sec-promise.prototype.catch Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

github-actions bot added the needs-ci PRs that need a full CI run. label May 12, 2021

aduh95 force-pushed the safer-primordial-catch branch from 5fcca83 to 6983de8 Compare May 12, 2021 12:55

aduh95 changed the title ~~lib: make primordials.PromisePrototypeCatch safe~~ lib: make safe primordials Promise methods May 12, 2021

aduh95 force-pushed the safer-primordial-catch branch from 6983de8 to 78eb904 Compare May 12, 2021 12:58

jasnell reviewed May 12, 2021

View reviewed changes

test/parallel/test-primordials-promise.js Show resolved Hide resolved

jasnell reviewed May 12, 2021

View reviewed changes

test/parallel/test-primordials-promise.js Show resolved Hide resolved

jasnell approved these changes May 12, 2021

View reviewed changes

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 12, 2021

aduh95 force-pushed the safer-primordial-catch branch from f2d1f4e to af4a071 Compare May 12, 2021 18:20

aduh95 added blocked PRs that are blocked by other issues or PRs. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels May 12, 2021

This comment has been minimized.

Sign in to view

lib: make primordials Promise methods safe

06e59ff

`catch` and `finally` methods on %Promise.prototype% looks up the `then` property of the instance, making it at risk of prototype pollution.

aduh95 force-pushed the safer-primordial-catch branch from af4a071 to 06e59ff Compare May 15, 2021 19:51

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels May 15, 2021

mcollina reviewed May 18, 2021

View reviewed changes

fixup! lib: make primordials Promise methods safe

41a3924

mcollina approved these changes May 18, 2021

View reviewed changes

fixup! fixup! lib: make primordials Promise methods safe

57b14cf

This comment has been minimized.

Sign in to view

jasnell closed this May 19, 2021

aduh95 deleted the safer-primordial-catch branch May 19, 2021 17:16

danielleadams added the backport-requested-v16.x label May 31, 2021

aduh95 mentioned this pull request May 31, 2021

[v16.x backport] lib: make primordials Promise methods safe #38878

Closed

danielleadams mentioned this pull request Jun 14, 2021

v16.4.0 proposal #39031

Merged

richardlau added the backport-requested-v14.x label Jul 19, 2021

targos added backported-to-v16.x and removed backport-requested-v16.x labels Aug 23, 2021

Uh oh!

Conversation

aduh95 commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented May 12, 2021

Uh oh!

This comment has been minimized.

nodejs-github-bot commented May 15, 2021

Uh oh!

nodejs-github-bot commented May 15, 2021

Uh oh!

nodejs-github-bot commented May 16, 2021

Uh oh!

nodejs-github-bot commented May 16, 2021

Uh oh!

nodejs-github-bot commented May 17, 2021 • edited by aduh95 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina May 18, 2021

Choose a reason for hiding this comment

Uh oh!

aduh95 May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina May 18, 2021

Choose a reason for hiding this comment

Uh oh!

targos May 18, 2021

Choose a reason for hiding this comment

Uh oh!

aduh95 May 18, 2021

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented May 18, 2021

Uh oh!

This comment has been minimized.

nodejs-github-bot commented May 18, 2021 • edited by jasnell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 19, 2021

Uh oh!

danielleadams commented May 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

aduh95 commented May 12, 2021 •

edited

Loading

nodejs-github-bot commented May 17, 2021 •

edited by aduh95

Loading

aduh95 May 18, 2021 •

edited

Loading

nodejs-github-bot commented May 18, 2021 •

edited by jasnell

Loading