diff --git a/WordPress/Helpers/ValidationHelper.php b/WordPress/Helpers/ValidationHelper.php index d8cece8205..d5a22d7d56 100644 --- a/WordPress/Helpers/ValidationHelper.php +++ b/WordPress/Helpers/ValidationHelper.php @@ -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; @@ -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', @@ -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; } diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 8e642108ae..369ecf0fc2 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -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. * diff --git a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php index cbbcab1059..6f46261b56 100644 --- a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php +++ b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php @@ -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; @@ -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 + */ + 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. * @@ -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; } @@ -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 ); } } diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.1.inc b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.1.inc index 90dfd3322b..cc4edc147a 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.1.inc +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.1.inc @@ -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() { @@ -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. @@ -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. } } @@ -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. } } @@ -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. + }; + } +} diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php index 92ec32566e..065162c2a8 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php @@ -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':