Security/ValidatedSanitizedInput: use PHPCSutils and bug fixes

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;
 			}

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.
 	 *

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<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.
 	 *
@@ -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
 		);
 	}
 }

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.
+		};
+	}
+}

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':