Skip to content
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

Avoid hitting __proto__ #201

Closed
wants to merge 2 commits into from
Closed

Conversation

ChALkeR
Copy link
Contributor

@ChALkeR ChALkeR commented Jan 22, 2021

This avoids hitting __proto__ for hardened environments where __proto__ is disabled.

See discussion in nodejs/node#31951 and nodejs/node#32279.

To check that this works, run with NODE_OPTIONS=--disable-proto=throw node -e 'require("./graceful-fs.js")' before and after this change.

There is no reason to set .read() proto when Object.setPrototypeOf is not available, as no special handling of util.promisify exists on those versions (even if util.promisify is shipped separately somehow), so no __proto__ fallback there.

This change works all the way down to 0.8 afaik — Object.getPrototypeOf is available there, and Object.setPrototypeOf is guarded. Have not checked on versions below 0.8.6.

I believe this could be a semver-minor or a semver-patch.

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Jan 25, 2021

CI failure seems unrelated, I think.

polyfills.js Show resolved Hide resolved
clone.js Outdated Show resolved Hide resolved
polyfills.js Show resolved Hide resolved
Copy link

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

sorry for the noise.

Copy link
Collaborator

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

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

LGTM. @ljharb thanks for the review, better to have a little noise here than deal with a regression release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants