From f55b2445e74d298c1ff4baeb90403bf01bdcbb7d Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 20 Jul 2025 06:03:42 +0200 Subject: [PATCH 1/8] Hooks/RestrictedHooks: improve the tests * Test against false positives for calls to methods or namespaced function calls. * Test against false positives for attribute class using the same name as the function. * Ensure function import `use` statements are not flagged. We're not interested in those. * Ensure PHP 8.1 first class callables are not flagged. We cannot determine the hook name for those. * Document how dynamic hook names are handled. * Add some more variations to the pre-existing tests: - Non-lowercase function call(s). - Fully qualified function calls. - Use PHP 7.3+ trailing comma's in a few function calls. --- .../Tests/Hooks/RestrictedHooksUnitTest.inc | 63 +++++++++++++++---- .../Tests/Hooks/RestrictedHooksUnitTest.php | 30 ++++----- 2 files changed, 66 insertions(+), 27 deletions(-) diff --git a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc index 9236f8ee..4d9a69fa 100644 --- a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc @@ -1,19 +1,58 @@ 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' , $callback); // 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( "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. +\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()`. diff --git a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php index 2f6172be..2d404cd4 100644 --- a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php +++ b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php @@ -33,21 +33,21 @@ 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, + 50 => 1, + 51 => 1, + 52 => 1, + 53 => 1, + 54 => 1, + 55 => 1, + 58 => 1, + 59 => 1, + 60 => 1, + 61 => 1, + 62 => 1, ]; } } From 3608427846f8e9eb1bedd5014d66c803e9eb9922 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 20 Jul 2025 01:52:15 +0200 Subject: [PATCH 2/8] Hooks/RestrictedHooks: bug fix - disregard comments in hook name parameter The PHPCSUtils `PassedParameters::getParameters()` return value includes a `'clean'` array index, which contains the contents of the parameter stripped of surrounding whitespace and comments. The `RestrictedHooks` sniff uses the parameter contents to compare against a list of restricted hooks, but would break if the hook name parameter would contain a comment. Fixed now. Includes test. --- WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php | 2 +- WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php index 3d64ec5c..70949f60 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php @@ -118,7 +118,7 @@ private function normalize_hook_name_from_parameter( $parameter ) { } } } else { - $hook_name = $parameter['raw']; + $hook_name = $parameter['clean']; } // Remove quotes (double and single), and use lowercase. diff --git a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc index 4d9a69fa..78a16deb 100644 --- a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc @@ -58,6 +58,6 @@ add_filter( 'upl' . "oad_" . "mimes" ,'bad_example_function'); // Multiple conca 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. } ); From 165aef3f741cbe917a3787ee518a5969dabb61a6 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 20 Jul 2025 06:21:01 +0200 Subject: [PATCH 3/8] Hooks/RestrictedHooks: add support for handling PHP 8.0+ function calls using named parameters Includes minor efficiency fix by moving the call to `normalize_hook_name_from_parameter()` out of the loops. Includes tests. --- .../Sniffs/Hooks/RestrictedHooksSniff.php | 11 ++++++++++- .../Tests/Hooks/RestrictedHooksUnitTest.inc | 7 +++++++ .../Tests/Hooks/RestrictedHooksUnitTest.php | 1 + 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php index 70949f60..5b8a8466 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php @@ -9,6 +9,7 @@ namespace WordPressVIPMinimum\Sniffs\Hooks; +use PHPCSUtils\Utils\PassedParameters; use WordPressCS\WordPress\AbstractFunctionParameterSniff; /** @@ -82,9 +83,17 @@ 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 ); + 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 ) { + if ( $normalized_hook_name === $hook ) { $addMethod = 'add' . $group_args['type']; $this->phpcsFile->{$addMethod}( $group_args['msg'], $stackPtr, $hook ); } diff --git a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc index 78a16deb..e79810cf 100644 --- a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc @@ -61,3 +61,10 @@ add_filter('http_request_args', 'bad_example_function' ); // Simple string + inc 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. diff --git a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php index 2d404cd4..05edf684 100644 --- a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php +++ b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php @@ -48,6 +48,7 @@ public function getWarningList() { 60 => 1, 61 => 1, 62 => 1, + 70 => 1, ]; } } From d43835c6b7cb00ff3d666be74f9617d64c6918ab Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 20 Jul 2025 06:28:10 +0200 Subject: [PATCH 4/8] Hooks/RestrictedHooks: use PHPCSUtils MessageHelper --- .../Sniffs/Hooks/RestrictedHooksSniff.php | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php index 5b8a8466..70902217 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php @@ -9,6 +9,7 @@ namespace WordPressVIPMinimum\Sniffs\Hooks; +use PHPCSUtils\Utils\MessageHelper; use PHPCSUtils\Utils\PassedParameters; use WordPressCS\WordPress\AbstractFunctionParameterSniff; @@ -44,7 +45,7 @@ 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', @@ -52,7 +53,7 @@ class RestrictedHooksSniff extends AbstractFunctionParameterSniff { ], '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', @@ -61,7 +62,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', @@ -94,8 +95,8 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p foreach ( $this->restricted_hook_groups as $group => $group_args ) { foreach ( $group_args['hooks'] as $hook ) { if ( $normalized_hook_name === $hook ) { - $addMethod = 'add' . $group_args['type']; - $this->phpcsFile->{$addMethod}( $group_args['msg'], $stackPtr, $hook ); + $isError = ( $group_args['type'] === 'error' ); + MessageHelper::addMessage( $this->phpcsFile, $group_args['msg'], $stackPtr, $isError, $hook ); } } } From c1f6aff6d8faa2c04cbb2a6abfd4556a4e2f93f7 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 20 Jul 2025 06:40:32 +0200 Subject: [PATCH 5/8] Hooks/RestrictedHooks: bug fix - hook names are case-sensitive WordPress stores hooknames as provided as an array key and uses `isset()` to determine whether to run an action/filter. It doesn't "normalize" hook names to lowercase, so neither should the sniff. Includes test to safeguard the fix. Refs: * https://github.com/WordPress/wordpress-develop/blob/c726220a21d13fdb5409372b652c9460c59ce1db/src/wp-includes/plugin.php#L124-L126 * https://github.com/WordPress/wordpress-develop/blob/c726220a21d13fdb5409372b652c9460c59ce1db/src/wp-includes/plugin.php#L173-L210 --- WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php | 4 ++-- WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc | 5 ++++- WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php | 1 - 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php index 70902217..7211ac83 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php @@ -131,7 +131,7 @@ private function normalize_hook_name_from_parameter( $parameter ) { $hook_name = $parameter['clean']; } - // Remove quotes (double and single), and use lowercase. - return strtolower( str_replace( [ "'", '"' ], '', $hook_name ) ); + // Remove quotes (double and single). + return str_replace( [ "'", '"' ], '', $hook_name ); } } diff --git a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc index e79810cf..b271db15 100644 --- a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc @@ -47,7 +47,7 @@ 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' , $callback); // 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. @@ -68,3 +68,6 @@ 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. diff --git a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php index 05edf684..95d0129a 100644 --- a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php +++ b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php @@ -37,7 +37,6 @@ public function getWarningList() { 47 => 1, 48 => 1, 49 => 1, - 50 => 1, 51 => 1, 52 => 1, 53 => 1, From 52ee7222082f38e966289204df40c1f916b1ca49 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 20 Jul 2025 07:07:03 +0200 Subject: [PATCH 6/8] Hooks/RestrictedHooks: bug fix - false negatives due to incorrect token walking The `$parameter['start']` key points to the very first token in the parameter, irrespective of what that token is (whitespace, comment, functional). The `$parameter['end']` key points to the very last token in the parameter, irrespective of what that token is (whitespace, comment, functional). This means that when walking the tokens of a parameter both the 'start' token, as well as the 'end' token need to be examined. The code in the `normalize_hook_name_from_parameter()` method handling concatenated hook names did not do this correctly, leading to false negatives. Fixed now. Includes test + updating some pre-existing tests which clearly had a trailing space in the parameter to get the test to pass despite this bug. --- .../Sniffs/Hooks/RestrictedHooksSniff.php | 2 +- .../Tests/Hooks/RestrictedHooksUnitTest.inc | 9 ++++++--- .../Tests/Hooks/RestrictedHooksUnitTest.php | 1 + 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php index 7211ac83..7900ee55 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php @@ -122,7 +122,7 @@ private function normalize_hook_name_from_parameter( $parameter ) { if ( $concat_ptr ) { $hook_name = ''; - for ( $i = $parameter['start'] + 1; $i < $parameter['end']; $i++ ) { + for ( $i = $parameter['start']; $i <= $parameter['end']; $i++ ) { if ( $this->tokens[ $i ]['code'] === T_CONSTANT_ENCAPSED_STRING ) { $hook_name .= str_replace( [ "'", '"' ], '', $this->tokens[ $i ]['content'] ); } diff --git a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc index b271db15..36f6f156 100644 --- a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc @@ -48,10 +48,10 @@ 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. +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( 'upl' . "oad_" . "mimes",'bad_example_function'); // Multiple concatenation with double and single quotes. \add_action( 'http_request_args', function() { // Anonymous callback. // Do stuff. }); @@ -71,3 +71,6 @@ add_filter(priority: 10, hook_name: 'robots_txt', callback: some_function(...) ) // 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. diff --git a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php index 95d0129a..8e1c65dc 100644 --- a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php +++ b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php @@ -48,6 +48,7 @@ public function getWarningList() { 61 => 1, 62 => 1, 70 => 1, + 76 => 1, ]; } } From ef84c2070b46cb885a942081b050a530e0b93614 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 20 Jul 2025 07:19:59 +0200 Subject: [PATCH 7/8] Hooks/RestrictedHooks: bug fix - false positives for dynamic name with concatenation As things were, the code in the `normalize_hook_name_from_parameter()` method would check for concatenation and if found, would gather the contents of all `T_CONSTANT_ENCAPSED_STRING` tokens found in the parameter. However, this presumes all the concatenated parts are text strings and disregards the possibility that variables would be concatenated in, leading to false positives. Fixed now. Includes tests. --- .../Sniffs/Hooks/RestrictedHooksSniff.php | 41 ++++++++++--------- .../Tests/Hooks/RestrictedHooksUnitTest.inc | 4 ++ 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php index 7900ee55..239600cd 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php @@ -9,6 +9,7 @@ namespace WordPressVIPMinimum\Sniffs\Hooks; +use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Utils\MessageHelper; use PHPCSUtils\Utils\PassedParameters; use WordPressCS\WordPress\AbstractFunctionParameterSniff; @@ -91,6 +92,10 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p } $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 ) { @@ -107,31 +112,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']; $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 .= str_replace( [ "'", '"' ], '', $this->tokens[ $i ]['content'] ); } - } else { - $hook_name = $parameter['clean']; } - // Remove quotes (double and single). - return str_replace( [ "'", '"' ], '', $hook_name ); + return $hook_name; } } diff --git a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc index 36f6f156..e90c5625 100644 --- a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc @@ -74,3 +74,7 @@ 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. From 32623e772933f5ed28c880f47e464ab0b6dfc944 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 20 Jul 2025 02:08:09 +0200 Subject: [PATCH 8/8] Hooks/RestrictedHooks: bug fix - quotes within a hook name would be stripped Use PHPCSUtils `TextStrings::stripQuotes()` to only strip surrounding quotes and not remove any quotes potentially present _within_ the text string. Includes tests. --- WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php | 3 ++- WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php b/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php index 239600cd..2ced308b 100644 --- a/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php +++ b/WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php @@ -12,6 +12,7 @@ use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Utils\MessageHelper; use PHPCSUtils\Utils\PassedParameters; +use PHPCSUtils\Utils\TextStrings; use WordPressCS\WordPress\AbstractFunctionParameterSniff; /** @@ -129,7 +130,7 @@ private function normalize_hook_name_from_parameter( $parameter ) { $hook_name = ''; for ( $i = $parameter['start']; $i <= $parameter['end']; $i++ ) { if ( $this->tokens[ $i ]['code'] === T_CONSTANT_ENCAPSED_STRING ) { - $hook_name .= str_replace( [ "'", '"' ], '', $this->tokens[ $i ]['content'] ); + $hook_name .= TextStrings::stripQuotes( $this->tokens[ $i ]['content'] ); } } diff --git a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc index e90c5625..6eab3fdd 100644 --- a/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.inc @@ -78,3 +78,7 @@ 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.