UnderscoreJS output escaping improvement?

[GaryJones]

The Underscorejs output escaping sniff checks for <%=, but it's possible that <%= _.escape(...) would also sufficiently escape the output.

I don't know UnderscoreJS, so this needs looking into, but it may help remove some false positives.

[mikeyarce]

It looks like .escape() would be ok: https://www.topjavatutorial.com/underscore-js/underscore-js-escape-unescape-functions/

[jrfnl]

I would love to have some more code samples for this to work with.

I also noticed that the sniff is supposed to check both PHP as well as JS files, but doesn't have a JS test file, so code samples from both would be very very welcome!!!

[GaryJones]

Example of what should not raise a violation:

function display_foo {
?>
	<script id="template" type="text/template">
		<li class="dashboard-post-item" dashboard-id="<%= _.escape( id ) %>">
			<div class="image-wrapper">
				<img src="<%= _.escape( image_url ) %>" class="dashboard-image">
			</div>
			...
		</li>
	</script>
<?php
}

[jrfnl]

I've done a file search on WPDirectory to find some more code samples and noticed that the <%= marker which is searched for, often is used in gruntfile.js. While that file should normally not be deployed, should I make an exception in the code to not trigger on <%= without escaping if the file in which it is found is gruntfile.js ?

[jrfnl]

Question: Undescorejs also supports the normal JS (unsafe) print function`, like so:

var compiled = _.template("<% print('Hello ' + epithet); %>");

Should this sniff try to detect that as well ?

[jrfnl]

@rebeccahum @GaryJones Your input on the above two questions would be much appreciated.

[GaryJones]

While that file should normally not be deployed, should I make an exception in the code to not trigger on <%= without escaping if the file in which it is found is gruntfile.js

We could probably exclude gruntfile.js (in the root of the repo, or root of the theme, or root of a plugin?) for all checks. So for this particular check, yes please to excluding it.

Should this sniff try to detect that as well ?

Yes please.

[jrfnl]

We could probably exclude gruntfile.js (in the root of the repo, or root of the theme, or root of a plugin?) for all checks

We could add an <exclude-pattern> to the ruleset for this, though a generically re-usable ruleset should normally not include <exclude-pattern>s.
In that case, no sniff specific changed would be needed. All the same, it's easy enough to address in the sniff itself.

[rebeccahum]

Should this sniff try to detect that as well ?

Echoing Gary, yes.