Skip to content

Commit

Permalink
Flag any non-validated/sanitized super global input vars.
Browse files Browse the repository at this point in the history
Closes #72
Switch list of sanitizing/autoescaping function to a static var to be used by other classes
  • Loading branch information
shadyvb committed Oct 20, 2013
1 parent af00eca commit f90170d
Show file tree
Hide file tree
Showing 4 changed files with 225 additions and 4 deletions.
143 changes: 143 additions & 0 deletions Sniffs/VIP/ValidatedSanitizedInputSniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
<?php
/**
* Flag any non-validated/sanitized input ( _GET / _POST / _REQUEST / _SERVER )
*
* PHP version 5
*
* @category PHP
* @package PHP_CodeSniffer
* @author Shady Sharaf <shady@x-team.com>
* @link https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/69
*/
class WordPress_Sniffs_VIP_ValidatedSanitizedInputSniff implements PHP_CodeSniffer_Sniff
{

/**
* Returns an array of tokens this test wants to listen for.
*
* @return array
*/
public function register()
{
return array(
T_VARIABLE,
);

}//end register()


/**
* Processes this test, when one of its tokens is encountered.
*
* @param PHP_CodeSniffer_File $phpcsFile The file being scanned.
* @param int $stackPtr The position of the current token
* in the stack passed in $tokens.
*
* @return void
*/
public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr )
{
$tokens = $phpcsFile->getTokens();

// Check for $wpdb variable
if ( ! in_array( $tokens[$stackPtr]['content'], array( '$_GET', '$_POST', '$_REQUEST', '$_SERVER' ) ) )
return;

$instance = $tokens[$stackPtr];
$varName = $instance['content'];

$nested = $instance['nested_parenthesis'];

// Ignore if wrapped inside ISSET
end($nested); // Get closest parenthesis
if ( T_ISSET === $tokens[ key( $nested ) - 1 ]['code'] )
return;

$varKey = $this->getArrayIndexKey( $phpcsFile, $tokens, $stackPtr );

if ( empty( $varKey ) ) {
$phpcsFile->addWarning( 'Detected access of super global var %s without targeting a member variable.', $stackPtr, null, array( $varName ) );
return;
}

// Check for validation first
$is_validated = false;

// Wrapped in a condition? check existence of isset with the variable as an argument
if ( ! empty( $tokens[$stackPtr]['conditions'] ) ) {
$conditionPtr = key( $tokens[$stackPtr]['conditions'] );
$condition = $tokens[$conditionPtr];

$issetPtr = $phpcsFile->findNext( array( T_ISSET, T_EMPTY ), $condition['parenthesis_opener'], $condition['parenthesis_closer'] );
if ( ! empty( $issetPtr ) ) {
$isset = $tokens[$issetPtr];
$issetOpener = $issetPtr + 1;
$issetCloser = $tokens[$issetOpener]['parenthesis_closer'];

// Check that it is the same variable name
if ( $validated = $phpcsFile->findNext( array( T_VARIABLE ), $issetOpener, $issetCloser, null, $varName ) ) {
// Double check the $varKey inside the variable, ex: 'hello' in $_POST['hello']

$varKeyValidated = $this->getArrayIndexKey( $phpcsFile, $tokens, $validated );

if ( $varKeyValidated == $varKey ) {
// everything matches, variable IS validated afterall ..
$is_validated = true;
}
}
}
}

if ( ! $is_validated ) {
$phpcsFile->addError( 'Detected usage of a non-validated input variable: %s', $stackPtr, null, array( $tokens[$stackPtr]['content'] ) );
// return; // Should we just return and not look for sanitizing functions ?
}

// Now look for sanitizing functions
$is_sanitized = false;

$functionPtr = key( $nested ) - 1;
$function = $tokens[$functionPtr];
if ( T_STRING === $function['code'] ) {
$functionName = $function['content'];
if (
in_array( $functionName, WordPress_Sniffs_XSS_EscapeOutputSniff::$autoEscapedFunctions )
||
in_array( $functionName, WordPress_Sniffs_XSS_EscapeOutputSniff::$sanitizingFunctions )
) {
$is_sanitized = true;
}
}

if ( ! $is_sanitized ) {
$phpcsFile->addError( 'Detected usage of a non-sanitized input variable: %s', $stackPtr, null, array( $tokens[$stackPtr]['content'] ) );
}


return;
}//end process()

/**
* Get array index key of the variable requested
* @param [type] $phpcsFile [description]
* @param [type] $tokens [description]
* @param [type] $stackPtr [description]
* @return [type] [description]
*/
public function getArrayIndexKey( $phpcsFile, $tokens, $stackPtr ) {
// Find next bracket
$bracketOpener = $phpcsFile->findNext( array( T_OPEN_SQUARE_BRACKET ), $stackPtr, $stackPtr + 3 );

// If no brackets, exit with a warning, this is a non-typical usage of super globals
if ( empty ( $bracketOpener ) ) {
return false;
}

$bracketCloser = $tokens[$bracketOpener]['bracket_closer'];

$varKey = trim( $phpcsFile->getTokensAsString( $bracketOpener + 1, $bracketCloser - $bracketOpener - 1 ) ); // aka 'hello' in $_POST['hello']

return $varKey;
}

}//end class
8 changes: 4 additions & 4 deletions Sniffs/XSS/EscapeOutputSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
class WordPress_Sniffs_XSS_EscapeOutputSniff implements PHP_CodeSniffer_Sniff
{

public $autoEscapedFunctions = array(
public static $autoEscapedFunctions = array(
'get_header',
'get_footer',
'get_sidebar',
Expand Down Expand Up @@ -196,7 +196,7 @@ class WordPress_Sniffs_XSS_EscapeOutputSniff implements PHP_CodeSniffer_Sniff
'do_shortcode_tag',
);

public $sanitizingFunctions = array(
public static $sanitizingFunctions = array(
'wp_kses_post',
'wp_kses_data',
'wp_kses_allowed_html',
Expand Down Expand Up @@ -319,11 +319,11 @@ public function process(PHP_CodeSniffer_File $phpcsFile, $stackPtr)
}

$functionName = $tokens[$stackPtr]['content'];
if (in_array($functionName, $this->autoEscapedFunctions) === true) {
if (in_array($functionName, self::$autoEscapedFunctions) === true) {
return;
}

if (in_array($functionName, $this->sanitizingFunctions) === false) {
if (in_array($functionName, self::$sanitizingFunctions) === false) {
$error = sprintf("Expected a sanitizing function (see Codex for 'Data Validation'), but instead saw '%s'", $tokens[$stackPtr]['content']);
$phpcsFile->addError($error, $stackPtr);
return;
Expand Down
22 changes: 22 additions & 0 deletions Tests/VIP/ValidatedSanitizedInputUnitTest.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php

do_something( $_POST ); // Warning, unusual usage

do_something_with( $_POST['hello'] ); // Error for no validation, Error for no sanitizing

esc_html( $_POST['foo'] ); // Error for no validation

if ( isset( $_POST['foo'] ) ) {
bar( $_POST['foo'] );} // Error for no sanitizing
}

// @TODO: Cover non-parenthesis'd conditions
// if ( isset( $_POST['foo'] ) )
// bar( $_POST['foo'] );


if ( isset( $_POST['foo'] ) ) {
bar( esc_html( $_POST['foo'] ) ); // Good, validated and sanitized
bar( $_POST['foo'] ); // Error, validated but not sanitized
bar( esc_html( $_POST['foo'] ) ); // Good, validated and sanitized
}
56 changes: 56 additions & 0 deletions Tests/VIP/ValidatedSanitizedInputUnitTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<?php
/**
* Unit test class for the ValidatedSanitizedInputSniff
*
* PHP version 5
*
* @category PHP
* @package PHP_CodeSniffer
* @author Shady Sharaf <shady@x-team.com>
* @link http://pear.php.net/package/PHP_CodeSniffer
*/

class WordPress_Tests_VIP_ValidatedSanitizedInputUnitTest extends AbstractSniffUnitTest
{


/**
* Returns the lines where errors should occur.
*
* The key of the array should represent the line number and the value
* should represent the number of errors that should occur on that line.
*
* @return array(int => int)
*/
public function getErrorList()
{
return array(
5 => 2,
7 => 1,
10 => 1,
20 => 1,
);

}//end getErrorList()


/**
* Returns the lines where warnings should occur.
*
* The key of the array should represent the line number and the value
* should represent the number of warnings that should occur on that line.
*
* @return array(int => int)
*/
public function getWarningList()
{
return array(
3 => 1,
);

}//end getWarningList()


}//end class

?>

1 comment on commit f90170d

@shadyvb
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@westonruter 🆘 This got here by mistake, should've been to the feature branch instead.

Please sign in to comment.