Merge pull request #675 from Automattic/fix/671-properescaping-open-with-echo

ProperEscapingFunction: allow for comma's in first short open echo tag expression

Author: rebeccahum

WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php

@@ -89,6 +89,14 @@ class ProperEscapingFunctionSniff extends Sniff {
 		'=\\"',
 	];
 
+	/**
+	 * Keep track of whether or not we're currently in the first statement of a short open echo tag.
+	 *
+	 * @var int|false Integer stack pointer to the end of the first statement in the current
+	 *                short open echo tag or false when not in a short open echo tag.
+	 */
+	private $in_short_echo = false;
+
 	/**
 	 * Returns an array of tokens this test wants to listen for.
 	 *
@@ -97,7 +105,10 @@ class ProperEscapingFunctionSniff extends Sniff {
 	public function register() {
 		$this->echo_or_concat_tokens += Tokens::$emptyTokens;
 
-		return [ T_STRING ];
+		return [
+			T_STRING,
+			T_OPEN_TAG_WITH_ECHO,
+		];
 	}
 
 	/**
@@ -108,6 +119,35 @@ public function register() {
 	 * @return void
 	 */
 	public function process_token( $stackPtr ) {
+		/*
+		 * Short open echo tags will act as an echo for the first expression and
+		 * allow for passing multiple comma-separated parameters.
+		 * However, short open echo tags also allow for additional statements after, but
+		 * those have to be full PHP statements, not expressions.
+		 *
+		 * This snippet of code will keep track of whether or not we're in the first
+		 * expression in a short open echo tag.
+		 * $phpcsFile->findStartOfStatement() unfortunately is useless, as it will return
+		 * the first token in the statement, which can be anything - variable, text string -
+		 * without any indication of whether this is the start of a normal statement or
+		 * a short open echo expression.
+		 * So, if we used that, we'd need to walk back from every start of statement to
+		 * the previous non-empty to see if it is the short open echo tag.
+		 */
+		if ( $this->tokens[ $stackPtr ]['code'] === T_OPEN_TAG_WITH_ECHO ) {
+			$end_of_echo = $this->phpcsFile->findNext( [ T_SEMICOLON, T_CLOSE_TAG ], ( $stackPtr + 1 ) );
+			if ( $end_of_echo === false ) {
+				$this->in_short_echo = $this->phpcsFile->numTokens;
+			} else {
+				$this->in_short_echo = $end_of_echo;
+			}
+
+			return;
+		}
+
+		if ( $this->in_short_echo !== false && $this->in_short_echo < $stackPtr ) {
+			$this->in_short_echo = false;
+		}
 
 		$function_name = strtolower( $this->tokens[ $stackPtr ]['content'] );
 
@@ -121,10 +161,14 @@ public function process_token( $stackPtr ) {
 			return;
 		}
 
-		$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 = $this->echo_or_concat_tokens;
+		if ( $this->in_short_echo !== false ) {
 			$ignore[ T_COMMA ] = T_COMMA;
+		} else {
+			$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 );

WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc

@@ -95,3 +95,14 @@ echo '<input class="something something-else something-more"
 	action="', esc_url( $my_var ), '">'; // OK.
 echo '<input class="something something-else something-more"
 action="', esc_attr( $my_var ), '">'; // Error.
+
+// Verify correct handling of comma's in short open echo tags, without affecting subsequent statements.
+?>
+<div>html</div>
+<?= '<h1>' , esc_attr( $test ) , '</h1>'; // Error.
+printf( '<meta name="generator" content="%s">', esc_attr( $content ) ); // OK.
+echo '<a href="',  esc_html($url), '">'; // Error.
+?>
+<div>html</div>
+<?= '<h1 class="', esc_attr( $test ), '">'; ?><!-- OK -->
+<div>html</div>

WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.php

@@ -25,36 +25,38 @@ class ProperEscapingFunctionUnitTest extends AbstractSniffUnitTest {
 	 */
 	public function getErrorList() {
 		return [
-			3  => 1,
-			5  => 1,
-			15 => 1,
-			17 => 1,
-			21 => 1,
-			23 => 1,
-			33 => 1,
-			37 => 1,
-			41 => 1,
-			45 => 1,
-			48 => 1,
-			62 => 1,
-			63 => 1,
-			64 => 1,
-			65 => 1,
-			67 => 1,
-			68 => 1,
-			69 => 1,
-			72 => 1,
-			73 => 1,
-			74 => 1,
-			75 => 1,
-			76 => 1,
-			77 => 1,
-			78 => 1,
-			79 => 1,
-			80 => 1,
-			82 => 1,
-			92 => 1,
-			97 => 1,
+			3   => 1,
+			5   => 1,
+			15  => 1,
+			17  => 1,
+			21  => 1,
+			23  => 1,
+			33  => 1,
+			37  => 1,
+			41  => 1,
+			45  => 1,
+			48  => 1,
+			62  => 1,
+			63  => 1,
+			64  => 1,
+			65  => 1,
+			67  => 1,
+			68  => 1,
+			69  => 1,
+			72  => 1,
+			73  => 1,
+			74  => 1,
+			75  => 1,
+			76  => 1,
+			77  => 1,
+			78  => 1,
+			79  => 1,
+			80  => 1,
+			82  => 1,
+			92  => 1,
+			97  => 1,
+			102 => 1,
+			104 => 1,
 		];
 	}