From b12a390220a0a38ec18557b52c2eec7cf25dfc49 Mon Sep 17 00:00:00 2001 From: Scott Dutton Date: Sun, 12 Apr 2020 16:38:59 +0100 Subject: [PATCH 1/2] Always try and show pre rendered preview Currently if the following situation happens Server generates preview Server has command removed which allows a preview to be shown Client asks for preview, gets a 404 error when preview exists (Mime checked before preview) This happens more often with documents, or video as the commands are not native PHP, they require a binary on the server. After the fix the following would happen Server generates preview Server has command removed which allows a preview to be shown Client asks for preview, gets preview which has been generated (Mime checked after preview) This would also allow offline generation (for example a docker image containing the extra binaries), allowing a reduction in attack surface of the instance serving the preview data. Signed-off-by: Scott Dutton --- lib/private/Preview/Generator.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/private/Preview/Generator.php b/lib/private/Preview/Generator.php index e47a7e5927..d55aa1cb28 100644 --- a/lib/private/Preview/Generator.php +++ b/lib/private/Preview/Generator.php @@ -126,9 +126,6 @@ class Generator { if ($mimeType === null) { $mimeType = $file->getMimeType(); } - if (!$this->previewManager->isMimeSupported($mimeType)) { - throw new NotFoundException(); - } $previewFolder = $this->getPreviewFolder($file); @@ -155,7 +152,7 @@ class Generator { $crop = $specification['crop'] ?? false; $mode = $specification['mode'] ?? IPreview::MODE_FILL; - // If both width and heigth are -1 we just want the max preview + // If both width and height are -1 we just want the max preview if ($width === -1 && $height === -1) { $width = $maxWidth; $height = $maxHeight; @@ -176,6 +173,10 @@ class Generator { try { $preview = $this->getCachedPreview($previewFolder, $width, $height, $crop, $maxPreview->getMimeType(), $previewVersion); } catch (NotFoundException $e) { + if (!$this->previewManager->isMimeSupported($mimeType)) { + throw new NotFoundException(); + } + if ($maxPreviewImage === null) { $maxPreviewImage = $this->helper->getImage($maxPreview); } From 8e249569af23fc461c109ec3a1abcc382c09d7d7 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Thu, 13 Aug 2020 23:24:21 +0200 Subject: [PATCH 2/2] Fix existing test and add a specific one for the new case Signed-off-by: Morris Jobke --- tests/lib/Preview/GeneratorTest.php | 81 +++++++++++++++++++++++++---- 1 file changed, 72 insertions(+), 9 deletions(-) diff --git a/tests/lib/Preview/GeneratorTest.php b/tests/lib/Preview/GeneratorTest.php index d2bfcad995..43f5c1e0d3 100644 --- a/tests/lib/Preview/GeneratorTest.php +++ b/tests/lib/Preview/GeneratorTest.php @@ -256,25 +256,92 @@ class GeneratorTest extends \Test\TestCase { $file = $this->createMock(File::class); $file->method('isReadable') ->willReturn(true); + $file->method('getId') + ->willReturn(42); $this->previewManager->method('isMimeSupported') ->with('invalidType') ->willReturn(false); + $previewFolder = $this->createMock(ISimpleFolder::class); + $this->appData->method('getFolder') + ->with($this->equalTo(42)) + ->willReturn($previewFolder); + + $maxPreview = $this->createMock(ISimpleFile::class); + $maxPreview->method('getName') + ->willReturn('2048-2048-max.png'); + $maxPreview->method('getMimeType') + ->willReturn('image/png'); + + $previewFolder->method('getDirectoryListing') + ->willReturn([$maxPreview]); + + $previewFolder->method('getFile') + ->with($this->equalTo('1024-512-crop.png')) + ->willThrowException(new NotFoundException()); + $this->eventDispatcher->expects($this->once()) ->method('dispatch') ->with( $this->equalTo(IPreview::EVENT), $this->callback(function (GenericEvent $event) use ($file) { return $event->getSubject() === $file && - $event->getArgument('width') === 0 && - $event->getArgument('height') === 0 && - $event->getArgument('crop') === true && - $event->getArgument('mode') === IPreview::MODE_COVER; + $event->getArgument('width') === 1024 && + $event->getArgument('height') === 512 && + $event->getArgument('crop') === true && + $event->getArgument('mode') === IPreview::MODE_COVER; }) ); - $this->generator->getPreview($file, 0, 0, true, IPreview::MODE_COVER, 'invalidType'); + $this->generator->getPreview($file, 1024, 512, true, IPreview::MODE_COVER, 'invalidType'); + } + + public function testReturnCachedPreviewsWithoutCheckingSupportedMimetype() { + $file = $this->createMock(File::class); + $file->method('isReadable') + ->willReturn(true); + $file->method('getId') + ->willReturn(42); + + + $previewFolder = $this->createMock(ISimpleFolder::class); + $this->appData->method('getFolder') + ->with($this->equalTo(42)) + ->willReturn($previewFolder); + + $maxPreview = $this->createMock(ISimpleFile::class); + $maxPreview->method('getName') + ->willReturn('2048-2048-max.png'); + $maxPreview->method('getMimeType') + ->willReturn('image/png'); + + $previewFolder->method('getDirectoryListing') + ->willReturn([$maxPreview]); + + $preview = $this->createMock(ISimpleFile::class); + $previewFolder->method('getFile') + ->with($this->equalTo('1024-512-crop.png')) + ->willReturn($preview); + + $this->previewManager->expects($this->never()) + ->method('isMimeSupported'); + + $this->eventDispatcher->expects($this->once()) + ->method('dispatch') + ->with( + $this->equalTo(IPreview::EVENT), + $this->callback(function (GenericEvent $event) use ($file) { + return $event->getSubject() === $file && + $event->getArgument('width') === 1024 && + $event->getArgument('height') === 512 && + $event->getArgument('crop') === true && + $event->getArgument('mode') === IPreview::MODE_COVER; + }) + ); + + $result = $this->generator->getPreview($file, 1024, 512, true, IPreview::MODE_COVER, 'invalidType'); + $this->assertSame($preview, $result); } public function testNoProvider() { @@ -286,10 +353,6 @@ class GeneratorTest extends \Test\TestCase { $file->method('getId') ->willReturn(42); - $this->previewManager->method('isMimeSupported') - ->with($this->equalTo('myMimeType')) - ->willReturn(true); - $previewFolder = $this->createMock(ISimpleFolder::class); $this->appData->method('getFolder') ->with($this->equalTo(42))