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

Objects with null prototype cause fast-deep-equal to throw an error #49

Open
thomas-jeepe opened this issue Jan 17, 2020 · 6 comments
Open

Comments

@thomas-jeepe
Copy link

Example code:

fastDeepEqual(Object.create(null, {a:true}), {a:true})

This will throw an error, the line below calls .valueOf() which is not a method for objects with a null prototype.

https://github.com/epoberezkin/fast-deep-equal/blob/master/src/index.jst#L51

@epoberezkin
Copy link
Owner

I believe it is not recommended In JS to have objects with null prototype (even though it is technically possible) - What is the reason to use them (and to support in this library)?

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/create

Mozilla here recommends as the best solution to resolving the problems of objects created with null prototype ... setting their prototype to Object. Not quite sure why null prototype was allowed in the first place, as using such objects creates more problems than null prototype can potentially solve (although I’m still not quite sure which problems can be solved with it).

@thomas-jeepe
Copy link
Author

I don't use null prototype directly, but graphql-js uses them for return values and I want to make assertions on those return values.

Some quick issue searching shows some reasoning for using null prototype.

graphql/graphql-js#504 (comment)

@epoberezkin
Copy link
Owner

Thank you - I commented there.

The design decision here is to consider instances of different classes (i.e. with different prototypes in JS) as not equal, even if they have the same properties. So if null prototype support is added (which I am considering), objects in your example would not throw an exception, but would return false - I believe it is different from what you expect.

I am also considering to introduce a version of this function that ignores classes/prototypes and only checks properties, with some specific exceptions (e.g. Date and RegExp).

To address it now, any possible workaround should be used in the application code.

@moander
Copy link

moander commented Mar 12, 2021

Same issue if A has valueOf defined as a property

fastDeepEqual({}, { valueOf: 'foo' }); // success

fastDeepEqual({ valueOf: 'foo' }, {}); // error

Uncaught TypeError: a.valueOf is not a function

Is typeof a.valueOf === 'function' too slow to consider?

@ephemer
Copy link

ephemer commented Mar 23, 2021

We are running into the same issue (also using graphql). You were asking about why anyone would do that, the reason is security, to avoid prototype pollution attacks: https://book.hacktricks.xyz/pentesting-web/deserialization/nodejs-proto-prototype-pollution#examples.

The design decision here is to consider instances of different classes (i.e. with different prototypes in JS) as not equal, even if they have the same properties.

Fair enough. Would it make sense to consider object.prototype === null and object.prototype === Object as equal for sake of comparison though? @epoberezkin

@jwhitaker-swiftnav
Copy link

Tangential but relevant: Array.prototype.group, which is on standards track, is spec'd to return objects with no prototype. I'd find it surprising if objects from such a builtin method were to cause a crash in fast-deep-equal.

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

No branches or pull requests

5 participants