Security/EscapingVoidReturnFunctions: various improvements

WordPressVIPMinimum/Sniffs/Security/EscapingVoidReturnFunctionsSniff.php

@@ -10,8 +10,9 @@
 namespace WordPressVIPMinimum\Sniffs\Security;
 
 use PHP_CodeSniffer\Util\Tokens;
+use PHPCSUtils\Utils\PassedParameters;
+use WordPressCS\WordPress\AbstractFunctionParameterSniff;
 use WordPressCS\WordPress\Helpers\PrintingFunctionsTrait;
-use WordPressVIPMinimum\Sniffs\Sniff;
 
 /**
  * Flag functions that don't return anything, yet are wrapped in an escaping function call.
@@ -20,46 +21,131 @@
  *
  * @uses \WordPressCS\WordPress\Helpers\PrintingFunctionsTrait::$customPrintingFunctions
  */
-class EscapingVoidReturnFunctionsSniff extends Sniff {
+class EscapingVoidReturnFunctionsSniff extends AbstractFunctionParameterSniff {
 
 	use PrintingFunctionsTrait;
 
 	/**
-	 * Returns an array of tokens this test wants to listen for.
+	 * The group name for this group of functions.
 	 *
-	 * @return array<int|string>
+	 * @var string
 	 */
-	public function register() {
-		return [
-			T_STRING,
-		];
-	}
+	protected $group_name = 'escaping_void';
+
+	/**
+	 * Functions this sniff is looking for.
+	 *
+	 * @var array<string, array{param_position: int, param_name: string}> Keys are the target functions,
+	 *                                                                    value, the name and position of the target parameter.
+	 */
+	protected $target_functions = [
+		'esc_attr' => [
+			'param_position' => 1,
+			'param_name'     => 'text',
+		],
+		'esc_attr__' => [
+			'param_position' => 1,
+			'param_name'     => 'text',
+		],
+		'esc_attr_e' => [
+			'param_position' => 1,
+			'param_name'     => 'text',
+		],
+		'esc_attr_x' => [
+			'param_position' => 1,
+			'param_name'     => 'text',
+		],
+		'esc_html' => [
+			'param_position' => 1,
+			'param_name'     => 'text',
+		],
+		'esc_html__' => [
+			'param_position' => 1,
+			'param_name'     => 'text',
+		],
+		'esc_html_e' => [
+			'param_position' => 1,
+			'param_name'     => 'text',
+		],
+		'esc_html_x' => [
+			'param_position' => 1,
+			'param_name'     => 'text',
+		],
+		'esc_js' => [
+			'param_position' => 1,
+			'param_name'     => 'text',
+		],
+		'esc_textarea' => [
+			'param_position' => 1,
+			'param_name'     => 'text',
+		],
+		'esc_url' => [
+			'param_position' => 1,
+			'param_name'     => 'url',
+		],
+		'esc_url_raw' => [
+			'param_position' => 1,
+			'param_name'     => 'url',
+		],
+		'esc_xml' => [
+			'param_position' => 1,
+			'param_name'     => 'text',
+		],
+		'tag_escape' => [
+			'param_position' => 1,
+			'param_name'     => 'tag_name',
+		],
+		'wp_kses' => [
+			'param_position' => 1,
+			'param_name'     => 'content',
+		],
+		'wp_kses_data' => [
+			'param_position' => 1,
+			'param_name'     => 'data',
+		],
+		'wp_kses_one_attr' => [
+			'param_position' => 1,
+			'param_name'     => 'attr',
+		],
+		'wp_kses_post' => [
+			'param_position' => 1,
+			'param_name'     => 'data',
+		],
+	];
 
 	/**
-	 * Process this test when one of its tokens is encountered
+	 * Process the parameters of a matched function.
 	 *
-	 * @param int $stackPtr The position of the current token in the stack passed in $tokens.
+	 * @param int    $stackPtr        The position of the current token in the stack.
+	 * @param string $group_name      The name of the group which was matched.
+	 * @param string $matched_content The token content (function name) which was matched
+	 *                                in lowercase.
+	 * @param array  $parameters      Array with information about the parameters.
 	 *
 	 * @return void
 	 */
-	public function process_token( $stackPtr ) {
+	public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
+		$param_position = $this->target_functions[ $matched_content ]['param_position'];
+		$param_name     = $this->target_functions[ $matched_content ]['param_name'];
 
-		if ( strpos( $this->tokens[ $stackPtr ]['content'], 'esc_' ) !== 0 && strpos( $this->tokens[ $stackPtr ]['content'], 'wp_kses' ) !== 0 ) {
-			// Not what we are looking for.
+		$target_param = PassedParameters::getParameterFromStack( $parameters, $param_position, $param_name );
+		if ( $target_param === false ) {
+			// Missing (required) target parameter. Probably live coding, nothing to examine (yet). Bow out.
 			return;
 		}
 
-		$next_token = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true );
+		$ignore                   = Tokens::$emptyTokens;
+		$ignore[ T_NS_SEPARATOR ] = T_NS_SEPARATOR;
 
-		if ( $this->tokens[ $next_token ]['code'] !== T_OPEN_PARENTHESIS ) {
-			// Not a function call.
+		$next_token = $this->phpcsFile->findNext( $ignore, $target_param['start'], ( $target_param['end'] + 1 ), true );
+		if ( $next_token === false || $this->tokens[ $next_token ]['code'] !== T_STRING ) {
+			// Not what we are looking for.
 			return;
 		}
 
-		$next_token = $this->phpcsFile->findNext( Tokens::$emptyTokens, $next_token + 1, null, true );
-
-		if ( $this->tokens[ $next_token ]['code'] !== T_STRING ) {
-			// Not what we are looking for.
+		$next_after = $this->phpcsFile->findNext( Tokens::$emptyTokens, $next_token + 1, ( $target_param['end'] + 1 ), true );
+		if ( $next_after === false || $this->tokens[ $next_after ]['code'] !== T_OPEN_PARENTHESIS ) {
+			// Not a function call inside the escaping function.
 			return;
 		}
 

WordPressVIPMinimum/Tests/Security/EscapingVoidReturnFunctionsUnitTest.inc

@@ -1,4 +1,100 @@
 <?php
 
-esc_html( _e( $something ) ); // NOK.
-esc_html( __( $something ) ); // NOK.
+/*
+ * Not the sniff target.
+ */
+use function esc_attr;
+
+my\ns\esc_html( _e( $something ) );
+$this->esc_js( _deprecated_file() );
+$this?->esc_textarea( _e( $something ) );
+MyClass::esc_url( wp_die(), );
+echo WP_KSES;
+namespace\wp_kses_post( user_error( $something ) );
+
+function esc_js( _E $param ) {} // Class "_E" as type declaration.
+
+/*
+ * These should all be okay.
+ */
+// Incomplete function call, should be ignored by the sniff.
+$incorrect_but_ok = esc_html();
+$incorrect_but_ok = wp_kses();
+
+// Parameter does not contain a function call.
+esc_html_x( $hook_name );
+\wp_kses_data( $obj->_deprecated_function() );
+esc_attr__( CONSTANT_NAME, );
+\ESC_ATTR_X( "do_$something" );
+
+esc_attr(...$params); // PHP 5.6 argument unpacking.
+
+#[Esc_Textarea('text')] // PHP 8.0+ class instantiation via an attribute. Can't contain a nested function call anyway.
+function foo() {}
+
+array_walk($text_strings, \esc_html__(...),); // PHP 8.1 first class callable.
+
+// Parameter contains a function call but not calling one of the printing functions.
+esc_html( __( $something ) );
+Esc_URL_Raw( \get( $url ) );
+\esc_xml( deprecated_argument() ); // Note: missing "_" prefix for printing function.
+wp_kses_one_attr   (
+	/* comment */
+	get_attr(),
+);
+
+
+/*
+ * These should all be flagged.
+ */
+esc_html( _e( $something ) );
+esc_attr__( \_deprecated_argument( $a ) );
+ESC_ATTR_E( _Deprecated_Constructor($a), );
+\esc_attr_x( _deprecated_file(), );
+esc_attr( _deprecated_function() );
+esc_HTML__( _deprecated_hook() );
+esc_html_e( _Doing_It_Wrong( $a ) );
+esc_html_X( \_e( $foo ), );
+\esc_html( /*comment*/ _ex( $foo ) );
+esc_js( printf( $foo ) );
+Esc_textarea( trigger_error( $foo ) );
+\esc_URL_raw( \user_Error( $foo, '' ), );
+esc_url( vprintf( $foo, ) /*comment*/ );
+WP_Kses_Data( WP_DIE( $foo ), );
+wp_kses_one_attr /*comment*/ ( wp_dropdown_pages( $pages ) );
+wp_kses_post(
+	\_deprecated_function( $fn )
+);
+wp_kses( _ex() );
+
+// Adding custom printing functions is supported.
+// phpcs:set WordPressVIPMinimum.Security.EscapingVoidReturnFunctions customPrintingFunctions[] to_screen,my_print
+esc_attr( to_screen( $var1 ) );
+\wp_kses_post( my_print() );
+// phpcs:set WordPressVIPMinimum.Security.EscapingVoidReturnFunctions customPrintingFunctions[]
+
+tag_escape( $tag ); // OK.
+tag_escape( _e() ); // Bad.
+
+// Bug: these are not function calls inside.
+esc_attr__( User_Error::CONSTANT_NAME, ); // OK.
+esc_js( _doing_it_wrong::class ); // OK, PHP 5.5 ::class resolution.
+
+// Safeguard handling of function calls using PHP 8.0+ named parameters.
+esc_attr_x( context: get_context(), domain: _e(),); // OK, well, not really, missing required $text param, but that's not the concern of this sniff.
+esc_url_raw(protocols: $protocols, url: $url); // OK.
+wp_kses_one_attr(element: $element, att: trigger_error( $foo ) ); // OK, well, not really, typo in param name, but that's not the concern of the sniff.
+wp_kses( content: \not_deprecated_function( $fn ) ); // OK.
+wp_kses( content: /*comment*/ ); // OK, well, not really, invalid function call, but that's not the concern of the sniff.
+
+esc_html_x(context: $c, text: _doing_it_wrong() ); // Bad.
+wp_kses(allowed_html: $allowed_html, content: printf() ); // Bad.
+esc_url(protocols: $protocols, url: vprintf()); // Bad.
+
+// These are no longer flagged as they are not "escaping" functions for the purpose of this sniff.
+esc_sql( trigger_error( $foo ) ); // OK.
+wp_kses_allowed_html( _deprecated_function() ); // OK.
+
+// These are no longer flagged as these are not the WP native escaping functions, so we don't know what parameter to check.
+esc_something( trigger_error( $foo ) ); // OK.
+wp_kses_page( _deprecated_function() ); // OK.

WordPressVIPMinimum/Tests/Security/EscapingVoidReturnFunctionsUnitTest.php

@@ -23,7 +23,29 @@ class EscapingVoidReturnFunctionsUnitTest extends AbstractSniffUnitTest {
 	 */
 	public function getErrorList() {
 		return [
-			3 => 1,
+			50 => 1,
+			51 => 1,
+			52 => 1,
+			53 => 1,
+			54 => 1,
+			55 => 1,
+			56 => 1,
+			57 => 1,
+			58 => 1,
+			59 => 1,
+			60 => 1,
+			61 => 1,
+			62 => 1,
+			63 => 1,
+			64 => 1,
+			65 => 1,
+			68 => 1,
+			72 => 1,
+			73 => 1,
+			77 => 1,
+			90 => 1,
+			91 => 1,
+			92 => 1,
 		];
 	}