Skip to content

Commit

Permalink
Merge pull request #395 from WordPress-Coding-Standards/issue/172
Browse files Browse the repository at this point in the history 
Require unlashing of input superglobals
  • Loading branch information
@JDGrimes
JDGrimes committed Jun 1, 2015
2 parents 8ddcd18 + e9a212d commit 27d7dd2
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 31 deletions.
71 changes: 66 additions & 5 deletions WordPress/Sniff.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,19 @@ abstract class WordPress_Sniff implements PHP_CodeSniffer_Sniff {
'wp_safe_redirect' => true,
);

/**
* Sanitizing functions that implicitly unslash the data passed to them.
*
* @since 0.5.0
*
* @var array
*/
public static $unslashingSanitizingFunctions = array(
'absint' => true,
'is_array' => true,
'sanitize_key' => true,
);

/**
* Functions that format strings.
*
Expand All @@ -291,7 +304,6 @@ abstract class WordPress_Sniff implements PHP_CodeSniffer_Sniff {
'wp_sprintf' => true,
);


/**
* Functions which print output incorporating the values passed to them.
*
Expand Down Expand Up @@ -675,11 +687,13 @@ protected function is_safe_casted( $stackPtr ) {
*
* @since 0.5.0
*
* @param int $stackPtr The index of the token in the stack.
* @param int $stackPtr The index of the token in the stack.
* @param bool $require_unslash Whether to give an error if wp_unslash() isn't
* used on the variable before sanitization.
*
* @return bool Whether the token being sanitized.
*/
protected function is_sanitized( $stackPtr ) {
protected function is_sanitized( $stackPtr, $require_unslash = false ) {

// First we check if it is being casted to a safe value.
if ( $this->is_safe_casted( $stackPtr ) ) {
Expand All @@ -688,14 +702,16 @@ protected function is_sanitized( $stackPtr ) {

// If this isn't within a function call, we know already that it's not safe.
if ( ! isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) {
if ( $require_unslash ) {
$this->add_unslash_error( $stackPtr );
}
return false;
}

// Get the function that it's in.
$function_closer = end( $this->tokens[ $stackPtr ]['nested_parenthesis'] );
$function_opener = key( $this->tokens[ $stackPtr ]['nested_parenthesis'] );
$functionPtr = $function_opener - 1;
$function = $this->tokens[ $functionPtr ];
$function = $this->tokens[ $function_opener - 1 ];

// If it is just being unset, the value isn't used at all, so it's safe.
if ( T_UNSET === $function['code'] ) {
Expand All @@ -704,11 +720,34 @@ protected function is_sanitized( $stackPtr ) {

// If this isn't a call to a function, it sure isn't sanitizing function.
if ( T_STRING !== $function['code'] ) {
if ( $require_unslash ) {
$this->add_unslash_error( $stackPtr );
}
return false;
}

$functionName = $function['content'];

// Check if wp_unslash() is being used.
if ( 'wp_unslash' === $functionName ) {

$is_unslashed = true;

$function_closer = prev( $this->tokens[ $stackPtr ]['nested_parenthesis'] );

// If there is no other function being used, this value is unsanitized.
if ( ! $function_closer ) {
return false;
}

$function_opener = key( $this->tokens[ $stackPtr ]['nested_parenthesis'] );
$functionName = $this->tokens[ $function_opener - 1 ]['content'];

} else {

$is_unslashed = false;
}

// Arrays might be sanitized via array_map().
if ( 'array_map' === $functionName ) {

Expand All @@ -726,10 +765,32 @@ protected function is_sanitized( $stackPtr ) {
}
}

// If slashing is required, give an error.
if ( ! $is_unslashed && $require_unslash && ! isset( self::$unslashingSanitizingFunctions[ $functionName ] ) ) {
$this->add_unslash_error( $stackPtr );
}

// Check if this is a sanitizing function.
return isset( self::$sanitizingFunctions[ $functionName ] );
}

/**
* Add an error for missing use of wp_unslash().
*
* @since 0.5.0
*
* @param int $stackPtr The index of the token in the stack.
*/
public function add_unslash_error( $stackPtr ) {

$this->phpcsFile->addError(
'Missing wp_unslash() before sanitization.',
$stackPtr,
'MissingUnslash',
array( $this->tokens[ $stackPtr ]['content'] )
);
}

/**
* Get the index key of an array variable.
*
Expand Down
16 changes: 15 additions & 1 deletion WordPress/Sniffs/VIP/ValidatedSanitizedInputSniff.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,15 @@ class WordPress_Sniffs_VIP_ValidatedSanitizedInputSniff extends WordPress_Sniff
*/
public $customSanitizingFunctions = array();

/**
* Custom sanitizing functions that implicitly unslash the values passed to them.
*
* @since 0.5.0
*
* @var string[]
*/
public $customUnslashingSanitizingFunctions = array();

/**
* Whether the custom list of functions has been added to the defaults yet.
*
Expand Down Expand Up @@ -67,6 +76,11 @@ public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) {
array_flip( $this->customSanitizingFunctions )
);

WordPress_Sniff::$unslashingSanitizingFunctions = array_merge(
WordPress_Sniff::$unslashingSanitizingFunctions,
array_flip( $this->customUnslashingSanitizingFunctions )
);

self::$addedCustomFunctions = true;
}

Expand Down Expand Up @@ -123,7 +137,7 @@ public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) {
}

// Now look for sanitizing functions
if ( ! $this->is_sanitized( $stackPtr ) ) {
if ( ! $this->is_sanitized( $stackPtr, true ) ) {
$phpcsFile->addError( 'Detected usage of a non-sanitized input variable: %s', $stackPtr, 'InputNotSanitized', array( $tokens[ $stackPtr ]['content'] ) );
}

Expand Down
55 changes: 33 additions & 22 deletions WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@

do_something( $_POST ); // OK

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

echo sanitize_text_field( $_POST['foo1'] ); // Error for no validation
echo sanitize_text_field( wp_unslash( $_POST['foo1'] ) ); // Error for no validation

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

// @TODO: Cover non-parenthesis'd conditions
Expand All @@ -16,9 +16,9 @@ if ( isset( $_POST['foo2'] ) ) {


if ( isset( $_POST['foo3'] ) ) {
bar( sanitize_text_field( $_POST['foo3'] ) ); // Good, validated and sanitized
bar( $_POST['foo3'] ); // Error, validated but not sanitized
bar( sanitize_text_field( $_POST['foo3'] ) ); // Good, validated and sanitized
bar( sanitize_text_field( wp_unslash( $_POST['foo3'] ) ) ); // Good, validated and sanitized
bar( wp_unslash( $_POST['foo3'] ) ); // Error, validated but not sanitized
bar( sanitize_text_field( wp_unslash( $_POST['foo3'] ) ) ); // Good, validated and sanitized
}

// Should all be OK
Expand All @@ -30,25 +30,25 @@ $empty = (
empty( $_POST['foo4'] )
);

$foo = $_POST['bar']; // Bad x 2
$foo = $_POST['bar']; // Bad x 3

function foo() {
// Ok, if WordPress_Sniffs_VIP_ValidatedSanitizedInputSniff::$check_validation_in_scope_only == false
if ( ! isset( $_REQUEST['bar1'] ) || ! foo( sanitize_text_field( $_REQUEST['bar1'] ) ) ) {
if ( ! isset( $_REQUEST['bar1'] ) || ! foo( sanitize_text_field( wp_unslash( $_REQUEST['bar1'] ) ) ) ) {
wp_die( 1 );
}
}

// Ok, if WordPress_Sniffs_VIP_ValidatedSanitizedInputSniff::$check_validation_in_scope_only == false
if ( ! isset( $_REQUEST['bar2'] ) || ! foo( sanitize_text_field( $_REQUEST['bar2'] ) ) ) { // Ok
if ( ! isset( $_REQUEST['bar2'] ) || ! foo( sanitize_text_field( wp_unslash( $_REQUEST['bar2'] ) ) ) ) { // Ok
wp_die( 1 );
}

function bar() {
if ( ! isset( $_GET['test'] ) ) {
return ;
}
echo sanitize_text_field( $_GET['test'] ); // Good
echo sanitize_text_field( wp_unslash( $_GET['test'] ) ); // Good
}

$_REQUEST['wp_customize'] = 'on'; // ok
Expand All @@ -69,32 +69,43 @@ some_func( $some_arg, (int) $_GET['test'] ); // ok

function zebra() {
if ( isset( $_GET['foo'], $_POST['bar'] ) ) {
echo sanitize_text_field( $_POST['bar'] ); // ok
echo sanitize_text_field( wp_unslash( $_POST['bar'] ) ); // ok
}
}

echo $_GET['test']; // WPCS: sanitization OK.

echo array_map( 'sanitize_text_field', $_GET['test'] ); // OK
echo array_map( 'foo', $_GET['test'] ); // Bad
echo array_map( $something, $_GET['test'] ); // Bad
echo array_map( array( $obj, 'func' ), $_GET['test'] ); // Bad
echo array_map( array( $obj, 'sanitize_text_field' ), $_GET['test'] ); // Bad
echo array_map( 'sanitize_text_field', wp_unslash( $_GET['test'] ) ); // OK
echo array_map( 'foo', wp_unslash( $_GET['test'] ) ); // Bad
echo array_map( $something, wp_unslash( $_GET['test'] ) ); // Bad
echo array_map( array( $obj, 'func' ), wp_unslash( $_GET['test'] ) ); // Bad
echo array_map( array( $obj, 'sanitize_text_field' ), wp_unslash( $_GET['test'] ) ); // Bad

// Sanitized but not validated.
$foo = (int) $_POST['foo6']; // Bad

// Non-assignment checks are OK.
if ( 'bar' === $_POST['foo'] ) {} // OK
if ( $_GET['test'] != 'a' ) {} // OK
if ( 'bar' === do_something( $_POST['foo'] ) ) {} // Bad
if ( 'bar' === do_something( wp_unslash( $_POST['foo'] ) ) ) {} // Bad

switch ( $_POST['foo'] ) {} // OK
switch ( do_something( $_POST['foo'] ) ) {} // Bad
switch ( do_something( wp_unslash( $_POST['foo'] ) ) ) {} // Bad

// Sanitization is required even when the value is being escaped.
echo esc_html( $_POST['foo'] ); // Bad
echo esc_html( sanitize_text_field( $_POST['foo'] ) ); // OK
echo esc_html( wp_unslash( $_POST['foo'] ) ); // Bad
echo esc_html( sanitize_text_field( wp_unslash( $_POST['foo'] ) ) ); // OK

$current_tax_slug = isset( $_GET['a'] ) ? sanitize_key( $_GET['a'] ) : false;
$current_tax_slug = isset( $_GET['a'] ) ? $_GET['a'] : false;
$current_tax_slug = isset( $_GET['a'] ) ? sanitize_key( $_GET['a'] ) : false; // OK
$current_tax_slug = isset( $_GET['a'] ) ? $_GET['a'] : false; // Bad x 2
$current_tax_slug = isset( $_GET['a'] ) ? wp_unslash( $_GET['a'] ) : false; // Bad
$current_tax_slug = isset( $_GET['a'] ) ? sanitize_text_field( wp_unslash( $_GET['a'] ) ) : false; // OK

echo sanitize_text_field( $_POST['foo545'] ); // Error for no validation, unslashing
echo array_map( 'sanitize_text_field', $_GET['test'] ); // Bad, no unslashing
echo array_map( 'sanitize_key', $_GET['test'] ); // OK

foo( absint( $_GET['foo'] ) ); // OK
$ids = array_map( 'absint', $_GET['test'] ); // OK

if ( is_array( $_GET['test'] ) ) {} // OK
9 changes: 6 additions & 3 deletions WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ class WordPress_Tests_VIP_ValidatedSanitizedInputUnitTest extends AbstractSniffU
public function getErrorList()
{
return array(
5 => 2,
5 => 3,
7 => 1,
10 => 1,
20 => 1,
33 => 2,
33 => 3,
65 => 1,
79 => 1,
80 => 1,
Expand All @@ -39,7 +39,10 @@ public function getErrorList()
90 => 1,
93 => 1,
96 => 1,
100 => 1,
100 => 2,
101 => 1,
104 => 2,
105 => 1,
);

}//end getErrorList()
Expand Down

0 comments on commit 27d7dd2

Please sign in to comment.