Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vulnerability: special characters should be escaped by default #41

Closed
wetneb opened this issue Oct 10, 2020 · 3 comments
Closed

Vulnerability: special characters should be escaped by default #41

wetneb opened this issue Oct 10, 2020 · 3 comments
Labels

Comments

@wetneb
Copy link

wetneb commented Oct 10, 2020

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
Copy link
Owner

davidcalhoun commented Oct 10, 2020

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
Copy link
Author

wetneb commented Oct 10, 2020

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
Copy link
Owner

davidcalhoun commented Oct 12, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants