Hooks/RestrictedHooks: various sniff improvements

WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php

@@ -9,6 +9,10 @@
 
 namespace WordPressVIPMinimum\Sniffs\Hooks;
 
+use PHP_CodeSniffer\Util\Tokens;
+use PHPCSUtils\Utils\MessageHelper;
+use PHPCSUtils\Utils\PassedParameters;
+use PHPCSUtils\Utils\TextStrings;
 use WordPressCS\WordPress\AbstractFunctionParameterSniff;
 
 /**
@@ -43,15 +47,15 @@ class RestrictedHooksSniff extends AbstractFunctionParameterSniff {
 	private $restricted_hook_groups = [
 		'upload_mimes' => [
 			// TODO: This error message needs a link to the VIP Documentation, see https://github.com/Automattic/VIP-Coding-Standards/issues/235.
-			'type'  => 'Warning',
+			'type'  => 'warning',
 			'msg'   => 'Please ensure that the mimes being filtered do not include insecure types (i.e. SVG, SWF, etc.). Manual inspection required.',
 			'hooks' => [
 				'upload_mimes',
 			],
 		],
 		'http_request' => [
 			// https://docs.wpvip.com/technical-references/code-quality-and-best-practices/retrieving-remote-data/.
-			'type'  => 'Warning',
+			'type'  => 'warning',
 			'msg'   => 'Please ensure that the timeout being filtered is not greater than 3s since remote requests require the user to wait for completion before the rest of the page will load. Manual inspection required.',
 			'hooks' => [
 				'http_request_timeout',
@@ -60,7 +64,7 @@ class RestrictedHooksSniff extends AbstractFunctionParameterSniff {
 		],
 		'robotstxt' => [
 			// https://docs.wpvip.com/how-tos/modify-the-robots-txt-file/.
-			'type'  => 'Warning',
+			'type'  => 'warning',
 			'msg'   => 'Don\'t forget to flush the robots.txt cache by going to Settings > Reading and toggling the privacy settings.',
 			'hooks' => [
 				'do_robotstxt',
@@ -82,11 +86,23 @@ class RestrictedHooksSniff extends AbstractFunctionParameterSniff {
 	 *                  normal file processing.
 	 */
 	public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
+		$hook_name_param = PassedParameters::getParameterFromStack( $parameters, 1, 'hook_name' );
+		if ( $hook_name_param === false ) {
+			// Missing required parameter. Nothing to examine. Bow out.
+			return;
+		}
+
+		$normalized_hook_name = $this->normalize_hook_name_from_parameter( $hook_name_param );
+		if ( $normalized_hook_name === '' ) {
+			// Dynamic hook name. Cannot reliably determine if it's one of the targets. Bow out.
+			return;
+		}
+
 		foreach ( $this->restricted_hook_groups as $group => $group_args ) {
 			foreach ( $group_args['hooks'] as $hook ) {
-				if ( $this->normalize_hook_name_from_parameter( $parameters[1] ) === $hook ) {
-					$addMethod = 'add' . $group_args['type'];
-					$this->phpcsFile->{$addMethod}( $group_args['msg'], $stackPtr, $hook );
+				if ( $normalized_hook_name === $hook ) {
+					$isError = ( $group_args['type'] === 'error' );
+					MessageHelper::addMessage( $this->phpcsFile, $group_args['msg'], $stackPtr, $isError, $hook );
 				}
 			}
 		}
@@ -97,31 +113,27 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p
 	 *
 	 * @param array $parameter Array with information about a parameter.
 	 *
-	 * @return string Normalized hook name.
+	 * @return string Normalized hook name or an empty string if the hook name could not be determined.
 	 */
 	private function normalize_hook_name_from_parameter( $parameter ) {
-		// If concatenation is found, build hook name.
-		$concat_ptr = $this->phpcsFile->findNext(
-			T_STRING_CONCAT,
-			$parameter['start'],
-			$parameter['end'],
-			false,
-			null,
-			true
-		);
+		$allowed_tokens  = Tokens::$emptyTokens;
+		$allowed_tokens += [
+			T_STRING_CONCAT            => T_STRING_CONCAT,
+			T_CONSTANT_ENCAPSED_STRING => T_CONSTANT_ENCAPSED_STRING,
+		];
 
-		if ( $concat_ptr ) {
-			$hook_name = '';
-			for ( $i = $parameter['start'] + 1; $i < $parameter['end']; $i++ ) {
-				if ( $this->tokens[ $i ]['code'] === T_CONSTANT_ENCAPSED_STRING ) {
-					$hook_name .= str_replace( [ "'", '"' ], '', $this->tokens[ $i ]['content'] );
-				}
+		$has_disallowed_token = $this->phpcsFile->findNext( $allowed_tokens, $parameter['start'], ( $parameter['end'] + 1 ), true );
+		if ( $has_disallowed_token !== false ) {
+			return '';
+		}
+
+		$hook_name = '';
+		for ( $i = $parameter['start']; $i <= $parameter['end']; $i++ ) {
+			if ( $this->tokens[ $i ]['code'] === T_CONSTANT_ENCAPSED_STRING ) {
+				$hook_name .= TextStrings::stripQuotes( $this->tokens[ $i ]['content'] );
 			}
-		} else {
-			$hook_name = $parameter['raw'];
 		}
 
-		// Remove quotes (double and single), and use lowercase.
-		return strtolower( str_replace( [ "'", '"' ], '', $hook_name ) );
+		return $hook_name;
 	}
 }

WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc

@@ -1,24 +1,84 @@
 <?php
 
-add_filter( 'upload_mime', 'good_example_function' ); // Ok.
-add_filter( 'upload_mimesX', 'good_example_function' ); // Ok.
-
-// Warnings.
-add_filter( 'upload_mimes', 'bad_example_function' ); // Simple string.
-add_filter('upload_mimes' ,'bad_example_function'); // Incorrect spacing.
-add_filter(  'upload_mimes','bad_example_function'); // Incorrect spacing.
-add_filter( "upload_mimes" ,'bad_example_function'); // Double quotes.
-add_filter( 'upLoad_mimeS' ,'bad_example_function'); // Uppercase characters.
-add_filter( 'upload_' . 'mimes' ,'bad_example_function'); // Single concatenation.
-add_filter( 'upl' . 'oad_' . 'mimes' ,'bad_example_function'); // Multiple concatenation.
-add_filter( "upload_" . 'mimes' ,'bad_example_function'); // Single concatenation with double and single quotes.
-add_filter( 'upl' . "oad_" . "mimes" ,'bad_example_function'); // Multiple concatenation with double and single quotes.
-add_filter( 'upload_mimes', function() { // Anonymous callback.
+/*
+ * Not the sniff target.
+ */
+use add_filter;
+
+my\ns\add_filter($a, $b);
+$this->add_action($a, $b);
+$this?->add_filter($a, $b);
+MyClass::add_action($a, $b);
+echo ADD_FILTER;
+namespace\add_action($a, $b);
+
+
+/*
+ * These should all be okay.
+ */
+add_filter( 'not_target_hook', 'good_example_function' );
+\add_filter( 'upload_mimesX'  , 'good_example_function' );
+
+add_action(...$params); // PHP 5.6 argument unpacking.
+
+// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute.
+#[ADD_FILTER('text')]
+function foo() {}
+
+// PHP 8.1 first class callable.
+// As we have no knowledge about what parameters will be passed, we shouldn't flag this.
+array_walk($filters, add_filter(...));
+
+// Ignore as undetermined.
+Add_Filter( $hook_name, 'undetermined' );
+\add_action( $obj->get_filterName(), 'undetermined' );
+add_filter( MyClass::FILTER_NAME, 'undetermined', );
+\add_filter( "upload_$mimes", 'undetermined' );
+
+// Incomplete function call, should be ignored by the sniff.
+$incorrect_but_ok = add_filter();
+$incorrect_but_ok = add_action();
+
+
+/*
+ * These should all be flagged with a warning.
+ */
+add_filter( 'do_robotstxt', 'bad_example_function' ); // Simple string.
+add_action('upload_mimes' , [$obj, 'method']); // Incorrect spacing.
+add_filter(  'robots_txt','bad_example_function'); // Incorrect spacing.
+\add_filter( "http_request_timeout" , fn($param) => $param * 10); // Double quotes.
+
+ADD_FILTER( 'upload_' . 'mimes','bad_example_function'); // Single concatenation.
+add_filter( 'upl' . 'oad_' . 'mimes','bad_example_function'); // Multiple concatenation.
+add_filter( "upload_" . 'mimes' , bad_example_function(...)); // Single concatenation with double and single quotes.
+add_filter( 'upl' . "oad_" . "mimes",'bad_example_function'); // Multiple concatenation with double and single quotes.
+\add_action( 'http_request_args', function() { // Anonymous callback.
 	// Do stuff.
 });
 add_action( 'upload_mimes', 'bad_example_function' ); // Check `add_action()`, which is an alias for `add_filter()`.
 add_filter( 'http_request_timeout', 'bad_example_function' ); // Simple string.
 add_filter('http_request_args', 'bad_example_function' ); // Simple string + incorrect spacing.
-add_action( 'do_robotstxt', 'my_do_robotstxt'); // Simple string.
+add_action( /*comment*/ 'do_robotstxt', 'my_do_robotstxt'); // Simple string.
 add_filter( 'robots_txt', function() { // Anonymous callback.
 } );
+
+// Safeguard correct handling of function calls using PHP 8.0+ named parameters.
+add_action(callback: 'invalid', priority: 10); // OK, well, not really, missing required $hook_name param, but that's not the concern of this sniff.
+add_action(callback: 'do_robotstxt', hook_name: 'not_our_target'); // OK.
+add_action(hookName: 'not_our_target', callback: 'do_robotstxt',); // OK, well, not really, typo in param name, but that's not the concern of the sniff.
+
+add_filter(priority: 10, hook_name: 'robots_txt', callback: some_function(...) ); // Warning.
+
+// Hook names are case-sensitive.
+add_filter( 'upLoad_mimeS' , $callback); // OK, not our target.
+
+// Bug fix - spacing vs concatenation.
+add_filter('do_' . 'robots' . 'txt', 'bad_example_function'); // Warning.
+
+// Ignore partially dynamic hook names.
+add_filter( 'robots_' . $something . 'txt' , $callback); // OK, ignored as undetermined.
+add_filter( 'http_request_timeout' . $something, $callback); // OK, ignored as undetermined.
+
+// Ensure quote stripping is done correctly.
+add_filter( 'upload"_mimes', 'bad_example_function' ); // OK, not a filter we're looking for.
+add_filter( "upload_'mimes", 'bad_example_function' ); // OK, not a filter we're looking for.

WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php

@@ -33,21 +33,22 @@ public function getErrorList() {
 	 */
 	public function getWarningList() {
 		return [
-			7  => 1,
-			8  => 1,
-			9  => 1,
-			10 => 1,
-			11 => 1,
-			12 => 1,
-			13 => 1,
-			14 => 1,
-			15 => 1,
-			16 => 1,
-			19 => 1,
-			20 => 1,
-			21 => 1,
-			22 => 1,
-			23 => 1,
+			46 => 1,
+			47 => 1,
+			48 => 1,
+			49 => 1,
+			51 => 1,
+			52 => 1,
+			53 => 1,
+			54 => 1,
+			55 => 1,
+			58 => 1,
+			59 => 1,
+			60 => 1,
+			61 => 1,
+			62 => 1,
+			70 => 1,
+			76 => 1,
 		];
 	}
 }