Also run sanitize on the content when converting from xml to json. #129
Conversation
Previously, the content was unsanitized when the data was converted from json to xml, but not when converted from xml to json. This is corrected now so that the conversion is reversible.
|
@c4milo Please have a look at this new PR for you in xml2json. I did this change because I am representing HTML strings in an XLIFF file (XML Localization Interchange File Format) and the embedded HTML tags were screwing up the XML syntax without this sanitizing. |
Apparently, node-expat already unsanitizes the text for us when parsing xml before handing it off. So, we don't need to do it again.
|
Nice, thanks for this, I'll review tomorrow. |
|
@c4milo review? |
lib/xml2json.js
Outdated
| if (options.sanitize) { | ||
| currentObject['$t'] = sanitizer.sanitize(currentObject['$t'], true); | ||
| } | ||
| // node-expat already reverse sanitizes it whether we like it or not |
There was a problem hiding this comment.
I would just remove these comments, leaving them in the commit message instead.
|
Apologies it took me so long, it looks good to me in general. I Only commented on a minor thing. |
lib/json2xml.js
Outdated
| } | ||
| this.xml += ' ' + key + '="' + val + '"'; | ||
| } | ||
| ToXml.prototype.addTextContent = function(text) { | ||
| this.completeTag(); | ||
| this.xml += text; | ||
| this.xml += (this.options.sanitize ? sanitizer.sanitize(text) : text); |
There was a problem hiding this comment.
Please use multiple lines so a step by step debugging can be possible.
|
Is this pull request still alive? This would be really useful fix. |
|
Hi Anti, I didn't see c4milo's response to this. I'll work on it now and resolve the merge conflict. |
| var result = parser.toJson(xml, {sanitize: true, reversible: true}); | ||
| var json = internals.readFixture('xmlsanitize3.json'); | ||
|
|
||
| expect(result).to.equal(json); |
There was a problem hiding this comment.
Travis is failing because done() is missing from this test case.
|
Okay, ready to merge. |
|
c4milo, can you publish these changes to npm also? |
Merge pull request buglabs#129 from ehoogerbeets/master
Previously, the content was unsanitized when the data was converted from xml to json, but not sanitized when converted from json to xml. This is corrected now so that the conversion is reversible.