lib: make safe primordials Promise methods - #38650
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
This comment has been minimized.
`catch` and `finally` methods on %Promise.prototype% looks up the `then` property of the instance, making it at risk of prototype pollution.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/38165/ |
Sorry, something went wrong.
| new Promise((a, b) => | ||
| new SafePromise((a, b) => PromisePrototypeThen(thisPromise, a, b)) | ||
| .finally(onFinally) | ||
| .then(a, b) |
There was a problem hiding this comment.
I think this creates too many promises to be usable in any hot code paths. I would recommend not adding this.
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
I'm not sure if this allocates 3 or 4 promises instead of 1.
Using this specific code will only create problems.
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
`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>
|
@aduh95 Do you mind backporting this? It broke the build when pulling into v16.x-staging. Thank you! |
Sorry, something went wrong.
`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>
`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>
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