davidcalhoun / jstoxml

JavaScript object to XML converter (useful for RSS, podcasts, GPX, AMP, etc)
MIT License
178 stars 23 forks source link

Vulnerability: special characters should be escaped by default #41

Closed wetneb closed 4 years ago

wetneb commented 4 years ago

By default, this package does not escape XML characters:

jstoxml.toXML({
  foo: '&'
}
// Output: <foo>&</foo>

This is dangerous behaviour, since it means invalid XML can be output, causing injection vulnerabilities.

As a user, I would expect this package to escape strings by default. If there is a need to disable escaping, this could be done in the config, but that should not be the default behaviour.

davidcalhoun commented 4 years ago

Thank you for the heads up! That seems reasonable that escaping & should be the default behavior. It looks like it should also be default behavior to escape < and >.

~I'll look to add this and publish it as a new minor version, since it will be a backwards-compatible change.~

wetneb commented 4 years ago

Thanks a lot, that would be fantastic!

I think it is a bit more than <, > and &: for instance, in XML attributes, you need to escape at least " (but I haven't checked if it is done already).

Also, you might consider this a breaking change, since people who handled escaping on their side could end up with doubly-escaped strings such as &amp;amp;

davidcalhoun commented 4 years ago

Thanks again! Entities are now escaped by default in v2.0.0, along with " in attributes. Note that I added some checks to make sure things won't be double-encoded.

Decided to make this a new major version in the end because it does in fact require dependents to make a code change to return to the old behavior, which is indeed a breaking change.