Skip to content

Commit

Permalink
Merge pull request #2362 from WordPress/feature/validatedsanitizedinp…
Browse files Browse the repository at this point in the history 
…ut-use-phpcsutils-and-more-fixes
  • Loading branch information
@GaryJones
GaryJones authored Aug 19, 2023
2 parents 4e8f340 + 5f8fe49 commit 1eb217b
Show file tree
Hide file tree
Showing 5 changed files with 198 additions and 44 deletions.
19 changes: 18 additions & 1 deletion WordPress/Helpers/ValidationHelper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Util\Tokens;
use PHPCSUtils\Tokens\Collections;
use PHPCSUtils\Utils\Conditions;
use PHPCSUtils\Utils\Context;
use PHPCSUtils\Utils\PassedParameters;
Expand All @@ -36,7 +37,6 @@ final class ValidationHelper {
private static $targets = array(
\T_ISSET => 'construct',
\T_EMPTY => 'construct',
\T_UNSET => 'construct',
\T_STRING => 'function_call',
\T_COALESCE => 'coalesce',
\T_COALESCE_EQUAL => 'coalesce',
Expand Down Expand Up @@ -156,6 +156,23 @@ public static function is_validated( File $phpcsFile, $stackPtr, $array_keys = a
// phpcs:ignore Generic.CodeAnalysis.JumbledIncrementer.Found -- On purpose, see below.
for ( $i = ( $scope_start + 1 ); $i < $scope_end; $i++ ) {

if ( isset( Collections::closedScopes()[ $tokens[ $i ]['code'] ] )
&& isset( $tokens[ $i ]['scope_closer'] )
) {
// Jump over nested closed scopes as validation done within those does not apply.
$i = $tokens[ $i ]['scope_closer'];
continue;
}

if ( \T_FN === $tokens[ $i ]['code']
&& isset( $tokens[ $i ]['scope_closer'] )
&& $tokens[ $i ]['scope_closer'] < $scope_end
) {
// Jump over nested arrow functions as long as the current variable isn't *in* the arrow function.
$i = $tokens[ $i ]['scope_closer'];
continue;
}

if ( isset( self::$targets[ $tokens[ $i ]['code'] ] ) === false ) {
continue;
}
Expand Down
17 changes: 0 additions & 17 deletions WordPress/Sniff.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,6 @@
*/
abstract class Sniff implements PHPCS_Sniff {

/**
* A list of superglobals that incorporate user input.
*
* @since 0.5.0
* @since 0.11.0 Changed from static to non-static.
*
* @var string[]
*/
protected $input_superglobals = array(
'$_COOKIE',
'$_GET',
'$_FILES',
'$_POST',
'$_REQUEST',
'$_SERVER',
);

/**
* The current file being sniffed.
*
Expand Down
64 changes: 54 additions & 10 deletions WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@

use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Util\Tokens;
use PHPCSUtils\Utils\Context;
use PHPCSUtils\Utils\TextStrings;
use PHPCSUtils\Utils\Variables;
use WordPressCS\WordPress\Helpers\ContextHelper;
use WordPressCS\WordPress\Helpers\SanitizationHelperTrait;
use WordPressCS\WordPress\Helpers\ValidationHelper;
Expand Down Expand Up @@ -43,6 +45,23 @@ class ValidatedSanitizedInputSniff extends Sniff {
*/
public $check_validation_in_scope_only = false;

/**
* Superglobals for which the values will be slashed by WP.
*
* @link https://developer.wordpress.org/reference/functions/wp_magic_quotes/
*
* @since 3.0.0
*
* @var array<string, true>
*/
private $slashed_superglobals = array(
'$_COOKIE' => true,
'$_GET' => true,
'$_POST' => true,
'$_REQUEST' => true,
'$_SERVER' => true,
);

/**
* Returns an array of tokens this test wants to listen for.
*
Expand All @@ -65,29 +84,44 @@ public function register() {
*/
public function process_token( $stackPtr ) {

$superglobals = $this->input_superglobals;

// Handling string interpolation.
if ( \T_DOUBLE_QUOTED_STRING === $this->tokens[ $stackPtr ]['code']
|| \T_HEREDOC === $this->tokens[ $stackPtr ]['code']
) {
// Retrieve all embeds, but use only the initial variable name part.
$interpolated_variables = array_map(
function ( $embed ) {
return '$' . preg_replace( '`^(\{?\$\{?\(?)([a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*)(.*)$`', '$2', $embed );
static function ( $embed ) {
return preg_replace( '`^(\{?\$\{?\(?)([a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*)(.*)$`', '$2', $embed );
},
TextStrings::getEmbeds( $this->tokens[ $stackPtr ]['content'] )
);

foreach ( array_intersect( $interpolated_variables, $superglobals ) as $bad_variable ) {
// Filter the embeds down to superglobals only.
$interpolated_superglobals = array_filter(
$interpolated_variables,
static function ( $var_name ) {
return ( 'GLOBALS' !== $var_name && Variables::isSuperglobalName( $var_name ) );
}
);

foreach ( $interpolated_superglobals as $bad_variable ) {
$this->phpcsFile->addError( 'Detected usage of a non-sanitized, non-validated input variable %s: %s', $stackPtr, 'InputNotValidatedNotSanitized', array( $bad_variable, $this->tokens[ $stackPtr ]['content'] ) );
}

return;
}

// Check if this is a superglobal.
if ( ! \in_array( $this->tokens[ $stackPtr ]['content'], $superglobals, true ) ) {
/* Handle variables */

// Check if this is a superglobal we want to examine.
if ( '$GLOBALS' === $this->tokens[ $stackPtr ]['content']
|| Variables::isSuperglobalName( $this->tokens[ $stackPtr ]['content'] ) === false
) {
return;
}

// If the variable is being unset, we don't care about it.
if ( Context::inUnset( $this->phpcsFile, $stackPtr ) ) {
return;
}

Expand Down Expand Up @@ -188,13 +222,23 @@ function ( $embed ) {
* @return void
*/
public function add_unslash_error( File $phpcsFile, $stackPtr ) {
$tokens = $phpcsFile->getTokens();
$tokens = $phpcsFile->getTokens();
$var_name = $tokens[ $stackPtr ]['content'];

if ( isset( $this->slashed_superglobals[ $var_name ] ) === false ) {
// WP doesn't slash these, so they don't need unslashing.
return;
}

// We know there will be array keys as that's checked in the process_token() method.
$array_keys = VariableHelper::get_array_access_keys( $phpcsFile, $stackPtr );
$error_data = array( $var_name . '[' . implode( '][', $array_keys ) . ']' );

$phpcsFile->addError(
'%s data not unslashed before sanitization. Use wp_unslash() or similar',
'%s not unslashed before sanitization. Use wp_unslash() or similar',
$stackPtr,
'MissingUnslash',
array( $tokens[ $stackPtr ]['content'] )
$error_data
);
}
}
128 changes: 112 additions & 16 deletions WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.1.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ unset( $_GET['test'] ); // Ok.

output( "some string {$_POST['some_var']}" ); // Bad.

echo (int) $_GET['test']; // Ok.
echo isset( $_GET['test'] ) ? (int) $_GET['test'] : 0; // Ok.
some_func( $some_arg, (int) $_GET['test'] ); // Ok.

function zebra() {
Expand Down Expand Up @@ -105,7 +105,7 @@ echo sanitize_text_field( $_POST['foo545'] ); // Error for no validation, unslas
echo array_map( 'sanitize_text_field', $_GET['test'] ); // Bad, no unslashing.
echo Array_Map( 'sanitize_key', $_GET['test'] ); // Ok.

foo( AbsINT( $_GET['foo'] ) ); // Ok.
isset( $_GET['foo'] ) && foo( AbsINT( $_GET['foo'] ) ); // Ok.
$ids = array_map( 'absint', $_GET['test'] ); // Ok.

if ( is_array( $_GET['test'] ) ) {} // Ok.
Expand Down Expand Up @@ -232,17 +232,17 @@ function test_allow_array_key_exists_alias() {
}
}
function test_correct_multi_level_array_validation() {
if ( isset( $_POST['toplevel']['sublevel'] ) ) {
$id = (int) $_POST['toplevel']; // OK, if a subkey exists, the top level key *must* also exist.
$id = (int) $_POST['toplevel']['sublevel']; // OK.
$id = (int) $_POST['toplevel']['sublevel']['subsub']; // Bad x1 - validate, this sub has not been validated.
if ( isset( $_COOKIE['toplevel']['sublevel'] ) ) {
$id = (int) $_COOKIE['toplevel']; // OK, if a subkey exists, the top level key *must* also exist.
$id = (int) $_COOKIE['toplevel']['sublevel']; // OK.
$id = (int) $_COOKIE['toplevel']['sublevel']['subsub']; // Bad x1 - validate, this sub has not been validated.
}

if ( array_key_exists( 'bar', $_POST['foo'] ) ) {
$id = (int) $_POST['bar']; // Bad x 1 - validate.
$id = (int) $_POST['foo']; // OK.
$id = (int) $_POST['foo']['bar']; // OK.
$id = (int) $_POST['foo']['bar']['baz']; // Bad x 1 - validate.
if ( array_key_exists( 'bar', $_COOKIE['foo'] ) ) {
$id = (int) $_COOKIE['bar']; // Bad x 1 - validate.
$id = (int) $_COOKIE['foo']; // OK.
$id = (int) $_COOKIE['foo']['bar']; // OK.
$id = (int) $_COOKIE['foo']['bar']['baz']; // Bad x 1 - validate.
}
}

Expand Down Expand Up @@ -380,11 +380,11 @@ function test_allow_array_key_exists_with_named_params_multilevel_access_wrong_p
}

function test_allow_array_key_exists_with_named_params_multilevel_access_test() {
if ( array_key_exists( array: $_POST['foo'], key: 'bar' ) ) {
$id = (int) $_POST['bar']; // Bad x 1 - validate.
$id = (int) $_POST['foo']; // OK.
$id = (int) $_POST['foo']['bar']; // OK.
$id = (int) $_POST['foo']['bar']['baz']; // Bad x 1 - validate.
if ( array_key_exists( array: $_SERVER['foo'], key: 'bar' ) ) {
$id = (int) $_SERVER['bar']; // Bad x 1 - validate.
$id = (int) $_SERVER['foo']; // OK.
$id = (int) $_SERVER['foo']['bar']; // OK.
$id = (int) $_SERVER['foo']['bar']['baz']; // Bad x 1 - validate.
}
}

Expand All @@ -404,3 +404,99 @@ function test_ignore_array_key_exists_as_first_class_callable() {
$callback = array_key_exists(...);
$id = (int) $_POST['foo']; // Bad.
}

/*
* Unsetting a superglobal key is not the same as validating it.
*/
function test_unset_is_not_validation() {
unset( $_REQUEST['key'] );
$id = (int) $_REQUEST['key']; // Bad, missing validation.
}

/*
* Only count a variable as validated if the validation happened in the same scope.
*/
function test_nested_closed_scopes() {
$closure = function() {
return isset( $_POST['bar'] );
};

$anon = new class() {
public function __construct() {
if ( isset( $_POST['bar'] ) ) {
// Do something.
}
}
};

$arrow = fn() => isset( $_POST['bar'] );

$id = (int) $_POST['bar']; // Bad x 1 - validate.
}

function test_arrow_open_scope() {
if ( ! isset( $_SERVER['key'] ) ) {
return;
}

$arrow = fn() => (int) $_SERVER['key']; // OK, validation outside the scope of the arrow function counts.

$arrow = fn() => isset( $_SERVER['abc'] ) ? (int) $_SERVER['abc'] : 0; // OK.
}

// Ensure the sniff flags $_ENV and $_SESSION too.
function test_examine_additional_superglobals_in_textstrings() {
$text = "Use {$_SESSION['key']} for something"; // Bad.
$text = "Use {$_ENV['key']} for something"; // Bad.
$text = "Use {$GLOBALS['key']} for something"; // OK.
}

function test_examine_additional_superglobals_as_vars() {
$key = sanitize_text_field( $_SESSION['key'] ); // Bad - missing validation.
$key = sanitize_text_field( $_ENV['key'] ); // Bad - missing validation.
$key = sanitize_text_field( $_FILES['key'] ); // Bad - missing validation.

if ( isset( $_SESSION['key'], $_ENV['key'], $_FILES['key'] ) === false ) {
return;
}

// OK.
$key = sanitize_text_field( wp_unslash( $_SESSION['key'] ) );
$key = sanitize_text_field( wp_unslash( $_ENV['key'] ) );
$key = sanitize_text_field( wp_unslash( $_FILES['key'] ) );

// OK - unslashing not needed for $_SESSION, $_ENV and $_FILES.
$key = sanitize_text_field( $_SESSION['key'] );
$key = sanitize_text_field( $_ENV['key'] );
$key = sanitize_text_field( $_FILES['key'] );

// Bad - missing sanitization.
do_something( $_SESSION['key'] );
do_something( $_ENV['key'] );
do_something( $_FILES['key'] );
}

function test_null_coalesce_equals_validation_extra_safeguard() {
$_POST['key'] ??= 'default'; // OK, assignment.
$key = $_POST['key']; // Bad, missing unslash + sanitization, validation okay.
}

function test_in_match_condition_is_regarded_as_comparison() {
if ( isset( $_REQUEST['key'] ) ) {
$test = match( $_REQUEST['key'] ) {
'valueA' => 'A',
default => 'B',
};
}
}

function test_in_match_condition_is_regarded_as_comparison() {
if ( isset( $_REQUEST['keyA'], $_REQUEST['keyB'], $_REQUEST['keyC'] ) ) {
$test = match( $toggle ) {
true => sanitize_text_field( wp_unslash( $_REQUEST['keyA'] ) ), // OK.
false => sanitize_text_field( $_REQUEST['keyB'] ), // Bad - missing unslash.
10 => wp_unslash( $_REQUEST['keyC'] ), // Bad - missing sanitization.
default => $_REQUEST['keyD'], // Bad - missing sanitization, unslash, validation.
};
}
}
14 changes: 14 additions & 0 deletions WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,20 @@ public function getErrorList( $testFile = '' ) {
387 => 1,
397 => 1,
405 => 1,
413 => 1,
434 => 1,
449 => 1,
450 => 1,
455 => 1,
456 => 1,
457 => 1,
474 => 1,
475 => 1,
476 => 1,
481 => 2,
497 => 1,
498 => 1,
499 => 3,
);

case 'ValidatedSanitizedInputUnitTest.2.inc':
Expand Down

0 comments on commit 1eb217b

Please sign in to comment.