Merge pull request #677 from Automattic/develop

2.3.1 Release

Author: rebeccahum

.github/ISSUE_TEMPLATE/release-template.md

@@ -1,7 +1,7 @@
 ---
 name: Release template
 about: Internally used for new releases
-title: Release 2.x.y
+title: Release x.y.z
 labels: 'Type: Maintenance'
 assignees: GaryJones, rebeccahum
 
@@ -13,6 +13,7 @@ assignees: GaryJones, rebeccahum
 
 PR for tracking changes for the X.Y.Z release. Target release date: DOW DD MMMM YYYY.
 
+- [ ] Scan WordPress (or just wp-admin folder) with prior version and compare results against new release for potential new bugs. 
 - [ ] Add change log for this release: PR #XXX
 - [ ] Double-check whether any dependencies need bumping.
 - [ ] Merge this PR.

CHANGELOG.md

@@ -4,6 +4,17 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [2.3.1] - 2021-04-23
+
+Props: jrfnl
+
+### Fixed
+- [#668](https://github.com/Automattic/VIP-Coding-Standards/pull/668): ProperEscapingFunction: fix overreach of comma usage in non-echo expressions for notAttrEscAttr.
+- [#670](https://github.com/Automattic/VIP-Coding-Standards/pull/670): ProperEscapingFunction: improve "action" match precision for hrefSrcEscUrl.
+
+## Deprecated
+- [#670](https://github.com/Automattic/VIP-Coding-Standards/pull/670): ProperEscapingFunction: private properties `$url_attrs` and `$attr_endings` are deprecated along with the public methods `is_html_attr()` and `attr_expects_url()`.
+
 ## [2.3.0] - 2021-04-19
 
 Props: jrfnl, rebeccahum, kevinfodness, GaryJones.
@@ -539,6 +550,7 @@ Initial release.
 Props: david-binda, pkevan.
 
 
+[2.3.1]: https://github.com/Automattic/VIP-Coding-Standards/compare/2.3.0...2.3.1
 [2.3.0]: https://github.com/Automattic/VIP-Coding-Standards/compare/2.2.0...2.3.0
 [2.2.0]: https://github.com/Automattic/VIP-Coding-Standards/compare/2.1.0...2.2.0
 [2.1.0]: https://github.com/Automattic/VIP-Coding-Standards/compare/2.0.0...2.1.0

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.
 	 *
@@ -46,13 +53,16 @@ class ProperEscapingFunctionSniff extends Sniff {
 		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.
 	 *
+	 * @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 = [
@@ -65,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 = [
@@ -75,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.
 	 *
@@ -83,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,
+		];
 	}
 
 	/**
@@ -94,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'] );
 
@@ -107,7 +161,17 @@ public function process_token( $stackPtr ) {
 			return;
 		}
 
-		$html = $this->phpcsFile->findPrevious( $this->echo_or_concat_tokens, $stackPtr - 1, null, true );
+		$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 );
 
 		// 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 ) {
@@ -129,13 +193,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;
@@ -145,6 +213,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.
@@ -165,6 +235,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>
@@ -82,3 +82,27 @@ 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.
+
+// 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.
+
+// 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,34 +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,
+			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,
 		];
 	}