Skip to content

Commit 1eb217b

Browse files
GaryJonesGaryJones
authored
Merge pull request #2362 from WordPress/feature/validatedsanitizedinput-use-phpcsutils-and-more-fixes
2 parents 4e8f340 + 5f8fe49 commit 1eb217b

File tree

5 files changed

+198
-44
lines changed

5 files changed

+198
-44
lines changed

‎WordPress/Helpers/ValidationHelper.php‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
use PHP_CodeSniffer\Files\File;
1313
use PHP_CodeSniffer\Util\Tokens;
14+
use PHPCSUtils\Tokens\Collections;
1415
use PHPCSUtils\Utils\Conditions;
1516
use PHPCSUtils\Utils\Context;
1617
use PHPCSUtils\Utils\PassedParameters;
@@ -36,7 +37,6 @@ final class ValidationHelper {
3637
private static $targets = array(
3738
\T_ISSET => 'construct',
3839
\T_EMPTY => 'construct',
39-
\T_UNSET => 'construct',
4040
\T_STRING => 'function_call',
4141
\T_COALESCE => 'coalesce',
4242
\T_COALESCE_EQUAL => 'coalesce',
@@ -156,6 +156,23 @@ public static function is_validated( File $phpcsFile, $stackPtr, $array_keys = a
156156
// phpcs:ignore Generic.CodeAnalysis.JumbledIncrementer.Found -- On purpose, see below.
157157
for ( $i = ( $scope_start + 1 ); $i < $scope_end; $i++ ) {
158158

159+
if ( isset( Collections::closedScopes()[ $tokens[ $i ]['code'] ] )
160+
&& isset( $tokens[ $i ]['scope_closer'] )
161+
) {
162+
// Jump over nested closed scopes as validation done within those does not apply.
163+
$i = $tokens[ $i ]['scope_closer'];
164+
continue;
165+
}
166+
167+
if ( \T_FN === $tokens[ $i ]['code']
168+
&& isset( $tokens[ $i ]['scope_closer'] )
169+
&& $tokens[ $i ]['scope_closer'] < $scope_end
170+
) {
171+
// Jump over nested arrow functions as long as the current variable isn't *in* the arrow function.
172+
$i = $tokens[ $i ]['scope_closer'];
173+
continue;
174+
}
175+
159176
if ( isset( self::$targets[ $tokens[ $i ]['code'] ] ) === false ) {
160177
continue;
161178
}

‎WordPress/Sniff.php‎

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -21,23 +21,6 @@
2121
*/
2222
abstract class Sniff implements PHPCS_Sniff {
2323

24-
/**
25-
* A list of superglobals that incorporate user input.
26-
*
27-
* @since 0.5.0
28-
* @since 0.11.0 Changed from static to non-static.
29-
*
30-
* @var string[]
31-
*/
32-
protected $input_superglobals = array(
33-
'$_COOKIE',
34-
'$_GET',
35-
'$_FILES',
36-
'$_POST',
37-
'$_REQUEST',
38-
'$_SERVER',
39-
);
40-
4124
/**
4225
* The current file being sniffed.
4326
*

‎WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php‎

Lines changed: 54 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@
1111

1212
use PHP_CodeSniffer\Files\File;
1313
use PHP_CodeSniffer\Util\Tokens;
14+
use PHPCSUtils\Utils\Context;
1415
use PHPCSUtils\Utils\TextStrings;
16+
use PHPCSUtils\Utils\Variables;
1517
use WordPressCS\WordPress\Helpers\ContextHelper;
1618
use WordPressCS\WordPress\Helpers\SanitizationHelperTrait;
1719
use WordPressCS\WordPress\Helpers\ValidationHelper;
@@ -43,6 +45,23 @@ class ValidatedSanitizedInputSniff extends Sniff {
4345
*/
4446
public $check_validation_in_scope_only = false;
4547

48+
/**
49+
* Superglobals for which the values will be slashed by WP.
50+
*
51+
* @link https://developer.wordpress.org/reference/functions/wp_magic_quotes/
52+
*
53+
* @since 3.0.0
54+
*
55+
* @var array<string, true>
56+
*/
57+
private $slashed_superglobals = array(
58+
'$_COOKIE' => true,
59+
'$_GET' => true,
60+
'$_POST' => true,
61+
'$_REQUEST' => true,
62+
'$_SERVER' => true,
63+
);
64+
4665
/**
4766
* Returns an array of tokens this test wants to listen for.
4867
*
@@ -65,29 +84,44 @@ public function register() {
6584
*/
6685
public function process_token( $stackPtr ) {
6786

68-
$superglobals = $this->input_superglobals;
69-
7087
// Handling string interpolation.
7188
if ( \T_DOUBLE_QUOTED_STRING === $this->tokens[ $stackPtr ]['code']
7289
|| \T_HEREDOC === $this->tokens[ $stackPtr ]['code']
7390
) {
7491
// Retrieve all embeds, but use only the initial variable name part.
7592
$interpolated_variables = array_map(
76-
function ( $embed ) {
77-
return '$' . preg_replace( '`^(\{?\$\{?\(?)([a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*)(.*)$`', '$2', $embed );
93+
static function ( $embed ) {
94+
return preg_replace( '`^(\{?\$\{?\(?)([a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*)(.*)$`', '$2', $embed );
7895
},
7996
TextStrings::getEmbeds( $this->tokens[ $stackPtr ]['content'] )
8097
);
8198

82-
foreach ( array_intersect( $interpolated_variables, $superglobals ) as $bad_variable ) {
99+
// Filter the embeds down to superglobals only.
100+
$interpolated_superglobals = array_filter(
101+
$interpolated_variables,
102+
static function ( $var_name ) {
103+
return ( 'GLOBALS' !== $var_name && Variables::isSuperglobalName( $var_name ) );
104+
}
105+
);
106+
107+
foreach ( $interpolated_superglobals as $bad_variable ) {
83108
$this->phpcsFile->addError( 'Detected usage of a non-sanitized, non-validated input variable %s: %s', $stackPtr, 'InputNotValidatedNotSanitized', array( $bad_variable, $this->tokens[ $stackPtr ]['content'] ) );
84109
}
85110

86111
return;
87112
}
88113

89-
// Check if this is a superglobal.
90-
if ( ! \in_array( $this->tokens[ $stackPtr ]['content'], $superglobals, true ) ) {
114+
/* Handle variables */
115+
116+
// Check if this is a superglobal we want to examine.
117+
if ( '$GLOBALS' === $this->tokens[ $stackPtr ]['content']
118+
|| Variables::isSuperglobalName( $this->tokens[ $stackPtr ]['content'] ) === false
119+
) {
120+
return;
121+
}
122+
123+
// If the variable is being unset, we don't care about it.
124+
if ( Context::inUnset( $this->phpcsFile, $stackPtr ) ) {
91125
return;
92126
}
93127

@@ -188,13 +222,23 @@ function ( $embed ) {
188222
* @return void
189223
*/
190224
public function add_unslash_error( File $phpcsFile, $stackPtr ) {
191-
$tokens = $phpcsFile->getTokens();
225+
$tokens = $phpcsFile->getTokens();
226+
$var_name = $tokens[ $stackPtr ]['content'];
227+
228+
if ( isset( $this->slashed_superglobals[ $var_name ] ) === false ) {
229+
// WP doesn't slash these, so they don't need unslashing.
230+
return;
231+
}
232+
233+
// We know there will be array keys as that's checked in the process_token() method.
234+
$array_keys = VariableHelper::get_array_access_keys( $phpcsFile, $stackPtr );
235+
$error_data = array( $var_name . '[' . implode( '][', $array_keys ) . ']' );
192236

193237
$phpcsFile->addError(
194-
'%s data not unslashed before sanitization. Use wp_unslash() or similar',
238+
'%s not unslashed before sanitization. Use wp_unslash() or similar',
195239
$stackPtr,
196240
'MissingUnslash',
197-
array( $tokens[ $stackPtr ]['content'] )
241+
$error_data
198242
);
199243
}
200244
}

‎WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.1.inc‎

Lines changed: 112 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ unset( $_GET['test'] ); // Ok.
6464

6565
output( "some string {$_POST['some_var']}" ); // Bad.
6666

67-
echo (int) $_GET['test']; // Ok.
67+
echo isset( $_GET['test'] ) ? (int) $_GET['test'] : 0; // Ok.
6868
some_func( $some_arg, (int) $_GET['test'] ); // Ok.
6969

7070
function zebra() {
@@ -105,7 +105,7 @@ echo sanitize_text_field( $_POST['foo545'] ); // Error for no validation, unslas
105105
echo array_map( 'sanitize_text_field', $_GET['test'] ); // Bad, no unslashing.
106106
echo Array_Map( 'sanitize_key', $_GET['test'] ); // Ok.
107107

108-
foo( AbsINT( $_GET['foo'] ) ); // Ok.
108+
isset( $_GET['foo'] ) && foo( AbsINT( $_GET['foo'] ) ); // Ok.
109109
$ids = array_map( 'absint', $_GET['test'] ); // Ok.
110110

111111
if ( is_array( $_GET['test'] ) ) {} // Ok.
@@ -232,17 +232,17 @@ function test_allow_array_key_exists_alias() {
232232
}
233233
}
234234
function test_correct_multi_level_array_validation() {
235-
if ( isset( $_POST['toplevel']['sublevel'] ) ) {
236-
$id = (int) $_POST['toplevel']; // OK, if a subkey exists, the top level key *must* also exist.
237-
$id = (int) $_POST['toplevel']['sublevel']; // OK.
238-
$id = (int) $_POST['toplevel']['sublevel']['subsub']; // Bad x1 - validate, this sub has not been validated.
235+
if ( isset( $_COOKIE['toplevel']['sublevel'] ) ) {
236+
$id = (int) $_COOKIE['toplevel']; // OK, if a subkey exists, the top level key *must* also exist.
237+
$id = (int) $_COOKIE['toplevel']['sublevel']; // OK.
238+
$id = (int) $_COOKIE['toplevel']['sublevel']['subsub']; // Bad x1 - validate, this sub has not been validated.
239239
}
240240

241-
if ( array_key_exists( 'bar', $_POST['foo'] ) ) {
242-
$id = (int) $_POST['bar']; // Bad x 1 - validate.
243-
$id = (int) $_POST['foo']; // OK.
244-
$id = (int) $_POST['foo']['bar']; // OK.
245-
$id = (int) $_POST['foo']['bar']['baz']; // Bad x 1 - validate.
241+
if ( array_key_exists( 'bar', $_COOKIE['foo'] ) ) {
242+
$id = (int) $_COOKIE['bar']; // Bad x 1 - validate.
243+
$id = (int) $_COOKIE['foo']; // OK.
244+
$id = (int) $_COOKIE['foo']['bar']; // OK.
245+
$id = (int) $_COOKIE['foo']['bar']['baz']; // Bad x 1 - validate.
246246
}
247247
}
248248

@@ -380,11 +380,11 @@ function test_allow_array_key_exists_with_named_params_multilevel_access_wrong_p
380380
}
381381

382382
function test_allow_array_key_exists_with_named_params_multilevel_access_test() {
383-
if ( array_key_exists( array: $_POST['foo'], key: 'bar' ) ) {
384-
$id = (int) $_POST['bar']; // Bad x 1 - validate.
385-
$id = (int) $_POST['foo']; // OK.
386-
$id = (int) $_POST['foo']['bar']; // OK.
387-
$id = (int) $_POST['foo']['bar']['baz']; // Bad x 1 - validate.
383+
if ( array_key_exists( array: $_SERVER['foo'], key: 'bar' ) ) {
384+
$id = (int) $_SERVER['bar']; // Bad x 1 - validate.
385+
$id = (int) $_SERVER['foo']; // OK.
386+
$id = (int) $_SERVER['foo']['bar']; // OK.
387+
$id = (int) $_SERVER['foo']['bar']['baz']; // Bad x 1 - validate.
388388
}
389389
}
390390

@@ -404,3 +404,99 @@ function test_ignore_array_key_exists_as_first_class_callable() {
404404
$callback = array_key_exists(...);
405405
$id = (int) $_POST['foo']; // Bad.
406406
}
407+
408+
/*
409+
* Unsetting a superglobal key is not the same as validating it.
410+
*/
411+
function test_unset_is_not_validation() {
412+
unset( $_REQUEST['key'] );
413+
$id = (int) $_REQUEST['key']; // Bad, missing validation.
414+
}
415+
416+
/*
417+
* Only count a variable as validated if the validation happened in the same scope.
418+
*/
419+
function test_nested_closed_scopes() {
420+
$closure = function() {
421+
return isset( $_POST['bar'] );
422+
};
423+
424+
$anon = new class() {
425+
public function __construct() {
426+
if ( isset( $_POST['bar'] ) ) {
427+
// Do something.
428+
}
429+
}
430+
};
431+
432+
$arrow = fn() => isset( $_POST['bar'] );
433+
434+
$id = (int) $_POST['bar']; // Bad x 1 - validate.
435+
}
436+
437+
function test_arrow_open_scope() {
438+
if ( ! isset( $_SERVER['key'] ) ) {
439+
return;
440+
}
441+
442+
$arrow = fn() => (int) $_SERVER['key']; // OK, validation outside the scope of the arrow function counts.
443+
444+
$arrow = fn() => isset( $_SERVER['abc'] ) ? (int) $_SERVER['abc'] : 0; // OK.
445+
}
446+
447+
// Ensure the sniff flags $_ENV and $_SESSION too.
448+
function test_examine_additional_superglobals_in_textstrings() {
449+
$text = "Use {$_SESSION['key']} for something"; // Bad.
450+
$text = "Use {$_ENV['key']} for something"; // Bad.
451+
$text = "Use {$GLOBALS['key']} for something"; // OK.
452+
}
453+
454+
function test_examine_additional_superglobals_as_vars() {
455+
$key = sanitize_text_field( $_SESSION['key'] ); // Bad - missing validation.
456+
$key = sanitize_text_field( $_ENV['key'] ); // Bad - missing validation.
457+
$key = sanitize_text_field( $_FILES['key'] ); // Bad - missing validation.
458+
459+
if ( isset( $_SESSION['key'], $_ENV['key'], $_FILES['key'] ) === false ) {
460+
return;
461+
}
462+
463+
// OK.
464+
$key = sanitize_text_field( wp_unslash( $_SESSION['key'] ) );
465+
$key = sanitize_text_field( wp_unslash( $_ENV['key'] ) );
466+
$key = sanitize_text_field( wp_unslash( $_FILES['key'] ) );
467+
468+
// OK - unslashing not needed for $_SESSION, $_ENV and $_FILES.
469+
$key = sanitize_text_field( $_SESSION['key'] );
470+
$key = sanitize_text_field( $_ENV['key'] );
471+
$key = sanitize_text_field( $_FILES['key'] );
472+
473+
// Bad - missing sanitization.
474+
do_something( $_SESSION['key'] );
475+
do_something( $_ENV['key'] );
476+
do_something( $_FILES['key'] );
477+
}
478+
479+
function test_null_coalesce_equals_validation_extra_safeguard() {
480+
$_POST['key'] ??= 'default'; // OK, assignment.
481+
$key = $_POST['key']; // Bad, missing unslash + sanitization, validation okay.
482+
}
483+
484+
function test_in_match_condition_is_regarded_as_comparison() {
485+
if ( isset( $_REQUEST['key'] ) ) {
486+
$test = match( $_REQUEST['key'] ) {
487+
'valueA' => 'A',
488+
default => 'B',
489+
};
490+
}
491+
}
492+
493+
function test_in_match_condition_is_regarded_as_comparison() {
494+
if ( isset( $_REQUEST['keyA'], $_REQUEST['keyB'], $_REQUEST['keyC'] ) ) {
495+
$test = match( $toggle ) {
496+
true => sanitize_text_field( wp_unslash( $_REQUEST['keyA'] ) ), // OK.
497+
false => sanitize_text_field( $_REQUEST['keyB'] ), // Bad - missing unslash.
498+
10 => wp_unslash( $_REQUEST['keyC'] ), // Bad - missing sanitization.
499+
default => $_REQUEST['keyD'], // Bad - missing sanitization, unslash, validation.
500+
};
501+
}
502+
}

‎WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,20 @@ public function getErrorList( $testFile = '' ) {
100100
387 => 1,
101101
397 => 1,
102102
405 => 1,
103+
413 => 1,
104+
434 => 1,
105+
449 => 1,
106+
450 => 1,
107+
455 => 1,
108+
456 => 1,
109+
457 => 1,
110+
474 => 1,
111+
475 => 1,
112+
476 => 1,
113+
481 => 2,
114+
497 => 1,
115+
498 => 1,
116+
499 => 3,
103117
);
104118

105119
case 'ValidatedSanitizedInputUnitTest.2.inc':

0 commit comments

Comments
 (0)