diff --git a/lib/Controller/AssetController.php b/lib/Controller/AssetController.php index 2d6e22a..9a34362 100644 --- a/lib/Controller/AssetController.php +++ b/lib/Controller/AssetController.php @@ -69,6 +69,13 @@ public function fetch(string $sessionId, string $path): DataDisplayResponse|Data if ($user === null) { return new DataResponse(['error' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } + // Package HTML may only render inside the viewer iframe. Opened as a + // top-level page it would run author scripts on the Nextcloud origin. + // ponytail: browsers without Fetch Metadata pass; the opaque-origin + // sandbox is the real fix. + if ($this->request->getHeader('Sec-Fetch-Dest') === 'document') { + return new DataResponse(['error' => 'Not embeddable here'], Http::STATUS_FORBIDDEN); + } $fileId = (int)$sessionId; if ($fileId <= 0) { return new DataResponse(['error' => 'Invalid session'], Http::STATUS_BAD_REQUEST); diff --git a/lib/Controller/ThumbnailController.php b/lib/Controller/ThumbnailController.php index d680062..9a43aa1 100644 --- a/lib/Controller/ThumbnailController.php +++ b/lib/Controller/ThumbnailController.php @@ -16,6 +16,7 @@ use OCP\Files\NotPermittedException; use OCP\IRequest; use OCP\IUserSession; +use RuntimeException; /** * Returns the `screenshot.png` from inside an `.elpx` package, suitable for @@ -49,7 +50,11 @@ public function byFileId(int $fileId): DataDisplayResponse|DataResponse { return new DataResponse(['error' => $e->getMessage()], Http::STATUS_FORBIDDEN); } - $bytes = $this->zipEntries->readEntry($file, 'screenshot.png'); + try { + $bytes = $this->zipEntries->readEntry($file, 'screenshot.png', ZipEntryService::MAX_SCREENSHOT_BYTES); + } catch (RuntimeException) { + $bytes = null; + } if ($bytes === null) { return new DataResponse(['error' => 'No screenshot'], Http::STATUS_NOT_FOUND); } diff --git a/lib/Preview/ElpxPreviewProvider.php b/lib/Preview/ElpxPreviewProvider.php index 5868eda..593363e 100644 --- a/lib/Preview/ElpxPreviewProvider.php +++ b/lib/Preview/ElpxPreviewProvider.php @@ -59,7 +59,7 @@ public function isAvailable(\OCP\Files\FileInfo $file): bool { public function getThumbnail(File $file, int $maxX, int $maxY): ?IImage { try { - $bytes = $this->zipEntries->readEntry($file, 'screenshot.png'); + $bytes = $this->zipEntries->readEntry($file, 'screenshot.png', ZipEntryService::MAX_SCREENSHOT_BYTES); if ($bytes !== null) { $image = new Image(); $image->loadFromData($bytes); diff --git a/lib/Service/ZipEntryService.php b/lib/Service/ZipEntryService.php index 8a42797..f13bb58 100644 --- a/lib/Service/ZipEntryService.php +++ b/lib/Service/ZipEntryService.php @@ -18,6 +18,12 @@ class ZipEntryService { public const MAX_ENTRIES = 5000; public const MAX_UNCOMPRESSED_SIZE_BYTES = 500 * 1024 * 1024; + /** + * Cap for `screenshot.png`. Thumbnails run in cron and the Files grid, so + * a package declaring a huge screenshot must not reserve a buffer anywhere + * near `memory_limit`. + */ + public const MAX_SCREENSHOT_BYTES = 10 * 1024 * 1024; /** * The limits are injectable so tests can exercise them with small @@ -36,8 +42,10 @@ public function __construct( * Entry names that are not already canonical — traversal, absolute paths, * dot segments, doubled slashes, backslashes — are rejected outright by * {@see self::normalizeEntry()} rather than repaired. + * + * `$maxBytes` tightens the uncompressed-size limit for this one read. */ - public function readEntry(File $file, string $entry): ?string { + public function readEntry(File $file, string $entry, ?int $maxBytes = null): ?string { $normalized = $this->normalizeEntry($entry); if ($normalized === null) { return null; @@ -45,7 +53,7 @@ public function readEntry(File $file, string $entry): ?string { $localPath = $file->getStorage()->getLocalFile($file->getInternalPath()); if (!is_string($localPath) || $localPath === '') { - return $this->readEntryFromStream($file, $normalized); + return $this->readEntryFromStream($file, $normalized, $maxBytes); } $zip = new ZipArchive(); @@ -54,10 +62,10 @@ public function readEntry(File $file, string $entry): ?string { // 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); + return $this->readEntryFromStream($file, $normalized, $maxBytes); } try { - return $this->extractEntry($zip, $normalized); + return $this->extractEntry($zip, $normalized, $maxBytes); } finally { $zip->close(); } @@ -68,7 +76,7 @@ public function readEntry(File $file, string $entry): ?string { * 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 { + private function extractEntry(ZipArchive $zip, string $entry, ?int $maxBytes): ?string { if ($zip->numFiles > $this->maxEntries) { return null; } @@ -77,7 +85,7 @@ private function extractEntry(ZipArchive $zip, string $entry): ?string { return null; } $declaredSize = $stat['size']; - if ($declaredSize > $this->maxUncompressedSizeBytes) { + if ($declaredSize > min($this->maxUncompressedSizeBytes, $maxBytes ?? PHP_INT_MAX)) { throw new RuntimeException('Uncompressed entry exceeds limit'); } // getFromName() allocates its whole $len buffer up front, so the read @@ -161,7 +169,7 @@ public function normalizeEntry(string $entry): ?string { * Fallback for storages that cannot expose a local file (object storage, * external mounts). Copies the package to a temp file first. */ - private function readEntryFromStream(File $file, string $entry): ?string { + private function readEntryFromStream(File $file, string $entry, ?int $maxBytes): ?string { $tmp = tempnam(sys_get_temp_dir(), 'elpx_'); if ($tmp === false) { return null; @@ -183,7 +191,7 @@ private function readEntryFromStream(File $file, string $entry): ?string { return null; } try { - return $this->extractEntry($zip, $entry); + return $this->extractEntry($zip, $entry, $maxBytes); } finally { $zip->close(); } diff --git a/tests/Unit/Controller/AssetControllerTest.php b/tests/Unit/Controller/AssetControllerTest.php index 4fed135..3c86195 100644 --- a/tests/Unit/Controller/AssetControllerTest.php +++ b/tests/Unit/Controller/AssetControllerTest.php @@ -17,18 +17,20 @@ use PHPUnit\Framework\TestCase; final class AssetControllerTest extends TestCase { + private IRequest $request; private IUserSession $userSession; private ElpxPackageService $packages; private ZipEntryService $zipEntries; private AssetController $controller; protected function setUp(): void { + $this->request = $this->createMock(IRequest::class); $this->userSession = $this->createMock(IUserSession::class); $this->packages = $this->createMock(ElpxPackageService::class); $this->zipEntries = $this->createMock(ZipEntryService::class); $this->controller = new AssetController( 'exelearning', - $this->createMock(IRequest::class), + $this->request, $this->userSession, $this->packages, $this->zipEntries, @@ -44,6 +46,16 @@ public function testRejectsAnonymousRequests(): void { self::assertSame(['error' => 'Not authenticated'], $response->getData()); } + public function testRefusesTopLevelNavigation(): void { + $this->authenticate(); + $this->request->method('getHeader')->with('Sec-Fetch-Dest')->willReturn('document'); + $this->packages->expects(self::never())->method('getForUserById'); + + $response = $this->controller->fetch('42', 'index.html'); + + self::assertSame(Http::STATUS_FORBIDDEN, $response->getStatus()); + } + public function testRejectsInvalidSessionId(): void { $this->authenticate(); diff --git a/tests/Unit/Controller/ThumbnailControllerTest.php b/tests/Unit/Controller/ThumbnailControllerTest.php index 207009f..ac2c610 100644 --- a/tests/Unit/Controller/ThumbnailControllerTest.php +++ b/tests/Unit/Controller/ThumbnailControllerTest.php @@ -60,7 +60,7 @@ public function testReturnsNotFoundWithoutScreenshot(): void { $this->authenticate(); $file = $this->createMock(File::class); $this->packages->method('getForUserById')->willReturn($file); - $this->zipEntries->method('readEntry')->with($file, 'screenshot.png')->willReturn(null); + $this->zipEntries->method('readEntry')->with($file, 'screenshot.png', ZipEntryService::MAX_SCREENSHOT_BYTES)->willReturn(null); $response = $this->controller->byFileId(42); @@ -68,6 +68,16 @@ public function testReturnsNotFoundWithoutScreenshot(): void { self::assertSame(['error' => 'No screenshot'], $response->getData()); } + public function testTreatsOversizedScreenshotAsMissing(): void { + $this->authenticate(); + $this->packages->method('getForUserById')->willReturn($this->createMock(File::class)); + $this->zipEntries->method('readEntry')->willThrowException(new \RuntimeException('too big')); + + $response = $this->controller->byFileId(42); + + self::assertSame(Http::STATUS_NOT_FOUND, $response->getStatus()); + } + public function testReturnsPngWithPrivateCacheHeaders(): void { $this->authenticate(); $file = $this->createMock(File::class); diff --git a/tests/Unit/Service/ZipEntryServiceTest.php b/tests/Unit/Service/ZipEntryServiceTest.php index 9ecf6c0..c6e90a4 100644 --- a/tests/Unit/Service/ZipEntryServiceTest.php +++ b/tests/Unit/Service/ZipEntryServiceTest.php @@ -157,6 +157,15 @@ public function testLocalPathRejectsEntriesOverTheUncompressedSizeLimit(): void } } + public function testPerReadCapTightensTheUncompressedSizeLimit(): void { + $archive = $this->createTestArchive('screenshot.png', '0123456789'); + $file = $this->createFakeFile($archive, ''); + + self::assertSame('0123456789', $this->service->readEntry($file, 'screenshot.png', 10)); + $this->expectException(RuntimeException::class); + $this->service->readEntry($file, 'screenshot.png', 9); + } + public function testStreamFallbackReturnsNullWhenArchiveHasTooManyEntries(): void { $service = new ZipEntryService(maxEntries: 1); $archivePath = tempnam(sys_get_temp_dir(), 'elpx_test_');