From 44e592f86ba20c7d4c908d3c3474b5e1cbce063c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 01:05:24 +0200 Subject: [PATCH 01/15] Files/IncludingNonPHPFile: minor efficiency fix [1] * Move the extensions against which checks are being run to private properties. * Removes the `.` from the part in the regex which is being remembered. Note: The `.` before the extension is still required, it is just no longer _remembered_. * Changes the extension checks from using the relatively expensive `in_array()` to the lightweight `isset()`. --- .../Sniffs/Files/IncludingNonPHPFileSniff.php | 28 +++++++++++++++++-- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php index ba1735c1..cba9925d 100644 --- a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php +++ b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php @@ -21,6 +21,28 @@ */ class IncludingNonPHPFileSniff extends Sniff { + /** + * File extensions used for PHP files. + * + * Files with these extensions are allowed to be `include`d. + * + * @var array Key is the extension, value is irrelevant. + */ + private $php_extensions = [ + 'php' => true, + 'inc' => true, + ]; + + /** + * File extensions used for SVG and CSS files. + * + * @var array Key is the extension, value is irrelevant. + */ + private $svg_css_extensions = [ + 'css' => true, + 'svg' => true, + ]; + /** * Returns an array of tokens this test wants to listen for. * @@ -49,14 +71,14 @@ public function process_token( $stackPtr ) { $stringWithoutEnclosingQuotationMarks = $this->tokens[ $curStackPtr ]['content']; } - $isFileName = preg_match( '/.*(\.[a-z]{2,})$/i', $stringWithoutEnclosingQuotationMarks, $regexMatches ); + $isFileName = preg_match( '/.*\.([a-z]{2,})$/i', $stringWithoutEnclosingQuotationMarks, $regexMatches ); if ( $isFileName === false || $isFileName === 0 ) { continue; } $extension = $regexMatches[1]; - if ( in_array( $extension, [ '.php', '.inc' ], true ) === true ) { + if ( isset( $this->php_extensions[ $extension ] ) === true ) { return; } @@ -64,7 +86,7 @@ public function process_token( $stackPtr ) { $data = [ $this->tokens[ $stackPtr ]['content'] ]; $code = 'IncludingNonPHPFile'; - if ( in_array( $extension, [ '.svg', '.css' ], true ) === true ) { + if ( isset( $this->svg_css_extensions[ $extension ] ) === true ) { // Be more specific for SVG and CSS files. $message = 'Local SVG and CSS files should be loaded via `file_get_contents` rather than via `%s`.'; $code = 'IncludingSVGCSSFile'; From 1f3143ef6e3603a19b8a87fec538ee16665beeef Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 01:07:37 +0200 Subject: [PATCH 02/15] Files/IncludingNonPHPFile: minor efficiency fix [2] The regex will work just the same without the `.*`. --- WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php index cba9925d..7e134582 100644 --- a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php +++ b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php @@ -71,7 +71,7 @@ public function process_token( $stackPtr ) { $stringWithoutEnclosingQuotationMarks = $this->tokens[ $curStackPtr ]['content']; } - $isFileName = preg_match( '/.*\.([a-z]{2,})$/i', $stringWithoutEnclosingQuotationMarks, $regexMatches ); + $isFileName = preg_match( '`\.([a-z]{2,})$`i', $stringWithoutEnclosingQuotationMarks, $regexMatches ); if ( $isFileName === false || $isFileName === 0 ) { continue; From 81f23d8dc8f0140dd7f39912c93c630574098c10 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 01:11:51 +0200 Subject: [PATCH 03/15] Files/IncludingNonPHPFile: improve the existing unit tests * The existing unit tests only tested against `require_once` and `include`, while the sniff looks for all variations. The newly added unit tests make sure that all four variations have at least one positive/negative test associated with it. * Add some capitalization variations to the `include|require[_once]` keywords. --- .../Tests/Files/IncludingNonPHPFileUnitTest.inc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc index ae1e4cf0..d32c71bc 100644 --- a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc @@ -2,25 +2,25 @@ require_once __DIR__ . "/my_file.php"; // OK. -require_once "my_file.php"; // OK. +require "my_file.php"; // OK. -include( __DIR__ . "/my_file.php" ); // OK. +include_once( __DIR__ . "/my_file.php" ); // OK. include ( MY_CONSTANT . "my_file.php" ); // OK. require_once ( MY_CONSTANT . "my_file.php" ); // OK. -include( locate_template('index-loop.php') ); // OK. +Include( locate_template('index-loop.php') ); // OK. require_once __DIR__ . "/my_file.svg"; // NOK. -require_once "my_file.svg"; // NOK. +Require_Once "my_file.svg"; // NOK. include( __DIR__ . "/my_file.svg" ); // NOK. include ( MY_CONSTANT . "my_file.svg" ); // NOK. -require_once ( MY_CONSTANT . "my_file.svg" ); // NOK. +require ( MY_CONSTANT . "my_file.svg" ); // NOK. include( locate_template('index-loop.svg') ); // NOK. @@ -28,7 +28,7 @@ require_once __DIR__ . "/my_file.css"; // NOK. require_once "my_file.css"; // NOK. -include( __DIR__ . "/my_file.css" ); // NOK. +include_once( __DIR__ . "/my_file.css" ); // NOK. include ( MY_CONSTANT . "my_file.css" ); // NOK. @@ -36,7 +36,7 @@ require_once ( MY_CONSTANT . "my_file.css" ); // NOK. include( locate_template('index-loop.css') ); // NOK. -require_once __DIR__ . "/my_file.csv"; // NOK. +REQUIRE_ONCE __DIR__ . "/my_file.csv"; // NOK. require_once "my_file.inc"; // OK. @@ -50,4 +50,4 @@ include( locate_template('index-loop.csv') ); // NOK. echo file_get_contents( 'index-loop.svg' ); // XSS OK. -echo file_get_contents( 'index-loop.css' ); // XSS OK. \ No newline at end of file +echo file_get_contents( 'index-loop.css' ); // XSS OK. From 0d8b86b9b1e22c36829bd62c0a95898bece23ea8 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 01:18:20 +0200 Subject: [PATCH 04/15] Files/IncludingNonPHPFile: improve the error message * Make sure the `include|require[_once]` keyword is always referred to in lower case in the error message. * Make the error message more informative by displaying the text snippet which triggered the error. --- .../Sniffs/Files/IncludingNonPHPFileSniff.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php index 7e134582..52241741 100644 --- a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php +++ b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php @@ -82,13 +82,16 @@ public function process_token( $stackPtr ) { return; } - $message = 'Local non-PHP file should be loaded via `file_get_contents` rather than via `%s`.'; - $data = [ $this->tokens[ $stackPtr ]['content'] ]; + $message = 'Local non-PHP file should be loaded via `file_get_contents` rather than via `%s`. Found: %s'; + $data = [ + strtolower( $this->tokens[ $stackPtr ]['content'] ), + $this->tokens[ $curStackPtr ]['content'], + ]; $code = 'IncludingNonPHPFile'; if ( isset( $this->svg_css_extensions[ $extension ] ) === true ) { // Be more specific for SVG and CSS files. - $message = 'Local SVG and CSS files should be loaded via `file_get_contents` rather than via `%s`.'; + $message = 'Local SVG and CSS files should be loaded via `file_get_contents` rather than via `%s`. Found: %s'; $code = 'IncludingSVGCSSFile'; } From 8693ccb3b46742b787e8970e6779ce36b9c593cd Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 01:25:40 +0200 Subject: [PATCH 05/15] Files/IncludingNonPHPFile: bug fix [1] - case sensitivity of file extension The regex used to retrieve an extension, would retrieve it case-insensitively, but the previous `in_array()` check (now `isset()`), checked the extension in a case-sensitive manner. Whether the file extension is used in upper, lower or mixed case, does not matter to the include as long as a matching file is found, so the sniff should check the extension in a case insensitive manner. As things were, a file name like `file.PHP` would trigger a false positive for the `IncludingNonPHPFile` error and a file called `file.Css` would incorrectly throw the `IncludingNonPHPFile` error instead of the `IncludingSVGCSSFile` error which it should have thrown. Fixed now. Tested by adjusting some of the existing unit tests. --- .../Sniffs/Files/IncludingNonPHPFileSniff.php | 2 +- .../Tests/Files/IncludingNonPHPFileUnitTest.inc | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php index 52241741..6db56e01 100644 --- a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php +++ b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php @@ -77,7 +77,7 @@ public function process_token( $stackPtr ) { continue; } - $extension = $regexMatches[1]; + $extension = strtolower( $regexMatches[1] ); if ( isset( $this->php_extensions[ $extension ] ) === true ) { return; } diff --git a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc index d32c71bc..aa7a62ff 100644 --- a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc @@ -6,13 +6,13 @@ require "my_file.php"; // OK. include_once( __DIR__ . "/my_file.php" ); // OK. -include ( MY_CONSTANT . "my_file.php" ); // OK. +include ( MY_CONSTANT . "my_file.INC" ); // OK. require_once ( MY_CONSTANT . "my_file.php" ); // OK. -Include( locate_template('index-loop.php') ); // OK. +Include( locate_template('index-loop.PHP') ); // OK. -require_once __DIR__ . "/my_file.svg"; // NOK. +require_once __DIR__ . "/my_file.SVG"; // NOK. Require_Once "my_file.svg"; // NOK. @@ -24,7 +24,7 @@ require ( MY_CONSTANT . "my_file.svg" ); // NOK. include( locate_template('index-loop.svg') ); // NOK. -require_once __DIR__ . "/my_file.css"; // NOK. +require_once __DIR__ . "/my_file.CSS"; // NOK. require_once "my_file.css"; // NOK. @@ -34,13 +34,13 @@ include ( MY_CONSTANT . "my_file.css" ); // NOK. require_once ( MY_CONSTANT . "my_file.css" ); // NOK. -include( locate_template('index-loop.css') ); // NOK. +include( locate_template('index-loop.Css') ); // NOK. REQUIRE_ONCE __DIR__ . "/my_file.csv"; // NOK. require_once "my_file.inc"; // OK. -include( __DIR__ . "/my_file.csv" ); // NOK. +include( __DIR__ . "/my_file.CSV" ); // NOK. include ( MY_CONSTANT . "my_file.csv" ); // NOK. From 939d4279a6805658986d51d0a3d1d3abb2cd25b8 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 00:10:33 +0200 Subject: [PATCH 06/15] Files/IncludingNonPHPFile: bug fix [2] - allow for "phar" file extension PHAR (PHP Archive) files should be regarded as PHP files for the purposes of this sniff. Includes unit tests. Fixes 478 --- .../Sniffs/Files/IncludingNonPHPFileSniff.php | 5 +++-- .../Tests/Files/IncludingNonPHPFileUnitTest.inc | 4 ++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php index 6db56e01..b93f4f95 100644 --- a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php +++ b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php @@ -29,8 +29,9 @@ class IncludingNonPHPFileSniff extends Sniff { * @var array Key is the extension, value is irrelevant. */ private $php_extensions = [ - 'php' => true, - 'inc' => true, + 'php' => true, + 'inc' => true, + 'phar' => true, ]; /** diff --git a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc index aa7a62ff..df9f8687 100644 --- a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc @@ -51,3 +51,7 @@ include( locate_template('index-loop.csv') ); // NOK. echo file_get_contents( 'index-loop.svg' ); // XSS OK. echo file_get_contents( 'index-loop.css' ); // XSS OK. + +include_once 'path/to/geoip.phar'; // OK. + +require dirname(__DIR__) . '/externals/aws-sdk.phar'; // OK. From 69949ba2476acaee909fbe979b973b7603d4b380 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 01:51:32 +0200 Subject: [PATCH 07/15] Files/IncludingNonPHPFile: bug fix [3] - extensions in interpolated strings The sniff looks for `Tokens::$stringTokens` when examining the statement. The `Tokens::$stringTokens` array contains two tokens: 1. `T_CONSTANT_ENCAPSED_STRING` - these are either single or double quoted text strings without variables within them. 2. `T_DOUBLE_QUOTED_STRING` - these are double quoted text strings with interpolated variables in the text string. As things were, the quotes would be stripped off the first, but not off the contents of `T_DOUBLE_QUOTED_STRING`-type tokens. As the regex matches extensions at the very end of the text string and doesn't allow for a double quote at the end, this effectively meant that text in double quoted strings would never be recognized as containing a non-PHP extension. (false negative) Includes unit tests. --- .../Sniffs/Files/IncludingNonPHPFileSniff.php | 6 +----- .../Tests/Files/IncludingNonPHPFileUnitTest.inc | 6 ++++++ .../Tests/Files/IncludingNonPHPFileUnitTest.php | 2 ++ 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php index b93f4f95..0e0dbf38 100644 --- a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php +++ b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php @@ -66,11 +66,7 @@ public function process_token( $stackPtr ) { while ( $this->phpcsFile->findNext( Tokens::$stringTokens, $curStackPtr + 1, null, false, null, true ) !== false ) { $curStackPtr = $this->phpcsFile->findNext( Tokens::$stringTokens, $curStackPtr + 1, null, false, null, true ); - if ( $this->tokens[ $curStackPtr ]['code'] === T_CONSTANT_ENCAPSED_STRING ) { - $stringWithoutEnclosingQuotationMarks = trim( $this->tokens[ $curStackPtr ]['content'], "\"'" ); - } else { - $stringWithoutEnclosingQuotationMarks = $this->tokens[ $curStackPtr ]['content']; - } + $stringWithoutEnclosingQuotationMarks = trim( $this->tokens[ $curStackPtr ]['content'], "\"'" ); $isFileName = preg_match( '`\.([a-z]{2,})$`i', $stringWithoutEnclosingQuotationMarks, $regexMatches ); diff --git a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc index df9f8687..5e64a41e 100644 --- a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc @@ -55,3 +55,9 @@ echo file_get_contents( 'index-loop.css' ); // XSS OK. include_once 'path/to/geoip.phar'; // OK. require dirname(__DIR__) . '/externals/aws-sdk.phar'; // OK. + +require "$path/$file.inc"; // OK. + +require "$path/$file.css"; // NOK. + +include_once $path . '/' . "$file.js"; // NOK. diff --git a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php index ad139cc6..ef07949b 100644 --- a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php +++ b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php @@ -41,6 +41,8 @@ public function getErrorList() { 45 => 1, 47 => 1, 49 => 1, + 61 => 1, + 63 => 1, ]; } From b04c016af6135ba7b09e0568a2f902de6bd91904 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 02:05:34 +0200 Subject: [PATCH 08/15] Files/IncludingNonPHPFile: bug fix [4] - fix bleed through /end of statement An `include`/`require` statement can be used within template files to include another template. In that case, the statement can be ended by a PHP close tag without there ever being a semi-colon. Even though the `findNext()` function call indicated that it only wants to look "locally", the `findNext()` method itself does not take a PHP close tag into account when determining whether the statement has ended. This could be considered a bug upstream, but that's another matter. In practice, this meant that when an `include|require[_once]` statement ended on a PHP close tag, the `findNext()` would continue looking for text string tokens and could report an error on a text string which is unrelated to the `include|require[_once]` statement. (false positive). Determining the end of the statement properly before calling `findNext()` fixes this, however, the `findEndOfStatement()` method will return the last non-whitespace token in a statement, which for statements nested in brackets can be a token which is _part_ of the statement. As the `findNext()` method does not look at the `$end` token, we need to `+1` the result of `findEndOfStatement()` to make sure the whole statement is considered. Includes unit tests. --- .../Sniffs/Files/IncludingNonPHPFileSniff.php | 7 ++++--- .../Tests/Files/IncludingNonPHPFileUnitTest.inc | 15 ++++++++++++++- .../Tests/Files/IncludingNonPHPFileUnitTest.php | 2 ++ 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php index 0e0dbf38..39b80a18 100644 --- a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php +++ b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php @@ -62,9 +62,10 @@ public function register() { * @return void */ public function process_token( $stackPtr ) { - $curStackPtr = $stackPtr; - while ( $this->phpcsFile->findNext( Tokens::$stringTokens, $curStackPtr + 1, null, false, null, true ) !== false ) { - $curStackPtr = $this->phpcsFile->findNext( Tokens::$stringTokens, $curStackPtr + 1, null, false, null, true ); + $curStackPtr = $stackPtr; + $end_of_statement = $this->phpcsFile->findEndOfStatement( $stackPtr ); + while ( $this->phpcsFile->findNext( Tokens::$stringTokens, $curStackPtr + 1, $end_of_statement + 1 ) !== false ) { + $curStackPtr = $this->phpcsFile->findNext( Tokens::$stringTokens, $curStackPtr + 1, $end_of_statement + 1 ); $stringWithoutEnclosingQuotationMarks = trim( $this->tokens[ $curStackPtr ]['content'], "\"'" ); diff --git a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc index 5e64a41e..d85097bb 100644 --- a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc @@ -60,4 +60,17 @@ require "$path/$file.inc"; // OK. require "$path/$file.css"; // NOK. -include_once $path . '/' . "$file.js"; // NOK. +include_once $path . '/' . "$file.js" ?> + 1, 61 => 1, 63 => 1, + 69 => 1, + 72 => 1, ]; } From 8e713d7907f6a3d70df91fd459a4332e9ecb0633 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 02:32:26 +0200 Subject: [PATCH 09/15] Files/IncludingNonPHPFile: efficiency fix [3] To prevent an assignment in condition, the same function call was executed twice. By changing the code around slightly and using a `do - while` loop instead of a `while`, the duplicate function call can be removed. --- .../Sniffs/Files/IncludingNonPHPFileSniff.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php index 39b80a18..39227cfa 100644 --- a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php +++ b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php @@ -64,8 +64,12 @@ public function register() { public function process_token( $stackPtr ) { $curStackPtr = $stackPtr; $end_of_statement = $this->phpcsFile->findEndOfStatement( $stackPtr ); - while ( $this->phpcsFile->findNext( Tokens::$stringTokens, $curStackPtr + 1, $end_of_statement + 1 ) !== false ) { - $curStackPtr = $this->phpcsFile->findNext( Tokens::$stringTokens, $curStackPtr + 1, $end_of_statement + 1 ); + + do { + $curStackPtr = $this->phpcsFile->findNext( Tokens::$stringTokens, $curStackPtr + 1, $end_of_statement + 1); + if ( $curStackPtr === false ) { + return; + } $stringWithoutEnclosingQuotationMarks = trim( $this->tokens[ $curStackPtr ]['content'], "\"'" ); @@ -94,7 +98,7 @@ public function process_token( $stackPtr ) { } $this->phpcsFile->addError( $message, $curStackPtr, $code, $data ); - } + } while ( $curStackPtr <= $end_of_statement ); } } From 3cc9dca43f1a2eac9e022dbd67607c40c3b50f5b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 02:59:46 +0200 Subject: [PATCH 10/15] Files/IncludingNonPHPFile: efficiency fix [4]/bug fix [5] - extension at end of statement An `include|require[_once]` statement can only include one (1) file and the file extension of that file will normally be at the _end_ of the statement. So far, the sniff would walk from the start of the statement to the end, while walking from the end of the statement to the start, will get us a result more quickly and more accurately. Includes unit tests. The first would be a false negative, the second a false positive without this fix. --- .../Sniffs/Files/IncludingNonPHPFileSniff.php | 6 +++--- .../Tests/Files/IncludingNonPHPFileUnitTest.inc | 3 +++ .../Tests/Files/IncludingNonPHPFileUnitTest.php | 1 + 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php index 39227cfa..e6b99b68 100644 --- a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php +++ b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php @@ -62,11 +62,11 @@ public function register() { * @return void */ public function process_token( $stackPtr ) { - $curStackPtr = $stackPtr; $end_of_statement = $this->phpcsFile->findEndOfStatement( $stackPtr ); + $curStackPtr = ( $end_of_statement + 1 ); do { - $curStackPtr = $this->phpcsFile->findNext( Tokens::$stringTokens, $curStackPtr + 1, $end_of_statement + 1); + $curStackPtr = $this->phpcsFile->findPrevious( Tokens::$stringTokens, $curStackPtr - 1, $stackPtr ); if ( $curStackPtr === false ) { return; } @@ -98,7 +98,7 @@ public function process_token( $stackPtr ) { } $this->phpcsFile->addError( $message, $curStackPtr, $code, $data ); - } while ( $curStackPtr <= $end_of_statement ); + } while ( $curStackPtr > $stackPtr ); } } diff --git a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc index d85097bb..1cccee42 100644 --- a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc @@ -71,6 +71,9 @@ if ((include 'vars.svg') && $imported_var === 'foo') {} // NOK. require ( dirname(__FILE__) . '/path' ) . 'concatafterparentheses.php'; // OK. require ( dirname(__FILE__) . '/path' ) . 'concatafterparentheses.py'; // NOK. +include_once '/src.css' . DIRECTORY_SEPARATOR . $subdir . '/filename.php'; // OK. +include_once '/src.php' . DIRECTORY_SEPARATOR . $subdir . '/filename.css'; // NOK. + // Live coding. // Intentional parse error. This has to be the last test in the file. include $path . 'filename.css diff --git a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php index 5902c598..ebb0075b 100644 --- a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php +++ b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php @@ -45,6 +45,7 @@ public function getErrorList() { 63 => 1, 69 => 1, 72 => 1, + 75 => 1, ]; } From 5f4de3ed4d5c8e8e19aaf96c69f5fe4dcf51f2ee Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 02:47:31 +0200 Subject: [PATCH 11/15] Files/IncludingNonPHPFile: efficiency fix [5]/bug fix [6] - one error per statement An `include|require[_once]` statement can only include one (1) file, so there should only ever be one file extension in the statement and by extension, only ever be one error for any particular `include|require[_once]` statement. Once a file extension has been found which generates an error, we can therefore stop sniffing that statement. This is similar to the sniff breaking off checking the statement once a text string with `.php` as a file extension is found, as was already done. Includes unit tests. --- WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php | 4 ++++ .../Tests/Files/IncludingNonPHPFileUnitTest.inc | 2 ++ .../Tests/Files/IncludingNonPHPFileUnitTest.php | 1 + 3 files changed, 7 insertions(+) diff --git a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php index e6b99b68..bf51b2b2 100644 --- a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php +++ b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php @@ -98,6 +98,10 @@ public function process_token( $stackPtr ) { } $this->phpcsFile->addError( $message, $curStackPtr, $code, $data ); + + // Don't throw more than one error for any one statement. + return; + } while ( $curStackPtr > $stackPtr ); } diff --git a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc index 1cccee42..f4bb68c3 100644 --- a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc @@ -74,6 +74,8 @@ require ( dirname(__FILE__) . '/path' ) . 'concatafterparentheses.py'; // NOK. include_once '/src.css' . DIRECTORY_SEPARATOR . $subdir . '/filename.php'; // OK. include_once '/src.php' . DIRECTORY_SEPARATOR . $subdir . '/filename.css'; // NOK. +include_once '/src.csv' . DIRECTORY_SEPARATOR . $subdir . '/filename.png'; // NOK. + // Live coding. // Intentional parse error. This has to be the last test in the file. include $path . 'filename.css diff --git a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php index ebb0075b..ff016f30 100644 --- a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php +++ b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php @@ -46,6 +46,7 @@ public function getErrorList() { 69 => 1, 72 => 1, 75 => 1, + 77 => 1, ]; } From 21138e17575e3e10185600b3ba5b52b83bcfd927 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 03:36:43 +0200 Subject: [PATCH 12/15] Files/IncludingNonPHPFile: add some additional unit tests ... to document the behaviour of the sniff for certain situations. --- .../Tests/Files/IncludingNonPHPFileUnitTest.inc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc index f4bb68c3..b4226a25 100644 --- a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc @@ -76,6 +76,10 @@ include_once '/src.php' . DIRECTORY_SEPARATOR . $subdir . '/filename.css'; // NO include_once '/src.csv' . DIRECTORY_SEPARATOR . $subdir . '/filename.png'; // NOK. +include 'http://www.example.com/file.php?foo=1&bar=2'; // OK - this sniff is not about remote includes. + +require __DIR__ . '/' . $subdir . '/' . $filename . '.' . $extension; // OK - undetermined. + // Live coding. // Intentional parse error. This has to be the last test in the file. include $path . 'filename.css From d00943682dba0e8b32eef0ca290f3f1ce09da5a1 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 15:05:46 +0200 Subject: [PATCH 13/15] Files/IncludingNonPHPFile: remove unused `use` statement --- WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php | 1 - 1 file changed, 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php index bf51b2b2..95dce91f 100644 --- a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php +++ b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php @@ -7,7 +7,6 @@ namespace WordPressVIPMinimum\Sniffs\Files; -use PHP_CodeSniffer\Files\File; use WordPressVIPMinimum\Sniffs\Sniff; use PHP_CodeSniffer\Util\Tokens; From 06fe6108847e67731d354e20ba308ff87be6aca0 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 03:22:20 +0200 Subject: [PATCH 14/15] Files/IncludingNonPHPFile: minor simplification `preg_match()` will return `false` for an invalid regex, `0` for "no match" and `1` for a match as it only looks for one match - in contrast to `preg_match_all()`-. As the regex uses an "end of text string" anchor, using `preg_match()` is the right function, but that also means we can simplify the return value check a little. --- WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php index 95dce91f..2022d5d6 100644 --- a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php +++ b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php @@ -74,7 +74,7 @@ public function process_token( $stackPtr ) { $isFileName = preg_match( '`\.([a-z]{2,})$`i', $stringWithoutEnclosingQuotationMarks, $regexMatches ); - if ( $isFileName === false || $isFileName === 0 ) { + if ( $isFileName !== 1 ) { continue; } From 1b4fccbf9dfc68263addd02f26d90e7d3fb2a564 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Sep 2020 00:11:37 +0200 Subject: [PATCH 15/15] Files/IncludingNonPHPFile: fix class docblocks The existing text was unrelated to the actual sniff. Probably legacy copy/paste. --- .../Sniffs/Files/IncludingNonPHPFileSniff.php | 6 ++---- .../Tests/Files/IncludingNonPHPFileUnitTest.php | 3 ++- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php index 2022d5d6..de30c6e5 100644 --- a/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php +++ b/WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php @@ -11,10 +11,9 @@ use PHP_CodeSniffer\Util\Tokens; /** - * WordPressVIPMinimum_Sniffs_Files_IncludingNonPHPFileSniff. + * Ensure that non-PHP files are included via `file_get_contents()` instead of using `include/require[_once]`. * - * Checks that __DIR__, dirname( __FILE__ ) or plugin_dir_path( __FILE__ ) - * is used when including or requiring files. + * This prevents potential PHP code embedded in those files from being automatically executed. * * @package VIPCS\WordPressVIPMinimum */ @@ -52,7 +51,6 @@ public function register() { return Tokens::$includeTokens; } - /** * Processes this test, when one of its tokens is encountered. * diff --git a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php index ff016f30..f16c6c2b 100644 --- a/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php +++ b/WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.php @@ -8,8 +8,9 @@ namespace WordPressVIPMinimum\Tests\Files; use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest; + /** - * Unit test class for the IncludingFile sniff. + * Unit test class for the IncludingNonPHPFile sniff. * * @package VIPCS\WordPressVIPMinimum *