ProperEscapingFunction: improve "action" match precision

In VIPCS 2.2.0, checking for the use of `esc_url()` for "action" HTML attributes was introduced in PR 575 in response to issue 554.

As it is not inconceivable that "action" is used as a suffix for custom HTML attributes - like "data-action"- for which the value does not necessarily has to be a URL, the check for the "action" HTML attribute should make sure it is the complete attribute name and not used as a suffix for a custom attribute name.

This change contains a minor refactor of the code which examines the content of the previous text string.

Instead of looping over the various lists and doing a `substr()` on the same content 25 times, it will now use a regular expression to gather the necessary information to throw the right errors in one go.

Includes unit tests.

Fixes 669

Note: This PR removes two `private` properties which were introduced in 624/VIPCS 2.3.0 and two `public` methods.

The removal of the properties is safe. The removal of the `public` methods could be considered a BC-break as these methods were `public`, though they never should have been.

If so preferred, the `public` methods could be deprecated instead and remain in the code base as dead code/emptied out methods until the next major release upon which they could be removed.

Author: jrfnl

WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php

@@ -18,6 +18,13 @@
  */
 class ProperEscapingFunctionSniff extends Sniff {
 
+	/**
+	 * Regular expression to match the end of HTML attributes.
+	 *
+	 * @var string
+	 */
+	const ATTR_END_REGEX = '`(?<attrname>href|src|url|\s+action)?=(?:(?:\\\\)?["\'])?$`i';
+
 	/**
 	 * List of escaping functions which are being tested.
 	 *
@@ -49,31 +56,6 @@ class ProperEscapingFunctionSniff extends Sniff {
 		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 = [
-		'=',
-		'="',
-		"='",
-		"=\\'",
-		'=\\"',
-	];
-
 	/**
 	 * Returns an array of tokens this test wants to listen for.
 	 *
@@ -134,57 +116,23 @@ public function process_token( $stackPtr ) {
 			return;
 		}
 
-		if ( $escaping_type !== 'url' && $this->attr_expects_url( $content ) ) {
+		if ( preg_match( self::ATTR_END_REGEX, $content, $matches ) !== 1 ) {
+			return;
+		}
+
+		if ( $escaping_type !== 'url' && empty( $matches['attrname'] ) === false ) {
 			$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 ( $escaping_type === 'html' && $this->is_html_attr( $content ) ) {
+		if ( $escaping_type === 'html' ) {
 			$message = 'Wrong escaping function. HTML attributes should be escaped by `esc_attr()`, not by `%s()`.';
 			$this->phpcsFile->addError( $message, $stackPtr, 'htmlAttrNotByEscHTML', $data );
 			return;
 		}
 	}
 
-	/**
-	 * Tests whether provided string ends with open attribute which expects a URL value.
-	 *
-	 * @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 expects a URL value.
-	 */
-	public function attr_expects_url( $content ) {
-		$attr_expects_url = false;
-		foreach ( $this->url_attrs as $attr ) {
-			foreach ( $this->attr_endings as $ending ) {
-				if ( $this->endswith( $content, $attr . $ending ) === true ) {
-					$attr_expects_url = true;
-					break;
-				}
-			}
-		}
-		return $attr_expects_url;
-	}
-
-	/**
-	 * Tests whether provided string ends with open HMTL attribute.
-	 *
-	 * @param string $content Haystack in which we look for open HTML attribute.
-	 *
-	 * @return bool True if string ends with open HTML attribute.
-	 */
-	public function is_html_attr( $content ) {
-		$is_html_attr = false;
-		foreach ( $this->attr_endings as $ending ) {
-			if ( $this->endswith( $content, $ending ) === true ) {
-				$is_html_attr = true;
-				break;
-			}
-		}
-		return $is_html_attr;
-	}
-
 	/**
 	 * Tests whether an attribute escaping function is being used outside of an HTML tag.
 	 *

WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc

@@ -38,9 +38,9 @@ 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)); ?>">
-
-
+<form method="post" action="<?php echo esc_html(admin_url('admin.php?page='.$base_name.'&amp;mode=logs&amp;id='.$poll_id)); ?>"><!-- Error. -->
+<input data-action="<?php echo esc_attr( $my_var ); ?>"><!-- OK. -->
+<a href='https://demo.com?foo=bar&my-action=<?php echo esc_attr( $var ); ?>'>link</a><!-- OK. -->
 
 <a href="#link"><?php echo esc_attr( 'testing' ); // Error.
 ?> </a>