diff --git a/lib/Service/ZipEntryService.php b/lib/Service/ZipEntryService.php index db55898..93ceec8 100644 --- a/lib/Service/ZipEntryService.php +++ b/lib/Service/ZipEntryService.php @@ -19,6 +19,16 @@ class ZipEntryService { public const MAX_ENTRIES = 5000; public const MAX_UNCOMPRESSED_SIZE_BYTES = 500 * 1024 * 1024; + /** + * The limits are injectable so tests can exercise them with small + * archives; production callers rely on the defaults. + */ + public function __construct( + private readonly int $maxEntries = self::MAX_ENTRIES, + private readonly int $maxUncompressedSizeBytes = self::MAX_UNCOMPRESSED_SIZE_BYTES, + ) { + } + /** * Reads a single entry from the package. Returns the raw bytes or null if * the entry is not present. @@ -32,31 +42,56 @@ public function readEntry(File $file, string $entry): ?string { } $localPath = $file->getStorage()->getLocalFile($file->getInternalPath()); - if ($localPath === false || $localPath === null) { + if (!is_string($localPath) || $localPath === '') { return $this->readEntryFromStream($file, $normalized); } $zip = new ZipArchive(); if ($zip->open($localPath) !== true) { - return null; + // Some virtual storage implementations (notably the php-wasm + // Playground filesystem) expose a nominal local path that native + // ZipArchive still cannot open. Treat it like any other non-local + // storage and retry through the portable File::fopen() path. + return $this->readEntryFromStream($file, $normalized); } try { - if ($zip->numFiles > self::MAX_ENTRIES) { - return null; - } - $contents = $zip->getFromName($normalized); - if ($contents === false) { - return null; - } - if (strlen($contents) > self::MAX_UNCOMPRESSED_SIZE_BYTES) { - throw new RuntimeException('Uncompressed entry exceeds limit'); - } - return $contents; + return $this->extractEntry($zip, $normalized); } finally { $zip->close(); } } + /** + * Reads one entry from an opened archive. Both the local-path branch and + * the stream fallback go through here so they enforce identical limits + * and agree on returning null for missing entries. + */ + private function extractEntry(ZipArchive $zip, string $entry): ?string { + if ($zip->numFiles > $this->maxEntries) { + return null; + } + $stat = $zip->statName($entry); + if ($stat === false) { + return null; + } + $declaredSize = $stat['size']; + if ($declaredSize > $this->maxUncompressedSizeBytes) { + throw new RuntimeException('Uncompressed entry exceeds limit'); + } + // getFromName() allocates its whole $len buffer up front, so the read + // must be bounded by the (already limit-checked) declared size rather + // than the limit itself. The extra byte exposes archives whose + // central directory understates the real entry size. + $contents = $zip->getFromName($entry, $declaredSize + 1); + if ($contents === false) { + return null; + } + if (strlen($contents) > $declaredSize) { + throw new RuntimeException('Entry is larger than its declared size'); + } + return $contents; + } + /** * Lists entry names (no contents). Useful for validating that a package * is shaped like an eXeLearning project. @@ -74,7 +109,7 @@ public function listEntries(File $file): array { } $entries = []; try { - $count = min($zip->numFiles, self::MAX_ENTRIES); + $count = min($zip->numFiles, $this->maxEntries); for ($i = 0; $i < $count; $i++) { $name = $zip->getNameIndex($i); if (is_string($name)) { @@ -132,11 +167,7 @@ private function readEntryFromStream(File $file, string $entry): ?string { return null; } try { - if ($zip->numFiles > self::MAX_ENTRIES) { - return null; - } - $contents = $zip->getFromName($entry); - return $contents === false ? null : $contents; + return $this->extractEntry($zip, $entry); } finally { $zip->close(); } diff --git a/tests/Unit/Service/ZipEntryServiceTest.php b/tests/Unit/Service/ZipEntryServiceTest.php index e0f792d..6522014 100644 --- a/tests/Unit/Service/ZipEntryServiceTest.php +++ b/tests/Unit/Service/ZipEntryServiceTest.php @@ -6,11 +6,13 @@ use OCA\ExeLearning\Service\ZipEntryService; use PHPUnit\Framework\TestCase; +use RuntimeException; /** - * Tests for {@see ZipEntryService::normalizeEntry}. The reading methods need a - * Nextcloud File handle which is too heavy to fake here — they are exercised - * by integration tests that run against a real server. + * Tests for {@see ZipEntryService}. `readEntry()` takes a Nextcloud `File`, + * which is mocked here (against the interface stub from + * `bootstrap-standalone.php`) so the local-path and stream-fallback branches + * can be exercised without a real Nextcloud server or storage backend. */ final class ZipEntryServiceTest extends TestCase { private ZipEntryService $service; @@ -44,4 +46,129 @@ public function testRejectsEmptyAndNulTaintedPaths(): void { self::assertNull($this->service->normalizeEntry('')); self::assertNull($this->service->normalizeEntry("a\0b")); } + + public function testReadEntryFallsBackToStreamWhenLocalPathCannotBeOpened(): void { + // Some virtual storage implementations (notably the php-wasm + // Playground filesystem) expose a nominal local path that native + // ZipArchive still cannot open. + $archive = $this->createTestArchive('index.html', '

Playground

'); + $file = $this->createFakeFile($archive, '/virtual/php-wasm/package.elpx'); + + self::assertSame('

Playground

', $this->service->readEntry($file, 'index.html')); + } + + public function testReadEntryFallsBackToStreamWhenLocalFileIsEmptyString(): void { + // S3/object storage and other non-local primary storages commonly + // return an empty string — not false/null — from getLocalFile() when + // there is no local path at all. That empty string must not be + // handed to ZipArchive::open(), which would emit a PHP warning and + // leave the entry unreadable. + $archive = $this->createTestArchive('content.xml', ''); + $file = $this->createFakeFile($archive, ''); + + self::assertSame('', $this->service->readEntry($file, 'content.xml')); + } + + public function testReadsZeroByteEntriesAsEmptyString(): void { + // A declared size of 0 means the bounded read asks for a single byte; + // getFromName() must still report the empty entry as '' — not false. + $archive = $this->createTestArchive('empty.txt', ''); + $file = $this->createFakeFile($archive, ''); + + self::assertSame('', $this->service->readEntry($file, 'empty.txt')); + } + + public function testStreamFallbackRejectsEntriesOverTheUncompressedSizeLimit(): void { + // Both branches must enforce the same limits: an oversized entry has + // to be rejected whether the package is opened from a local path or + // through the File::fopen() fallback. + $service = new ZipEntryService(maxUncompressedSizeBytes: 8); + $archive = $this->createTestArchive('index.html', '

Playground

'); + $file = $this->createFakeFile($archive, ''); + + $this->expectException(RuntimeException::class); + $service->readEntry($file, 'index.html'); + } + + public function testLocalPathRejectsEntriesOverTheUncompressedSizeLimit(): void { + $service = new ZipEntryService(maxUncompressedSizeBytes: 8); + $archive = $this->createTestArchive('index.html', '

Playground

'); + $archivePath = tempnam(sys_get_temp_dir(), 'elpx_test_'); + self::assertNotFalse($archivePath); + file_put_contents($archivePath, $archive); + $file = $this->createFakeFile($archive, $archivePath); + + try { + $this->expectException(RuntimeException::class); + $service->readEntry($file, 'index.html'); + } finally { + @unlink($archivePath); + } + } + + public function testStreamFallbackReturnsNullWhenArchiveHasTooManyEntries(): void { + $service = new ZipEntryService(maxEntries: 1); + $archivePath = tempnam(sys_get_temp_dir(), 'elpx_test_'); + self::assertNotFalse($archivePath); + $zip = new \ZipArchive(); + self::assertTrue($zip->open($archivePath, \ZipArchive::OVERWRITE) === true); + $zip->addFromString('index.html', '

Playground

'); + $zip->addFromString('content.xml', ''); + $zip->close(); + $archive = file_get_contents($archivePath); + @unlink($archivePath); + self::assertIsString($archive); + $file = $this->createFakeFile($archive, ''); + + self::assertNull($service->readEntry($file, 'index.html')); + } + + /** + * Builds a minimal in-memory ZIP archive and returns its raw bytes. + */ + private function createTestArchive(string $entryName, string $entryContents): string { + $archivePath = tempnam(sys_get_temp_dir(), 'elpx_test_'); + self::assertNotFalse($archivePath); + $zip = new \ZipArchive(); + self::assertTrue($zip->open($archivePath, \ZipArchive::OVERWRITE) === true); + $zip->addFromString($entryName, $entryContents); + $zip->close(); + $archive = file_get_contents($archivePath); + @unlink($archivePath); + self::assertIsString($archive); + return $archive; + } + + /** + * Mocks a Nextcloud `File` whose storage reports `$localPath` as the + * local path (any value `ZipEntryService` should NOT be able to open + * directly — e.g. a virtual path or an empty string) and whose + * `fopen()` streams the given archive bytes. + */ + private function createFakeFile(string $archive, string $localPath): \OCP\Files\File { + $storage = new class($localPath) { + public function __construct( + private readonly string $localPath, + ) { + } + + public function getLocalFile(string $path): string { + return $this->localPath; + } + }; + + $file = $this->createMock(\OCP\Files\File::class); + $file->method('getStorage')->willReturn($storage); + $file->method('getInternalPath')->willReturn('files/package.elpx'); + $file->method('fopen')->with('rb')->willReturnCallback( + static function () use ($archive) { + $stream = fopen('php://temp', 'w+b'); + fwrite($stream, $archive); + rewind($stream); + return $stream; + }, + ); + + return $file; + } } diff --git a/tests/bootstrap-standalone.php b/tests/bootstrap-standalone.php index ae11387..0188c19 100644 --- a/tests/bootstrap-standalone.php +++ b/tests/bootstrap-standalone.php @@ -44,6 +44,19 @@ public function boot(IBootContext $context): void; if (!interface_exists('OCP\\IPreview', false)) { eval('namespace OCP; interface IPreview {}'); } +if (!interface_exists('OCP\\Files\\File', false)) { + // Like the real OCP\Files\File (an interface extending Node), but reduced + // to the members our code touches. Methods stay untyped as in the real + // API so PHPUnit mocks can implement them freely. + eval(' + namespace OCP\\Files; + interface File { + public function getStorage(); + public function getInternalPath(); + public function fopen($mode); + } + '); +} if (!class_exists('OCP\\Util', false)) { eval('namespace OCP; class Util { public static function addInitScript(string $app, string $script): void {} public static function addScript(string $app, string $script): void {} }'); }