search

Leonidas-from-XIV / node-xml2js

XML to JavaScript object converter.
MIT License
4.84k stars 596 forks source link

Object prototype set to null #670

Closed TMaszko closed 1 year ago

TMaszko commented 1 year ago

Objects since 0.5.0 are created using Object.create(null) which results in the hard to track bugs when code that is using this parsed object wants to use it as normal JS object i.e using hasOwnProperty results with error ("hasOwnProperty" is not a function) like here

https://github.com/expo/expo/issues/22083?fbclid=IwAR3Rv6gP0gB2nrn1eHpq6E-Sd9jotjwn_HEyw4vD6XW4GZAaqym4B60Vp9k

Leonidas-from-XIV commented 1 year ago

Yes, this is a security fix due to #664 (and half a dozen other issues opened in this repo, this is just one example). I would've preferred to do it in a backwards compatible way but couldn't whitelist all kinds of possible "valid" names.

TMaszko commented 1 year ago

Can't we just blacklist the __proto__ name?

Leonidas-from-XIV commented 1 year ago

People can still add attributes like hasOwnProperty and then the code that uses the object fails.

TMaszko commented 1 year ago

You're right it's much more complex issue than I initially thought

TMaszko commented 1 year ago

@Leonidas-from-XIV How about setting the prototype after parsing is done ? We're going to create an object using Object.create(null) but after parsing we could set its prototype with Object.setPrototypeOf(parsedObject, Object.prototype) Wdyt?

Leonidas-from-XIV commented 1 year ago

Hmm, I haven't though of it too much but I believe that it could work! Though you have to recurse through all the created objects.

Could you make a PR? It would be good to give this a bit more thought and test the original report against this solution.

TMaszko commented 1 year ago

Sure thing! I'll prepare a PR soon ;)