Skip to content

Commit 22a787a

Browse files
david-bindadavid-binda
authored
Merge pull request #172 from Automattic/fix-171/file_get_contents_mods
Fix 171/file get contents mods
2 parents 5017a12 + 932dd3a commit 22a787a

10 files changed

+303
-5
lines changed

‎WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
<?php
2+
/**
3+
* WordPressVIPMinimum_Sniffs_Files_IncludingNonPHPFileSniff.
4+
*
5+
* @package VIPCS\WordPressVIPMinimum
6+
*/
7+
8+
namespace WordPressVIPMinimum\Sniffs\Files;
9+
10+
use PHP_CodeSniffer_File as File;
11+
use PHP_CodeSniffer_Tokens as Tokens;
12+
13+
/**
14+
* WordPressVIPMinimum_Sniffs_Files_IncludingNonPHPFileSniff.
15+
*
16+
* Checks that __DIR__, dirname( __FILE__ ) or plugin_dir_path( __FILE__ )
17+
* is used when including or requiring files.
18+
*
19+
* @package VIPCS\WordPressVIPMinimum
20+
*/
21+
class IncludingNonPHPFileSniff implements \PHP_CodeSniffer_Sniff {
22+
23+
/**
24+
* Returns an array of tokens this test wants to listen for.
25+
*
26+
* @return array
27+
*/
28+
public function register() {
29+
return Tokens::$includeTokens;
30+
31+
}//end register()
32+
33+
34+
/**
35+
* Processes this test, when one of its tokens is encountered.
36+
*
37+
* @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned.
38+
* @param int $stackPtr The position of the current token in the
39+
* stack passed in $tokens.
40+
*
41+
* @return void
42+
*/
43+
public function process( File $phpcsFile, $stackPtr ) {
44+
$tokens = $phpcsFile->getTokens();
45+
46+
$curStackPtr = $stackPtr;
47+
while ( $curStackPtr = $phpcsFile->findNext( Tokens::$stringTokens, ( $curStackPtr + 1 ), null, false, null, true ) ) {
48+
49+
if ( T_CONSTANT_ENCAPSED_STRING === $tokens[ $curStackPtr ]['code'] ) {
50+
$stringWithoutEnclosingQuotationMarks = trim( $tokens[ $curStackPtr ]['content'], "\"'" );
51+
} else {
52+
$stringWithoutEnclosingQuotationMarks = $tokens[ $curStackPtr ]['content'];
53+
}
54+
55+
$isFileName = preg_match( '/.*(\.[a-z]{2,})$/i', $stringWithoutEnclosingQuotationMarks, $regexMatches );
56+
57+
if ( false === $isFileName || 0 === $isFileName ) {
58+
continue;
59+
}
60+
61+
$extension = $regexMatches[1];
62+
if ( true === in_array( $extension, array( '.php', '.inc' ), true ) ) {
63+
return;
64+
}
65+
66+
if ( true === in_array( $extension, array( '.svg', '.css' ), true ) ) {
67+
$phpcsFile->addError( sprintf( 'Local SVG and CSS files should be loaded via `get_file_contents` rather than via `%s`.', $tokens[ $stackPtr ]['content'] ), $curStackPtr, 'IncludingSVGCSSFile' );
68+
} else {
69+
$phpcsFile->addError( sprintf( 'Local non-PHP file should be loaded via `get_file_contents` rather than via `%s`', $tokens[ $stackPtr ]['content'] ), $curStackPtr, 'IncludingNonPHPFile' );
70+
}
71+
}
72+
73+
}//end process()
74+
75+
76+
}//end class

‎WordPressVIPMinimum/Sniffs/VIP/FetchingRemoteDataSniff.php‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
<?php
2+
/**
3+
* WordPress-VIP-Minimum Coding Standard.
4+
*
5+
* @package VIPCS\WordPressVIPMinimum
6+
* @link https://github.com/Automattic/VIP-Coding-Standards
7+
*/
8+
9+
namespace WordPressVIPMinimum\Sniffs\VIP;
10+
11+
use PHP_CodeSniffer_File as File;
12+
use PHP_CodeSniffer_Tokens as Tokens;
13+
14+
/**
15+
* Restricts usage of rewrite rules flushing
16+
*
17+
* @package VIPCS\WordPressVIPMinimum
18+
*/
19+
class FetchingRemoteDataSniff implements \PHP_CodeSniffer_Sniff {
20+
21+
/**
22+
* Returns an array of tokens this test wants to listen for.
23+
*
24+
* @return array
25+
*/
26+
public function register() {
27+
return Tokens::$functionNameTokens;
28+
}
29+
30+
/**
31+
* Process this test when one of its tokens is encountered.
32+
*
33+
* @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned.
34+
* @param int $stackPtr The position of the current token in the stack passed in $tokens.
35+
*
36+
* @return void
37+
*/
38+
public function process( File $phpcsFile, $stackPtr ) {
39+
40+
$tokens = $phpcsFile->getTokens();
41+
42+
$functionName = $tokens[ $stackPtr ]['content'];
43+
if ( 'file_get_contents' !== $functionName ) {
44+
return;
45+
}
46+
47+
$fileNameStackPtr = $phpcsFile->findNext( Tokens::$stringTokens, ( $stackPtr + 1 ), null, false, null, true );
48+
if ( false === $fileNameStackPtr ) {
49+
$phpcsFile->addWarning( sprintf( '`%s()` is highly discouraged for remote requests, please use `wpcom_vip_file_get_contents()` or `vip_safe_wp_remote_get()` instead.', $tokens[ $stackPtr ]['content'] ), $stackPtr, 'fileGetContentsUknown' );
50+
}
51+
52+
$fileName = $tokens[ $fileNameStackPtr ]['content'];
53+
54+
$isRemoteFile = ( false !== strpos( $fileName, '://' ) );
55+
if ( true === $isRemoteFile ) {
56+
$phpcsFile->addWarning( sprintf( '`%s()` is highly discouraged for remote requests, please use `wpcom_vip_file_get_contents()` or `vip_safe_wp_remote_get()` instead.', $tokens[ $stackPtr ]['content'] ), $stackPtr, 'fileGetContentsRemoteFile' );
57+
}
58+
}
59+
60+
}

‎WordPressVIPMinimum/Sniffs/VIP/RestrictedFunctionsSniff.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,8 @@ public function getGroups() {
102102
unset( $original_groups[ $restricted ] );
103103
}
104104

105+
unset( $original_groups['file_get_contents'] );
106+
105107
return array_merge( $original_groups, $new_groups );
106108

107109
} // end getGroups().

‎WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
<?php
2+
3+
require_once __DIR__ . "/my_file.php"; // OK.
4+
5+
require_once "my_file.php"; // OK.
6+
7+
include( __DIR__ . "/my_file.php" ); // OK.
8+
9+
include ( MY_CONSTANT . "my_file.php" ); // OK.
10+
11+
require_once ( MY_CONSTANT . "my_file.php" ); // OK.
12+
13+
include( locate_template('index-loop.php') ); // OK.
14+
15+
require_once __DIR__ . "/my_file.svg"; // NOK.
16+
17+
require_once "my_file.svg"; // NOK.
18+
19+
include( __DIR__ . "/my_file.svg" ); // NOK.
20+
21+
include ( MY_CONSTANT . "my_file.svg" ); // NOK.
22+
23+
require_once ( MY_CONSTANT . "my_file.svg" ); // NOK.
24+
25+
include( locate_template('index-loop.svg') ); // NOK.
26+
27+
require_once __DIR__ . "/my_file.css"; // NOK.
28+
29+
require_once "my_file.css"; // NOK.
30+
31+
include( __DIR__ . "/my_file.css" ); // NOK.
32+
33+
include ( MY_CONSTANT . "my_file.css" ); // NOK.
34+
35+
require_once ( MY_CONSTANT . "my_file.css" ); // NOK.
36+
37+
include( locate_template('index-loop.css') ); // NOK.
38+
39+
require_once __DIR__ . "/my_file.csv"; // NOK.
40+
41+
require_once "my_file.inc"; // OK.
42+
43+
include( __DIR__ . "/my_file.csv" ); // NOK.
44+
45+
include ( MY_CONSTANT . "my_file.csv" ); // NOK.
46+
47+
require_once ( MY_CONSTANT . "my_file.csv" ); // NOK.
48+
49+
include( locate_template('index-loop.csv') ); // NOK.
50+
51+
echo file_get_contents( 'index-loop.svg' ); // XSS OK.
52+
53+
echo file_get_contents( 'index-loop.css' ); // XSS OK.

‎WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
<?php
2+
/**
3+
* Unit test class for WordPressVIPMinimum Coding Standard.
4+
*
5+
* @package VIPCS\WordPressVIPMinimum
6+
*/
7+
8+
namespace WordPressVIPMinimum\Tests\Files;
9+
10+
use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;
11+
/**
12+
* Unit test class for the IncludingFile sniff.
13+
*
14+
* @package VIPCS\WordPressVIPMinimum
15+
*/
16+
class IncludingNonPHPFileUnitTest extends AbstractSniffUnitTest {
17+
18+
/**
19+
* Returns the lines where errors should occur.
20+
*
21+
* @return array <int line number> => <int number of errors>
22+
*/
23+
public function getErrorList() {
24+
return array(
25+
15 => 1,
26+
17 => 1,
27+
19 => 1,
28+
21 => 1,
29+
23 => 1,
30+
25 => 1,
31+
27 => 1,
32+
29 => 1,
33+
31 => 1,
34+
33 => 1,
35+
35 => 1,
36+
37 => 1,
37+
39 => 1,
38+
43 => 1,
39+
45 => 1,
40+
47 => 1,
41+
49 => 1,
42+
);
43+
}
44+
45+
/**
46+
* Returns the lines where warnings should occur.
47+
*
48+
* @return array <int line number> => <int number of warnings>
49+
*/
50+
public function getWarningList() {
51+
return array();
52+
}
53+
54+
} // End class.

‎WordPressVIPMinimum/Tests/VIP/FetchingRemoteDataUnitTest.inc‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
<?php
2+
3+
$file_content = file_get_contents( 'my-file.css' ); // OK.
4+
5+
$file_content = file_get_contents( 'my-file.svg' ); // OK.
6+
7+
$external_resource = file_get_contents( 'https://example.com' ); // NOK.

‎WordPressVIPMinimum/Tests/VIP/FetchingRemoteDataUnitTest.php‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
<?php
2+
/**
3+
* Unit test class for WordPressVIPMinimum Coding Standard.
4+
*
5+
* @package VIPCS\WordPressVIPMinimum
6+
*/
7+
8+
namespace WordPressVIPMinimum\Tests\VIP;
9+
10+
use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;
11+
12+
/**
13+
* Unit test class for the ExitAfterRedirect sniff.
14+
*
15+
* @package VIPCS\WordPressVIPMinimum
16+
*/
17+
class FetchingRemoteDataUnitTest extends AbstractSniffUnitTest {
18+
19+
/**
20+
* Returns the lines where errors should occur.
21+
*
22+
* @return array <int line number> => <int number of errors>
23+
*/
24+
public function getErrorList() {
25+
return array();
26+
}
27+
28+
/**
29+
* Returns the lines where warnings should occur.
30+
*
31+
* @return array <int line number> => <int number of warnings>
32+
*/
33+
public function getWarningList() {
34+
return array(
35+
7 => 1,
36+
);
37+
38+
}
39+
40+
} // End class.

‎WordPressVIPMinimum/ruleset.xml‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,9 @@
105105
<!-- https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#use-wp_json_encode-over-json_encode -->
106106
<!-- https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#filesystem-writes -->
107107
<!-- https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#remote-calls -->
108-
<rule ref="WordPress.WP.AlternativeFunctions"/>
108+
<rule ref="WordPress.WP.AlternativeFunctions">
109+
<exclude name="WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents"/>
110+
</rule>
109111
<!-- VIP recommends other functions -->
110112
<rule ref="WordPress.WP.AlternativeFunctions.curl">
111113
<message>Using cURL functions is highly discouraged within VIP context. Check (Fetching Remote Data) on VIP Documentation.</message>
@@ -118,4 +120,4 @@
118120
<type>error</type>
119121
<message>`%1$s()` performs a no-LIMIT query by default, make sure to set a reasonable `posts_per_page`. `%1$s()` will do a -1 query by default, a maximum of 100 should be used.</message>
120122
</rule>
121-
</ruleset>
123+
</ruleset>

‎ruleset_test.inc‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,5 +171,8 @@ wp_safe_redirect( 'https.//vip.wordpress.com' ); // NOK. Error.
171171

172172
is_multi_author(); // NOK. Warning.
173173

174-
?>
174+
include( 'non-php-file.svg' ); // NOK. Error. Including non-php file.
175+
176+
echo file_get_contents( 'non-php-file.svg' ); // XSS OK. Preferred way of including SVG/CSS file. WP_Filesystem Warning.
175177

178+
?> <!-- closing PHP tag should be omitted -->

‎ruleset_test.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@
5050
157 => 1,
5151
166 => 1,
5252
170 => 1,
53-
174 => 1, // Error on the end of the file. When any code is added, bounce this.
53+
174 => 1,
5454
),
5555
'warnings' => array(
5656
9 => 1,
@@ -76,10 +76,11 @@
7676
164 => 1,
7777
168 => 1,
7878
172 => 1,
79+
176 => 1,
7980
),
8081
'messages' => array(
8182
129 => array(
82-
'get_children() performs a no-LIMIT query by default, make sure to set a reasonable posts_per_page. get_children() will do a -1 query by default, a maximum of 100 should be used.',
83+
'`get_children()` performs a no-LIMIT query by default, make sure to set a reasonable `posts_per_page`. `get_children()` will do a -1 query by default, a maximum of 100 should be used.',
8384
),
8485
),
8586
);

0 commit comments

Comments
 (0)