Merge pull request #2234 from WordPress/feature/move-is_in_array_comp…

…arison-to-helper-class

WordPress/Helpers/ContextHelper.php

@@ -91,6 +91,27 @@ final class ContextHelper {
 		'key_exists'       => true, // Alias.
 	);
 
+	/**
+	 * Array functions to compare a $needle to a predefined set of values.
+	 *
+	 * If the value is set to an array, the parameter specified in the array is
+	 * required for the function call to be considered as a comparison.
+	 *
+	 * @since 2.1.0
+	 * @since 3.0.0 - Moved from the Sniff class to this class.
+	 *              - The property visibility was changed from `protected` to `private static`.
+	 *
+	 * @var array <string function name> => <true|array>
+	 */
+	private static $arrayCompareFunctions = array(
+		'in_array'     => true,
+		'array_search' => true,
+		'array_keys'   => array(
+			'position' => 2,
+			'name'     => 'filter_value',
+		),
+	);
+
 	/**
 	 * Check if a particular token acts - statically or non-statically - on an object.
 	 *
@@ -303,4 +324,38 @@ public static function is_safe_casted( File $phpcsFile, $stackPtr ) {
 
 		return isset( self::$safe_casts[ $tokens[ $prev ]['code'] ] );
 	}
+
+	/**
+	 * Check if a token is inside of an array-value comparison function.
+	 *
+	 * @since 2.1.0
+	 * @since 3.0.0 - Moved from the Sniff class to this class.
+	 *              - The method visibility was changed from `protected` to `public static`.
+	 *
+	 * @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned.
+	 * @param int                         $stackPtr  The index of the token in the stack.
+	 *
+	 * @return bool Whether the token is (part of) a parameter to an
+	 *              array-value comparison function.
+	 */
+	public static function is_in_array_comparison( File $phpcsFile, $stackPtr ) {
+		$function_ptr = self::is_in_function_call( $phpcsFile, $stackPtr, self::$arrayCompareFunctions, true, true );
+		if ( false === $function_ptr ) {
+			return false;
+		}
+
+		$tokens        = $phpcsFile->getTokens();
+		$function_name = strtolower( $tokens[ $function_ptr ]['content'] );
+		if ( true === self::$arrayCompareFunctions[ $function_name ] ) {
+			return true;
+		}
+
+		$target_param = self::$arrayCompareFunctions[ $function_name ];
+		$found_param  = PassedParameters::getParameter( $phpcsFile, $function_ptr, $target_param['position'], $target_param['name'] );
+		if ( false !== $found_param ) {
+			return true;
+		}
+
+		return false;
+	}
 }

WordPress/Sniff.php

@@ -267,22 +267,6 @@ abstract class Sniff implements PHPCS_Sniff {
 		'map_deep'  => 2,
 	);
 
-	/**
-	 * Array functions to compare a $needle to a predefined set of values.
-	 *
-	 * If the value is set to an integer, the function needs to have at least that
-	 * many parameters for it to be considered as a comparison.
-	 *
-	 * @since 2.1.0
-	 *
-	 * @var array <string function name> => <true|int>
-	 */
-	protected $arrayCompareFunctions = array(
-		'in_array'     => true,
-		'array_search' => true,
-		'array_keys'   => 2,
-	);
-
 	/**
 	 * Functions that format strings.
 	 *
@@ -837,34 +821,6 @@ protected function is_validated( $stackPtr, $array_keys = array(), $in_condition
 		return false;
 	}
 
-	/**
-	 * Check if a token is inside of an array-value comparison function.
-	 *
-	 * @since 2.1.0
-	 *
-	 * @param int $stackPtr The index of the token in the stack.
-	 *
-	 * @return bool Whether the token is (part of) a parameter to an
-	 *              array-value comparison function.
-	 */
-	protected function is_in_array_comparison( $stackPtr ) {
-		$function_ptr = ContextHelper::is_in_function_call( $this->phpcsFile, $stackPtr, $this->arrayCompareFunctions, true, true );
-		if ( false === $function_ptr ) {
-			return false;
-		}
-
-		$function_name = $this->tokens[ $function_ptr ]['content'];
-		if ( true === $this->arrayCompareFunctions[ $function_name ] ) {
-			return true;
-		}
-
-		if ( PassedParameters::getParameterCount( $this->phpcsFile, $function_ptr ) >= $this->arrayCompareFunctions[ $function_name ] ) {
-			return true;
-		}
-
-		return false;
-	}
-
 	/**
 	 * Determine whether an arbitrary T_STRING token is the use of a global constant.
 	 *

WordPress/Sniffs/Security/NonceVerificationSniff.php

@@ -203,7 +203,7 @@ private function has_nonce_check( $stackPtr ) {
 		if ( ContextHelper::is_in_isset_or_empty( $this->phpcsFile, $stackPtr )
 			|| ContextHelper::is_in_type_test( $this->phpcsFile, $stackPtr )
 			|| VariableHelper::is_comparison( $this->phpcsFile, $stackPtr )
-			|| $this->is_in_array_comparison( $stackPtr )
+			|| ContextHelper::is_in_array_comparison( $this->phpcsFile, $stackPtr )
 			|| ContextHelper::is_in_function_call( $this->phpcsFile, $stackPtr, $this->unslashingFunctions ) !== false
 			|| $this->is_only_sanitized( $stackPtr )
 		) {

WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php

@@ -188,7 +188,7 @@ function( $embed ) {
 		}
 
 		// If this is a comparison using the array comparison functions, sanitization isn't needed.
-		if ( $this->is_in_array_comparison( $stackPtr ) ) {
+		if ( ContextHelper::is_in_array_comparison( $this->phpcsFile, $stackPtr ) ) {
 			return;
 		}
 

WordPress/Tests/Security/NonceVerificationUnitTest.inc

@@ -362,3 +362,32 @@ function allow_in_array_key_exists_before_noncecheck_with_named_params() {
 
 	wp_verify_nonce( 'some_action' );
 }
+
+// Tests specifically for the ContextHelper::is_in_array_comparison().
+function allow_for_array_comparison_in_condition() {
+	if ( Array_Keys( $_GET['actions'], 'my_action', true ) ) { // OK.
+		check_admin_referer( 'foo' );
+		foo();
+	}
+}
+
+function disallow_for_non_array_comparison_in_condition() {
+	if ( array_keys( $_GET['actions'] ) ) { // Bad.
+		check_admin_referer( 'foo' );
+		foo();
+	}
+}
+
+function allow_for_array_comparison_in_condition_with_named_params() {
+	if ( array_keys( filter_value: 'my_action', array: $_GET['actions'], strict: true, ) ) { // OK.
+		check_admin_referer( 'foo' );
+		foo();
+	}
+}
+
+function disallow_for_non_array_comparison_in_condition_with_named_params() {
+	if ( array_keys( strict: true, array: $_GET['actions'], ) ) { // Bad, missing $filter_value param. Invalid function call, but not our concern.
+		check_admin_referer( 'foo' );
+		foo();
+	}
+}

WordPress/Tests/Security/NonceVerificationUnitTest.php

@@ -23,6 +23,7 @@
  * @covers \WordPressCS\WordPress\Helpers\ContextHelper::is_in_function_call
  * @covers \WordPressCS\WordPress\Helpers\ContextHelper::is_in_type_test
  * @covers \WordPressCS\WordPress\Helpers\ContextHelper::is_in_isset_or_empty
+ * @covers \WordPressCS\WordPress\Helpers\ContextHelper::is_in_array_comparison
  * @covers \WordPressCS\WordPress\Sniffs\Security\NonceVerificationSniff
  */
 final class NonceVerificationUnitTest extends AbstractSniffUnitTest {
@@ -74,7 +75,9 @@ public function getErrorList() {
 	 * @return array <int line number> => <int number of warnings>
 	 */
 	public function getWarningList() {
-		return array();
+		return array(
+			375 => 1,
+			389 => 1,
+		);
 	}
-
 }