Skip to content
New issue

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

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

Already on GitHub? # to your account

xunit reporter escaping is broken #43

Open
thom-nic opened this issue Mar 12, 2018 · 2 comments
Open

xunit reporter escaping is broken #43

thom-nic opened this issue Mar 12, 2018 · 2 comments

Comments

@thom-nic
Copy link

While working on node-snmpjs I upgraded tap (from v0.4!) to gain support for reporters and coverage. Unfortunately it looks like some tests generate non-ascii bytes in test names, which causes the xunit reporter to emit invalid XML.

The fix for the mocha xunit reporter is here: mochajs/mocha@9f403bf#diff-50e3aa130a4f97a42ee2cf111c7b1d9d

Here's the test in question:
https://github.com/joyent/node-snmpjs/blob/master/test/protocol/data.test.js#L359-L385

And the invalid XML output:

<testcase classname="./test/protocol/data.test.js octet string encode" name="encode foobar" time="0"/>
<testcase classname="./test/protocol/data.test.js octet string encode" name="encode ��AL� M" time="0"/>
<testcase classname="./test/protocol/data.test.js octet string encode" name="encode foo bar baz quux" time="0"/>
<testcase classname="./test/protocol/data.test.js octet string encode" name="encode �$Y�����" time="0"/>
<testcase classname="./test/protocol/data.test.js octet string encode" name="encode fööbå®" time="0"/>

Furthermore, the reserved-XML character escaping is even broken. It only replaces the first instance of each char:

// currently implemented:
exports.escape = function(html){
  return String(html)
    .replace('&', '&amp;')
    .replace('"', '&quot;')
    .replace('<', '&lt;')
    .replace('>', '&gt;');
}

to wit:

'bbb'.replace('b', 'o'); // results in 'obb' !
'bbb'.replace(/b/g, 'o'); // results in 'ooo'

The proper form should use /expr/g:

exports.escape = function(html){
  return String(html)
    .replace(/&/g, '&amp;')
    .replace(/"/g, '&quot;')
    .replace(/</g, '&lt;')
    .replace(/>/g, '&gt;');
}
@thom-nic
Copy link
Author

I was going to suggest, as an alternative to adding a dependency on he you might use punycode.toASCII however that doesn't handle non-printing ascii chars like "\u0000". Seems like the best option may be to use he like the mocha ended up doing.

@thom-nic
Copy link
Author

Hey thanks for the quick merge! I missed that the PR was sitting in #41! So that fixes the HTML-escaping half but leaves the non-ASCII.

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

No branches or pull requests

1 participant