Skip to content

Commit bef6b9a

Browse files
GaryJonesGaryJones
authored
Merge pull request #855 from Automattic/feature/performance-fetchingremotedata-sniff-review
2 parents 4f91688 + 7d23be5 commit bef6b9a

File tree

3 files changed

+216
-24
lines changed

3 files changed

+216
-24
lines changed

‎WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php‎

Lines changed: 119 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -10,50 +10,149 @@
1010
namespace WordPressVIPMinimum\Sniffs\Performance;
1111

1212
use PHP_CodeSniffer\Util\Tokens;
13-
use WordPressVIPMinimum\Sniffs\Sniff;
13+
use PHPCSUtils\Utils\PassedParameters;
14+
use WordPressCS\WordPress\AbstractFunctionParameterSniff;
1415

1516
/**
1617
* Restricts usage of file_get_contents().
1718
*/
18-
class FetchingRemoteDataSniff extends Sniff {
19+
class FetchingRemoteDataSniff extends AbstractFunctionParameterSniff {
1920

2021
/**
21-
* Returns an array of tokens this test wants to listen for.
22+
* The group name for this group of functions.
2223
*
23-
* @return array<int|string>
24+
* @var string
2425
*/
25-
public function register() {
26-
return [ T_STRING ];
27-
}
26+
protected $group_name = 'file_get_contents';
27+
28+
/**
29+
* Functions this sniff is looking for.
30+
*
31+
* @var array<string, bool> Key is the function name, value irrelevant.
32+
*/
33+
protected $target_functions = [
34+
'file_get_contents' => true,
35+
];
2836

2937
/**
30-
* Process this test when one of its tokens is encountered.
38+
* Process the parameters of a matched function.
3139
*
32-
* @param int $stackPtr The position of the current token in the stack passed in $tokens.
40+
* @param int $stackPtr The position of the current token in the stack.
41+
* @param string $group_name The name of the group which was matched.
42+
* @param string $matched_content The token content (function name) which was matched
43+
* in lowercase.
44+
* @param array $parameters Array with information about the parameters.
3345
*
3446
* @return void
3547
*/
36-
public function process_token( $stackPtr ) {
37-
38-
$functionName = $this->tokens[ $stackPtr ]['content'];
39-
if ( $functionName !== 'file_get_contents' ) {
48+
public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
49+
$filename_param = PassedParameters::getParameterFromStack( $parameters, 1, 'filename' );
50+
if ( $filename_param === false ) {
51+
// Missing required parameter. Probably live coding, nothing to examine (yet). Bow out.
4052
return;
4153
}
4254

43-
$data = [ $this->tokens[ $stackPtr ]['content'] ];
55+
$data = [ $matched_content ];
56+
$param_start = $filename_param['start'];
57+
$search_end = ( $filename_param['end'] + 1 );
4458

45-
$fileNameStackPtr = $this->phpcsFile->findNext( Tokens::$stringTokens, $stackPtr + 1, null, false, null, true );
46-
if ( $fileNameStackPtr === false ) {
47-
$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.';
48-
$this->phpcsFile->addWarning( $message, $stackPtr, 'FileGetContentsUnknown', $data );
59+
$has_magic_dir = $this->phpcsFile->findNext( T_DIR, $param_start, $search_end );
60+
if ( $has_magic_dir !== false ) {
61+
// In all likelyhood a local file (disregarding creative code).
62+
return;
4963
}
5064

51-
$fileName = $this->tokens[ $fileNameStackPtr ]['content'];
65+
$isRemoteFile = false;
66+
$search_start = $param_start;
67+
// phpcs:ignore Generic.CodeAnalysis.AssignmentInCondition.FoundInWhileCondition -- Valid usage.
68+
while ( ( $has_text_string = $this->phpcsFile->findNext( Tokens::$stringTokens, $search_start, $search_end ) ) !== false ) {
69+
if ( strpos( $this->tokens[ $has_text_string ]['content'], '://' ) !== false ) {
70+
$isRemoteFile = true;
71+
break;
72+
}
73+
74+
$search_start = ( $has_text_string + 1 );
75+
}
5276

53-
$isRemoteFile = ( strpos( $fileName, '://' ) !== false );
5477
if ( $isRemoteFile === true ) {
5578
$message = '`%s()` is highly discouraged for remote requests, please use `wpcom_vip_file_get_contents()` or `vip_safe_wp_remote_get()` instead.';
5679
$this->phpcsFile->addWarning( $message, $stackPtr, 'FileGetContentsRemoteFile', $data );
80+
return;
5781
}
82+
83+
/*
84+
* Okay, so we haven't been able to determine for certain this is a remote file.
85+
* Check for tokens which would make the parameter contents dynamic.
86+
*/
87+
$ignore = Tokens::$emptyTokens;
88+
$ignore += Tokens::$stringTokens;
89+
$ignore += [ T_STRING_CONCAT => T_STRING_CONCAT ];
90+
91+
$has_non_text_string = $this->phpcsFile->findNext( $ignore, $param_start, $search_end, true );
92+
if ( $has_non_text_string !== false ) {
93+
$this->add_contents_unknown_warning( $stackPtr, $data );
94+
}
95+
}
96+
97+
/**
98+
* Process the function if no parameters were found.
99+
*
100+
* {@internal This method is only needed for handling PHP 8.1+ first class callables on WPCS < 3.2.0.
101+
* Once the minimum supported WPCS is 3.2.0 or higher, this method should be removed.}
102+
*
103+
* @param int $stackPtr The position of the current token in the stack.
104+
* @param string $group_name The name of the group which was matched.
105+
* @param string $matched_content The token content (function name) which was matched
106+
* in lowercase.
107+
*
108+
* @return void
109+
*/
110+
public function process_no_parameters( $stackPtr, $group_name, $matched_content ) {
111+
// Check if this is a first class callable.
112+
$next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
113+
if ( $next === false
114+
|| $this->tokens[ $next ]['code'] !== T_OPEN_PARENTHESIS
115+
|| isset( $this->tokens[ $next ]['parenthesis_closer'] ) === false
116+
) {
117+
// Live coding/parse error. Ignore.
118+
return;
119+
}
120+
121+
// First class callable.
122+
$firstNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $next + 1 ), null, true );
123+
if ( $this->tokens[ $firstNonEmpty ]['code'] === T_ELLIPSIS ) {
124+
$secondNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $firstNonEmpty + 1 ), null, true );
125+
if ( $this->tokens[ $secondNonEmpty ]['code'] === T_CLOSE_PARENTHESIS ) {
126+
$this->add_contents_unknown_warning( $stackPtr, [ $matched_content ] );
127+
}
128+
}
129+
}
130+
131+
/**
132+
* Process the function if it is used as a first class callable.
133+
*
134+
* @param int $stackPtr The position of the current token in the stack.
135+
* @param string $group_name The name of the group which was matched.
136+
* @param string $matched_content The token content (function name) which was matched
137+
* in lowercase.
138+
*
139+
* @return void
140+
*/
141+
public function process_first_class_callable( $stackPtr, $group_name, $matched_content ) {
142+
$this->add_contents_unknown_warning( $stackPtr, [ $matched_content ] );
143+
}
144+
145+
/**
146+
* Add a warning if the function is used with unknown parameter(s) or with a $filename parameter for which
147+
* it could not be determined if it references a local file or a remote file.
148+
*
149+
* @param int $stackPtr The position of the current token in the stack.
150+
* @param array<string> $data Data to use for string replacement in the error message.
151+
*
152+
* @return void
153+
*/
154+
private function add_contents_unknown_warning( $stackPtr, $data ) {
155+
$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.';
156+
$this->phpcsFile->addWarning( $message, $stackPtr, 'FileGetContentsUnknown', $data );
58157
}
59158
}

‎WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.inc‎

Lines changed: 84 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,88 @@
11
<?php
22

3-
$file_content = file_get_contents( 'my-file.css' ); // OK.
3+
/*
4+
* Not the sniff target.
5+
*/
6+
use function file_get_contents;
47

5-
$file_content = file_get_contents( 'my-file.svg' ); // OK.
8+
my\ns\file_get_contents($url);
9+
$this->file_get_contents($url);
10+
$this?->file_get_contents($file);
11+
MyClass::file_get_contents($file);
12+
echo FILE_GET_CONTENTS;
13+
namespace\file_get_contents($url);
614

7-
$external_resource = file_get_contents( 'https://example.com' ); // NOK.
15+
16+
/*
17+
* These should all be okay.
18+
*/
19+
$file_content = file_get_contents( 'my-file.css' );
20+
$file_content = \File_Get_Contents( /*comment*/ 'my-file.svg', );
21+
$file_content = file_get_contents( __DIR__ . "/filename.css" );
22+
23+
// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute.
24+
#[File_Get_Contents('text')]
25+
function foo() {}
26+
27+
// Incomplete function call, should be ignored by the sniff.
28+
$live_coding = file_get_contents();
29+
30+
31+
/*
32+
* These should all be flagged with a warning.
33+
*/
34+
// Warning FileGetContentsRemoteFile.
35+
$external_resource = file_get_contents( 'https://example.com', );
36+
\file_get_contents( "http://$url" );
37+
$external_resource = FILE_GET_CONTENTS(
38+
// Sort of protocol relative URL (the ":" is superfluous).
39+
'://example.com'
40+
);
41+
$external_resource = file_get_contents( 'HTTP://example.com', );
42+
43+
// Warning FileGetContentsUnknown.
44+
file_get_contents( $url, true, $context, $offset, $length );
45+
\File_GET_Contents( $obj->get_fileName() );
46+
file_get_contents( MyClass::FILE_NAME, );
47+
48+
file_get_contents(...$params); // PHP 5.6 argument unpacking. Warning FileGetContentsUnknown.
49+
50+
// Safeguard correct handling of function calls using PHP 8.0+ named parameters.
51+
file_get_contents(context: $context, ); // OK, well, not really, missing required $filename param, but that's not the concern of this sniff.
52+
file_get_contents(file: 'https://example.com',); // OK, well, not really, typo in param name, but that's not the concern of the sniff.
53+
file_get_contents(
54+
context: stream_context_create(
55+
options: [
56+
$proxy => 'tcp://proxy.example.com:5100',
57+
],
58+
),
59+
filename: 'my-file.svg',
60+
); // OK.
61+
62+
file_get_contents(
63+
context: stream_context_create(
64+
options: [
65+
$cafile => __DIR__ . '/cacert.pem',
66+
],
67+
),
68+
filename: 'https://example.com'
69+
); // Warning FileGetContentsRemoteFile.
70+
file_get_contents(
71+
context: stream_context_create(
72+
options: [
73+
$cafile => __DIR__ . '/cacert.pem',
74+
],
75+
),
76+
filename: $fileName,
77+
); // Warning FileGetContentsUnknown.
78+
79+
array_walk($files, file_get_contents(...)); // PHP 8.1 first class callable. Warning FileGetContentsUnknown.
80+
81+
// Bug: don't throw a warning when __DIR__ is used as this indicates use with a local file.
82+
\file_get_contents( __DIR__ . $filename ); // OK.
83+
84+
// Bug: check all text string tokens in the parameter.
85+
$result = file_get_contents(( is_ssl() ? 'http' : 'https' ) . '://example.com'); // Warning FileGetContentsRemoteFile.
86+
87+
// Bug: don't presume if the parameter contains a text string token, that will be the only token.
88+
\file_get_contents( $url . '/filename.css' ); // Warning FileGetContentsUnknown.

‎WordPressVIPMinimum/Tests/Performance/FetchingRemoteDataUnitTest.php‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,19 @@ public function getErrorList() {
3232
*/
3333
public function getWarningList() {
3434
return [
35-
7 => 1,
35+
35 => 1,
36+
36 => 1,
37+
37 => 1,
38+
41 => 1,
39+
44 => 1,
40+
45 => 1,
41+
46 => 1,
42+
48 => 1,
43+
62 => 1,
44+
70 => 1,
45+
79 => 1,
46+
85 => 1,
47+
88 => 1,
3648
];
3749
}
3850
}

0 commit comments

Comments
 (0)