Skip to content

chore: replace __proto__ by getPrototypeOf#17386

Merged
pavelfeldman merged 2 commits intomicrosoft:mainfrom
kapouer:main
Sep 21, 2022
Merged

chore: replace __proto__ by getPrototypeOf#17386
pavelfeldman merged 2 commits intomicrosoft:mainfrom
kapouer:main

Conversation

@kapouer
Copy link
Copy Markdown
Contributor

@kapouer kapouer commented Sep 16, 2022

Some users might want to run --disable-proto=throw|delete option for a hardened Node.
It's easy to fix and doesn't prevent { __proto__: null } pattern.

@ghost
Copy link
Copy Markdown

ghost commented Sep 16, 2022

CLA assistant check
All CLA requirements met.

@jfgreffier
Copy link
Copy Markdown
Contributor

Should we add the no-proto ESLint rule in .eslintrc.js to enforce this ?

https://eslint.org/docs/latest/rules/no-proto

@kapouer
Copy link
Copy Markdown
Contributor Author

kapouer commented Sep 16, 2022

Let's see how it goes

@mxschmitt
Copy link
Copy Markdown
Contributor

The CI is failing: page/page-expose-function.spec.ts:77:1 › should support throwing "null"

@kapouer
Copy link
Copy Markdown
Contributor Author

kapouer commented Sep 16, 2022

Oh ! fixed and rebased.

@mxschmitt mxschmitt changed the title feat: replace __proto__ by getPrototypeOf chore: replace __proto__ by getPrototypeOf Sep 16, 2022

function isError(obj: any): obj is Error {
return obj instanceof Error || obj?.__proto__?.name === 'Error' || (obj?.__proto__ && isError(obj.__proto__));
const proto = obj ? Object.getPrototypeOf(obj) : null;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will fail if there is a user code like Object.getPrototypeOf = () => {};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, this is a server side code, we don't run random js here so it should be ok.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed, and obj.__proto__ was an order of magnitude less safe than Object.getPrototypeOf :)

Copy link
Copy Markdown
Member

@yury-s yury-s left a comment

Choose a reason for hiding this comment

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

Could you add a test for this?


function isError(obj: any): obj is Error {
return obj instanceof Error || obj?.__proto__?.name === 'Error' || (obj?.__proto__ && isError(obj.__proto__));
const proto = obj ? Object.getPrototypeOf(obj) : null;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, this is a server side code, we don't run random js here so it should be ok.

@kapouer
Copy link
Copy Markdown
Contributor Author

kapouer commented Sep 16, 2022

Could you add a test for this?

What do you mean ? this is already tested. The behavior didn't change at all ?

@kapouer
Copy link
Copy Markdown
Contributor Author

kapouer commented Sep 16, 2022

For the record, the more modules are going to fix this, the more node is likely to get rid of proto ! nodejs/node#31951

@mxschmitt mxschmitt self-requested a review September 19, 2022 11:01
@yury-s
Copy link
Copy Markdown
Member

yury-s commented Sep 19, 2022

Could you add a test for this?

What do you mean ? this is already tested. The behavior didn't change at all ?

I mean a test that would fail before your change and pass after, this way we can ensure that this is documented and will not break in the future during refactoring of the code.

@jfgreffier
Copy link
Copy Markdown
Contributor

Could you add a test for this?

What do you mean ? this is already tested. The behavior didn't change at all ?

I mean a test that would fail before your change and pass after, this way we can ensure that this is documented and will not break in the future during refactoring of the code.

Not a test, but the no-proto ESLint rule that was added will guarantee that __proto__ will not be used again

@kapouer
Copy link
Copy Markdown
Contributor Author

kapouer commented Sep 20, 2022

Maybe process.env.NODE_OPTIONS = (process.env.NODE_OPTIONS || "") + " --disable-proto=throw" should be added but honestly I'm not sure where.

@pavelfeldman
Copy link
Copy Markdown
Member

pavelfeldman commented Sep 21, 2022

Some users might want to run --disable-proto=throw|delete option for a hardened Node.

I'm fine with the change, but I don't get the idea behind the option above. We can't guarantee Playwright operation in the mode above, because we use third party modules and __proto__ is a part of JavaScript. So we can land it, but it won't mean anything in the context of the above request.

@pavelfeldman pavelfeldman merged commit 840a1f6 into microsoft:main Sep 21, 2022
@kapouer
Copy link
Copy Markdown
Contributor Author

kapouer commented Sep 21, 2022

I'm fine with the change, but I don't get the idea behind the option above. We can't guarantee Playwright operation in the mode above, because we use third party modules and __proto__ is a part of JavaScript. So we can land it, but it won't mean anything in the context of the above request.

__proto__ is a deprecated part of js, and, while still used here and there, it is not so far away from being abandoned by modules authors.
Thanks !

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.

5 participants