Skip to content

Commit a30da72

Browse files
authored
Merge pull request #852 from Automattic/feature/hooks-restrictedhooks-sniff-review
2 parents 025a1e0 + 32623e7 commit a30da72

File tree

3 files changed

+129
-56
lines changed

3 files changed

+129
-56
lines changed

WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php

Lines changed: 38 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@
99

1010
namespace WordPressVIPMinimum\Sniffs\Hooks;
1111

12+
use PHP_CodeSniffer\Util\Tokens;
13+
use PHPCSUtils\Utils\MessageHelper;
14+
use PHPCSUtils\Utils\PassedParameters;
15+
use PHPCSUtils\Utils\TextStrings;
1216
use WordPressCS\WordPress\AbstractFunctionParameterSniff;
1317

1418
/**
@@ -43,15 +47,15 @@ class RestrictedHooksSniff extends AbstractFunctionParameterSniff {
4347
private $restricted_hook_groups = [
4448
'upload_mimes' => [
4549
// TODO: This error message needs a link to the VIP Documentation, see https://github.com/Automattic/VIP-Coding-Standards/issues/235.
46-
'type' => 'Warning',
50+
'type' => 'warning',
4751
'msg' => 'Please ensure that the mimes being filtered do not include insecure types (i.e. SVG, SWF, etc.). Manual inspection required.',
4852
'hooks' => [
4953
'upload_mimes',
5054
],
5155
],
5256
'http_request' => [
5357
// https://docs.wpvip.com/technical-references/code-quality-and-best-practices/retrieving-remote-data/.
54-
'type' => 'Warning',
58+
'type' => 'warning',
5559
'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.',
5660
'hooks' => [
5761
'http_request_timeout',
@@ -60,7 +64,7 @@ class RestrictedHooksSniff extends AbstractFunctionParameterSniff {
6064
],
6165
'robotstxt' => [
6266
// https://docs.wpvip.com/how-tos/modify-the-robots-txt-file/.
63-
'type' => 'Warning',
67+
'type' => 'warning',
6468
'msg' => 'Don\'t forget to flush the robots.txt cache by going to Settings > Reading and toggling the privacy settings.',
6569
'hooks' => [
6670
'do_robotstxt',
@@ -82,11 +86,23 @@ class RestrictedHooksSniff extends AbstractFunctionParameterSniff {
8286
* normal file processing.
8387
*/
8488
public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
89+
$hook_name_param = PassedParameters::getParameterFromStack( $parameters, 1, 'hook_name' );
90+
if ( $hook_name_param === false ) {
91+
// Missing required parameter. Nothing to examine. Bow out.
92+
return;
93+
}
94+
95+
$normalized_hook_name = $this->normalize_hook_name_from_parameter( $hook_name_param );
96+
if ( $normalized_hook_name === '' ) {
97+
// Dynamic hook name. Cannot reliably determine if it's one of the targets. Bow out.
98+
return;
99+
}
100+
85101
foreach ( $this->restricted_hook_groups as $group => $group_args ) {
86102
foreach ( $group_args['hooks'] as $hook ) {
87-
if ( $this->normalize_hook_name_from_parameter( $parameters[1] ) === $hook ) {
88-
$addMethod = 'add' . $group_args['type'];
89-
$this->phpcsFile->{$addMethod}( $group_args['msg'], $stackPtr, $hook );
103+
if ( $normalized_hook_name === $hook ) {
104+
$isError = ( $group_args['type'] === 'error' );
105+
MessageHelper::addMessage( $this->phpcsFile, $group_args['msg'], $stackPtr, $isError, $hook );
90106
}
91107
}
92108
}
@@ -97,31 +113,27 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p
97113
*
98114
* @param array $parameter Array with information about a parameter.
99115
*
100-
* @return string Normalized hook name.
116+
* @return string Normalized hook name or an empty string if the hook name could not be determined.
101117
*/
102118
private function normalize_hook_name_from_parameter( $parameter ) {
103-
// If concatenation is found, build hook name.
104-
$concat_ptr = $this->phpcsFile->findNext(
105-
T_STRING_CONCAT,
106-
$parameter['start'],
107-
$parameter['end'],
108-
false,
109-
null,
110-
true
111-
);
119+
$allowed_tokens = Tokens::$emptyTokens;
120+
$allowed_tokens += [
121+
T_STRING_CONCAT => T_STRING_CONCAT,
122+
T_CONSTANT_ENCAPSED_STRING => T_CONSTANT_ENCAPSED_STRING,
123+
];
112124

113-
if ( $concat_ptr ) {
114-
$hook_name = '';
115-
for ( $i = $parameter['start'] + 1; $i < $parameter['end']; $i++ ) {
116-
if ( $this->tokens[ $i ]['code'] === T_CONSTANT_ENCAPSED_STRING ) {
117-
$hook_name .= str_replace( [ "'", '"' ], '', $this->tokens[ $i ]['content'] );
118-
}
125+
$has_disallowed_token = $this->phpcsFile->findNext( $allowed_tokens, $parameter['start'], ( $parameter['end'] + 1 ), true );
126+
if ( $has_disallowed_token !== false ) {
127+
return '';
128+
}
129+
130+
$hook_name = '';
131+
for ( $i = $parameter['start']; $i <= $parameter['end']; $i++ ) {
132+
if ( $this->tokens[ $i ]['code'] === T_CONSTANT_ENCAPSED_STRING ) {
133+
$hook_name .= TextStrings::stripQuotes( $this->tokens[ $i ]['content'] );
119134
}
120-
} else {
121-
$hook_name = $parameter['raw'];
122135
}
123136

124-
// Remove quotes (double and single), and use lowercase.
125-
return strtolower( str_replace( [ "'", '"' ], '', $hook_name ) );
137+
return $hook_name;
126138
}
127139
}
Lines changed: 75 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,84 @@
11
<?php
22

3-
add_filter( 'upload_mime', 'good_example_function' ); // Ok.
4-
add_filter( 'upload_mimesX', 'good_example_function' ); // Ok.
5-
6-
// Warnings.
7-
add_filter( 'upload_mimes', 'bad_example_function' ); // Simple string.
8-
add_filter('upload_mimes' ,'bad_example_function'); // Incorrect spacing.
9-
add_filter( 'upload_mimes','bad_example_function'); // Incorrect spacing.
10-
add_filter( "upload_mimes" ,'bad_example_function'); // Double quotes.
11-
add_filter( 'upLoad_mimeS' ,'bad_example_function'); // Uppercase characters.
12-
add_filter( 'upload_' . 'mimes' ,'bad_example_function'); // Single concatenation.
13-
add_filter( 'upl' . 'oad_' . 'mimes' ,'bad_example_function'); // Multiple concatenation.
14-
add_filter( "upload_" . 'mimes' ,'bad_example_function'); // Single concatenation with double and single quotes.
15-
add_filter( 'upl' . "oad_" . "mimes" ,'bad_example_function'); // Multiple concatenation with double and single quotes.
16-
add_filter( 'upload_mimes', function() { // Anonymous callback.
3+
/*
4+
* Not the sniff target.
5+
*/
6+
use add_filter;
7+
8+
my\ns\add_filter($a, $b);
9+
$this->add_action($a, $b);
10+
$this?->add_filter($a, $b);
11+
MyClass::add_action($a, $b);
12+
echo ADD_FILTER;
13+
namespace\add_action($a, $b);
14+
15+
16+
/*
17+
* These should all be okay.
18+
*/
19+
add_filter( 'not_target_hook', 'good_example_function' );
20+
\add_filter( 'upload_mimesX' , 'good_example_function' );
21+
22+
add_action(...$params); // PHP 5.6 argument unpacking.
23+
24+
// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute.
25+
#[ADD_FILTER('text')]
26+
function foo() {}
27+
28+
// PHP 8.1 first class callable.
29+
// As we have no knowledge about what parameters will be passed, we shouldn't flag this.
30+
array_walk($filters, add_filter(...));
31+
32+
// Ignore as undetermined.
33+
Add_Filter( $hook_name, 'undetermined' );
34+
\add_action( $obj->get_filterName(), 'undetermined' );
35+
add_filter( MyClass::FILTER_NAME, 'undetermined', );
36+
\add_filter( "upload_$mimes", 'undetermined' );
37+
38+
// Incomplete function call, should be ignored by the sniff.
39+
$incorrect_but_ok = add_filter();
40+
$incorrect_but_ok = add_action();
41+
42+
43+
/*
44+
* These should all be flagged with a warning.
45+
*/
46+
add_filter( 'do_robotstxt', 'bad_example_function' ); // Simple string.
47+
add_action('upload_mimes' , [$obj, 'method']); // Incorrect spacing.
48+
add_filter( 'robots_txt','bad_example_function'); // Incorrect spacing.
49+
\add_filter( "http_request_timeout" , fn($param) => $param * 10); // Double quotes.
50+
51+
ADD_FILTER( 'upload_' . 'mimes','bad_example_function'); // Single concatenation.
52+
add_filter( 'upl' . 'oad_' . 'mimes','bad_example_function'); // Multiple concatenation.
53+
add_filter( "upload_" . 'mimes' , bad_example_function(...)); // Single concatenation with double and single quotes.
54+
add_filter( 'upl' . "oad_" . "mimes",'bad_example_function'); // Multiple concatenation with double and single quotes.
55+
\add_action( 'http_request_args', function() { // Anonymous callback.
1756
// Do stuff.
1857
});
1958
add_action( 'upload_mimes', 'bad_example_function' ); // Check `add_action()`, which is an alias for `add_filter()`.
2059
add_filter( 'http_request_timeout', 'bad_example_function' ); // Simple string.
2160
add_filter('http_request_args', 'bad_example_function' ); // Simple string + incorrect spacing.
22-
add_action( 'do_robotstxt', 'my_do_robotstxt'); // Simple string.
61+
add_action( /*comment*/ 'do_robotstxt', 'my_do_robotstxt'); // Simple string.
2362
add_filter( 'robots_txt', function() { // Anonymous callback.
2463
} );
64+
65+
// Safeguard correct handling of function calls using PHP 8.0+ named parameters.
66+
add_action(callback: 'invalid', priority: 10); // OK, well, not really, missing required $hook_name param, but that's not the concern of this sniff.
67+
add_action(callback: 'do_robotstxt', hook_name: 'not_our_target'); // OK.
68+
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.
69+
70+
add_filter(priority: 10, hook_name: 'robots_txt', callback: some_function(...) ); // Warning.
71+
72+
// Hook names are case-sensitive.
73+
add_filter( 'upLoad_mimeS' , $callback); // OK, not our target.
74+
75+
// Bug fix - spacing vs concatenation.
76+
add_filter('do_' . 'robots' . 'txt', 'bad_example_function'); // Warning.
77+
78+
// Ignore partially dynamic hook names.
79+
add_filter( 'robots_' . $something . 'txt' , $callback); // OK, ignored as undetermined.
80+
add_filter( 'http_request_timeout' . $something, $callback); // OK, ignored as undetermined.
81+
82+
// Ensure quote stripping is done correctly.
83+
add_filter( 'upload"_mimes', 'bad_example_function' ); // OK, not a filter we're looking for.
84+
add_filter( "upload_'mimes", 'bad_example_function' ); // OK, not a filter we're looking for.

WordPressVIPMinimum/Tests/Hooks/RestrictedHooksUnitTest.php

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -33,21 +33,22 @@ public function getErrorList() {
3333
*/
3434
public function getWarningList() {
3535
return [
36-
7 => 1,
37-
8 => 1,
38-
9 => 1,
39-
10 => 1,
40-
11 => 1,
41-
12 => 1,
42-
13 => 1,
43-
14 => 1,
44-
15 => 1,
45-
16 => 1,
46-
19 => 1,
47-
20 => 1,
48-
21 => 1,
49-
22 => 1,
50-
23 => 1,
36+
46 => 1,
37+
47 => 1,
38+
48 => 1,
39+
49 => 1,
40+
51 => 1,
41+
52 => 1,
42+
53 => 1,
43+
54 => 1,
44+
55 => 1,
45+
58 => 1,
46+
59 => 1,
47+
60 => 1,
48+
61 => 1,
49+
62 => 1,
50+
70 => 1,
51+
76 => 1,
5152
];
5253
}
5354
}

0 commit comments

Comments
 (0)