Merge pull request #434 from consolidation-org/escape-args

Always escape arguments CommandArguments.

CHANGELOG.md

@@ -21,6 +21,8 @@
 * Use league/container to do Dependency Injection
    * *Breaking* Tasks' loadTasks traits must use `$this->task(TaskClass::class);` instead of `new TaskClass();`
    * *Breaking* Tasks that use other tasks must use `$this->collectionBuilder()->taskName();` instead of `new TaskClass();` when creating task objects to call. Implement `Robo\Contract\BuilderAwareInterface` and use `Robo\Contract\BuilderAwareTrait` to add the `collectionBuilder()` method to your task class.
+* *Breaking* The `arg()`, `args()` and `option()` methods in CommandArguments now escape the values passed in to them. There is now a `rawArg()` method if you need to add just one argument that has already been escaped.
+* *Breaking* taskWrite is now called taskWriteToFile
 * [Extract] task added
 * [Pack] task added
 * [TmpDir], [WorkDir] and [TmpFile] tasks added
@@ -30,7 +32,6 @@
 * Add `robo generate:task` code-generator to make new stack-based task wrappers around existing classes
 * Add `robo sniff` by @dustinleblanc. Runs the PHP code sniffer followed by the code beautifier, if needed.
 * Implement ArrayInterface for Result class, so result data may be accessed like an array 
-* *Breaking* taskWrite is now called taskWriteToFile
 * Defer execution of operations in taskWriteToFile until the run() method
 * Add Write::textIfMatch() for taskWriteToFile
 * ResourceExistenceChecker used for error checking in DeleteDir, CopyDir, CleanDir and Concat tasks by @burzum

src/Common/CommandArguments.php

@@ -1,6 +1,8 @@
 <?php
 namespace Robo\Common;
 
+use Symfony\Component\Process\ProcessUtils;
+
 /**
  * Use this to add arguments and options to the $arguments property.
  */
@@ -30,15 +32,35 @@ public function args($args)
         if (!is_array($args)) {
             $args = func_get_args();
         }
+        array_map('static::escape', $args);
         $this->arguments .= " ".implode(' ', $args);
         return $this;
     }
 
+    public function rawArg($arg)
+    {
+        $this->arguments .= " $arg";
+    }
+
+    /**
+     * Escape the provided value, unless it contains only alphanumeric
+     * plus a few other basic characters.
+     *
+     * @param string $value
+     * @return string
+     */
+    public static function escape($value)
+    {
+        if (preg_match('/^[a-zA-Z0-9\/.@~_-]*$/', $value)) {
+            return $value;
+        }
+        return ProcessUtils::escapeArgument($value);
+    }
+
     /**
      * Pass option to executable. Options are prefixed with `--` , value can be provided in second parameter.
      *
-     * Option values must be explicitly escaped with escapeshellarg if necessary before being passed to
-     * this function.
+     * Option values are automatically escaped if necessary.
      *
      * @param $option
      * @param string $value
@@ -50,15 +72,14 @@ public function option($option, $value = null)
             $option = "--$option";
         }
         $this->arguments .= null == $option ? '' : " " . $option;
-        $this->arguments .= null == $value ? '' : " " . $value;
+        $this->arguments .= null == $value ? '' : " " . static::escape($value);
         return $this;
     }
 
     /**
      * Pass multiple options to executable. Value can be a string or array.
      *
-     * Option values should be provided in raw, unescaped form; they are always
-     * escaped via escapeshellarg in this function.
+     * Option values should be provided in raw, unescaped form
      *
      * @param $option
      * @param string|array $value
@@ -71,7 +92,7 @@ public function optionList($option, $value = array())
                 $this->optionList($option, $item);
             }
         } else {
-            $this->option($option, escapeshellarg($value));
+            $this->option($option, $value);
         }
 
         return $this;

src/Task/Remote/Rsync.php

@@ -315,8 +315,8 @@ public function run()
      */
     public function getCommand()
     {
-        $this->option(null, escapeshellarg($this->getFromPathSpec()))
-            ->option(null, escapeshellarg($this->getToPathSpec()));
+        $this->option(null, $this->getFromPathSpec())
+            ->option(null, $this->getToPathSpec());
 
         return $this->command . $this->arguments;
     }

tests/unit/Task/ApiGenTest.php

@@ -43,7 +43,7 @@ public function testPHPUnitCommand()
             ->tree('Y') // boolean as string
             ->debug('n');
 
-        $cmd = 'apigen --config ./apigen.neon --source src --extensions php --exclude test --exclude tmp --skip-doc-path a --skip-doc-path b --charset utf8,iso88591 --internal no --php yes --tree yes --debug no';
+        $cmd = 'apigen --config ./apigen.neon --source src --extensions php --exclude test --exclude tmp --skip-doc-path a --skip-doc-path b --charset \'utf8,iso88591\' --internal no --php yes --tree yes --debug no';
         verify($task->getCommand())->equals($cmd);
 
         $task->run();

tests/unit/Task/RsyncTest.php

@@ -29,14 +29,7 @@ public function testRsync()
                 ->stats()
                 ->getCommand()
         )->equals(
-            sprintf(
-                'rsync --recursive --exclude %s --exclude %s --exclude %s --checksum --whole-file --verbose --progress --human-readable --stats %s %s',
-                escapeshellarg('.git'),
-                escapeshellarg('.svn'),
-                escapeshellarg('.hg'),
-                escapeshellarg('src/'),
-                escapeshellarg('dev@localhost:/var/www/html/app/')
-            )
+                'rsync --recursive --exclude .git --exclude .svn --exclude .hg --checksum --whole-file --verbose --progress --human-readable --stats src/ \'dev@localhost:/var/www/html/app/\''
         );
 
         verify(
@@ -47,11 +40,7 @@ public function testRsync()
                 ->toPath('/var/path/with/a space')
                 ->getCommand()
         )->equals(
-            sprintf(
-                'rsync %s %s',
-                escapeshellarg('src/foo bar/baz'),
-                escapeshellarg('dev@localhost:/var/path/with/a space')
-            )
+                'rsync \'src/foo bar/baz\' \'dev@localhost:/var/path/with/a space\''
         );
     }
 }