diff --git a/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php b/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php index bc4002ac..06b67503 100644 --- a/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php @@ -9,11 +9,17 @@ namespace WordPressVIPMinimum\Sniffs\Security; +use PHP_CodeSniffer\Util\Tokens; +use PHPCSUtils\Utils\PassedParameters; use WordPressCS\WordPress\AbstractFunctionParameterSniff; /** * 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 { @@ -26,15 +32,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 +78,52 @@ 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]['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'] ) ]; - $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 ) { + /* + * 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; + } } - 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'] ) ]; - $this->phpcsFile->addWarning( $message, $stackPtr, 'RestrictedFilter', $data ); + $message = 'Missing $%s parameter for "%s()".'; + $data = [ $param_name, $matched_content ]; + + // 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'] ] ) ) { + $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, $first_non_empty, 'RestrictedFilter', $data ); } } } diff --git a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc index 9960f4ed..0159aac5 100644 --- a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc @@ -1,29 +1,100 @@ 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. -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_var( $url ); // Missing second parameter. -filter_var( $url, FILTER_DEFAULT ); // This filter ID does nothing. +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() ); +\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 $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 $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, FILTER_DEFAULT ); // This filter ID does nothing. -filter_input_array( $array, FILTER_UNSAFE_RAW ); // This filter ID does nothing. + +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. + +// 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. + +// 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, + ], + ] +); diff --git a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php index dae998cc..e708a38c 100644 --- a/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php +++ b/WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php @@ -32,18 +32,24 @@ 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, + 65 => 1, + 70 => 1, + 71 => 1, + 73 => 1, + 75 => 1, + 81 => 1, ]; } }