Security/PHPFilterFunctions: various sniff improvements

WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php

@@ -9,11 +9,17 @@
 
 namespace WordPressVIPMinimum\Sniffs\Security;
 
+use PHP_CodeSniffer\Util\Tokens;
+use PHPCSUtils\Utils\PassedParameters;
 use WordPressCS\WordPress\AbstractFunctionParameterSniff;
 
 /**
  * This sniff ensures that proper sanitization is occurring when PHP's filter_* functions are used.
  *
+ * {@internal The $options parameter for filter_var_array() and filter_input_array() can take either an
+ * integer (filter constant) or an array with options, which could include an option setting the filter constant.
+ * At this time, this sniff does not handle an array with options being passed.}
+ *
  * @since 0.4.0
  */
 class PHPFilterFunctionsSniff extends AbstractFunctionParameterSniff {
@@ -26,15 +32,27 @@ class PHPFilterFunctionsSniff extends AbstractFunctionParameterSniff {
 	protected $group_name = 'php_filter_functions';
 
 	/**
-	 * Functions this sniff is looking for.
+	 * Functions this sniff is looking for and information on which parameter to check for in those function calls.
 	 *
-	 * @var array<string, bool> Key is the function name, value irrelevant.
+	 * @var array<string, array{param_position: int, param_name: string}> Key is the function name.
 	 */
 	protected $target_functions = [
-		'filter_var'         => true,
-		'filter_input'       => true,
-		'filter_var_array'   => true,
-		'filter_input_array' => true,
+		'filter_var'         => [
+			'param_position' => 2,
+			'param_name'     => 'filter',
+		],
+		'filter_input'       => [
+			'param_position' => 3,
+			'param_name'     => 'filter',
+		],
+		'filter_var_array'   => [
+			'param_position' => 2,
+			'param_name'     => 'options',
+		],
+		'filter_input_array' => [
+			'param_position' => 2,
+			'param_name'     => 'options',
+		],
 	];
 
 	/**
@@ -60,30 +78,52 @@ class PHPFilterFunctionsSniff extends AbstractFunctionParameterSniff {
 	 *                  normal file processing.
 	 */
 	public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
-		if ( $matched_content === 'filter_input' ) {
-			if ( count( $parameters ) === 2 ) {
-				$message = 'Missing third parameter for "%s".';
-				$data    = [ $matched_content ];
-				$this->phpcsFile->addWarning( $message, $stackPtr, 'MissingThirdParameter', $data );
-			}
+		$param_position = $this->target_functions[ $matched_content ]['param_position'];
+		$param_name     = $this->target_functions[ $matched_content ]['param_name'];
 
-			if ( isset( $parameters[3], $this->restricted_filters[ $parameters[3]['raw'] ] ) ) {
-				$message = 'Please use an appropriate filter to sanitize, as "%s" does no filtering, see: http://php.net/manual/en/filter.filters.sanitize.php.';
-				$data    = [ strtoupper( $parameters[3]['raw'] ) ];
-				$this->phpcsFile->addWarning( $message, $stackPtr, 'RestrictedFilter', $data );
-			}
-		} else {
-			if ( count( $parameters ) === 1 ) {
-				$message = 'Missing second parameter for "%s".';
-				$data    = [ $matched_content ];
-				$this->phpcsFile->addWarning( $message, $stackPtr, 'MissingSecondParameter', $data );
+		$target_param = PassedParameters::getParameterFromStack( $parameters, $param_position, $param_name );
+		if ( $target_param === false ) {
+			/*
+			 * Check for PHP 5.6+ argument unpacking.
+			 *
+			 * No need for extensive defensive coding, we already know this is syntactically a valid function call,
+			 * otherwise this method would not have been reached.
+			 */
+			$tokens       = $this->phpcsFile->getTokens();
+			$open_parens  = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
+			$has_ellipses = $this->phpcsFile->findNext( T_ELLIPSIS, ( $open_parens + 1 ), $tokens[ $open_parens ]['parenthesis_closer'] );
+
+			if ( $has_ellipses !== false ) {
+				$target_nesting_level = 1;
+				if ( isset( $tokens[ $open_parens ]['nested_parenthesis'] ) ) {
+					$target_nesting_level = ( count( $tokens[ $open_parens ]['nested_parenthesis'] ) + 1 );
+				}
+
+				if ( $target_nesting_level === count( $tokens[ $has_ellipses ]['nested_parenthesis'] ) ) {
+					// Bow out as undetermined.
+					return;
+				}
 			}
 
-			if ( isset( $parameters[2], $this->restricted_filters[ $parameters[2]['raw'] ] ) ) {
-				$message = 'Please use an appropriate filter to sanitize, as "%s" does no filtering, see http://php.net/manual/en/filter.filters.sanitize.php.';
-				$data    = [ strtoupper( $parameters[2]['raw'] ) ];
-				$this->phpcsFile->addWarning( $message, $stackPtr, 'RestrictedFilter', $data );
+			$message = 'Missing $%s parameter for "%s()".';
+			$data    = [ $param_name, $matched_content ];
+
+			// Error codes should probably be made more descriptive, but that would be a BC-break.
+			$error_code = 'MissingSecondParameter';
+			if ( $matched_content === 'filter_input' ) {
+				$error_code = 'MissingThirdParameter';
 			}
+
+			$this->phpcsFile->addWarning( $message, $stackPtr, $error_code, $data );
+			return;
+		}
+
+		if ( isset( $this->restricted_filters[ $target_param['clean'] ] ) ) {
+			$first_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $target_param['start'], ( $target_param['end'] + 1 ), true );
+
+			$message = 'Please use an appropriate filter to sanitize, as "%s" does no filtering, see: http://php.net/manual/en/filter.filters.sanitize.php.';
+			$data    = [ $target_param['clean'] ];
+			$this->phpcsFile->addWarning( $message, $first_non_empty, 'RestrictedFilter', $data );
 		}
 	}
 }

WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc

@@ -1,29 +1,100 @@
 <?php
 
-$url = 'http://www.google.ca';
-$_GET['foo'] = 'bar';
-$array = [ 'something_something', 'https://www.google.com', '6' ];
+/*
+ * Not the sniff target.
+ */
+use filter_var;
 
-// Ok.
+my\ns\filter_input($a, $b);
+$this->filter_var_array($a, $b);
+$this?->filter_input_array($a, $b);
+MyClass::filter_var($a, $b);
+echo FILTER_INPUT;
+namespace\filter_var_array($a, $b);
+
+// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute.
+#[Filter_Input('text')]
+function foo() {}
+
+// PHP 8.1 first class callable.
+// As we have no knowledge about what parameters will be passed, we shouldn't flag this.
+add_filter('my_filter', filter_var(...));
+
+
+/*
+ * These should all be okay.
+ */
 filter_var( $url, FILTER_SANITIZE_URL );
-filter_var( 'test', FILTER_SANITIZE_STRING );
-filter_var(  "test", FILTER_SANITIZE_STRING );
-filter_input( INPUT_GET, 'foo', FILTER_SANITIZE_STRING );
+\filter_var( 'test', FILTER_SANITIZE_STRING );
+FILTER_INPUT( INPUT_GET, 'foo', FILTER_SANITIZE_STRING, );
 filter_input( INPUT_GET,  "foo"   , FILTER_SANITIZE_STRING );
-filter_var_array( $array, FILTER_SANITIZE_STRING );
-filter_input_array( $array, FILTER_SANITIZE_STRING );
-filter_input_array( $array,FILTER_SANITIZE_STRING      );
-
-// Bad.
-filter_input( INPUT_GET, 'foo' ); // Missing third parameter.
-filter_input( INPUT_GET, 'foo', FILTER_DEFAULT ); // This filter ID does nothing.
-filter_input( INPUT_GET, "foo", FILTER_UNSAFE_RAW  ); // This filter ID does nothing.
-filter_var( $url ); // Missing second parameter.
-filter_var( $url, FILTER_DEFAULT ); // This filter ID does nothing.
+filter_input_array( $array, filter_default ); // Constants are case-sensitive, so this is not the FILTER_DEFAULT constant.
+
+// Ignore as undetermined.
+filter_var(  "test", get_filter() );
+\Filter_Var_Array( $array, $filterName );
+filter_input_array( $array,$obj->get_filter()     , );
+
+// Incomplete function call, should be ignored by the sniff.
+$incorrect_but_ok = filter_input();
+
+/*
+ * These should all be flagged with a warning.
+ */
+filter_input( INPUT_GET, 'foo' ); // Missing $filter parameter.
+\filter_input( INPUT_GET, 'foo', FILTER_DEFAULT ); // This filter ID does nothing.
+filter_input( INPUT_GET, "foo", FILTER_UNSAFE_RAW /* comment */ ,); // This filter ID does nothing.
+
+filter_var( $url ); // Missing $filter parameter.
+Filter_Var( $url, FILTER_DEFAULT ); // This filter ID does nothing.
 filter_var( 'https://google.com', FILTER_UNSAFE_RAW ); // This filter ID does nothing.
-filter_var_array( $array ); // Missing second parameter.
+
+filter_var_array( $array, ); // Missing $options parameter.
 filter_var_array( $array, FILTER_DEFAULT ); // This filter ID does nothing.
 filter_var_array( $array, FILTER_UNSAFE_RAW ); // This filter ID does nothing.
-filter_input_array( $array ); // Missing second parameter.
-filter_input_array( $array, FILTER_DEFAULT ); // This filter ID does nothing.
-filter_input_array( $array, FILTER_UNSAFE_RAW ); // This filter ID does nothing.
+
+filter_input_array( $array ); // Missing $options parameter.
+\FILTER_INPUT_ARRAY( $array, FILTER_DEFAULT ); // This filter ID does nothing.
+filter_input_array( $array, FILTER_UNSAFE_RAW, ); // This filter ID does nothing.
+
+// Safeguard handling of function calls using PHP 8.0+ named parameters.
+filter_input(var_name: $var_name, filter: FILTER_SANITIZE_STRING, type: FILTER_DEFAULT); // OK, invalid input value for $type, but that's not our concern.
+filter_input(var_name: $var_name, filter: $filter, type: $type); // Ignore, undetermined.
+filter_input(
+    var_name: $var_name,
+    filter: FILTER_DEFAULT, // This filter ID does nothing.
+    type: $type,
+);
+
+filter_var(filter: FILTER_SANITIZE_URL); // OK, well not really, missing required parameter, but that's not our concern.
+filter_var($value, options: FILTER_NULL_ON_FAILURE); // Missing $filter parameter.
+filter_var(value: $value, filters: FILTER_SANITIZE_URL); // Typo in parameter name, report as missing $filter parameter.
+
+filter_var_array(options: FILTER_UNSAFE_RAW, array: $array); // This filter ID does nothing.
+
+filter_input_array($type, add_empty: false, options: FILTER_DEFAULT,); // This filter ID does nothing.
+
+// Ignore function calls using PHP 5.6 argument unpacking as we don't know what parameters were passed.
+filter_input(INPUT_GET, ...$params);
+trim(filter_input(INPUT_GET, ...$params));
+// ... but only ignore unpacking if done at the correct nesting level.
+filter_input(INPUT_GET, $obj->getVarname(...$params)); // Missing $filter parameter.
+
+// False negatives: $options arrays are currently not (yet) supported by this sniff.
+// See: https://www.php.net/manual/en/function.filter-var-array.php and https://www.php.net/manual/en/function.filter-input-array.php
+filter_var_array(
+	$array,
+	array('keyA' => FILTER_DEFAULT), // This filter ID does nothing.
+);
+filter_input_array(
+	$array,
+	[
+		'keyA' => [
+			'filter' => FILTER_UNSAFE_RAW, // This filter ID does nothing.
+			'flags'  => FILTER_FORCE_ARRAY,
+		],
+		'keyB' => [
+			'filter' => FILTER_SANITIZE_ENCODED,
+		],
+	]
+);

WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php

@@ -32,18 +32,24 @@ public function getErrorList() {
 	 */
 	public function getWarningList() {
 		return [
-			18 => 1,
-			19 => 1,
-			20 => 1,
-			21 => 1,
-			22 => 1,
-			23 => 1,
-			24 => 1,
-			25 => 1,
-			26 => 1,
-			27 => 1,
-			28 => 1,
-			29 => 1,
+			44 => 1,
+			45 => 1,
+			46 => 1,
+			48 => 1,
+			49 => 1,
+			50 => 1,
+			52 => 1,
+			53 => 1,
+			54 => 1,
+			56 => 1,
+			57 => 1,
+			58 => 1,
+			65 => 1,
+			70 => 1,
+			71 => 1,
+			73 => 1,
+			75 => 1,
+			81 => 1,
 		];
 	}
 }