BadFunctions/EasyRFI: add unit tests, includes various bug fixes

Security/Sniffs/BadFunctions/EasyRFISniff.php

@@ -7,13 +7,26 @@
 
 class EasyRFISniff implements Sniff {
 
+	/**
+	 * Tokens to search for within an include/require statement.
+	 *
+	 * @var array
+	 */
+	private $search = [];
+
 	/**
 	* Returns the token types that this sniff is interested in.
 	*
 	* @return array(int)
 	*/
 	public function register() {
-		return array(T_INCLUDE, T_INCLUDE_ONCE, T_REQUIRE, T_REQUIRE_ONCE);
+		// Set the $search property.
+		$this->search  = \PHP_CodeSniffer\Util\Tokens::$emptyTokens;
+		$this->search += \PHP_CodeSniffer\Util\Tokens::$bracketTokens;
+		$this->search += \PHPCS_SecurityAudit\Security\Sniffs\Utils::$staticTokens;
+		$this->search[T_STRING_CONCAT] = T_STRING_CONCAT;
+
+		return \PHP_CodeSniffer\Util\Tokens::$includeTokens;
 	}
 
 	/**
@@ -26,24 +39,29 @@ public function register() {
 	* @return void
 	*/
 	public function process(File $phpcsFile, $stackPtr) {
-		$utils = \PHPCS_SecurityAudit\Security\Sniffs\UtilsFactory::getInstance();
+		$closer = $phpcsFile->findNext(array(T_SEMICOLON, T_CLOSE_TAG), ($stackPtr + 1));
+		if ($closer === false) {
+			// Live coding or parse error.
+			return;
+		}
+
+		$utils  = \PHPCS_SecurityAudit\Security\Sniffs\UtilsFactory::getInstance();
 		$tokens = $phpcsFile->getTokens();
-		$s = $phpcsFile->findNext(\PHP_CodeSniffer\Util\Tokens::$emptyTokens, $stackPtr, null, true, null, true);
+		$s      = $stackPtr;
 
-		if ($tokens[$s]['code'] == T_OPEN_PARENTHESIS) {
-			$closer = $tokens[$s]['parenthesis_closer'];
-		} else {
-			$closer = $phpcsFile->findNext(T_SEMICOLON, $stackPtr);
-			$s = $stackPtr;
-		}
-		while ($s) {
-			$s = $phpcsFile->findNext(array_merge(\PHP_CodeSniffer\Util\Tokens::$emptyTokens, \PHP_CodeSniffer\Util\Tokens::$bracketTokens, \PHPCS_SecurityAudit\Security\Sniffs\Utils::$staticTokens), $s + 1, $closer, true);
-			if ($s && $utils::is_token_user_input($tokens[$s])) {
+
+		while (($s = $phpcsFile->findNext($this->search, $s + 1, $closer, true)) !== false) {
+			$data = array(
+				$tokens[$s]['content'],
+				$tokens[$stackPtr]['content'],
+			);
+
+			if ($utils::is_token_user_input($tokens[$s])) {
 				if (\PHP_CodeSniffer\Config::getConfigData('ParanoiaMode') || !$utils::is_token_false_positive($tokens[$s], $tokens[$s+2])) {
-					$phpcsFile->addError('Easy RFI detected because of direct user input with ' . $tokens[$s]['content'] . ' on ' . $tokens[$stackPtr]['content'], $s, 'ErrEasyRFI');
+					$phpcsFile->addError('Easy RFI detected because of direct user input with %s on %s', $s, 'ErrEasyRFI', $data);
 				}
-			} elseif ($s && \PHP_CodeSniffer\Config::getConfigData('ParanoiaMode') && $tokens[$s]['content'] != '.') {
-				$phpcsFile->addWarning('Possible RFI detected with ' . $tokens[$s]['content'] . ' on ' . $tokens[$stackPtr]['content'], $s, 'WarnEasyRFI');
+			} elseif (\PHP_CodeSniffer\Config::getConfigData('ParanoiaMode')) {
+				$phpcsFile->addWarning('Possible RFI detected with %s on %s', $s, 'WarnEasyRFI', $data);
 			}
 		}
 	}

Security/Sniffs/Utils.php

@@ -3,7 +3,12 @@
 class Utils {
 
 	// Tokens that can't containts or use any variables (so no user input)
-	public static $staticTokens = array(T_CONSTANT_ENCAPSED_STRING, T_COMMA, T_LNUMBER, T_DNUMBER);
+	public static $staticTokens = array(
+		T_CONSTANT_ENCAPSED_STRING => T_CONSTANT_ENCAPSED_STRING,
+		T_COMMA                    => T_COMMA,
+		T_LNUMBER                  => T_LNUMBER,
+		T_DNUMBER                  => T_DNUMBER,
+	);
 
 	/**
 	* Heavy used function to verify if a string from a token contains user input

Security/Tests/BadFunctions/EasyRFIUnitTest.0.inc

@@ -0,0 +1,25 @@
+<?php
+
+/*
+ * Paranoia mode = 0.
+ */
+
+// Base.
+include ( 'path/to/' . $_GET['filename'] ); // Error.
+include_once ('path/to/' . "$filename") . '.' . $extension;
+require getenv('PATHTOFILE'); // Error.
+
+// Drupal 7.
+require_once ( 'path/to/' . $form['filename'] );
+include arg(2) . drupal_get_query_parameters()['param'];
+
+// Prevent false positives on safe $_SERVER variables.
+include $_SERVER['DOCUMENT_ROOT'] . '/filename.php';
+
+?>
+<?php include $_POST['path'] ?><!-- Error. -->
+<?php
+echo function_call($param);
+
+// Intentional parse error. This should be the last test in the file.
+require_once

Security/Tests/BadFunctions/EasyRFIUnitTest.1.inc

@@ -0,0 +1,25 @@
+<?php
+
+/*
+ * Paranoia mode = 1.
+ */
+
+// Base.
+include ( 'path/to/' . $_GET['filename'] ); // Error.
+include ('path/to/' . "$filename") . '.' . $extension; // Warning x 2.
+include getenv('PATHTOFILE'); // Error.
+
+// Drupal 7.
+include ( 'path/to/' . $form['filename'] ); // Warning.
+include arg(2) . drupal_get_query_parameters()['param']; // Warning x 2.
+
+// Prevent false positives on safe $_SERVER variables.
+include $_SERVER['DOCUMENT_ROOT'] . '/filename.php'; // Error.
+
+?>
+<?php include $_POST['path'] ?><!-- Error. -->
+<?php
+echo function_call($param);
+
+// Intentional parse error. This should be the last test in the file.
+require $_GET['path']

Security/Tests/BadFunctions/EasyRFIUnitTest.Drupal7.1.inc

@@ -0,0 +1,17 @@
+<?php
+
+/*
+ * Paranoia mode = 1, CmsFramework = Drupal7.
+ */
+
+// Base.
+include ( 'path/to/' . $_GET['filename'] ); // Error.
+include ('path/to/' . "$filename") . '.' . $extension; // Warning x 2.
+include getenv('PATHTOFILE'); // Error.
+
+// Drupal 7.
+include ( 'path/to/' . $form['filename'] ); // Error.
+include arg(2) . drupal_get_query_parameters()['param']; // Error x 2.
+
+// Intentional parse error. This should be the last test in the file.
+include_once ( $_GET['path']

Security/Tests/BadFunctions/EasyRFIUnitTest.php

@@ -0,0 +1,85 @@
+<?php
+
+namespace PHPCS_SecurityAudit\Security\Tests\BadFunctions;
+
+use PHPCS_SecurityAudit\Security\Tests\AbstractSecurityTestCase;
+
+/**
+ * Unit test class for the EasyRFI sniff.
+ *
+ * @covers \PHPCS_SecurityAudit\Security\Sniffs\BadFunctions\EasyRFISniff
+ */
+class EasyRFIUnitTest extends AbstractSecurityTestCase
+{
+
+	/**
+	 * Returns the lines where errors should occur.
+	 *
+	 * The key of the array should represent the line number and the value
+	 * should represent the number of errors that should occur on that line.
+	 *
+	 * @param string $testFile The name of the file being tested.
+	 *
+	 * @return array<int, int>
+	 */
+	public function getErrorList($testFile = '')
+	{
+		switch ($testFile) {
+			case 'EasyRFIUnitTest.0.inc':
+				return [
+					8  => 1,
+					10 => 1,
+					20 => 1,
+				];
+
+			case 'EasyRFIUnitTest.1.inc':
+				return [
+					8  => 1,
+					10 => 1,
+					17 => 1,
+					20 => 1,
+				];
+
+			case 'EasyRFIUnitTest.Drupal7.1.inc':
+				return [
+					8  => 1,
+					10 => 1,
+					13 => 1,
+					14 => 2,
+				];
+
+			default:
+				return [];
+		}
+	}
+
+	/**
+	 * Returns the lines where warnings should occur.
+	 *
+	 * The key of the array should represent the line number and the value
+	 * should represent the number of warnings that should occur on that line.
+	 *
+	 * @param string $testFile The name of the file being tested.
+	 *
+	 * @return array<int, int>
+	 */
+	public function getWarningList($testFile = '')
+	{
+		switch ($testFile) {
+			case 'EasyRFIUnitTest.1.inc':
+				return [
+					9  => 2,
+					13 => 1,
+					14 => 2,
+				];
+
+			case 'EasyRFIUnitTest.Drupal7.1.inc':
+				return [
+					9  => 2,
+				];
+
+			default:
+				return [];
+		}
+	}
+}