From d477df00eb5b357f953e693b6d99c13551edeb3f Mon Sep 17 00:00:00 2001
From: Joshua Spence <josh@joshuaspence.com>
Date: Mon, 5 Jan 2015 10:51:33 +1100
Subject: [PATCH] Add a linter rule to detect the use of blacklisted functions

Summary:
As mentioned in the [[https://secure.phabricator.com/book/phabcontrib/article/php_coding_standards/ | Phabricator PHP coding standards]], the `eval` function should be avoided. There is some good discussion on [[http://stackoverflow.com/questions/951373/when-is-eval-evil-in-php | StackOverflow]] as well.

Having said that, instead of hardcoding `eval()`, I have generalised this enough to allow a set of "blacklisted" functions to be defined with `xhpast.blacklisted.function` in the `.arclint` file.

Test Plan: Added a test case.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D10686
---
 .arclint                                      | 17 +++++++++-
 src/lint/linter/ArcanistXHPASTLinter.php      | 31 ++++++++++++++++++-
 .../__tests__/xhpast/blacklisted.lint-test    |  7 +++++
 3 files changed, 53 insertions(+), 2 deletions(-)
 create mode 100644 src/lint/linter/__tests__/xhpast/blacklisted.lint-test

diff --git a/.arclint b/.arclint
index 63d108e6..63450ef2 100644
--- a/.arclint
+++ b/.arclint
@@ -41,7 +41,19 @@
       "exclude": "(resources/spelling/.*\\.json$)"
     },
     "text": {
-      "type": "text"
+      "type": "text",
+      "exclude": [
+        "(^\\.arclint$)"
+      ]
+    },
+    "text-without-length": {
+      "type": "text",
+      "severity": {
+        "3": "disabled"
+      },
+      "include": [
+        "(^\\.arclint$)"
+      ]
     },
     "xhpast": {
       "type": "xhpast",
@@ -50,6 +62,9 @@
         "16": "advice",
         "34": "error"
       },
+      "xhpast.blacklisted.function": {
+        "eval": "The eval() function should be avoided. It is potentially unsafe and makes debugging more difficult."
+      },
       "xhpast.php-version": "5.2.3",
       "xhpast.php-version.windows": "5.3.0"
     }
diff --git a/src/lint/linter/ArcanistXHPASTLinter.php b/src/lint/linter/ArcanistXHPASTLinter.php
index 6694954f..aaa77f88 100644
--- a/src/lint/linter/ArcanistXHPASTLinter.php
+++ b/src/lint/linter/ArcanistXHPASTLinter.php
@@ -50,7 +50,9 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
   const LINT_ARRAY_SEPARATOR           = 48;
   const LINT_CONSTRUCTOR_PARENTHESES   = 49;
   const LINT_DUPLICATE_SWITCH_CASE     = 50;
+  const LINT_BLACKLISTED_FUNCTION      = 50;
 
+  private $blacklistedFunctions = array();
   private $naminghook;
   private $switchhook;
   private $version;
@@ -111,6 +113,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       self::LINT_ARRAY_SEPARATOR           => 'Array Separator',
       self::LINT_CONSTRUCTOR_PARENTHESES   => 'Constructor Parentheses',
       self::LINT_DUPLICATE_SWITCH_CASE     => 'Duplicate Case Statements',
+      self::LINT_BLACKLISTED_FUNCTION      => 'Use of Blacklisted Function',
     );
   }
 
@@ -157,6 +160,10 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
 
   public function getLinterConfigurationOptions() {
     return parent::getLinterConfigurationOptions() + array(
+      'xhpast.blacklisted.function' => array(
+        'type' => 'optional map<string, string>',
+        'help' => pht('Blacklisted functions which should not be used.'),
+      ),
       'xhpast.naminghook' => array(
         'type' => 'optional string',
         'help' => pht(
@@ -182,6 +189,9 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
 
   public function setLinterConfigurationValue($key, $value) {
     switch ($key) {
+      case 'xhpast.blacklisted.function':
+        $this->blacklistedFunctions = $value;
+        return;
       case 'xhpast.naminghook':
         $this->naminghook = $value;
         return;
@@ -201,7 +211,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
 
   public function getVersion() {
     // The version number should be incremented whenever a new rule is added.
-    return '12';
+    return '13';
   }
 
   protected function resolveFuture($path, Future $future) {
@@ -274,6 +284,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       'lintArraySeparator' => self::LINT_ARRAY_SEPARATOR,
       'lintConstructorParentheses' => self::LINT_CONSTRUCTOR_PARENTHESES,
       'lintSwitchStatements' => self::LINT_DUPLICATE_SWITCH_CASE,
+      'lintBlacklistedFunction' => self::LINT_BLACKLISTED_FUNCTION,
     );
 
     foreach ($method_codes as $method => $codes) {
@@ -2969,6 +2980,24 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
     }
   }
 
+  private function lintBlacklistedFunction(XHPASTNode $root) {
+    $calls = $root->selectDescendantsOfType('n_FUNCTION_CALL');
+
+    foreach ($calls as $call) {
+      $node = $call->getChildByIndex(0);
+      $name = $node->getConcreteString();
+
+      $reason = idx($this->blacklistedFunctions, $name);
+
+      if ($reason) {
+        $this->raiseLintAtNode(
+          $node,
+          self::LINT_BLACKLISTED_FUNCTION,
+          $reason);
+      }
+    }
+  }
+
   public function getSuperGlobalNames() {
     return array(
       '$GLOBALS',
diff --git a/src/lint/linter/__tests__/xhpast/blacklisted.lint-test b/src/lint/linter/__tests__/xhpast/blacklisted.lint-test
new file mode 100644
index 00000000..9b217a62
--- /dev/null
+++ b/src/lint/linter/__tests__/xhpast/blacklisted.lint-test
@@ -0,0 +1,7 @@
+<?php
+eval('evil code');
+~~~~~~~~~~
+error:2:1
+~~~~~~~~~~
+~~~~~~~~~~
+{"config": {"xhpast.blacklisted.function": {"eval": "Evil function"}}}