Performance/FetchingRemoteData: various sniff improvements

WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php

@@ -10,50 +10,149 @@
 namespace WordPressVIPMinimum\Sniffs\Performance;
 
 use PHP_CodeSniffer\Util\Tokens;
-use WordPressVIPMinimum\Sniffs\Sniff;
+use PHPCSUtils\Utils\PassedParameters;
+use WordPressCS\WordPress\AbstractFunctionParameterSniff;
 
 /**
  * Restricts usage of file_get_contents().
  */
-class FetchingRemoteDataSniff extends Sniff {
+class FetchingRemoteDataSniff 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 = 'file_get_contents';
+
+	/**
+	 * Functions this sniff is looking for.
+	 *
+	 * @var array<string, bool> Key is the function name, value irrelevant.
+	 */
+	protected $target_functions = [
+		'file_get_contents' => true,
+	];
 
 	/**
-	 * Process this test when one of its tokens is encountered.
+	 * Process the parameters of a matched function.
 	 *
-	 * @param int $stackPtr The position of the current token in the stack passed in $tokens.
+	 * @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 ) {
-
-		$functionName = $this->tokens[ $stackPtr ]['content'];
-		if ( $functionName !== 'file_get_contents' ) {
+	public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
+		$filename_param = PassedParameters::getParameterFromStack( $parameters, 1, 'filename' );
+		if ( $filename_param === false ) {
+			// Missing required parameter. Probably live coding, nothing to examine (yet). Bow out.
 			return;
 		}
 
-		$data = [ $this->tokens[ $stackPtr ]['content'] ];
+		$data        = [ $matched_content ];
+		$param_start = $filename_param['start'];
+		$search_end  = ( $filename_param['end'] + 1 );
 
-		$fileNameStackPtr = $this->phpcsFile->findNext( Tokens::$stringTokens, $stackPtr + 1, null, false, null, true );
-		if ( $fileNameStackPtr === false ) {
-			$message = '`%s()` is highly discouraged for remote requests, please use `wpcom_vip_file_get_contents()` or `vip_safe_wp_remote_get()` instead. If it\'s for a local file please use WP_Filesystem instead.';
-			$this->phpcsFile->addWarning( $message, $stackPtr, 'FileGetContentsUnknown', $data );
+		$has_magic_dir = $this->phpcsFile->findNext( T_DIR, $param_start, $search_end );
+		if ( $has_magic_dir !== false ) {
+			// In all likelyhood a local file (disregarding creative code).
+			return;
 		}
 
-		$fileName = $this->tokens[ $fileNameStackPtr ]['content'];
+		$isRemoteFile = false;
+		$search_start = $param_start;
+		// phpcs:ignore Generic.CodeAnalysis.AssignmentInCondition.FoundInWhileCondition -- Valid usage.
+		while ( ( $has_text_string = $this->phpcsFile->findNext( Tokens::$stringTokens, $search_start, $search_end ) ) !== false ) {
+			if ( strpos( $this->tokens[ $has_text_string ]['content'], '://' ) !== false ) {
+				$isRemoteFile = true;
+				break;
+			}
+
+			$search_start = ( $has_text_string + 1 );
+		}
 
-		$isRemoteFile = ( strpos( $fileName, '://' ) !== false );
 		if ( $isRemoteFile === true ) {
 			$message = '`%s()` is highly discouraged for remote requests, please use `wpcom_vip_file_get_contents()` or `vip_safe_wp_remote_get()` instead.';
 			$this->phpcsFile->addWarning( $message, $stackPtr, 'FileGetContentsRemoteFile', $data );
+			return;
 		}
+
+		/*
+		 * Okay, so we haven't been able to determine for certain this is a remote file.
+		 * Check for tokens which would make the parameter contents dynamic.
+		 */
+		$ignore  = Tokens::$emptyTokens;
+		$ignore += Tokens::$stringTokens;
+		$ignore += [ T_STRING_CONCAT => T_STRING_CONCAT ];
+
+		$has_non_text_string = $this->phpcsFile->findNext( $ignore, $param_start, $search_end, true );
+		if ( $has_non_text_string !== false ) {
+			$this->add_contents_unknown_warning( $stackPtr, $data );
+		}
+	}
+
+	/**
+	 * Process the function if no parameters were found.
+	 *
+	 * {@internal This method is only needed for handling PHP 8.1+ first class callables on WPCS < 3.2.0.
+	 * Once the minimum supported WPCS is 3.2.0 or higher, this method should be removed.}
+	 *
+	 * @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.
+	 *
+	 * @return void
+	 */
+	public function process_no_parameters( $stackPtr, $group_name, $matched_content ) {
+		// Check if this is a first class callable.
+		$next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
+		if ( $next === false
+			|| $this->tokens[ $next ]['code'] !== T_OPEN_PARENTHESIS
+			|| isset( $this->tokens[ $next ]['parenthesis_closer'] ) === false
+		) {
+			// Live coding/parse error. Ignore.
+			return;
+		}
+
+		// First class callable.
+		$firstNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $next + 1 ), null, true );
+		if ( $this->tokens[ $firstNonEmpty ]['code'] === T_ELLIPSIS ) {
+			$secondNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $firstNonEmpty + 1 ), null, true );
+			if ( $this->tokens[ $secondNonEmpty ]['code'] === T_CLOSE_PARENTHESIS ) {
+				$this->add_contents_unknown_warning( $stackPtr, [ $matched_content ] );
+			}
+		}
+	}
+
+	/**
+	 * Process the function if it is used as a first class callable.
+	 *
+	 * @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.
+	 *
+	 * @return void
+	 */
+	public function process_first_class_callable( $stackPtr, $group_name, $matched_content ) {
+		$this->add_contents_unknown_warning( $stackPtr, [ $matched_content ] );
+	}
+
+	/**
+	 * Add a warning if the function is used with unknown parameter(s) or with a $filename parameter for which
+	 * it could not be determined if it references a local file or a remote file.
+	 *
+	 * @param int           $stackPtr The position of the current token in the stack.
+	 * @param array<string> $data     Data to use for string replacement in the error message.
+	 *
+	 * @return void
+	 */
+	private function add_contents_unknown_warning( $stackPtr, $data ) {
+		$message = '`%s()` is highly discouraged for remote requests, please use `wpcom_vip_file_get_contents()` or `vip_safe_wp_remote_get()` instead. If it\'s for a local file please use WP_Filesystem instead.';
+		$this->phpcsFile->addWarning( $message, $stackPtr, 'FileGetContentsUnknown', $data );
 	}
 }

WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc

@@ -1,7 +1,88 @@
 <?php
 
-$file_content = file_get_contents( 'my-file.css' ); // OK.
+/*
+ * Not the sniff target.
+ */
+use function file_get_contents;
 
-$file_content = file_get_contents( 'my-file.svg' ); // OK.
+my\ns\file_get_contents($url);
+$this->file_get_contents($url);
+$this?->file_get_contents($file);
+MyClass::file_get_contents($file);
+echo FILE_GET_CONTENTS;
+namespace\file_get_contents($url);
 
-$external_resource = file_get_contents( 'https://example.com' ); // NOK.
+
+/*
+ * These should all be okay.
+ */
+$file_content = file_get_contents( 'my-file.css' );
+$file_content = \File_Get_Contents( /*comment*/ 'my-file.svg', );
+$file_content = file_get_contents( __DIR__ . "/filename.css" );
+
+// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute.
+#[File_Get_Contents('text')]
+function foo() {}
+
+// Incomplete function call, should be ignored by the sniff.
+$live_coding = file_get_contents();
+
+
+/*
+ * These should all be flagged with a warning.
+ */
+// Warning FileGetContentsRemoteFile.
+$external_resource = file_get_contents( 'https://example.com', );
+\file_get_contents( "http://$url" );
+$external_resource = FILE_GET_CONTENTS(
+	// Sort of protocol relative URL (the ":" is superfluous).
+	'://example.com'
+);
+$external_resource = file_get_contents( 'HTTP://example.com', );
+
+// Warning FileGetContentsUnknown.
+file_get_contents( $url, true, $context, $offset, $length );
+\File_GET_Contents( $obj->get_fileName() );
+file_get_contents( MyClass::FILE_NAME, );
+
+file_get_contents(...$params); // PHP 5.6 argument unpacking. Warning FileGetContentsUnknown.
+
+// Safeguard correct handling of function calls using PHP 8.0+ named parameters.
+file_get_contents(context: $context, ); // OK, well, not really, missing required $filename param, but that's not the concern of this sniff.
+file_get_contents(file: 'https://example.com',); // OK, well, not really, typo in param name, but that's not the concern of the sniff.
+file_get_contents(
+	context: stream_context_create(
+		options: [
+			$proxy => 'tcp://proxy.example.com:5100',
+		],
+	),
+	filename: 'my-file.svg',
+); // OK.
+
+file_get_contents(
+	context: stream_context_create(
+		options: [
+			$cafile => __DIR__ . '/cacert.pem',
+		],
+	),
+	filename: 'https://example.com'
+); // Warning FileGetContentsRemoteFile.
+file_get_contents(
+	context: stream_context_create(
+		options: [
+			$cafile => __DIR__ . '/cacert.pem',
+		],
+	),
+	filename: $fileName,
+); // Warning FileGetContentsUnknown.
+
+array_walk($files, file_get_contents(...)); // PHP 8.1 first class callable. Warning FileGetContentsUnknown.
+
+// Bug: don't throw a warning when __DIR__ is used as this indicates use with a local file.
+\file_get_contents( __DIR__ . $filename ); // OK.
+
+// Bug: check all text string tokens in the parameter.
+$result = file_get_contents(( is_ssl() ? 'http' : 'https' ) . '://example.com'); // Warning FileGetContentsRemoteFile.
+
+// Bug: don't presume if the parameter contains a text string token, that will be the only token.
+\file_get_contents( $url . '/filename.css' ); // Warning FileGetContentsUnknown.

WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php

@@ -32,7 +32,19 @@ public function getErrorList() {
 	 */
 	public function getWarningList() {
 		return [
-			7 => 1,
+			35 => 1,
+			36 => 1,
+			37 => 1,
+			41 => 1,
+			44 => 1,
+			45 => 1,
+			46 => 1,
+			48 => 1,
+			62 => 1,
+			70 => 1,
+			79 => 1,
+			85 => 1,
+			88 => 1,
 		];
 	}
 }