Merge pull request #1668 from WordPress-Coding-Standards/feature/new-utility-method-is-in-function-call

New utility method Sniff::is_in_function_call() + implement use of it

Author: GaryJones

WordPress/Sniff.php

@@ -1462,16 +1462,9 @@ protected function is_in_isset_or_empty( $stackPtr ) {
 			return true;
 		}
 
-		if ( \T_STRING === $previous_code && 'array_key_exists' === $this->tokens[ $previous_non_empty ]['content'] ) {
-			if ( $this->is_class_object_call( $previous_non_empty ) === true ) {
-				return false;
-			}
-
-			if ( $this->is_token_namespaced( $previous_non_empty ) === true ) {
-				return false;
-			}
-
-			$second_param = $this->get_function_call_parameter( $previous_non_empty, 2 );
+		$functionPtr = $this->is_in_function_call( $stackPtr, array( 'array_key_exists' => true ) );
+		if ( false !== $functionPtr ) {
+			$second_param = $this->get_function_call_parameter( $functionPtr, 2 );
 			if ( $stackPtr >= $second_param['start'] && $stackPtr <= $second_param['end'] ) {
 				return true;
 			}
@@ -1545,6 +1538,85 @@ protected function is_token_namespaced( $stackPtr ) {
 		return true;
 	}
 
+	/**
+	 * Check if a token is (part of) a parameter for a function call to a select list of functions.
+	 *
+	 * This is useful, for instance, when trying to determine the context a variable is used in.
+	 *
+	 * For example: this function could be used to determine if the variable `$foo` is used
+	 * in a global function call to the function `is_foo()`.
+	 * In that case, a call to this function would return the stackPtr to the T_STRING `is_foo`
+	 * for code like: `is_foo( $foo, 'some_other_param' )`, while it would return `false` for
+	 * the following code `is_bar( $foo, 'some_other_param' )`.
+	 *
+	 * @since 2.1.0
+	 *
+	 * @param int   $stackPtr        The index of the token in the stack.
+	 * @param array $valid_functions List of valid function names.
+	 *                               Note: The keys to this array should be the function names
+	 *                               in lowercase. Values are irrelevant.
+	 * @param bool  $global          Optional. Whether to make sure that the function call is
+	 *                               to a global function. If `false`, calls to methods, be it static
+	 *                               `Class::method()` or via an object `$obj->method()`, and
+	 *                               namespaced function calls, like `MyNS\function_name()` will
+	 *                               also be accepted.
+	 *                               Defaults to `true`.
+	 * @param bool  $allow_nested    Optional. Whether to allow for nested function calls within the
+	 *                               call to this function.
+	 *                               I.e. when checking whether a token is within a function call
+	 *                               to `strtolower()`, whether to accept `strtolower( trim( $var ) )`
+	 *                               or only `strtolower( $var )`.
+	 *                               Defaults to `false`.
+	 *
+	 * @return int|bool Stack pointer to the function call T_STRING token or false otherwise.
+	 */
+	protected function is_in_function_call( $stackPtr, $valid_functions, $global = true, $allow_nested = false ) {
+		if ( ! isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) {
+			return false;
+		}
+
+		$nested_parenthesis = $this->tokens[ $stackPtr ]['nested_parenthesis'];
+		if ( false === $allow_nested ) {
+			$nested_parenthesis = array_reverse( $nested_parenthesis, true );
+		}
+
+		foreach ( $nested_parenthesis as $open => $close ) {
+
+			$prev_non_empty = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $open - 1 ), null, true, null, true );
+			if ( false === $prev_non_empty || \T_STRING !== $this->tokens[ $prev_non_empty ]['code'] ) {
+				continue;
+			}
+
+			if ( isset( $valid_functions[ strtolower( $this->tokens[ $prev_non_empty ]['content'] ) ] ) === false ) {
+				if ( false === $allow_nested ) {
+					// Function call encountered, but not to one of the allowed functions.
+					return false;
+				}
+
+				continue;
+			}
+
+			if ( false === $global ) {
+				return $prev_non_empty;
+			}
+
+			/*
+			 * Now, make sure it is a global function.
+			 */
+			if ( $this->is_class_object_call( $prev_non_empty ) === true ) {
+				continue;
+			}
+
+			if ( $this->is_token_namespaced( $prev_non_empty ) === true ) {
+				continue;
+			}
+
+			return $prev_non_empty;
+		}
+
+		return false;
+	}
+
 	/**
 	 * Check if something is only being sanitized.
 	 *
@@ -1643,9 +1715,16 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) {
 			return true;
 		}
 
-		// If this isn't a call to a function, it sure isn't a sanitizing function.
-		if ( \T_STRING !== $this->tokens[ $functionPtr ]['code'] ) {
-			if ( $require_unslash ) {
+		$valid_functions               = $this->sanitizingFunctions;
+		$valid_functions              += $this->unslashingSanitizingFunctions;
+		$valid_functions['wp_unslash'] = true;
+		$valid_functions['array_map']  = true;
+
+		$functionPtr = $this->is_in_function_call( $stackPtr, $valid_functions );
+
+		// If this isn't a call to one of the valid functions, it sure isn't a sanitizing function.
+		if ( false === $functionPtr ) {
+			if ( true === $require_unslash ) {
 				$this->add_unslash_error( $stackPtr );
 			}
 
@@ -1657,15 +1736,17 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) {
 		// Check if wp_unslash() is being used.
 		if ( 'wp_unslash' === $functionName ) {
 
-			$is_unslashed    = true;
-			$function_opener = array_pop( $nested_openers );
+			$is_unslashed = true;
+
+			unset( $valid_functions['wp_unslash'] );
+			$higherFunctionPtr = $this->is_in_function_call( $functionPtr, $valid_functions );
 
-			// If there is no other function being used, this value is unsanitized.
-			if ( ! isset( $function_opener ) ) {
+			// If there is no other valid function being used, this value is unsanitized.
+			if ( false === $higherFunctionPtr ) {
 				return false;
 			}
 
-			$functionPtr  = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $function_opener - 1 ), null, true, null, true );
+			$functionPtr  = $higherFunctionPtr;
 			$functionName = $this->tokens[ $functionPtr ]['content'];
 
 		} else {

WordPress/Sniffs/WP/CronIntervalSniff.php

@@ -55,6 +55,15 @@ class CronIntervalSniff extends Sniff {
 		'YEAR_IN_SECONDS'   => 31536000,
 	);
 
+	/**
+	 * Function within which the hook should be found.
+	 *
+	 * @var array
+	 */
+	protected $valid_functions = array(
+		'add_filter' => true,
+	);
+
 	/**
 	 * Returns an array of tokens this test wants to listen for.
 	 *
@@ -82,8 +91,8 @@ public function process_token( $stackPtr ) {
 		}
 
 		// If within add_filter.
-		$functionPtr = $this->phpcsFile->findPrevious( \T_STRING, key( $token['nested_parenthesis'] ) );
-		if ( false === $functionPtr || 'add_filter' !== $this->tokens[ $functionPtr ]['content'] ) {
+		$functionPtr = $this->is_in_function_call( $stackPtr, $this->valid_functions );
+		if ( false === $functionPtr ) {
 			return;
 		}
 
@@ -92,6 +101,11 @@ public function process_token( $stackPtr ) {
 			return;
 		}
 
+		if ( $stackPtr >= $callback['start'] ) {
+			// "cron_schedules" found in the second parameter, not the first.
+			return;
+		}
+
 		// Detect callback function name.
 		$callbackArrayPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, $callback['start'], ( $callback['end'] + 1 ), true );
 

WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc

@@ -211,3 +211,8 @@ if ( ClassName::array_key_exists( 'my_field5', $_POST ) ) {
 }
 
 echo sanitize_text_field (wp_unslash ($_GET['test'])); // OK.
+
+if ( isset( $_GET['unslash_check'] ) ) {
+	$clean = sanitize_text_field( WP_Faker::wp_unslash( $_GET['unslash_check'] ) ); // Bad x1 - unslash.
+	$clean = WP_Faker\sanitize_text_field( wp_unslash( $_GET['unslash_check'] ) ); // Bad x1 - sanitize.
+}

WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php

@@ -58,6 +58,8 @@ public function getErrorList() {
 			202 => 1,
 			206 => 1,
 			210 => 1,
+			216 => 1,
+			217 => 1,
 		);
 	}
 

WordPress/Tests/WP/CronIntervalUnitTest.inc

@@ -52,7 +52,7 @@ add_filter( 'cron_schedules' ); // Ignore, no callback parameter.
 
 add_filter( 'cron_schedules', [ 'Foo', 'my_add_quicklier' ] ); // Error: 5 min.
 
-// Ignore, not our function. Currently gives false positive, will be fixed later when function call detection utility functions are added.
+// Ignore, not our function.
 My_Custom::add_filter( 'cron_schedules', [ 'Foo', 'my_add_quicklier' ] );
 
 // Deal correctly with the WP time constants.
@@ -139,3 +139,8 @@ add_filter( 'cron_schedules', function ( $schedules ) {
 	);
 	return $schedules;
 } ); // Error: 9 min.
+
+Custom::add_filter( 'cron_schedules', array( $class, $method ) ); // OK, not the WP function.
+add_filter( 'some_hook', array( $place, 'cron_schedules' ) ); // OK, not the hook we're looking for.
+add_filter( function() { return get_hook_name('cron_schedules'); }(), array( $class, $method ) ); // OK, nested in another function call.
+

WordPress/Tests/WP/CronIntervalUnitTest.php

@@ -45,7 +45,6 @@ public function getWarningList() {
 			41  => 1,
 			43  => 1,
 			53  => 1,
-			56  => 1, // False positive.
 			67  => 1,
 			76  => 1,
 			85  => 1,