From 03199df925f50368333352f1c644a0684bdf6893 Mon Sep 17 00:00:00 2001
From: vrana <jakubv@fb.com>
Date: Sat, 2 Feb 2013 12:33:22 -0800
Subject: [PATCH] Lint dynamic string usage in parameters treated as safe

Summary:
This disallows code like this:

  $cmd = 'ls';
  execx($cmd);

But I guess it's not that big deal?

Test Plan: Linted whole Arcanist and Phabricator codebases, most parts looks fixable.

Reviewers: epriestley

CC: nh, aran, Korvin

Differential Revision: https://secure.phabricator.com/D4794
---
 .../linter/ArcanistPhutilXHPASTLinter.php     | 71 ++++++++++++++++++-
 1 file changed, 70 insertions(+), 1 deletion(-)

diff --git a/src/lint/linter/ArcanistPhutilXHPASTLinter.php b/src/lint/linter/ArcanistPhutilXHPASTLinter.php
index 09ff2249..5c8b91e7 100644
--- a/src/lint/linter/ArcanistPhutilXHPASTLinter.php
+++ b/src/lint/linter/ArcanistPhutilXHPASTLinter.php
@@ -7,6 +7,7 @@ final class ArcanistPhutilXHPASTLinter extends ArcanistBaseXHPASTLinter {
 
   const LINT_PHT_WITH_DYNAMIC_STRING = 1;
   const LINT_ARRAY_COMBINE           = 2;
+  const LINT_UNSAFE_DYNAMIC_STRING   = 4;
 
   private $xhpastLinter;
 
@@ -28,12 +29,16 @@ final class ArcanistPhutilXHPASTLinter extends ArcanistBaseXHPASTLinter {
     return array(
       self::LINT_PHT_WITH_DYNAMIC_STRING => 'Use of pht() on Dynamic String',
       self::LINT_ARRAY_COMBINE           => 'array_combine() Unreliable',
+      self::LINT_UNSAFE_DYNAMIC_STRING   => 'Unsafe Usage of Dynamic String',
     );
   }
 
   public function getLintSeverityMap() {
+    $warning = ArcanistLintSeverity::SEVERITY_WARNING;
+
     return array(
-      self::LINT_ARRAY_COMBINE => ArcanistLintSeverity::SEVERITY_WARNING,
+      self::LINT_ARRAY_COMBINE           => $warning,
+      self::LINT_UNSAFE_DYNAMIC_STRING   => $warning,
     );
   }
 
@@ -41,6 +46,10 @@ final class ArcanistPhutilXHPASTLinter extends ArcanistBaseXHPASTLinter {
     return 'PHLXHP';
   }
 
+  public function getCacheVersion() {
+    return 1;
+  }
+
   public function willLintPaths(array $paths) {
     $this->xhpastLinter->willLintPaths($paths);
   }
@@ -55,6 +64,7 @@ final class ArcanistPhutilXHPASTLinter extends ArcanistBaseXHPASTLinter {
 
     $this->lintPHT($root);
     $this->lintArrayCombine($root);
+    $this->lintUnsafeDynamicString($root);
   }
 
 
@@ -85,6 +95,65 @@ final class ArcanistPhutilXHPASTLinter extends ArcanistBaseXHPASTLinter {
   }
 
 
+  private function lintUnsafeDynamicString($root) {
+    $safe = array(
+      'hsprintf' => 0,
+
+      'csprintf' => 0,
+      'vcsprintf' => 0,
+      'execx' => 0,
+      'exec_manual' => 0,
+      'phutil_passthru' => 0,
+
+      'qsprintf' => 1,
+      'vqsprintf' => 1,
+      'queryfx' => 1,
+      'vqueryfx' => 1,
+      'queryfx_all' => 1,
+      'vqueryfx_all' => 1,
+      'queryfx_one' => 1,
+    );
+
+    $calls = $root->selectDescendantsOfType('n_FUNCTION_CALL');
+    $this->lintUnsafeDynamicStringCall($calls, $safe);
+
+    $safe = array(
+      'execfuture' => 0,
+    );
+
+    $news = $root->selectDescendantsOfType('n_NEW');
+    $this->lintUnsafeDynamicStringCall($news, $safe);
+  }
+
+  private function lintUnsafeDynamicStringCall(
+    AASTNodeList $calls,
+    array $safe) {
+
+    foreach ($calls as $call) {
+      $name = $call->getChildByIndex(0)->getConcreteString();
+      $param = idx($safe, strtolower($name));
+
+      if ($param === null) {
+        continue;
+      }
+
+      $parameters = $call->getChildByIndex(1);
+      if (count($parameters->getChildren()) <= $param) {
+        continue;
+      }
+
+      $identifier = $parameters->getChildByIndex($param);
+      if (!$identifier->isConstantString()) {
+        $this->raiseLintAtNode(
+          $call,
+          self::LINT_UNSAFE_DYNAMIC_STRING,
+          "Parameter ".($param + 1)." of {$name}() should be a scalar string, ".
+            "otherwise it's not safe.");
+      }
+    }
+  }
+
+
   private function lintArrayCombine($root) {
     $function_calls = $root->selectDescendantsOfType('n_FUNCTION_CALL');
     foreach ($function_calls as $call) {