From 8581c40044265539c6835be25ec0a2f4b1b6a9f2 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 18 Jul 2025 01:36:19 +0200 Subject: [PATCH 1/7] Security/PHPFilterFunctions: improve the tests * Test against false positives for calls to methods or namespaced function calls. * Test against false positives for attribute class using the same name as the function. * Ensure function import `use` statements are not flagged. We're not interested in those. * Document that if the filter is passed in dynamically (via a variable or function call), the sniff will stay silent. * Add some more variations to the pre-existing tests: - Non-lowercase function call(s). - Fully qualified function calls. - Use PHP 7.3+ trailing comma's in a few function calls. --- .../Security/PHPFilterFunctionsUnitTest.inc | 61 ++++++++++++++----- .../Security/PHPFilterFunctionsUnitTest.php | 24 ++++---- 2 files changed, 57 insertions(+), 28 deletions(-) diff --git a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc index 9960f4ed..58195afe 100644 --- a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc @@ -1,29 +1,58 @@ filter_var_array($a, $b); +$this?->filter_input_array($a, $b); +MyClass::filter_var($a, $b); +echo FILTER_INPUT; +namespace\filter_var_array($a, $b); + +// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute. +#[Filter_Input('text')] +function foo() {} + +// PHP 8.1 first class callable. +// As we have no knowledge about what parameters will be passed, we shouldn't flag this. +add_filter('my_filter', filter_var(...)); + + +/* + * These should all be okay. + */ filter_var( $url, FILTER_SANITIZE_URL ); -filter_var( 'test', FILTER_SANITIZE_STRING ); -filter_var( "test", FILTER_SANITIZE_STRING ); -filter_input( INPUT_GET, 'foo', FILTER_SANITIZE_STRING ); +\filter_var( 'test', FILTER_SANITIZE_STRING ); +FILTER_INPUT( INPUT_GET, 'foo', FILTER_SANITIZE_STRING, ); filter_input( INPUT_GET, "foo" , FILTER_SANITIZE_STRING ); -filter_var_array( $array, FILTER_SANITIZE_STRING ); filter_input_array( $array, FILTER_SANITIZE_STRING ); -filter_input_array( $array,FILTER_SANITIZE_STRING ); -// Bad. +// Ignore as undetermined. +filter_var( "test", get_filter() ); +\Filter_Var_Array( $array, $filterName ); +filter_input_array( $array,$obj->get_filter() , ); + +// Incomplete function call, should be ignored by the sniff. +$incorrect_but_ok = filter_input(); + +/* + * These should all be flagged with a warning. + */ filter_input( INPUT_GET, 'foo' ); // Missing third parameter. -filter_input( INPUT_GET, 'foo', FILTER_DEFAULT ); // This filter ID does nothing. -filter_input( INPUT_GET, "foo", FILTER_UNSAFE_RAW ); // This filter ID does nothing. +\filter_input( INPUT_GET, 'foo', FILTER_DEFAULT ); // This filter ID does nothing. +filter_input( INPUT_GET, "foo", FILTER_UNSAFE_RAW ,); // This filter ID does nothing. + filter_var( $url ); // Missing second parameter. -filter_var( $url, FILTER_DEFAULT ); // This filter ID does nothing. +Filter_Var( $url, FILTER_DEFAULT ); // This filter ID does nothing. filter_var( 'https://google.com', FILTER_UNSAFE_RAW ); // This filter ID does nothing. -filter_var_array( $array ); // Missing second parameter. + +filter_var_array( $array, ); // Missing second parameter. filter_var_array( $array, FILTER_DEFAULT ); // This filter ID does nothing. filter_var_array( $array, FILTER_UNSAFE_RAW ); // This filter ID does nothing. + filter_input_array( $array ); // Missing second parameter. -filter_input_array( $array, FILTER_DEFAULT ); // This filter ID does nothing. -filter_input_array( $array, FILTER_UNSAFE_RAW ); // This filter ID does nothing. +\FILTER_INPUT_ARRAY( $array, FILTER_DEFAULT ); // This filter ID does nothing. +filter_input_array( $array, FILTER_UNSAFE_RAW, ); // This filter ID does nothing. diff --git a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php index dae998cc..201f4a8d 100644 --- a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php +++ b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php @@ -32,18 +32,18 @@ public function getErrorList() { */ public function getWarningList() { return [ - 18 => 1, - 19 => 1, - 20 => 1, - 21 => 1, - 22 => 1, - 23 => 1, - 24 => 1, - 25 => 1, - 26 => 1, - 27 => 1, - 28 => 1, - 29 => 1, + 44 => 1, + 45 => 1, + 46 => 1, + 48 => 1, + 49 => 1, + 50 => 1, + 52 => 1, + 53 => 1, + 54 => 1, + 56 => 1, + 57 => 1, + 58 => 1, ]; } } From dafc562563bd348d742384cef35419177ab9cced Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 18 Jul 2025 02:51:12 +0200 Subject: [PATCH 2/7] Security/PHPFilterFunctions: remove redundant `strtoupper()` calls The `isset( $this->restricted_filters[ $parameters[3]['raw'] ] )` check on the array is case-sensitive, so if the filter constant name was not in upper case already, it wouldn't be matched anyhow. Also note: the case-sensitivity is correct as constants in PHP are case-sensitive (unless explicitly declared as case-insensitive, which these aren't: https://3v4l.org/vq53U#veol). Includes updating one of the pre-existing tests to document this. --- .../Sniffs/Security/PHPFilterFunctionsSniff.php | 4 ++-- .../Tests/Security/PHPFilterFunctionsUnitTest.inc | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php b/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php index bc4002ac..c4fc0678 100644 --- a/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php @@ -69,7 +69,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p if ( isset( $parameters[3], $this->restricted_filters[ $parameters[3]['raw'] ] ) ) { $message = 'Please use an appropriate filter to sanitize, as "%s" does no filtering, see: http://php.net/manual/en/filter.filters.sanitize.php.'; - $data = [ strtoupper( $parameters[3]['raw'] ) ]; + $data = [ $parameters[3]['raw'] ]; $this->phpcsFile->addWarning( $message, $stackPtr, 'RestrictedFilter', $data ); } } else { @@ -81,7 +81,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p if ( isset( $parameters[2], $this->restricted_filters[ $parameters[2]['raw'] ] ) ) { $message = 'Please use an appropriate filter to sanitize, as "%s" does no filtering, see http://php.net/manual/en/filter.filters.sanitize.php.'; - $data = [ strtoupper( $parameters[2]['raw'] ) ]; + $data = [ $parameters[2]['raw'] ]; $this->phpcsFile->addWarning( $message, $stackPtr, 'RestrictedFilter', $data ); } } diff --git a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc index 58195afe..6493e791 100644 --- a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc @@ -28,7 +28,7 @@ filter_var( $url, FILTER_SANITIZE_URL ); \filter_var( 'test', FILTER_SANITIZE_STRING ); FILTER_INPUT( INPUT_GET, 'foo', FILTER_SANITIZE_STRING, ); filter_input( INPUT_GET, "foo" , FILTER_SANITIZE_STRING ); -filter_input_array( $array, FILTER_SANITIZE_STRING ); +filter_input_array( $array, filter_default ); // Constants are case-sensitive, so this is not the FILTER_DEFAULT constant. // Ignore as undetermined. filter_var( "test", get_filter() ); From e953ac727c77377f2bb0aabe1c2adf8aa8704d96 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 18 Jul 2025 02:54:08 +0200 Subject: [PATCH 3/7] Security/PHPFilterFunctions: prevent some false positives The `'raw'` key in the parameter arrays returned from the `PassedParameters` class contains - as per the name - the _raw_ contents of the parameter. Since PHPCSUtils 1.0.0-alpha4, the return array also contain a `'clean'` index, which contains the contents of the parameter cleaned of comments. By switching to using that key, some false positives get prevented. Includes unit tests demonstrating the issue and safeguarding the fix. --- .../Sniffs/Security/PHPFilterFunctionsSniff.php | 8 ++++---- .../Tests/Security/PHPFilterFunctionsUnitTest.inc | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php b/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php index c4fc0678..0ed98497 100644 --- a/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php @@ -67,9 +67,9 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $this->phpcsFile->addWarning( $message, $stackPtr, 'MissingThirdParameter', $data ); } - if ( isset( $parameters[3], $this->restricted_filters[ $parameters[3]['raw'] ] ) ) { + if ( isset( $parameters[3], $this->restricted_filters[ $parameters[3]['clean'] ] ) ) { $message = 'Please use an appropriate filter to sanitize, as "%s" does no filtering, see: http://php.net/manual/en/filter.filters.sanitize.php.'; - $data = [ $parameters[3]['raw'] ]; + $data = [ $parameters[3]['clean'] ]; $this->phpcsFile->addWarning( $message, $stackPtr, 'RestrictedFilter', $data ); } } else { @@ -79,9 +79,9 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $this->phpcsFile->addWarning( $message, $stackPtr, 'MissingSecondParameter', $data ); } - if ( isset( $parameters[2], $this->restricted_filters[ $parameters[2]['raw'] ] ) ) { + if ( isset( $parameters[2], $this->restricted_filters[ $parameters[2]['clean'] ] ) ) { $message = 'Please use an appropriate filter to sanitize, as "%s" does no filtering, see http://php.net/manual/en/filter.filters.sanitize.php.'; - $data = [ $parameters[2]['raw'] ]; + $data = [ $parameters[2]['clean'] ]; $this->phpcsFile->addWarning( $message, $stackPtr, 'RestrictedFilter', $data ); } } diff --git a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc index 6493e791..9b422978 100644 --- a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc @@ -43,7 +43,7 @@ $incorrect_but_ok = filter_input(); */ filter_input( INPUT_GET, 'foo' ); // Missing third parameter. \filter_input( INPUT_GET, 'foo', FILTER_DEFAULT ); // This filter ID does nothing. -filter_input( INPUT_GET, "foo", FILTER_UNSAFE_RAW ,); // This filter ID does nothing. +filter_input( INPUT_GET, "foo", FILTER_UNSAFE_RAW /* comment */ ,); // This filter ID does nothing. filter_var( $url ); // Missing second parameter. Filter_Var( $url, FILTER_DEFAULT ); // This filter ID does nothing. From e10dd27c43357947222c055cfaa99ecb4ca5ed24 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 18 Jul 2025 03:12:03 +0200 Subject: [PATCH 4/7] Security/PHPFilterFunctions: add support for PHP 8.0+ named parameters 1. Changed the `$target_functions` property to contain information about the target parameter name and position. 2. Adjusted the logic in the sniff to allow for named parameters using the new PHPCSUtils 1.0.0-alpha4 `PassedParameters::getParameterFromStack()` method. 3. The parameter names used are in line with the name as per the PHP 8.0 release. PHP itself renamed a lot of parameters in PHP 8.0. As named parameters did not exist before PHP 8.0, the parameter name as per PHP 8.0 (or above) is the only relevant name. Also see: https://github.com/php/doc-en/pull/2044 4. Updated the error messages to use the parameter name instead of its position. As a lot of the logic is now independent of which function is called, this commit also reduces code duplication in the sniff by some logic changes. Includes additional unit tests. Note: in the context of named parameters, it would be advisable to rename the `MissingSecondParameter` and `MissingThirdParameter` error codes to a dynamic error code using the parameter name instead, but as that would be a BC-break, this will need to wait for the next major (if deemed worth making the change). --- .../Security/PHPFilterFunctionsSniff.php | 65 +++++++++++-------- .../Security/PHPFilterFunctionsUnitTest.inc | 25 +++++-- .../Security/PHPFilterFunctionsUnitTest.php | 5 ++ 3 files changed, 64 insertions(+), 31 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php b/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php index 0ed98497..b1a5b662 100644 --- a/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php @@ -9,6 +9,7 @@ namespace WordPressVIPMinimum\Sniffs\Security; +use PHPCSUtils\Utils\PassedParameters; use WordPressCS\WordPress\AbstractFunctionParameterSniff; /** @@ -26,15 +27,27 @@ class PHPFilterFunctionsSniff extends AbstractFunctionParameterSniff { protected $group_name = 'php_filter_functions'; /** - * Functions this sniff is looking for. + * Functions this sniff is looking for and information on which parameter to check for in those function calls. * - * @var array Key is the function name, value irrelevant. + * @var array Key is the function name. */ protected $target_functions = [ - 'filter_var' => true, - 'filter_input' => true, - 'filter_var_array' => true, - 'filter_input_array' => true, + 'filter_var' => [ + 'param_position' => 2, + 'param_name' => 'filter', + ], + 'filter_input' => [ + 'param_position' => 3, + 'param_name' => 'filter', + ], + 'filter_var_array' => [ + 'param_position' => 2, + 'param_name' => 'options', + ], + 'filter_input_array' => [ + 'param_position' => 2, + 'param_name' => 'options', + ], ]; /** @@ -60,30 +73,28 @@ class PHPFilterFunctionsSniff extends AbstractFunctionParameterSniff { * normal file processing. */ public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) { - if ( $matched_content === 'filter_input' ) { - if ( count( $parameters ) === 2 ) { - $message = 'Missing third parameter for "%s".'; - $data = [ $matched_content ]; - $this->phpcsFile->addWarning( $message, $stackPtr, 'MissingThirdParameter', $data ); - } + $param_position = $this->target_functions[ $matched_content ]['param_position']; + $param_name = $this->target_functions[ $matched_content ]['param_name']; - if ( isset( $parameters[3], $this->restricted_filters[ $parameters[3]['clean'] ] ) ) { - $message = 'Please use an appropriate filter to sanitize, as "%s" does no filtering, see: http://php.net/manual/en/filter.filters.sanitize.php.'; - $data = [ $parameters[3]['clean'] ]; - $this->phpcsFile->addWarning( $message, $stackPtr, 'RestrictedFilter', $data ); - } - } else { - if ( count( $parameters ) === 1 ) { - $message = 'Missing second parameter for "%s".'; - $data = [ $matched_content ]; - $this->phpcsFile->addWarning( $message, $stackPtr, 'MissingSecondParameter', $data ); - } + $target_param = PassedParameters::getParameterFromStack( $parameters, $param_position, $param_name ); + if ( $target_param === false ) { + $message = 'Missing $%s parameter for "%s()".'; + $data = [ $param_name, $matched_content ]; - if ( isset( $parameters[2], $this->restricted_filters[ $parameters[2]['clean'] ] ) ) { - $message = 'Please use an appropriate filter to sanitize, as "%s" does no filtering, see http://php.net/manual/en/filter.filters.sanitize.php.'; - $data = [ $parameters[2]['clean'] ]; - $this->phpcsFile->addWarning( $message, $stackPtr, 'RestrictedFilter', $data ); + // Error codes should probably be made more descriptive, but that would be a BC-break. + $error_code = 'MissingSecondParameter'; + if ( $matched_content === 'filter_input' ) { + $error_code = 'MissingThirdParameter'; } + + $this->phpcsFile->addWarning( $message, $stackPtr, $error_code, $data ); + return; + } + + if ( isset( $this->restricted_filters[ $target_param['clean'] ] ) ) { + $message = 'Please use an appropriate filter to sanitize, as "%s" does no filtering, see: http://php.net/manual/en/filter.filters.sanitize.php.'; + $data = [ $target_param['clean'] ]; + $this->phpcsFile->addWarning( $message, $stackPtr, 'RestrictedFilter', $data ); } } } diff --git a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc index 9b422978..004ceaf2 100644 --- a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc @@ -41,18 +41,35 @@ $incorrect_but_ok = filter_input(); /* * These should all be flagged with a warning. */ -filter_input( INPUT_GET, 'foo' ); // Missing third parameter. +filter_input( INPUT_GET, 'foo' ); // Missing $filter parameter. \filter_input( INPUT_GET, 'foo', FILTER_DEFAULT ); // This filter ID does nothing. filter_input( INPUT_GET, "foo", FILTER_UNSAFE_RAW /* comment */ ,); // This filter ID does nothing. -filter_var( $url ); // Missing second parameter. +filter_var( $url ); // Missing $filter parameter. Filter_Var( $url, FILTER_DEFAULT ); // This filter ID does nothing. filter_var( 'https://google.com', FILTER_UNSAFE_RAW ); // This filter ID does nothing. -filter_var_array( $array, ); // Missing second parameter. +filter_var_array( $array, ); // Missing $options parameter. filter_var_array( $array, FILTER_DEFAULT ); // This filter ID does nothing. filter_var_array( $array, FILTER_UNSAFE_RAW ); // This filter ID does nothing. -filter_input_array( $array ); // Missing second parameter. +filter_input_array( $array ); // Missing $options parameter. \FILTER_INPUT_ARRAY( $array, FILTER_DEFAULT ); // This filter ID does nothing. filter_input_array( $array, FILTER_UNSAFE_RAW, ); // This filter ID does nothing. + +// Safeguard handling of function calls using PHP 8.0+ named parameters. +filter_input(var_name: $var_name, filter: FILTER_SANITIZE_STRING, type: FILTER_DEFAULT); // OK, invalid input value for $type, but that's not our concern. +filter_input(var_name: $var_name, filter: $filter, type: $type); // Ignore, undetermined. +filter_input( + var_name: $var_name, + filter: FILTER_DEFAULT, // This filter ID does nothing. + type: $type, +); + +filter_var(filter: FILTER_SANITIZE_URL); // OK, well not really, missing required parameter, but that's not our concern. +filter_var($value, options: FILTER_NULL_ON_FAILURE); // Missing $filter parameter. +filter_var(value: $value, filters: FILTER_SANITIZE_URL); // Typo in parameter name, report as missing $filter parameter. + +filter_var_array(options: FILTER_UNSAFE_RAW, array: $array); // This filter ID does nothing. + +filter_input_array($type, add_empty: false, options: FILTER_DEFAULT,); // This filter ID does nothing. diff --git a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php index 201f4a8d..1f4473fd 100644 --- a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php +++ b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php @@ -44,6 +44,11 @@ public function getWarningList() { 56 => 1, 57 => 1, 58 => 1, + 63 => 1, + 70 => 1, + 71 => 1, + 73 => 1, + 75 => 1, ]; } } From c65e55c59da28f1689027e37f21adf0c1100639e Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 18 Jul 2025 03:25:10 +0200 Subject: [PATCH 5/7] Security/PHPFilterFunctions: report restricted filter on the correct token Throw the warning about the use of a restricted filter on the actual filter constant, not on the function call token. This will more clearly indicate the issue when people use the PHPCS `code` view. --- .../Sniffs/Security/PHPFilterFunctionsSniff.php | 5 ++++- .../Tests/Security/PHPFilterFunctionsUnitTest.php | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php b/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php index b1a5b662..17945b3e 100644 --- a/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php @@ -9,6 +9,7 @@ namespace WordPressVIPMinimum\Sniffs\Security; +use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Utils\PassedParameters; use WordPressCS\WordPress\AbstractFunctionParameterSniff; @@ -92,9 +93,11 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p } if ( isset( $this->restricted_filters[ $target_param['clean'] ] ) ) { + $first_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $target_param['start'], ( $target_param['end'] + 1 ), true ); + $message = 'Please use an appropriate filter to sanitize, as "%s" does no filtering, see: http://php.net/manual/en/filter.filters.sanitize.php.'; $data = [ $target_param['clean'] ]; - $this->phpcsFile->addWarning( $message, $stackPtr, 'RestrictedFilter', $data ); + $this->phpcsFile->addWarning( $message, $first_non_empty, 'RestrictedFilter', $data ); } } } diff --git a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php index 1f4473fd..33d23ffd 100644 --- a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php +++ b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php @@ -44,7 +44,7 @@ public function getWarningList() { 56 => 1, 57 => 1, 58 => 1, - 63 => 1, + 65 => 1, 70 => 1, 71 => 1, 73 => 1, From 4febbf34adf236f4cc846c13a760fa9da80ac518 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 18 Jul 2025 04:18:50 +0200 Subject: [PATCH 6/7] Security/PHPFilterFunctions: ignore function calls using PHP 5.6+ argument unpacking This sniff ignores the function call if it cannot reliably be determined if a warning should be thrown. So, for function calls using PHP 5.6+ argument unpacking, I've elected to also ignore these function calls. Includes tests covering the change. --- .../Security/PHPFilterFunctionsSniff.php | 22 +++++++++++++++++++ .../Security/PHPFilterFunctionsUnitTest.inc | 6 +++++ .../Security/PHPFilterFunctionsUnitTest.php | 1 + 3 files changed, 29 insertions(+) diff --git a/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php b/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php index 17945b3e..a507ac13 100644 --- a/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php @@ -79,6 +79,28 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $target_param = PassedParameters::getParameterFromStack( $parameters, $param_position, $param_name ); if ( $target_param === false ) { + /* + * Check for PHP 5.6+ argument unpacking. + * + * No need for extensive defensive coding, we already know this is syntactically a valid function call, + * otherwise this method would not have been reached. + */ + $tokens = $this->phpcsFile->getTokens(); + $open_parens = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); + $has_ellipses = $this->phpcsFile->findNext( T_ELLIPSIS, ( $open_parens + 1 ), $tokens[ $open_parens ]['parenthesis_closer'] ); + + if ( $has_ellipses !== false ) { + $target_nesting_level = 1; + if ( isset( $tokens[ $open_parens ]['nested_parenthesis'] ) ) { + $target_nesting_level = ( count( $tokens[ $open_parens ]['nested_parenthesis'] ) + 1 ); + } + + if ( $target_nesting_level === count( $tokens[ $has_ellipses ]['nested_parenthesis'] ) ) { + // Bow out as undetermined. + return; + } + } + $message = 'Missing $%s parameter for "%s()".'; $data = [ $param_name, $matched_content ]; diff --git a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc index 004ceaf2..f9a4f5ab 100644 --- a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc @@ -73,3 +73,9 @@ filter_var(value: $value, filters: FILTER_SANITIZE_URL); // Typo in parameter na filter_var_array(options: FILTER_UNSAFE_RAW, array: $array); // This filter ID does nothing. filter_input_array($type, add_empty: false, options: FILTER_DEFAULT,); // This filter ID does nothing. + +// Ignore function calls using PHP 5.6 argument unpacking as we don't know what parameters were passed. +filter_input(INPUT_GET, ...$params); +trim(filter_input(INPUT_GET, ...$params)); +// ... but only ignore unpacking if done at the correct nesting level. +filter_input(INPUT_GET, $obj->getVarname(...$params)); // Missing $filter parameter. diff --git a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php index 33d23ffd..e708a38c 100644 --- a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php +++ b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php @@ -49,6 +49,7 @@ public function getWarningList() { 71 => 1, 73 => 1, 75 => 1, + 81 => 1, ]; } } From 824bb1be72349dcafa6c5ceb2b95cc6dc141e6e5 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 18 Jul 2025 04:37:42 +0200 Subject: [PATCH 7/7] Security/PHPFilterFunctions: document unsupported PHP feature The `$options` parameter for `filter_var_array()` and `filter_input_array()` can take either an integer (filter constant) or an array with options, which could include an option setting the filter constant. At this time, this sniff does not handle an array with options being passed to these functions. Adding support for this will make the sniff much more complicated as PHP supports multiple array formats. Additionally, if the `$options` parameter is passed as an array, the likelyhood that the array is passed in as a variable increases exponentially, so then the next problem would be finding the variable definition and analysing that. All in all, this is a rabbit hole without end. Refs: * https://www.php.net/manual/en/function.filter-var-array.php * https://www.php.net/manual/en/function.filter-input-array.php --- .../Security/PHPFilterFunctionsSniff.php | 4 ++++ .../Security/PHPFilterFunctionsUnitTest.inc | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php b/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php index a507ac13..06b67503 100644 --- a/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php @@ -16,6 +16,10 @@ /** * This sniff ensures that proper sanitization is occurring when PHP's filter_* functions are used. * + * {@internal The $options parameter for filter_var_array() and filter_input_array() can take either an + * integer (filter constant) or an array with options, which could include an option setting the filter constant. + * At this time, this sniff does not handle an array with options being passed.} + * * @since 0.4.0 */ class PHPFilterFunctionsSniff extends AbstractFunctionParameterSniff { diff --git a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc index f9a4f5ab..0159aac5 100644 --- a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc @@ -79,3 +79,22 @@ filter_input(INPUT_GET, ...$params); trim(filter_input(INPUT_GET, ...$params)); // ... but only ignore unpacking if done at the correct nesting level. filter_input(INPUT_GET, $obj->getVarname(...$params)); // Missing $filter parameter. + +// False negatives: $options arrays are currently not (yet) supported by this sniff. +// See: https://www.php.net/manual/en/function.filter-var-array.php and https://www.php.net/manual/en/function.filter-input-array.php +filter_var_array( + $array, + array('keyA' => FILTER_DEFAULT), // This filter ID does nothing. +); +filter_input_array( + $array, + [ + 'keyA' => [ + 'filter' => FILTER_UNSAFE_RAW, // This filter ID does nothing. + 'flags' => FILTER_FORCE_ARRAY, + ], + 'keyB' => [ + 'filter' => FILTER_SANITIZE_ENCODED, + ], + ] +);