ValidatedSanitizedInput: handle null coalesce (equal) correctly

WordPress/Sniff.php

@@ -2097,10 +2097,12 @@ protected function is_validated( $stackPtr, $array_keys = array(), $in_condition
 
 		$bare_array_keys = array_map( array( $this, 'strip_quotes' ), $array_keys );
 		$targets         = array(
-			\T_ISSET  => 'construct',
-			\T_EMPTY  => 'construct',
-			\T_UNSET  => 'construct',
-			\T_STRING => 'function_call',
+			\T_ISSET          => 'construct',
+			\T_EMPTY          => 'construct',
+			\T_UNSET          => 'construct',
+			\T_STRING         => 'function_call',
+			\T_COALESCE       => 'coalesce',
+			\T_COALESCE_EQUAL => 'coalesce',
 		);
 
 		// phpcs:ignore Generic.CodeAnalysis.JumbledIncrementer.Found -- On purpose, see below.
@@ -2215,6 +2217,40 @@ protected function is_validated( $stackPtr, $array_keys = array(), $in_condition
 					}
 
 					return true;
+
+				case 'coalesce':
+					$prev = $i;
+					do {
+						$prev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $prev - 1 ), null, true, null, true );
+						// Skip over array keys, like $_GET['key']['subkey'].
+						if ( \T_CLOSE_SQUARE_BRACKET === $this->tokens[ $prev ]['code'] ) {
+							$prev = $this->tokens[ $prev ]['bracket_opener'];
+							continue;
+						}
+
+						break;
+					} while ( $prev >= ( $scope_start + 1 ) );
+
+					// We should now have reached the variable.
+					if ( \T_VARIABLE !== $this->tokens[ $prev ]['code'] ) {
+						continue 2;
+					}
+
+					if ( $this->tokens[ $prev ]['content'] !== $this->tokens[ $stackPtr ]['content'] ) {
+						continue 2;
+					}
+
+					if ( ! empty( $bare_array_keys ) ) {
+						$found_keys = $this->get_array_access_keys( $prev );
+						$found_keys = array_map( array( $this, 'strip_quotes' ), $found_keys );
+						$diff       = array_diff_assoc( $bare_array_keys, $found_keys );
+						if ( ! empty( $diff ) ) {
+							continue 2;
+						}
+					}
+
+					// Right variable, correct key.
+					return true;
 			}
 		}
 
@@ -2229,12 +2265,24 @@ protected function is_validated( $stackPtr, $array_keys = array(), $in_condition
 	 * Also recognizes `switch ( $var )`.
 	 *
 	 * @since 0.5.0
+	 * @since 2.1.0 Added the $include_coalesce parameter.
 	 *
-	 * @param int $stackPtr The index of this token in the stack.
+	 * @param int  $stackPtr         The index of this token in the stack.
+	 * @param bool $include_coalesce Optional. Whether or not to regard the null
+	 *                               coalesce operator - ?? - as a comparison operator.
+	 *                               Defaults to true.
+	 *                               Null coalesce is a special comparison operator in this
+	 *                               sense as it doesn't compare a variable to whatever is
+	 *                               on the other side of the comparison operator.
 	 *
 	 * @return bool Whether this is a comparison.
 	 */
-	protected function is_comparison( $stackPtr ) {
+	protected function is_comparison( $stackPtr, $include_coalesce = true ) {
+
+		$comparisonTokens = Tokens::$comparisonTokens;
+		if ( false === $include_coalesce ) {
+			unset( $comparisonTokens[ \T_COALESCE ] );
+		}
 
 		// We first check if this is a switch statement (switch ( $var )).
 		if ( isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) {
@@ -2258,7 +2306,7 @@ protected function is_comparison( $stackPtr ) {
 			true
 		);
 
-		if ( isset( Tokens::$comparisonTokens[ $this->tokens[ $previous_token ]['code'] ] ) ) {
+		if ( isset( $comparisonTokens[ $this->tokens[ $previous_token ]['code'] ] ) ) {
 			return true;
 		}
 
@@ -2281,7 +2329,7 @@ protected function is_comparison( $stackPtr ) {
 			);
 		}
 
-		if ( false !== $next_token && isset( Tokens::$comparisonTokens[ $this->tokens[ $next_token ]['code'] ] ) ) {
+		if ( false !== $next_token && isset( $comparisonTokens[ $this->tokens[ $next_token ]['code'] ] ) ) {
 			return true;
 		}
 

WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php

@@ -10,6 +10,7 @@
 namespace WordPressCS\WordPress\Sniffs\Security;
 
 use WordPressCS\WordPress\Sniff;
+use PHP_CodeSniffer\Util\Tokens;
 
 /**
  * Flag any non-validated/sanitized input ( _GET / _POST / etc. ).
@@ -131,8 +132,37 @@ function ( $symbol ) {
 
 		$error_data = array( $this->tokens[ $stackPtr ]['content'] . '[' . implode( '][', $array_keys ) . ']' );
 
-		// Check for validation first.
-		if ( ! $this->is_validated( $stackPtr, $array_keys, $this->check_validation_in_scope_only ) ) {
+		/*
+		 * Check for validation first.
+		 */
+		$validated = false;
+
+		for ( $i = ( $stackPtr + 1 ); $i < $this->phpcsFile->numTokens; $i++ ) {
+			if ( isset( Tokens::$emptyTokens[ $this->tokens[ $i ]['code'] ] ) ) {
+				continue;
+			}
+
+			if ( \T_OPEN_SQUARE_BRACKET === $this->tokens[ $i ]['code']
+				&& isset( $this->tokens[ $i ]['bracket_closer'] )
+			) {
+				// Skip over array keys.
+				$i = $this->tokens[ $i ]['bracket_closer'];
+				continue;
+			}
+
+			if ( \T_COALESCE === $this->tokens[ $i ]['code'] ) {
+				$validated = true;
+			}
+
+			// Anything else means this is not a validation coalesce.
+			break;
+		}
+
+		if ( false === $validated ) {
+			$validated = $this->is_validated( $stackPtr, $array_keys, $this->check_validation_in_scope_only );
+		}
+
+		if ( false === $validated ) {
 			$this->phpcsFile->addError(
 				'Detected usage of a possibly undefined superglobal array index: %s. Use isset() or empty() to check the index exists before using it',
 				$stackPtr,
@@ -151,7 +181,7 @@ function ( $symbol ) {
 		}
 
 		// If this is a comparison ('a' == $_POST['foo']), sanitization isn't needed.
-		if ( $this->is_comparison( $stackPtr ) ) {
+		if ( $this->is_comparison( $stackPtr, false ) ) {
 			return;
 		}
 

WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc

@@ -161,7 +161,7 @@ some string {$_POST[some_var]} {$_GET['evil']}
 EOD
 ); // Bad x2.
 
-if ( ( $_POST['foo'] ?? 'post' ) === 'post' ) {} // OK.
+if ( ( $_POST['foo'] ?? 'post' ) === 'post' ) {} // Bad x2 - unslash, sanitize - more complex compares are not handled.
 if ( ( $_POST['foo'] <=> 'post' ) === 0 ) {} // OK.
 
 // Test whitespace independent isset/empty detection.
@@ -289,3 +289,36 @@ function test_recognize_array_comparison_functions_as_such() {
 	if ( array_keys( $_POST['form_fields'], 'my_form_hidden_field_value', true ) ) {} // OK.
 	if ( array_keys( $_POST['form_fields'] ) ) {} // Bad x2.
 }
+
+/*
+ * Test recognition of validation via null coalesce, while still checking the var for sanitization.
+ */
+function test_null_coalesce_1() {
+	$var = sanitize_text_field( wp_unslash( $_POST['foo'] ?? '' ) ); // OK.
+	$var = sanitize_text_field( wp_unslash( $_POST['fool'] ?? $_POST['secondary'] ?? '' ) ); // OK.
+	$var = sanitize_text_field( wp_unslash( $_POST['bar']['sub'] ?? '' ) ); // OK.
+	$var = sanitize_text_field( $_POST['foobar'] ?? '' ); // Bad x1 - unslash.
+}
+
+// The below two sets should give the same errors.
+function test_null_coalesce_2() {
+	$var = $_POST['foo'] ?? ''; // Bad x2 - sanitize + unslash.
+	$var = $_POST['bar']['sub'] ?? ''; // Bad x2 - sanitize + unslash.
+	$var = ( $_POST['foobar']['sub'] ?? '' ); // Bad x2 - sanitize + unslash.
+
+	$var = isset( $_POST['_foo'] ) ? $_POST['_foo'] : ''; // Bad x2 - sanitize + unslash.
+	$var = isset( $_POST['_bar']['_sub'] ) ? $_POST['_bar']['_sub'] : ''; // Bad x2 - sanitize + unslash.
+	$var = ( isset( $_POST['_foobar']['_sub'] ) ? $_POST['_foobar']['_sub'] : '' ); // Bad x2 - sanitize + unslash.
+}
+
+function test_null_coalesce_validation() {
+	$_POST['key'] = $_POST['key'] ?? 'default'; // OK, assignment & Bad x2 - unslash, sanitize.
+	$key            = sanitize_text_field( wp_unslash( $_POST['key'] ) ); // OK, validated via null coalesce.
+	$another_key    = sanitize_text_field( wp_unslash( $_POST['another_key'] ) ); // Bad, not validated, different key.
+}
+
+function test_null_coalesce_equals_validation() {
+	$_POST['key'] ??= 'default'; // OK, assignment.
+	$key            = sanitize_text_field( wp_unslash( $_POST['key'] ) ); // OK, validated via null coalesce equals.
+	$another_key    = sanitize_text_field( wp_unslash( $_POST['another_key'] ) ); // Bad, not validated, different key.
+}

WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php

@@ -54,6 +54,7 @@ public function getErrorList() {
 			138 => 1,
 			150 => 2,
 			160 => 2,
+			164 => 2,
 			189 => 1,
 			202 => 1,
 			206 => 1,
@@ -68,6 +69,16 @@ public function getErrorList() {
 			266 => 1,
 			277 => 1,
 			290 => 2,
+			300 => 1,
+			305 => 2,
+			306 => 2,
+			307 => 2,
+			309 => 2,
+			310 => 2,
+			311 => 2,
+			315 => 2,
+			317 => 1,
+			323 => 1,
 		);
 	}