Merge pull request #1676 from WordPress-Coding-Standards/feature/validatedsanitizedinput-multi-level-array-key-check

ValidatedSanitizedInput/Sniff::is_validated(): allow for multi-level array key check

Author: GaryJones

WordPress/Sniff.php

@@ -1814,38 +1814,80 @@ public function add_unslash_error( $stackPtr ) {
 		);
 	}
 
+	/**
+	 * Get the index keys of an array variable.
+	 *
+	 * E.g., "bar" and "baz" in $foo['bar']['baz'].
+	 *
+	 * @since 2.1.0
+	 *
+	 * @param int  $stackPtr The index of the variable token in the stack.
+	 * @param bool $all      Whether to get all keys or only the first.
+	 *                       Defaults to `true`(= all).
+	 *
+	 * @return array An array of index keys whose value is being accessed.
+	 *               or an empty array if this is not array access.
+	 */
+	protected function get_array_access_keys( $stackPtr, $all = true ) {
+
+		$keys = array();
+
+		if ( \T_VARIABLE !== $this->tokens[ $stackPtr ]['code'] ) {
+			return $keys;
+		}
+
+		$current = $stackPtr;
+
+		do {
+			// Find the next non-empty token.
+			$open_bracket = $this->phpcsFile->findNext(
+				Tokens::$emptyTokens,
+				( $current + 1 ),
+				null,
+				true
+			);
+
+			// If it isn't a bracket, this isn't an array-access.
+			if ( false === $open_bracket
+				|| \T_OPEN_SQUARE_BRACKET !== $this->tokens[ $open_bracket ]['code']
+				|| ! isset( $this->tokens[ $open_bracket ]['bracket_closer'] )
+			) {
+				break;
+			}
+
+			$key = $this->phpcsFile->getTokensAsString(
+				( $open_bracket + 1 ),
+				( $this->tokens[ $open_bracket ]['bracket_closer'] - $open_bracket - 1 )
+			);
+
+			$keys[]  = trim( $key );
+			$current = $this->tokens[ $open_bracket ]['bracket_closer'];
+		} while ( isset( $this->tokens[ $current ] ) && true === $all );
+
+		return $keys;
+	}
+
 	/**
 	 * Get the index key of an array variable.
 	 *
 	 * E.g., "bar" in $foo['bar'].
 	 *
 	 * @since 0.5.0
+	 * @since 2.1.0 Now uses get_array_access_keys() under the hood.
 	 *
 	 * @param int $stackPtr The index of the token in the stack.
 	 *
 	 * @return string|false The array index key whose value is being accessed.
 	 */
 	protected function get_array_access_key( $stackPtr ) {
 
-		// Find the next non-empty token.
-		$open_bracket = $this->phpcsFile->findNext(
-			Tokens::$emptyTokens,
-			( $stackPtr + 1 ),
-			null,
-			true
-		);
+		$keys = $this->get_array_access_keys( $stackPtr, false );
 
-		// If it isn't a bracket, this isn't an array-access.
-		if ( false === $open_bracket || \T_OPEN_SQUARE_BRACKET !== $this->tokens[ $open_bracket ]['code'] ) {
-			return false;
+		if ( isset( $keys[0] ) ) {
+			return $keys[0];
 		}
 
-		$key = $this->phpcsFile->getTokensAsString(
-			( $open_bracket + 1 ),
-			( $this->tokens[ $open_bracket ]['bracket_closer'] - $open_bracket - 1 )
-		);
-
-		return trim( $key );
+		return false;
 	}
 
 	/**
@@ -1874,17 +1916,20 @@ protected function get_array_access_key( $stackPtr ) {
 	 *
 	 * @since 0.5.0
 	 * @since 2.1.0 Now recognizes array_key_exists() and key_exists() as validation functions.
+	 * @since 2.1.0 Stricter check on whether the correct variable and the correct
+	 *              array keys are being validated.
 	 *
-	 * @param int    $stackPtr          The index of this token in the stack.
-	 * @param string $array_key         An array key to check for ("bar" in $foo['bar']).
-	 * @param bool   $in_condition_only Whether to require that this use of the
-	 *                                  variable occur within the scope of the
-	 *                                  validating condition, or just in the same
-	 *                                  scope as it (default).
+	 * @param int          $stackPtr          The index of this token in the stack.
+	 * @param array|string $array_keys        An array key to check for ("bar" in $foo['bar'])
+	 *                                        or an array of keys for multi-level array access.
+	 * @param bool         $in_condition_only Whether to require that this use of the
+	 *                                        variable occur within the scope of the
+	 *                                        validating condition, or just in the same
+	 *                                        scope as it (default).
 	 *
 	 * @return bool Whether the var is validated.
 	 */
-	protected function is_validated( $stackPtr, $array_key = null, $in_condition_only = false ) {
+	protected function is_validated( $stackPtr, $array_keys = array(), $in_condition_only = false ) {
 
 		if ( $in_condition_only ) {
 			/*
@@ -1935,11 +1980,14 @@ protected function is_validated( $stackPtr, $array_key = null, $in_condition_onl
 			}
 
 			$scope_end = $stackPtr;
+		}
 
+		if ( ! empty( $array_keys ) && ! is_array( $array_keys ) ) {
+			$array_keys = (array) $array_keys;
 		}
 
-		$bare_array_key = $this->strip_quotes( $array_key );
-		$targets        = array(
+		$bare_array_keys = array_map( array( $this, 'strip_quotes' ), $array_keys );
+		$targets         = array(
 			\T_ISSET  => 'construct',
 			\T_EMPTY  => 'construct',
 			\T_UNSET  => 'construct',
@@ -1974,11 +2022,15 @@ protected function is_validated( $stackPtr, $array_key = null, $in_condition_onl
 							continue;
 						}
 
-						// If we're checking for a specific array key (ex: 'hello' in
+						// If we're checking for specific array keys (ex: 'hello' in
 						// $_POST['hello']), that must match too. Quote-style, however, doesn't matter.
-						if ( isset( $array_key )
-							&& $this->strip_quotes( $this->get_array_access_key( $i ) ) !== $bare_array_key ) {
-							continue;
+						if ( ! empty( $bare_array_keys ) ) {
+							$found_keys = $this->get_array_access_keys( $i );
+							$found_keys = array_map( array( $this, 'strip_quotes' ), $found_keys );
+							$diff       = array_diff_assoc( $bare_array_keys, $found_keys );
+							if ( ! empty( $diff ) ) {
+								continue;
+							}
 						}
 
 						return true;
@@ -2011,12 +2063,45 @@ protected function is_validated( $stackPtr, $array_key = null, $in_condition_onl
 					}
 
 					$params = $this->get_function_call_parameters( $i );
-					if ( $params[2]['raw'] !== $this->tokens[ $stackPtr ]['content'] ) {
+					if ( count( $params ) < 2 ) {
+						continue 2;
+					}
+
+					$param2_first_token = $this->phpcsFile->findNext( Tokens::$emptyTokens, $params[2]['start'], ( $params[2]['end'] + 1 ), true );
+					if ( false === $param2_first_token
+						|| \T_VARIABLE !== $this->tokens[ $param2_first_token ]['code']
+						|| $this->tokens[ $param2_first_token ]['content'] !== $this->tokens[ $stackPtr ]['content']
+					) {
 						continue 2;
 					}
 
-					if ( isset( $array_key )
-						&& $this->strip_quotes( $params[1]['raw'] ) !== $bare_array_key ) {
+					if ( ! empty( $bare_array_keys ) ) {
+						// Prevent the original array from being altered.
+						$bare_keys = $bare_array_keys;
+						$last_key  = array_pop( $bare_keys );
+
+						/*
+						 * For multi-level array access, the complete set of keys could be split between
+						 * the first and the second parameter, but could also be completely in the second
+						 * parameter, so we need to check both options.
+						 */
+
+						$found_keys = $this->get_array_access_keys( $param2_first_token );
+						$found_keys = array_map( array( $this, 'strip_quotes' ), $found_keys );
+
+						// First try matching the complete set against the second parameter.
+						$diff = array_diff_assoc( $bare_array_keys, $found_keys );
+						if ( empty( $diff ) ) {
+							return true;
+						}
+
+						// If that failed, try getting an exact match for the subset against the
+						// second parameter and the last key against the first.
+						if ( $bare_keys === $found_keys && $this->strip_quotes( $params[1]['raw'] ) === $last_key ) {
+							return true;
+						}
+
+						// Didn't find the correct array keys.
 						continue 2;
 					}
 

WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php

@@ -123,16 +123,16 @@ function ( $symbol ) {
 			return;
 		}
 
-		$array_key = $this->get_array_access_key( $stackPtr );
+		$array_keys = $this->get_array_access_keys( $stackPtr );
 
-		if ( empty( $array_key ) ) {
+		if ( empty( $array_keys ) ) {
 			return;
 		}
 
 		$error_data = array( $this->tokens[ $stackPtr ]['content'] );
 
 		// Check for validation first.
-		if ( ! $this->is_validated( $stackPtr, $array_key, $this->check_validation_in_scope_only ) ) {
+		if ( ! $this->is_validated( $stackPtr, $array_keys, $this->check_validation_in_scope_only ) ) {
 			$this->phpcsFile->addError(
 				'Detected usage of a non-validated input variable: %s',
 				$stackPtr,

WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc

@@ -230,3 +230,30 @@ function test_allow_array_key_exists_alias() {
 	if ( key_exists( 'my_field1', $_POST ) ) {
 	$id = (int) $_POST['my_field1']; // OK.
 }
+
+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 ( 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.
+	}
+}
+
+function test_correct_multi_level_array_validation_key_order() {
+	if ( isset( $_POST['toplevel']['sublevel'] ) ) {
+		$id = (int) $_POST['sublevel']['toplevel']; // Bad x 1 - validate - key order is wrong.
+	}
+}
+
+function test_invalid_array_key_exists_call() {
+	if ( array_key_exists( 'bar' ) ) {
+		$id = (int) $_POST['bar']; // Bad x 1 - validate.
+	}
+}

WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php

@@ -60,6 +60,11 @@ public function getErrorList() {
 			210 => 1,
 			216 => 1,
 			217 => 1,
+			238 => 1,
+			242 => 1,
+			245 => 1,
+			251 => 1,
+			257 => 1,
 		);
 	}