Merge pull request #70 from jrfnl/feature/initial-unit-test-setup

Initial unit test setup, including tests for the Backticks sniff

Author: jmarcil

.gitattributes

@@ -8,6 +8,9 @@
 /.gitattributes export-ignore
 /.gitignore export-ignore
 /.travis.yml export-ignore
+/phpunit.xml.dist export-ignore
+/phpunit-bootstrap.php export-ignore
+/Security/Tests/ export-ignore
 
 #
 # Auto detect text files and perform LF normalization

.gitignore

@@ -1,2 +1,4 @@
 /vendor
 composer.lock
+/phpunit.xml
+/.phpunit.result.cache

.travis.yml

@@ -13,13 +13,30 @@ cache:
 
 php:
   - 5.4
-  - 7.4
-  - "nightly"
+  - 5.5
+  - 5.6
+  - 7.0
+  - 7.1
+  - 7.2
+
+env:
+  jobs:
+    # `master`
+    - PHPCS_VERSION="dev-master" LINT=1
+    # Lowest supported PHPCS version.
+    - PHPCS_VERSION="3.1.0"
 
 # Define the stages used.
+# For non-PRs, only the sniff and quicktest stages are run.
+# For pull requests and merges, the full script is run (skipping quicktest).
+# Note: for pull requests, "master" is the base branch name.
+# See: https://docs.travis-ci.com/user/conditions-v1
 stages:
   - name: sniff
+  - name: quicktest
+    if: type = push AND branch NOT IN (master)
   - name: test
+    if: branch IN (master)
 
 jobs:
   fast_finish: true
@@ -48,6 +65,37 @@ jobs:
         - diff -B ./example_base_ruleset.xml <(xmllint --format "./example_base_ruleset.xml")
         - diff -B ./example_drupal7_ruleset.xml <(xmllint --format "./example_drupal7_ruleset.xml")
 
+    #### QUICK TEST STAGE ####
+    # This is a much quicker test which only runs the unit tests and linting against the low/high
+    # supported PHP/PHPCS combinations.
+    - stage: quicktest
+      php: 7.4
+      env: PHPCS_VERSION="dev-master" LINT=1
+    - php: 7.2
+      # PHP 7.3 is only supported since PHPCS 3.3.1, PHP 7.4 since PHPCS 3.5.0, so running low against PHP 7.2.
+      env: PHPCS_VERSION="3.1.0"
+
+    - php: 5.4
+      env: PHPCS_VERSION="dev-master" LINT=1
+    - php: 5.4
+      env: PHPCS_VERSION="3.1.0"
+
+    #### TEST STAGE ####
+    # Additional builds to prevent issues with PHPCS versions incompatible with certain PHP versions.
+    # PHP 7.3 is only supported since PHPCS 3.3.1, PHP 7.4 since PHPCS 3.5.0.
+    - stage: test
+      php: 7.4
+      env: PHPCS_VERSION="dev-master" LINT=1
+    - php: 7.4
+      env: PHPCS_VERSION="3.5.0"
+    - php: 7.3
+      env: PHPCS_VERSION="dev-master" LINT=1
+    - php: 7.3
+      env: PHPCS_VERSION="3.3.1"
+
+    - php: "nightly"
+      env: PHPCS_VERSION="n/a" LINT=1
+
   allow_failures:
     # Allow failures for unstable builds.
     - php: "nightly"
@@ -58,9 +106,35 @@ before_install:
 
   - export XMLLINT_INDENT="    "
 
+  # On stable PHPCS versions, allow for PHP deprecation notices.
+  # Unit tests don't need to fail on those for stable releases where those issues won't get fixed anymore.
+  - |
+    if [[ "$TRAVIS_BUILD_STAGE_NAME" != "Sniff" && $PHPCS_BRANCH != "dev-master" && "$PHPCS_VERSION" != "n/a" ]]; then
+      echo 'error_reporting = E_ALL & ~E_DEPRECATED' >> ~/.phpenv/versions/$(phpenv version-name)/etc/conf.d/travis.ini
+    fi
+
+install:
+  # Set up test environment using Composer.
+  - |
+    if [[ $PHPCS_VERSION != "n/a" ]]; then
+      composer require --no-update --no-scripts squizlabs/php_codesniffer:${PHPCS_VERSION}
+    fi
+  - |
+    if [[ "$TRAVIS_BUILD_STAGE_NAME" == "Sniff" || $PHPCS_VERSION == "n/a" ]]; then
+      # The sniff stage doesn't run the unit tests, so no need for PHPUnit.
+      # The build on nightly also doesn't run the tests (yet).
+      composer remove --dev phpunit/phpunit --no-update --no-scripts
+    fi
+
   # --prefer-dist will allow for optimal use of the travis caching ability.
   - composer install --prefer-dist --no-suggest
 
 script:
   # Lint PHP files against parse errors.
-  - composer lint
+  - if [[ "$LINT" == "1" ]]; then composer lint; fi
+
+  # Run the unit tests.
+  - |
+    if [[ $PHPCS_VERSION != "n/a" ]]; then
+      composer test
+    fi

README.md

@@ -31,7 +31,7 @@ phpcs-security-audit in its beginning was backed by Pheromone (later on named Fl
 Install
 -------
 
-Requires [PHP CodeSniffer](http://pear.php.net/package/PHP_CodeSniffer/) version 3.x with PHP 5.4 or higher.
+Requires [PHP CodeSniffer](http://pear.php.net/package/PHP_CodeSniffer/) version 3.1.0 or higher with PHP 5.4 or higher.
 
 The easiest way to install is using [Composer](https://getcomposer.org/):
 ```bash

Security/Sniffs/BadFunctions/BackticksSniff.php

@@ -26,20 +26,20 @@ public function register() {
 	* @return void
 	*/
 	public function process(File $phpcsFile, $stackPtr) {
-		$utils = \PHPCS_SecurityAudit\Security\Sniffs\UtilsFactory::getInstance();
-		$tokens = $phpcsFile->getTokens();
-        $closer = $phpcsFile->findNext(T_BACKTICK, $stackPtr + 1, null, false, null, true);
+		$closer = $phpcsFile->findNext(T_BACKTICK, $stackPtr + 1, null, false, null, true);
 		if (!$closer) {
 			return;
 		}
-        $s = $stackPtr + 1;
-		$s = $phpcsFile->findNext(T_VARIABLE, $s, $closer);
-        if ($s) {
+
+		$utils  = \PHPCS_SecurityAudit\Security\Sniffs\UtilsFactory::getInstance();
+		$tokens = $phpcsFile->getTokens();
+		$s      = $stackPtr;
+		while (($s = $phpcsFile->findNext(T_VARIABLE, ($s + 1), $closer)) !== false) {
 			$msg = 'System execution with backticks detected with dynamic parameter';
 			if ($utils::is_token_user_input($tokens[$s])) {
-				$phpcsFile->addError($msg . ' directly from user input', $stackPtr, 'ErrSystemExec');
+				$phpcsFile->addError($msg . ' directly from user input', $s, 'ErrSystemExec');
 			} else {
-				$phpcsFile->addWarning($msg, $stackPtr, 'WarnSystemExec');
+				$phpcsFile->addWarning($msg, $s, 'WarnSystemExec');
 			}
 		}
 

Security/Tests/AbstractSecurityTestCase.php

@@ -0,0 +1,86 @@
+<?php
+/**
+ * An abstract class that all Security sniff unit tests must extend.
+ *
+ * A sniff unit test checks a .inc file for expected violations of a single
+ * coding standard. Expected errors and warnings that are not found, as well as
+ * unexpected warnings and errors, are considered test failures.
+ *
+ * This class will take care of setting the configuration variables in PHP_CodeSniffer
+ * needed to test all relevant configuration combinations for each sniff in
+ * the Security standard.
+ *
+ * The configuration variables set are based on the file name of a test case file.
+ *
+ * Naming conventions for the test case files:
+ * SniffNameUnitTest[.CmsFramework][.ParanoiaMode].inc
+ *
+ * Both `[.CmsFramework]` as well as `[.ParanoiaMode]` are optional.
+ * If neither is set, the defaults of no CmsFramework and Paranoia level 0 will be used.
+ *
+ * Separate test case files for different paranoia levels and different frameworks are
+ * only needed if the sniff behaves differently based on these settings.
+ *
+ * - If the sniff behaviour is the same all round, just having one plain `SniffNameUnitTest.inc`
+ *   test case file will be sufficient.
+ * - If the sniff behaviour is only dependent on one of the two configuration settings,
+ *   the other can be left out.
+ *   Examples:
+ *   - Sniff behaviour only depends on `ParanoiaMode`: `SniffNameUnitTest.[01].inc`.
+ *   - Sniff behaviour only depends on `CmsFramework`: `SniffNameUnitTest.[CmsFramework].inc`.
+ */
+
+namespace PHPCS_SecurityAudit\Security\Tests;
+
+use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;
+
+abstract class AbstractSecurityTestCase extends AbstractSniffUnitTest
+{
+
+	/**
+	 * Get a list of CLI values to set before the file is tested.
+	 *
+	 * @param string                  $filename The name of the file being tested.
+	 * @param \PHP_CodeSniffer\Config $config   The config data for the run.
+	 *
+	 * @return void
+	 */
+	public function setCliValues($filename, $config)
+	{
+		// Set paranoia level.
+		$paranoia = substr($filename, (strlen($filename) - 5), 1);
+		if ($paranoia === '1') {
+			$config->setConfigData('ParanoiaMode', 1, true);
+		} else {
+			$config->setConfigData('ParanoiaMode', 0, true);
+		}
+
+		// Set the CMS Framework if necessary.
+		$firstDot    = strpos($filename, '.');
+		$firstOffset = ($firstDot + 1);
+		$secondDot   = strpos($filename, '.', $firstOffset);
+
+		$extendedExtension = '';
+		if ($secondDot !== false) {
+			$extendedExtension = substr($filename, $firstOffset, ($secondDot - $firstOffset));
+		}
+
+		switch ($extendedExtension) {
+			case 'Drupal7':
+				$config->setConfigData('CmsFramework', 'Drupal7', true);
+				break;
+
+			case 'Drupal8':
+				$config->setConfigData('CmsFramework', 'Drupal8', true);
+				break;
+
+			case 'Symfony2':
+				$config->setConfigData('CmsFramework', 'Symfony2', true);
+				break;
+
+			default:
+				$config->setConfigData('CmsFramework', null, true);
+				break;
+		}
+	}
+}

Security/Tests/BadFunctions/BackticksUnitTest.Drupal7.inc

@@ -0,0 +1,5 @@
+<?php
+
+$output = `$form['field']`; // Error (user input).
+$output = `$request['field']`; // Warning.
+`$_GET`; // Error (user input).

Security/Tests/BadFunctions/BackticksUnitTest.Drupal8.inc

@@ -0,0 +1,5 @@
+<?php
+
+$output = `$form['field']`; // Error (user input).
+$output = `$request['field']`; // Error (user input).
+`$_GET`; // Error (user input).

Security/Tests/BadFunctions/BackticksUnitTest.Symfony2.inc

@@ -0,0 +1,5 @@
+<?php
+
+$output = `$form['field']`; // Warning.
+$output = `$request['field']`; // Error (user input).
+`$_GET`; // Error (user input).

Security/Tests/BadFunctions/BackticksUnitTest.inc

@@ -0,0 +1,21 @@
+<?php
+
+// Not using variable input.
+$output = `ls -al`;
+
+// These should all give an error/warning.
+$output = `$form['field']`; // Warning.
+$output = `$request['field']`; // Warning.
+`$_GET`; // Error (user input).
+
+$output = `git blame --date=short "$filename"`; // Warning.
+
+$output = `git blame --date=$_POST['key'] "$filename"`; // Warning + error.
+
+$output = `git blame
+	--date=$_POST['key'] // Error.
+	"$filename"`; // Warning.
+
+// Incomplete command. Ignore.
+// Intentional parse error. This should be the last test in the file.
+$output = `ls