Re-factor v2 with feedback from @jrfnl

Author: rebeccahum

WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php

@@ -35,12 +35,28 @@ class ProperEscapingFunctionSniff extends Sniff {
 		'esc_html_e' => 'html',
 	];
 
+	/**
+	 * List of tokens we can skip.
+	 *
+	 * @var array
+	 */
+	private $echo_or_concat_tokens =
+	[
+		T_ECHO               => T_ECHO,
+		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,
+	];
+
 	/**
 	 * Returns an array of tokens this test wants to listen for.
 	 *
 	 * @return array
 	 */
 	public function register() {
+		$this->echo_or_concat_tokens += Tokens::$emptyTokens;
+
 		return [ T_STRING ];
 	}
 
@@ -59,36 +75,17 @@ public function process_token( $stackPtr ) {
 
 		$function_name = $this->tokens[ $stackPtr ]['content'];
 
-		$data = [ $function_name ];
-
-		$echo_or_concat_or_html = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $stackPtr - 1, null, true );
+		$html = $this->phpcsFile->findPrevious( $this->echo_or_concat_tokens, $stackPtr - 1, null, true );
 
-		if ( $this->tokens[ $echo_or_concat_or_html ]['code'] === T_ECHO ) {
-			// Very likely inline HTML with <?php tag.
-			$php_open = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $echo_or_concat_or_html - 1, null, true );
-
-			if ( $this->tokens[ $php_open ]['code'] !== T_OPEN_TAG ) {
-				return;
-			}
+		$data = [ $function_name ];
 
-			$html = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $php_open - 1, null, true );
+		if ( isset( Tokens::$textStringTokens[ $this->tokens[ $html ]['code'] ] ) === false ) { // Use $textStringTokens b/c heredoc and nowdoc tokens shouldn't be matched anyways.
+			return;
+		}
 
-			if ( $this->tokens[ $html ]['code'] === T_INLINE_HTML && $this->is_outside_html_attr_context( $function_name, $this->tokens[ $html ]['content'] ) ) {
-				$message = 'Wrong escaping function, please do not use `%s()` in a context outside of HTML attributes.';
-				$this->phpcsFile->addError( $message, $html, 'notAttrEscAttr', $data );
-				return;
-			}
-		} elseif ( $this->tokens[ $echo_or_concat_or_html ]['code'] === T_STRING_CONCAT || $this->tokens[ $echo_or_concat_or_html ]['code'] === T_COMMA ) {
-			// Very likely string concatenation mixing strings and functions/variables.
-			$html = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $echo_or_concat_or_html - 1, null, true );
-
-			if ( $this->tokens[ $html ]['code'] === T_CONSTANT_ENCAPSED_STRING && $this->is_outside_html_attr_context( $function_name, trim( $this->tokens[ $html ]['content'], '"\'' ) ) ) {
-				$message = 'Wrong escaping function, please do not use `%s()` in a context outside of HTML attributes.';
-				$this->phpcsFile->addError( $message, $html, 'notAttrEscAttr', $data );
-				return;
-			}
-		} else {
-			// Neither - bailing.
+		if ( $this->is_outside_html_attr_context( $function_name, Sniff::strip_quotes( $this->tokens[ $html ]['content'] ) ) ) {
+			$message = 'Wrong escaping function, please do not use `%s()` in a context outside of HTML attributes.';
+			$this->phpcsFile->addError( $message, $html, 'notAttrEscAttr', $data );
 			return;
 		}
 
@@ -97,6 +94,7 @@ public function process_token( $stackPtr ) {
 			$this->phpcsFile->addError( $message, $stackPtr, 'hrefSrcEscUrl', $data );
 			return;
 		}
+
 		if ( $function_name === 'esc_html' && $this->is_html_attr( $this->tokens[ $html ]['content'] ) ) {
 			$message = 'Wrong escaping function. HTML attributes should be escaped by `esc_attr()`, not by `%s()`.';
 			$this->phpcsFile->addError( $message, $stackPtr, 'htmlAttrNotByEscHTML', $data );
@@ -167,12 +165,12 @@ public function endswith( $haystack, $needle ) {
 	/**
 	 * Tests whether provided string ends with closing HTML tag in an attribute context.
 	 *
-	 * @param string $function_name Escaping attribute function name.
-	 * @param string $content Haystack in which we look for open HTML tag.
+	 * @param string $function_name Name of function found.
+	 * @param string $content       Haystack in which we look for open HTML tag.
 	 *
 	 * @return bool True if string ends with open HTML tag.
 	 */
 	public function is_outside_html_attr_context( $function_name, $content ) {
-		return $this->escaping_functions[ $function_name ] === 'attr' && substr( $content, -1 ) === '>';
+		return $this->escaping_functions[ $function_name ] === 'attr' && substr( trim( $content ), -1 ) === '>';
 	}
 }

WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc

@@ -62,4 +62,9 @@ Test
 <h1><?php echo esc_attr__( $title, 'domain' ); ?></h1> <!-- Error --> ?>
 <?php echo '<h1>' . esc_attr__( $some_var, 'domain' ) . '</h1>'; // Error.
 echo '<h1>',      esc_attr__( $title, 'domain' ), '</h1>'; // Error.
-echo '<h1>',      esc_attr__( $title, 'domain' )      . '</h1>'; // Error.
+echo '<h1>',      esc_attr__( $title, 'domain' )      . '</h1>'; // Error.
+?>
+<h1> <?php echo esc_attr( $title ) . '</h1>'; ?> // Error.
+<div><?= esc_attr($attr) ?></div><!-- Error -->
+<div><?php esc_attr_e($attr) ?></div><!-- Error -->
+<div data-tag="<?= esc_attr( $attr ); ?>"> <!-- OK -->

WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.php

@@ -39,6 +39,9 @@ public function getErrorList() {
 			63 => 1,
 			64 => 1,
 			65 => 1,
+			67 => 1,
+			68 => 1,
+			69 => 1,
 		];
 	}