[PWA-1119] UPWARD Path Fixes (#1)

* - Fixed test for file outside of base directory
- Fixed phpcs config issue
- Ran code fixer

* - Make relative check more strict

* - Handle not found case in file resolver

* Handle not found use case

Author: tjwiebell

.php_cs

@@ -70,7 +70,7 @@ return PhpCsFixer\Config::create()
         'no_unreachable_default_argument_value'  => true,
         'no_useless_else'                        => true,
         'no_useless_return'                      => true,
-        'ordered_class_elements'                 => ['sortAlgorithm' => 'alpha'],
+        'ordered_class_elements'                 => ['sort_algorithm' => 'alpha'],
         'ordered_imports'                        => ['imports_order' => ['const', 'class', 'function']],
         'php_unit_strict'                        => true,
         'php_unit_set_up_tear_down_visibility'   => true,

composer.json

@@ -13,7 +13,7 @@
     },
     "require-dev": {
         "bebat/verify": "^2.0",
-        "friendsofphp/php-cs-fixer": "^2.13",
+        "friendsofphp/php-cs-fixer": "^2.17",
         "mockery/mockery": "^1.2",
         "phpunit/phpunit": "^6.2",
         "phpmd/phpmd": "^2.6"

src/AbstractKeyValueStore.php

@@ -149,7 +149,6 @@ public function rewind(): void
     /**
      * Assign a new key in store.
      *
-     *
      * @throws RuntimeException if $lookup is empty
      * @throws RuntimeException if $lookup is already set
      * @throws RuntimeException if an existing parent of lookup is a scalar value

src/DefinitionIterator.php

@@ -76,10 +76,7 @@ public function get($lookup, $definition = null)
                     $originalLookup = $lookup;
                     $lookup         = $parentLookup;
                 } else {
-                    throw new \RuntimeException(sprintf(
-                        'No definition for %s',
-                        \is_string($lookup) || is_numeric($lookup) ? $lookup : \gettype($lookup)
-                    ));
+                    throw new \RuntimeException(sprintf('No definition for %s', \is_string($lookup) || is_numeric($lookup) ? $lookup : \gettype($lookup)));
                 }
             }
 
@@ -113,11 +110,7 @@ public function get($lookup, $definition = null)
             if (\is_array($value)) {
                 $value = $this->get($originalLookup);
             } elseif (!$value instanceof Response) {
-                throw new \RuntimeException(sprintf(
-                    'Could not get nested value %s from value of type %s',
-                    $originalLookup,
-                    \gettype($value)
-                ));
+                throw new \RuntimeException(sprintf('Could not get nested value %s from value of type %s', $originalLookup, \gettype($value)));
             }
         }
 
@@ -198,11 +191,7 @@ private function getFromResolver(string $lookup, $definedValue, Resolver\Resolve
         $resolver->setIterator($this);
 
         if ($definedValue instanceof Definition && !$resolver->isValid($definedValue)) {
-            throw new \RuntimeException(sprintf(
-                'Definition %s is not valid for %s.',
-                json_encode($definedValue),
-                \get_class($resolver)
-            ));
+            throw new \RuntimeException(sprintf('Definition %s is not valid for %s.', json_encode($definedValue), \get_class($resolver)));
         }
 
         $value = $resolver->resolve($definedValue);

src/Resolver/Directory.php

@@ -61,7 +61,7 @@ public function resolve($definition)
         $filename   = $this->getIterator()->get('request.url.pathname');
         $path       = realpath($root . $filename);
 
-        if (!$path || strpos($path, $root) !== 0 || !is_file($path)) {
+        if (!$path || strpos($path, $root) !== 0 || strpos($path, $upwardRoot) !== 0 || !is_file($path)) {
             $response->setStatusCode(Response::STATUS_CODE_404);
         } else {
             $mimeType = (new MimeTypes())->getMimeType(pathinfo($path, PATHINFO_EXTENSION));

src/Resolver/File.php

@@ -9,6 +9,7 @@
 namespace Magento\Upward\Resolver;
 
 use Magento\Upward\Definition;
+use Zend\Http\Response;
 
 class File extends AbstractResolver
 {
@@ -71,9 +72,10 @@ public function isValid(Definition $definition): bool
      */
     public function resolve($definition)
     {
-        $encoding = 'utf-8';
-        $parse    = 'auto';
-        $path     = $definition;
+        $encoding   = 'utf-8';
+        $parse      = 'auto';
+        $path       = $definition;
+        $upwardRoot = $this->getIterator()->getRootDefinition()->getBasepath();
 
         if ($definition instanceof Definition) {
             $path = $this->getIterator()->get('file', $definition);
@@ -86,9 +88,13 @@ public function resolve($definition)
             }
         }
 
-        // Path is relative, expand it from definition base path.
-        if (substr($path, 0, 1) === '.') {
-            $path = realpath($this->getIterator()->getRootDefinition()->getBasepath() . \DIRECTORY_SEPARATOR . $path);
+        $path = realpath($upwardRoot . \DIRECTORY_SEPARATOR . $path);
+
+        if (!$path || strpos($path, $upwardRoot) !== 0) {
+            $response = new Response();
+            $response->setStatusCode(Response::STATUS_CODE_404);
+
+            return $response;
         }
 
         $content = file_get_contents($path);

src/Resolver/Url.php

@@ -62,8 +62,7 @@ public function resolve($definition)
         }
 
         $baseUrl = $this->getIterator()->get('baseUrl', $definition)
-            ? $this->getIterator()->get('baseUrl', $definition)
-            : self::FAKE_BASE_URL;
+            ?: self::FAKE_BASE_URL;
         $uri = UriFactory::factory($baseUrl);
 
         if ($definition->has('hostname')) {

test/AbstractKeyValueStoreTest.php

@@ -16,7 +16,7 @@ class AbstractKeyValueStoreTest extends TestCase
 {
     public function emptyKeyDataProvider(): array
     {
-        return[
+        return [
             [''],
             ['  '],
         ];

test/Resolver/DirectoryTest.php

@@ -53,7 +53,7 @@ public function dataProviderFor404(): array
             'Missing File'      => ['directory' => './_data',          'filename' => '/not-real-file.txt'],
             'Missing Directory' => ['directory' => './not-real',       'filename' => '/sample.txt'],
             'Not Directory'     => ['directory' => basename(__FILE__), 'filename' => '/sample.txt'],
-            'Outside Directory' => ['directory' => './_data',          'filename' => '/../' . basename(__FILE__)],
+            'Outside Directory' => ['directory' => '../_data',          'filename' => '/sample.txt'],
             'File is Root'      => ['directory' => './_data',          'filename' => '/'],
             'File is Directory' => ['directory' => './_data',          'filename' => '/test'],
         ];

test/Resolver/FileTest.php

@@ -14,6 +14,7 @@
 use Mockery;
 use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration;
 use PHPUnit\Framework\TestCase;
+use Zend\Http\Response;
 use function BeBat\Verify\verify;
 
 class FileTest extends TestCase
@@ -106,6 +107,17 @@ public function testResolve(): void
         verify($this->resolver->resolve($definition))->is()->sameAs("This is a sample file.\n");
     }
 
+    public function testResolve404(): void
+    {
+        $notFoundResult = $this->resolver->resolve('./_data/notfound.txt');
+        $pathResult     = $this->resolver->resolve('../_data/sample.txt');
+
+        verify($notFoundResult)->is()->instanceOf(Response::class);
+        verify($notFoundResult->getStatusCode())->is()->sameAs(404);
+        verify($pathResult)->is()->instanceOf(Response::class);
+        verify($pathResult->getStatusCode())->is()->sameAs(404);
+    }
+
     public function testResolveJson(): void
     {
         verify($this->resolver->resolve('./_data/sample.json'))->is()->sameAs(['json' => true]);

test/_data/sample.txt

@@ -0,0 +1 @@
+Nothing to see here...