Security/EscapeOutput: bug fix - sniff did not handle arrays correctly

In particular when examining the parameters passed to a function call, the sniff may encounter parameters which are arrays.

As things were, the sniff would skip over the array opener and then continue like before.

This is buggy, in the same way as examining all function parameters as one long code block was buggy and would lead to errors throw on the incorrect token and/or false positives when the array item contains a ternary.

To fix this, I'm special casing arrays, parsing the array items and then recursively calling the `check_code_is_escaped()` method to examine each array item individually. After this, the original call to `check_code_is_escaped()` will continue from the array closer onwards.

This gives much higher accuracy when examining array items and fixes the bug.

Includes tests proving the bug(s) and safeguarding the fix.

Author: jrfnl

WordPress/Sniffs/Security/EscapeOutputSniff.php

@@ -12,6 +12,7 @@
 use PHP_CodeSniffer\Util\Tokens;
 use PHPCSUtils\BackCompat\BCFile;
 use PHPCSUtils\Tokens\Collections;
+use PHPCSUtils\Utils\Arrays;
 use PHPCSUtils\Utils\Conditions;
 use PHPCSUtils\Utils\Operators;
 use PHPCSUtils\Utils\PassedParameters;
@@ -536,13 +537,22 @@ protected function check_code_is_escaped( $start, $end ) {
 				continue;
 			}
 
-			// Handle arrays for those functions that accept them.
-			if ( \T_ARRAY === $this->tokens[ $i ]['code'] ) {
-				++$i; // Skip the opening parenthesis.
-				continue;
-			}
+			// Examine the items in an array individually for array parameters.
+			if ( isset( Collections::arrayOpenTokensBC()[ $this->tokens[ $i ]['code'] ] ) ) {
+				$array_open_close = Arrays::getOpenClose( $this->phpcsFile, $i );
+				if ( false === $array_open_close ) {
+					// Short list or misidentified short array token.
+					continue;
+				}
+
+				$array_items = PassedParameters::getParameters( $this->phpcsFile, $i, 0, true );
+				if ( ! empty( $array_items ) ) {
+					foreach ( $array_items as $array_item ) {
+						$this->check_code_is_escaped( $array_item['start'], ( $array_item['end'] + 1 ) );
+					}
+				}
 
-			if ( isset( Collections::shortArrayTokens()[ $this->tokens[ $i ]['code'] ] ) ) {
+				$i = $array_open_close['closer'];
 				continue;
 			}
 

WordPress/Tests/Security/EscapeOutputUnitTest.1.inc

@@ -605,3 +605,18 @@ _e( domain: $domain ); // OK, well not really, required $text parameter missing,
 _ex( domain: $domain, text: 'plain text', context: $context ); // OK.
 _e( domain: $domain, text: $text, ); // Bad x 1, only text param.
 // phpcs:enable
+
+// Array keys and values should be examined individually.
+wp_die( esc_html( $message ), '', array() ); // OK.
+wp_die(
+	esc_html( $message ), // OK.
+	'',
+	array(
+		'back_link' => true, // OK.
+		'response'  => $response ?? 200, // Bad.
+		'link_url'  => ( '' !== $this->link_url ) ? $this->link_url : '', // Bad.
+		'link_text' => ( '' !== $link_title ) ? esc_attr( $link_title ) : '', // OK.
+		'charset'   => [$set, $rtl] = $array, // Bad x 2 (silly code, but the list shouldn't be treated as an array).
+		'exit'      => $do_exit, // Bad x 1.
+	)
+);

WordPress/Tests/Security/EscapeOutputUnitTest.php

@@ -149,6 +149,10 @@ public function getErrorList( $testFile = '' ) {
 					595 => 1,
 					603 => 1,
 					606 => 1,
+					616 => 1,
+					617 => 1,
+					619 => 2,
+					620 => 1,
 				);
 
 			case 'EscapeOutputUnitTest.6.inc':