Skip to content

Commit

Permalink
Require unlashing of input superglobals
Browse files Browse the repository at this point in the history 
This updates the validated/sanitized input sniff to also check for
slashing. This could have been made into another sniff instead,
however, it would have required lots of duplicated logic and this sniff
would need to be updated to accommodate the use of `wp_unslash()`
anyway.

Currently only `wp_unslash()` is recognized as an unlashing function,
but this can be changed in the future if needed.

The sniff currently requires that `wp_unslash()` be used *before* the
data is passed through a sanitizing function. Sanitizing first and then
wrapping that in `wp_unslash()` is not accepted.

The error for missing the use of `wp_unslash()` is independent of the
missing sanitizing function error, so an error will be given for
missing use of an unslashing function whether or not a sanitizing
function is used, and vice versa.

Unslashing is not required when sanitization is provided via casting,
or when certain sanitization functions are used which implicitly or
explicitly perform an unslash or for which unslashing isn’t necessary.
`absint()` implicitly unslashes, and `sanitize_key()` will remove
slashes explicitly. And unslashing isn’t necessary when testing a value
with `is_array()`. This list can be expanded in the future, and is
configurable via the `customUnslashingSanitizingFunctions` property.

See #172
  • Loading branch information
@JDGrimes
JDGrimes committed May 30, 2015
1 parent d2482fb commit e9a212d
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 e9a212d

Please sign in to comment.