epoberezkin / fast-deep-equal

The fastest deep equality check with Date, RegExp and ES6 Map, Set and typed arrays support
MIT License
1.91k stars 102 forks source link

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

Open thomas-jeepe opened 4 years ago

thomas-jeepe commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

https://github.com/graphql/graphql-js/pull/504#issuecomment-250343574

epoberezkin commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 1 year ago

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.