diff --git a/changelog/unreleased/41834 b/changelog/unreleased/41834 new file mode 100644 index 00000000000..ff82c6f13ad --- /dev/null +++ b/changelog/unreleased/41834 @@ -0,0 +1,23 @@ +Security: Pin the Imagick coder for each preview provider + +Bitmap and SVG previews decoded content with no format hint, so ImageMagick's +own content-sniffing - independent of the mime-type check that decides whether +a preview is attempted at all - could pick a different coder than the one a +provider actually serves. PostScript-looking content, which the mime check must +allow through for the PDF and Postscript providers, could therefore still reach +the Ghostscript delegate through any other bitmap provider (SGI, Font, +Illustrator, Photoshop, TIFF, Heic). + +Each provider now pins the exact Imagick coder it expects instead of letting +ImageMagick guess from the file's content. The pin is applied in memory, so +preview generation adds no filesystem access and no temporary file. + +Because media types are derived from the file name extension, a file whose +extension does not match its actual content no longer gets a preview: a JPEG +saved as photo.tif is routed to the TIFF provider, pinned to the TIFF coder, +and falls back to a media type icon where content sniffing previously rendered +it. This affects the tif, psd, sgi, heic and ai extensions and is the intended +trade-off - content sniffing is what allowed a preview provider to be steered +to an unrelated coder in the first place. + +https://github.com/owncloud/core/pull/41834 diff --git a/lib/private/Preview/Bitmap.php b/lib/private/Preview/Bitmap.php index 30f09c161f5..a511511e37d 100644 --- a/lib/private/Preview/Bitmap.php +++ b/lib/private/Preview/Bitmap.php @@ -49,7 +49,7 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) { // Creates \Imagick object from bitmap or vector file try { - $bp = $this->getResizedPreview($stream, $maxX, $maxY); + $bp = $this->getResizedPreview($stream, $maxX, $maxY, $file->getMimeType()); } catch (\Exception $e) { Util::writeLog('core', 'ImageMagick says: ' . $e->getmessage(), Util::ERROR); return false; @@ -82,10 +82,12 @@ public function isAvailable(FileInfo $file) { * @param resource $stream the handle of the file to convert * @param int $maxX * @param int $maxY + * @param string $mimeType the file's own detected mime type, used to pin the + * Imagick coder so it cannot be redirected by the file's actual content * * @return Imagick */ - private function getResizedPreview($stream, int $maxX, int $maxY): Imagick { + private function getResizedPreview($stream, int $maxX, int $maxY, string $mimeType): Imagick { $content = \stream_get_contents($stream); if ($this->isDangerousToDecode($content)) { @@ -94,17 +96,52 @@ private function getResizedPreview($stream, int $maxX, int $maxY): Imagick { $bp = ImagickFactory::create(); - # setIteratorIndex(0) will make previews to be generated from the first page + # Pin the coder instead of letting Imagick's own content-sniffing pick one: + # reading with no format set re-derives the format from a ~130-entry magic + # table independently of isDangerousToDecode()'s check above, so content that + # looks like PostScript/PDF (which that check must allow through for the + # Postscript/PDF providers) would otherwise reach the Ghostscript delegate via + # any Bitmap provider, not just those two. + # + # Deliberately not guarded by queryFormats(): if this build does not register + # the coder, throwing here is correct - the only alternative is falling back to + # the content-sniffing this pin exists to prevent. + $bp->setFormat($this->getImagickFormat($mimeType)); $bp->readImageBlob($content); + + # setIteratorIndex(0) will make previews to be generated from the first page $bp->setIteratorIndex(0); $bp = $this->resize($bp, $maxX, $maxY); + # setFormat() above pins the wand's *output* format as well as the input coder, + # so both have to be set here. setImageFormat() alone would leave getThumbnail()'s + # (string) cast re-encoding back to the pinned input format instead of PNG. $bp->setImageFormat('png'); + $bp->setFormat('png'); return $bp; } + /** + * Maps this provider's own detected mime type(s) to the Imagick coder name that + * must decode them - the format pinned in getResizedPreview() above. + * + * $mimeType comes from $file->getMimeType(), deliberately not from the type that + * selected this provider (OC\Preview::$mimeType). Those two can differ, because + * callers may override the selection type via getThumbnail(['mimeType' => ...]) - + * apps/files_trashbin/ajax/preview.php does, and apps/dav passes the request's query + * parameters straight through. The file's own type cannot be steered by a request, + * which is the property the pin depends on. + * + * The consequence is that an implementation must cope with a mime type it does not + * serve: a trashed file reports application/octet-stream, because the .d + * suffix defeats extension-based detection. Returning a constant handles that + * correctly. Do NOT "fix" the divergence by rejecting a $mimeType that fails this + * provider's own getMimeType() regex - that rejects every trashbin preview. + */ + abstract protected function getImagickFormat(string $mimeType): string; + /** * Bitmap providers must never hand text-based content (SVG, XML, or any other * text/* type, e.g. a raw MVG script) to Imagick::readImageBlob() - ImageMagick's diff --git a/lib/private/Preview/Font.php b/lib/private/Preview/Font.php index 775147d83ea..94c77880094 100644 --- a/lib/private/Preview/Font.php +++ b/lib/private/Preview/Font.php @@ -29,4 +29,21 @@ class Font extends Bitmap { public function getMimeType() { return '/application\/(?:font-sfnt|x-font$)/'; } + + protected function getImagickFormat(string $mimeType): string { + if ($mimeType === 'application/x-font') { + return 'PFB'; + } + # .otf and .ttf are indistinguishable by mime type alone (both are + # application/font-sfnt); TTF is what actually decodes real font files here, + # both tagged variants included. + # + # This is the only provider whose coder depends on $mimeType, so it is also the + # only one where the divergence documented on Bitmap::getImagickFormat() is + # observable: a .pfb whose stored mime type is not application/x-font - a trashed + # one reports application/octet-stream - lands here rather than in the branch + # above and gets no preview. Deciding from the content instead would mean + # re-deriving the format from magic bytes, which is what the pin exists to avoid. + return 'TTF'; + } } diff --git a/lib/private/Preview/Heic.php b/lib/private/Preview/Heic.php index 6e7bba4125f..040c7665ae6 100644 --- a/lib/private/Preview/Heic.php +++ b/lib/private/Preview/Heic.php @@ -29,4 +29,11 @@ class Heic extends Bitmap { public function getMimeType() { return '/image\/hei(f|c)/'; } + + protected function getImagickFormat(string $mimeType): string { + # image/heic and image/heif are the same container handled by the same coder + # module, and not every ImageMagick build registers a distinct HEIF coder - so + # both mime types pin HEIC rather than risk pinning a format that is absent. + return 'HEIC'; + } } diff --git a/lib/private/Preview/Illustrator.php b/lib/private/Preview/Illustrator.php index 06a98d9e4c5..0267b4712d1 100644 --- a/lib/private/Preview/Illustrator.php +++ b/lib/private/Preview/Illustrator.php @@ -30,4 +30,8 @@ class Illustrator extends Bitmap { public function getMimeType() { return '/application\/illustrator/'; } + + protected function getImagickFormat(string $mimeType): string { + return 'AI'; + } } diff --git a/lib/private/Preview/Office.php b/lib/private/Preview/Office.php index d8f038279b2..759b74fbf49 100644 --- a/lib/private/Preview/Office.php +++ b/lib/private/Preview/Office.php @@ -68,7 +68,12 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) { $pdfPreview = $tmpDir . '/' . $pathInfo['filename'] . '.pdf'; # Note: no SVG sanitization of the file content required .... - $imagick = ImagickFactory::create($pdfPreview . '[0]'); + # Pin the coder: this is LibreOffice's own PDF output, but content-sniffing + # is avoided everywhere else Imagick decodes a file in this codebase, so pin + # it here too rather than rely on the ".pdf" path extension. Unlike + # setFormat(), a "FORMAT:path" constructor argument pins only the input + # coder, so setImageFormat('jpg') below is still all the output needs. + $imagick = ImagickFactory::create('PDF:' . $pdfPreview . '[0]'); $imagick->setImageFormat('jpg'); } catch (\Exception $e) { @\unlink($pdfPreview); diff --git a/lib/private/Preview/PDF.php b/lib/private/Preview/PDF.php index 0ab92bfb9cb..40ae6397091 100644 --- a/lib/private/Preview/PDF.php +++ b/lib/private/Preview/PDF.php @@ -30,4 +30,8 @@ class PDF extends Bitmap { public function getMimeType() { return '/application\/pdf/'; } + + protected function getImagickFormat(string $mimeType): string { + return 'PDF'; + } } diff --git a/lib/private/Preview/Photoshop.php b/lib/private/Preview/Photoshop.php index ca15dc1a12b..7f7add29fa9 100644 --- a/lib/private/Preview/Photoshop.php +++ b/lib/private/Preview/Photoshop.php @@ -30,4 +30,8 @@ class Photoshop extends Bitmap { public function getMimeType() { return '/application\/x-photoshop/'; } + + protected function getImagickFormat(string $mimeType): string { + return 'PSD'; + } } diff --git a/lib/private/Preview/Postscript.php b/lib/private/Preview/Postscript.php index bab73b80849..e852b66ba16 100644 --- a/lib/private/Preview/Postscript.php +++ b/lib/private/Preview/Postscript.php @@ -30,4 +30,10 @@ class Postscript extends Bitmap { public function getMimeType() { return '/application\/postscript/'; } + + protected function getImagickFormat(string $mimeType): string { + # EPS is the coder ImageMagick registers for application/postscript; it shares + # ReadPSImage() with the plain PS coder, so it covers .ps as well as .eps. + return 'EPS'; + } } diff --git a/lib/private/Preview/SGI.php b/lib/private/Preview/SGI.php index 81f43410fe3..c9c24d4ab52 100644 --- a/lib/private/Preview/SGI.php +++ b/lib/private/Preview/SGI.php @@ -27,4 +27,8 @@ class SGI extends Bitmap { public function getMimeType() { return '/image\/sgi/'; } + + protected function getImagickFormat(string $mimeType): string { + return 'SGI'; + } } diff --git a/lib/private/Preview/SVG.php b/lib/private/Preview/SVG.php index 50347f4de65..49e80f5de23 100644 --- a/lib/private/Preview/SVG.php +++ b/lib/private/Preview/SVG.php @@ -58,8 +58,21 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) { return false; } + # Pin the coder so Imagick's own content-sniffing cannot pick a different one + # than the svg:sanitize/embed/decode options set by ImagickFactory assume. + # Guarded, unlike Bitmap.php: a build that registers no SVG coder cannot be + # pinned to it and cannot decode SVG at all either way, and $output here is + # already DOMSanitizer's serialized output rather than the raw file bytes. + if (\count(\Imagick::queryFormats('SVG')) > 0) { + $imagick->setFormat('SVG'); + } $imagick->readImageBlob($output); + + # setFormat() above pins the wand's *output* format as well as the input + # coder, so both have to be set - setImageFormat() alone would leave + # getImageBlob() below re-encoding back to SVG instead of PNG. $imagick->setImageFormat('png32'); + $imagick->setFormat('png32'); } catch (\Exception $e) { \OCP\Util::writeLog('core', $e->getmessage(), \OCP\Util::ERROR); return false; diff --git a/lib/private/Preview/TIFF.php b/lib/private/Preview/TIFF.php index 25404d6e6a0..9f7ff4bfedc 100644 --- a/lib/private/Preview/TIFF.php +++ b/lib/private/Preview/TIFF.php @@ -30,4 +30,8 @@ class TIFF extends Bitmap { public function getMimeType() { return '/image\/tiff/'; } + + protected function getImagickFormat(string $mimeType): string { + return 'TIFF'; + } } diff --git a/tests/data/testimage.heic b/tests/data/testimage.heic new file mode 100644 index 00000000000..6b0bbd258cd Binary files /dev/null and b/tests/data/testimage.heic differ diff --git a/tests/data/testimage.psd b/tests/data/testimage.psd new file mode 100644 index 00000000000..16bee932e8d Binary files /dev/null and b/tests/data/testimage.psd differ diff --git a/tests/data/testimage.sgi b/tests/data/testimage.sgi new file mode 100644 index 00000000000..28f500ef08b Binary files /dev/null and b/tests/data/testimage.sgi differ diff --git a/tests/data/testimage.tiff b/tests/data/testimage.tiff new file mode 100644 index 00000000000..9db137a2f5a Binary files /dev/null and b/tests/data/testimage.tiff differ diff --git a/tests/lib/Preview/BitmapTest.php b/tests/lib/Preview/BitmapTest.php index 2c808cffb78..8d26199b62e 100644 --- a/tests/lib/Preview/BitmapTest.php +++ b/tests/lib/Preview/BitmapTest.php @@ -30,6 +30,12 @@ */ class BitmapTest extends Provider { public function setUp(): void { + # Postscript::getImagickFormat() pins EPS, so on a build without that coder this + # provider cannot decode the fixture at all. Unguarded, that is a failure rather + # than a skip - previously ImageMagick's own sniffing hid the dependency. + if (\count(\Imagick::queryFormats('EPS')) === 0) { + $this->markTestSkipped('This ImageMagick build registers no EPS coder'); + } parent::setUp(); $fileName = 'testimage.eps'; diff --git a/tests/lib/Preview/CoderPinningTest.php b/tests/lib/Preview/CoderPinningTest.php new file mode 100644 index 00000000000..2ed0d40a7f5 --- /dev/null +++ b/tests/lib/Preview/CoderPinningTest.php @@ -0,0 +1,255 @@ + + * + * @copyright Copyright (c) 2026, ownCloud GmbH + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace Test\Preview; + +use Generator; +use OC\Image\ImagickFactory; +use OC\Preview\Bitmap; +use OC\Preview\Font; +use OC\Preview\Heic; +use OC\Preview\Illustrator; +use OC\Preview\PDF; +use OC\Preview\Photoshop; +use OC\Preview\Postscript; +use OC\Preview\SGI; +use OC\Preview\TIFF; +use OCP\Files\File; +use Test\TestCase; + +/** + * @requires extension imagick + */ +class CoderPinningTest extends TestCase { + /** + * The payload both negative tests feed to a provider it is foreign to. Deliberately + * minimal and harmless: what is under test is which coder ImageMagick hands it to, + * not what Ghostscript would draw from it. + */ + private const FOREIGN_POSTSCRIPT = "%!PS-Adobe-3.0\n%%BoundingBox: 0 0 10 10\nshowpage\n"; + + private function makeFile(string $content, string $mimeType): File { + $stream = \fopen('php://memory', 'rb+'); + \fwrite($stream, $content); + \rewind($stream); + $file = $this->createMock(File::class); + $file->method('fopen')->willReturn($stream); + $file->method('getMimeType')->willReturn($mimeType); + return $file; + } + + /** + * Skip only when the one coder under test is absent from this ImageMagick build, + * rather than probing for some unrelated coder as a proxy for "extended build". + */ + private function requireCoder(string $coder): void { + if (\count(\Imagick::queryFormats($coder)) === 0) { + $this->markTestSkipped("This ImageMagick build registers no $coder coder"); + } + } + + /** + * A registered coder does not mean the delegate behind it can decode these particular + * bytes: coders/heic.c registers HEIC, HEIF and AVIF whenever libheif is present, but + * decoding the AVIF fixture additionally needs an AV1 decoder inside libheif. Read the + * fixture unpinned first - if this build cannot decode it at all, the pinned read + * failing below would say nothing about the pin, so skip rather than report a failure + * against the build. + * + * The unpinned read is content-sniffed, which is exactly what the pin exists to + * prevent. That is fine here: it is only ever a capability probe, never an assertion. + */ + private function requireDecodableFixture(string $coder, string $content): void { + $this->requireCoder($coder); + try { + $probe = ImagickFactory::create(); + $probe->readImageBlob($content); + $probe->clear(); + } catch (\ImagickException $e) { + $this->markTestSkipped("This ImageMagick build cannot decode the $coder fixture: " . $e->getMessage()); + } + } + + /** + * isDangerousToDecode() is a deny-list over the *sniffed* type, and it denies text/*. + * On a build whose libmagic reported the payload as text/plain rather than + * application/postscript, the provider would reject it at that gate and the negative + * assertions below would hold without the coder pin ever running. Assert the detected + * type, so such a build fails loudly with an actionable message instead of passing + * vacuously. + */ + private function assertPayloadReachesTheCoderPin(string $content): void { + $detected = \OC::$server->getMimeTypeDetector()->detectString($content); + $this->assertStringStartsWith( + 'application/postscript', + $detected, + 'payload must survive isDangerousToDecode(), which denies text/* - libmagic here says: ' . $detected + ); + } + + /** + * @dataProvider providesLegitimateContent + */ + public function testDecodesItsOwnFormat(string $fixture, string $mimeType, Bitmap $provider, string $coder): void { + $content = \file_get_contents(\OC::$SERVERROOT . '/' . $fixture); + $this->requireDecodableFixture($coder, $content); + $file = $this->makeFile($content, $mimeType); + + $result = $provider->getThumbnail($file, 32, 32, false); + + $this->assertNotFalse($result, "$fixture via " . \get_class($provider) . ' should have decoded'); + } + + public function providesLegitimateContent(): Generator { + yield 'PDF' => ['tests/data/testimage.pdf', 'application/pdf', new PDF(), 'PDF']; + yield 'Postscript (EPS)' => ['tests/data/testimage.eps', 'application/postscript', new Postscript(), 'EPS']; + # Modern .ai files really are PDF containers, and ImageMagick's AI coder is a + # Ghostscript alias for the PDF one - so testimage.pdf is a faithful fixture for + # this case, and a separate .ai file would be a byte-identical copy of it. + yield 'Illustrator (AI)' => ['tests/data/testimage.pdf', 'application/illustrator', new Illustrator(), 'AI']; + yield 'Photoshop (PSD)' => ['tests/data/testimage.psd', 'application/x-photoshop', new Photoshop(), 'PSD']; + yield 'SGI' => ['tests/data/testimage.sgi', 'image/sgi', new SGI(), 'SGI']; + yield 'TIFF' => ['tests/data/testimage.tiff', 'image/tiff', new TIFF(), 'TIFF']; + # Reuses the in-tree OpenSans rather than adding a font fixture of its own. No + # genuine OTF ('OTTO'-tagged) case here: this environment's ImageMagick/FreeType + # delegate cannot decode CFF-outline OpenType fonts at all, pinned or not - + # confirmed against four real system .otf files. TTF-tagged content, which the TTF + # coder decodes fine, is what's actually exercised in practice for the font-sfnt + # mime type. + yield 'Font (font-sfnt, ttf bytes)' => ['core/fonts/OpenSans-Regular.ttf', 'application/font-sfnt', new Font(), 'TTF']; + # The HEIC fixture is AVIF-branded on purpose: coders/heic.c registers HEIC, HEIF + # and AVIF as three separate coders, so an AVIF-branded file served by the Heic + # provider is the case worth a real sample - and an HEVC-encoded one would need a + # libde265 delegate that is not present everywhere. + # + # Both mime types pin HEIC, so neither case needs a distinct HEIF coder to be + # registered - which is the point: pinning HEIF would break image/heif previews + # on every build that only registers HEIC. + yield 'Heic (image/heic)' => ['tests/data/testimage.heic', 'image/heic', new Heic(), 'HEIC']; + yield 'Heic (image/heif)' => ['tests/data/testimage.heic', 'image/heif', new Heic(), 'HEIC']; + } + + /** + * PostScript content is sniffed by libmagic as application/postscript, which + * isDangerousToDecode() must not reject since Postscript/PDF legitimately decode + * it - so the mime-type gate alone lets it through here too. Pinning the expected + * coder is what stops ImageMagick's own content-sniffing from handing it to the + * Ghostscript delegate through a provider that has nothing to do with PostScript. + * + * PDF/Postscript/Illustrator are deliberately not in this set: they are the + * Ghostscript-backed providers PostScript-ish content is NOT foreign to, so + * feeding it to them tests Ghostscript's own leniency, not cross-coder confusion. + * Font is also excluded: see testFontNeverInvokesADangerousCoderForForeignContent(). + * + * @dataProvider providesForeignProviders + */ + public function testRejectsPostScriptContentFromAForeignProvider(Bitmap $provider, string $mimeType, string $coder): void { + $this->requireCoder($coder); + $this->assertPayloadReachesTheCoderPin(self::FOREIGN_POSTSCRIPT); + $file = $this->makeFile(self::FOREIGN_POSTSCRIPT, $mimeType); + + $result = $provider->getThumbnail($file, 32, 32, false); + + $this->assertFalse($result); + } + + public function providesForeignProviders(): Generator { + yield 'SGI' => [new SGI(), 'image/sgi', 'SGI']; + yield 'Photoshop' => [new Photoshop(), 'application/x-photoshop', 'PSD']; + yield 'TIFF' => [new TIFF(), 'image/tiff', 'TIFF']; + yield 'Heic' => [new Heic(), 'image/heic', 'HEIC']; + } + + public function testFontNeverInvokesADangerousCoderForForeignContent(): void { + $this->requireCoder('TTF'); + $this->assertPayloadReachesTheCoderPin(self::FOREIGN_POSTSCRIPT); + $file = $this->makeFile(self::FOREIGN_POSTSCRIPT, 'application/font-sfnt'); + + $result = (new Font())->getThumbnail($file, 32, 32, false); + + # FreeType fails on non-font bytes either by refusing them outright or by producing + # a blank placeholder, never by invoking Ghostscript or a script coder - both are + # safe outcomes. What must never happen is a large image carrying rendered + # PostScript content. Assert that as one branch-free expression: branching would + # leave the test assertion-less on builds that return false, and failOnRisky in + # tests/phpunit-autotest.xml turns a zero-assertion test into a hard failure. + $renderedBytes = $result === false ? 0 : \strlen((string)$result->data()); + $this->assertLessThan(2048, $renderedBytes, 'Font must not render PostScript content'); + } + + /** + * The pin is derived from the file's own mime type, not from the one that selected + * the provider - callers can override the latter via getThumbnail(['mimeType' => ...]), + * and apps/files_trashbin/ajax/preview.php does exactly that, because a trashed + * file's .d suffix defeats extension-based detection and leaves it + * reporting application/octet-stream. + * + * So a provider must still decode when handed a mime type it does not serve. Guard + * that: making the provider reject a mime type failing its own getMimeType() regex + * looks like a tightening, but it would silently kill every trashbin bitmap preview. + * + * @dataProvider providesForeignMimeTypeButOwnContent + */ + public function testDecodesWhenTheStoredMimeTypeIsNotTheProvidersOwn( + string $fixture, + Bitmap $provider, + string $coder + ): void { + $content = \file_get_contents(\OC::$SERVERROOT . '/' . $fixture); + $this->requireDecodableFixture($coder, $content); + # what a trashed "photo.tif.d1700000000" actually reports + $file = $this->makeFile($content, 'application/octet-stream'); + + $this->assertSame( + 0, + \preg_match($provider->getMimeType(), 'application/octet-stream'), + 'precondition: this mime type must NOT match the provider regex, or the case proves nothing' + ); + $this->assertNotFalse( + $provider->getThumbnail($file, 32, 32, false), + 'a provider pinning a constant coder must still decode its own content' + ); + } + + public function providesForeignMimeTypeButOwnContent(): Generator { + yield 'TIFF' => ['tests/data/testimage.tiff', new TIFF(), 'TIFF']; + yield 'Photoshop' => ['tests/data/testimage.psd', new Photoshop(), 'PSD']; + yield 'SGI' => ['tests/data/testimage.sgi', new SGI(), 'SGI']; + } + + /** + * setFormat() pins the wand's *output* format as well as the input coder, so a + * provider that reset only the image format would hand back the input format + * re-encoded instead of a PNG. Guard that explicitly: for TIFF the re-encode is + * byte-identical to the input, which makes the mistake easy to reintroduce and + * hard to spot. + */ + public function testPinnedDecodeReturnsPngAndNotThePinnedInputFormat(): void { + $content = \file_get_contents(\OC::$SERVERROOT . '/tests/data/testimage.tiff'); + $this->requireDecodableFixture('TIFF', $content); + $file = $this->makeFile($content, 'image/tiff'); + + $result = (new TIFF())->getThumbnail($file, 32, 32, false); + + $this->assertNotFalse($result); + $this->assertSame('image/png', $result->mimeType()); + } +} diff --git a/tests/lib/Preview/PDFTest.php b/tests/lib/Preview/PDFTest.php index 4c7d700b7d8..ee0c8484204 100644 --- a/tests/lib/Preview/PDFTest.php +++ b/tests/lib/Preview/PDFTest.php @@ -37,16 +37,19 @@ class PDFTest extends Provider { * @throws NotFoundException */ public function setUp(): void { - if (\count(\Imagick::queryFormats('SVG')) === 1) { - parent::setUp(); - - $fileName = 'testimage.pdf'; - $this->imgPath = $this->prepareTestFile($fileName, \OC::$SERVERROOT . '/tests/data/' . $fileName); - $this->width = 595; - $this->height = 842; - $this->provider = new PDF(); - } else { - $this->markTestSkipped('No PDF provider present'); + # PDF is the coder PDF::getImagickFormat() pins. This used to gate on the SVG + # coder, which this provider never touches - so on any build registering no SVG + # coder (owncloudci/php:8.3 among them) every case here skipped, reporting "No + # PDF provider present" while the PDF coder was in fact present. + if (\count(\Imagick::queryFormats('PDF')) === 0) { + $this->markTestSkipped('This ImageMagick build registers no PDF coder'); } + parent::setUp(); + + $fileName = 'testimage.pdf'; + $this->imgPath = $this->prepareTestFile($fileName, \OC::$SERVERROOT . '/tests/data/' . $fileName); + $this->width = 595; + $this->height = 842; + $this->provider = new PDF(); } } diff --git a/tests/lib/Preview/SVGTest.php b/tests/lib/Preview/SVGTest.php index dd4a2af53af..5926e0a6811 100644 --- a/tests/lib/Preview/SVGTest.php +++ b/tests/lib/Preview/SVGTest.php @@ -30,16 +30,17 @@ */ class SVGTest extends Provider { public function setUp(): void { - if (\count(\Imagick::queryFormats('SVG')) === 1) { - parent::setUp(); - - $fileName = 'testimagelarge.svg'; - $this->imgPath = $this->prepareTestFile($fileName, \OC::$SERVERROOT . '/tests/data/' . $fileName); - $this->width = 3000; - $this->height = 2000; - $this->provider = new \OC\Preview\SVG; - } else { - $this->markTestSkipped('No SVG provider present'); + # === 0 rather than === 1: a build may register SVG alongside SVGZ/MSVG, which + # would have skipped these cases while the SVG coder was present all along + if (\count(\Imagick::queryFormats('SVG')) === 0) { + $this->markTestSkipped('This ImageMagick build registers no SVG coder'); } + parent::setUp(); + + $fileName = 'testimagelarge.svg'; + $this->imgPath = $this->prepareTestFile($fileName, \OC::$SERVERROOT . '/tests/data/' . $fileName); + $this->width = 3000; + $this->height = 2000; + $this->provider = new \OC\Preview\SVG; } } diff --git a/tests/lib/Preview/SanitizeTest.php b/tests/lib/Preview/SanitizeTest.php index 882a1230866..5defde72527 100644 --- a/tests/lib/Preview/SanitizeTest.php +++ b/tests/lib/Preview/SanitizeTest.php @@ -28,14 +28,18 @@ use OCP\Files\File; use Test\TestCase; +/** + * @requires extension imagick + */ class SanitizeTest extends TestCase { /** * @dataProvider providesSVG */ - public function test(string $svgContent, Bitmap $provider): void { - if (\count(\Imagick::queryFormats('SVG')) === 0) { - $this->markTestSkipped('No SVG provider present'); - } + public function test(string $svgContent, Bitmap $provider, string $mimeType): void { + # no coder guard on purpose: isDangerousToDecode() rejects this content before + # ImagickFactory::create() and before setFormat(), so these cases never reach a + # coder at all. Requiring one would only let a reduced build skip the OC10-164 + # regression assertions silently. # mock it all .... $stream = fopen('php://memory', 'rb+'); fwrite($stream, $svgContent); @@ -43,6 +47,7 @@ public function test(string $svgContent, Bitmap $provider): void { $file = $this->createMock(File::class); $file->method('getContent')->willReturn($svgContent); $file->method('fopen')->willReturn($stream); + $file->method('getMimeType')->willReturn($mimeType); # create the preview - SVG/text/script-shaped content must never reach Imagick via a Bitmap provider $return = $provider->getThumbnail($file, 32, 32, false); @@ -78,13 +83,13 @@ public function providesSVG(): Generator { SVG; # all Bitmap based providers use the same thumbnailing logic - two is enough .... - yield 'PDF provider - image tag' => [$svgContent0, new PDF()]; - yield 'Font Provider - image tag' => [$svgContent0, new Font()]; - yield 'PDF provider - malformed SVG with MSL href' => [$malformedSvgWithMslHref, new PDF()]; - yield 'Font Provider - malformed SVG with MSL href' => [$malformedSvgWithMslHref, new Font()]; - yield 'PDF provider - raw MVG' => [$rawMvg, new PDF()]; - yield 'Font Provider - raw MVG' => [$rawMvg, new Font()]; - yield 'PDF provider - well-formed SVG' => [$wellFormedSvg, new PDF()]; - yield 'Font Provider - well-formed SVG' => [$wellFormedSvg, new Font()]; + yield 'PDF provider - image tag' => [$svgContent0, new PDF(), 'application/pdf']; + yield 'Font Provider - image tag' => [$svgContent0, new Font(), 'application/font-sfnt']; + yield 'PDF provider - malformed SVG with MSL href' => [$malformedSvgWithMslHref, new PDF(), 'application/pdf']; + yield 'Font Provider - malformed SVG with MSL href' => [$malformedSvgWithMslHref, new Font(), 'application/font-sfnt']; + yield 'PDF provider - raw MVG' => [$rawMvg, new PDF(), 'application/pdf']; + yield 'Font Provider - raw MVG' => [$rawMvg, new Font(), 'application/font-sfnt']; + yield 'PDF provider - well-formed SVG' => [$wellFormedSvg, new PDF(), 'application/pdf']; + yield 'Font Provider - well-formed SVG' => [$wellFormedSvg, new Font(), 'application/font-sfnt']; } }