[Fix] #48134 container unzip performance

components/ILIAS/ResourceStorage/classes/Container/Wrapper/ContainerWrapper.php

@@ -22,8 +22,8 @@
 
 use ILIAS\ResourceStorage\Identification\ResourceIdentification;
 use ILIAS\FileDelivery\Delivery\Disposition;
-use ILIAS\Filesystem\Stream\Streams;
 use ILIAS\Filesystem\Stream\ZIPStream;
+use ILIAS\Filesystem\Util\Archive\Zip;
 
 /**
  * @author   Fabian Schmid <fabian@sr.solutions>
@@ -125,26 +125,80 @@ public function unzip(string $path_inside_zip): bool
         $tmp_file = tempnam($tmp_directory, 'ilias_zip_');
 
         /** @var ZIPStream $stream */
-        $return = file_put_contents($tmp_file, $stream->detach());
+        file_put_contents($tmp_file, $stream->detach());
 
-        $zip_reader = new ZipReader(
-            Streams::ofResource(fopen($tmp_file, 'rb'))
-        );
+        $tmp_extract_directory = $tmp_file . '_extracted';
 
-        foreach ($zip_reader->getStructure() as $append_path_inside_zip => $item) {
-            if ($item['is_dir']) {
-                continue;
+        try {
+            $zip = new \ZipArchive();
+            if ($zip->open($tmp_file, \ZipArchive::RDONLY) !== true) {
+                return false;
+            }
+
+            // collect the entries we accept, they are extracted in one single go
+            $entries = [];
+            for ($i = 0; $i < $zip->count(); $i++) {
+                $entry = $zip->getNameIndex($i, \ZipArchive::FL_UNCHANGED);
+                if ($entry === false
+                    || str_ends_with($entry, '/')
+                    || str_ends_with($entry, '\\')
+                    || in_array(basename($entry), $this->ignored, true)
+                    || basename($entry) === Zip::DOT_EMPTY
+                    || !$this->isSafePathInsideZip($entry)
+                ) {
+                    continue;
+                }
+                $entries[] = $entry;
+            }
+            if ($entries === []) {
+                $zip->close();
+                return false;
+            }
+            if (!is_dir($tmp_extract_directory)
+                && !mkdir($tmp_extract_directory, 0777, true)
+                && !is_dir($tmp_extract_directory)
+            ) {
+                $zip->close();
+                return false;
+            }
+            $zip->extractTo($tmp_extract_directory, $entries);
+            $zip->close();
+
+            $files = [];
+            foreach ($entries as $entry) {
+                $local_path = $tmp_extract_directory . DIRECTORY_SEPARATOR . $entry;
+                if (!is_file($local_path)) {
+                    continue;
+                }
+                $files[$this->current_level . '/' . ltrim($entry, './')] = $local_path;
             }
-            [$stream, $info] = $zip_reader->getItem($append_path_inside_zip, $this->data);
-            $this->irss->manageContainer()->addStreamToContainer(
+
+            // one single call, otherwise the container ZIP is rewritten per file
+            return $this->irss->manageContainer()->addFilesToContainer(
                 $this->rid,
-                $stream,
-                $this->current_level . '/' . ltrim($append_path_inside_zip, './')
+                $files
             );
+        } finally {
+            if (is_file($tmp_file)) {
+                unlink($tmp_file);
+            }
+            if (is_dir($tmp_extract_directory)) {
+                \ilFileUtils::delDir($tmp_extract_directory);
+            }
+        }
+    }
+
+    /**
+     * Rejects absolute paths and paths escaping the extraction directory (zip slip).
+     */
+    private function isSafePathInsideZip(string $path): bool
+    {
+        $path = str_replace('\\', '/', $path);
+        if (str_starts_with($path, '/') || preg_match('#^[a-zA-Z]:#', $path) === 1) {
+            return false;
         }
 
-        unlink($tmp_file);
-        return true;
+        return !in_array('..', explode('/', $path), true);
     }
 
     public function getEntries(): \Generator

components/ILIAS/ResourceStorage/classes/Container/class.ilContainerResourceGUI.php

@@ -356,12 +356,19 @@ private function unzip(): void
             $this->abortWithPermissionDenied();
             return;
         }
-        $paths = $this->getPathsFromRequest()[0];
-        $this->view_request->getWrapper()->unzip(
-            $paths
+        $paths = $this->getPathsFromRequest();
+
+        // the unzip action is triggered by a GET request, therefore we must not
+        // rely on postUpload() which determines its message from the POST body.
+        $success = $paths !== [] && $this->view_request->getWrapper()->unzip($paths[0]);
+
+        $this->main_tpl->setOnScreenMessage(
+            $success ? 'success' : 'failure',
+            $this->language->txt($success ? 'rids_appended' : 'rids_appended_failed'),
+            true
         );
 
-        $this->postUpload();
+        $this->ctrl->redirect($this, self::CMD_INDEX);
     }
 
     private function renderConfirmRemove(): void

components/ILIAS/ResourceStorage/src/Manager/ContainerManager.php

@@ -122,4 +122,31 @@ public function addStreamToContainer(
         );
     }
 
+    /**
+     * Adds multiple files at once, which rewrites the container ZIP only a
+     * single time. Prefer this over repeated addStreamToContainer calls.
+     * @param array<string, string> $files path inside container => absolute local path
+     */
+    public function addFilesToContainer(
+        ResourceIdentification $container,
+        array $files,
+    ): bool {
+        $normalized = [];
+        foreach ($files as $path_inside_container => $local_path) {
+            $path_inside_container = $this->normalizePath((string) $path_inside_container);
+            if (empty($path_inside_container)) {
+                continue;
+            }
+            $normalized[$path_inside_container] = $local_path;
+        }
+        if ($normalized === []) {
+            return false;
+        }
+
+        return $this->resource_builder->addFilesToContainer(
+            $this->getResource($container),
+            $normalized
+        );
+    }
+
 }

components/ILIAS/ResourceStorage/src/Resource/ResourceBuilder.php

@@ -742,6 +742,56 @@ public function addStreamToContainer(
         return true;
     }
 
+    /**
+     * Adds multiple files to a container at once: the ZIP is rewritten a single
+     * time, the flavours are cleared once and the revision is stored once.
+     * Adding the files one by one would rewrite the whole archive per file,
+     * since ZipArchive::close() never appends in place.
+     * @param array<string, string> $files path inside container => absolute local path
+     */
+    public function addFilesToContainer(
+        StorableContainerResource $container,
+        array $files,
+    ): bool {
+        if ($files === []) {
+            return true;
+        }
+
+        $revision = $container->getCurrentRevisionIncludingDraft();
+        $revision_stream = $this->extractStream($revision);
+        $uri = $revision_stream->getMetadata()['uri'];
+
+        try {
+            $zip = new \ZipArchive();
+            if ($zip->open($uri) !== true) {
+                return false;
+            }
+
+            $return = true;
+            foreach ($files as $path_inside_container => $local_path) {
+                $path_inside_container = $this->ensurePathInZIP($zip, (string) $path_inside_container, true);
+                if ($path_inside_container === '') {
+                    $return = false;
+                    continue;
+                }
+                // libzip reads the source file lazily on close(), therefore the
+                // contents are never held in memory (unlike addFromString).
+                $return = $zip->addFile($local_path, $path_inside_container) && $return;
+            }
+            $zip->close();
+
+            // cleanup revision and flavours
+            $this->storage_handler_factory->getHandlerForRevision($revision)->clearFlavours($revision);
+            $revision->getInformation()->setSize(filesize($uri));
+            $this->storeRevision($revision);
+
+            return $return;
+        } catch (\Throwable $exception) {
+            $this->storage_handler_factory->getHandlerForRevision($revision)->clearFlavours($revision);
+            return false;
+        }
+    }
+
     private function deleteRevision(StorableResource $resource, Revision $revision): void
     {
         try {