fast-deep-equal icon indicating copy to clipboard operation
fast-deep-equal copied to clipboard

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

Open thomas-jeepe opened this issue 4 years ago • 6 comments

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

thomas-jeepe avatar Jan 17 '20 20:01 thomas-jeepe

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).

epoberezkin avatar Jan 18 '20 08:01 epoberezkin

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

thomas-jeepe avatar Jan 18 '20 15:01 thomas-jeepe

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.

epoberezkin avatar Jan 30 '20 07:01 epoberezkin

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?

moander avatar Mar 12 '21 20:03 moander

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

ephemer avatar Mar 23 '21 12:03 ephemer

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.

jwhitaker-swiftnav avatar Jan 09 '23 04:01 jwhitaker-swiftnav