Shell escape

Command.php

@@ -82,7 +82,7 @@ public function __construct(string $imageMagickPath = '/usr/bin')
         }
 
         // For imagemagick 7 we'll use the "magick" base command each time.
-        if (file_exists($imageMagickPath.'/magick') || file_exists($imageMagickPath.'/magick.exe')) {
+        if (preg_match('~magick(?:\.exe)?$~iU', $imageMagickPath) || file_exists($imageMagickPath.'/magick') || file_exists($imageMagickPath.'/magick.exe')) {
             $this->version = static::VERSION_7;
             $imageMagickPath .= '/magick ';
         } elseif (file_exists($imageMagickPath.'/convert') || file_exists($imageMagickPath.'/convert.exe')) {
@@ -223,15 +223,11 @@ public function getCommand(): string
     /**
      * Escapes a string in order to inject it in the shell command.
      */
-    public function escape(string $string, bool $addQuotes = true): string
+    public function escape(string $string): string
     {
-        $string = str_replace(
-            ['"', '`', '’', '\\\''],
-            ['\"', "'", "'", "'"],
-            trim($string)
-        );
+        $string = escapeshellarg($string);
 
-        return $addQuotes ? '"'.$string.'"' : $string;
+        return $string;
     }
 
     /**
@@ -293,7 +289,7 @@ public function fill(string $color): self
      */
     public function resize($geometry): self
     {
-        $this->command .= ' -resize '.$this->escape($this->ref->geometry($geometry)).' ';
+        $this->command .= ' -resize "'.$this->ref->geometry($geometry).'" ';
 
         return $this;
     }
@@ -305,7 +301,7 @@ public function resize($geometry): self
      */
     public function size($geometry): self
     {
-        $this->command .= ' -size '.$this->escape($this->ref->geometry($geometry)).' ';
+        $this->command .= ' -size "'.$this->ref->geometry($geometry).'" ';
 
         return $this;
     }
@@ -331,7 +327,7 @@ public function xc(string $canvasColor = 'none'): self
      */
     public function crop($geometry): self
     {
-        $this->command .= ' -crop '.$this->escape($this->ref->geometry($geometry), false).' ';
+        $this->command .= ' -crop '.$this->ref->geometry($geometry, false).' ';
 
         return $this;
     }
@@ -343,7 +339,7 @@ public function crop($geometry): self
      */
     public function extent($geometry): self
     {
-        $this->command .= ' -extent '.$this->escape($this->ref->geometry($geometry));
+        $this->command .= ' -extent '.$this->ref->geometry($geometry);
 
         return $this;
     }
@@ -355,7 +351,7 @@ public function extent($geometry): self
      */
     public function thumbnail($geometry): self
     {
-        $this->command .= ' -thumbnail '.$this->escape($this->ref->geometry($geometry));
+        $this->command .= ' -thumbnail "'.$this->ref->geometry($geometry).'" ';
 
         return $this;
     }
@@ -451,16 +447,16 @@ public function stroke(string $color): self
     }
 
     /**
-     * @param string|Geometry $geometry
+     * @param string|array $options
      *
      * @link http://imagemagick.org/script/command-line-options.php#annotate
      */
-    public function text(array $options = []): self
+    public function text($options = []): self
     {
         $mandatoryKeys = ['text', 'geometry', 'textSize'];
         foreach ($options as $key => $v) {
             if (!isset($mandatoryKeys[$key])) {
-                throw new \InvalidArgumentException(\sprintf('Key "%s" is missing for the %s function.', $key, __METHOD__));
+                throw new \InvalidArgumentException(\sprintf('Options "%s" is missing for the %s function.', $key, __METHOD__));
             }
         }
 

Tests/CommandTest.php

@@ -19,28 +19,22 @@ class CommandTest extends AbstractTestCase
     /**
      * @dataProvider provideWrongConvertDirs
      */
-    public function testWrongConvertDirs($path, $expectedMessage, $expectedException)
+    public function testWrongConvertDirs($path, $expectedMessage)
     {
-        $exception = '';
-        $exceptionClass = '';
-
         $path = str_replace('\\', '/', $path);
         $expectedMessage = str_replace('\\', '/', $expectedMessage);
-        try {
-            new Command($path);
-        } catch (\Exception $e) {
-            $exception = $e->getMessage();
-            $exceptionClass = get_class($e);
-        }
-        $this->assertContains($expectedMessage, $exception);
-        $this->assertEquals($exceptionClass, $expectedException);
+
+        $this->expectException(\InvalidArgumentException::class);
+        $this->expectExceptionMessage($expectedMessage);
+
+        new Command($path);
     }
 
     public function provideWrongConvertDirs()
     {
         return array(
-            array('/this/is/a/dummy/dir', 'The specified path (/this/is/a/dummy/dir) is not a directory', 'InvalidArgumentException'),
-            array('./', 'The specified path (.) does not seem to contain ImageMagick binaries, or it is not readable', 'InvalidArgumentException'),
+            array('/this/is/a/dummy/dir', 'The specified path (/this/is/a/dummy/dir) is not a directory'),
+            array('./', 'The specified path (.) does not seem to contain ImageMagick binaries, or it is not readable'),
         );
     }
 
@@ -153,7 +147,7 @@ public function testCommandString($source, $output, $geometry, $quality)
 
         $expected = ' '.$command->getExecutable('convert').
                     ' "'.$source.'"'.
-                    ' -thumbnail "'.$geometry.'"'.
+                    ' -thumbnail "'.$geometry.'" '.
                     ' -quality '.$quality.
                     ' "'.$output.'" ';
 
@@ -197,13 +191,13 @@ public function testInexistingFiles()
 
     public function testEscape()
     {
-        $string = 'PSR\'s a great `code` style standard. ';
+        $string = '25% $(touch hacked) #';
 
         $command = new Command(IMAGEMAGICK_DIR);
 
-        $escaped = $command->escape($string, true);
+        $escaped = $command->escape($string);
 
-        $this->assertEquals('"PSR\'s a great \'code\' style standard."', $escaped);
+        $this->assertEquals('"25  $(touch hacked) #"', $escaped);
     }
 
 }

Tests/bootstrap.php

@@ -30,11 +30,14 @@
     '/usr/local/bin/',
 ];
 foreach ($possibleDirectories as $dir) {
-    $dir = rtrim($dir, '/\\').'/';
-
     echo 'Check "'.$dir.'convert" binary'."\n";
-    exec($dir.'convert -version', $o, $code);
+    exec($dir.'convert -version 2>&1', $o, $code);
+    if (0 !== $code) {
+        echo 'Check "'.$dir.'magick" binary (for ImageMagick 7)'."\n";
+        exec($dir.'magick -version 2>&1', $o, $code);
+    }
     if (0 === $code) {
+        echo "Resolved as $dir\n";
         define('IMAGEMAGICK_DIR', $dir);
         break;
     }

phpunit.xml.dist

@@ -1,20 +1,13 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!-- http://phpunit.de/manual/4.1/en/appendixes.configuration.html -->
 <phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
-         xsi:noNamespaceSchemaLocation="http://schema.phpunit.de/4.1/phpunit.xsd"
-         backupGlobals="false"
-         backupStaticAttributes="false"
+         xsi:noNamespaceSchemaLocation="http://schema.phpunit.de/6.5/phpunit.xsd"
          colors="true"
-         convertErrorsToExceptions="true"
-         convertNoticesToExceptions="true"
-         convertWarningsToExceptions="true"
-         processIsolation="false"
-         stopOnFailure="false"
          bootstrap="Tests/bootstrap.php"
 >
     <testsuites>
         <testsuite name="OrbitaleImageMagickPHP test suite">
-            <directory suffix="Test.php">Tests</directory>
+            <directory>Tests</directory>
         </testsuite>
     </testsuites>
 
@@ -25,16 +18,11 @@
     -->
 
     <filter>
-        <whitelist processUncoveredFilesFromWhitelist="true">
+        <whitelist>
             <file>Command.php</file>
             <file>CommandOptions.php</file>
             <file>CommandResponse.php</file>
             <directory suffix=".php">./ReferenceClasses</directory>
         </whitelist>
-        <blacklist>
-            <directory>./build</directory>
-            <directory>./vendor</directory>
-            <directory>./Tests</directory>
-        </blacklist>
     </filter>
 </phpunit>