Skip to content

Commit 363ff11

Browse files
GaryJonesGaryJones
authored
Merge pull request #1684 from WordPress-Coding-Standards/feature/837-840-validatedsanitizedinput-allow-for-null-coalesce
ValidatedSanitizedInput: handle null coalesce (equal) correctly
2 parents 146e9a1 + 7c6f0a8 commit 363ff11

File tree

4 files changed

+134
-12
lines changed

4 files changed

+134
-12
lines changed

‎WordPress/Sniff.php‎

Lines changed: 56 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2099,10 +2099,12 @@ protected function is_validated( $stackPtr, $array_keys = array(), $in_condition
20992099

21002100
$bare_array_keys = array_map( array( $this, 'strip_quotes' ), $array_keys );
21012101
$targets = array(
2102-
\T_ISSET => 'construct',
2103-
\T_EMPTY => 'construct',
2104-
\T_UNSET => 'construct',
2105-
\T_STRING => 'function_call',
2102+
\T_ISSET => 'construct',
2103+
\T_EMPTY => 'construct',
2104+
\T_UNSET => 'construct',
2105+
\T_STRING => 'function_call',
2106+
\T_COALESCE => 'coalesce',
2107+
\T_COALESCE_EQUAL => 'coalesce',
21062108
);
21072109

21082110
// phpcs:ignore Generic.CodeAnalysis.JumbledIncrementer.Found -- On purpose, see below.
@@ -2217,6 +2219,40 @@ protected function is_validated( $stackPtr, $array_keys = array(), $in_condition
22172219
}
22182220

22192221
return true;
2222+
2223+
case 'coalesce':
2224+
$prev = $i;
2225+
do {
2226+
$prev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $prev - 1 ), null, true, null, true );
2227+
// Skip over array keys, like $_GET['key']['subkey'].
2228+
if ( \T_CLOSE_SQUARE_BRACKET === $this->tokens[ $prev ]['code'] ) {
2229+
$prev = $this->tokens[ $prev ]['bracket_opener'];
2230+
continue;
2231+
}
2232+
2233+
break;
2234+
} while ( $prev >= ( $scope_start + 1 ) );
2235+
2236+
// We should now have reached the variable.
2237+
if ( \T_VARIABLE !== $this->tokens[ $prev ]['code'] ) {
2238+
continue 2;
2239+
}
2240+
2241+
if ( $this->tokens[ $prev ]['content'] !== $this->tokens[ $stackPtr ]['content'] ) {
2242+
continue 2;
2243+
}
2244+
2245+
if ( ! empty( $bare_array_keys ) ) {
2246+
$found_keys = $this->get_array_access_keys( $prev );
2247+
$found_keys = array_map( array( $this, 'strip_quotes' ), $found_keys );
2248+
$diff = array_diff_assoc( $bare_array_keys, $found_keys );
2249+
if ( ! empty( $diff ) ) {
2250+
continue 2;
2251+
}
2252+
}
2253+
2254+
// Right variable, correct key.
2255+
return true;
22202256
}
22212257
}
22222258

@@ -2231,12 +2267,24 @@ protected function is_validated( $stackPtr, $array_keys = array(), $in_condition
22312267
* Also recognizes `switch ( $var )`.
22322268
*
22332269
* @since 0.5.0
2270+
* @since 2.1.0 Added the $include_coalesce parameter.
22342271
*
2235-
* @param int $stackPtr The index of this token in the stack.
2272+
* @param int $stackPtr The index of this token in the stack.
2273+
* @param bool $include_coalesce Optional. Whether or not to regard the null
2274+
* coalesce operator - ?? - as a comparison operator.
2275+
* Defaults to true.
2276+
* Null coalesce is a special comparison operator in this
2277+
* sense as it doesn't compare a variable to whatever is
2278+
* on the other side of the comparison operator.
22362279
*
22372280
* @return bool Whether this is a comparison.
22382281
*/
2239-
protected function is_comparison( $stackPtr ) {
2282+
protected function is_comparison( $stackPtr, $include_coalesce = true ) {
2283+
2284+
$comparisonTokens = Tokens::$comparisonTokens;
2285+
if ( false === $include_coalesce ) {
2286+
unset( $comparisonTokens[ \T_COALESCE ] );
2287+
}
22402288

22412289
// We first check if this is a switch statement (switch ( $var )).
22422290
if ( isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) {
@@ -2260,7 +2308,7 @@ protected function is_comparison( $stackPtr ) {
22602308
true
22612309
);
22622310

2263-
if ( isset( Tokens::$comparisonTokens[ $this->tokens[ $previous_token ]['code'] ] ) ) {
2311+
if ( isset( $comparisonTokens[ $this->tokens[ $previous_token ]['code'] ] ) ) {
22642312
return true;
22652313
}
22662314

@@ -2283,7 +2331,7 @@ protected function is_comparison( $stackPtr ) {
22832331
);
22842332
}
22852333

2286-
if ( false !== $next_token && isset( Tokens::$comparisonTokens[ $this->tokens[ $next_token ]['code'] ] ) ) {
2334+
if ( false !== $next_token && isset( $comparisonTokens[ $this->tokens[ $next_token ]['code'] ] ) ) {
22872335
return true;
22882336
}
22892337

‎WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php‎

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
namespace WordPressCS\WordPress\Sniffs\Security;
1111

1212
use WordPressCS\WordPress\Sniff;
13+
use PHP_CodeSniffer\Util\Tokens;
1314

1415
/**
1516
* Flag any non-validated/sanitized input ( _GET / _POST / etc. ).
@@ -131,8 +132,37 @@ function ( $symbol ) {
131132

132133
$error_data = array( $this->tokens[ $stackPtr ]['content'] . '[' . implode( '][', $array_keys ) . ']' );
133134

134-
// Check for validation first.
135-
if ( ! $this->is_validated( $stackPtr, $array_keys, $this->check_validation_in_scope_only ) ) {
135+
/*
136+
* Check for validation first.
137+
*/
138+
$validated = false;
139+
140+
for ( $i = ( $stackPtr + 1 ); $i < $this->phpcsFile->numTokens; $i++ ) {
141+
if ( isset( Tokens::$emptyTokens[ $this->tokens[ $i ]['code'] ] ) ) {
142+
continue;
143+
}
144+
145+
if ( \T_OPEN_SQUARE_BRACKET === $this->tokens[ $i ]['code']
146+
&& isset( $this->tokens[ $i ]['bracket_closer'] )
147+
) {
148+
// Skip over array keys.
149+
$i = $this->tokens[ $i ]['bracket_closer'];
150+
continue;
151+
}
152+
153+
if ( \T_COALESCE === $this->tokens[ $i ]['code'] ) {
154+
$validated = true;
155+
}
156+
157+
// Anything else means this is not a validation coalesce.
158+
break;
159+
}
160+
161+
if ( false === $validated ) {
162+
$validated = $this->is_validated( $stackPtr, $array_keys, $this->check_validation_in_scope_only );
163+
}
164+
165+
if ( false === $validated ) {
136166
$this->phpcsFile->addError(
137167
'Detected usage of a possibly undefined superglobal array index: %s. Use isset() or empty() to check the index exists before using it',
138168
$stackPtr,
@@ -151,7 +181,7 @@ function ( $symbol ) {
151181
}
152182

153183
// If this is a comparison ('a' == $_POST['foo']), sanitization isn't needed.
154-
if ( $this->is_comparison( $stackPtr ) ) {
184+
if ( $this->is_comparison( $stackPtr, false ) ) {
155185
return;
156186
}
157187

‎WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc‎

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ some string {$_POST[some_var]} {$_GET['evil']}
161161
EOD
162162
); // Bad x2.
163163

164-
if ( ( $_POST['foo'] ?? 'post' ) === 'post' ) {} // OK.
164+
if ( ( $_POST['foo'] ?? 'post' ) === 'post' ) {} // Bad x2 - unslash, sanitize - more complex compares are not handled.
165165
if ( ( $_POST['foo'] <=> 'post' ) === 0 ) {} // OK.
166166

167167
// Test whitespace independent isset/empty detection.
@@ -289,3 +289,36 @@ function test_recognize_array_comparison_functions_as_such() {
289289
if ( array_keys( $_POST['form_fields'], 'my_form_hidden_field_value', true ) ) {} // OK.
290290
if ( array_keys( $_POST['form_fields'] ) ) {} // Bad x2.
291291
}
292+
293+
/*
294+
* Test recognition of validation via null coalesce, while still checking the var for sanitization.
295+
*/
296+
function test_null_coalesce_1() {
297+
$var = sanitize_text_field( wp_unslash( $_POST['foo'] ?? '' ) ); // OK.
298+
$var = sanitize_text_field( wp_unslash( $_POST['fool'] ?? $_POST['secondary'] ?? '' ) ); // OK.
299+
$var = sanitize_text_field( wp_unslash( $_POST['bar']['sub'] ?? '' ) ); // OK.
300+
$var = sanitize_text_field( $_POST['foobar'] ?? '' ); // Bad x1 - unslash.
301+
}
302+
303+
// The below two sets should give the same errors.
304+
function test_null_coalesce_2() {
305+
$var = $_POST['foo'] ?? ''; // Bad x2 - sanitize + unslash.
306+
$var = $_POST['bar']['sub'] ?? ''; // Bad x2 - sanitize + unslash.
307+
$var = ( $_POST['foobar']['sub'] ?? '' ); // Bad x2 - sanitize + unslash.
308+
309+
$var = isset( $_POST['_foo'] ) ? $_POST['_foo'] : ''; // Bad x2 - sanitize + unslash.
310+
$var = isset( $_POST['_bar']['_sub'] ) ? $_POST['_bar']['_sub'] : ''; // Bad x2 - sanitize + unslash.
311+
$var = ( isset( $_POST['_foobar']['_sub'] ) ? $_POST['_foobar']['_sub'] : '' ); // Bad x2 - sanitize + unslash.
312+
}
313+
314+
function test_null_coalesce_validation() {
315+
$_POST['key'] = $_POST['key'] ?? 'default'; // OK, assignment & Bad x2 - unslash, sanitize.
316+
$key = sanitize_text_field( wp_unslash( $_POST['key'] ) ); // OK, validated via null coalesce.
317+
$another_key = sanitize_text_field( wp_unslash( $_POST['another_key'] ) ); // Bad, not validated, different key.
318+
}
319+
320+
function test_null_coalesce_equals_validation() {
321+
$_POST['key'] ??= 'default'; // OK, assignment.
322+
$key = sanitize_text_field( wp_unslash( $_POST['key'] ) ); // OK, validated via null coalesce equals.
323+
$another_key = sanitize_text_field( wp_unslash( $_POST['another_key'] ) ); // Bad, not validated, different key.
324+
}

‎WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ public function getErrorList() {
5454
138 => 1,
5555
150 => 2,
5656
160 => 2,
57+
164 => 2,
5758
189 => 1,
5859
202 => 1,
5960
206 => 1,
@@ -68,6 +69,16 @@ public function getErrorList() {
6869
266 => 1,
6970
277 => 1,
7071
290 => 2,
72+
300 => 1,
73+
305 => 2,
74+
306 => 2,
75+
307 => 2,
76+
309 => 2,
77+
310 => 2,
78+
311 => 2,
79+
315 => 2,
80+
317 => 1,
81+
323 => 1,
7182
);
7283
}
7384

0 commit comments

Comments
 (0)