Merge pull request #9058 from Seldaek/zip-cleanup

Clean up Zip Util to be more strict about what is a valid package archive

src/Composer/Repository/ArtifactRepository.php

@@ -88,7 +88,12 @@ class ArtifactRepository extends ArrayRepository implements ConfigurableReposito
 
     private function getComposerInformation(\SplFileInfo $file)
     {
+        $json = null;
+        try {
             $json = Zip::getComposerJson($file->getPathname());
+        } catch (\Exception $exception) {
+            $this->io->write('Failed loading package '.$file->getPathname().': '.$exception->getMessage(), false, IOInterface::VERBOSE);
+        }
 
         if (null === $json) {
             return false;

src/Composer/Util/Zip.php

@@ -66,42 +66,44 @@ class Zip
      *
      * @param \ZipArchive $zip
      * @param string      $filename
+     * @throws \RuntimeException
      *
-     * @return bool|int
+     * @return int
      */
     private static function locateFile(\ZipArchive $zip, $filename)
     {
-        $indexOfShortestMatch = false;
-        $lengthOfShortestMatch = -1;
-
-        for ($i = 0; $i < $zip->numFiles; $i++) {
-            $stat = $zip->statIndex($i);
-            if (strcmp(basename($stat['name']), $filename) === 0) {
-                $directoryName = dirname($stat['name']);
-                if ($directoryName === '.') {
-                    //if composer.json is in root directory
-                    //it has to be the one to use.
-                    return $i;
+        // return root composer.json if it is there and is a file
+        if (false !== ($index = $zip->locateName($filename)) && $zip->getFromIndex($index) !== false) {
+            return $index;
         }
 
-                if (strpos($directoryName, '\\') !== false ||
-                    strpos($directoryName, '/') !== false) {
-                    //composer.json files below first directory are rejected
+        $topLevelPaths = array();
+        for ($i = 0; $i < $zip->numFiles; $i++) {
+            $name = $zip->getNameIndex($i);
+            $dirname = dirname($name);
+
+            // handle archives with proper TOC
+            if ($dirname === '.') {
+                $topLevelPaths[$name] = true;
+                if (\count($topLevelPaths) > 1) {
+                    throw new \RuntimeException('Archive has more than one top level directories, and no composer.json was found on the top level, so it\'s an invalid archive. Top level paths found were: '.implode(',', array_keys($topLevelPaths)));
+                }
                 continue;
             }
 
-                $length = strlen($stat['name']);
-                if ($indexOfShortestMatch === false || $length < $lengthOfShortestMatch) {
-                    //Check it's not a directory.
-                    $contents = $zip->getFromIndex($i);
-                    if ($contents !== false) {
-                        $indexOfShortestMatch = $i;
-                        $lengthOfShortestMatch = $length;
-                    }
+            // handle archives which do not have a TOC record for the directory itself
+            if (false === strpos('\\', $dirname) && false === strpos('/', $dirname)) {
+                $topLevelPaths[$dirname.'/'] = true;
+                if (\count($topLevelPaths) > 1) {
+                    throw new \RuntimeException('Archive has more than one top level directories, and no composer.json was found on the top level, so it\'s an invalid archive. Top level paths found were: '.implode(',', array_keys($topLevelPaths)));
                 }
             }
         }
 
-        return $indexOfShortestMatch;
+        if ($topLevelPaths && false !== ($index = $zip->locateName(key($topLevelPaths).$filename)) && $zip->getFromIndex($index) !== false) {
+            return $index;
+        }
+
+        throw new \RuntimeException('No composer.json found either at the top level or within the topmost directory');
     }
 }

tests/Composer/Test/Util/ZipTest.php

@@ -20,7 +20,7 @@ use Composer\Test\TestCase;
  */
 class ZipTest extends TestCase
 {
-    public function testThrowsExceptionIfZipExcentionIsNotLoaded()
+    public function testThrowsExceptionIfZipExtensionIsNotLoaded()
     {
         if (extension_loaded('zip')) {
             $this->markTestSkipped('The PHP zip extension is loaded.');
@@ -55,28 +55,30 @@ class ZipTest extends TestCase
         $this->assertNull($result);
     }
 
-    public function testReturnsNullIfTheZipHasNoComposerJson()
+    /**
+     * @expectedException \RuntimeException
+     */
+    public function testThrowsExceptionIfTheZipHasNoComposerJson()
     {
         if (!extension_loaded('zip')) {
             $this->markTestSkipped('The PHP zip extension is not loaded.');
             return;
         }
 
-        $result = Zip::getComposerJson(__DIR__.'/Fixtures/Zip/nojson.zip');
-
-        $this->assertNull($result);
+        Zip::getComposerJson(__DIR__.'/Fixtures/Zip/nojson.zip');
     }
 
-    public function testReturnsNullIfTheComposerJsonIsInASubSubfolder()
+    /**
+     * @expectedException \RuntimeException
+     */
+    public function testThrowsExceptionIfTheComposerJsonIsInASubSubfolder()
     {
         if (!extension_loaded('zip')) {
             $this->markTestSkipped('The PHP zip extension is not loaded.');
             return;
         }
 
-        $result = Zip::getComposerJson(__DIR__.'/Fixtures/Zip/subfolder.zip');
-
-        $this->assertNull($result);
+        Zip::getComposerJson(__DIR__.'/Fixtures/Zip/subfolders.zip');
     }
 
     public function testReturnsComposerJsonInZipRoot()
@@ -99,19 +101,44 @@ class ZipTest extends TestCase
         }
 
         $result = Zip::getComposerJson(__DIR__.'/Fixtures/Zip/folder.zip');
-
         $this->assertEquals("{\n    \"name\": \"foo/bar\"\n}\n", $result);
     }
 
-    public function testReturnsRootComposerJsonAndSkipsSubfolders()
+    /**
+     * @expectedException \RuntimeException
+     */
+    public function testMultipleTopLevelDirsIsInvalid()
     {
         if (!extension_loaded('zip')) {
             $this->markTestSkipped('The PHP zip extension is not loaded.');
             return;
         }
 
-        $result = Zip::getComposerJson(__DIR__.'/Fixtures/Zip/multiple.zip');
+        Zip::getComposerJson(__DIR__.'/Fixtures/Zip/multiple.zip');
+    }
+
+    public function testReturnsComposerJsonFromFirstSubfolder()
+    {
+        if (!extension_loaded('zip')) {
+            $this->markTestSkipped('The PHP zip extension is not loaded.');
+            return;
+        }
+
+        $result = Zip::getComposerJson(__DIR__.'/Fixtures/Zip/single-sub.zip');
 
         $this->assertEquals("{\n    \"name\": \"foo/bar\"\n}\n", $result);
     }
+
+    /**
+     * @expectedException \RuntimeException
+     */
+    public function testThrowsExceptionIfMultipleComposerInSubFoldersWereFound()
+    {
+        if (!extension_loaded('zip')) {
+            $this->markTestSkipped('The PHP zip extension is not loaded.');
+            return;
+        }
+
+        Zip::getComposerJson(__DIR__.'/Fixtures/Zip/multiple_subfolders.zip');
+    }
 }