Skip to content

Commit 31f6da9

Browse files
GaryJonesGaryJones
authored
Merge pull request #861 from Automattic/feature/security-escapingvoidreturnfunctions-sniff-review
2 parents b6ba453 + e17d08b commit 31f6da9

3 files changed

Lines changed: 228 additions & 24 deletions

File tree

‎WordPressVIPMinimum/Sniffs/Security/EscapingVoidReturnFunctionsSniff.php‎

Lines changed: 107 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,9 @@
1010
namespace WordPressVIPMinimum\Sniffs\Security;
1111

1212
use PHP_CodeSniffer\Util\Tokens;
13+
use PHPCSUtils\Utils\PassedParameters;
14+
use WordPressCS\WordPress\AbstractFunctionParameterSniff;
1315
use WordPressCS\WordPress\Helpers\PrintingFunctionsTrait;
14-
use WordPressVIPMinimum\Sniffs\Sniff;
1516

1617
/**
1718
* Flag functions that don't return anything, yet are wrapped in an escaping function call.
@@ -20,46 +21,131 @@
2021
*
2122
* @uses \WordPressCS\WordPress\Helpers\PrintingFunctionsTrait::$customPrintingFunctions
2223
*/
23-
class EscapingVoidReturnFunctionsSniff extends Sniff {
24+
class EscapingVoidReturnFunctionsSniff extends AbstractFunctionParameterSniff {
2425

2526
use PrintingFunctionsTrait;
2627

2728
/**
28-
* Returns an array of tokens this test wants to listen for.
29+
* The group name for this group of functions.
2930
*
30-
* @return array<int|string>
31+
* @var string
3132
*/
32-
public function register() {
33-
return [
34-
T_STRING,
35-
];
36-
}
33+
protected $group_name = 'escaping_void';
34+
35+
/**
36+
* Functions this sniff is looking for.
37+
*
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.
40+
*/
41+
protected $target_functions = [
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+
],
114+
];
37115

38116
/**
39-
* Process this test when one of its tokens is encountered
117+
* Process the parameters of a matched function.
40118
*
41-
* @param int $stackPtr The position of the current token in the stack passed in $tokens.
119+
* @param int $stackPtr The position of the current token in the stack.
120+
* @param string $group_name The name of the group which was matched.
121+
* @param string $matched_content The token content (function name) which was matched
122+
* in lowercase.
123+
* @param array $parameters Array with information about the parameters.
42124
*
43125
* @return void
44126
*/
45-
public function process_token( $stackPtr ) {
127+
public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
128+
$param_position = $this->target_functions[ $matched_content ]['param_position'];
129+
$param_name = $this->target_functions[ $matched_content ]['param_name'];
46130

47-
if ( strpos( $this->tokens[ $stackPtr ]['content'], 'esc_' ) !== 0 && strpos( $this->tokens[ $stackPtr ]['content'], 'wp_kses' ) !== 0 ) {
48-
// Not what we are looking for.
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.
49134
return;
50135
}
51136

52-
$next_token = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true );
137+
$ignore = Tokens::$emptyTokens;
138+
$ignore[ T_NS_SEPARATOR ] = T_NS_SEPARATOR;
53139

54-
if ( $this->tokens[ $next_token ]['code'] !== T_OPEN_PARENTHESIS ) {
55-
// Not a function call.
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 ) {
142+
// Not what we are looking for.
56143
return;
57144
}
58145

59-
$next_token = $this->phpcsFile->findNext( Tokens::$emptyTokens, $next_token + 1, null, true );
60-
61-
if ( $this->tokens[ $next_token ]['code'] !== T_STRING ) {
62-
// Not what we are looking for.
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 ) {
148+
// Not a function call inside the escaping function.
63149
return;
64150
}
65151

‎WordPressVIPMinimum/Tests/Security/EscapingVoidReturnFunctionsUnitTest.inc‎

Lines changed: 98 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,100 @@
11
<?php
22

3-
esc_html( _e( $something ) ); // NOK.
4-
esc_html( __( $something ) ); // NOK.
3+
/*
4+
* Not the sniff target.
5+
*/
6+
use function esc_attr;
7+
8+
my\ns\esc_html( _e( $something ) );
9+
$this->esc_js( _deprecated_file() );
10+
$this?->esc_textarea( _e( $something ) );
11+
MyClass::esc_url( wp_die(), );
12+
echo WP_KSES;
13+
namespace\wp_kses_post( user_error( $something ) );
14+
15+
function esc_js( _E $param ) {} // Class "_E" as type declaration.
16+
17+
/*
18+
* These should all be okay.
19+
*/
20+
// Incomplete function call, should be ignored by the sniff.
21+
$incorrect_but_ok = esc_html();
22+
$incorrect_but_ok = wp_kses();
23+
24+
// Parameter does not contain a function call.
25+
esc_html_x( $hook_name );
26+
\wp_kses_data( $obj->_deprecated_function() );
27+
esc_attr__( CONSTANT_NAME, );
28+
\ESC_ATTR_X( "do_$something" );
29+
30+
esc_attr(...$params); // PHP 5.6 argument unpacking.
31+
32+
#[Esc_Textarea('text')] // PHP 8.0+ class instantiation via an attribute. Can't contain a nested function call anyway.
33+
function foo() {}
34+
35+
array_walk($text_strings, \esc_html__(...),); // PHP 8.1 first class callable.
36+
37+
// Parameter contains a function call but not calling one of the printing functions.
38+
esc_html( __( $something ) );
39+
Esc_URL_Raw( \get( $url ) );
40+
\esc_xml( deprecated_argument() ); // Note: missing "_" prefix for printing function.
41+
wp_kses_one_attr (
42+
/* comment */
43+
get_attr(),
44+
);
45+
46+
47+
/*
48+
* These should all be flagged.
49+
*/
50+
esc_html( _e( $something ) );
51+
esc_attr__( \_deprecated_argument( $a ) );
52+
ESC_ATTR_E( _Deprecated_Constructor($a), );
53+
\esc_attr_x( _deprecated_file(), );
54+
esc_attr( _deprecated_function() );
55+
esc_HTML__( _deprecated_hook() );
56+
esc_html_e( _Doing_It_Wrong( $a ) );
57+
esc_html_X( \_e( $foo ), );
58+
\esc_html( /*comment*/ _ex( $foo ) );
59+
esc_js( printf( $foo ) );
60+
Esc_textarea( trigger_error( $foo ) );
61+
\esc_URL_raw( \user_Error( $foo, '' ), );
62+
esc_url( vprintf( $foo, ) /*comment*/ );
63+
WP_Kses_Data( WP_DIE( $foo ), );
64+
wp_kses_one_attr /*comment*/ ( wp_dropdown_pages( $pages ) );
65+
wp_kses_post(
66+
\_deprecated_function( $fn )
67+
);
68+
wp_kses( _ex() );
69+
70+
// Adding custom printing functions is supported.
71+
// phpcs:set WordPressVIPMinimum.Security.EscapingVoidReturnFunctions customPrintingFunctions[] to_screen,my_print
72+
esc_attr( to_screen( $var1 ) );
73+
\wp_kses_post( my_print() );
74+
// phpcs:set WordPressVIPMinimum.Security.EscapingVoidReturnFunctions customPrintingFunctions[]
75+
76+
tag_escape( $tag ); // OK.
77+
tag_escape( _e() ); // Bad.
78+
79+
// Bug: these are not function calls inside.
80+
esc_attr__( User_Error::CONSTANT_NAME, ); // OK.
81+
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: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,29 @@ class EscapingVoidReturnFunctionsUnitTest extends AbstractSniffUnitTest {
2323
*/
2424
public function getErrorList() {
2525
return [
26-
3 => 1,
26+
50 => 1,
27+
51 => 1,
28+
52 => 1,
29+
53 => 1,
30+
54 => 1,
31+
55 => 1,
32+
56 => 1,
33+
57 => 1,
34+
58 => 1,
35+
59 => 1,
36+
60 => 1,
37+
61 => 1,
38+
62 => 1,
39+
63 => 1,
40+
64 => 1,
41+
65 => 1,
42+
68 => 1,
43+
72 => 1,
44+
73 => 1,
45+
77 => 1,
46+
90 => 1,
47+
91 => 1,
48+
92 => 1,
2749
];
2850
}
2951

0 commit comments

Comments
 (0)