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

Don't assume `hasOwnProperty` is safe #7

Merged
merged 2 commits into from Feb 9, 2019

Conversation

@rgrove
Copy link
Contributor

rgrove commented Feb 8, 2019

It's not safe to use hasOwnProperty on an object that came from JSON.parse(), since it may have been overwritten, as it is here:

{ "hasOwnProperty": "boom", "__proto__": { "foo": "bar" } }

Calling node.hasOwnProperty('__proto__') on this object results in a TypeError since hasOwnProperty is a string and not a function.

It's not safe to use `hasOwnProperty` on an object that came from
`JSON.parse()`, since it may have been overwritten, as it is here:

    { "hasOwnProperty": "boom", "__proto__": { "foo": "bar" } }

Calling `node.hasOwnProperty('__proto__')` on this object results in a
`TypeError` since `hasOwnProperty` is a string and not a function.
@@ -65,7 +65,7 @@ exports.scan = function (obj, options) {
next = [];

for (const node of nodes) {
if (node.hasOwnProperty('__proto__')) {
if (Object.prototype.hasOwnProperty.call(node, '__proto__')) {

This comment has been minimized.

Copy link
@mcollina

mcollina Feb 8, 2019

Contributor

I would recommend doing const hasOwnProperty = Object.prototype.hasOwnProperty; at the top of the file, and then reuse that. It's slightly more secure.

Copy link
Contributor

mcollina left a comment

LGTM

@hueniverse hueniverse self-assigned this Feb 9, 2019
@hueniverse hueniverse merged commit 6591ef7 into hapijs:master Feb 9, 2019
@hueniverse hueniverse added this to the 1.1.2 milestone Feb 9, 2019
hueniverse added a commit that referenced this pull request Feb 9, 2019
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Feb 9, 2019

Thanks. This is not a major issue since it will cause the function to throw and JSON.parse() already throws. But good catch.

@rgrove

This comment has been minimized.

Copy link
Contributor Author

rgrove commented Feb 9, 2019

Agreed. Definitely not a big deal, but thanks for merging so quickly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.