ProperEscapingFunction: Flag when esc_attr is being used outside of HTML attributes

WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php

@@ -23,10 +23,56 @@ class ProperEscapingFunctionSniff extends Sniff {
 	 *
 	 * @var array
 	 */
-	public $escaping_functions = [
-		'esc_url',
-		'esc_attr',
-		'esc_html',
+	protected $escaping_functions = [
+		'esc_url'    => 'url',
+		'esc_attr'   => 'attr',
+		'esc_attr__' => 'attr',
+		'esc_attr_x' => 'attr',
+		'esc_attr_e' => 'attr',
+		'esc_html'   => 'html',
+		'esc_html__' => 'html',
+		'esc_html_x' => 'html',
+		'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,
+		T_NS_SEPARATOR       => T_NS_SEPARATOR,
+	];
+
+	/**
+	 * List of attributes associated with url outputs.
+	 *
+	 * @var array
+	 */
+	private $url_attrs = [
+		'href',
+		'src',
+		'url',
+		'action',
+	];
+
+	/**
+	 * List of syntaxes for inside attribute detection.
+	 *
+	 * @var array
+	 */
+	private $attr_endings = [
+		'=',
+		'="',
+		"='",
+		"=\\'",
+		'=\\"',
 	];
 
 	/**
@@ -35,6 +81,8 @@ class ProperEscapingFunctionSniff extends Sniff {
 	 * @return array
 	 */
 	public function register() {
+		$this->echo_or_concat_tokens += Tokens::$emptyTokens;
+
 		return [ T_STRING ];
 	}
 
@@ -47,47 +95,47 @@ public function register() {
 	 */
 	public function process_token( $stackPtr ) {
 
-		if ( in_array( $this->tokens[ $stackPtr ]['content'], $this->escaping_functions, true ) === false ) {
+		$function_name = strtolower( $this->tokens[ $stackPtr ]['content'] );
+
+		if ( isset( $this->escaping_functions[ $function_name ] ) === false ) {
 			return;
 		}
 
-		$function_name = $this->tokens[ $stackPtr ]['content'];
+		$next_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
+		if ( $next_non_empty === false || $this->tokens[ $next_non_empty ]['code'] !== T_OPEN_PARENTHESIS ) {
+			// Not a function call.
+			return;
+		}
 
-		$echo_or_string_concat = $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_string_concat ]['code'] === T_ECHO ) {
-			// Very likely inline HTML with <?php tag.
-			$php_open = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $echo_or_string_concat - 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 ) {
+			return;
+		}
 
-			if ( $this->tokens[ $php_open ]['code'] !== T_OPEN_TAG ) {
-				return;
-			}
+		$data = [ $function_name ];
 
-			$html = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $php_open - 1, null, true );
+		$content = $this->tokens[ $html ]['content'];
+		if ( isset( Tokens::$stringTokens[ $this->tokens[ $html ]['code'] ] ) === true ) {
+			$content = Sniff::strip_quotes( $content );
+		}
 
-			if ( $this->tokens[ $html ]['code'] !== T_INLINE_HTML ) {
-				return;
-			}
-		} elseif ( $this->tokens[ $echo_or_string_concat ]['code'] === T_STRING_CONCAT ) {
-			// Very likely string concatenation mixing strings and functions/variables.
-			$html = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $echo_or_string_concat - 1, null, true );
+		$escaping_type = $this->escaping_functions[ $function_name ];
 
-			if ( $this->tokens[ $html ]['code'] !== T_CONSTANT_ENCAPSED_STRING ) {
-				return;
-			}
-		} else {
-			// Neither - bailing.
+		if ( $escaping_type === 'attr' && $this->is_outside_html_attr_context( $content ) ) {
+			$message = 'Wrong escaping function, using `%s()` in a context outside of HTML attributes may not escape properly.';
+			$this->phpcsFile->addError( $message, $html, 'notAttrEscAttr', $data );
 			return;
 		}
 
-		$data = [ $function_name ];
-
-		if ( $function_name !== 'esc_url' && $this->attr_expects_url( $this->tokens[ $html ]['content'] ) ) {
+		if ( $escaping_type !== 'url' && $this->attr_expects_url( $content ) ) {
 			$message = 'Wrong escaping function. href, src, and action attributes should be escaped by `esc_url()`, not by `%s()`.';
 			$this->phpcsFile->addError( $message, $stackPtr, 'hrefSrcEscUrl', $data );
 			return;
 		}
-		if ( $function_name === 'esc_html' && $this->is_html_attr( $this->tokens[ $html ]['content'] ) ) {
+
+		if ( $escaping_type === 'html' && $this->is_html_attr( $content ) ) {
 			$message = 'Wrong escaping function. HTML attributes should be escaped by `esc_attr()`, not by `%s()`.';
 			$this->phpcsFile->addError( $message, $stackPtr, 'htmlAttrNotByEscHTML', $data );
 			return;
@@ -99,17 +147,12 @@ public function process_token( $stackPtr ) {
 	 *
 	 * @param string $content Haystack in which we look for an open attribute which exects a URL value.
 	 *
-	 * @return bool True if string ends with open attribute which exects a URL value.
+	 * @return bool True if string ends with open attribute which expects a URL value.
 	 */
 	public function attr_expects_url( $content ) {
 		$attr_expects_url = false;
-		foreach ( [ 'href', 'src', 'url', 'action' ] as $attr ) {
-			foreach ( [
-				'="',
-				"='",
-				'=\'"', // The tokenizer does some fun stuff when it comes to mixing double and single quotes.
-				'="\'', // The tokenizer does some fun stuff when it comes to mixing double and single quotes.
-			] as $ending ) {
+		foreach ( $this->url_attrs as $attr ) {
+			foreach ( $this->attr_endings as $ending ) {
 				if ( $this->endswith( $content, $attr . $ending ) === true ) {
 					$attr_expects_url = true;
 					break;
@@ -128,12 +171,7 @@ public function attr_expects_url( $content ) {
 	 */
 	public function is_html_attr( $content ) {
 		$is_html_attr = false;
-		foreach ( [
-			'="',
-			"='",
-			'=\'"', // The tokenizer does some fun stuff when it comes to mixing double and single quotes.
-			'="\'', // The tokenizer does some fun stuff when it comes to mixing double and single quotes.
-		] as $ending ) {
+		foreach ( $this->attr_endings as $ending ) {
 			if ( $this->endswith( $content, $ending ) === true ) {
 				$is_html_attr = true;
 				break;
@@ -142,11 +180,22 @@ public function is_html_attr( $content ) {
 		return $is_html_attr;
 	}
 
+	/**
+	 * Tests whether an attribute escaping function is being used outside of an HTML tag.
+	 *
+	 * @param string $content Haystack where we look for the end of a HTML tag.
+	 *
+	 * @return bool True if the passed string ends a HTML tag.
+	 */
+	public function is_outside_html_attr_context( $content ) {
+		return $this->endswith( trim( $content ), '>' );
+	}
+
 	/**
 	 * A helper function which tests whether string ends with some other.
 	 *
 	 * @param string $haystack String which is being tested.
-	 * @param string $needle The substring, which we try to locate on the end of the $haystack.
+	 * @param string $needle   The substring, which we try to locate on the end of the $haystack.
 	 *
 	 * @return bool True if haystack ends with needle.
 	 */

WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc

@@ -1,26 +1,26 @@
 <?php
 
-echo '<a href="' . esc_attr( $some_var ) . '"></a>'; // NOK.
+echo '<a href="' . esc_attr( $some_var ) . '"></a>'; // Error.
 
-echo "<a href='" . esc_attr( $some_var ) . "'></a>"; // NOK.
+echo "<a href='" . \esc_attr( $some_var ) . "'></a>"; // Error.
 
-echo '<a href="' . esc_url( $some_var ) . '"></a>'; // OK.
+echo '<a href="' . \esc_url( $some_var ) . '"></a>'; // OK.
 
 echo "<a href='" . esc_url( $some_var ) . "'></a>"; // OK.
 
 echo '<a title="' . esc_attr( $some_var ) . '"></a>'; // OK.
 
-echo "<a title='" . esc_attr( $some_var ) . "'></a>"; // OK.
+echo "<a title='" . \esc_attr( $some_var ) . "'></a>"; // OK.
 
-echo '<a title="' . esc_html( $some_var ) . '"></a>'; // NOK.
+echo '<a title="' . esc_html_x( $some_var ) . '"></a>'; // Error.
 
-echo "<a title='" . esc_html( $some_var ) . "'></a>"; // NOK.
+echo "<a title='" . \esc_html( $some_var ) . "'></a>"; // Error.
 
 ?>
 
-<a href="<?php echo esc_attr( $some_var ); ?>">Hello</a> <!-- NOK. -->
+<a href="<?php echo esc_attr( $some_var ); ?>">Hello</a> <!-- Error. -->
 
-<a href="" class="<?php echo esc_html( $some_var); ?>">Hey</a> <!-- NOK. -->
+<a href="" class="<?php esc_html_e( $some_var); ?>">Hey</a> <!-- Error. -->
 
 <a href="<?php esc_url( $url );?>"></a> <!-- OK. -->
 
@@ -30,12 +30,55 @@ echo "<a title='" . esc_html( $some_var ) . "'></a>"; // NOK.
 
 echo '<media:content url="' . esc_url( $post_image ) . '" medium="image">'; // OK.
 
-echo '<media:content url="' . esc_attr( $post_image ) . '" medium="image">'; // NOK.
+echo '<media:content url="' . esc_attr( $post_image ) . '" medium="image">'; // Error.
 
 echo 'data-param-url="' . esc_url( $share_url ) . '"'; // OK.
 
-echo 'data-param-url="' . esc_html( $share_url ) . '"'; // NOK.
+echo 'data-param-url="' . Esc_HTML( $share_url ) . '"'; // Error.
 
 ?>
 
 <form method="post" action="<?php echo esc_html(admin_url('admin.php?page='.$base_name.'&amp;mode=logs&amp;id='.$poll_id)); ?>">
+
+
+
+<a href="#link"><?php echo esc_attr( 'testing' ); // Error.
+?> </a>
+
+<?php echo ' <h' . "2>    " ,        esc_attr( $heading ) . "</h2>"; // Error.
+?> <style>
+h1 {
+  color: <?php echo esc_attr( $color ) ?>; /* OK */
+  font-family: Arial;
+  font-size: 10px;
+}
+</style>
+
+<article id="foo-bar" data-property="<?php echo esc_attr( $year ); // OK.
+?>">
+Test
+</article>
+
+<h1><?php echo esc_attr__( $title, 'domain' ); ?></h1> <!-- Error --> ?>
+<?php echo '<h1>' . esc_attr__( $some_var, 'domain' ) . '</h1>'; // Error.
+echo '<h1>',      \esc_attr_x( $title, 'domain' ), '</h1>'; // Error.
+echo "<$tag>  " , esc_attr( $test ) , "</$tag>"; // 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 -->
+<?php echo "<div>" . $test . "</div>"; // OK.
+echo "<{$tag}>" . esc_attr( $tag_content ) . "</{$tag}>"; // Error.
+echo "<$tag" . '   >' . esc_attr( $tag_content ) . "</$tag>"; // Error.
+echo '<div class=\'' . esc_html($class) . '\'>'; // Error.
+echo "<div class=\"" . \esc_html__($class) . '">'; // Error.
+echo "<div $someAttribute class=\"" . esc_html($class) . '">'; // Error.
+echo '<a href=\'' . esc_html($url) . '\'>'; // Error.
+echo "<img src=\"" . esc_html($src) . '"/>'; // Error.
+echo "<div $someAttributeName-url=\"" . esc_html($url) . '">'; // Error.
+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.

WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.php

@@ -34,6 +34,25 @@ public function getErrorList() {
 			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,
 		];
 	}