Merge pull request #775 from robfrawley/bugfix-763

Amend path resolution handlers and outside root check conditional in FileSystemLoader

.gitignore

@@ -4,3 +4,4 @@ composer.lock
 composer.phar
 vendor/
 Tests/Functional/app/web/media/cache
+.idea/

Binary/Loader/FileSystemLoader.php

@@ -2,6 +2,7 @@
 
 namespace Liip\ImagineBundle\Binary\Loader;
 
+use Liip\ImagineBundle\Exception\InvalidArgumentException;
 use Liip\ImagineBundle\Model\FileBinary;
 use Symfony\Component\HttpFoundation\File\MimeType\ExtensionGuesserInterface;
 use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface;
@@ -37,19 +38,25 @@ public function __construct(
         $this->mimeTypeGuesser = $mimeTypeGuesser;
         $this->extensionGuesser = $extensionGuesser;
 
-        $this->rootPath = rtrim($rootPath, '/');
+        if (!($realRootPath = realpath($rootPath))) {
+            throw new InvalidArgumentException(sprintf('Root image path not resolvable "%s"', $rootPath));
+        }
+
+        $this->rootPath = $realRootPath;
     }
 
     /**
      * {@inheritdoc}
      */
     public function find($path)
     {
-        if (false !== strpos($path, '../')) {
-            throw new NotLoadableException(sprintf("Source image was searched with '%s' out side of the defined root path", $path));
+        if (!($absolutePath = realpath($this->rootPath.DIRECTORY_SEPARATOR.ltrim($path, DIRECTORY_SEPARATOR)))) {
+            throw new NotLoadableException(sprintf('Source image not resolvable "%s"', $path));
         }
 
-        $absolutePath = $this->rootPath.'/'.ltrim($path, '/');
+        if (0 !== strpos($absolutePath, $this->rootPath)) {
+            throw new NotLoadableException(sprintf('Source image invalid "%s" as it is outside of the defined root path', $absolutePath));
+        }
 
         if (false == file_exists($absolutePath)) {
             throw new NotLoadableException(sprintf('Source image not found in "%s"', $absolutePath));

Exception/InvalidArgumentException.php

@@ -0,0 +1,7 @@
+<?php
+
+namespace Liip\ImagineBundle\Exception;
+
+class InvalidArgumentException extends \InvalidArgumentException implements ExceptionInterface
+{
+}

Tests/Binary/Loader/FileSystemLoaderTest.php

@@ -19,9 +19,9 @@ public static function provideLoadCases()
             array(__DIR__, $fileName),
             array(__DIR__.'/', $fileName),
             array(__DIR__, '/'.$fileName),
-            array(__DIR__.'/', '/'.$fileName),
+            array(__DIR__.'/../../Binary/Loader', '/'.$fileName),
             array(realpath(__DIR__.'/..'), 'Loader/'.$fileName),
-            array(realpath(__DIR__.'/../'), '/Loader/'.$fileName),
+            array(__DIR__.'/../', '/Loader/../../Binary/Loader/'.$fileName),
         );
     }
 
@@ -41,7 +41,21 @@ public function testCouldBeConstructedWithExpectedArguments()
         );
     }
 
-    public function testThrowExceptionIfPathHasDoublePointSlashAtBegging()
+    public function testThrowExceptionIfRootPathDoesNotExist()
+    {
+        $this->setExpectedException(
+            'Liip\ImagineBundle\Exception\InvalidArgumentException',
+            'Root image path not resolvable'
+        );
+
+        new FileSystemLoader(
+            MimeTypeGuesser::getInstance(),
+            ExtensionGuesser::getInstance(),
+            '/a/bad/root/path'
+        );
+    }
+
+    public function testThrowExceptionIfRealPathIsOutsideRootPath1()
     {
         $loader = new FileSystemLoader(
             MimeTypeGuesser::getInstance(),
@@ -51,13 +65,13 @@ public function testThrowExceptionIfPathHasDoublePointSlashAtBegging()
 
         $this->setExpectedException(
             'Liip\ImagineBundle\Exception\Binary\Loader\NotLoadableException',
-            'Source image was searched with'
+            'Source image invalid'
         );
 
-        $loader->find('../foo/bar');
+        $loader->find('../Loader/../../Binary/Loader/../../../Resources/config/routing.xml');
     }
 
-    public function testThrowExceptionIfPathHasDoublePointSlashInTheMiddle()
+    public function testThrowExceptionIfRealPathIsOutsideRootPath2()
     {
         $loader = new FileSystemLoader(
             MimeTypeGuesser::getInstance(),
@@ -67,10 +81,21 @@ public function testThrowExceptionIfPathHasDoublePointSlashInTheMiddle()
 
         $this->setExpectedException(
             'Liip\ImagineBundle\Exception\Binary\Loader\NotLoadableException',
-            'Source image was searched with'
+            'Source image invalid'
+        );
+
+        $loader->find('../../Binary/');
+    }
+
+    public function testThrowExceptionIfPathHasDoublePointSlashInTheMiddle()
+    {
+        $loader = new FileSystemLoader(
+            MimeTypeGuesser::getInstance(),
+            ExtensionGuesser::getInstance(),
+            __DIR__
         );
 
-        $loader->find('foo/../bar');
+        $loader->find('/../../Binary/Loader/'.pathinfo(__FILE__, PATHINFO_BASENAME));
     }
 
     public function testThrowExceptionIfFileNotExist()
@@ -83,7 +108,7 @@ public function testThrowExceptionIfFileNotExist()
 
         $this->setExpectedException(
             'Liip\ImagineBundle\Exception\Binary\Loader\NotLoadableException',
-            'Source image not found'
+            'Source image not resolvable'
         );
 
         $loader->find('fileNotExist');