Merge pull request #670 from Automattic/fix/669-properescaping-attribute-matching-precision

ProperEscapingFunction: improve "action" match precision

Author: rebeccahum

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.
 	 *
@@ -52,6 +59,10 @@ class ProperEscapingFunctionSniff extends Sniff {
 	/**
 	 * List of attributes associated with url outputs.
 	 *
+	 * @deprecated 2.3.1 Currently unused by the sniff, but needed for
+	 *                   for public methods which extending sniffs may be
+	 *                   relying on.
+	 *
 	 * @var array
 	 */
 	private $url_attrs = [
@@ -64,6 +75,10 @@ class ProperEscapingFunctionSniff extends Sniff {
 	/**
 	 * List of syntaxes for inside attribute detection.
 	 *
+	 * @deprecated 2.3.1 Currently unused by the sniff, but needed for
+	 *                   for public methods which extending sniffs may be
+	 *                   relying on.
+	 *
 	 * @var array
 	 */
 	private $attr_endings = [
@@ -134,13 +149,17 @@ 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;
@@ -150,6 +169,8 @@ public function process_token( $stackPtr ) {
 	/**
 	 * Tests whether provided string ends with open attribute which expects a URL value.
 	 *
+	 * @deprecated 2.3.1
+	 *
 	 * @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.
@@ -170,6 +191,8 @@ public function attr_expects_url( $content ) {
 	/**
 	 * Tests whether provided string ends with open HMTL attribute.
 	 *
+	 * @deprecated 2.3.1
+	 *
 	 * @param string $content Haystack in which we look for open HTML attribute.
 	 *
 	 * @return bool True if string ends with open HTML attribute.

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>
@@ -85,3 +85,13 @@ 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.
+?>
+
+// Making sure tabs and new lines before "action" are handled correctly.
+<input class="something something-else something-more"
+	action="<?php echo esc_attr( $my_var ); ?>"><!-- Error. -->
+<?php
+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.

WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.php

@@ -53,6 +53,8 @@ public function getErrorList() {
 			79 => 1,
 			80 => 1,
 			82 => 1,
+			92 => 1,
+			97 => 1,
 		];
 	}