Security/StaticStrreplace: various sniff improvements

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<int|string>
+	 * @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<string, bool> 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<string, int|string> $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;
 	}
 }

WordPressVIPMinimum/Tests/Security/StaticStrreplaceUnitTest.inc

@@ -1,13 +1,101 @@
 <?php
 
+/*
+ * Not the sniff target.
+ */
+use str_replace;
+
+my\ns\str_replace('foo', 'bar', 'foobar');
+$this->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(<<<EOD
+foo
+EOD
+	),
+	'bar',
+	<<<"EOD"
+foobar
+EOD
+); // Bad.
 
-str_replace( array( $foo, $bar ), array( 'bar', 'foo' ), $foobar ); // Ok.
+// Ensure heredocs with interpolation are ignored.
+str_replace( 'foo', 'bar', <<<EOD
+Some text and $foobar
+EOD
+); // OK.

WordPressVIPMinimum/Tests/Security/StaticStrreplaceUnitTest.php

@@ -23,8 +23,17 @@ class StaticStrreplaceUnitTest extends AbstractSniffUnitTest {
 	 */
 	public function getErrorList() {
 		return [
-			3 => 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,
 		];
 	}