Skip to content

Commit 4b3d7f9

Browse files
rebeccahumrebeccahum
authored
Merge pull request #595 from Automattic/fix/345-underscorejs-output-escaping
Security/Underscorejs: bug fixes and enhancements
2 parents 97b8cfc + 09d5b92 commit 4b3d7f9

File tree

5 files changed

+411
-18
lines changed

5 files changed

+411
-18
lines changed

‎WordPressVIPMinimum/Sniffs/Security/UnderscorejsSniff.php‎

Lines changed: 111 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
namespace WordPressVIPMinimum\Sniffs\Security;
1010

11+
use PHP_CodeSniffer\Util\Tokens;
1112
use WordPressVIPMinimum\Sniffs\Sniff;
1213

1314
/**
@@ -17,6 +18,29 @@
1718
*/
1819
class UnderscorejsSniff extends Sniff {
1920

21+
/**
22+
* Regex to match unescaped output notations containing variable interpolation
23+
* and retrieve a code snippet.
24+
*
25+
* @var string
26+
*/
27+
const UNESCAPED_INTERPOLATE_REGEX = '`<%=\s*(?:.+?%>|$)`';
28+
29+
/**
30+
* Regex to match execute notations containing a print command
31+
* and retrieve a code snippet.
32+
*
33+
* @var string
34+
*/
35+
const UNESCAPED_PRINT_REGEX = '`<%\s*(?:print\s*\(.+?\)\s*;|__p\s*\+=.+?)\s*%>`';
36+
37+
/**
38+
* Regex to match the "interpolate" keyword when used to overrule the ERB-style delimiters.
39+
*
40+
* @var string
41+
*/
42+
const INTERPOLATE_KEYWORD_REGEX = '`(?:templateSettings\.interpolate|\.interpolate\s*=\s*/|interpolate\s*:\s*/)`';
43+
2044
/**
2145
* A list of tokenizers this sniff supports.
2246
*
@@ -30,12 +54,11 @@ class UnderscorejsSniff extends Sniff {
3054
* @return array
3155
*/
3256
public function register() {
33-
return [
34-
T_CONSTANT_ENCAPSED_STRING,
35-
T_PROPERTY,
36-
T_INLINE_HTML,
37-
T_HEREDOC,
38-
];
57+
$targets = Tokens::$textStringTokens;
58+
$targets[] = T_PROPERTY;
59+
$targets[] = T_STRING;
60+
61+
return $targets;
3962
}
4063

4164
/**
@@ -46,15 +69,91 @@ public function register() {
4669
* @return void
4770
*/
4871
public function process_token( $stackPtr ) {
72+
/*
73+
* Ignore Gruntfile.js files as they are configuration, not code.
74+
*/
75+
$file_name = $this->strip_quotes( $this->phpcsFile->getFileName() );
76+
$file_name = strtolower( basename( $file_name ) );
77+
78+
if ( $file_name === 'gruntfile.js' ) {
79+
return;
80+
}
81+
82+
/*
83+
* Check for delimiter change in JS files.
84+
*/
85+
if ( $this->tokens[ $stackPtr ]['code'] === T_STRING
86+
|| $this->tokens[ $stackPtr ]['code'] === T_PROPERTY
87+
) {
88+
if ( $this->phpcsFile->tokenizerType !== 'JS' ) {
89+
// These tokens are only relevant for JS files.
90+
return;
91+
}
92+
93+
if ( $this->tokens[ $stackPtr ]['content'] !== 'interpolate' ) {
94+
return;
95+
}
96+
97+
// Check the context to prevent false positives.
98+
if ( $this->tokens[ $stackPtr ]['code'] === T_STRING ) {
99+
$prev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true );
100+
if ( $prev === false || $this->tokens[ $prev ]['code'] !== T_OBJECT_OPERATOR ) {
101+
return;
102+
}
103+
104+
$prevPrev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true );
105+
$next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
106+
if ( ( $prevPrev === false
107+
|| $this->tokens[ $prevPrev ]['code'] !== T_STRING
108+
|| $this->tokens[ $prevPrev ]['content'] !== 'templateSettings' )
109+
&& ( $next === false
110+
|| $this->tokens[ $next ]['code'] !== T_EQUAL )
111+
) {
112+
return;
113+
}
114+
}
115+
116+
// Underscore.js delimiter change.
117+
$message = 'Found Underscore.js delimiter change notation.';
118+
$this->phpcsFile->addWarning( $message, $stackPtr, 'InterpolateFound' );
119+
120+
return;
121+
}
122+
123+
$content = $this->strip_quotes( $this->tokens[ $stackPtr ]['content'] );
124+
125+
$match_count = preg_match_all( self::UNESCAPED_INTERPOLATE_REGEX, $content, $matches );
126+
if ( $match_count > 0 ) {
127+
foreach ( $matches[0] as $match ) {
128+
if ( strpos( $match, '_.escape(' ) !== false ) {
129+
continue;
130+
}
131+
132+
// Underscore.js unescaped output.
133+
$message = 'Found Underscore.js unescaped output notation: "%s".';
134+
$data = [ $match ];
135+
$this->phpcsFile->addWarning( $message, $stackPtr, 'OutputNotation', $data );
136+
}
137+
}
138+
139+
$match_count = preg_match_all( self::UNESCAPED_PRINT_REGEX, $content, $matches );
140+
if ( $match_count > 0 ) {
141+
foreach ( $matches[0] as $match ) {
142+
if ( strpos( $match, '_.escape(' ) !== false ) {
143+
continue;
144+
}
49145

50-
if ( strpos( $this->tokens[ $stackPtr ]['content'], '<%=' ) !== false ) {
51-
// Underscore.js unescaped output.
52-
$message = 'Found Underscore.js unescaped output notation: "<%=".';
53-
$this->phpcsFile->addWarning( $message, $stackPtr, 'OutputNotation' );
146+
// Underscore.js unescaped output.
147+
$message = 'Found Underscore.js unescaped print execution: "%s".';
148+
$data = [ $match ];
149+
$this->phpcsFile->addWarning( $message, $stackPtr, 'PrintExecution', $data );
150+
}
54151
}
55152

56-
if ( strpos( $this->tokens[ $stackPtr ]['content'], 'interpolate' ) !== false ) {
57-
// Underscore.js unescaped output.
153+
if ( $this->phpcsFile->tokenizerType !== 'JS'
154+
&& preg_match( self::INTERPOLATE_KEYWORD_REGEX, $content ) > 0
155+
) {
156+
// Underscore.js delimiter change.
58157
$message = 'Found Underscore.js delimiter change notation.';
59158
$this->phpcsFile->addWarning( $message, $stackPtr, 'InterpolateFound' );
60159
}

‎WordPressVIPMinimum/Tests/Security/Gruntfile.js‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
2+
module.exports = function(grunt) {
3+
4+
require('load-grunt-tasks')(grunt);
5+
6+
// Project configuration.
7+
grunt.initConfig({
8+
pkg: grunt.file.readJSON('package.json'),
9+
10+
checktextdomain: {
11+
options:{
12+
text_domain: '<%= pkg.name %>',
13+
correct_domain: true,
14+
keywords: [
15+
'__:1,2d',
16+
'_e:1,2d',
17+
'_x:1,2c,3d',
18+
'esc_html__:1,2d',
19+
'esc_html_e:1,2d',
20+
'esc_html_x:1,2c,3d',
21+
'esc_attr__:1,2d',
22+
'esc_attr_e:1,2d',
23+
'esc_attr_x:1,2c,3d',
24+
'_ex:1,2c,3d',
25+
'_n:1,2,4d',
26+
'_nx:1,2,4c,5d',
27+
'_n_noop:1,2,3d',
28+
'_nx_noop:1,2,3c,4d'
29+
]
30+
},
31+
files: {
32+
src: [
33+
'**/*.php',
34+
],
35+
expand: true
36+
}
37+
},
38+
39+
makepot: {
40+
target: {
41+
options: {
42+
domainPath: '/languages/', // Where to save the POT file.
43+
mainFile: 'style.css', // Main project file.
44+
potFilename: '<%= pkg.name %>.pot', // Name of the POT file.
45+
type: 'wp-theme', // Type of project (wp-plugin or wp-theme).
46+
processPot: function( pot, options ) {
47+
pot.headers['plural-forms'] = 'nplurals=2; plural=n != 1;';
48+
pot.headers['x-poedit-basepath'] = '.\n';
49+
pot.headers['x-poedit-language'] = 'English\n';
50+
pot.headers['x-poedit-country'] = 'UNITED STATES\n';
51+
pot.headers['x-poedit-sourcecharset'] = 'utf-8\n';
52+
pot.headers['X-Poedit-KeywordsList'] = '__;_e;__ngettext:1,2;_n:1,2;__ngettext_noop:1,2;_n_noop:1,2;_c,_nc:4c,1,2;_x:1,2c;_ex:1,2c;_nx:4c,1,2;_nx_noop:4c,1,2;\n';
53+
pot.headers['x-textdomain-support'] = 'yes\n';
54+
return pot;
55+
}
56+
}
57+
}
58+
},
59+
60+
// Clean up build directory
61+
clean: {
62+
main: ['build/<%= pkg.name %>']
63+
},
64+
65+
// Copy the theme into the build directory
66+
copy: {
67+
main: {
68+
src: [
69+
'**',
70+
'!build/**',
71+
'!.git/**',
72+
'!Gruntfile.js',
73+
'!package.json',
74+
'!.gitignore',
75+
'!.gitmodules',
76+
],
77+
dest: 'build/<%= pkg.name %>/'
78+
}
79+
},
80+
81+
//Compress build directory into <name>.zip and <name>-<version>.zip
82+
compress: {
83+
main: {
84+
options: {
85+
mode: 'zip',
86+
archive: './build/<%= pkg.name %>.zip'
87+
},
88+
expand: true,
89+
cwd: 'build/<%= pkg.name %>/',
90+
src: ['**/*'],
91+
dest: '<%= pkg.name %>/'
92+
}
93+
}
94+
95+
});
96+
97+
// Default task(s).
98+
grunt.registerTask( 'build', [ 'clean', 'copy', 'compress' ] );
99+
grunt.registerTask( 'i18n', [ 'checktextdomain', 'makepot' ] );
100+
};

‎WordPressVIPMinimum/Tests/Security/UnderscorejsUnitTest.inc‎

Lines changed: 104 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,109 @@ EOT;
1212
<script type="text/template" id="js-tpl--example">
1313
<a
1414
href="<%= post_url %>"> <!-- NOK. -->
15-
<h3><%- post_title %></h3>
15+
<h3><%- post_title %></h3><!-- OK -->
1616
</a>
1717
</script>
18+
19+
<?php
20+
21+
function single_quoted_string() {
22+
echo '<div class="thumb" data-attid="<%= data.id %>" data-origsrc="<%- data.originalSrc %>">'. // NOK x 1.
23+
'<img src="<%- data.src %>" alt="<%=data.alt%>"/>'. // NOK x 1.
24+
'</div>';
25+
}
26+
27+
function single_quoted_string_with_concatenation( $data ) {
28+
echo '<img src="<%= ' . $data['src'] . ' %>" alt="<%- data.alt %>"/>'; // NOK x 1.
29+
}
30+
31+
function double_quoted_string( $name, $value, $is_template ) {
32+
echo $is_template ? "<%={$name}%>" : esc_attr( $value ); // NOK.
33+
}
34+
35+
$nowdoc = <<<'EOT'
36+
<script type="text/template" id="prefix-template">
37+
<section class="prefix-photo prefix-image">
38+
<img src="<%= img.src %>" class="prefix-image" width="<%= img.width %>" height="<%= img.height %>" /><!-- NOK x 3 -->
39+
</section>
40+
</script>
41+
EOT;
42+
43+
$heredoc = <<<EOD
44+
<label
45+
class="$classes prefix-form-ui-label-<%- htmlAttr.id %><%= ordinal %>"
46+
for="<%- htmlAttr.id %><%= ordinal %>">
47+
<%= name %>
48+
</label><!-- NOK - 1 per line -->
49+
EOD;
50+
51+
// Make sure the JS specific check does not trigger on PHP code.
52+
$obj->interpolate = true;
53+
54+
// Test matching the "interpolate" keyword with higher precision (mirrors same check in JS).
55+
function test_interpolate_match_precision() {
56+
?>
57+
<script type="text/javascript">
58+
_.templateSettings.interpolate = /\{\{(.+?)\}\}/g; /* NOK */
59+
60+
options.interpolate=_.templateSettings.interpolate; /* NOK */
61+
var interpolate = options.interpolate || reNoMatch, /* Ignore */
62+
source = "__p += '";
63+
64+
// Prevent false positives on "interpolate".
65+
var preventMisidentification = 'text interpolate text'; // OK.
66+
var interpolate = THREE.CurveUtils.interpolate; // OK.
67+
68+
var p = function(f, d) {
69+
return s.interpolate(m(f), _(d), 0.5, e.color_space) // OK.
70+
}
71+
72+
y.interpolate.bezier = b; // OK.
73+
</script>
74+
<?php
75+
}
76+
77+
// Recognize escaping.
78+
function dont_trigger_when_escaped() {
79+
$script = <<<EOD
80+
var html = _.template('<li><%= _.escape(name) %></li>', { name: 'John Smith' }); // OK.
81+
82+
var html = _.template(
83+
"<pre>The \"<% __p+=_.escape(o.text) %>\" is the same<br />" + // OK.
84+
"as the \"<%= _.escape(o.text) %>\" and the same<br />" + // OK.
85+
"as the \"<%- o.text %>\"</pre>", // OK.
86+
{
87+
text: "<b>some text</b> and \n it's a line break"
88+
},
89+
{
90+
variable: "o"
91+
}
92+
);
93+
EOD;
94+
95+
echo $script;
96+
}
97+
98+
function display_foo {
99+
?>
100+
<script id="template" type="text/template">
101+
<li class="dashboard-post-item" dashboard-id="<%= _.escape( id ) %>"><!-- OK -->
102+
<div class="image-wrapper">
103+
<img src="<%= _.escape( image_url ) %>" class="dashboard-image"><!-- OK -->
104+
</div>
105+
</li>
106+
</script>
107+
<?php
108+
}
109+
110+
function print_foo() {
111+
?>
112+
<script id="template" type="text/template">
113+
var compiled = _.template("<% print('Hello ' + _.escape(epithet)); %>"); /* OK */
114+
var compiled = _.template("<% print('Hello ' + epithet); %>"); /* NOK */
115+
var compiled = _.template("<% __p+=o.text %>"); /* NOK */
116+
117+
compiled({epithet: "stooge"});
118+
</script>
119+
<?php
120+
}

0 commit comments

Comments
 (0)