Merge pull request #586 from Automattic/feature/includingnonphpfile-v…

…arious-improvements

WordPressVIPMinimum/Sniffs/Files/IncludingNonPHPFileSniff.php

@@ -7,20 +7,41 @@
 
 namespace WordPressVIPMinimum\Sniffs\Files;
 
-use PHP_CodeSniffer\Files\File;
 use WordPressVIPMinimum\Sniffs\Sniff;
 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
  */
 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,
+		'phar' => 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.
 	 *
@@ -30,7 +51,6 @@ public function register() {
 		return Tokens::$includeTokens;
 	}
 
-
 	/**
 	 * Processes this test, when one of its tokens is encountered.
 	 *
@@ -39,39 +59,47 @@ 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 );
-
-			if ( $this->tokens[ $curStackPtr ]['code'] === T_CONSTANT_ENCAPSED_STRING ) {
-				$stringWithoutEnclosingQuotationMarks = trim( $this->tokens[ $curStackPtr ]['content'], "\"'" );
-			} else {
-				$stringWithoutEnclosingQuotationMarks = $this->tokens[ $curStackPtr ]['content'];
+		$end_of_statement = $this->phpcsFile->findEndOfStatement( $stackPtr );
+		$curStackPtr      = ( $end_of_statement + 1 );
+
+		do {
+			$curStackPtr = $this->phpcsFile->findPrevious( Tokens::$stringTokens, $curStackPtr - 1, $stackPtr );
+			if ( $curStackPtr === false ) {
+				return;
 			}
 
-			$isFileName = preg_match( '/.*(\.[a-z]{2,})$/i', $stringWithoutEnclosingQuotationMarks, $regexMatches );
+			$stringWithoutEnclosingQuotationMarks = trim( $this->tokens[ $curStackPtr ]['content'], "\"'" );
+
+			$isFileName = preg_match( '`\.([a-z]{2,})$`i', $stringWithoutEnclosingQuotationMarks, $regexMatches );
 
-			if ( $isFileName === false || $isFileName === 0 ) {
+			if ( $isFileName !== 1 ) {
 				continue;
 			}
 
-			$extension = $regexMatches[1];
-			if ( in_array( $extension, [ '.php', '.inc' ], true ) === true ) {
+			$extension = strtolower( $regexMatches[1] );
+			if ( isset( $this->php_extensions[ $extension ] ) === true ) {
 				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 ( 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`.';
+				$message = 'Local SVG and CSS files should be loaded via `file_get_contents` rather than via `%s`. Found: %s';
 				$code    = 'IncludingSVGCSSFile';
 			}
 
 			$this->phpcsFile->addError( $message, $curStackPtr, $code, $data );
-		}
+
+			// Don't throw more than one error for any one statement.
+			return;
+
+		} while ( $curStackPtr > $stackPtr );
 	}
 
 }

WordPressVIPMinimum/Tests/Files/IncludingNonPHPFileUnitTest.inc

@@ -2,45 +2,45 @@
 
 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.
+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.
+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.
 
-require_once __DIR__ . "/my_file.css"; // NOK.
+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.
 
 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 __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.
 
@@ -50,4 +50,36 @@ 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.
+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. -->
+<?php
+echo 'some .css'; // OK.
+
+if ((include 'vars.php') == TRUE) {} // OK.
+
+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.
+
+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

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
  *
@@ -41,6 +42,12 @@ public function getErrorList() {
 			45 => 1,
 			47 => 1,
 			49 => 1,
+			61 => 1,
+			63 => 1,
+			69 => 1,
+			72 => 1,
+			75 => 1,
+			77 => 1,
 		];
 	}