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

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.

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

No branches or pull requests

1 participant