Skip to content

Commit 48e83cc

Browse files
rebeccahumrebeccahum
authored
Merge pull request #437 from Automattic/fix/xss-js
HTMLExecutingFunctions: Add more functions
2 parents fe60d9f + a2ea440 commit 48e83cc

File tree

3 files changed

+127
-25
lines changed

3 files changed

+127
-25
lines changed

‎WordPressVIPMinimum/Sniffs/JS/HTMLExecutingFunctionsSniff.php‎

Lines changed: 62 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,26 @@ class HTMLExecutingFunctionsSniff extends Sniff {
2222
/**
2323
* List of HTML executing functions.
2424
*
25+
* Name of function => content or target.
26+
* Value indicates whether the function's arg is the content to be inserted, or the target where the inserted
27+
* content is to be inserted before/after/replaced. For the latter, the content is in the preceding method's arg.
28+
*
2529
* @var array
2630
*/
2731
public $HTMLExecutingFunctions = [
28-
'html',
29-
'append',
30-
'write',
31-
'writeln',
32+
'after' => 'content', // jQuery.
33+
'append' => 'content', // jQuery.
34+
'appendTo' => 'target', // jQuery.
35+
'before' => 'content', // jQuery.
36+
'html' => 'content', // jQuery.
37+
'insertAfter' => 'target', // jQuery.
38+
'insertBefore' => 'target', // jQuery.
39+
'prepend' => 'content', // jQuery.
40+
'prependTo' => 'target', // jQuery.
41+
'replaceAll' => 'target', // jQuery.
42+
'replaceWith' => 'content', // jQuery.
43+
'write' => 'content',
44+
'writeln' => 'content',
3245
];
3346

3447
/**
@@ -58,29 +71,63 @@ public function register() {
5871
*/
5972
public function process_token( $stackPtr ) {
6073

61-
if ( false === in_array( $this->tokens[ $stackPtr ]['content'], $this->HTMLExecutingFunctions, true ) ) {
74+
if ( ! isset( $this->HTMLExecutingFunctions[ $this->tokens[ $stackPtr ]['content'] ] ) ) {
6275
// Looking for specific functions only.
6376
return;
6477
}
6578

66-
$nextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true, null, true );
79+
if ( 'content' === $this->HTMLExecutingFunctions[ $this->tokens[ $stackPtr ]['content'] ] ) {
80+
$nextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true, null, true );
6781

68-
if ( T_OPEN_PARENTHESIS !== $this->tokens[ $nextToken ]['code'] ) {
69-
// Not a function.
70-
return;
71-
}
82+
if ( T_OPEN_PARENTHESIS !== $this->tokens[ $nextToken ]['code'] ) {
83+
// Not a function.
84+
return;
85+
}
86+
87+
$parenthesis_closer = $this->tokens[ $nextToken ]['parenthesis_closer'];
88+
89+
while ( $nextToken < $parenthesis_closer ) {
90+
$nextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextToken + 1, null, true, null, true );
91+
if ( T_STRING === $this->tokens[ $nextToken ]['code'] ) { // Contains a variable, function call or something else dynamic.
92+
$message = 'Any HTML passed to `%s` gets executed. Make sure it\'s properly escaped.';
93+
$data = [ $this->tokens[ $stackPtr ]['content'] ];
94+
$this->phpcsFile->addWarning( $message, $stackPtr, $this->tokens[ $stackPtr ]['content'], $data );
95+
96+
return;
97+
}
98+
}
99+
} elseif ( 'target' === $this->HTMLExecutingFunctions[ $this->tokens[ $stackPtr ]['content'] ] ) {
100+
$prevToken = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $stackPtr - 1, null, true, null, true );
101+
102+
if ( T_OBJECT_OPERATOR !== $this->tokens[ $prevToken ]['code'] ) {
103+
return;
104+
}
72105

73-
$parenthesis_closer = $this->tokens[ $nextToken ]['parenthesis_closer'];
106+
$prevPrevToken = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $prevToken - 1, null, true, null, true );
74107

75-
while ( $nextToken < $parenthesis_closer ) {
76-
$nextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextToken + 1, null, true, null, true );
77-
if ( T_STRING === $this->tokens[ $nextToken ]['code'] ) {
78-
$message = 'Any HTML passed to `%s` gets executed. Make sure it\'s properly escaped.';
108+
if ( T_CLOSE_PARENTHESIS !== $this->tokens[ $prevPrevToken ]['code'] ) {
109+
// Not a function call, but may be a variable containing an element reference, so just
110+
// flag all remaining instances of these target HTML executing functions.
111+
$message = 'Any HTML used with `%s` gets executed. Make sure it\'s properly escaped.';
79112
$data = [ $this->tokens[ $stackPtr ]['content'] ];
80113
$this->phpcsFile->addWarning( $message, $stackPtr, $this->tokens[ $stackPtr ]['content'], $data );
81114

82115
return;
83116
}
117+
118+
// Check if it's a function call (typically $() ) that contains a dynamic part.
119+
$parenthesis_opener = $this->tokens[ $prevPrevToken ]['parenthesis_opener'];
120+
121+
while ( $prevPrevToken > $parenthesis_opener ) {
122+
$prevPrevToken = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $prevPrevToken - 1, null, true, null, true );
123+
if ( T_STRING === $this->tokens[ $prevPrevToken ]['code'] ) { // Contains a variable, function call or something else dynamic.
124+
$message = 'Any HTML used with `%s` gets executed. Make sure it\'s properly escaped.';
125+
$data = [ $this->tokens[ $stackPtr ]['content'] ];
126+
$this->phpcsFile->addWarning( $message, $stackPtr, $this->tokens[ $stackPtr ]['content'], $data );
127+
128+
return;
129+
}
130+
}
84131
}
85132
}
86133

‎WordPressVIPMinimum/Tests/JS/HTMLExecutingFunctionsUnitTest.js‎

Lines changed: 44 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,48 @@
11
(function(){
22
var el = document.querySelector(".myclass");
3-
el.html(''); // OK.
4-
el.html('<b>Hand written HTML</b>'); // OK.
5-
el.html( '<b>' + variable + '</b>' ); // NOK.
6-
el.html( variable ); // NOK.
7-
el.append( variable ); // NOK.
3+
el.after(''); // OK.
4+
el.after('<b>Hand written HTML</b>'); // OK.
5+
el.after( '<b>' + variable + '</b>' ); // Warning.
6+
el.after( variable ); // Warning.
87
el.append( '<b>Hand written HTML</b>' ); // OK.
9-
el.append( '<b>' + variable + '</b>' ); // NOK.
8+
el.append( '<b>' + variable + '</b>' ); // Warning.
9+
el.append( variable ); // Warning.
10+
el.before('<b>Hand written HTML</b>'); // OK.
11+
el.before( '<b>' + variable + '</b>' ); // Warning.
12+
el.before( variable ); // Warning.
13+
el.html('<b>Hand written HTML</b>'); // OK.
14+
el.html( '<b>' + variable + '</b>' ); // Warning.
15+
el.html( variable ); // Warning.
16+
el.prepend('<b>Hand written HTML</b>'); // OK.
17+
el.prepend( '<b>' + variable + '</b>' ); // Warning.
18+
el.prepend( variable ); // Warning.
19+
el.replaceWith('<b>Hand written HTML</b>'); // OK.
20+
el.replaceWith( '<b>' + variable + '</b>' ); // Warning.
21+
el.replaceWith( variable ); // Warning.
1022
document.write( '<script>console.log("hello")</script>' ); // OK. No variable, conscious.
11-
document.write( hello ); // NOK.
12-
document.writeln( hey ); // NOK.
13-
})();
23+
document.write( hello ); // Warning.
24+
document
25+
. writeln( hey ); // Warning.
26+
27+
$('').appendTo(el); // OK.
28+
$('<b>Hand written HTML</b>').appendTo( el ); // OK.
29+
$( '<b>' + variable + '</b>' ).appendTo( el ); // Warning.
30+
$( variable ).appendTo( el ); // Warning.
31+
$('<b>Hand written HTML</b>').insertAfter( el ); // OK.
32+
$( '<b>' + variable + '</b>' ).insertAfter( el ); // Warning.
33+
$( variable ).insertAfter( el ); // Warning.
34+
$('<b>Hand written HTML</b>').insertBefore( el ); // OK.
35+
$( '<b>' + variable + '</b>' ).insertBefore( el ); // Warning.
36+
$( variable ).insertBefore( el ); // Warning.
37+
$('<b>Hand written HTML</b>').prependTo( el ); // OK.
38+
$( '<b>' + variable + '</b>' ).prependTo( el ); // Warning.
39+
$( variable ).prependTo( el ); // Warning.
40+
$('<b>Hand written HTML</b>').replaceAll( el ); // OK.
41+
$( '<b>' + variable + '</b>' ).replaceAll( el ); // Warning.
42+
$( variable )
43+
. replaceAll( el ); // Warning.
44+
})();
45+
46+
$( foo_that_contains_script_element() ).appendTo( el ); // Warning.
47+
var $foo = $( '.my-selector' );
48+
$foo.appendTo( el ); // Warning.

‎WordPressVIPMinimum/Tests/JS/HTMLExecutingFunctionsUnitTest.php‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,30 @@ public function getWarningList() {
3434
return [
3535
5 => 1,
3636
6 => 1,
37-
7 => 1,
37+
8 => 1,
3838
9 => 1,
3939
11 => 1,
4040
12 => 1,
41+
14 => 1,
42+
15 => 1,
43+
17 => 1,
44+
18 => 1,
45+
20 => 1,
46+
21 => 1,
47+
23 => 1,
48+
25 => 1,
49+
29 => 1,
50+
30 => 1,
51+
32 => 1,
52+
33 => 1,
53+
35 => 1,
54+
36 => 1,
55+
38 => 1,
56+
39 => 1,
57+
41 => 1,
58+
43 => 1,
59+
46 => 1,
60+
48 => 1,
4161
];
4262
}
4363

0 commit comments

Comments
 (0)