diff --git a/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php b/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php index 358ff2ca..807a5529 100644 --- a/WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php +++ b/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 + * @var string */ - public function register() { - return [ T_STRING ]; - } + protected $group_name = 'file_get_contents'; + + /** + * Functions this sniff is looking for. + * + * @var array 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 $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 ); } } diff --git a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc index 9edbd772..40e48b7f 100644 --- a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc @@ -1,7 +1,88 @@ 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. \ No newline at end of file + +/* + * 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. diff --git a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php b/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php index 05c27d4d..670ef4c5 100644 --- a/WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php +++ b/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, ]; } }