Repository: WordPress/WordPress-Coding-Standards (php)

Issue:
Does $_FILES really need wp_unslash?
## Bug Description
I'm using `wp_handle_upload` in one of my plugins. PHPCS complains about the `$_FILES` slice not being run through `wp_unslash`. However, [`wp_magic_quotes` does not add slashes to `$_FILES`](https://developer.wordpress.org/reference/functions/wp_magic_quotes/). 

Running an apparently superfluous `wp_unslash` on `$_FILES` is harmless on POSIX platforms, but [breaks file uploading on Windows](https://wordpress.org/support/topic/there-was-an-error-uploading-the-avatar-specified-file-failed-upload-test/).

## Minimal Code Snippet
```php
$mimetypes = [
	'jpg|jpeg|jpe' => 'image/jpeg',
	'gif'          => 'image/gif',
	'png'          => 'image/png',
];
$args = [
	'mimes'                    => $mimetypes,
	'test_form'                => false,
];
$avatar = \wp_handle_upload( \wp_unslash( $_FILES[ 'avatar' ], $args ) ); 
```

## Error Code
`WordPress.Security.ValidatedSanitizedInput.MissingUnslash`

## Environment

| Question               | Answer
| ------------------------| -------
| PHP version             | 7.3.6
| PHP_CodeSniffer version | 3.4.2
| WPCS version            | 2.1.1
| WPCS install type       | Composer project local

## Tested Against `develop` branch?
- [ ] I have verified the issue still exists in the `develop` branch of WPCS.

Pull request description:
Security/ValidatedSanitizedInput: use PHPCSutils and bug fixes
### ValidationHelper: bug fix - unsetting is not validation

A superglobal key being unset doesn't need sanitization or validation, however, the unset should _not_ be seen as validation for use of the superglobal key later on.

This bug was introduced long ago (0.3.0 - 64fab4f) and later moved into the `is_validated()` method via ba15db9

Discovered when making some changes to the tests for the `ValidatedSanitizedInput` sniff.

Includes dedicated test for this situation.

### ValidationHelper: bug fix - validation done in another scope

Discovered when making some changes to the tests for the `ValidatedSanitizedInput` sniff.

A number of the tests in that file are in the _global_ scope.
As things were, if validation was encountered for a particular superglobal key in a _closed_ scope (function, class) prior to the use of the superglobal key in the _global_ scope, the superglobal key would be seen as correctly validated, even though the code within the closed scope may not have been executed.

IMO tracking whether the code within the closed scope is executed is outside the scope of this sniff and a "missing validation" error should be thrown for those cases.

And while PHP 7.4+ arrow functions are _open_ scopes, validation done within an arrow function should not be counted as valid for superglobals used _outside the arrow function, though validation done outside an arrow function should IMO be okay for superglobals used _within_ an arrow function.

Fixed now.

Includes a minor tweak to the pre-existing tests for the case which made me discover the bug (superglobal use in global scope).
Includes additional tests with closed scopes within an already closed scope.
Includes additional tests documenting the behaviour around arrow functions.

### Security/ValidatedSanitizedInput: make sure all superglobals being examined are used in tests

The `$_SERVER` and `$_COOKIE` superglobals are being examined by the sniff, but there were no tests safeguarding this.

Fixed now by adjusting some pre-existing tests.

### Security/ValidatedSanitizedInput: use PHPCSUtils for superglobal determination [1]

Previously, the sniff would disregard the `$_SESSION` and `$_ENV` superglobals. This is now no longer the case when examining variables embedded in text strings.

This may have been on purpose, but I could not find any discussion about this and it seems more consistent (and more correct) to flag the unvalidated/unsanitized use of **all** superglobals.

Note: the `$GLOBALS` superglobal is exempt as in that case, any variable may be accessed (`$GLOBALS['var']`), not just superglobals (`$GLOBALS['_GET']`).

Includes tests.

### Security/ValidatedSanitizedInput: use PHPCSUtils for superglobal determination [2]

Previously, the sniff would disregard the `$_SESSION` and `$_ENV` superglobals. This will now no longer be the case when the sniff examines variable tokens.

I suspect these superglobals were originally disregarded as the values in these superglobals are not slashed by WP (see the below refs), but that doesn't mean the values shouldn't be validated and sanitized before use.

The strange thing is that `$_FILES` _was_ already being examined, even though it also isn't slashed by WP.
The sniff, however, would throw an incorrect "requires unslashing" error when the `$_FILES` superglobal was being used. Also see bug report #1720.

Either way, along the same lines of the previous commit, when examining variable tokens, the `ValidatedSanitizedInput` sniff will now take all superglobals into account, with the exception of the `$GLOBALS` superglobal (this last bit is the same as before).

As the values in the `$_FILES`, `$_SESSION` and `$_ENV` superglobals aren't slashed by WP, they get an exception for the "unslash" error.

And as the `Sniff::$input_superglobals` property is now unused, I'm removing it.

Includes tests.

Fixes #1720

Refs:
* https://developer.wordpress.org/reference/functions/wp_magic_quotes/
* https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/load.php#L1229-L1238

### Security/ValidatedSanitizedInput: add extra test for PHP 7.4+ null coalesce equals

PR #1684 (WPCS 2.1.0) already took care of the handling of validation via the null coalesce equals assignment operator, but there was one test case missing.

This adds that extra test as an additional safeguard.

### Security/ValidatedSanitizedInput: add test with PHP 8.0+ match [1]

When a superglobal is used in a match condition, it doesn't need to be unslashed/sanitized.

This is already handled correctly, this just adds a test to safeguard this.

### Security/ValidatedSanitizedInput: add tests with PHP 8.0+ match [2]

When a superglobal is used in the match return expressions, it should be validated, unslashed and sanitized.

This is already handled correctly, this just adds a test to safeguard this.

### Security/ValidatedSanitizedInput: make MissingUnslash message more informative

Previously, the error would read `$_POST data not unslashed...`. Now, the error message will show the exact variable with keys we're talking about like `$_POST['key'] not unslashed...`.

Patch:
diff --git a/WordPress/Helpers/ValidationHelper.php b/WordPress/Helpers/ValidationHelper.php
index d8cece82..d5a22d7d 100644
--- a/WordPress/Helpers/ValidationHelper.php
+++ b/WordPress/Helpers/ValidationHelper.php
@@ -11,6 +11,7 @@ namespace WordPressCS\WordPress\Helpers;
 
 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 @@ final class ValidationHelper {
 		// 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 8e642108..369ecf0f 100644
--- a/WordPress/Sniff.php
+++ b/WordPress/Sniff.php
@@ -21,23 +21,6 @@ use PHP_CodeSniffer\Sniffs\Sniff as PHPCS_Sniff;
  */
 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 cbbcab10..6f46261b 100644
--- a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php
+++ b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php
@@ -11,7 +11,9 @@ namespace WordPressCS\WordPress\Sniffs\Security;
 
 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 @@ class ValidatedSanitizedInputSniff extends Sniff {
 	 */
 	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 @@ class ValidatedSanitizedInputSniff extends Sniff {
 	 * @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
 		);
 	}
 }


Test patch:
diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.1.inc b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.1.inc
index 90dfd332..cc4edc14 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 92ec3256..065162c2 100644
--- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php
+++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php
@@ -100,6 +100,20 @@ final class ValidatedSanitizedInputUnitTest extends AbstractSniffUnitTest {
 					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':