From 70f1e96e19abbff09680677bafddbb14505aada8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:40:07 +0200 Subject: [PATCH 01/18] fix: prevent arbitrary file write via unsanitized SVG/MVG bitmap previews (OC10-164) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bitmap::getResizedPreview() sanitized SVG content before handing it to Imagick::readImageBlob(), but fell back to the ORIGINAL, unsanitized bytes whenever the sanitizer returned an empty string - which it does for any content libxml cannot parse, not just for genuinely malformed SVG. A malformed SVG (or any non-XML payload such as a raw MVG script) therefore reached ImageMagick unsanitized, where an or an MVG "fill 'url(...)'" primitive can execute an MSL script that reads and writes arbitrary files as the web user (CVSS 8.8). Bitmap providers (PDF, Font, Postscript, ...) only ever need to decode real bitmap/vector image formats, never SVG or script-shaped text content - that belongs exclusively to the dedicated SVG provider. getResizedPreview() now rejects any content whose libmagic-detected media type is text/*, image/svg+xml, application/xml, or image/x-mvg before ever calling into Imagick, instead of trying to sanitize and falling back on failure. It also now goes through ImagickFactory::create() so the svg:sanitize/svg:embed/ svg:decode hardening options apply here as they already did in the SVG provider. SVG::sanitizeSVGContent() return type changes from string to ?string so it can report "could not sanitize" (null) separately from "sanitized to an empty document" (''); its own provider now bails out on null instead of silently passing empty content to Imagick. The removal of the sanitize-with-fallback path in Bitmap changes the behaviour asserted by SanitizeTest: SVG content fed to a Bitmap provider (PDF, Font) now yields false instead of a rendered PNG, since Bitmap providers no longer attempt to handle SVG-shaped content at all. Added regression cases for a malformed SVG with an MSL xlink:href, a raw MVG script, and a well-formed SVG - all must return false from a Bitmap provider. Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- lib/private/Preview/Bitmap.php | 29 +++++++++++++++++++------- lib/private/Preview/SVG.php | 9 +++++++- tests/lib/Preview/SanitizeTest.php | 33 ++++++++++++++++++++++++++---- 3 files changed, 59 insertions(+), 12 deletions(-) diff --git a/lib/private/Preview/Bitmap.php b/lib/private/Preview/Bitmap.php index 33d5c24488dd..bfdd86f340f2 100644 --- a/lib/private/Preview/Bitmap.php +++ b/lib/private/Preview/Bitmap.php @@ -25,6 +25,7 @@ namespace OC\Preview; use Imagick; +use OC\Image\ImagickFactory; use OC\Preview; use OCP\Files\File; use OCP\Files\FileInfo; @@ -96,18 +97,16 @@ public function isAvailable(FileInfo $file) { * @return Imagick */ private function getResizedPreview($stream, int $maxX, int $maxY): Imagick { - # file content can be SVG - we need to sanitize it first $content = \stream_get_contents($stream); - $output = SVG::sanitizeSVGContent($content); - # in case the content is not an SVG we use the original content - if ($output === '') { - $output = $content; + + if ($this->isDangerousToDecode($content)) { + throw new \RuntimeException('Refusing to decode text-based content for a bitmap preview'); } - $bp = new Imagick(); + $bp = ImagickFactory::create(); # setIteratorIndex(0) will make previews to be generated from the first page - $bp->readImageBlob($output); + $bp->readImageBlob($content); $bp->setIteratorIndex(0); $bp = $this->resize($bp, $maxX, $maxY); @@ -117,6 +116,22 @@ private function getResizedPreview($stream, int $maxX, int $maxY): Imagick { return $bp; } + /** + * 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 + * text/vector coders can be abused to read and write arbitrary files. + */ + private function isDangerousToDecode(string $content): bool { + $mimeType = \OC::$server->getMimeTypeDetector()->detectString($content); + $mimeType = \strtolower(\trim(\explode(';', $mimeType, 2)[0])); + + if (\strpos($mimeType, 'text/') === 0) { + return true; + } + + return \in_array($mimeType, ['image/svg+xml', 'application/xml', 'image/x-mvg'], true); + } + /** * Returns a resized \Imagick object * diff --git a/lib/private/Preview/SVG.php b/lib/private/Preview/SVG.php index 8153f0dc9438..960c7368d3db 100644 --- a/lib/private/Preview/SVG.php +++ b/lib/private/Preview/SVG.php @@ -71,6 +71,9 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) { # sanitize SVG content $output = self::sanitizeSVGContent($content); + if ($output === null) { + return false; + } $imagick->readImageBlob($output); $imagick->setImageFormat('png32'); @@ -98,7 +101,7 @@ public function isAvailable(FileInfo $file) { return true; } - public static function sanitizeSVGContent(string $content): string { + public static function sanitizeSVGContent(string $content): ?string { $sanitizer = new DOMSanitizer(DOMSanitizer::SVG); $sanitizer->addDisallowedTags(['image']); $sanitizer->addDisallowedAttributes(['xlink:href']); @@ -107,6 +110,10 @@ public static function sanitizeSVGContent(string $content): string { // XML errors are expected here if the SVG is malformed \libxml_clear_errors(); + if (!\is_string($sanitized_content)) { + return null; + } + return $sanitized_content; } } diff --git a/tests/lib/Preview/SanitizeTest.php b/tests/lib/Preview/SanitizeTest.php index bcac9efb3500..882a1230866c 100644 --- a/tests/lib/Preview/SanitizeTest.php +++ b/tests/lib/Preview/SanitizeTest.php @@ -44,10 +44,10 @@ public function test(string $svgContent, Bitmap $provider): void { $file->method('getContent')->willReturn($svgContent); $file->method('fopen')->willReturn($stream); - # create the preview + # create the preview - SVG/text/script-shaped content must never reach Imagick via a Bitmap provider $return = $provider->getThumbnail($file, 32, 32, false); - $this->assertImage(__DIR__ . '/white-32x32.png', $return); + $this->assertFalse($return); } public function providesSVG(): Generator { @@ -56,10 +56,35 @@ public function providesSVG(): Generator { +SVG; + + # malformed SVG (unclosed ) - the DOM sanitizer cannot parse this and + # used to fall back to the raw, unsanitized content + $malformedSvgWithMslHref = << + + +SVG; + + $rawMvg = << SVG; # all Bitmap based providers use the same thumbnailing logic - two is enough .... - yield 'PDF provider' => [$svgContent0, new PDF()]; - yield 'Font Provider' => [$svgContent0, new Font()]; + 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()]; } } From abbe1a11f528035c1564ee64f0bc5884382bea39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:41:56 +0200 Subject: [PATCH 02/18] docs: add changelog entry for OC10-164 bitmap preview fix (#41827) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- changelog/unreleased/41827 | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 changelog/unreleased/41827 diff --git a/changelog/unreleased/41827 b/changelog/unreleased/41827 new file mode 100644 index 000000000000..ed593a0087d3 --- /dev/null +++ b/changelog/unreleased/41827 @@ -0,0 +1,15 @@ +Security: Reject SVG/script content before it reaches ImageMagick bitmap previews + +Bitmap previews (PDF, Font, ...) sanitized SVG content before decoding it, but +fell back to the original, unsanitized bytes whenever the sanitizer could not +parse the input - which happened for any malformed SVG or non-XML payload, +not only for genuinely broken SVG files. A crafted malformed SVG or a raw MVG +script could therefore reach ImageMagick unsanitized and trigger an MSL +script that reads or writes arbitrary files as the web server user. + +Bitmap previews no longer attempt to sanitize and fall back; they now reject +any content that is detected as text, XML, SVG, or MVG before ImageMagick +ever sees it, and decode through the same hardened Imagick options already +used by the dedicated SVG preview provider. + +https://github.com/owncloud/core/pull/41827 From 367e8890de37784e2b283d7eb79cf01f045cd0de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Fri, 11 Sep 2026 12:01:21 +0200 Subject: [PATCH 03/18] fix: match "image/svg" without the +xml suffix in the OC10-164 mime gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Confirmed while testing the 10.16 backport: PHP 7.4's bundled fileinfo extension reports the same SVG content as "image/svg", not "image/svg+xml" - the exact-match check silently let it through on that runtime while still catching it on PHP 8.3. Match by prefix instead so the gate added in af3c147ef5 ("fix: prevent arbitrary file write via unsanitized SVG/MVG bitmap previews (OC10-164)") is not dependent on which libmagic build a given PHP runtime happens to link. Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- lib/private/Preview/Bitmap.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/private/Preview/Bitmap.php b/lib/private/Preview/Bitmap.php index bfdd86f340f2..fd2b92d89b62 100644 --- a/lib/private/Preview/Bitmap.php +++ b/lib/private/Preview/Bitmap.php @@ -125,11 +125,12 @@ private function isDangerousToDecode(string $content): bool { $mimeType = \OC::$server->getMimeTypeDetector()->detectString($content); $mimeType = \strtolower(\trim(\explode(';', $mimeType, 2)[0])); - if (\strpos($mimeType, 'text/') === 0) { + // libmagic reports "image/svg" without the "+xml" suffix on some PHP/OS builds + if (\strpos($mimeType, 'text/') === 0 || \strpos($mimeType, 'image/svg') === 0) { return true; } - return \in_array($mimeType, ['image/svg+xml', 'application/xml', 'image/x-mvg'], true); + return \in_array($mimeType, ['application/xml', 'image/x-mvg'], true); } /** From dc4ebd3e8699ab45b18910bbf8191bac40892465 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Wed, 16 Sep 2026 13:47:34 +0200 Subject: [PATCH 04/18] fix: pin the Imagick coder per provider without a temporary file (OC10-164) (#41834) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: pin the Imagick coder per bitmap preview provider (OC10-164) isDangerousToDecode() (af3c147ef5) is a deny-list over the libmagic-sniffed type, but the decode that follows re-derives the format independently: readImageBlob() with no format set consults Imagick's own ~130-entry magic table, so the coder actually invoked can differ from what the mime check reasoned about. application/postscript and application/pdf are deliberately not denied - Postscript and PDF legitimately decode them - which means PostScript-looking bytes still pass the gate through every other Bitmap provider (SGI, Font, Illustrator, Photoshop, TIFF, Heic), and Imagick's own sniffing then hands them to the Ghostscript delegate anyway. Pin the coder each provider actually expects instead of leaving Imagick to guess: getImagickFormat() maps a provider's own detected mime type(s) to an explicit Imagick format name, and getResizedPreview() installs it with setFormat() before readImageBlob(), so no temporary file is involved and the content never leaves memory. setFormat() pins the wand's output format as well as the input coder, so both setImageFormat('png') and setFormat('png') are needed afterwards - otherwise getThumbnail()'s (string) cast re-encodes back to the pinned input format and hands back the original bytes. That one missing call is what previously made setFormat() look as though it skipped rasterization altogether. It does decode: verified against unpinned geometry for tiff/psd/sgi/ai/heic/ttf on both ImageMagick 6.9.11-60 with imagick 3.8.1 and ImageMagick 7.1.1-36 with imagick 3.7.0. The pin is deliberately not guarded by queryFormats(): if a build does not register the coder a provider needs, throwing is correct, because the only alternative is falling back to the content-sniffing this pin exists to prevent. Heic pins HEIC for both image/heic and image/heif, as they are one container handled by one coder module and not every build registers a distinct HEIF coder. Office.php pins through its constructor argument instead. A "FORMAT:path" prefix there pins only the input coder and leaves the output format alone, so its setImageFormat('jpg') needs no counterpart. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: pin the Imagick coder for SVG previews too (OC10-164) SVG::getThumbnail() is the one Imagick read path in core that is not a Bitmap provider, and it had the same gap: ImagickFactory sets svg:sanitize, svg:embed and svg:decode, but the read that follows let Imagick pick the coder from the content, so those options could be reasoning about a different coder than the one that ran. Pin SVG explicitly, and reset both the image and wand output formats to png32 afterwards for the same reason as Bitmap.php - setFormat() pins the output format as well, so setImageFormat() alone would leave getImageBlob() re-encoding back to SVG. Unlike Bitmap.php the pin is guarded by queryFormats(). A build that registers no SVG coder cannot be pinned to it and cannot decode SVG at all either way, so throwing would trade a clear "no decode delegate" failure for a confusing "Unable to set format" one; owncloudci/php:8.3 is such a build. The value at risk is also lower here: what gets pinned is DOMSanitizer's serialized output, not the raw file bytes. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: cover Imagick coder pinning with real per-format fixtures (OC10-164) Adds CoderPinningTest, which asserts both halves of the pin: every provider still decodes its own format, and PostScript content is rejected by the providers it is foreign to (SGI, Photoshop, TIFF, Heic) rather than being handed to the Ghostscript delegate by ImageMagick's own content-sniffing. Six fixtures had to be added - tests/data had no .ai/.heic/.psd/.sgi/.tiff/ .ttf sample at all, so there was nothing to decode per provider. The HEIC fixture is AVIF-encoded on purpose: ImageMagick classifies the avif brand as HEIC, and an HEVC-encoded sample needs a libde265 delegate that is not present everywhere. Skips are per-coder rather than blanket. The tests these are modelled on gated on Imagick::queryFormats('SVG') as a stand-in for "this build has the extended coder set", but owncloudci/php:8.3 registers no SVG coder at all, so that guard skipped every case and the assertions never ran in CI. Each case now requires only the one coder it exercises, which is also why the image/heif case runs here: it pins HEIC, so it no longer depends on a distinct HEIF coder being registered. testPinnedDecodeReturnsPngAndNotThePinnedInputFormat covers the one non-obvious part of the mechanism - setFormat() pins the output format as well, and for TIFF the re-encode is byte-identical to the input, so dropping the second setFormat() call would be easy to reintroduce and hard to notice. SanitizeTest needs the mime type plumbed through, since providers now pin based on it. Its skip guard moves to the PDF/TTF coders its two providers actually use - it deliberately does not require an SVG coder, because the whole point of those cases is that the content never reaches Imagick. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: add changelog entry for the OC10-164 in-memory coder pin (#41834) Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: report build-dependent coder-pinning results correctly (OC10-164) CoderPinningTest reported the wrong thing on any ImageMagick build differing from the one it was written against, which matters for the pending PHP 7.4 backport: tests/phpunit-autotest.xml sets failOnRisky, and PHPUnit 9.6 defaults beStrictAboutTestsThatDoNotTestAnything to true, so a test executing zero assertions is a hard failure rather than a warning. testFontNeverInvokesADangerousCoderForForeignContent kept its only assertion inside `if ($result !== false)`. On any build where FreeType refuses the PostScript payload outright - the safest outcome, and the one the test exists to assert about - it executed no assertion at all and failed as risky. Both outcomes now collapse into one branch-free assertion. requireCoder() proves a coder is registered, not that the delegate behind it can decode a given fixture. coders/heic.c registers HEIC, HEIF and AVIF whenever libheif is present, but decoding the AVIF fixture additionally needs an AV1 decoder inside libheif, so a build without one failed instead of skipping. requireDecodableFixture() reads the fixture unpinned first and skips when the build cannot decode those bytes at all, since the pinned read failing then says nothing about the pin. The fixture stays AVIF-branded deliberately: an AVIF-branded file served by the Heic provider, which pins HEIC for it, is exactly the case worth a real sample. The negative tests could also pass for the wrong reason. isDangerousToDecode() is a deny-list over the sniffed type and it denies text/*, so a libmagic build reporting the payload as text/plain would reject it at that gate before the coder pin ever ran. assertPayloadReachesTheCoderPin() asserts the sniffed type, so such a build fails loudly with an actionable message instead of passing vacuously. The payload itself was duplicated in both tests and is now a constant. Both fixtures the Font and Illustrator cases used are replaced by files already in the tree. testimage.ttf was Microsoft Verdana, carrying an "All Rights Reserved" notice and a trademark notice, so the Font case now reads the Apache-2.0 core/fonts/OpenSans-Regular.ttf instead - same sfnt tag, same DSIG table, same coder path. testimage.ai was byte-identical to testimage.pdf, and ImageMagick's AI coder is a Ghostscript alias for the PDF one, so the Illustrator case reads testimage.pdf directly. Fixture paths now resolve through OC::$SERVERROOT, the existing idiom in tests/lib. CoderPinningTest and SanitizeTest both call Imagick::queryFormats() unguarded, which raises a class-not-found Error rather than skipping on a build without ext-imagick. Both get the @requires annotation the neighbouring provider tests already use. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: gate preview tests on the coder each provider pins (OC10-164) Three neighbouring preview tests guarded on the wrong thing, in ways the coder pin makes load-bearing. PDFTest gated on Imagick::queryFormats('SVG'), a coder the PDF provider never touches. On any build registering no SVG coder - owncloudci/php:8.3 among them - all four cases skipped while reporting "No PDF provider present", so the PDF preview assertions never ran in CI even though the PDF coder was present. It now requires PDF, the coder PDF::getImagickFormat() actually pins. SVGTest names the right coder but compared the count to exactly 1, which skips whenever a build registers SVG alongside SVGZ or MSVG. It now checks for zero, matching the idiom the rest of the directory uses. BitmapTest had no coder guard at all. It drives Postscript against testimage.eps, which now hard-requires the EPS coder rather than reaching one through ImageMagick's own sniffing, so on a reduced build it would fail instead of skipping. It now requires EPS. SanitizeTest's guard goes the other way and is removed entirely. isDangerousToDecode() rejects that content before ImagickFactory::create() and before setFormat(), so those eight cases never reach a coder - requiring PDF and TTF could only ever let a reduced build skip the OC10-164 regression assertions silently, which is the failure mode this whole series is trying to remove. The changelog entry also now records that pinning costs previews for files whose extension does not match their content, since media types come from the extension. That is the intended trade-off, but it is user-visible. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: cover decoding when the stored mime type is not the provider's (OC10-164) The coder pin reads $file->getMimeType(), not the mime type that selected the provider. Those differ whenever a caller overrides the selection type through getThumbnail(['mimeType' => ...]): apps/files_trashbin/ajax/preview.php does, because a trashed file's .d suffix defeats extension-based detection and leaves the node reporting application/octet-stream, and apps/dav forwards the request's query parameters straight through. Using the file's own type is deliberate - a request cannot steer it, which is the property the pin depends on. The cost is that an implementation is handed mime types it does not serve, and must still decode; returning a constant coder does that correctly. That is easy to mistake for a bug and "tighten" by rejecting any mime type which fails the provider's own getMimeType() regex. Doing so would reject every trashbin bitmap preview - tif, psd, sgi, heic, ai, pdf and eps alike - to fix one case. Three cases now assert the opposite, each first asserting that the mime type really does fail the provider's regex so they cannot pass vacuously. Font is the only provider whose coder depends on the argument, so it is the only place the divergence is observable: a .pfb not stored as application/x-font gets no preview. Deciding from content instead would mean re-deriving the format from magic bytes, which is what the pin exists to avoid, so this is recorded rather than fixed. Comments only in lib/; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --------- Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) --- changelog/unreleased/41834 | 23 +++ lib/private/Preview/Bitmap.php | 43 ++++- lib/private/Preview/Font.php | 17 ++ lib/private/Preview/Heic.php | 7 + lib/private/Preview/Illustrator.php | 4 + lib/private/Preview/Office.php | 7 +- lib/private/Preview/PDF.php | 4 + lib/private/Preview/Photoshop.php | 4 + lib/private/Preview/Postscript.php | 6 + lib/private/Preview/SGI.php | 4 + lib/private/Preview/SVG.php | 13 ++ lib/private/Preview/TIFF.php | 4 + tests/data/testimage.heic | Bin 0 -> 1326 bytes tests/data/testimage.psd | Bin 0 -> 14988 bytes tests/data/testimage.sgi | Bin 0 -> 12885 bytes tests/data/testimage.tiff | Bin 0 -> 5568 bytes tests/lib/Preview/BitmapTest.php | 6 + tests/lib/Preview/CoderPinningTest.php | 255 +++++++++++++++++++++++++ tests/lib/Preview/PDFTest.php | 23 ++- tests/lib/Preview/SVGTest.php | 21 +- tests/lib/Preview/SanitizeTest.php | 29 +-- 21 files changed, 434 insertions(+), 36 deletions(-) create mode 100644 changelog/unreleased/41834 create mode 100644 tests/data/testimage.heic create mode 100644 tests/data/testimage.psd create mode 100644 tests/data/testimage.sgi create mode 100644 tests/data/testimage.tiff create mode 100644 tests/lib/Preview/CoderPinningTest.php diff --git a/changelog/unreleased/41834 b/changelog/unreleased/41834 new file mode 100644 index 000000000000..ff82c6f13ada --- /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 fd2b92d89b62..494e22944b67 100644 --- a/lib/private/Preview/Bitmap.php +++ b/lib/private/Preview/Bitmap.php @@ -58,7 +58,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; @@ -93,10 +93,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)) { @@ -105,17 +107,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 775147d83eaa..94c778800941 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 6e7bba4125ff..040c7665ae6b 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 06a98d9e4c5b..0267b4712d15 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 d8f038279b23..759b74fbf49a 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 0ab92bfb9cbe..40ae6397091b 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 ca15dc1a12bd..7f7add29fa96 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 bab73b808494..e852b66ba16d 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 81f43410fe3a..c9c24d4ab523 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 960c7368d3db..f8701db6d8fc 100644 --- a/lib/private/Preview/SVG.php +++ b/lib/private/Preview/SVG.php @@ -75,8 +75,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 25404d6e6a0f..9f7ff4bfedc7 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 0000000000000000000000000000000000000000..6b0bbd258cd3e51d65f9ab7bdb8d9a1d28ad3fdf GIT binary patch literal 1326 zcmZQzV30{GsVqn=%S>Yc0uY^>nP!-qnF!*)%1tdv1c?KIVn#|%5roOWP>`8i0%OBy zzJkn>Trh_bNGfIK-DFSR%y0#U>%S|9^&)(PR$6Kdjev zKB~Jo+|_#?;;~jr;fct*+D>=73=W=VFFW7o2~XVi{+d^FvvB83Yd$gW3U>edQp$9l3v2B&PDFC*sj#C=E3V&8TUq0KNTkn7gmrr`jp>*Voz0#M{m@KYr`?vbj&N(UnUf(H;SD9M(<8c3k#fMI2&aTbO z{p$NRbmHo)$%XNj>So<4Sv_l6_TIYm$=9`_|Gqx!Bk9n+3|}~yEBwzr@HnfMq2E1^ zVf_UbQT_c>6<+y1`MGDg8~bnhKR&lz)-W$Rzs>2NPvT)GIlYz(_tys;y2RbiHtpAU z?W`9Q6h)7<{I@$}^e7|qjH?>g}Y`iRW8Hg%mMu(DO-w%Lg^-ER5hl9~Uq z?{w6e?)m69CF$?F2Mwkd`V{tEmHCv~==v>Di@c zfow|5`oaAlS&W_}#IOFgy7g9n#Ea^HhZ#FpZJTgT{^*+{^I!4j_*Nu-QCC})y<%GA zjLw)TDRQrF9OP`ju_)xWi@FNaaa4C$z0-vNx3W> z?>0#9U6t*3SuXX#*3#)$6jt;5^Z$?xaE=LAzZ<03)xl9&$+#e^Rqxs?Z6(Gj+p4%Z zL`<0CzPkio71wR?*4!g1wd4QsncZjqC?N(WXh9r_BGR(^0>+bI*2v$nQ=~T-`W>cj8+M>&$8a;~xdB-FYpOcI2N9 z-5A~HsX5zh56AUwMv*OR=BQL={_vk7X!Cd$hpR+WyG~Wn!Us=cxQeIVyM6l5X>X1t zW)U@xJJ?tGWkfI})+uIPvtU9G0u|9*W;} z_sKYLR%ubFslb5??`l*R&1l`g^iS8HIeS@e#wOSLiGL;s^0r>S5V`f6apSJwf7Uzh z{;A}4i|vccc#9b3E}hWxY0-r@O0Cy+&Y1sZVf4bn?Tlwn@dmz|+|uIy F001b^F_-`V literal 0 HcmV?d00001 diff --git a/tests/data/testimage.psd b/tests/data/testimage.psd new file mode 100644 index 0000000000000000000000000000000000000000..16bee932e8def3c72af8bf20c82cb07db6d05ff0 GIT binary patch literal 14988 zcmeI3XIK==y6<}iNh)H-)C4ghHd#f%05p;$C?ZKfK}8Y7fC7?65J6FqBnA)=Ng_d1 zKqV=nf(R-INX|J7d4?J2`}W{Id#$t2vp?+*_ul8&kKH}>|5tTa)mv}ECf2jLq;p7rA@Q7(#d3fn(B3S9}%Jf#V^T+T* zH%(rM-ja-7lhj(f`PI#NQU>E+C@Izl(p@(FGJ9^~{OWM!+h!A;BJq2fOJfoZ7Ou$B zTXc+8eeHpkWpMa|Tk}IiN+-i}a-1*jah8*Ie6vG^PsnAjruJi+j$iYRowC&OR$q$C zLn;5g{L0H|axPMcFH^h4fwIXjiziyfN=tc3fq9wn@-*$R%|_1-B?YG(OP(ifTzM~2 zNhbUtE$@3x(OJ}TsV-fn!QfNVThj$Bx5fFn=C3X}+#XqB;q3kD(varJQbLhG(7-vz zmTq3`a;O%OlXd&i_=Sz@i!^>skBizJ?7MPb>Bt%$`pb2~F`+Z>`)*&REn2Qv z5L4Fk)yFow z=lXf@${VY@ya}@U>RRp_qQ?+X6}TvaMs-N#SP6FRT6T74OPX(8=E2iX-54|XC)Y84 z@$V5x{TlRaJ0G!n^mcu7#pH$Ar7;4;_tmO!xexVV+kX zm1#e93Q61Hw@;vp+oYQ(Z#%d6!PLuggEPPGWV&Upp>s*AofY`}tmIB~8-IT@cY37& zg)#8bnW7-_FvYhnruk*AQH{_lLkT`N|7zvoi-S!!4)5xyM}5DQ#!N4vTUKm(Du3nG z!8YdwX%Rp&On zTO3(i5V>KDJMGP;bB)5?m#{h86eGBM!n#AN`%Mp6F?Ou&+VkV}YV^m-^&hTey5|cr zOuUDV`d`lBi5}lcEvxxD;J91J&U48-sq5#cvUf>{bIi|MrCZ~ER~(6&KW?42VSu)A zgmG!Z;M@mHyC3)2bI zV4Z*W;OKHoGyYMFmetWdhjR7i=KI^;<1_jo>9{vTlD(epf2d*0i_P(Nt|uRyQ+Me& z{l&>QKd2#q9;Vfq)@F`g-O}RA{Y0!NCXZ(Lamr3E|GMKZRc?N1c8O8m$Fo8AQ|2!> z!w{9=MIO>#V<%o|*=1!%e$*E-vskEmX?^NMzTJhw(~I-*Qzu-FKM!=&>J4b$dc-xH zN*AC<)E`|pTof{W%s->URClG?^L;U?^v~VvR@pUA?->+I73=_&`=PClb3MwhX58NJ zOj_`VLh)zg!YwzJFDX5`U>CPb=5?9FTNg~6$nYrIX=2@*@aN#g2iWA zhLYCgUp?wF*F7zUH*Y7)Iy3BMNO|Bjzs1In?oD{E+U2xB~mU-q&X1>9>D5btN zJMVti*Shi?m2yoUdrQ<@xEHxcm96JE=V3hYQ!+`U4p z%J|xLWs%mWO?zu9Pty(+e_E$_E%N-NNW8yL&e%dqkoQAh?{#ZblEwC22(+)fRake~ zMom=WjYEw4oYcp>dmk1meSa`tWwE@VYT)&QId@JLbRjSS%U;e7xQG8pK{wMkSV+{-Q(jRQpmU7}A zW&NyZwp1Is^ljZ=(Q+Ur-}4 z6{1_SpxU`wQsqp=s=g}m_Y+;F6L#t@W;SwzwHi5{A6`_w6`aYi7-?8h8hleKv8%qi z<6C>e73#?F(i0AErB^fiZ+$(%lpJ4oC2ilQ^w$t}lFOlyop|c4NqmJ2=G($$FeNHfelwG;C%@Z+J*> zS(oJWbW&?m)8>U&WxQg>CaZ;S)9^_XKKE8)zF%GLpB>M9iN=c0#FOpYe_i zzg#C<85wnnaq%`vY*op1d1CZDy4ff3mF4(HJ72R!Hdx@MdzsPRE9PDGpIdZJ@8yom zw$8R8x3BdJ4o-%bmgTTKPdaauAI)QJQOy?RQ&pYIb0mA-``9IJ3y&ygalMbfctw<3 zxR0jb63?}GH15MYB~i5T7v%=?U~AF>_jdQw!x_hmgV*ca+wx;>gL`~tQcBCzaFHE< zw`6IoQ#XBr{am!da8*rAou-kA%&?orBI8}fDzRszgH$Kq*B>304yNpj@E zGL+JuCEFPI;e-gggB~&E>TzBrnm#4_iKUm_`tj2zU6cdYM9nX$v?*VC`0FB{*gZddo-a7g8_hMfxP0MW zor2!Y%Kp3!jlzQ_-StV=uL-%_HX5qS7j)=7Q6K4pRt9>AZyHs4^&!5+|_ic4@ z!tw1gk9b&q;oJg6~S7)}=erC9%vZ}ycnHs}Y zZBDT)gWWA7^F*!Y3&;Fgn82gHkNK^0cO~(>amV(lq)mCok2ZYVe5Wb(dbf4IqyO_b z!;juuKRqc~?G}A^Xk4aW$WP;xQbc=Y&hOY|YUR%d6P^b=h@6uWbZ%Vf1zWF3)zYG3 ze$?2B+I-KgmEVg={jpZoe23qe*AYVY)9XAZ;$B&7 zGOO2V9cnXcXvZ#Xow<{L_(Gx4;{5Apwz8A%-RR&+T~jJ6 zxamMCAvZ=vvFm`!`p3vcr`)}3ymlSp+q$*QG}LU0a@Wv2C7o|if4~9${dV@d4&nm* zua~tFYbC#J6dbTp99sQQ;ibC$RkyWsK82_)cUW;R)|zp2gK1M|BYm#S-2AKO&TI-> zs+=7*FX4rF=S-W5QtV~T*2?G2>st6~=dbjZ%`jc$)Ae2Om?3O#yPp3%e6w=_!ng&_gCw&hCf*U~FRx0!Wj__^{<`rZx-Ke8f{OKG^Ru{`2>iKGA3^!3%K?M73+ zlu0$Tu3oM)`-IfFIO!9&KP>9~mGR{Mo5(7~z!P66jcS?47Nrh0yXl=V&XXMba$!$S z&|(3>q3`=E#LI8lPRo`UO@_JsT)A6m$;(4=Q3a_Px$e7_UfoD6Fbs82cM@KAJSHhK z##(LZjlo0NvP16*-`p@6JQR}AraCS7F4i^vrF{Oj$md&X?=RA!$7=4roSfvAn4DC# zRO8e=GX0y9`@ndRcJGk>?kjnfO;%cE;ZrQ7;`}|K5z^z63Bg`ok}87P%y~)q`3vj> zHOHbaUDaia=uV_g&2DWh<0TsKW$w+p<7#&JjwdZ~OJB{U;I8PQ^j8lL$E^2mn+8P zIzvlaTpw5vbJyk7DgRyh#^;*2Cce9e&ch-z#a$20XNGuBZWFz}dq3-#_w<8&%cYVz zivq?swy3P|#FSg_TrYjml5n&UqZ_ZtTQx3Vjo12U zmG!Q6S${~tbJ4urOWY6hBwwWF>D{Q2cX0g_{rgj^o{*}q`$f~~poLtXuV3y6>*47v zc`N_RV1r`R(59nj&-D&E>QB4BH?zq0<*#s4+P*PhmCFtu_MBDQel?7Lj`ZrmRLZ|z z7B}#9SVBGX`~iBoN9PHTJLiyx;-lh3kPKWh%4FY#@P4-sJ z>04txv@Jc_`HcZvg@4HE#^A$6dXmOI31@X8q~i@IGg=rKa*g?m8-|1S^jQkz8+Ec5 z7@CRPEGwa2Vh_JGYS!V;dl{|Ro^LxYtABs1(4Jj$4a?h8S)y$gipui++gATR*6sGA z!eqEdmcIH;o1g^0bYa_~Ro5eRQmf|YKl-TOv1G;*t=#9Wk+|x@sc7x~X=&vz^0Jlo zcV=Xt7U#Y_U0C;pVW{wQ`?11>JIq7Ya#wWRM)SARj0Ruxm*Lkn5{0F-v>Uf~i^qgd zxKcMU54yR^qigB@TXhD4!fv1C*C|I^?E^oA_D{WIuvhE3hT40omYp-5TIu=Pc#E0V zoJzjP3_10!h0-G*is}uvuv>d^$Kj?xV6q0I^(l=M>-Ym60 zr*Y+kbj_2N>d`m;CfnqQ`!&6D6||)L@A`(`o#H>c@ayMdHvfw-!*GdQy7en}U;ChJ z^9So+*9r#f?sLLMy1zcNlrUG;ZQZV&GIheRVehMH$A}|8bXo;9Q*w&0h=rSHpD#O@ zxBcy~G2c+}vIUE+cK>!(*)4Rt%b~vh!4iR{r^bo*X{dCRMe5f@!yO^qo7*y#9zQL( zt~TK(FaEpoNIT>Frh|#+3(rn`uD&`{wb1&Peel(Wfxe1Dm8|>%y@v-wE1zVWj?P`c zvtZ`;dV15s`Hr*};VUP;^CqkhPgTB<_dw<??lfCAUq3EA)dO!6FMM|l zjfn~#d#HCf$LRDF!Dx#Y^$KA}@+7aH+l1IW^i-Cc5j$o4?8*WY=~e9_G=*9og#~WI zhWgj_#1e0eU(+_MPkP-N`=xla*y5elNZzr~#^vu9*PQrp`S$vUKN5~g>Rc6E5Im0d z2srv(=-9q6BFFb>`_&NjSN_We_w?VDc6qRna#E|9QoLb}ibf1Bt8v+2B_m`~>977|~5o*^ZwW0dV}8QMKVZWlhb zJ<(C-Up!M|^X16GWenT$6dP?uLXy2l5%tA|lkXIL9^T7Kmk4o?dVc)97TbOISI7Hw zA49tyUXyI)I%Ty)kNc%tF1|3DskwaD&LcTdNXpq(&Q&YqoKoa(6`exU-)fg_R;IQ@ zyizLv#d9!Fd$0w|T4)vV$X?%jQ91LBS+sq%Npyi*(KXZFo6^?Ag%ZnkjW#l`+kV#` z7vEF2dZLEMuTC%_sM|9xv^wHUM!U6Jg46pOXVfeNdgLk`;xz2Tcdp8pEMCWm8L2xP zxHI=+tWT!B>T6EA|M^raB)_(((7b z+S7cDJ!FqG^_h-6m$uIQYGpQ*W9sJW;ilp_gOp1)dk$|OKHqnG6LYwL>m7?OAx+Z-iCol0y4Y9>7Hifyq%=kHxysyqNI5X;s1Cg`GJbzKwKYrAm z{dE;V*lHd#CLS|8PqVZjtJ$ecNx zx2I|-&F6|&-bhnZ6Xn%A+g+T7CGsf4%?T{{R0l|H~kX z1vybnM)Ev37I4o0jDp$N_}BUWzq{nWNAy2?M)s3D$bK&yB_h#Lf;K^fqZ0&;L4=?T zg2p6*P$ofR5w}qmL1PmED4U>76F1Q5|GLZ&e&`HAqY#8I3a5YGAcPN!5VXHvykS+- z{xhdibgtCJxSSsiC9fWuiM>Z@Whm9Uh|M6V^rYQ|DD68+F(?@&sH|AIB9!(6rT9+M z2}+;-CzMu!Qp8gR2ny5jGx-5|-bYZTt-heNN)++#At<9N#VD;2owM2ate)gFO3`6= z6O>x95|q}0atV7j&CC$o(=Rnq^m8YHxI%F&N>MCg64WB}SO+{~(^r(%hEk+@;bpF% zAKD4TiBp1PFbJ+`+085{c?%`&C~{(wpngNov=Nla4W*otbSOzhAG8v@I%sJ0jSKrl zDmdj0P<{_hZ6&Bj(fVnE+W-8}FO=4SQY2+d+?fb-M9~&lE1obppH%U<5ZLB3-yhQ}TerJns zWTVK{F@mf8-hK%t$|q$U(?<{o(1{6xk}FzAIt5Pk1}mbdgszS2OcCbM6*(NEAU#GPahw`FqGG75YyK2;V`S}CCc(>kD1nL?+^e4?cx!_9 zL*1QBFx%FX<4BbVorJfyM0;2S_ZJ*p=i4_-aCdr&p_toez8WB z)`e0w6^#;n{-`4{sDm2QyNEe``lwd>B!Sp~-i=a(-;5BvlRHHURM9J}4q^_|5!GNY z2qcLkBVlv`$(unDw-$nW7IgvPCkQ<#Vme50?M01QEd&>1Kl*|R^DWYZ(t4rAiXnpQ zC8{*oL~wO0NHjrg%~Ymbw-V*Z$Z1fAVAxGjIT(F11Rd8IKxs^r zvJN7hvU3YOT_5cLwFWgABq&{SgD7nZrHH(N08`uob;re2e-RV~X;`q|M2AQb2_Gk@ zhUn-pL9IkBe-TszlsP~k&LFZ-lg&@c-rl=M-HP_P~QL@ z0!z`Xr2=Lk7S*4IHQMjL1^S4?h-?$@C3x%6eN{i>TyO^GCHi16&Cv&JVqTd%irGK- z)b>qPxC_R%7CqDpbmIWNW0ll^Rh`T%_b?ojk0xa-i9 zEP~Rg#p0OT2MNA3bbSF)w_f}OOnaZ$5pe3E&EPCTJs|D0ORzc4(gA`eNpx+7CVCI@ zpVit?h?{OHjL11=hR7hGOw7OWe?X-;F8`UtLK zG4$so!8L_#>Z>ETTGvXCf=f|+3|xU67e#iupWtGONX$TZd!kqB2&z3A0j5gy2rS(k zFu|fJ=_kkG@HoLWsw7t1KyZ}_@0j8|?+KWCW=%)Hl#O~rzWcFu&oC^>zU^p655d(Y zE;sSV1;bx7Ku~khE29Kg1bT!GYkJB6_3S3IZOp{%^H?ZFV{(w7Hllhk>(=O1FkM7V zSkP3Fuo4Sgci@2#%4Z-ku*BA}Mu|B?+fgqz{2YmiAo{?kvY) znA+4)b4Z&lo4^f=)O--akG&W5gtCFXY?LbM+ylL9cNB&$=)tUWf09N~v2a+cHR=!w z6DW0a*yK2*?4(^NDjdjSz}iw}qSOrmLlEWtKH?0Ny8iYA^t$VkC@eUeVTf8*$XbZG zF_deYf7uK~Pfy$qXkPm_DJ5*1K)J+>AGg6AH$5;Efy}c#q7!Da;re$`uDcbn=%<^~0`Sa;tk?0RA{T-nGP}LvC`a@iQ`0Edi{b4h>V6%vZ%_15$i)h#^ zqG7X$hRq@xHj8NF1s;q2K{V_ih{m~r7sX}~?JpN^48dssnNuk?cbCWWg39KyH!&w= z-($2gjOtK44eNb4O{Wl}ea9$A${?w-lT?Z@+7FC!n?$sJ^G_J90;9;kBoWQ^Gx-6@ zAQA1%7mQYkAps(vXh1X*>`_9HyQy>qr@R@;Be7QiT=rNKWZJ=uqrWg(2S(XJBAOre zg+wu|6msQfvuaX6R3OYMdEN0XSd8Kzp z1NjmZ_aPXDRO~t+8siEk?bQv4*5)j=;|(OQ2(WZw6qy7-v>{p9c0jbFm`*JqT7^31 zOCs8UOe03?!YJE|0nvgmS7OKnvw=J~r{5eq+yTk?Ea=@BMJf*vjiD)BWPtgQ>6Gb; z9U>7ejUyuwfM|s?7;+5|%?0xY;TH%!7;*wKu`XsqrgMfVmIa8$kZ!_gz0hJ6?3CG< z))4Fmy{d{WB%)Qnykj>jBTk@>s7TiUqLpDr%>D>1m=A?!vMK^KA-=o-(Jo*HvY6EI z=$z_NVyy8q3}Gim>%$NYs68OJwHFYrMHbTx?|_i#jo8_Z(MB*{IorUvcegNqXyrZ@ z3|pix77v+~eoMX=qm5zI6WL=hLE%^tqN# zK4K@U396}dKgfON{TOW$qXd(PW`i+64#W~beuyOjqA^ScFd7r1C_toBG?hrIz(|a! z!;S-@^{5PDv?+`tod*H7PYLQy$QS^SsP2LV`$Kw&6p^TL*!!_D*b=I-)4(%FFcQ(O zg2=)UM_@1)tPaFam?wzmu{NmMyN!;~*boEFAO~R|K+eR1K)!~R0)+)}ROBioj1(*! z#5gP-#0S`G5bI~fMiBdCv_X9OM|=eor8A7trZGepitVv77{3=-Fx2$LzJPdpR=fjZ z4(0`7{-0N>2QhP2%qJ0Tw|oO2+QjQH7h7x04(eNAbg&f9TB=|Mk}z`;(FTK*ppOS& zL~waPv_{PE_pj&P2N;}}=!e0y#i9YxeyU>1m!nHMei%sgz}VJfM**bwwL{W7-T zG{aiKS&UtWw9~nR&2fGQL`##F%QC_uA^)9`BN44v$^c;K74`@aEp^tG1&EfffMKR& z%6^O81J@1gKDhj`99TD2)11b<0MWk5V!ud4Q{LVWh}JH*i$pXH`EhWCb6k|Y97L1e zK_Z$z=2Hh>R8WDF72JnlhlTdd!+cG%cBWK(wDy8YH6mV@Y7@oi*8lDHjWX zd{-&Ae*~6fzdDu$xYQx9GWo{^!(R*#^8xb#M2p32r%6P!!2AKxT(LxeFYkRBVOO4E9wcO9-$&;_#&K(vxw z7$y@1Yqjnmgu(>m;|K;I8aqt~!=%CiuqnL;OpLlUgbq$!)Fl={~#Lv4@BeKz-!~Pi1wF@H;&-6 z|IDcrpL;Oi_2=Jh74MUsHQ(d3GMwsBPOkUSY^y??_8q63tsoKY<>4Zn_5-IpoPq6Q z(BTtKtH3E*Igqxe{63Q(khg$nGj3mSS|yG^lAw%R7vr=>e2%`8pS{5}PO&2qty!Z4 zr?uc*>R}xuqRkYX!toz~Xm_Ew6{nb&!+unby91&P>VCy(Z8+uN0K7~PUJ5{Vi&KK+ z1ES3s>63`|2};^=#A}kER^h%RqUn@!O7fs22Tuk>v%}+GMBY53Uco7cFB#NCJO_Zw z9dCt9OMiR*7f$QIDS9NLh2kWN-Nt`HuKZ?SO$x|i2y;KY63WByukdy=QR=@rO_7K; zRQ2I9o(YIH-dvjXrmT$ttHlRzftC1qe=X_aEeAyV?0tMc8%M$+WpyW>(PQF#2c2FF z5`-hpfF1P9zBM$C?nELQ z^9-IxBAR*=PV0phYhkA>z%9tH?ES`i9b|0P=RCTyk0T>KppG8i*8qrCfuEWhOt66Y z&~7HHB2bfh7rzaN=8YfM#MLh)f2Z*b2aS8>VqH;IP~MRNu2VSL^LOyM6^fv3y|aR3=+}o z25=e^r)Wc@Q;rzG(;e_$P@8brt|)zn2XWdIInWSbW(H7qQo{y-#MlrPY}LLYQbdx* zfxPf>*b?gSE5I{n@hL#GKoD85q=3Qv@FoyT@mnC$aQH!$GO(YH)7TILZ6HVD#USV5 zkst@-KY_v`IVute2_qAa1u+dz2k|MM3u5!E*b3r+h9!upf5aT1D63(dHjN|4px7O+ zfbn~SKjt)B2I9k6@ezn0@!KGN`twT7Am+`ApGZVA)M^1lo6L>&bGgXrk|RC>mhxFk zEzH16++haRC_T~u`gj~h#8e9qtrb7nQ2YAsaR%ol24OIr@nk@>>cjW}-{kMzRW|B< zFt*M3c|fTl{2YmBX*yFJ_Y?$2uYt}FK(qoaoHhiAmW*eBJ04F4cMFUr{5E!)#WD8) zqUGW`Ux^kStvs0aK@Ash+T$JIEXPA2?eyreInG~zXxaNT^KJ1&$bW8{;}BW>>c;_w za`1RSw5(ZM9w6E$Z5+2JQ}%oO3ApazBv*yuA7R~`pXM}n8xZZMCSD8MH|v1zARtVS~(6fo7{F0gbzg9#Q*1<+SBei0CD+(M%n z5UpC>9Ddb;3K?J;m^Hb8=_7t0^4)LEqoc4S2Tk#Oz@=`j!;C*J82)mAm|{E#5G@6F zArXz`h=&29`Qa}BzR2gXaLP#%(b{o)m~{_4983ZDB@)rf)h$Rwdjc*9WnU7}G_*-X z8!^ShNJL}Z!YxO@*m*#s0+N>_7@0Wo1X}aNO99cmaNHSE6ZeoKj(d@aHhvsW0pm*; zSO!iBBN6QvZaE1Vv>bLXt0vgeYRus7$|MX^_X*q?(q^YFxM7hx50i*?3=f4e`Y|?6 z-FIgIde`lz4qdSC21NUAh~pYSv6QA05DF7GRX>4)XxWyK)?-OT8?a&GRGp|1i1MNP zS`4_4Wd%Z}HCm>a{nOe3pFL>qWz4$Yr#0*RGrI)QU(IHf|C6UB&4siA#c9Dynlp_^B=u@ zGcrBxk^d#jL42ONLt@eI0aoAd&yhAA|0&w|_ssG?vJQ!8e@CJ}tOSVm7k~mpBdH1y zjbto9G?KUg(MbLRL?dbJ51Wl5-0LsK<^LW{BDhC?=f_=KKZ?*M5b7$wsz{&+zf}_m zje$@Fu8lu{|MqZAfPv7M2*s#33?SNwiO^UGg>o&3pj@M{5E>hy%u5d>DCzUq2yGga z^xN=(GEII!t_2XtHMpHY5ThFeVl;!$;9uVf^um2FRRBR~@UQ25{NU>?=XTXN+>Wn8 zXcWZHm!SBOH&t*`h0u`oJ_NF!hFpZpMH&(ZmpB@d50`uz@*6I{X~-yCMrqyzz~cX& z{+}CzKhyV*$^0|5ea<^{t|+J3CzC*?B7E9Z$bXI Q@c(a1o+@d-!sq5JhM9aF6PUp<<~yo z{{mX}e`ZekPw~HE1zee>YfT7)kuV>=g7ffH)7&aSW9S72d4Lj zX_x?v(T92XB)|(ztKJ*7Y1(V~p&u}h8VzB!ruh=bcb%ryWLz~5YFaJETZ{O$=%cnD zybsL1_5)3mwShUxFMu`k^Mcke7FZ*{lbU8DhOG&-gFY}0ro#$g3^roeEw# z$`Am-&;v#R^YY&Ur{R&N1>gyw&wy?)8R#qEJGiN7frX(KIi_@_AJ>k+8n`ZjZ{aHW z%?FItjXb#xfT^$^SO>RX;W;^V17G+T&_|iUFa%Bm)v{9X>vaVm_tR@sA4*~9|){%<+tGwxo-xnr(kUbd2qMEE^;3PjNj7-bhtv^ z8^Hp|AnzSu6L~KV#PB{&-UHzy$Rh9Efim`ZF$n_HO{=4rl@B0S;i_1~8_8L~>sen71pXD|_0N zdAoi9pTIu2Lw;SL1~6AQa^%K%+_u6=NG6}JKn3sv1zlk{5Ub20_#F1a8Mq72$a5*E z2E;7O*vs~Y(STl#*yVeX_d>9kyt7X#t|0FfVHSBW026@quF?lCl6&$g*e~vk-<^4R z425gty#*|Xr{uj8d_msbfbms5O5SV2bfB+l^igdmd1o#@QNW(5-W2A;ZSwvGB#?K; zUIV?xLvlZkyq5sxS&M#a(Qhs0S{qO8p1`4mS3SEgsn0HukfPHMcPq+Y@s8D&Y5T0_|Y{;P+n%d*CKHW(@;;p%n}S<{Gd8 zPQU|l&$t2^hbyJ)zkv00jRoSl{s<}Lm^f}WVEk^(*KHx}g!4cjWeNkwNtrr8oHD&& z3`~V(um$$R1)#sO`G9z3eW4jJ_Okt80=cK3ayQ8T5^~SDDjXvBAwXG)@m1PJ-pN_z z<>cKJh(QxgOoZp;p1(cFpGQ17&ku~n^BeMB9!9|_@*V&);U0OXZ|~2@dvRd@Rbz}k z^?)_>c|zX1!A5vN?t26CsKGpId`#|%EyZ!1i=Vc2JA&YV%X|KYv>6hAqEx$W3Um!b`}x=uYVa}E&Q7Ud)t2q#K9Uk z07>MyB=|!(42OBZF%UpM0h#2UaRky{BZq&B3m%KS-h%x7{}R?-YYrTD8tbh20c+`` zI;9Wfmbt&Jzgb_6wa_G3ZEAowI5$V0(w*NHx%Jj>j~u)xJ;Bnd0&C;cDp6L<`Rz$* z^=o0pwc-VW72EpT^5w$*&|LH2#&7na#@^T1Z%#^3T97A=<4h|J|C9C1o12&AE3f{4 z`}<}6{ZGdFvbO)^|DXIf=NS0s`2X+igS^M|%Z}H)dfwmGImtTzSmd6Qa?4`WSy+GP zmPPq(k++Y$zh8E2|5Nwh%jwJ3E$>?A{hgbayt&GozucT!Wp2L9mNff{lBPx>vkPSv zT8z)AggqD1yO=TQ7xpBK^5Gg)%p4?&nN}gQ3uP6Wy;AJizNYT0V<>7CB11BD5&^iqxXiD@xOS zqRf0gf2tit?WjN1CQzI3r`kQ#?)g*g1!^z+sWzS3^#58LWwK8HWnBL&>q?2ZI^9Rl zm=hW5TU5kV4WInk51$&1ON(j$iWm{MVtGttm!@uFhI_jOJ5MG){q^KGQ(IIKS}7UV zVnEdVMH9L-Dk|dwyT@!k_}TQ3(sEjxe(%p+oiKZ1-_~w&hF`bkyU!#&y?18s-0&(= zE2ZK(ji0e{RqUX!qAD(A*oQlhZClj6w3^m??6hT@H!X=9)6-4O2pYO}|AmySqzea^ z^{JvXXHH58NBy|tFAAIXdkKvS38VFeLZ&@yP5olR;njKh&7!8`>PH1Fu@`zltcP9KoYE-lhHR z^&^5xD0{?)MB3k666&JjLPst;czQ!jw<0-{C?@PR;~y6{&O1tfYM4H^5;70WgEpvk4T3Q4V~GR8fyn!Kat)8zg;1(m(!tQ4BujSXVWhuo#fPaTvBUPqIw{R-ub zu81(EJS%NFj*6xkB}{#P0b#s;zJh7|)R132Gs>Edn?a%*JzlR-P-sP@5%(PTPqOtR z+=mNDBlHsP(=GWm@Dc9YZ^;3;Z#F0>wIa%hx7#D4>j9S8<>3%CCu!ao_Ey z#^JuxwqVY1T!cQ$&9wIs-@j@`_Z6b+gGy#ha(f~AWtBG_>2Hdq#Z7$`BXN=X0*u4t z0gRDS_J4>mrKgm`pJDthT&}{njzPF6{S%B)>L|u3N{vg$m=>wj6w8>=S8c$!EjNB) zp$~R59a&!0vPzoKX${1bXI0GTr=vyFv2(BmcmbgmmiidW=wJ64#`{4s?zv@* zlCAG!yxG+MIL00Yq*hqz(Uvi|wPk!5q2euL)(qA2A;vrHgHL0OERZvQ_CxSvhWm>M zV?(_wbWhVUN_5G>y5GG?RxwkbMR2yj+nKno%dWVF%4xVJ%f8QW9rkNwxfT$atoOHE zx6}|^lhs08)72Qubt0&T<@)50u=;+s%C}--Nz<`Zbbp5RX}E~0Xxbyi$2fO6qw0q- zeJ|~pR>=X+EmIE}hiQUbi)pVjsD1*|VKo=i1~uL?WksrZOtESUrh~aL3ktn^S<}8l z7$r={$*TW)P}OuKH4s}F-c9eoM@3Ej9zhFAeE_a+<#b$U0^2;ql^i6u79-%HA3ydb+=WgOk62>!XCkTQFJQKhHBzk{n~Lk#kPaERy0vL&#GHP!~(p%s;1+;>;5L*jp9QW zQ(wzjqkzf^8$`i~>sk8QYGQA$ta>wbwHn*@+%pCf<^(=7e`1f#rt3rP?uZOnUjWacPJkBBNHqI03L!9@* z@(kgLE9a70J_qxg(c8R5|Kx(E{Uh-SE#`~Gw74Yt7cz}sT5)Y*2`uy7#&v(hyrYgB zk%D=pT!(py{222UIRf+bPFz)3h^Lqn!$VJE-rGujkcN4^+KPF#T95g<`T+B-fw|Wc zmLtiPT_HXyXwEoZN5~-`9k_X{h5N6xxG3JHMY4FSfN6iRygCPJZu0X3qyXRb$ljCX^61u8w8uh))n{k&#%d@8v#w%`d7|2tr{}mpZpe zA*r{=r|RJQMRvIG^XEo;ZegbFAFcE^uG8aTH(8)`d_*D<($5`w=4wz znoDw)zxSmgW?aUmnZvqw61{4h_HdC_j%#fxXWTuO<@GvOusr%Qe*A!UPqe z?vhgw_sA&35%MzPSJ@GkAnsE$5f7*th##nHh`aOj-5kd|;s}oQVj7M~Vjqs^?7rI~ z4w1(Zx5#;j(ee=D%LUk<~ne>B*&mvQ+&(B3LJCAMjWrx zH>dw!5R?2{BYrAZBYq^mKn$gC&iXeHQ-XUUZd98QKUKRBd*$hS9FEmuHjd>veQzpj zl0;|UI^qVo5HTUA?=KNAmv;6&8}Tc(3UO0T-`^r$E1k36++uoNDZyDwe2ZnXm`eEf z#UU(Hot+#0*HGum?@_;!b5R}gDC%5i??&)V)Me^0>Tb0Rb)Gtfx-7SQHfmoS!^C+U z-{++BdKr%U*XW+@8HqYxUP3*Plh2!|iS_B8O*sWMR$WCsnv>8ws3~o7yXQt{VR6&m zL8N0jBYHA`aPg#!sgHAY-rTG}>?MCgJSPVtc9)i@yE<=Z)*+5i8HkshJ8_tLia6Uf zXJrZt!((SL(`Z<`G_MT2T5x>SwGsW8r?YRvwi~f!6U0aU;fO7oA)d53`!<61BX$i( zd>q^dv2zE+vn_M`b`eIk&D_B@uFaXCow&`Re_nKPGmT#Z-K@JpqwZIzAx*4vkhD%Z zmtdS$xGB0NfHb zH4b@jP%3iD(6TuZc#w7RaRZE2VA`FxOc>LvK}Alcv9=P<2MWo<$es$ zS_C}AaM$l03}@afXFagl$L`I*uu2`Z44p%qSIMA}7%p@vmlNkzo_D*)EdRor`f&B3++`NZ3oOw_-2NTLtiB4p-0Qr==W#TDKE8Gd5RssWYZ6eR6P1B zwH$q(+Km2iQLFMfLCOieAy=T}s@3mSH)F1PiN>>UapQ9@zE(w{m6LifVoG4WM8xX> zGGyUx#KfhID@v`L(z_w1g@kfX6LUL6b$33UrmP9CnA3N8E)GPVAjVcV=M4~VdI|R~ zA8kLAZl+)UVP0GJ%0es8WdVm>PQfu!h6hN$zAJZ~OE-V}<>2B7zsgc8&jkU;95oY% zq56iXpfT(BUCuBwZX8P(5>)w*Py-pvgFw%ptC_J!M97n*_$HkBs(Cqsj7EF-hV4As zaoO=n^3qnE`09E&ySJmIcWQYB?X-IDY3iCDocCIL{ob+R&LkY$M3Yo*dhgcty_(;Q zd2wb#OfS!>R^NsnGjQyXt+As8f2?#b#DYQe8*nZ!@B)W?i_a8k}vOC zGO=aVoCpD`lbJ01Ad-=>ZVv7q$g9n{gka|Cqwz7})ypS0ox1(fQ{Z>%=Sd@3v zXiIrASnalypxiDi3O&9wmzVrPf8WzO$BpM+?v%ICDoTAR!VmeS?m!qX4TPV)rB+et zYY-0SSNc4JSTzUXqIb>^D+Y^ zlM9$JkB1A-FZ@8W z<_$|s;`|Wx-fdjTVYy+2i1qXm0TcE-H!(l^xS_Q5Sbi1rB@TEgLoD+MA8X>VEkjhtIY+WE{}$54 zGOrI)L6K{3V7{6#I7q4PPciF0IZGhKtoIVZN-YXQ9a+3 z()RA!7R_k!d39@4vy7C4u1bXsiJG(AP~&KEf739=(sj6F;b-gOjmR#%#TvDxa|c>{ zS^f8+#qBrEXWtA^uW?e+d5FHXppCDKnCjR0<0R8;FCKAG{?^kuq`1ST`F!ul_VqbM zO~ZM6SwtfjIkj1zwaKR0Q$EJ|oy|FvxDz-J8hzVv!di&)?%Kg&E^6vqqqn4*W~9o* zd8p+dL(m@==3URRFC)b2(q>%x#qZXBw(rU#ZnFo9OYGclUVF`25j_)E%E@o~$ql%E zyS#tn=0n#W;~Fim;M!aNHER`|uC%k?k5ymc%D8rP`_AJxGjZ9~bzBGAyp}WbDnbwB zk`XC>^spXK_p+;=Pj{~&v?@}617U(Zju0pJTf&EaHKbNW>75W_)ft3%^&`TI6+tzA z@6Ygk!SN6!_92fIhj>!^A*`m=pV7z?V&x$#l01rVuxm}LKO@W%<|`}GOmzz3*bx3r zqVsk^2%Tx!7l@6By~SlVTk_{`*LJSNHH2mI3xpx^Dnjbk-nE^R=asH~t=fe!N!>uW z|MiI4fAq)g6OY6nd6;pbV*bjhBinfRIlJS=iD%%I)OV3f*36w0VdEdUv6S30@zA@T z`ax>-mSxih2l@TcSwF;MVz4`xgzEL2-3+JZq#Q#{X1#jO4yI6ZRz+Jiq4jc_x(j`4 z5nfjY)XWm5{&jup*w=YI;CSA=kyW#&zIB}Id#HKQv7J?Op#JYo4ZlPl$|s8NsUGcY zYBWot#x4&~6Xk4byp=-DTy>P18M#e8gwfE-B&GA0Xn1enZu?1OW6NpO*=GGQp|NE(TJ7SG zr?HK5y#B(7=H0H5)UJup{G~C|`Xjkp6RG(tWAO|ANFCZF#~UDw@FP4-WvvQqDzpG; zbU)5t&(^)sRB8dr7GZufHy|I?_J$u=V=q!KTpHrB+AT zC!etXOyAM#^_&*<#B{$=3A;{SIr7Egu3pWBR!>fEHu>|tXRaOJ`Emb%=2EMtroT1) z%Yzqgp8jtAxRB;K?fyUbfTh3m^PK+jPkAr@?Dz6dgfIV`nEmPTr5`*EE%$rnU&#N* zubuztbLn4wn9cLe^{*`d>D%q!eE|Nu56FL^=lKNv_n)!<{!{qhe@_2*KFa?)ANT+L He=6`F9S(1Y literal 0 HcmV?d00001 diff --git a/tests/data/testimage.tiff b/tests/data/testimage.tiff new file mode 100644 index 0000000000000000000000000000000000000000..9db137a2f5a6c78c1a7133c9551c38d51389bd7d GIT binary patch literal 5568 zcmZXYbySpXx5ft$P*DjH5G2H)QKUg;kd!VJ7#u)Gx&#Cqff>4_Bm_z6kZwc-&xPPpZ(i=?Q7rrzh^y{iV82lnH~W6Fuk2ZX+W>f7#R5gLFW-F zTBe-QZk*X}5CVCo*nGp+H~5=@>|M@KGiBORU+#=aZT=yH()Lz{`m(kNfocP~%poFN zTs_siVyl0re{1W<;cDH0f`P?uueb|(#$`bAaJ7%+Nr3Ly9llZx<#C&`S8|yv!q!2& zX0UE>pptYqc)IE)N-&|kLR@KWKqLqWFLX64iXP^Ydq8DrXfyy8sb_f7dj6S-@O9^G z%7Wfj-moY0$fsa!MbcK-FsZlLk1G&p_Z*SaK!s{8ePystrY98YQFvBvYZ`~7HqQl( z$WWrfEbYUFDKpF%XbS_%H)V4Ne;ip(qs^1dj|NF~tU1oy+PG)r}mphf`jBj&n#9OaF#gy|YV~?{<#5u}Z!Y zP?sKfUW$?Loh%96-rIEnU@ftj1<(sG>0=U4gHXq$5WDUO27K!_@+oNa%G_|A&%>g`3(k+j|8Lrd z$|F(+T#bT`P`vp5(VQ2%AM_%j%Jb}CkU$x^w$nfv>x2{Bo<=0xxFe-mV%(tnqdxRN zY@NCfce0_5*?f`tTGE9Xk6)fo8y-)=@mMT6|M1+-8J^YmuX#qC-U71A^nThHFEksT zZ^Ysp!DXw`iIVO-v$$=a;UK7v9R#rppXKImig8};eRGV2*At_j5;}&5Zd%3x`9Hn+ zjwS9Qb_ZiiWjAJ!0a-Z0W=Xur6FaG**;@H^`O3@H1;pOq&!gFXnfy370ec;G?Milv z2V58X*t~-QbFwiKCl^E=i;+$RdTFwHoDH>^l<;`fmYZo{(F5`E1+R)_6{!#k`Ux`d z_3rCfLe~g?TK;hWS&<2YKsQo6o+`dr_j@r}o!4n~M&-Gr^Y3qPndHvdz7W-T{(ho> zx6_BiY8mX)Js;QiYI=VzHRr>7p#g_ax4a^@w zCoRp;-y4#?V6n64rOXba%E*M9A`TPrsWL?Q{E_>7s_WG3v&U6bwT}HX(1*P_*?i)I z<*?7Awa@H7ow&A6uE$3IPz)KlFRi-HE^L0y)oONh0ljxEZzMyy;ln;7>wHC`GrHe? zAL)Q{^~O@4z4EEJ4^my5|Dkv<5hScWYPtByX+zXss@g%xB`2)9d*h?XOz9QH;+1mN&X)AeV=AtPw(Q5x#*j#VD_ zopUAko!Q(&yNGuOy)dBW6sPK1qpdt?93LS+Z=k&`cQ7k2K*Uy;MzSVme8TeD4m`aQ zdJ-9%%AmE<6We=GkE3QEDfXhYZkyg+%e}{SYcrF_0E{SXNu6sH?0Kri4!sBEz6)a& zFnJunJXVt5DM_07%VN+uXScxr_KQZhF*{JrZbznwsFV*Jc=pN6OO!%wliGvW*-t-| zM$N?9<+X2nGc!-K?-BpS_P-u>=;F&?fTi5&M6C^!cP;n%6KIp34^c&j7nBrUQVk=b zsqM2)=DfsF^Y=8DH^PnHC&@q;J(^`iCGD8%Tpggt?jWz@WQSR#a?K9DeOjjbt1wp_ z_p5}^%v%ELyN$W^CVool-@=XDNBUhqtyMT@D|k(~9sgu{`ouStxSyM?Rb~}Ch-mwj zPsDnj!wHGQCu~_`)~L1N72EfORr?flPlY=p*3G|WXx>$L`9`+=lwwlNZnm0!u}dQo zxDcQBTRUP}N-U>ZfMukE-Kz;PSLX;|JwFpkk**a&*(xv^$p4z5?XV8#PDj)`B6IdQ zOu(Oi1VrmQW+c_@9HgOpl7fJIMxmkf=TSA(R~Z&;0ed8f*;26wec|*~h)JmReD-1B zS>|I3gznGF^%I}8%QDyxMJX!9w;ECI=VdEH4l_SnI#|6Ee6dyE_s^9Ev~uDfnGBW5 zd=CfC{T5B)v6@j=OS917P6v%UIF|-aO3#?=QF@LP-W4s-kC}sUeMg|XMl4@ss_0|a z{N&!h!#OwL+N3l{J(LE*jJi8uYx)&n^Xgp+PvW$jhg)*}hXAA&(MP=Z%zrcYH&+|C zm)Fxsna2#fZx%hTm5aq55Z|yZY%Z!2Jlw}^sl1VX?b=dFCaiOxe|3;UOy_RV2_@N{ z?9>yNH&yl+FE7{RHz_3~l}3MgB7JFMlkpZPvu;E_|9uyo(l#YjtwNKHA`SFK=?VcN zbbkueB!dHu-NQZfxm&QTi7$Kv}h3n7S~EtWuyZe>IAqROeDxi z+4q})2U2-phe$2G7Sc19W+pSnqHmpspMJ~KDVeDdT+rQ$Q040x$YR!TY<*B9Ja@XU z{*XA|aMvfY1#l8wZ*sZuq$ifQ*{k~&Uu+zPDI!funJg$g^O{&tlav3A69&T5rk^tqN$&{Z&UV12fT zES&0i1^Vg1P}t2PJWLXtM&Gx6L%#5JO>XnO-(5 z8?fY^%YDV^5!z%_gCzBdzjQ2cxuu6oL+u`;yGC1UyJiGVRObwG6G z7d_$4$jRF9mqhr>_>w7WI#KbE4xnk#4O_QWQWHZz$Bzf14bXah9UuHSq<{f>7GKyH zOeu4G)IfHX_U1PZ!`6%g0fY6=A^y=jVP&2ZY%;sr0I@Bc3#Pg~UX>gI7=N-E- zu;ecS-gQW*Uqn+^CxP*s(|(S;;?tcT#!vTM@`iL`QkzPU(5T*=~&G zfOSuIUBAhg6l5m5>Bu4(ofrD9lmXJU*`!_*(J^Me1uG0b^f9IQ!HFCu%@y z!^w@*Znj35Zc99|NuT>6I*nCZFZl(|gQ7`12_IEvTKQdZG(C)uEuIh|NncEcT4@m2 zwH7(z6s6gfhw5ou>)_BC4lR1C@-1@_+g@jI#as11w$PJ2K>W`g}I8+&PP** za5@=yR#FD@ZRdDObX{q)_Ql(D2pUVdP*-8JAX1+uM&jvO$(p!gh4nQf$X(-><<-L5t)$Bzi?oYH?6u-1ENE?@&Ei+t^MU?%RP8G< zj5k{xt|s){bu$Wv73pMiWq+?V!M8zV$Lbpv*tVcIS@2c$@v8ZGi#Ev*Sx-qv$0s=IN$!m%NFHgHj~3 z9@(`p^9d;%=|5jHtMDd9vb5gG&p^D4f-`4fcuOjsEn_;Ibz{;xqlf5NJTE5g57Y7c zF~(#5*-0|06ascX%(-UKV+WDZ5=%?WZ<>=Yq_acMvM>;HBSJ3qGCZTx7|-&a!h^j zc*%R!Y;gtzAB%9o{K>i#4$L7dbe!o1>Z%w+@labCEdz|&A7~o$eiM|8`#9?%H6jt> z%yx8lR6$`!*!f!)yK|5| zt3K=&N-? zMG%UEeFm6P`WAEPk%TvnZIdNIzoo;^^KIQY>{66+DEgXRdhDsS63XgVV(i1frN-=3 z7b=WauU9T4NnVPV$=bfygI)b)M=n8V&L%HLuprj;)mswZrUI|?{NiG^SX}|^_&{co!ZTR{$ohU^%wlEuZjmXLPx zGiJ;QHJi*T?AQP|N-@D@rby$F07;S6d&WAI??T7zI?``_F~kOi8MPDBWXbrD-|Q=Yi`Xyr$incH%|or-5mQ_z*1TFgyEKXi}j zBXJ!l*Xkn&G9_n_FYqz@uRAx7)9VJ_hGCqp@MIjmb29!p3+e1=8OxJup5~aaRNA5g zx7n#d>exEFnx_rj2A(E6ju!bhP#QguR_`Jz+PC$yf)9YBc?WgIAT|*c5(kKU*)EYo zb?UPZ)Ed-mKf+UP!;EYtop9YH?E~T4!i|3S;Eq9G_nERy0gGCoiAHN zgQbSsM>7;=FHgo^=^5Wo26-)D3;O#J+sr1~DI0P5vObF4=w)r#-EwraCeXdPdwcd`q(_-EOFRtus* zJI78b8cks=tn6Z#v*(F<8T|j>-uN%K-u5vZg3hN`=QB5l7g zVPvX0{~yZ*qE0loL&^W~R^*;4etWmws|@UVC-HzMEea%4+qndxwTJ#YSs&3u0As=uMT(}P?2vf6O+97JGLPiwKy zzTLgP9`MV;l*>6X6JUA7eN~8ApO|rZmKpE!Z@dFXLG$8jg7@kvUQf3F^j+c|*A&ko z@Gi1X*A;FtCp3vcV|TJb`SH>_RBMCmUCAF{N%jrzh2^oL{004Mj z9Fl)A=>=c9sC~Ev0FeEyU%tpG{xazWU;WE}^t2?G{+aWqhwfkXTYve_EQY`AeZh=> z`)w{(U?w5|M{9L)7t3FMcfoi5HHZ5zlU-n8!rR?^8{?_{80c}FnNF4Nc(!#|3s4iL~p>~oHX(m=1-kQ RocY_INXi#<^&)Tp{s%UN0|fv8 literal 0 HcmV?d00001 diff --git a/tests/lib/Preview/BitmapTest.php b/tests/lib/Preview/BitmapTest.php index 2c808cffb787..8d26199b62e9 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 000000000000..2ed0d40a7f5f --- /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 4c7d700b7d83..ee0c84842048 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 dd4a2af53af7..5926e0a6811e 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 882a1230866c..5defde72527b 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']; } } From db44e8471baf90015860a5bac2262c2065f0af50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Tue, 22 Sep 2026 22:42:30 +0200 Subject: [PATCH 05/18] fix: keep a missing stored mime type from turning a preview into a 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pinning the Imagick coder made $file->getMimeType() load-bearing: it now feeds getResizedPreview()'s string-typed $mimeType parameter. That value comes from FileInfo::getMimetype(), which returns whatever Cache::get() stored, and that in turn is MimeTypeLoader::getMimetypeById() - documented to return null for an id with no row in oc_mimetypes. Nothing constrains oc_filecache.mimetype with a foreign key, so a dangling id is reachable. Uncast, such a file raised a TypeError. A TypeError is an \Error, so it escaped getThumbnail()'s catch (\Exception) and surfaced as a 500 instead of degrading to a media-type icon - the same failure mode the unchecked fopen() had. Casting keeps the degradation graceful without weakening the pin: seven of the eight providers return a constant coder and ignore the argument entirely, and Font, the only one that branches on it, falls through to TTF exactly as it already does for a trashed .pfb reporting application/octet-stream. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- lib/private/Preview/Bitmap.php | 8 +++++++- tests/lib/Preview/CoderPinningTest.php | 28 ++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/lib/private/Preview/Bitmap.php b/lib/private/Preview/Bitmap.php index 494e22944b67..b0354281c64a 100644 --- a/lib/private/Preview/Bitmap.php +++ b/lib/private/Preview/Bitmap.php @@ -58,7 +58,13 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) { // Creates \Imagick object from bitmap or vector file try { - $bp = $this->getResizedPreview($stream, $maxX, $maxY, $file->getMimeType()); + // cast on purpose: getMimeType() reaches a string-typed parameter, but it comes + // from FileInfo::getMimetype(), which hands back whatever Cache::get() stored - + // and that is MimeTypeLoader::getMimetypeById(), null for an id with no row in + // oc_mimetypes. No foreign key guards that column, so a dangling id is + // reachable, and an uncast null would raise a TypeError. Being an \Error that + // escapes the handler below, it would turn a missing preview into a 500. + $bp = $this->getResizedPreview($stream, $maxX, $maxY, (string)$file->getMimeType()); } catch (\Exception $e) { Util::writeLog('core', 'ImageMagick says: ' . $e->getmessage(), Util::ERROR); return false; diff --git a/tests/lib/Preview/CoderPinningTest.php b/tests/lib/Preview/CoderPinningTest.php index 2ed0d40a7f5f..4b47bab46ffc 100644 --- a/tests/lib/Preview/CoderPinningTest.php +++ b/tests/lib/Preview/CoderPinningTest.php @@ -235,6 +235,34 @@ public function providesForeignMimeTypeButOwnContent(): Generator { yield 'SGI' => ['tests/data/testimage.sgi', new SGI(), 'SGI']; } + /** + * The stored mime type can also be missing altogether, not just be the wrong one: + * FileInfo::getMimetype() returns whatever Cache::get() put in the row, which is + * MimeTypeLoader::getMimetypeById() - null for a mimetype id with no matching row in + * oc_mimetypes. Nothing constrains that column, so a dangling id survives there. + * + * Uncast, that null hits getResizedPreview()'s string parameter as a TypeError, and a + * TypeError is an \Error: it escapes getThumbnail()'s catch (\Exception) and reaches + * the caller as a 500 instead of degrading to a media-type icon. assertNotFalse() is + * what detects it - the \Error propagates out of this test as an error, not a failure. + */ + public function testDecodesWhenTheStoredMimeTypeIsMissingEntirely(): void { + $content = \file_get_contents(\OC::$SERVERROOT . '/tests/data/testimage.tiff'); + $this->requireDecodableFixture('TIFF', $content); + + $stream = \fopen('php://memory', 'rb+'); + \fwrite($stream, $content); + \rewind($stream); + $file = $this->createMock(File::class); + $file->method('fopen')->willReturn($stream); + $file->method('getMimeType')->willReturn(null); + + $this->assertNotFalse( + (new TIFF())->getThumbnail($file, 32, 32, false), + 'a null stored mime type must still decode, because TIFF pins a constant coder' + ); + } + /** * 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 From 0d1bc8e0c2b8885c7d58737d5287251e55623807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Tue, 22 Sep 2026 22:46:23 +0200 Subject: [PATCH 06/18] test: make the preview gate tests fail when the gate is gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent weaknesses, both found by re-reviewing this range rather than by any failure. SanitizeTest could only half detect a reverted mime gate. Removing the gate left the four PDF cases passing: PDF pins a Ghostscript-backed coder, which rejects SVG and MVG bytes on its own, so the assertion held on the pin alone. Only the four Font cases failed, because the TTF coder renders arbitrary bytes as a specimen sheet and so yields a preview. That left the two providers whose pin offers no protection - the Ghostscript-backed ones, exactly where the gate is the only defence - with no coverage at all. Fixed by adding a payload that is denied by the gate yet decodable by the pin: PostScript with its %!PS-Adobe header sniffs as application/postscript, which the gate has to allow so real ones still preview, but drop the header and libmagic reports text/plain while an affirmed PDF:/EPS: pin still hands it to Ghostscript. Verified both directions - the two new cases pass with the gate and fail without it, taking the revert detectors from four of eight to six of ten. Also added a control assertion mirroring the deny-list, so a build whose libmagic classifies one of these payloads differently fails with an actionable message instead of going quiet, and renamed the parameter from $svgContent, which no longer describes what it carries. PDFTest and BitmapTest gated on Imagick::queryFormats(), which reports only that a coder is registered. coders/pdf.c and coders/ps.c register PDF, AI and EPS unconditionally and wire the Ghostscript delegate separately, and MagickQueryFormats() never consults policy.xml - which on stock Debian and Ubuntu denies the PDF/PS/EPS/XPS coders. On either build the guard answered "present" and the cases failed where they meant to skip. Both now probe the fixture they actually decode, via a shared helper on Provider. That helper is requireDecodableFixtureFile(), named for the path it takes so it cannot be confused with CoderPinningTest's blob-taking equivalent: handing one a path would throw inside the probe and be converted into a skip, silently retiring a test. It reads the fixture outside the probe, so an unreadable one still fails, and catches \Throwable inside it, because imagick reports some delegate and policy conditions at warning severity and failOnWarning="true" turns those into PHPUnit warnings rather than ImagickException. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapTest.php | 17 ++++++----- tests/lib/Preview/PDFTest.php | 12 ++++---- tests/lib/Preview/Provider.php | 44 +++++++++++++++++++++++++++ tests/lib/Preview/SanitizeTest.php | 49 ++++++++++++++++++++++++++++-- 4 files changed, 105 insertions(+), 17 deletions(-) diff --git a/tests/lib/Preview/BitmapTest.php b/tests/lib/Preview/BitmapTest.php index 8d26199b62e9..296c927dcb01 100644 --- a/tests/lib/Preview/BitmapTest.php +++ b/tests/lib/Preview/BitmapTest.php @@ -30,16 +30,17 @@ */ 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'); - } + # Postscript::getImagickFormat() pins EPS, so on a build that cannot decode through + # that coder this provider cannot produce a preview at all. Unguarded, that is a + # failure rather than a skip - previously ImageMagick's own sniffing hid the + # dependency. Registration alone does not answer it: coders/ps.c registers EPS + # whether or not Ghostscript is there, so the guard probes the fixture instead. + $fileName = 'testimage.eps'; + $fixture = \OC::$SERVERROOT . '/tests/data/' . $fileName; + $this->requireDecodableFixtureFile('EPS', $fixture); parent::setUp(); - $fileName = 'testimage.eps'; - $this->imgPath = $this->prepareTestFile($fileName, \OC::$SERVERROOT . '/tests/data/' . $fileName); + $this->imgPath = $this->prepareTestFile($fileName, $fixture); $this->width = 2400; $this->height = 1707; $this->provider = new \OC\Preview\Postscript; diff --git a/tests/lib/Preview/PDFTest.php b/tests/lib/Preview/PDFTest.php index ee0c84842048..30e1d9ca8244 100644 --- a/tests/lib/Preview/PDFTest.php +++ b/tests/lib/Preview/PDFTest.php @@ -40,14 +40,14 @@ public function setUp(): void { # 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'); - } + # PDF provider present" while the PDF coder was in fact present. A registration + # check is not the right replacement either: see requireDecodableFixtureFile(). + $fileName = 'testimage.pdf'; + $fixture = \OC::$SERVERROOT . '/tests/data/' . $fileName; + $this->requireDecodableFixtureFile('PDF', $fixture); parent::setUp(); - $fileName = 'testimage.pdf'; - $this->imgPath = $this->prepareTestFile($fileName, \OC::$SERVERROOT . '/tests/data/' . $fileName); + $this->imgPath = $this->prepareTestFile($fileName, $fixture); $this->width = 595; $this->height = 842; $this->provider = new PDF(); diff --git a/tests/lib/Preview/Provider.php b/tests/lib/Preview/Provider.php index 3dfeecf1eca2..4bba73a80103 100644 --- a/tests/lib/Preview/Provider.php +++ b/tests/lib/Preview/Provider.php @@ -21,6 +21,7 @@ namespace Test\Preview; +use OC\Image\ImagickFactory; use OC\Preview\TXT; use OCP\Files\File; use OCP\Files\Node; @@ -71,6 +72,49 @@ protected function tearDown(): void { parent::tearDown(); } + /** + * Skips unless this ImageMagick build can actually decode the file at $fixturePath + * through $coder - the coder the provider under test pins. + * + * Named for the *File* it takes, because Test\Preview\CoderPinningTest has a helper of + * the same purpose that takes the content blob instead. Passing a path where a blob is + * expected would make readImageBlob() throw, which a probe like this one converts into + * a skip - so the mistake would silently stop a test running rather than fail. + * + * Imagick::queryFormats() is not enough on its own: it reports only whether a coder is + * *registered*, and coders/pdf.c and coders/ps.c register PDF, AI and EPS + * unconditionally, wiring the Ghostscript delegate behind them separately. + * MagickQueryFormats() does not consult policy.xml either, and the stock Debian and + * Ubuntu policy denies the PDF/PS/EPS/XPS coders outright. So on a build with no + * Ghostscript, or under that policy, a registration check still answers "present" and + * the calling test fails where it should skip. Probing the fixture covers both. + * + * The probe reads unpinned, which is the very thing the pin exists to prevent. That is + * fine: it is only ever a capability probe, never an assertion. + */ + protected function requireDecodableFixtureFile(string $coder, string $fixturePath): void { + if (\count(\Imagick::queryFormats($coder)) === 0) { + $this->markTestSkipped("This ImageMagick build registers no $coder coder"); + } + + # read outside the try: an unreadable fixture is a broken test, not a build + # limitation, and must not be converted into a skip + $content = \file_get_contents($fixturePath); + $this->assertNotFalse($content, "fixture $fixturePath must be readable"); + + try { + $probe = ImagickFactory::create(); + $probe->readImageBlob($content); + $probe->clear(); + } catch (\Throwable $e) { + # \Throwable rather than \ImagickException: imagick reports some delegate and + # policy conditions at warning severity, and phpunit-autotest.xml sets + # failOnWarning="true", so those reach us as PHPUnit\Framework\Error\Warning + # instead. Both mean the same thing for a capability probe. + $this->markTestSkipped("This ImageMagick build cannot decode the $coder fixture: " . $e->getMessage()); + } + } + public static function dimensionsDataProvider() { return [ [-\random_int(5, 100), -\random_int(5, 100)], diff --git a/tests/lib/Preview/SanitizeTest.php b/tests/lib/Preview/SanitizeTest.php index 5defde72527b..fb6065429b2b 100644 --- a/tests/lib/Preview/SanitizeTest.php +++ b/tests/lib/Preview/SanitizeTest.php @@ -25,6 +25,7 @@ use OC\Preview\Bitmap; use OC\Preview\Font; use OC\Preview\PDF; +use OC\Preview\Postscript; use OCP\Files\File; use Test\TestCase; @@ -35,17 +36,19 @@ class SanitizeTest extends TestCase { /** * @dataProvider providesSVG */ - public function test(string $svgContent, Bitmap $provider, string $mimeType): void { + public function test(string $content, 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. + $this->assertPayloadIsDeniedByTheMimeGate($content); + # mock it all .... $stream = fopen('php://memory', 'rb+'); - fwrite($stream, $svgContent); + fwrite($stream, $content); rewind($stream); $file = $this->createMock(File::class); - $file->method('getContent')->willReturn($svgContent); + $file->method('getContent')->willReturn($content); $file->method('fopen')->willReturn($stream); $file->method('getMimeType')->willReturn($mimeType); @@ -55,6 +58,30 @@ public function test(string $svgContent, Bitmap $provider, string $mimeType): vo $this->assertFalse($return); } + /** + * Control assertion, mirroring Bitmap::isDangerousToDecode()'s deny-list. + * + * That gate keys on the *sniffed* type, and libmagic's answer varies by build - the + * same SVG is image/svg+xml on PHP 8.3 and image/svg on 7.4. A build that classified + * one of these payloads as something the deny-list misses would send it to the coder + * instead, where the pin would very likely reject it anyway and assertFalse() below + * would still pass: the case would go quiet rather than fail. Assert the precondition + * so such a build reports an actionable failure. + */ + private function assertPayloadIsDeniedByTheMimeGate(string $content): void { + $detected = \OC::$server->getMimeTypeDetector()->detectString($content); + $type = \strtolower(\trim(\explode(';', $detected, 2)[0])); + + $denied = \strpos($type, 'text/') === 0 + || \strpos($type, 'image/svg') === 0 + || \in_array($type, ['application/xml', 'image/x-mvg'], true); + + $this->assertTrue( + $denied, + 'payload must be one isDangerousToDecode() denies, or this case proves nothing - libmagic here says: ' . $detected + ); + } + public function providesSVG(): Generator { $embeddedImagePath = __DIR__ . '/../../data/testimage.jpg'; $svgContent0 = << SVG; + # The payload below is the one the PDF and Postscript cases need, because those two + # providers pin a Ghostscript-backed coder and so are the only ones whose pin does + # NOT reject foreign content - for every other provider the pin is a second line of + # defence that makes the assertion hold with or without the mime gate. PostScript + # with its %!PS-Adobe header sniffs as application/postscript, which the gate must + # let through (Preview\Postscript and Preview\PDF have to decode real ones), so it + # cannot serve here. Drop the header and libmagic reports text/plain - denied by the + # gate - while an affirmed PDF:/EPS: pin still hands it straight to Ghostscript, + # which renders it at 595x842 / 612x792. That combination is what makes these cases + # fail if the gate is ever removed, instead of passing on the pin alone. + $headerlessPostScript = "newpath 10 10 moveto 50 50 lineto 4 setlinewidth stroke showpage\n"; + # all Bitmap based providers use the same thumbnailing logic - two is enough .... yield 'PDF provider - image tag' => [$svgContent0, new PDF(), 'application/pdf']; yield 'Font Provider - image tag' => [$svgContent0, new Font(), 'application/font-sfnt']; @@ -91,5 +130,9 @@ public function providesSVG(): Generator { 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']; + yield 'PDF provider - headerless PostScript' => [$headerlessPostScript, new PDF(), 'application/pdf']; + yield 'Postscript provider - headerless PostScript' => [ + $headerlessPostScript, new Postscript(), 'application/postscript' + ]; } } From bd82b79bce97cfb238afeb85f658188830378b56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 23 Sep 2026 13:58:04 +0200 Subject: [PATCH 07/18] test: give the Font coder-pin case an observable it can actually fail on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit testFontNeverInvokesADangerousCoderForForeignContent() asserted that the returned preview stayed under 2048 bytes, meaning to catch the PostScript payload coming back rendered by Ghostscript. It could not: OC_Image::data() re-encodes through GD, and the image has already been downscaled to fit 32x32 by then, so both the safe and the unsafe outcome land within a few hundred bytes of each other. Removing the coder pin left the case green. The thumbnail's shape does distinguish them. The payload now declares a portrait bounding box, ImageMagick's TTF coder draws a fixed 800x480 specimen sheet, and on owncloudci/php:8.3 that is 32x19 through the TTF pin against 25x32 with the pin removed. Verified both directions: green as shipped, and failing with the pin removed, which takes CoderPinningTest's detectors from four to five. The page stays blank - the bounding box is the whole difference from FOREIGN_POSTSCRIPT, since nothing here inspects a pixel. assertFalse() on the result would not work either: the placeholder FreeType returns for non-font bytes is a valid image on this build, so it would fail against a correctly pinned decode. Kept as one branch-free expression, because failOnRisky turns a zero-assertion test into a hard failure on any build that refuses the bytes. The case now also requires that this build can rasterize PostScript at all. Without Ghostscript, or under the stock Debian policy denying the PS coder, the pin-removed mutant throws instead of rendering, getThumbnail() returns false and the assertion would hold with no pin in place - green while protecting nothing. Both capability probes here and on Provider catch \Exception rather than \ImagickException. imagick reports some delegate and policy conditions at warning severity, and PHPUnit 9 turns PHP warnings into PHPUnit\Framework\Error\Warning through convertWarningsToExceptions, which defaults to true and is unset in tests/phpunit-autotest.xml; failOnWarning only decides whether an emitted warning fails the run. That class reaches \Exception via PHPUnit\Framework\Exception, so one catch covers both - and unlike \Throwable it still lets an \Error fail rather than become a green skip. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/CoderPinningTest.php | 76 +++++++++++++++++++++----- tests/lib/Preview/Provider.php | 13 +++-- 2 files changed, 71 insertions(+), 18 deletions(-) diff --git a/tests/lib/Preview/CoderPinningTest.php b/tests/lib/Preview/CoderPinningTest.php index 4b47bab46ffc..5e7497baabc1 100644 --- a/tests/lib/Preview/CoderPinningTest.php +++ b/tests/lib/Preview/CoderPinningTest.php @@ -40,12 +40,23 @@ */ 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. + * The payload testRejectsPostScriptContentFromAForeignProvider() feeds 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. A square box is + * fine there, because that test's observable is assertFalse(), which any successful + * decode trips whatever shape it produced. */ private const FOREIGN_POSTSCRIPT = "%!PS-Adobe-3.0\n%%BoundingBox: 0 0 10 10\nshowpage\n"; + /** + * The same payload with a *portrait* bounding box. That is the only difference, and it + * is the one testFontNeverInvokesADangerousCoderForForeignContent() rests on: its + * observable is the thumbnail's shape, and the square box above thumbnails square + * whichever coder produced it. The page stays blank on purpose - nothing here inspects + * a pixel, so drawing operators would only suggest the assertion is stronger than it is. + */ + private const FOREIGN_POSTSCRIPT_PORTRAIT = "%!PS-Adobe-3.0\n%%BoundingBox: 0 0 600 800\nshowpage\n"; + private function makeFile(string $content, string $mimeType): File { $stream = \fopen('php://memory', 'rb+'); \fwrite($stream, $content); @@ -83,11 +94,33 @@ private function requireDecodableFixture(string $coder, string $content): void { $probe = ImagickFactory::create(); $probe->readImageBlob($content); $probe->clear(); - } catch (\ImagickException $e) { + } catch (\Exception $e) { + # \Exception, not \ImagickException: imagick reports some delegate and policy + # conditions at warning severity, and PHPUnit 9 converts PHP warnings into + # PHPUnit\Framework\Error\Warning by default (convertWarningsToExceptions, + # unset in tests/phpunit-autotest.xml and defaulting to true - failOnWarning + # only decides whether an emitted warning fails the run). That class extends + # \Exception via PHPUnit\Framework\Exception, so one catch covers both, and + # an \Error still surfaces rather than being turned into a green skip. $this->markTestSkipped("This ImageMagick build cannot decode the $coder fixture: " . $e->getMessage()); } } + /** + * Skips unless this build can rasterize PostScript, which needs the PS coder to be both + * registered and permitted by policy.xml plus a working Ghostscript delegate. Reads + * unpinned, as every probe here does - a capability check, never an assertion. + */ + private function requireRenderablePostScript(string $content): void { + try { + $probe = ImagickFactory::create(); + $probe->readImageBlob($content); + $probe->clear(); + } catch (\Exception $e) { + $this->markTestSkipped('This build cannot rasterize PostScript: ' . $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 @@ -180,19 +213,36 @@ public function providesForeignProviders(): Generator { public function testFontNeverInvokesADangerousCoderForForeignContent(): void { $this->requireCoder('TTF'); - $this->assertPayloadReachesTheCoderPin(self::FOREIGN_POSTSCRIPT); - $file = $this->makeFile(self::FOREIGN_POSTSCRIPT, 'application/font-sfnt'); + # The mutant this test is meant to catch - the pin removed - only produces a portrait + # render on a build that can rasterize PostScript at all. Without Ghostscript, or + # under the stock Debian policy that denies the PS coder, readImageBlob() would throw + # instead, getThumbnail() would return false, and the assertion below would hold with + # no pin in place. Skip rather than report protection this build is not providing. + $this->requireRenderablePostScript(self::FOREIGN_POSTSCRIPT_PORTRAIT); + $this->assertPayloadReachesTheCoderPin(self::FOREIGN_POSTSCRIPT_PORTRAIT); + $file = $this->makeFile(self::FOREIGN_POSTSCRIPT_PORTRAIT, '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'); + # a placeholder, never by invoking Ghostscript or a script coder - both are safe + # outcomes. What must never happen is the PostScript page itself coming back + # rendered. The payload's bounding box is portrait (600x800) while ImageMagick's TTF + # coder draws a fixed 800x480 specimen sheet, so the thumbnail's shape is what + # separates them: measured on owncloudci/php:8.3, 32x19 through the TTF pin against + # 25x32 with the pin removed. + # + # Size cannot be used for this. OC_Image::data() re-encodes through GD, and by then + # the image is already downscaled to fit 32x32, so both outcomes land within a few + # hundred bytes of each other - an earlier revision of this test asserted a 2048 + # byte ceiling and could not fail. Nor can assertFalse(): the placeholder is a + # valid image on this build, so that would fail where the pin is working. + # + # One branch-free expression on purpose - branching would leave the test + # assertion-less on a build that returns false, and failOnRisky in + # tests/phpunit-autotest.xml makes a zero-assertion test a hard failure. + $renderedThePortraitPage = $result !== false && $result->height() > $result->width(); + $this->assertFalse($renderedThePortraitPage, 'Font must not render PostScript content'); } /** diff --git a/tests/lib/Preview/Provider.php b/tests/lib/Preview/Provider.php index 4bba73a80103..056fbf7f81e8 100644 --- a/tests/lib/Preview/Provider.php +++ b/tests/lib/Preview/Provider.php @@ -106,11 +106,14 @@ protected function requireDecodableFixtureFile(string $coder, string $fixturePat $probe = ImagickFactory::create(); $probe->readImageBlob($content); $probe->clear(); - } catch (\Throwable $e) { - # \Throwable rather than \ImagickException: imagick reports some delegate and - # policy conditions at warning severity, and phpunit-autotest.xml sets - # failOnWarning="true", so those reach us as PHPUnit\Framework\Error\Warning - # instead. Both mean the same thing for a capability probe. + } catch (\Exception $e) { + # \Exception rather than \ImagickException: imagick reports some delegate and + # policy conditions at warning severity, and PHPUnit 9 converts PHP warnings + # into PHPUnit\Framework\Error\Warning by default (convertWarningsToExceptions, + # which phpunit-autotest.xml leaves unset; failOnWarning only decides whether an + # emitted warning fails the run). That class reaches \Exception via + # PHPUnit\Framework\Exception, so one catch covers both - and unlike \Throwable + # it still lets an \Error fail instead of becoming a green skip. $this->markTestSkipped("This ImageMagick build cannot decode the $coder fixture: " . $e->getMessage()); } } From 3aeccc962815ec84b05c863e687c90037b4c96b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 23 Sep 2026 15:11:47 +0200 Subject: [PATCH 08/18] test: make the PostScript coder-pin cases fail when the pin is gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three ways these cases could stay green with Bitmap::getResizedPreview()'s setFormat() pin removed. testFontNeverInvokesADangerousCoderForForeignContent() asserted the preview came back under 2048 bytes. OC_Image::data() re-encodes through GD after the image has been downscaled to fit 32x32, so the safe and unsafe outcomes both land within a few hundred bytes and the ceiling could never be crossed. Shape does separate them: ImageMagick's TTF coder draws a fixed 800x480 specimen sheet, landscape, while an unpinned read goes to the PS coder and rasterizes a full page, portrait - 32x19 against 25x32 on owncloudci/php:8.3. assertFalse() would not work either, since the placeholder FreeType returns for non-font bytes is a valid image here. That shape is a property of the build, not of the payload: measured, this build's PS coder ignores %%BoundingBox, EPSF-3.0 branding and setpagedevice alike and always renders 612x792. So requirePortraitPostScriptRender() now asserts it, and does so after reproducing the first-frame selection and bestfit downscale the assertion actually observes - a raster only slightly taller than wide collapses to exactly 32x32 there, which would have waved through a build that cannot discriminate. The payload keeps a portrait bounding box even so, because upstream coders/ps.c does derive Ghostscript's -g geometry from it on other builds, and a portrait box is portrait under both behaviours where a square one would silently stop exercising the pin. testRejectsPostScriptContentFromAForeignProvider() had no PostScript precondition at all. Under a policy.xml denying PS/EPS/PDF the unpinned read throws just as the pinned one does, so all four rows held with no pin in place. They now require a renderable payload first, via a plain capability helper the portrait one builds on; with the pin removed and PS denied the class skips 8 instead of falsely passing 4. FOREIGN_POSTSCRIPT_PORTRAIT is gone - one constant serves both tests again. Verified on owncloudci/php:8.3: 19 pass as shipped; 5 fail with the pin removed; 8 skip and none pass falsely with the pin removed and PS denied. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/CoderPinningTest.php | 105 +++++++++++++++++-------- 1 file changed, 73 insertions(+), 32 deletions(-) diff --git a/tests/lib/Preview/CoderPinningTest.php b/tests/lib/Preview/CoderPinningTest.php index 5e7497baabc1..4ae3c2673bca 100644 --- a/tests/lib/Preview/CoderPinningTest.php +++ b/tests/lib/Preview/CoderPinningTest.php @@ -40,22 +40,22 @@ */ class CoderPinningTest extends TestCase { /** - * The payload testRejectsPostScriptContentFromAForeignProvider() feeds 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. A square box is - * fine there, because that test's observable is assertFalse(), which any successful - * decode trips whatever shape it produced. - */ - private const FOREIGN_POSTSCRIPT = "%!PS-Adobe-3.0\n%%BoundingBox: 0 0 10 10\nshowpage\n"; - - /** - * The same payload with a *portrait* bounding box. That is the only difference, and it - * is the one testFontNeverInvokesADangerousCoderForForeignContent() rests on: its - * observable is the thumbnail's shape, and the square box above thumbnails square - * whichever coder produced it. The page stays blank on purpose - nothing here inspects - * a pixel, so drawing operators would only suggest the assertion is stronger than it is. + * 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. + * + * The bounding box is portrait so that it stays portrait however this build treats it. + * testFontNeverInvokesADangerousCoderForForeignContent() tells a Ghostscript render from + * FreeType's output by shape, and the two known behaviours both give a portrait raster: + * on owncloudci/php:8.3 an unpinned %!PS-Adobe read goes to the PS coder, which + * rasterizes a whole page at Ghostscript's default 612x792 and ignores %%BoundingBox, + * EPSF branding and setpagedevice alike (all three measured); where coders/ps.c instead + * derives Ghostscript's -g geometry from %%BoundingBox, this box yields 600x800. A square + * box would be portrait only under the first, so it would silently stop exercising the + * Font pin under the second. That test still asserts the shape at runtime rather than + * trusting either behaviour. */ - private const FOREIGN_POSTSCRIPT_PORTRAIT = "%!PS-Adobe-3.0\n%%BoundingBox: 0 0 600 800\nshowpage\n"; + private const FOREIGN_POSTSCRIPT = "%!PS-Adobe-3.0\n%%BoundingBox: 0 0 600 800\nshowpage\n"; private function makeFile(string $content, string $mimeType): File { $stream = \fopen('php://memory', 'rb+'); @@ -107,20 +107,57 @@ private function requireDecodableFixture(string $coder, string $content): void { } /** - * Skips unless this build can rasterize PostScript, which needs the PS coder to be both - * registered and permitted by policy.xml plus a working Ghostscript delegate. Reads - * unpinned, as every probe here does - a capability check, never an assertion. + * Skips unless this build can rasterize PostScript at all - the PS coder registered, + * permitted by policy.xml, with a working Ghostscript delegate behind it. Callers that + * also depend on the render's *shape* want requirePortraitPostScriptRender() instead. + * + * Returns the wand so a caller can measure it; clear() is the caller's to make. + * + * Reads unpinned, as every probe here does - a capability check, never an assertion. */ - private function requireRenderablePostScript(string $content): void { + private function requireRenderablePostScript(string $content): \Imagick { try { $probe = ImagickFactory::create(); $probe->readImageBlob($content); - $probe->clear(); + return $probe; } catch (\Exception $e) { $this->markTestSkipped('This build cannot rasterize PostScript: ' . $e->getMessage()); } } + /** + * As requireRenderablePostScript(), and additionally that the render comes out portrait. + * + * testFontNeverInvokesADangerousCoderForForeignContent() tells a Ghostscript render from + * FreeType's output by shape alone, and portrait-ness is not a given - it follows from + * Ghostscript's default page, which no part of the payload reliably pins (see + * FOREIGN_POSTSCRIPT). Asserting it means a build with a landscape default page skips + * instead of passing with the coder pin removed. + * + * Measured after reproducing what Bitmap::getResizedPreview() does before its assertion + * is observable - select the first frame, then bestfit to 32x32. A raster only a little + * taller than it is wide collapses to exactly 32x32 there, so checking the raw geometry + * would wave through a build on which the assertion cannot discriminate. + */ + private function requirePortraitPostScriptRender(string $content): void { + $probe = $this->requireRenderablePostScript($content); + + $probe->setIteratorIndex(0); + if ($probe->getImageWidth() > 32 || $probe->getImageHeight() > 32) { + $probe->resizeImage(32, 32, \Imagick::FILTER_LANCZOS, 1, true); + } + $isPortrait = $probe->getImageHeight() > $probe->getImageWidth(); + $geometry = $probe->getImageWidth() . 'x' . $probe->getImageHeight(); + $probe->clear(); + + if (!$isPortrait) { + $this->markTestSkipped( + "This build's PostScript render thumbnails to $geometry, so shape cannot tell " + . 'a Ghostscript render from the TTF specimen sheet' + ); + } + } + /** * 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 @@ -196,6 +233,9 @@ public function providesLegitimateContent(): Generator { */ public function testRejectsPostScriptContentFromAForeignProvider(Bitmap $provider, string $mimeType, string $coder): void { $this->requireCoder($coder); + # Without a usable PostScript path the mutant this guards against - the pin removed - + # cannot decode the payload either, so assertFalse() would hold with no pin in place. + $this->requireRenderablePostScript(self::FOREIGN_POSTSCRIPT)->clear(); $this->assertPayloadReachesTheCoderPin(self::FOREIGN_POSTSCRIPT); $file = $this->makeFile(self::FOREIGN_POSTSCRIPT, $mimeType); @@ -213,24 +253,25 @@ public function providesForeignProviders(): Generator { public function testFontNeverInvokesADangerousCoderForForeignContent(): void { $this->requireCoder('TTF'); - # The mutant this test is meant to catch - the pin removed - only produces a portrait - # render on a build that can rasterize PostScript at all. Without Ghostscript, or - # under the stock Debian policy that denies the PS coder, readImageBlob() would throw - # instead, getThumbnail() would return false, and the assertion below would hold with - # no pin in place. Skip rather than report protection this build is not providing. - $this->requireRenderablePostScript(self::FOREIGN_POSTSCRIPT_PORTRAIT); - $this->assertPayloadReachesTheCoderPin(self::FOREIGN_POSTSCRIPT_PORTRAIT); - $file = $this->makeFile(self::FOREIGN_POSTSCRIPT_PORTRAIT, 'application/font-sfnt'); + # Both preconditions the assertion below rests on, asserted rather than assumed: that + # the pin-removed mutant would rasterize this payload at all, and that it would come + # out portrait. Without Ghostscript, or under the stock Debian policy denying the PS + # coder, readImageBlob() throws, getThumbnail() returns false and the assertion would + # hold with no pin in place - green while protecting nothing. + $this->requirePortraitPostScriptRender(self::FOREIGN_POSTSCRIPT); + $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 placeholder, never by invoking Ghostscript or a script coder - both are safe # outcomes. What must never happen is the PostScript page itself coming back - # rendered. The payload's bounding box is portrait (600x800) while ImageMagick's TTF - # coder draws a fixed 800x480 specimen sheet, so the thumbnail's shape is what - # separates them: measured on owncloudci/php:8.3, 32x19 through the TTF pin against - # 25x32 with the pin removed. + # rendered. Shape is what separates the two: ImageMagick's TTF coder draws a fixed + # 800x480 specimen sheet, landscape, while the PS coder rasterizes a full page at + # Ghostscript's default size, portrait. Measured on owncloudci/php:8.3, 32x19 through + # the TTF pin against 25x32 with the pin removed - 25x32 being 612x792 scaled, i.e. + # the default page, NOT anything this payload asked for. # # Size cannot be used for this. OC_Image::data() re-encodes through GD, and by then # the image is already downscaled to fit 32x32, so both outcomes land within a few From 5e58cf46a06324039f4f2e68474cd7224d99d45d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 23 Sep 2026 15:11:59 +0200 Subject: [PATCH 09/18] docs: state which extensions the Imagick coder pin actually affects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The entry named tif, psd, sgi, heic and ai - the most visible subset of the trade-off rather than its extent. The list now comes from resources/config/mimetypemapping.dist.json instead of memory, so image/sgi contributes bw, int, inta, rgb and rgba alongside sgi, and image/heif contributes heif. svgz is not mapped there at all, so it is deliberately absent. It also says which of those providers a stock install actually has. Only SGI and Heic are in PreviewManager's defaults; PDF, Postscript, Illustrator, Photoshop, TIFF and Font need enabling, so without that note the list reads as a much wider regression than most admins will see. Fonts get their own paragraph rather than being listed or omitted. The font coder accepts any bytes, so a mismatched .ttf still yields a thumbnail - just one the font coder drew instead of the file's content - and a real .otf gains a preview it never had, since an unpinned read has no decode delegate for CFF outlines. Neither "affected" nor "unaffected" describes that honestly. Office and SVG stay excluded: the Office pin covers the PDF LibreOffice just produced rather than user bytes, and non-XML content never reached a coder in SVG before this change either. Also softened the "no filesystem access" claim to what this change is responsible for. The pin adds no temporary file of its own, but it does not stop the Ghostscript-backed coders staging their own input, and the original wording read as a promise about the whole preview path. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- changelog/unreleased/41834 | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/changelog/unreleased/41834 b/changelog/unreleased/41834 index ff82c6f13ada..a6d06ca3dc74 100644 --- a/changelog/unreleased/41834 +++ b/changelog/unreleased/41834 @@ -9,15 +9,30 @@ 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. +ImageMagick guess from the file's content. The pin is applied in memory and +introduces no temporary file of its own. 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. +it. This is the intended trade-off - content sniffing is what allowed a preview +provider to be steered to an unrelated coder in the first place. + +The affected extensions are ai, bw, eps, heic, heif, int, inta, pdf, ps, psd, +rgb, rgba, sgi, tif and tiff. Of those providers only SGI and Heic are registered +by default, so on a stock install this is visible for bw, int, inta, rgb, rgba, +sgi, heic and heif; the rest need their provider enabled in enabledPreviewProviders. + +The font extensions otf, pfb and ttf change differently: the font coder accepts +any bytes, so a mismatched file still produces a thumbnail, just one drawn by the +font coder rather than reflecting the file's real content. Real .otf files gain +previews they did not have before, because an unpinned read had no decode delegate +for them at all. + +Office documents and SVG are pinned too but are not affected. For Office the pin +covers the PDF LibreOffice has just produced rather than anything the user +uploaded, and for SVG content that is not parseable XML never reached a coder +before this change either. https://github.com/owncloud/core/pull/41834 From 96c9b26b0b5a16dcf9158559cb1ea5d97564332c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:07:49 +0200 Subject: [PATCH 10/18] fix: always report a media type when detecting from file content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Detection::detectString() is documented as returning a string, but returned finfo_buffer()'s value unchecked, which is typed string|false, and passed finfo_open()'s result to it without checking that either. Both matter to the bitmap preview media type gate added earlier in this pull request, which compares the detected type against a list it refuses to hand to ImageMagick: - finfo_open() returns false when libmagic cannot load its magic database. finfo_buffer() then raises a TypeError, and because that is an \Error rather than an \Exception it escapes the handler in Bitmap::getThumbnail(), so a preview failed the whole request with a server error instead of falling back to a media type icon. - a false from finfo_buffer() reached the gate as the empty string, which matches no entry in the list, so the content was admitted. Both now fall back to application/octet-stream. The finfo_open() warning is suppressed the same way finfo_file()'s already is a few lines above, since the fallback is what the caller acts on. The function_exists() guard also tested finfo_file(), which this branch never calls; it now tests finfo_buffer(). Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- lib/private/Files/Type/Detection.php | 22 +++++++++++++--- tests/lib/Files/Type/DetectionTest.php | 35 ++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/lib/private/Files/Type/Detection.php b/lib/private/Files/Type/Detection.php index 5992caebb3f7..6cea3a2cfdcd 100644 --- a/lib/private/Files/Type/Detection.php +++ b/lib/private/Files/Type/Detection.php @@ -262,9 +262,25 @@ public function detect($path) { * @return string */ public function detectString($data) { - if (\function_exists('finfo_open') and \function_exists('finfo_file')) { - $finfo = \finfo_open(FILEINFO_MIME); - return \finfo_buffer($finfo, $data); + if (\function_exists('finfo_open') and \function_exists('finfo_buffer')) { + // suppressed like finfo_file() below: libmagic warns when it cannot load its + // magic database - reachable through the MAGIC environment variable, or a + // broken install - and then returns false. Handing that false to + // finfo_buffer() is a TypeError, an \Error rather than an \Exception, so it + // escapes callers that guard against a failed detection: it would turn a + // missing bitmap preview into a 500 in OC\Preview\Bitmap::getThumbnail(). + $finfo = @\finfo_open(FILEINFO_MIME); + if ($finfo === false) { + return 'application/octet-stream'; + } + + // finfo_buffer() is typed string|false. An unusable return has to become the + // fallback rather than reach a caller, because this method is documented as + // returning a string and OC\Preview\Bitmap compares the result against a + // deny-list of media types it refuses to decode - '' matches no entry there + // and would admit the very content the list exists to reject. + $mimeType = \finfo_buffer($finfo, $data); + return \is_string($mimeType) && $mimeType !== '' ? $mimeType : 'application/octet-stream'; } else { $tmpFile = \OC::$server->getTempManager()->getTemporaryFile(); $fh = \fopen($tmpFile, 'wb'); diff --git a/tests/lib/Files/Type/DetectionTest.php b/tests/lib/Files/Type/DetectionTest.php index b0671c055139..ef93ed8153e3 100644 --- a/tests/lib/Files/Type/DetectionTest.php +++ b/tests/lib/Files/Type/DetectionTest.php @@ -96,6 +96,41 @@ public function testDetectString(): void { $this->assertEquals($expected, $result); } + /** + * detectString() is documented as returning a string, and OC\Preview\Bitmap's + * media-type deny-list depends on that: it compares the return value against a + * list of types it refuses to hand to ImageMagick, so a non-string return would + * match nothing and admit the content. + * + * libmagic reads the MAGIC environment variable when finfo_open() is called, so + * pointing it at a file that is not a magic database makes finfo_open() return + * false - the one condition under which finfo_buffer() used to be handed a bool + * and raise a TypeError. Being an \Error rather than an \Exception, that escaped + * the handler in Bitmap::getThumbnail() and turned a missing preview into a 500. + */ + public function testDetectStringWithUnusableMagicDatabase(): void { + $previous = \getenv('MAGIC'); + \putenv('MAGIC=' . \OC::$SERVERROOT . '/tests/data/no-such-magic-database'); + + try { + # Precondition, not decoration: if this build ignores MAGIC and opens its + # compiled-in database anyway, the case below proves nothing at all and has + # to report that rather than pass quietly. + $this->assertFalse( + @\finfo_open(FILEINFO_MIME), + 'MAGIC must make finfo_open() fail, or this case exercises the normal path' + ); + + $this->assertSame('application/octet-stream', $this->detection->detectString('anything')); + } finally { + if ($previous === false) { + \putenv('MAGIC'); + } else { + \putenv('MAGIC=' . $previous); + } + } + } + public function testMimeTypeIcon(): void { $confDir = vfsStream::setup(); $mimetypealiases_dist = vfsStream::newFile('mimetypealiases.dist.json')->at($confDir); From fdb06e53fd76e4756dd80792de3ab0e00cbc85c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:08:07 +0200 Subject: [PATCH 11/18] fix: apply the hardened Imagick options before decoding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ImagickFactory::create() passed its argument to the Imagick constructor, which reads the image immediately, and only then set svg:sanitize, svg:embed and svg:decode. Anything loaded that way was therefore decoded with none of the hardening the options exist to provide - so the one form that takes a path was the only unhardened way through the factory, while the comment added to Office::getThumbnail() earlier in this pull request presents it as the hardened route. The instance is now always constructed empty, the options applied, and the file read afterwards. Verified in a container that this is behaviour preserving for the only caller that passes a path: Office's "PDF:[0]" produces identical geometry and identical output bytes either way, a foreign image pinned to the PDF coder is still rejected, and readImage() does not pin the wand's output format the way setFormat() does - so setImageFormat() alone remains sufficient there. The parameter is narrowed from mixed to ?string at the same time. Imagick's constructor also accepts an array of paths, but no caller has ever passed one, and carrying that form would mean a second, untested branch. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- lib/private/Image/ImagickFactory.php | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/lib/private/Image/ImagickFactory.php b/lib/private/Image/ImagickFactory.php index 704366180ec6..e703e859d808 100644 --- a/lib/private/Image/ImagickFactory.php +++ b/lib/private/Image/ImagickFactory.php @@ -7,18 +7,32 @@ class ImagickFactory { /** - * @param mixed $files

- * The path to an image to load or an array of paths. Paths can include - * wildcards for file names, or can be URLs. - *

+ * Returns an Imagick instance with the hardened SVG options already applied. + * + * @param string|null $file path of an image to read, optionally prefixed with an + * explicit "FORMAT:" to pin the input coder. Left null, nothing is read and the + * caller decodes itself. * @return Imagick * @throws ImagickException */ - public static function create($files = null): Imagick { - $imagick = new Imagick($files); + public static function create(?string $file = null): Imagick { + // Deliberately not "new Imagick($file)": the constructor reads immediately, so the + // options below would only be applied to an already decoded image. That is exactly + // what the path-taking form used to do, leaving it the one unhardened way in. + $imagick = new Imagick(); $imagick->setOption('svg:sanitize', 'true'); $imagick->setOption('svg:embed', 'false'); $imagick->setOption('svg:decode', 'true'); + + if ($file !== null) { + // A "FORMAT:path" argument pins the input coder here just as it does in the + // constructor, and - unlike setFormat() - it does not pin the wand's output + // format as well, so callers still only need setImageFormat(). Verified + // against Office's "PDF:[0]": identical geometry and bytes to the + // constructor form, and a foreign image pinned to PDF is still rejected. + $imagick->readImage($file); + } + return $imagick; } From 2547d41afd0acec2ffda1b8c83569c75d4566f15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:08:07 +0200 Subject: [PATCH 12/18] docs: record what the bitmap preview media type gate depends on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate can only be as good as the detection behind it, and that needs either ext-fileinfo or the "file" binary. Neither is actually required to run ownCloud: OC_Util::checkServer() does not list ext-fileinfo among its hard dependencies, and OC_Util::fileInfoLoaded() only raises a recommendation in the admin panel. On an install with neither, every payload is reported as application/octet-stream and the gate admits all of them, leaving the per-provider coder pin as the only remaining layer. Recorded rather than closed: failing closed here would cost every bitmap preview on a configuration ownCloud supports, and the condition pre-dates this check. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- lib/private/Preview/Bitmap.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/private/Preview/Bitmap.php b/lib/private/Preview/Bitmap.php index b0354281c64a..7f6c463be55e 100644 --- a/lib/private/Preview/Bitmap.php +++ b/lib/private/Preview/Bitmap.php @@ -163,6 +163,17 @@ 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 * text/vector coders can be abused to read and write arbitrary files. + * + * Known limitation, pre-dating this check and deliberately not closed here: the + * deny-list can only be as good as the detection behind it. Detection::detectString() + * needs either ext-fileinfo or the "file" binary, and ext-fileinfo is not actually + * required to run ownCloud - OC_Util::checkServer() does not list it, and + * OC_Util::fileInfoLoaded() only raises an admin-panel recommendation. On an install + * with neither, every payload is reported as application/octet-stream and this method + * returns false for all of them, leaving the per-provider coder pin in + * getResizedPreview() as the only remaining layer. Failing closed instead would cost + * every bitmap preview on a configuration ownCloud supports, which is why it is + * recorded here rather than rejected. */ private function isDangerousToDecode(string $content): bool { $mimeType = \OC::$server->getMimeTypeDetector()->detectString($content); From b2ca901973343811dd2b5fc099cd4a724d21f12c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:08:08 +0200 Subject: [PATCH 13/18] test: stop requiring imagick for the preview gate regression cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SanitizeTest carried a class level "@requires extension imagick" that contradicts the comment directly below it, which argues against guarding these cases so that a reduced build cannot silently skip the regression assertions. No case in the class reaches Imagick at all: the media type gate rejects every payload before ImagickFactory::create() is called. All ten cases still run and pass without it. Also drops TestCase::assertImage() and tests/lib/Preview/white-32x32.png. Both arrived together with an earlier change, and their only caller was the SanitizeTest assertion this pull request replaced, so nothing references either any more. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/SanitizeTest.php | 3 --- tests/lib/Preview/white-32x32.png | Bin 105 -> 0 bytes tests/lib/TestCase.php | 16 ---------------- 3 files changed, 19 deletions(-) delete mode 100644 tests/lib/Preview/white-32x32.png diff --git a/tests/lib/Preview/SanitizeTest.php b/tests/lib/Preview/SanitizeTest.php index fb6065429b2b..c4e18cec0fb6 100644 --- a/tests/lib/Preview/SanitizeTest.php +++ b/tests/lib/Preview/SanitizeTest.php @@ -29,9 +29,6 @@ use OCP\Files\File; use Test\TestCase; -/** - * @requires extension imagick - */ class SanitizeTest extends TestCase { /** * @dataProvider providesSVG diff --git a/tests/lib/Preview/white-32x32.png b/tests/lib/Preview/white-32x32.png deleted file mode 100644 index 35f7df1145212d938fc4a3f30ab64537b32add5c..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 105 zcmeAS@N?(olHy`uVBq!ia0vp^3LwnL3?x0byx0z;m;-!5T>t<7zx;^w2_ToVz$3Dl wfr0M`2s2LA=92~r@_4#9hH%VGPLQ|*putContent($fileContent); return $file; } - - public function assertImage(string $expectedImagePath, \OC_Image $actualImage): void { - # GD to Imagick conversion - ob_start(); - imagepng($actualImage->resource()); - $actualImageBlob = ob_get_clean(); - - # Compare images - $img1 = new \Imagick($expectedImagePath); - $img2 = new \Imagick(); - $img2->readImageBlob($actualImageBlob); - - $result = $img1->compareImages($img2, \Imagick::METRIC_MEANSQUAREERROR); - - $this->assertLessThan(0.001, $result[1]); - } } From e8c3c50883f1fd73c69c45afdc3ff39453909f78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:08:08 +0200 Subject: [PATCH 14/18] docs: changelog for the media type detection and coder pin follow-ups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records the two preview hardening fixes from this review round, and states the residual the coder pin deliberately leaves open: which provider serves a preview is steerable by the request, so a file can still be routed to the PDF coder whatever its content. That is not a change in behaviour - content sniffing reached the same coder before the pin - and a distribution's ImageMagick policy is the process-wide control for it. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- changelog/unreleased/41827 | 9 +++++++++ changelog/unreleased/41834 | 8 ++++++++ 2 files changed, 17 insertions(+) diff --git a/changelog/unreleased/41827 b/changelog/unreleased/41827 index ed593a0087d3..70df19242aeb 100644 --- a/changelog/unreleased/41827 +++ b/changelog/unreleased/41827 @@ -12,4 +12,13 @@ any content that is detected as text, XML, SVG, or MVG before ImageMagick ever sees it, and decode through the same hardened Imagick options already used by the dedicated SVG preview provider. +Media type detection from file content now always reports a media type. It +previously passed an unusable value on to its caller when the magic database +behind it could not be loaded, which made a bitmap preview fail the request +with a server error rather than fall back to a media type icon, and left the +new check above with nothing to test the content against. + +Previews that read a file also no longer pass it to ImageMagick before the +hardened Imagick options are applied. + https://github.com/owncloud/core/pull/41827 diff --git a/changelog/unreleased/41834 b/changelog/unreleased/41834 index a6d06ca3dc74..169c86d911f3 100644 --- a/changelog/unreleased/41834 +++ b/changelog/unreleased/41834 @@ -35,4 +35,12 @@ covers the PDF LibreOffice has just produced rather than anything the user uploaded, and for SVG content that is not parseable XML never reached a coder before this change either. +One route is deliberately left open, and is worth stating so the expectation is +set: which provider handles a preview can be steered by the request, so asking for +a file to be previewed as a PDF hands that file's bytes to the PDF coder whatever +they are. This is not a change - content sniffing reached the same coder before - +and the PDF, PostScript and EPS coders are the ones a distribution's ImageMagick +policy denies by default. Deployments that enable those coders should keep that +policy as the control, because it applies process-wide rather than per provider. + https://github.com/owncloud/core/pull/41834 From 1802f9f8b848dd42f9857d60f4310b093a68bb9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:24:14 +0200 Subject: [PATCH 15/18] fix: fall back to a media type when no temporary file can be written MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit detectString()'s branch for installs without ext-fileinfo writes the content to a temporary file so that detect() can inspect it. It did not check either step: TempManager::getTemporaryFile() returns false when its directory is not writable, and fopen(false, ...) raises a ValueError on PHP 8. Being an \Error rather than an \Exception, that escapes the handler in Bitmap::getThumbnail(), so the request failed with a server error instead of falling back to a media type icon. Nothing in core called detectString() before the media type gate added in this pull request, so this branch was not previously reachable from a request. It now runs on every bitmap preview on such an install, which is what makes it worth closing alongside the finfo_open() guard above it - the same failure mode, on the sibling path. Not covered by a test: reaching this branch needs a build without ext-fileinfo, which cannot be simulated from within the test suite. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- lib/private/Files/Type/Detection.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/private/Files/Type/Detection.php b/lib/private/Files/Type/Detection.php index 6cea3a2cfdcd..dfa155cc5131 100644 --- a/lib/private/Files/Type/Detection.php +++ b/lib/private/Files/Type/Detection.php @@ -282,8 +282,22 @@ public function detectString($data) { $mimeType = \finfo_buffer($finfo, $data); return \is_string($mimeType) && $mimeType !== '' ? $mimeType : 'application/octet-stream'; } else { + // This branch is only reached without ext-fileinfo, but it is now reached from + // a preview request, so the same reasoning as above applies: getTemporaryFile() + // returns false when its directory is not writable, and fopen(false, ...) is a + // ValueError on PHP 8 - an \Error, so it would escape the catch (\Exception) in + // OC\Preview\Bitmap::getThumbnail() and fail the request instead of falling + // back to a media type icon. $tmpFile = \OC::$server->getTempManager()->getTemporaryFile(); + if ($tmpFile === false) { + return 'application/octet-stream'; + } + $fh = \fopen($tmpFile, 'wb'); + if ($fh === false) { + return 'application/octet-stream'; + } + \fwrite($fh, $data, 8024); \fclose($fh); $mime = $this->detect($tmpFile); From 6cd99d456e13d2dfef2388218412e69a5dee5170 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:24:14 +0200 Subject: [PATCH 16/18] docs: justify the PostScript coder pin by measurement, not coder internals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment argued that pinning EPS covers .ps because EPS and PS share ReadPSImage(). They do share it, but not unconditionally - it branches on the requested coder - so the comment asserted an equivalence it did not establish. Replaced with what was actually measured on the shipped image: plain PostScript and EPSF-tagged content, both declaring a bounding box smaller than the page, render to identical geometry read unpinned, pinned EPS and pinned PS. The pin therefore does not change what a .ps file previews as, which is the property the comment needed to support. No functional change. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- lib/private/Preview/Postscript.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/private/Preview/Postscript.php b/lib/private/Preview/Postscript.php index e852b66ba16d..24d886742ea4 100644 --- a/lib/private/Preview/Postscript.php +++ b/lib/private/Preview/Postscript.php @@ -32,8 +32,11 @@ public function getMimeType() { } 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. + # EPS is the coder ImageMagick registers for application/postscript, and it covers + # .ps as well as .eps. Measured rather than inferred from the coder internals: + # plain PostScript and EPSF-tagged content declaring a bounding box smaller than + # the page both render to the same geometry read unpinned, pinned EPS and pinned + # PS, so the pin does not change what a .ps file previews as. return 'EPS'; } } From a20641b329e98aeb082678c196377e5727ed85b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:31:37 +0200 Subject: [PATCH 17/18] fix: do not let a disabled popen turn a preview into a server error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit detect() falls back to the "file" binary when nothing else identified the content, and OC_Helper::canExecute() only establishes that the binary exists - not that we are allowed to start a process. popen() is a common disable_functions entry, and on PHP 8 a disabled function is undefined, so calling it raises an \Error. popen() is also documented to return false when the process cannot be forked, which makes the fgets() and pclose() that follow raise a TypeError. All of those are \Error rather than \Exception, so they escape the handler in Bitmap::getThumbnail() and fail the request instead of degrading to a media type icon - the same hole the previous commit closed two frames up, on the path that commit routes into. An install without ext-fileinfo reaches this code on every bitmap preview, so the guard there was incomplete without this one. Verified that the guard is transparent when popen is available: the branch still returns the type the "file" binary reports. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- lib/private/Files/Type/Detection.php | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/lib/private/Files/Type/Detection.php b/lib/private/Files/Type/Detection.php index dfa155cc5131..d5bed83b7c7d 100644 --- a/lib/private/Files/Type/Detection.php +++ b/lib/private/Files/Type/Detection.php @@ -237,19 +237,30 @@ public function detect($path) { // use mime magic extension if available $mimeType = \mime_content_type($path); } - if (!$isWrapped and $mimeType === 'application/octet-stream' && \OC_Helper::canExecute("file")) { + // canExecute() only tells us the binary is there, not that we may start it: + // popen() is a common entry in disable_functions, and on PHP 8 a disabled function + // is undefined, so calling it raises an \Error. popen() is also documented to + // return false if the process cannot be forked, which makes fgets() and pclose() + // raise a TypeError. All three are \Error rather than \Exception, so they escape + // callers that guard a failed detection - OC\Preview\Bitmap::getThumbnail() among + // them, where the result is a failed request instead of a media type icon. + if (!$isWrapped and $mimeType === 'application/octet-stream' && \OC_Helper::canExecute("file") + && \function_exists('popen') + ) { // it looks like we have a 'file' command, // lets see if it does have mime support $path = \escapeshellarg($path); $fp = \popen("file -b --mime-type $path 2>/dev/null", "r"); - $reply = \fgets($fp); - \pclose($fp); + if (\is_resource($fp)) { + $reply = \fgets($fp); + \pclose($fp); - //trim the newline - $mimeType = \trim($reply); + //trim the newline + $mimeType = \trim((string)$reply); - if (empty($mimeType)) { - $mimeType = 'application/octet-stream'; + if (empty($mimeType)) { + $mimeType = 'application/octet-stream'; + } } } return $mimeType; From cb4196acd44c632822703c28d3d0dcd045041fa4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:31:38 +0200 Subject: [PATCH 18/18] docs: record which build the PostScript pin was measured on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rewritten comment rests the pin's safety on a measurement, so it should say where that measurement came from: ImageMagick 6.9.11-60 with Ghostscript 9.55.0, as shipped in owncloudci/php:8.3. Versions read from the build rather than recalled. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- lib/private/Preview/Postscript.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/private/Preview/Postscript.php b/lib/private/Preview/Postscript.php index 24d886742ea4..934909ddfbeb 100644 --- a/lib/private/Preview/Postscript.php +++ b/lib/private/Preview/Postscript.php @@ -36,7 +36,8 @@ protected function getImagickFormat(string $mimeType): string { # .ps as well as .eps. Measured rather than inferred from the coder internals: # plain PostScript and EPSF-tagged content declaring a bounding box smaller than # the page both render to the same geometry read unpinned, pinned EPS and pinned - # PS, so the pin does not change what a .ps file previews as. + # PS, so the pin does not change what a .ps file previews as. Measured on + # ImageMagick 6.9.11-60 with Ghostscript 9.55.0, the owncloudci/php:8.3 build. return 'EPS'; } }