-
Notifications
You must be signed in to change notification settings - Fork 29.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
esm: protect ESM loader from prototype pollution #45175
esm: protect ESM loader from prototype pollution #45175
Conversation
In a previous commit, the loader implementation was modified to be protected against most prototype pollution, but was kept vulnerable to `Array.prototype` pollution. This commit fixes that, the tradeoff is that it modifies the `ESMLoader.prototype.import` return type from an `Array` to an array-like object. Refs: nodejs#45044
Review requested:
|
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.
LGTM.
Out of scope for this PR, but should we have tests that things like our primordial PromisePrototypeAll
behaves equivalently to Promise.all
? Maybe an identical expected result from each in a test, where we wouldn’t have to worry about prototype pollution?
This comment was marked as outdated.
This comment was marked as outdated.
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.
lgtm
Landed in 2e2dc99 |
In a previous commit, the loader implementation was modified to be protected against most prototype pollution, but was kept vulnerable to `Array.prototype` pollution. This commit fixes that, the tradeoff is that it modifies the `ESMLoader.prototype.import` return type from an `Array` to an array-like object. Refs: #45044 PR-URL: #45175 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
In a previous commit, the loader implementation was modified to be protected against most prototype pollution, but was kept vulnerable to `Array.prototype` pollution. This commit fixes that, the tradeoff is that it modifies the `ESMLoader.prototype.import` return type from an `Array` to an array-like object. Refs: #45044 PR-URL: #45175 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
In a previous commit, the loader implementation was modified to be protected against most prototype pollution, but was kept vulnerable to `Array.prototype` pollution. This commit fixes that, the tradeoff is that it modifies the `ESMLoader.prototype.import` return type from an `Array` to an array-like object. Refs: #45044 PR-URL: #45175 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
In a previous commit, the loader implementation was modified to be protected against most prototype pollution, but was kept vulnerable to `Array.prototype` pollution. This commit fixes that, the tradeoff is that it modifies the `ESMLoader.prototype.import` return type from an `Array` to an array-like object. Refs: #45044 PR-URL: #45175 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
In a previous commit, the loader implementation was modified to be protected against most prototype pollution, but was kept vulnerable to `Array.prototype` pollution. This commit fixes that, the tradeoff is that it modifies the `ESMLoader.prototype.import` return type from an `Array` to an array-like object. Refs: #45044 PR-URL: #45175 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
In angular#49144 we introduced a change to only path `Promise.prototype.then` due to Node.js `SafePromise` complaining about `Promise.prototype.then` called on incompatible receiver. This however introduced a number of regressions. This commit reverts this change and re-introduces the changes to path the entire promise on Node. The original problem `SafePromise` problem is no longer reproducible as of Node.js version 18.13+ as it was addressed as part of nodejs/node#45175 While the Angular CLI does not yet generate ESM server bundles, users using ESM with dynamic imports will require using Node.js 18.13 or later. Closes angular#50513, closes angular#50457, closes angular#50414 and closes angular#49930
In angular#49144 we introduced a change to only path `Promise.prototype.then` due to Node.js `SafePromise` complaining about `Promise.prototype.then` called on incompatible receiver. This however introduced a number of regressions. This commit reverts this change and re-introduces the changes to patch the entire promise on Node. The original `SafePromise` problem is no longer reproducible as of Node.js version 18.13+ as it was addressed as part of nodejs/node#45175. While the Angular CLI does not yet generate ESM server bundles, users using ESM with dynamic imports will require using Node.js 18.13 or later. Closes angular#50513, closes angular#50457, closes angular#50414 and closes angular#49930
In angular#49144 we introduced a change to only path `Promise.prototype.then` due to Node.js `SafePromise` complaining about `Promise.prototype.then` called on incompatible receiver. This however introduced a number of regressions. This commit reverts this change and re-introduces the changes to patch the entire promise on Node. The original `SafePromise` problem is no longer reproducible as of Node.js version 18.13+ as it was addressed as part of nodejs/node#45175. While the Angular CLI does not yet generate ESM server bundles, users using ESM with dynamic imports will require using Node.js 18.13 or later. Closes angular#50513, closes angular#50457, closes angular#50414 and closes angular#49930
In #49144 we introduced a change to only path `Promise.prototype.then` due to Node.js `SafePromise` complaining about `Promise.prototype.then` called on incompatible receiver. This however introduced a number of regressions. This commit reverts this change and re-introduces the changes to patch the entire promise on Node. The original `SafePromise` problem is no longer reproducible as of Node.js version 18.13+ as it was addressed as part of nodejs/node#45175. While the Angular CLI does not yet generate ESM server bundles, users using ESM with dynamic imports will require using Node.js 18.13 or later. Closes #50513, closes #50457, closes #50414 and closes #49930 PR Close #50552
In #49144 we introduced a change to only path `Promise.prototype.then` due to Node.js `SafePromise` complaining about `Promise.prototype.then` called on incompatible receiver. This however introduced a number of regressions. This commit reverts this change and re-introduces the changes to patch the entire promise on Node. The original `SafePromise` problem is no longer reproducible as of Node.js version 18.13+ as it was addressed as part of nodejs/node#45175. While the Angular CLI does not yet generate ESM server bundles, users using ESM with dynamic imports will require using Node.js 18.13 or later. Closes #50513, closes #50457, closes #50414 and closes #49930 PR Close #50552
In angular#49144 we introduced a change to only path `Promise.prototype.then` due to Node.js `SafePromise` complaining about `Promise.prototype.then` called on incompatible receiver. This however introduced a number of regressions. This commit reverts this change and re-introduces the changes to patch the entire promise on Node. The original `SafePromise` problem is no longer reproducible as of Node.js version 18.13+ as it was addressed as part of nodejs/node#45175. While the Angular CLI does not yet generate ESM server bundles, users using ESM with dynamic imports will require using Node.js 18.13 or later. Closes angular#50513, closes angular#50457, closes angular#50414 and closes angular#49930 PR Close angular#50552
In angular#49144 we introduced a change to only path `Promise.prototype.then` due to Node.js `SafePromise` complaining about `Promise.prototype.then` called on incompatible receiver. This however introduced a number of regressions. This commit reverts this change and re-introduces the changes to patch the entire promise on Node. The original `SafePromise` problem is no longer reproducible as of Node.js version 18.13+ as it was addressed as part of nodejs/node#45175. While the Angular CLI does not yet generate ESM server bundles, users using ESM with dynamic imports will require using Node.js 18.13 or later. Closes angular#50513, closes angular#50457, closes angular#50414 and closes angular#49930 PR Close angular#50552
In a previous commit, the loader implementation was modified to be protected against most prototype pollution, but was kept vulnerable to
Array.prototype
pollution. This commit fixes that, the tradeoff is that it modifies theESMLoader.prototype.import
return type from anArray
to an array-like object.FWIW I don't expect changing the return type of
ESMLoader.prototype.import
to have any kind of unforeseen consequences, and I don't think it's exposed to user land in any way. //cc @nodejs/loadersRefs: #45044