Leonidas-from-XIV / node-xml2js

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

XML elements "<constructor>" ignored since 0.6.0 #682

Closed JumpLink closed 11 months ago

JumpLink commented 1 year ago

Hi, I use xml2js for ts-for-gir, a *.gir (which is xml) file parser and TypeScript type definition generator.

This *.gir files also contains constructor elements, this elements are ignored since I have upgrade from 0.5.0 to 0.6.0.

I think this comes from #681 with this change:

isValidKey = (key) ->
  return key != '__proto__' && key != 'constructor' && key != 'prototype'

Is there a possibility that you allow the constructor key again or solve this differently?

Leonidas-from-XIV commented 1 year ago

Unfortunately, this is already the alternate solution. It is either this or use an object with no prototype which breaks all other consumers (as mentioned by numerous issues on this repo opened because of it)

I admit that the situation is not optimal, I am open for any better solutions.

Maybe it would work if we rewrote these properties?

JumpLink commented 1 year ago

Maybe it would work if we rewrote these properties?

Yes, it is not the nicest solution, but at least it would be better than forgetting this information completely. When I know this, I can adjust my code accordingly.

Why were these changes necessary? Is it because of a dependency? Maybe the problem can be solved there as well?

Leonidas-from-XIV commented 1 year ago

Why were these changes necessary?

It just accepting these causes a security vulnerability, CVE-2023-0842.

The other option could be to make the output an prototype-less object if the value is __proto__ or constructor.

JumpLink commented 1 year ago

The other option could be to make the output an prototype-less object if the value is __proto__ or constructor.

I had already adjusted my code for 0.5.0 where that was the case, right? So that would be ok for me, since I already adapted my code for that anyway

Leonidas-from-XIV commented 1 year ago

Exactly. In 0.5.0 this was the case for all nested objects, no matter whether they were causing clashes with special methods or not.

I am somewhat not particularly happy about this weird mix of different types of objects, maybe there's a nicer solution still. But for now, you could also just stay with 0.5.0...

JumpLink commented 1 year ago

Yeah, I would do the same as long as there is no solution for it. However, I always try to use the latest dependencies, so it would be nice if a solution could be found eventually. I can also imagine that others might encounter a similar problem. For example, there might be XML files with <prototype> elements as well.

JumpLink commented 1 year ago

@Leonidas-from-XIV Maybe renaming is the better solution after all, since keywords like constructor and prototype may cause other problems as well

JumpLink commented 1 year ago

An alternative solution might also be to enable the 0.5.0 behavior again via an option.

polomsky commented 11 months ago

@Leonidas-from-XIV

Unfortunately, this is already the alternate solution. It is either this or use an object with no prototype which breaks all other consumers (as mentioned by numerous issues on this repo opened because of it)

I can offer you better solution, though in JS (I am not familiar with CS, but I expect that can be used directly).

Instead of obj[key] = newValue which can break prototype, you can use: Object.defineProperty(obj, key, {value: newValue, writable: true, enumerable: true, configurable: true})

Leonidas-from-XIV commented 11 months ago

@polomsky Good point, thanks. I've updated it. It looks like #681 also missed one location where the value is set.

I'll be releasing this as 0.6.1 as we speak.