Skip to content

HTMLExecutingFunctions: Add more functions #437

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.

Sign up for GitHub

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

Merged
rebeccahum merged 2 commits into from
Jul 3, 2020
Merged

HTMLExecutingFunctions: Add more functions #437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4a8e464
HTMLExecutingFunctions: Add more functions
GaryJones Aug 18, 2019
a2ea440
HTMLExecutingFunctions: Add further test cases
GaryJones Jul 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 62 additions & 15 deletions WordPressVIPMinimum/Sniffs/JS/HTMLExecutingFunctionsSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,26 @@ class HTMLExecutingFunctionsSniff extends Sniff {
/**
* List of HTML executing functions.
*
* Name of function => content or target.
* Value indicates whether the function's arg is the content to be inserted, or the target where the inserted
* content is to be inserted before/after/replaced. For the latter, the content is in the preceding method's arg.
*
* @var array
*/
public $HTMLExecutingFunctions = [
'html',
'append',
'write',
'writeln',
'after' => 'content', // jQuery.
'append' => 'content', // jQuery.
'appendTo' => 'target', // jQuery.
'before' => 'content', // jQuery.
'html' => 'content', // jQuery.
'insertAfter' => 'target', // jQuery.
'insertBefore' => 'target', // jQuery.
'prepend' => 'content', // jQuery.
'prependTo' => 'target', // jQuery.
'replaceAll' => 'target', // jQuery.
'replaceWith' => 'content', // jQuery.
'write' => 'content',
'writeln' => 'content',
];

/**
Expand Down Expand Up @@ -58,29 +71,63 @@ public function register() {
*/
public function process_token( $stackPtr ) {

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

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

if ( T_OPEN_PARENTHESIS !== $this->tokens[ $nextToken ]['code'] ) {
// Not a function.
return;
}
if ( T_OPEN_PARENTHESIS !== $this->tokens[ $nextToken ]['code'] ) {
// Not a function.
return;
}

$parenthesis_closer = $this->tokens[ $nextToken ]['parenthesis_closer'];

while ( $nextToken < $parenthesis_closer ) {
$nextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextToken + 1, null, true, null, true );
if ( T_STRING === $this->tokens[ $nextToken ]['code'] ) { // Contains a variable, function call or something else dynamic.
$message = 'Any HTML passed to `%s` gets executed. Make sure it\'s properly escaped.';
$data = [ $this->tokens[ $stackPtr ]['content'] ];
$this->phpcsFile->addWarning( $message, $stackPtr, $this->tokens[ $stackPtr ]['content'], $data );

return;
}
}
} elseif ( 'target' === $this->HTMLExecutingFunctions[ $this->tokens[ $stackPtr ]['content'] ] ) {
$prevToken = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $stackPtr - 1, null, true, null, true );

if ( T_OBJECT_OPERATOR !== $this->tokens[ $prevToken ]['code'] ) {
return;
}

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

while ( $nextToken < $parenthesis_closer ) {
$nextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextToken + 1, null, true, null, true );
if ( T_STRING === $this->tokens[ $nextToken ]['code'] ) {
$message = 'Any HTML passed to `%s` gets executed. Make sure it\'s properly escaped.';
if ( T_CLOSE_PARENTHESIS !== $this->tokens[ $prevPrevToken ]['code'] ) {
Copy link
Contributor

@david-binda david-binda Aug 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that we should also check whether the keyword we have found is a function call. As, for instance, the code, as it is now, would flag following code:

foo( variable ).appendTo;

// Not a function call, but may be a variable containing an element reference, so just
// flag all remaining instances of these target HTML executing functions.
$message = 'Any HTML used with `%s` gets executed. Make sure it\'s properly escaped.';
$data = [ $this->tokens[ $stackPtr ]['content'] ];
$this->phpcsFile->addWarning( $message, $stackPtr, $this->tokens[ $stackPtr ]['content'], $data );

return;
}

// Check if it's a function call (typically $() ) that contains a dynamic part.
$parenthesis_opener = $this->tokens[ $prevPrevToken ]['parenthesis_opener'];

while ( $prevPrevToken > $parenthesis_opener ) {
$prevPrevToken = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $prevPrevToken - 1, null, true, null, true );
if ( T_STRING === $this->tokens[ $prevPrevToken ]['code'] ) { // Contains a variable, function call or something else dynamic.
$message = 'Any HTML used with `%s` gets executed. Make sure it\'s properly escaped.';
$data = [ $this->tokens[ $stackPtr ]['content'] ];
$this->phpcsFile->addWarning( $message, $stackPtr, $this->tokens[ $stackPtr ]['content'], $data );

return;
}
}
}
}

Expand Down
53 changes: 44 additions & 9 deletions WordPressVIPMinimum/Tests/JS/HTMLExecutingFunctionsUnitTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,48 @@
(function(){
var el = document.querySelector(".myclass");
el.html(''); // OK.
el.html('<b>Hand written HTML</b>'); // OK.
el.html( '<b>' + variable + '</b>' ); // NOK.
el.html( variable ); // NOK.
el.append( variable ); // NOK.
el.after(''); // OK.
el.after('<b>Hand written HTML</b>'); // OK.
el.after( '<b>' + variable + '</b>' ); // Warning.
el.after( variable ); // Warning.
el.append( '<b>Hand written HTML</b>' ); // OK.
el.append( '<b>' + variable + '</b>' ); // NOK.
el.append( '<b>' + variable + '</b>' ); // Warning.
el.append( variable ); // Warning.
el.before('<b>Hand written HTML</b>'); // OK.
el.before( '<b>' + variable + '</b>' ); // Warning.
el.before( variable ); // Warning.
el.html('<b>Hand written HTML</b>'); // OK.
el.html( '<b>' + variable + '</b>' ); // Warning.
el.html( variable ); // Warning.
el.prepend('<b>Hand written HTML</b>'); // OK.
el.prepend( '<b>' + variable + '</b>' ); // Warning.
el.prepend( variable ); // Warning.
el.replaceWith('<b>Hand written HTML</b>'); // OK.
el.replaceWith( '<b>' + variable + '</b>' ); // Warning.
el.replaceWith( variable ); // Warning.
document.write( '<script>console.log("hello")</script>' ); // OK. No variable, conscious.
document.write( hello ); // NOK.
document.writeln( hey ); // NOK.
})();
document.write( hello ); // Warning.
document
. writeln( hey ); // Warning.

$('').appendTo(el); // OK.
$('<b>Hand written HTML</b>').appendTo( el ); // OK.
$( '<b>' + variable + '</b>' ).appendTo( el ); // Warning.
$( variable ).appendTo( el ); // Warning.
$('<b>Hand written HTML</b>').insertAfter( el ); // OK.
$( '<b>' + variable + '</b>' ).insertAfter( el ); // Warning.
$( variable ).insertAfter( el ); // Warning.
$('<b>Hand written HTML</b>').insertBefore( el ); // OK.
$( '<b>' + variable + '</b>' ).insertBefore( el ); // Warning.
$( variable ).insertBefore( el ); // Warning.
$('<b>Hand written HTML</b>').prependTo( el ); // OK.
$( '<b>' + variable + '</b>' ).prependTo( el ); // Warning.
$( variable ).prependTo( el ); // Warning.
$('<b>Hand written HTML</b>').replaceAll( el ); // OK.
$( '<b>' + variable + '</b>' ).replaceAll( el ); // Warning.
$( variable )
. replaceAll( el ); // Warning.
})();

$( foo_that_contains_script_element() ).appendTo( el ); // Warning.
var $foo = $( '.my-selector' );
$foo.appendTo( el ); // Warning.
22 changes: 21 additions & 1 deletion WordPressVIPMinimum/Tests/JS/HTMLExecutingFunctionsUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,30 @@ public function getWarningList() {
return [
5 => 1,
6 => 1,
7 => 1,
8 => 1,
9 => 1,
11 => 1,
12 => 1,
14 => 1,
15 => 1,
17 => 1,
18 => 1,
20 => 1,
21 => 1,
23 => 1,
25 => 1,
29 => 1,
30 => 1,
32 => 1,
33 => 1,
35 => 1,
36 => 1,
38 => 1,
39 => 1,
41 => 1,
43 => 1,
46 => 1,
48 => 1,
];
}

Expand Down