diff --git a/components/ILIAS/Filesystem/src/Stream/Streams.php b/components/ILIAS/Filesystem/src/Stream/Streams.php index 81f55368948b..c9674633f9d7 100755 --- a/components/ILIAS/Filesystem/src/Stream/Streams.php +++ b/components/ILIAS/Filesystem/src/Stream/Streams.php @@ -83,16 +83,24 @@ public static function ofReattachableResource($resource): ReattachableStream public static function ofFileInsideZIP(string $path_to_zip, string $path_inside_zip): ZIPStream { - // we try to open the zip file with the path inside the zip file, once with a leading slash and once without - try { - $resource = @fopen('zip://' . $path_to_zip . '#/' . $path_inside_zip, 'rb'); - } catch (\Throwable) { - $resource = null; - } - try { - $resource = $resource ?: @fopen('zip://' . $path_to_zip . '#' . $path_inside_zip, 'rb'); - } catch (\Throwable) { - $resource = null; + // Entries are stored relative since Mantis 45580 / 47237, therefore that variant + // is tried first. Containers written before still hold entries with a leading + // slash, they are covered by the second candidate. If a container holds both + // variants of a file, the relative one is the up to date one (Mantis 48047): + // the ZIP stream wrapper matches names literally, so the entry we do not ask for + // is never returned by accident. + $relative_path = ltrim($path_inside_zip, '/'); + $resource = null; + + foreach ([$relative_path, '/' . $relative_path] as $candidate) { + try { + $resource = @fopen('zip://' . $path_to_zip . '#' . $candidate, 'rb') ?: null; + } catch (\Throwable) { + $resource = null; + } + if ($resource !== null) { + break; + } } if (!is_resource($resource)) { diff --git a/components/ILIAS/Filesystem/tests/Stream/FileInsideZIPTest.php b/components/ILIAS/Filesystem/tests/Stream/FileInsideZIPTest.php new file mode 100644 index 000000000000..8f580c88e1d8 --- /dev/null +++ b/components/ILIAS/Filesystem/tests/Stream/FileInsideZIPTest.php @@ -0,0 +1,105 @@ + + */ +final class FileInsideZIPTest extends TestCase +{ + private string $zip_file; + + protected function setUp(): void + { + parent::setUp(); + $this->zip_file = tempnam(sys_get_temp_dir(), 'irss_zip_read_test_'); + } + + protected function tearDown(): void + { + parent::tearDown(); + @unlink($this->zip_file); + } + + /** + * @param array $entries + */ + private function buildZip(array $entries): void + { + $zip = new ZipArchive(); + $zip->open($this->zip_file, ZipArchive::OVERWRITE); + foreach ($entries as $name => $content) { + $zip->addFromString($name, $content); + } + $zip->close(); + } + + public function testRelativeEntryIsRead(): void + { + $this->buildZip(['style.css' => 'relative']); + + $stream = Streams::ofFileInsideZIP($this->zip_file, 'style.css'); + + $this->assertSame('relative', (string) $stream); + } + + public function testLegacyEntryIsStillRead(): void + { + $this->buildZip(['/style.css' => 'legacy']); + + $stream = Streams::ofFileInsideZIP($this->zip_file, 'style.css'); + + $this->assertSame('legacy', (string) $stream); + } + + public function testRelativeEntryWinsOverLegacyEntry(): void + { + $this->buildZip(['/style.css' => 'legacy', 'style.css' => 'relative']); + + $stream = Streams::ofFileInsideZIP($this->zip_file, 'style.css'); + + $this->assertSame('relative', (string) $stream); + } + + public function testRelativeEntryWinsAlsoIfTheRequestedPathHasALeadingSlash(): void + { + $this->buildZip(['/style.css' => 'legacy', 'style.css' => 'relative']); + + $stream = Streams::ofFileInsideZIP($this->zip_file, '/style.css'); + + $this->assertSame('relative', (string) $stream); + } + + public function testUnknownEntryThrows(): void + { + $this->buildZip(['style.css' => 'relative']); + + $this->expectException(\InvalidArgumentException::class); + Streams::ofFileInsideZIP($this->zip_file, 'does_not_exist.css'); + } +} diff --git a/components/ILIAS/ResourceStorage/src/Resource/ResourceBuilder.php b/components/ILIAS/ResourceStorage/src/Resource/ResourceBuilder.php index 00dd9737e7ed..03aedc87c737 100755 --- a/components/ILIAS/ResourceStorage/src/Resource/ResourceBuilder.php +++ b/components/ILIAS/ResourceStorage/src/Resource/ResourceBuilder.php @@ -633,10 +633,33 @@ private function ensurePathInZIP(\ZipArchive $zip, string $path, bool $is_file): return $path_inside_container . '/' . $filename; } + /** + * Entries have been stored with a leading slash before Mantis 45580 / 47237, + * e.g. "/style.css" instead of "style.css". Both variants of an entry can exist + * side by side in containers of that age, so removing an entry has to cover both - + * otherwise the legacy entry stays in the container forever (Mantis 48047). + * + * @return string[] empty if the given path does not address an entry at all + */ + private function pathVariantsInZIP(string $path): array + { + $path = ltrim($path, '/'); + if ($path === '') { + return []; + } + + return [$path, '/' . $path]; + } + public function removePathInsideContainer( StorableContainerResource $container, string $path_inside_container, ): bool { + $paths_to_remove = $this->pathVariantsInZIP($path_inside_container); + if ($paths_to_remove === []) { + return false; + } + $revision = $container->getCurrentRevisionIncludingDraft(); $stream = $this->extractStream($revision); @@ -645,15 +668,21 @@ public function removePathInsideContainer( $zip = new \ZipArchive(); $zip->open($stream->getMetadata()['uri']); - $return = $zip->deleteName($path_inside_container); + $return = false; + foreach ($paths_to_remove as $path_to_remove) { + $return = $zip->deleteName($path_to_remove) || $return; + } // remove all files inside the directory for ($i = 0; $i < $zip->numFiles; $i++) { $path = $zip->getNameIndex($i); if ($path === false) { continue; } - if (str_starts_with($path, $path_inside_container)) { - $zip->deleteIndex($i); + foreach ($paths_to_remove as $path_to_remove) { + if (str_starts_with($path, $path_to_remove)) { + $zip->deleteIndex($i); + break; + } } } @@ -686,7 +715,9 @@ public function addUploadToContainer( $parent_path_inside_container = $this->ensurePathInZIP($zip, $parent_path_inside_container, false); - $path_inside_zip = rtrim($parent_path_inside_container, '/') . '/' . $result->getName(); + // an empty parent path must not result in an entry like "/file.txt", + // entries are stored relative since Mantis 45580 / 47237 + $path_inside_zip = ltrim(rtrim($parent_path_inside_container, '/') . '/' . $result->getName(), '/'); $return = $zip->addFile( $result->getPath(), diff --git a/components/ILIAS/ResourceStorage/tests/Resource/EnsurePathInZipTest.php b/components/ILIAS/ResourceStorage/tests/Resource/EnsurePathInZipTest.php index d7829479466a..844c7d43d088 100644 --- a/components/ILIAS/ResourceStorage/tests/Resource/EnsurePathInZipTest.php +++ b/components/ILIAS/ResourceStorage/tests/Resource/EnsurePathInZipTest.php @@ -108,4 +108,28 @@ public function testWritingRootFileProducesRelativeEntry(): void } $this->assertContains('index.html', $names); } + + /** + * Mantis 48047: containers written before the fix above still contain entries + * like "/style.css" next to the relative entry of the same file. Removing an + * entry has to cover both variants, otherwise the legacy one stays forever. + */ + #[DataProvider('pathVariantProvider')] + public function testPathVariantsCoverLegacyEntries(string $input, array $expected): void + { + $method = new \ReflectionMethod(ResourceBuilder::class, 'pathVariantsInZIP'); + $builder = (new \ReflectionClass(ResourceBuilder::class))->newInstanceWithoutConstructor(); + + $this->assertSame($expected, $method->invoke($builder, $input)); + } + + public static function pathVariantProvider(): \Iterator + { + yield 'root file' => ['style.css', ['style.css', '/style.css']]; + yield 'root file, leading slash' => ['/style.css', ['style.css', '/style.css']]; + yield 'nested file' => ['images/header.png', ['images/header.png', '/images/header.png']]; + yield 'directory' => ['images/', ['images/', '/images/']]; + yield 'empty path' => ['', []]; + yield 'slash only' => ['/', []]; + } }