Process class constructor accepts arguments as array

Previously it accepts just whole command without escaping, so it was potential unsafe code
JakubOnderka · Sep 30, 2018 · dfe8a64 · dfe8a64
1 parent 04b53a6
commit dfe8a64
Show file tree

Hide file tree

Showing 6 changed files with 33 additions and 16 deletions.
diff --git a/src/Process/GitBlameProcess.php b/src/Process/GitBlameProcess.php
@@ -9,15 +9,17 @@ class GitBlameProcess extends Process
      * @param string $gitExecutable
      * @param string $file
      * @param int $line
+     * @throws RunTimeException
      */
     public function __construct($gitExecutable, $file, $line)
     {
-        $cmd = escapeshellcmd($gitExecutable) . " blame -p -L $line,+1 " . escapeshellarg($file);
-        parent::__construct($cmd);
+        $arguments = array('blame', '-p', '-L', "$line,+1", $file);
+        parent::__construct($gitExecutable, $arguments);
     }
 
     /**
      * @return bool
+     * @throws RunTimeException
      */
     public function isSuccess()
     {
@@ -106,10 +108,11 @@ public function getSummary()
     /**
      * @param string $gitExecutable
      * @return bool
+     * @throws RunTimeException
      */
     public static function gitExists($gitExecutable)
     {
-        $process = new Process(escapeshellcmd($gitExecutable) . ' --version');
+        $process = new Process($gitExecutable, array('--version'));
         $process->waitForFinish();
         return $process->getStatusCode() === 0;
     }
@@ -120,6 +123,7 @@ public static function gitExists($gitExecutable)
      * @param int $time
      * @param string $zone
      * @return \DateTime
+     * @throws \Exception
      */
     protected function getDateTime($time, $zone)
     {
@@ -140,4 +144,4 @@ protected function getDateTime($time, $zone)
 
         return new \DateTime($datetime->format('Y-m-d\TH:i:s') . $zone, $utcTimeZone);
     }
-}
+}
diff --git a/src/Process/LintProcess.php b/src/Process/LintProcess.php
@@ -20,6 +20,7 @@ class LintProcess extends PhpProcess
      * @param bool $aspTags
      * @param bool $shortTag
      * @param bool $deprecated
+     * @throws RunTimeException
      */
     public function __construct(PhpExecutable $phpExecutable, $fileToCheck, $aspTags = false, $shortTag = false, $deprecated = false)
     {
@@ -33,7 +34,7 @@ public function __construct(PhpExecutable $phpExecutable, $fileToCheck, $aspTags
             '-d error_reporting=E_ALL',
             '-n',
             '-l',
-            escapeshellarg($fileToCheck),
+            $fileToCheck,
         );
 
         $this->showDeprecatedErrors = $deprecated;
@@ -42,6 +43,7 @@ public function __construct(PhpExecutable $phpExecutable, $fileToCheck, $aspTags
 
     /**
      * @return bool
+     * @throws
      */
     public function containsError()
     {
@@ -86,6 +88,7 @@ public function getSyntaxError()
 
     /**
      * @return bool
+     * @throws RunTimeException
      */
     public function isFail()
     {
@@ -94,6 +97,7 @@ public function isFail()
 
     /**
      * @return bool
+     * @throws RunTimeException
      */
     public function isSuccess()
     {

diff --git a/src/Process/PhpExecutable.php b/src/Process/PhpExecutable.php
@@ -78,7 +78,7 @@ public static function getPhpExecutable($phpExecutable)
 echo 'PHP;', PHP_VERSION_ID, ';', defined('HPHP_VERSION') ? HPHP_VERSION : null;
 PHP;
 
-        $process = new Process(escapeshellarg($phpExecutable) . ' -n -r ' . escapeshellarg($codeToExecute));
+        $process = new Process($phpExecutable, array('-n', '-r', $codeToExecute));
         $process->waitForFinish();
 
         try {
@@ -90,7 +90,7 @@ public static function getPhpExecutable($phpExecutable)
 
         } catch (RunTimeException $e) {
             // Try HHVM type
-            $process = new Process(escapeshellarg($phpExecutable) . ' --php -r ' . escapeshellarg($codeToExecute));
+            $process = new Process($phpExecutable, array('--php', '-r', $codeToExecute));
             $process->waitForFinish();
 
             if ($process->getStatusCode() !== 0 && $process->getStatusCode() !== 255) {

diff --git a/src/Process/PhpProcess.php b/src/Process/PhpProcess.php
@@ -8,21 +8,25 @@ class PhpProcess extends Process
      * @param PhpExecutable $phpExecutable
      * @param array $parameters
      * @param string|null $stdIn
+     * @throws \JakubOnderka\PhpParallelLint\RunTimeException
      */
     public function __construct(PhpExecutable $phpExecutable, array $parameters = array(), $stdIn = null)
     {
         $constructedParameters = $this->constructParameters($parameters, $phpExecutable->isIsHhvmType());
-        $cmdLine = escapeshellcmd($phpExecutable->getPath()) . ' ' . $constructedParameters;
-        parent::__construct($cmdLine, $stdIn);
+        parent::__construct($phpExecutable->getPath(), $constructedParameters, $stdIn);
     }
 
     /**
      * @param array $parameters
      * @param bool $isHhvm
-     * @return string
+     * @return array
      */
     private function constructParameters(array $parameters, $isHhvm)
     {
-        return ($isHhvm ? '--php ' : '') . implode(' ', $parameters);
+        if ($isHhvm) {
+            $parameters = array_merge(array('-php'), $parameters);
+        }
+
+        return $parameters;
     }
-}
+}
diff --git a/src/Process/Process.php b/src/Process/Process.php
@@ -32,18 +32,20 @@ class Process
     private $statusCode;
 
     /**
-     * @param string $cmdLine
+     * @param string $executable
+     * @param string[] $arguments
      * @param string $stdInInput
      * @throws RunTimeException
      */
-    public function __construct($cmdLine, $stdInInput = null)
+    public function __construct($executable, array $arguments = array(), $stdInInput = null)
     {
         $descriptors = array(
             self::STDIN  => array('pipe', self::READ),
             self::STDOUT => array('pipe', self::WRITE),
             self::STDERR => array('pipe', self::WRITE),
         );
 
+        $cmdLine = $executable . ' ' . implode(' ', array_map('escapeshellarg', $arguments));
         $this->process = proc_open($cmdLine, $descriptors, $pipes, null, null, array('bypass_shell' => true));
 
         if ($this->process === false || $this->process === null) {
@@ -142,6 +144,7 @@ public function getStatusCode()
 
     /**
      * @return bool
+     * @throws RunTimeException
      */
     public function isFail()
     {

diff --git a/src/Process/SkipLintProcess.php b/src/Process/SkipLintProcess.php
@@ -30,11 +30,13 @@ public function __construct(PhpExecutable $phpExecutable, array $filesToCheck)
 
         $script = str_replace('<?php', '', $script);
 
-        $parameters = array('-d display_errors=stderr', '-r ' . escapeshellarg($script));
-
+        $parameters = array('-d', 'display_errors=stderr', '-r', $script);
         parent::__construct($phpExecutable, $parameters, implode(PHP_EOL, $filesToCheck));
     }
 
+    /**
+     * @throws RunTimeException
+     */
     public function getChunk()
     {
         if (!$this->isFinished()) {