From cc1022e0b4f1d0872db5e56c12d4e6f02d0ee952 Mon Sep 17 00:00:00 2001
From: jrfnl <jrfnl@users.noreply.github.com>
Date: Tue, 20 Apr 2021 15:21:46 +0200
Subject: [PATCH] ProperEscapingFunction: fix overreach

As part of the changes made in 624, the `T_COMMA` token was added to the list of tokens to skip over to allow for `echo` statements with multiple arguments passed as a coma-delimited list.

As a side-effect, this caused the sniff to also examine `[s]printf()`-like function calls where the first parameter is a text string, while the second is often a variable within a call to one of the escaping functions.

The current change fixes this by only adding the `T_COMMA` token to the "ignore when looking for the previous token"-list when in an `echo` statement.

Includes unit test.

Fixes 667

Additional notes:
* I've run the sniff over WP Core to verify the fix and have verified that all 23 violations being throw up are correctly detected violations.
* If it would be considered a good idea to also examine, `[s]printf()`-like function calls for this sniff for proper escaping, I suggest opening a separate, new feature request as that change would need significantly different and quite complex logic and does not fall within the scope of this bug fix.
---
 .../Sniffs/Security/ProperEscapingFunctionSniff.php      | 9 +++++++--
 .../Tests/Security/ProperEscapingFunctionUnitTest.inc    | 3 +++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php b/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php
index 9db2983e..7c2db497 100644
--- a/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php
+++ b/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php
@@ -46,7 +46,6 @@ class ProperEscapingFunctionSniff extends Sniff {
 		T_OPEN_TAG           => T_OPEN_TAG,
 		T_OPEN_TAG_WITH_ECHO => T_OPEN_TAG_WITH_ECHO,
 		T_STRING_CONCAT      => T_STRING_CONCAT,
-		T_COMMA              => T_COMMA,
 		T_NS_SEPARATOR       => T_NS_SEPARATOR,
 	];
 
@@ -107,7 +106,13 @@ public function process_token( $stackPtr ) {
 			return;
 		}
 
-		$html = $this->phpcsFile->findPrevious( $this->echo_or_concat_tokens, $stackPtr - 1, null, true );
+		$ignore             = $this->echo_or_concat_tokens;
+		$start_of_statement = $this->phpcsFile->findStartOfStatement( $stackPtr, T_COMMA );
+		if ( $this->tokens[ $start_of_statement ]['code'] === T_ECHO ) {
+			$ignore[ T_COMMA ] = T_COMMA;
+		}
+
+		$html = $this->phpcsFile->findPrevious( $ignore, $stackPtr - 1, null, true );
 
 		// Use $textStringTokens b/c heredoc and nowdoc tokens will never be encountered in this context anyways..
 		if ( $html === false || isset( Tokens::$textStringTokens[ $this->tokens[ $html ]['code'] ] ) === false ) {
diff --git a/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc b/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc
index a0ff39cc..bd5523d7 100644
--- a/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc
+++ b/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc
@@ -82,3 +82,6 @@ echo '<a href="',  esc_html($url), '">'; // Error.
 echo '<a href=', esc_html($url), '>'; // Error.
 
 echo 'data-param-url="' . Esc_HTML::static_method( $share_url ) . '"'; // OK.
+
+// Not a target for this sniff (yet).
+printf( '<meta name="generator" content="%s">', esc_attr( $content ) ); // OK.