Skip to content

Commit e17d08b

Browse files
jrfnljrfnl
committed
Security/EscapingVoidReturnFunctions: add support for PHP 8.0+ named parameters
To allow support for named parameters, we need to know the position and name of the parameter to examine within a function call. Previously, this sniff used "wildcard" matching for `esc_*` and `wp_kses*` functions. While the parameter position is the same for all of these, the parameter name is not, so we can no longer use wildcard matching if we want the sniff to support PHP 8.0+ function calls using named parameters. To this end: 1. Change the `$target_functions` property to be explicit about the functions the sniff is targetting. Notes: - I've included all WP native `esc_*` functions with the exception of `esc_sql()` which doesn't feel like it belongs in this list. Please let me know if you prefer that `esc_sql()` is still included. - I've included all WP native `wp_kses_*` functions with the exception of `wp_kses_allowed_html()` which is not an escaping function. - Also take note that this also means that custom/user defined `esc_*`/`wp_kses*` functions wrapping printing functions will no longer be flagged. 2. Changed the `$target_functions` property to contain information about the target parameter name and position. 3. Adjusted the logic in the sniff to allow for named parameters using the new PHPCSUtils 1.0.0-alpha4 `PassedParameters::getParameterFromStack()` method. Includes additional unit tests.
1 parent ab01e6f commit e17d08b

File tree

3 files changed

+107
-11
lines changed

3 files changed

+107
-11
lines changed

‎WordPressVIPMinimum/Sniffs/Security/EscapingVoidReturnFunctionsSniff.php‎

Lines changed: 85 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
namespace WordPressVIPMinimum\Sniffs\Security;
1111

1212
use PHP_CodeSniffer\Util\Tokens;
13+
use PHPCSUtils\Utils\PassedParameters;
1314
use WordPressCS\WordPress\AbstractFunctionParameterSniff;
1415
use WordPressCS\WordPress\Helpers\PrintingFunctionsTrait;
1516

@@ -34,12 +35,82 @@ class EscapingVoidReturnFunctionsSniff extends AbstractFunctionParameterSniff {
3435
/**
3536
* Functions this sniff is looking for.
3637
*
37-
* @var array<string, true> Keys are target functions, value irrelevant.
38+
* @var array<string, array{param_position: int, param_name: string}> Keys are the target functions,
39+
* value, the name and position of the target parameter.
3840
*/
3941
protected $target_functions = [
40-
'esc_*' => true,
41-
'tag_escape' => true,
42-
'wp_kses*' => true,
42+
'esc_attr' => [
43+
'param_position' => 1,
44+
'param_name' => 'text',
45+
],
46+
'esc_attr__' => [
47+
'param_position' => 1,
48+
'param_name' => 'text',
49+
],
50+
'esc_attr_e' => [
51+
'param_position' => 1,
52+
'param_name' => 'text',
53+
],
54+
'esc_attr_x' => [
55+
'param_position' => 1,
56+
'param_name' => 'text',
57+
],
58+
'esc_html' => [
59+
'param_position' => 1,
60+
'param_name' => 'text',
61+
],
62+
'esc_html__' => [
63+
'param_position' => 1,
64+
'param_name' => 'text',
65+
],
66+
'esc_html_e' => [
67+
'param_position' => 1,
68+
'param_name' => 'text',
69+
],
70+
'esc_html_x' => [
71+
'param_position' => 1,
72+
'param_name' => 'text',
73+
],
74+
'esc_js' => [
75+
'param_position' => 1,
76+
'param_name' => 'text',
77+
],
78+
'esc_textarea' => [
79+
'param_position' => 1,
80+
'param_name' => 'text',
81+
],
82+
'esc_url' => [
83+
'param_position' => 1,
84+
'param_name' => 'url',
85+
],
86+
'esc_url_raw' => [
87+
'param_position' => 1,
88+
'param_name' => 'url',
89+
],
90+
'esc_xml' => [
91+
'param_position' => 1,
92+
'param_name' => 'text',
93+
],
94+
'tag_escape' => [
95+
'param_position' => 1,
96+
'param_name' => 'tag_name',
97+
],
98+
'wp_kses' => [
99+
'param_position' => 1,
100+
'param_name' => 'content',
101+
],
102+
'wp_kses_data' => [
103+
'param_position' => 1,
104+
'param_name' => 'data',
105+
],
106+
'wp_kses_one_attr' => [
107+
'param_position' => 1,
108+
'param_name' => 'attr',
109+
],
110+
'wp_kses_post' => [
111+
'param_position' => 1,
112+
'param_name' => 'data',
113+
],
43114
];
44115

45116
/**
@@ -54,23 +125,26 @@ class EscapingVoidReturnFunctionsSniff extends AbstractFunctionParameterSniff {
54125
* @return void
55126
*/
56127
public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
57-
$next_token = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true );
58-
if ( $this->tokens[ $next_token ]['code'] !== T_OPEN_PARENTHESIS ) {
59-
// Not a function call.
128+
$param_position = $this->target_functions[ $matched_content ]['param_position'];
129+
$param_name = $this->target_functions[ $matched_content ]['param_name'];
130+
131+
$target_param = PassedParameters::getParameterFromStack( $parameters, $param_position, $param_name );
132+
if ( $target_param === false ) {
133+
// Missing (required) target parameter. Probably live coding, nothing to examine (yet). Bow out.
60134
return;
61135
}
62136

63137
$ignore = Tokens::$emptyTokens;
64138
$ignore[ T_NS_SEPARATOR ] = T_NS_SEPARATOR;
65139

66-
$next_token = $this->phpcsFile->findNext( $ignore, $next_token + 1, null, true );
67-
if ( $this->tokens[ $next_token ]['code'] !== T_STRING ) {
140+
$next_token = $this->phpcsFile->findNext( $ignore, $target_param['start'], ( $target_param['end'] + 1 ), true );
141+
if ( $next_token === false || $this->tokens[ $next_token ]['code'] !== T_STRING ) {
68142
// Not what we are looking for.
69143
return;
70144
}
71145

72-
$next_after = $this->phpcsFile->findNext(Tokens::$emptyTokens, $next_token + 1, null, true );
73-
if ( $this->tokens[ $next_after ]['code'] !== T_OPEN_PARENTHESIS ) {
146+
$next_after = $this->phpcsFile->findNext( Tokens::$emptyTokens, $next_token + 1, ( $target_param['end'] + 1 ), true );
147+
if ( $next_after === false || $this->tokens[ $next_after ]['code'] !== T_OPEN_PARENTHESIS ) {
74148
// Not a function call inside the escaping function.
75149
return;
76150
}

‎WordPressVIPMinimum/Tests/Security/EscapingVoidReturnFunctionsUnitTest.inc‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,3 +79,22 @@ tag_escape( _e() ); // Bad.
7979
// Bug: these are not function calls inside.
8080
esc_attr__( User_Error::CONSTANT_NAME, ); // OK.
8181
esc_js( _doing_it_wrong::class ); // OK, PHP 5.5 ::class resolution.
82+
83+
// Safeguard handling of function calls using PHP 8.0+ named parameters.
84+
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.
85+
esc_url_raw(protocols: $protocols, url: $url); // OK.
86+
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.
87+
wp_kses( content: \not_deprecated_function( $fn ) ); // OK.
88+
wp_kses( content: /*comment*/ ); // OK, well, not really, invalid function call, but that's not the concern of the sniff.
89+
90+
esc_html_x(context: $c, text: _doing_it_wrong() ); // Bad.
91+
wp_kses(allowed_html: $allowed_html, content: printf() ); // Bad.
92+
esc_url(protocols: $protocols, url: vprintf()); // Bad.
93+
94+
// These are no longer flagged as they are not "escaping" functions for the purpose of this sniff.
95+
esc_sql( trigger_error( $foo ) ); // OK.
96+
wp_kses_allowed_html( _deprecated_function() ); // OK.
97+
98+
// These are no longer flagged as these are not the WP native escaping functions, so we don't know what parameter to check.
99+
esc_something( trigger_error( $foo ) ); // OK.
100+
wp_kses_page( _deprecated_function() ); // OK.

‎WordPressVIPMinimum/Tests/Security/EscapingVoidReturnFunctionsUnitTest.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,9 @@ public function getErrorList() {
4343
72 => 1,
4444
73 => 1,
4545
77 => 1,
46+
90 => 1,
47+
91 => 1,
48+
92 => 1,
4649
];
4750
}
4851

0 commit comments

Comments
 (0)