diff --git a/WordPressVIPMinimum/Sniffs/Security/StaticStrreplaceSniff.php b/WordPressVIPMinimum/Sniffs/Security/StaticStrreplaceSniff.php index 8ace2cd5..017c36a5 100644 --- a/WordPressVIPMinimum/Sniffs/Security/StaticStrreplaceSniff.php +++ b/WordPressVIPMinimum/Sniffs/Security/StaticStrreplaceSniff.php @@ -10,77 +10,135 @@ namespace WordPressVIPMinimum\Sniffs\Security; use PHP_CodeSniffer\Util\Tokens; -use WordPressVIPMinimum\Sniffs\Sniff; +use PHPCSUtils\Tokens\Collections; +use PHPCSUtils\Utils\Arrays; +use PHPCSUtils\Utils\PassedParameters; +use PHPCSUtils\Utils\TextStrings; +use WordPressCS\WordPress\AbstractFunctionParameterSniff; /** * Restricts usage of str_replace with all 3 params being static. */ -class StaticStrreplaceSniff extends Sniff { +class StaticStrreplaceSniff extends AbstractFunctionParameterSniff { /** - * Returns an array of tokens this test wants to listen for. + * The group name for this group of functions. * - * @return array + * @var string */ - public function register() { - return [ T_STRING ]; - } + protected $group_name = 'str_replace'; /** - * Process this test when one of its tokens is encountered + * Functions this sniff is looking for. * - * @param int $stackPtr The position of the current token in the stack passed in $tokens. + * @var array Key is the function name, value irrelevant. + */ + protected $target_functions = [ + 'str_replace' => true, + ]; + + /** + * Process the parameters of a matched function. + * + * @param int $stackPtr The position of the current token in the stack. + * @param string $group_name The name of the group which was matched. + * @param string $matched_content The token content (function name) which was matched + * in lowercase. + * @param array $parameters Array with information about the parameters. * * @return void */ - public function process_token( $stackPtr ) { - - if ( $this->tokens[ $stackPtr ]['content'] !== 'str_replace' ) { + public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) { + $search_param = PassedParameters::getParameterFromStack( $parameters, 1, 'search' ); + $replace_param = PassedParameters::getParameterFromStack( $parameters, 2, 'replace' ); + $subject_param = PassedParameters::getParameterFromStack( $parameters, 3, 'subject' ); + + if ( $search_param === false || $replace_param === false || $subject_param === false ) { + /* + * Either an invalid function call (missing PHP required parameter); or function call + * with argument unpacking; or live coding. + * In all these cases, this is not the code pattern this sniff is looking for, so bow out. + */ return; } - $openBracket = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true ); - - if ( $this->tokens[ $openBracket ]['code'] !== T_OPEN_PARENTHESIS ) { - return; + foreach ( [ $search_param, $replace_param, $subject_param ] as $param ) { + if ( $this->is_parameter_static_text( $param ) === false ) { + // Non-static text token found. Not what we're looking for. + return; + } } - $next_start_ptr = $openBracket + 1; - for ( $i = 0; $i < 3; $i++ ) { - $param_ptr = $this->phpcsFile->findNext( array_merge( Tokens::$emptyTokens, [ T_COMMA ] ), $next_start_ptr, null, true ); - - if ( $this->tokens[ $param_ptr ]['code'] === T_ARRAY ) { - $openBracket = $this->phpcsFile->findNext( Tokens::$emptyTokens, $param_ptr + 1, null, true ); - if ( $this->tokens[ $openBracket ]['code'] !== T_OPEN_PARENTHESIS ) { - return; - } + $message = 'This code pattern is often used to run a very dangerous shell programs on your server. The code in these files needs to be reviewed, and possibly cleaned.'; + $this->phpcsFile->addError( $message, $stackPtr, 'StaticStrreplace' ); + } - // Find the closing bracket. - $closeBracket = $this->tokens[ $openBracket ]['parenthesis_closer']; + /** + * Check whether the current parameter, or array item, only contains tokens which should be regarded + * as a valid part of a static text string. + * + * @param array $param_info Array with information about a single parameter or array item. + * Must be an array as returned via the PassedParameters class. + * + * @return bool + */ + private function is_parameter_static_text( $param_info ) { + // List of tokens which can be skipped over without further examination. + $static_tokens = [ + T_CONSTANT_ENCAPSED_STRING => T_CONSTANT_ENCAPSED_STRING, + T_PLUS => T_PLUS, + T_STRING_CONCAT => T_STRING_CONCAT, + ]; + $static_tokens += Tokens::$emptyTokens; + + for ( $i = $param_info['start']; $i <= $param_info['end']; $i++ ) { + $next_to_examine = $this->phpcsFile->findNext( $static_tokens, $i, ( $param_info['end'] + 1 ), true ); + if ( $next_to_examine === false ) { + // The parameter contained only tokens which could be considered static text. + return true; + } - $array_item_ptr = $this->phpcsFile->findNext( array_merge( Tokens::$emptyTokens, [ T_COMMA ] ), $openBracket + 1, $closeBracket, true ); - while ( $array_item_ptr !== false ) { + if ( isset( Collections::arrayOpenTokensBC()[ $this->tokens[ $next_to_examine ]['code'] ] ) ) { + $arrayOpenClose = Arrays::getOpenClose( $this->phpcsFile, $next_to_examine ); + if ( $arrayOpenClose === false ) { + // Short list, parse error or live coding, bow out. + return false; + } - if ( $this->tokens[ $array_item_ptr ]['code'] !== T_CONSTANT_ENCAPSED_STRING ) { - return; + $array_items = PassedParameters::getParameters( $this->phpcsFile, $next_to_examine ); + foreach ( $array_items as $array_item ) { + if ( $this->is_parameter_static_text( $array_item ) === false ) { + return false; } - $array_item_ptr = $this->phpcsFile->findNext( array_merge( Tokens::$emptyTokens, [ T_COMMA ] ), $array_item_ptr + 1, $closeBracket, true ); } - $next_start_ptr = $closeBracket + 1; + // The array only contained items with tokens which could be considered static text. + $i = $arrayOpenClose['closer']; continue; - } - if ( $this->tokens[ $param_ptr ]['code'] !== T_CONSTANT_ENCAPSED_STRING ) { - return; + if ( $this->tokens[ $next_to_examine ]['code'] === T_START_HEREDOC ) { + $heredoc_text = TextStrings::getCompleteTextString( $this->phpcsFile, $next_to_examine ); + $stripped_text = TextStrings::stripEmbeds( $heredoc_text ); + if ( $heredoc_text !== $stripped_text ) { + // Heredoc with interpolated expression(s). Not a static text. + return false; + } } - $next_start_ptr = $param_ptr + 1; + if ( ( $this->tokens[ $next_to_examine ]['code'] === T_START_HEREDOC + || $this->tokens[ $next_to_examine ]['code'] === T_START_NOWDOC ) + && isset( $this->tokens[ $next_to_examine ]['scope_closer'] ) + ) { + // No interpolation. Skip to end of a heredoc/nowdoc. + $i = $this->tokens[ $next_to_examine ]['scope_closer']; + continue; + } + // Any other token means this parameter should be regarded as non-static text. Not what we're looking for. + return false; } - $message = 'This code pattern is often used to run a very dangerous shell programs on your server. The code in these files needs to be reviewed, and possibly cleaned.'; - $this->phpcsFile->addError( $message, $stackPtr, 'StaticStrreplace' ); + return true; } } diff --git a/WordPressVIPMinimum/Tests/Security/StaticStrreplaceUnitTest.inc b/WordPressVIPMinimum/Tests/Security/StaticStrreplaceUnitTest.inc index d5dd8a1f..4b7af5dc 100644 --- a/WordPressVIPMinimum/Tests/Security/StaticStrreplaceUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Security/StaticStrreplaceUnitTest.inc @@ -1,13 +1,101 @@ str_replace('foo', 'bar', 'foobar'); +$this?->str_replace('foo', 'bar', 'foobar'); +MyClass::str_replace('foo', 'bar', 'foobar'); +echo STR_REPLACE; +namespace\str_replace('foo', 'bar', 'foobar', $count); + +// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute. +#[Str_Replace('foo', 'bar', 'foobar')] +function foo() {} + +// Ignore PHP 5.6 argument unpacking. +str_replace('foo', 'bar', ...$params); + +// Ignore PHP 8.1 first class callable. +add_filter('my_filter', str_replace(...)); + +// Incomplete function calls, should be ignored by the sniff. +str_replace(); +str_replace('foo', 'bar'); + + +/* + * These should all be okay. + */ +str_replace( $foo->getSearch(), 'bar', 'foobar' ); +\str_replace( 'foo', $bar, 'foobar' ); +str_replace( 'foo', 'bar', $foobar, ); +STR_REPLACE( $foo, $bar, 'foobar' ); +str_replace( 'foo', get_replacements(), $foobar, $count ); + +str_replace( array( 'foo', "$bar" ), $bar, 'foobar' ); + +str_replace( array( $foo, $bar ), array( 'bar', 'foo' ), array( 'foobar', 'foobar' ) ); +\Str_Replace( [ 'foo', 'bar' ], array( $foo, SOME_CONSTANT ), 'foobar' ); +str_replace( array( 'foo', "bar" ), [ 'bar', 'foo' ], $foobar, ); +str_replace( [ $foo, $bar->prop ], /*comment*/ [ 'bar', 'foo' ], $foobar ); + + +/* + * These should all be flagged with a warning. + */ str_replace( 'foo', 'bar', 'foobar' ); // Bad. +Str_replace( + 'foo', // Comment. + 'bar', /* comment */ + 'foobar' +); // Bad. +str_replace( array( 'foo', 'bar' ), array( 'bar', 'foo' ), array( 'foobar', 'foobar' ) ); // Bad. +str_replace( 'foo', 'bar', 'foobar', $count ); // Bad. + +// Handle PHP 5.4+ short array syntax correctly. +\str_replace( [ 'foo', 'bar' ], [ 'bar', 'foo' ], [ 'foobar', 'foobar' ], ); // Bad. + +// Don't confuse PHP 7.1+ short list with short array. +str_replace( [ $a, $b ] = $search, [ 'bar', 'foo' ], 'foobar', ); // OK. + +// Safeguard support for PHP 8.0+ named parameters. +str_replace( search: [ 'foo', 'bar' ], replace: 'baz', count: $count ); // OK, well not really, missing required $subject param, but not our concern. +str_replace( search: 'foo', replace: 'baz', subjects: [ 'foobar', 'foobar' ] ); // OK, well not really, typo in $subject param name, not our concern. +str_replace( subject: $subject, count: $count, replace: 'baz', search: 'foo', ); // OK, different parameter order. -str_replace( 'foo', 'bar', $foobar ); // Ok. +str_replace( count: $count, subject: [ 'foobar', 'foobar' ], search: 'foo', replace: 'baz', ); // Bad with different parameter order. +str_replace( [ 'foo', 'bar' ], 'baz', count: $count, subject: 'foobar' ); // Bad, with mixed named and unnamed params. -str_replace( array( 'foo', 'bar' ), array( 'bar', 'foo' ), 'foobar' ); // Bad. +// Make sure some variations of how plain strings can be passed are handled. +str_replace( 'fo' . 'o', 'bar', 'foobar' ); // Bad. +str_replace( array( 'foo', 'bar' ) + array( 'baz', 'fop' ), array( 'bar', 'foo' ), 'foobar' ); // Bad. -str_replace( array( 'foo', 'bar' ), array( 'bar', 'foo' ), $foobar ); // Ok. +// Safeguard correct handling of PHP 5.3+ nowdocs. +str_replace( + 'foo', + 'bar', + <<<'EOD' +foobar +EOD +); // Bad. -str_replace( array( 'foo', 'bar' ), array( $foo, $bar ), $foobar ); // Ok. +// Safeguard correct handling of heredocs without interpolation. +str_replace( array(<< 1, - 7 => 1, + 50 => 1, + 51 => 1, + 56 => 1, + 57 => 1, + 60 => 1, + 70 => 1, + 71 => 1, + 74 => 1, + 75 => 1, + 78 => 1, + 87 => 1, ]; }