From 0d8faa5e108652054dfc8d27e2be226ceeeb0003 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:05:20 +0200 Subject: [PATCH 01/14] test: stub the mime type in BitmapStreamTest so it survives the coder pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BitmapStreamTest mocks OCP\Files\File without stubbing getMimeType(), so the mock returns null. That is harmless today, but #41827 has Bitmap providers read the mime type to decide which Imagick coder to pin, and getResizedPreview() declares it as string - null there is a TypeError, which being an \Error escapes getThumbnail()'s \Exception handler rather than degrading to no preview. Merging #41827 would therefore turn these cases red on master. The success case also decoded a PNG through the Photoshop provider, which only works while ImageMagick is free to sniff the format. Once Photoshop pins the PSD coder, a PNG stops decoding and the case fails for a reason that has nothing to do with the stream. It now uses the PDF provider against testimage.pdf, so the provider, the file's mime type and the content all agree and the success path stays a success either way - guarded on the PDF coder, since pinning makes that a hard requirement. Verified against both trees: on master 3 tests / 5 assertions, and on master merged with #41827 the full tests/lib/Preview/ suite is 79 tests / 215 assertions / 0 failures, where before this change it reported 2 errors. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 35 +++++++++++++++++++++----- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index 1ed03f9b176..df92e88530c 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -21,6 +21,7 @@ namespace Test\Preview; +use OC\Preview\PDF; use OC\Preview\Photoshop; use OCP\Files\File; use Test\TestCase; @@ -30,14 +31,21 @@ */ class BitmapStreamTest extends TestCase { /** + * The mime type has to be stubbed even where this test does not care about it: + * Bitmap providers read $file->getMimeType() to decide which Imagick coder to use, + * and an unstubbed mock returns null, which is a TypeError rather than a bad + * preview - and a TypeError is an \Error, so it escapes getThumbnail()'s + * \Exception handler entirely. + * * @return array{0: File, 1: resource} */ - private function makeFile(string $content): array { + private function makeFile(string $content, string $mimeType): array { $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); $file->method('getSize')->willReturn(\strlen($content)); $file->method('getPath')->willReturn('/test/bitmap-stream'); return [$file, $stream]; @@ -49,9 +57,15 @@ private function makeFile(string $content): array { * released on that path too. Otherwise a preview pre-generation run, or a cron * preview job, over a directory of undecodable files exhausts the process's * descriptors one file at a time. + * + * XML content is rejected before any coder is consulted, so this case needs no + * particular coder to be registered. */ public function testClosesTheStreamWhenDecodingThrows(): void { - list($file, $stream) = $this->makeFile('x'); + list($file, $stream) = $this->makeFile( + 'x', + 'application/x-photoshop' + ); $result = (new Photoshop())->getThumbnail($file, 32, 32, false); @@ -59,13 +73,21 @@ public function testClosesTheStreamWhenDecodingThrows(): void { $this->assertFalse(\is_resource($stream), 'the stream must be closed on the failure path'); } + /** + * Uses PDF against a PDF, so the provider, the file's mime type and the content all + * agree - the success path has to stay a success regardless of whether the provider + * decodes by pinning a coder or by letting ImageMagick sniff one. + */ public function testClosesTheStreamOnSuccess(): void { - $png = \file_get_contents(\OC::$SERVERROOT . '/tests/data/testimage.png'); - list($file, $stream) = $this->makeFile($png); + if (\count(\Imagick::queryFormats('PDF')) === 0) { + $this->markTestSkipped('This ImageMagick build registers no PDF coder'); + } + $pdf = \file_get_contents(\OC::$SERVERROOT . '/tests/data/testimage.pdf'); + list($file, $stream) = $this->makeFile($pdf, 'application/pdf'); - $result = (new Photoshop())->getThumbnail($file, 32, 32, false); + $result = (new PDF())->getThumbnail($file, 32, 32, false); - $this->assertNotFalse($result, 'a PNG should still decode'); + $this->assertNotFalse($result, 'a PDF should decode through the PDF provider'); $this->assertFalse(\is_resource($stream), 'the stream must be closed on the success path'); } @@ -78,6 +100,7 @@ public function testClosesTheStreamOnSuccess(): void { public function testReturnsFalseWhenTheFileCannotBeOpened(): void { $file = $this->createMock(File::class); $file->method('fopen')->willReturn(false); + $file->method('getMimeType')->willReturn('application/x-photoshop'); $file->method('getSize')->willReturn(1024); $file->method('getPath')->willReturn('/test/unopenable'); From 20b31317feef1a22c6a9c1a6774e8897c731c422 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:18:58 +0200 Subject: [PATCH 02/14] test: decode a self-written TIFF rather than gating on the PDF coder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Imagick::queryFormats('PDF') reports that the coder was compiled in. It says nothing about whether a PDF can actually be decoded: it consults neither the coder rights in policy.xml nor the presence of the Ghostscript delegate. On an image that revokes the PDF coder - the ImageMagick hardening OC10-164 is itself driving - or one without the gs binary, the guard passes, readImageBlob() throws, and the case fails red over an environment difference rather than over the stream handling it exists to check. That is the same mistake as gating a test on a coder the provider never uses, which this series has been removing elsewhere. The success case now writes its own TIFF through Imagick and decodes it through the TIFF provider. TIFF needs no external delegate, and a build cannot disagree with itself about a blob it just produced, so the remaining skip fires only where TIFF is unavailable altogether - in which case no assertion here could run anyway. It also drops a fixture dependency. The comments claiming that the mime type is read and that XML is rejected before any coder is consulted described the coder-pin change on #41827, which is not in this tree. They now say what happens here and what they anticipate. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 57 ++++++++++++++++++-------- 1 file changed, 39 insertions(+), 18 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index df92e88530c..40160ba78cc 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -21,8 +21,8 @@ namespace Test\Preview; -use OC\Preview\PDF; use OC\Preview\Photoshop; +use OC\Preview\TIFF; use OCP\Files\File; use Test\TestCase; @@ -31,11 +31,12 @@ */ class BitmapStreamTest extends TestCase { /** - * The mime type has to be stubbed even where this test does not care about it: - * Bitmap providers read $file->getMimeType() to decide which Imagick coder to use, - * and an unstubbed mock returns null, which is a TypeError rather than a bad - * preview - and a TypeError is an \Error, so it escapes getThumbnail()'s - * \Exception handler entirely. + * The mime type is stubbed even though nothing in this tree reads it yet. It + * anticipates the coder-pin change on #41827, where Bitmap providers derive the + * Imagick coder from $file->getMimeType() and getResizedPreview() declares that + * parameter as string: an unstubbed mock yields null, which is a TypeError rather + * than a failed preview, and a TypeError is an \Error, so it escapes + * getThumbnail()'s \Exception handler entirely. * * @return array{0: File, 1: resource} */ @@ -58,8 +59,9 @@ private function makeFile(string $content, string $mimeType): array { * preview job, over a directory of undecodable files exhausts the process's * descriptors one file at a time. * - * XML content is rejected before any coder is consulted, so this case needs no - * particular coder to be registered. + * Needs no particular coder: no build has one for non-image XML, so readImageBlob() + * throws everywhere. On #41827 it never reaches a coder at all, because the mime + * gate rejects XML first - either way the throw is what this asserts about. */ public function testClosesTheStreamWhenDecodingThrows(): void { list($file, $stream) = $this->makeFile( @@ -74,23 +76,42 @@ public function testClosesTheStreamWhenDecodingThrows(): void { } /** - * Uses PDF against a PDF, so the provider, the file's mime type and the content all - * agree - the success path has to stay a success regardless of whether the provider - * decodes by pinning a coder or by letting ImageMagick sniff one. + * Provider, mime type and content all agree, so the success path stays a success + * whether the provider decodes by pinning a coder (#41827) or by letting ImageMagick + * sniff one. + * + * TIFF rather than PDF, and a blob this test writes itself rather than a fixture: + * PDF decoding depends on the Ghostscript delegate and is the coder most likely to + * be revoked by a hardened policy.xml, neither of which queryFormats() reports - so + * gating on it would leave this red on exactly the images OC10-164 hardens. Writing + * and reading the blob with the same build cannot disagree with itself. */ public function testClosesTheStreamOnSuccess(): void { - if (\count(\Imagick::queryFormats('PDF')) === 0) { - $this->markTestSkipped('This ImageMagick build registers no PDF coder'); - } - $pdf = \file_get_contents(\OC::$SERVERROOT . '/tests/data/testimage.pdf'); - list($file, $stream) = $this->makeFile($pdf, 'application/pdf'); + list($file, $stream) = $this->makeFile($this->tiffBlob(), 'image/tiff'); - $result = (new PDF())->getThumbnail($file, 32, 32, false); + $result = (new TIFF())->getThumbnail($file, 32, 32, false); - $this->assertNotFalse($result, 'a PDF should decode through the PDF provider'); + $this->assertNotFalse($result, 'a TIFF should decode through the TIFF provider'); $this->assertFalse(\is_resource($stream), 'the stream must be closed on the success path'); } + /** + * A TIFF needs no external delegate, so this skips only where the build cannot + * handle TIFF at all - in which case the assertions above could not run either way. + */ + private function tiffBlob(): string { + try { + $image = new \Imagick(); + $image->newImage(64, 48, new \ImagickPixel('white')); + $image->setImageFormat('tiff'); + $blob = $image->getImageBlob(); + $image->clear(); + } catch (\ImagickException $e) { + $this->markTestSkipped('This ImageMagick build cannot produce a TIFF: ' . $e->getMessage()); + } + return $blob; + } + /** * A storage that cannot open the file returns false rather than throwing, and * stream_get_contents(false) raises a TypeError - an \Error, so it would escape the From b4a737b72fe96ebe3d7faeb13b9b5d8710b80207 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:28:37 +0200 Subject: [PATCH 03/14] test: probe the TIFF read path, not just the write path, before asserting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard added in the previous commit wrote a TIFF and treated that as proof the build could handle TIFF. ImageMagick grants coder rights per direction, so a policy of rights="write" for TIFF lets the blob be produced, declines to skip, and then fails red on the decode - reintroducing exactly the failure the guard exists to remove. It now reads the blob back inside the guard, so what is probed is what the assertion needs. Verified by revoking TIFF read in a throwaway container: the case skips with a clear message instead of failing. The guard also caught only \ImagickException, while ImagickPixelException extends \Exception directly and is a sibling rather than a subclass, so a pixel-wand failure would have escaped as an error rather than the intended skip. It now catches \Exception, and the Imagick handles are released in finally blocks rather than only on the success path - which matters in a test about releasing handles. Finally, the claim that no coder is consulted for the XML payload was wrong: ImageMagick's SVG coder claims any blob opening with " root. The comment now says that, and warns that another XML payload is not automatically substitutable. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 33 ++++++++++++++++++++------ 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index 40160ba78cc..1fc806ae216 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -59,9 +59,12 @@ private function makeFile(string $content, string $mimeType): array { * preview job, over a directory of undecodable files exhausts the process's * descriptors one file at a time. * - * Needs no particular coder: no build has one for non-image XML, so readImageBlob() - * throws everywhere. On #41827 it never reaches a coder at all, because the mime - * gate rejects XML first - either way the throw is what this asserts about. + * Needs no particular coder registered for the payload's own sake: ImageMagick's SVG + * coder claims any blob opening with " root, so readImageBlob() throws on every build. On #41827 it does not reach a + * coder at all, because the mime gate rejects XML first. Either way the throw is what + * this asserts about - but a different XML payload is not automatically substitutable, + * since the guarantee rests on SVG rendering erroring out. */ public function testClosesTheStreamWhenDecodingThrows(): void { list($file, $stream) = $this->makeFile( @@ -97,17 +100,33 @@ public function testClosesTheStreamOnSuccess(): void { /** * A TIFF needs no external delegate, so this skips only where the build cannot - * handle TIFF at all - in which case the assertions above could not run either way. + * round-trip one - in which case the assertions above could not run either way. + * + * The blob is read back inside the guard on purpose: coder rights are granted per + * direction, so writing a TIFF does not establish that this build can read one. + * Probing only the write half would let a write-but-not-read build produce a blob, + * decline to skip, and then fail red - the failure mode this guard exists to remove. */ private function tiffBlob(): string { + $image = new \Imagick(); try { - $image = new \Imagick(); $image->newImage(64, 48, new \ImagickPixel('white')); $image->setImageFormat('tiff'); $blob = $image->getImageBlob(); + + $probe = new \Imagick(); + try { + $probe->readImageBlob($blob); + } finally { + $probe->clear(); + } + } catch (\Exception $e) { + # \Exception, not \ImagickException: ImagickPixelException extends \Exception + # directly and is a sibling of ImagickException, so a pixel-wand failure would + # otherwise escape as an error rather than the intended skip + $this->markTestSkipped('This ImageMagick build cannot round-trip a TIFF: ' . $e->getMessage()); + } finally { $image->clear(); - } catch (\ImagickException $e) { - $this->markTestSkipped('This ImageMagick build cannot produce a TIFF: ' . $e->getMessage()); } return $blob; } From 3cd90dca50d8aa96049fba3eaed3bf4e5721780d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:38:51 +0200 Subject: [PATCH 04/14] test: skip only when TIFF is absent, and assert the stream before the decode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-trip probe added in the previous commit closed one hole by opening another: it turned any TIFF failure into a skip, and a skip here costs the success-path fclose() assertion - which is the OC10-164 stream-leak guard itself. A guard quietly withholding these assertions is exactly how they came to never run in CI, so a misconfiguration should be loud, not green. The guard is now the single condition that is genuinely an absent feature rather than a broken setup: no TIFF coder registered at all. Revoked coder rights, an unparsable policy.xml or a wand that cannot be constructed all fail. TIFF can be held to that standard because no stock policy revokes it, unlike PDF, which Debian and Ubuntu deny out of the box - the reason this uses a TIFF in the first place. The stream assertion also moves ahead of the decode assertion, so an environment that cannot decode the blob still exercises the handle release under test and still reports the decode as the failure. Verified by revoking TIFF read in a throwaway container: all five assertions run, and the failure names the decode. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 53 +++++++++++++------------- 1 file changed, 27 insertions(+), 26 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index 1fc806ae216..762f5bce063 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -83,52 +83,53 @@ public function testClosesTheStreamWhenDecodingThrows(): void { * whether the provider decodes by pinning a coder (#41827) or by letting ImageMagick * sniff one. * - * TIFF rather than PDF, and a blob this test writes itself rather than a fixture: - * PDF decoding depends on the Ghostscript delegate and is the coder most likely to - * be revoked by a hardened policy.xml, neither of which queryFormats() reports - so - * gating on it would leave this red on exactly the images OC10-164 hardens. Writing - * and reading the blob with the same build cannot disagree with itself. + * TIFF rather than PDF, and a blob this test writes itself rather than a fixture: PDF + * decoding needs the Ghostscript delegate and is denied by Debian's and Ubuntu's + * stock policy.xml, neither of which queryFormats() reports - so gating on a PDF + * would be red on a plain apt-installed ImageMagick rather than skipped. + * + * The stream is asserted before the decode result, so that an environment which + * cannot decode the blob still exercises the handle-release behaviour under test and + * reports the decode as the failure it is. */ public function testClosesTheStreamOnSuccess(): void { list($file, $stream) = $this->makeFile($this->tiffBlob(), 'image/tiff'); $result = (new TIFF())->getThumbnail($file, 32, 32, false); - $this->assertNotFalse($result, 'a TIFF should decode through the TIFF provider'); $this->assertFalse(\is_resource($stream), 'the stream must be closed on the success path'); + $this->assertNotFalse($result, 'a TIFF should decode through the TIFF provider'); } /** - * A TIFF needs no external delegate, so this skips only where the build cannot - * round-trip one - in which case the assertions above could not run either way. + * Skips on exactly one condition - no TIFF coder in this build at all - and lets + * every other failure be loud. * - * The blob is read back inside the guard on purpose: coder rights are granted per - * direction, so writing a TIFF does not establish that this build can read one. - * Probing only the write half would let a write-but-not-read build produce a blob, - * decline to skip, and then fail red - the failure mode this guard exists to remove. + * That line is deliberate rather than convenient. A skip here costs the success-path + * fclose() assertion, and a guard quietly withholding these assertions is how they + * came to never run in CI in the first place, so anything that is a misconfiguration + * rather than an absent feature should be visible: revoked TIFF coder rights, a + * broken policy.xml, a wand that cannot be constructed. TIFF is safe to hold to that + * standard because no stock policy revokes it - unlike PDF, which Debian and Ubuntu + * deny out of the box, and which is why this does not use a PDF. + * + * This does not attempt to prove the whole path: getThumbnail() also needs PNG + * encoding and GD to read the result back. Both are hard requirements of the product + * (composer.json requires ext-gd), and PNG output is what every preview in ownCloud + * depends on, so a build failing those should fail this test rather than skip it. */ private function tiffBlob(): string { + if (\count(\Imagick::queryFormats('TIFF')) === 0) { + $this->markTestSkipped('This ImageMagick build registers no TIFF coder'); + } $image = new \Imagick(); try { $image->newImage(64, 48, new \ImagickPixel('white')); $image->setImageFormat('tiff'); - $blob = $image->getImageBlob(); - - $probe = new \Imagick(); - try { - $probe->readImageBlob($blob); - } finally { - $probe->clear(); - } - } catch (\Exception $e) { - # \Exception, not \ImagickException: ImagickPixelException extends \Exception - # directly and is a sibling of ImagickException, so a pixel-wand failure would - # otherwise escape as an error rather than the intended skip - $this->markTestSkipped('This ImageMagick build cannot round-trip a TIFF: ' . $e->getMessage()); + return $image->getImageBlob(); } finally { $image->clear(); } - return $blob; } /** From 9587bf1d9cee5e2ac4dee689e2f9c5f78e1a4a33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:46:27 +0200 Subject: [PATCH 05/14] test: separate an absent TIFF delegate from a denied one by ImageMagick's message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit gated on Imagick::queryFormats('TIFF'), on the assumption that registration implies support. It does not: coders/tiff.c registers TIFF, TIF and TIFF64 unconditionally and only assigns the decoder and encoder pointers when built against libtiff, while GetMagickList() behind queryFormats() matches on the coder name alone. A build without libtiff therefore reports TIFF as registered, declines to skip, and - with the catch removed by that same commit - errors instead. That is the fourth variant of one mistake in this file: checking something adjacent to what the assertion needs. There is no registration check that can tell an absent feature from a broken setup, so this stops using a proxy and reads what ImageMagick reports. A missing delegate yields "no encode delegate for this image format" (or the decode equivalent) and skips; a policy denial yields "not allowed by the security policy" and is re-thrown, along with anything else. Both directions are probed, since coder rights are granted per direction. The success-path assertion message is also outcome-neutral now. It runs before the decode assertion, so it fires when the decode failed too, and must not claim the leak was on the success path when the decode is the actual defect. Verified in throwaway containers: a normal build passes; a policy revoking TIFF is loud rather than skipped; and MagickCore's message catalogue carries both delegate strings this matches on. The missing-delegate branch is matched against that catalogue rather than executed, since this build has libtiff. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 60 ++++++++++++++++++-------- 1 file changed, 43 insertions(+), 17 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index 762f5bce063..3efb6dc5c61 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -97,36 +97,62 @@ public function testClosesTheStreamOnSuccess(): void { $result = (new TIFF())->getThumbnail($file, 32, 32, false); - $this->assertFalse(\is_resource($stream), 'the stream must be closed on the success path'); + # outcome-neutral message: this assertion runs first, so it also fires where the + # decode did not succeed, and must not then claim the leak was on the success path + $this->assertFalse(\is_resource($stream), 'the stream must be closed once getThumbnail() returns'); $this->assertNotFalse($result, 'a TIFF should decode through the TIFF provider'); } /** - * Skips on exactly one condition - no TIFF coder in this build at all - and lets - * every other failure be loud. + * Skips when this build has no TIFF support, and is loud about everything else. * - * That line is deliberate rather than convenient. A skip here costs the success-path + * That line is deliberate rather than convenient. A skip costs the success-path * fclose() assertion, and a guard quietly withholding these assertions is how they - * came to never run in CI in the first place, so anything that is a misconfiguration - * rather than an absent feature should be visible: revoked TIFF coder rights, a - * broken policy.xml, a wand that cannot be constructed. TIFF is safe to hold to that - * standard because no stock policy revokes it - unlike PDF, which Debian and Ubuntu - * deny out of the box, and which is why this does not use a PDF. + * came to never run in CI in the first place - so an absent feature may skip, while a + * broken setup (revoked coder rights, an unparsable policy.xml, a wand that will not + * construct) has to be visible. * - * This does not attempt to prove the whole path: getThumbnail() also needs PNG - * encoding and GD to read the result back. Both are hard requirements of the product - * (composer.json requires ext-gd), and PNG output is what every preview in ownCloud - * depends on, so a build failing those should fail this test rather than skip it. + * The two are told apart by what ImageMagick reports, because no registration check + * can do it: coders/tiff.c registers TIFF, TIF, TIFF64 and friends unconditionally + * and only assigns the decoder/encoder pointers when built against libtiff, while + * GetMagickList() behind queryFormats() matches on the coder name alone. A build with + * no libtiff therefore reports TIFF as registered and then fails with MagickCore's + * "no encode delegate for this image format", whereas a policy denial says "not + * allowed by the security policy" - so the message is what separates them. + * + * Both directions are exercised: coder rights are granted per direction, so writing a + * TIFF does not establish that one can be read back, which is what the assertion + * actually needs. + * + * This deliberately does not try to prove the whole path - getThumbnail() also needs + * PNG encoding and GD to load the result. Both are hard product requirements + * (composer.json requires ext-gd) and PNG output underpins every ownCloud preview, so + * a build failing those should fail this test rather than skip it. */ private function tiffBlob(): string { - if (\count(\Imagick::queryFormats('TIFF')) === 0) { - $this->markTestSkipped('This ImageMagick build registers no TIFF coder'); - } $image = new \Imagick(); try { $image->newImage(64, 48, new \ImagickPixel('white')); $image->setImageFormat('tiff'); - return $image->getImageBlob(); + $blob = $image->getImageBlob(); + + $probe = new \Imagick(); + try { + $probe->readImageBlob($blob); + } finally { + $probe->clear(); + } + return $blob; + } catch (\Exception $e) { + # \Exception rather than \ImagickException: ImagickPixelException extends + # \Exception directly and is a sibling, not a subclass + $message = $e->getMessage(); + if (\stripos($message, 'no encode delegate') !== false + || \stripos($message, 'no decode delegate') !== false + ) { + $this->markTestSkipped('This ImageMagick build has no TIFF delegate: ' . $message); + } + throw $e; } finally { $image->clear(); } From f33dd9813f50f124d5ea0452625dc38ca229eb36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:53:26 +0200 Subject: [PATCH 06/14] test: keep both TIFF guards, since neither covers the other's case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit swapped the queryFormats() check for a message check when the two are complementary. Without libtiff, a modular ImageMagick - Debian and Ubuntu configure --with-modules - never builds coders/tiff.so, so TIFF is not registered and setImageFormat() fails with php-imagick's own "Unable to set the image format" before any delegate is consulted. That matches neither delegate substring, so it was rethrown and turned a build with no TIFF feature red. queryFormats() is what catches that case; the message check catches the non-modular build, which registers TIFF regardless and fails later at the delegate. Both are back. Also records two limits instead of implying they do not exist. A module- or coder-domain policy denial can surface as MissingDelegateError, textually identical to an absent delegate, so such a build skips - the classifier only rejects messages that name a policy outright rather than guessing. And an allowlist-style policy.xml denying all but a few coders fails here, which is the accepted cost of being loud about misconfiguration; the note explaining why TIFF rather than PDF is restored alongside it. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 38 +++++++++++++++++++------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index 3efb6dc5c61..60f5f566aa0 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -112,13 +112,26 @@ public function testClosesTheStreamOnSuccess(): void { * broken setup (revoked coder rights, an unparsable policy.xml, a wand that will not * construct) has to be visible. * - * The two are told apart by what ImageMagick reports, because no registration check - * can do it: coders/tiff.c registers TIFF, TIF, TIFF64 and friends unconditionally - * and only assigns the decoder/encoder pointers when built against libtiff, while - * GetMagickList() behind queryFormats() matches on the coder name alone. A build with - * no libtiff therefore reports TIFF as registered and then fails with MagickCore's - * "no encode delegate for this image format", whereas a policy denial says "not - * allowed by the security policy" - so the message is what separates them. + * It takes two checks, because neither sees the other's case. Without libtiff, a + * modular build (Debian and Ubuntu configure --with-modules) never builds + * coders/tiff.so, so TIFF is not registered and setImageFormat() fails before any + * delegate is consulted - queryFormats() is what catches that. A non-modular build + * registers TIFF regardless, because coders/tiff.c registers the entries + * unconditionally and only assigns the decoder/encoder pointers when libtiff is + * present, while GetMagickList() behind queryFormats() matches on the name alone; that + * one fails later with MagickCore's "no encode delegate for this image format", which + * is what the message check catches. A policy denial instead says "not allowed by the + * security policy" and stays loud. + * + * Known limit: a module- or coder-domain policy denial can itself surface as + * MissingDelegateError, which is textually identical to an absent delegate. Such a + * build skips rather than failing. Rather than guess, this only rejects messages that + * name a policy outright. + * + * TIFF rather than PDF because no stock policy revokes it, whereas Debian and Ubuntu + * deny PDF out of the box. An allowlist-style policy.xml that denies all coders bar a + * handful would still fail here - an accepted cost, since being loud about a + * misconfiguration is the point. * * Both directions are exercised: coder rights are granted per direction, so writing a * TIFF does not establish that one can be read back, which is what the assertion @@ -130,6 +143,9 @@ public function testClosesTheStreamOnSuccess(): void { * a build failing those should fail this test rather than skip it. */ private function tiffBlob(): string { + if (\count(\Imagick::queryFormats('TIFF')) === 0) { + $this->markTestSkipped('This ImageMagick build registers no TIFF coder'); + } $image = new \Imagick(); try { $image->newImage(64, 48, new \ImagickPixel('white')); @@ -147,9 +163,11 @@ private function tiffBlob(): string { # \Exception rather than \ImagickException: ImagickPixelException extends # \Exception directly and is a sibling, not a subclass $message = $e->getMessage(); - if (\stripos($message, 'no encode delegate') !== false - || \stripos($message, 'no decode delegate') !== false - ) { + $missingDelegate = \stripos($message, 'no encode delegate') !== false + || \stripos($message, 'no decode delegate') !== false; + $namesAPolicy = \stripos($message, 'policy') !== false + || \stripos($message, 'not authorized') !== false; + if ($missingDelegate && !$namesAPolicy) { $this->markTestSkipped('This ImageMagick build has no TIFF delegate: ' . $message); } throw $e; From efd27d8ade06e381c59af4b2eb760b84e188a524 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:23:07 +0200 Subject: [PATCH 07/14] test: fail the undecodable case on bytes no coder claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The payload was 'x', which is not environment-independent. ImageMagick's IsSVG() claims any blob opening with " Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index 60f5f566aa0..c8d133fffb9 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -59,16 +59,20 @@ private function makeFile(string $content, string $mimeType): array { * preview job, over a directory of undecodable files exhausts the process's * descriptors one file at a time. * - * Needs no particular coder registered for the payload's own sake: ImageMagick's SVG - * coder claims any blob opening with " root, so readImageBlob() throws on every build. On #41827 it does not reach a - * coder at all, because the mime gate rejects XML first. Either way the throw is what - * this asserts about - but a different XML payload is not automatically substitutable, - * since the guarantee rests on SVG rendering erroring out. + * The payload is bytes no coder claims: ImageMagick sniffs the format as "" and + * readImageBlob() fails with "no decode delegate for this image format `'" on every + * build, whatever delegates it has. That is deliberate. An XML payload would be + * claimed by the SVG coder - IsSVG() matches any blob opening with "makeFile( - 'x', + "\x00\x01\x02\x03 oc10-164 not an image \xff\xfe", 'application/x-photoshop' ); From a4952d77dde962972e692989d1d6663c731baa1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:31:20 +0200 Subject: [PATCH 08/14] docs: correct the recorded reasons in BitmapStreamTest's comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three claims in these docblocks were wrong, and the payload rationale was the one that mattered: it said an XML payload "would throw only where no SVG renderer is registered" and would otherwise return a blank canvas. Measured in three builds - stock, with libmagickcore-6.q16-6-extra installed, and with the policy opened up - it throws in all of them, as "no decode delegate `SVG'", then "not allowed by the security policy `MVG'", then MVG's own "must specify image size". The coder is MVG rather than SVG too. So the reason to prefer bytes no coder claims is not that the XML payload is unusable, it is that its failure reason varies by build and that libmagic reads it as text/xml, which #41827's mime gate rejects before the decode. The comment now says that, so nobody rules out a working option on a wrong premise. The read-back rationale claimed both directions get denied; what actually happens with TIFF rights revoked is that getImageBlob() still returns a blob and only the read raises - which is the argument for probing the read, now stated as measured. The mime-type stub was described as anticipating #41827 and reading as speculative, when omitting it is precisely what turned that PR red. It is stated as a requirement instead, so it does not invite deletion once the pin lands. Also trims the libtiff explanation. It asserted ImageMagick internals no assertion here pins and which differ across major versions, and it is where the errors above were concentrated; the two-check rationale and the PDF-vs-TIFF choice stay. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 57 +++++++++++--------------- 1 file changed, 25 insertions(+), 32 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index c8d133fffb9..cdb50945d37 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -31,12 +31,11 @@ */ class BitmapStreamTest extends TestCase { /** - * The mime type is stubbed even though nothing in this tree reads it yet. It - * anticipates the coder-pin change on #41827, where Bitmap providers derive the - * Imagick coder from $file->getMimeType() and getResizedPreview() declares that - * parameter as string: an unstubbed mock yields null, which is a TypeError rather - * than a failed preview, and a TypeError is an \Error, so it escapes - * getThumbnail()'s \Exception handler entirely. + * Every File mock here must stub getMimeType(). Bitmap providers derive the Imagick + * coder from it, and getResizedPreview() declares that parameter as string, so an + * unstubbed mock yields null and a TypeError - which is an \Error, and therefore + * escapes getThumbnail()'s \Exception handler instead of degrading to no preview. + * Required rather than speculative: dropping these stubs is what turned #41827 red. * * @return array{0: File, 1: resource} */ @@ -59,16 +58,16 @@ private function makeFile(string $content, string $mimeType): array { * preview job, over a directory of undecodable files exhausts the process's * descriptors one file at a time. * - * The payload is bytes no coder claims: ImageMagick sniffs the format as "" and - * readImageBlob() fails with "no decode delegate for this image format `'" on every - * build, whatever delegates it has. That is deliberate. An XML payload would be - * claimed by the SVG coder - IsSVG() matches any blob opening with "makeFile( @@ -116,30 +115,24 @@ public function testClosesTheStreamOnSuccess(): void { * broken setup (revoked coder rights, an unparsable policy.xml, a wand that will not * construct) has to be visible. * - * It takes two checks, because neither sees the other's case. Without libtiff, a - * modular build (Debian and Ubuntu configure --with-modules) never builds - * coders/tiff.so, so TIFF is not registered and setImageFormat() fails before any - * delegate is consulted - queryFormats() is what catches that. A non-modular build - * registers TIFF regardless, because coders/tiff.c registers the entries - * unconditionally and only assigns the decoder/encoder pointers when libtiff is - * present, while GetMagickList() behind queryFormats() matches on the name alone; that - * one fails later with MagickCore's "no encode delegate for this image format", which - * is what the message check catches. A policy denial instead says "not allowed by the - * security policy" and stays loud. + * It takes two checks, because neither sees the other's case. Depending on how + * ImageMagick was built, a missing libtiff either leaves TIFF unregistered - which + * queryFormats() catches - or leaves it registered and failing later at the delegate, + * which the message check catches. A policy denial says "not allowed by the security + * policy" instead and stays loud. * * Known limit: a module- or coder-domain policy denial can itself surface as - * MissingDelegateError, which is textually identical to an absent delegate. Such a - * build skips rather than failing. Rather than guess, this only rejects messages that - * name a policy outright. + * MissingDelegateError, textually identical to an absent delegate, so such a build + * skips. Rather than guess, this only rejects messages naming a policy outright. * * TIFF rather than PDF because no stock policy revokes it, whereas Debian and Ubuntu - * deny PDF out of the box. An allowlist-style policy.xml that denies all coders bar a + * deny PDF out of the box. An allowlist-style policy.xml denying all coders bar a * handful would still fail here - an accepted cost, since being loud about a * misconfiguration is the point. * - * Both directions are exercised: coder rights are granted per direction, so writing a - * TIFF does not establish that one can be read back, which is what the assertion - * actually needs. + * Both directions are exercised because writing a TIFF does not establish that one can + * be read back, and reading is what the assertion needs. Measured: with TIFF coder + * rights revoked, getImageBlob() still returned a blob and only the read-back raised. * * This deliberately does not try to prove the whole path - getThumbnail() also needs * PNG encoding and GD to load the result. Both are hard product requirements From 2c1f3d5a21530e2e66da55567ba171b077a46bea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:37:48 +0200 Subject: [PATCH 09/14] test: pin why the undecodable case throws, and assert the handle first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both assertions in testClosesTheStreamWhenDecodingThrows are satisfied by any early return from getThumbnail(), and nothing tied the failure to the decode. That matters on #41827, which adds a pre-decode mime gate denying text/*: a build whose libmagic read these bytes as text would refuse them before any coder, leave this test green, and quietly stop covering the path the test is named for. The detected media type is now asserted, so that drift fails instead of hiding. The two tests also disagreed on assertion order. PHPUnit stops at the first failure, so asserting the result first meant an unexpectedly decodable payload would mask a co-occurring leak - the handle being the regression guard this file exists for. testClosesTheStreamOnSuccess already ordered it the other way and said why; the failure case now matches. The payload rationale claimed the sniffed format is "", which holds here but not under the pin, where nothing is sniffed and the pinned coder rejects the header instead. Both throw without depending on the build's delegates, which is the actual property being relied on, so the comment says that rather than one tree's mechanism. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 36 ++++++++++++++++---------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index cdb50945d37..4e152a82d6c 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -58,27 +58,37 @@ private function makeFile(string $content, string $mimeType): array { * preview job, over a directory of undecodable files exhausts the process's * descriptors one file at a time. * - * The payload is bytes no coder claims, so ImageMagick sniffs the format as "" and - * readImageBlob() fails identically on every build. An XML payload also throws - * everywhere measured, but for a reason that varies with the build - no SVG delegate, - * or a denied MVG coder, or MVG's own "must specify image size" once the coders are - * installed and the policy opened - and libmagic reads it as text/xml, which #41827's - * mime gate rejects before the decode is reached at all. + * The payload is bytes no coder claims. Here that means ImageMagick sniffs the format + * as "" and readImageBlob() reports no decode delegate; under #41827's pin there is no + * sniffing at all and the pinned coder rejects the header instead. Different messages, + * same outcome, and neither depends on which delegates the build happens to have. * - * These bytes are read as application/octet-stream instead, so they are not text and - * still reach the decode on that branch. Keeping the failure in one place, for one - * reason, on both trees is the point. + * An XML payload also throws everywhere measured, but for a reason that varies with the + * build - no SVG delegate, or a denied MVG coder, or MVG's own "must specify image + * size" once the coders are installed and the policy opened. More importantly libmagic + * reads it as text/xml, and #41827 adds a pre-decode mime gate that denies text/*, so + * it would stop reaching the decode at all on that branch. These bytes read as + * application/octet-stream, which the assertion below pins so the drift is visible. */ public function testClosesTheStreamWhenDecodingThrows(): void { - list($file, $stream) = $this->makeFile( - "\x00\x01\x02\x03 oc10-164 not an image \xff\xfe", - 'application/x-photoshop' + $content = "\x00\x01\x02\x03 oc10-164 not an image \xff\xfe"; + # Pin why this throws. Both assertions below are satisfied by any early return, so + # on #41827's tree a build whose libmagic called these bytes text/* would refuse + # them at the mime gate, keep this test green, and quietly stop covering the decode + # this test is named for. + $this->assertStringStartsWith( + 'application/octet-stream', + \OC::$server->getMimeTypeDetector()->detectString($content), + 'the payload must not read as text, or the decode is never reached on the pinned branch' ); + list($file, $stream) = $this->makeFile($content, 'application/x-photoshop'); $result = (new Photoshop())->getThumbnail($file, 32, 32, false); + # stream first, as in testClosesTheStreamOnSuccess: PHPUnit stops at the first + # failure, and the handle is the regression guard worth keeping + $this->assertFalse(\is_resource($stream), 'the stream must be closed once getThumbnail() returns'); $this->assertFalse($result, 'undecodable content must not produce a preview'); - $this->assertFalse(\is_resource($stream), 'the stream must be closed on the failure path'); } /** From 7b7305aa6d550b9841557abfca98fdc33004b35a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:54:16 +0200 Subject: [PATCH 10/14] test: decode a PSD, dropping the TIFF availability guard entirely MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every guard in this file existed because the success case used TIFF, and TIFF can be absent: coders/tiff.so links libtiff. PSD cannot be absent for that reason - coders/psd.so links no image library at all, ImageMagick implements the format natively - and the Photoshop provider was already here for the failure case. So the success case now writes and decodes a PSD, and the whole apparatus goes: no queryFormats() check, no write-then-read-back probe, no message classifier separating an absent delegate from a denied one, and no docblock asserting ImageMagick internals that nothing pins. The test is unconditional, which is what it should have been throughout - a skip would retire the success-path fclose() assertion, and a guard quietly withholding assertions is how the OC10-164 preview tests came to never run in CI to begin with. The file loses 44 lines. This also removes a contradiction with the branch it is written to be compatible with: CoderPinningTest::requireDecodableFixture() skips on a policy denial where the classifier here rethrew, so the same suite gave two answers for the same coder. Verified: unconditional pass on master and on the tree merged with the pin, and still red when the finally that releases the handle is removed. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 130 ++++++++----------------- 1 file changed, 38 insertions(+), 92 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index 4e152a82d6c..03e3e24d6ff 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -22,7 +22,6 @@ namespace Test\Preview; use OC\Preview\Photoshop; -use OC\Preview\TIFF; use OCP\Files\File; use Test\TestCase; @@ -31,11 +30,13 @@ */ class BitmapStreamTest extends TestCase { /** - * Every File mock here must stub getMimeType(). Bitmap providers derive the Imagick - * coder from it, and getResizedPreview() declares that parameter as string, so an - * unstubbed mock yields null and a TypeError - which is an \Error, and therefore - * escapes getThumbnail()'s \Exception handler instead of degrading to no preview. - * Required rather than speculative: dropping these stubs is what turned #41827 red. + * getMimeType() is stubbed on every mock that reaches a decode. #41834 has Bitmap + * providers derive the Imagick coder from it, and getResizedPreview() there declares + * the parameter as string, so an unstubbed mock yields null and a TypeError - an + * \Error, which escapes getThumbnail()'s \Exception handler instead of degrading to no + * preview. Omitting these is what turned #41827 red, so they are a requirement once + * that pin lands rather than a precaution. On this branch getResizedPreview() takes no + * mime type at all, so nothing reads them yet. * * @return array{0: File, 1: resource} */ @@ -54,26 +55,25 @@ private function makeFile(string $content, string $mimeType): array { /** * getResizedPreview() throwing is a routine outcome rather than an exceptional one - * any content ImageMagick has no coder for reaches it - so the handle has to be - * released on that path too. Otherwise a preview pre-generation run, or a cron - * preview job, over a directory of undecodable files exhausts the process's - * descriptors one file at a time. + * released on that path too. Otherwise a preview pre-generation run, or a cron preview + * job, over a directory of undecodable files exhausts the process's descriptors one + * file at a time. * * The payload is bytes no coder claims. Here that means ImageMagick sniffs the format - * as "" and readImageBlob() reports no decode delegate; under #41827's pin there is no - * sniffing at all and the pinned coder rejects the header instead. Different messages, - * same outcome, and neither depends on which delegates the build happens to have. + * as "" and reports no decode delegate; under #41834's pin there is no sniffing and the + * pinned PSD coder rejects the header instead. Different messages, same outcome, and + * neither depends on which delegates the build happens to have. * * An XML payload also throws everywhere measured, but for a reason that varies with the * build - no SVG delegate, or a denied MVG coder, or MVG's own "must specify image * size" once the coders are installed and the policy opened. More importantly libmagic - * reads it as text/xml, and #41827 adds a pre-decode mime gate that denies text/*, so - * it would stop reaching the decode at all on that branch. These bytes read as - * application/octet-stream, which the assertion below pins so the drift is visible. + * reads it as text/xml, and #41834 adds a pre-decode mime gate denying text/*, so it + * would stop reaching the decode at all there. */ public function testClosesTheStreamWhenDecodingThrows(): void { $content = "\x00\x01\x02\x03 oc10-164 not an image \xff\xfe"; # Pin why this throws. Both assertions below are satisfied by any early return, so - # on #41827's tree a build whose libmagic called these bytes text/* would refuse + # on #41834's tree a build whose libmagic called these bytes text/* would refuse # them at the mime gate, keep this test green, and quietly stop covering the decode # this test is named for. $this->assertStringStartsWith( @@ -85,99 +85,46 @@ public function testClosesTheStreamWhenDecodingThrows(): void { $result = (new Photoshop())->getThumbnail($file, 32, 32, false); - # stream first, as in testClosesTheStreamOnSuccess: PHPUnit stops at the first - # failure, and the handle is the regression guard worth keeping + # stream first: PHPUnit stops at the first failure, and the handle is the + # regression guard worth keeping if the payload ever becomes decodable $this->assertFalse(\is_resource($stream), 'the stream must be closed once getThumbnail() returns'); $this->assertFalse($result, 'undecodable content must not produce a preview'); } /** - * Provider, mime type and content all agree, so the success path stays a success - * whether the provider decodes by pinning a coder (#41827) or by letting ImageMagick - * sniff one. + * Provider, declared mime type and content all agree, so the success path stays a + * success whether the provider decodes by pinning a coder (#41834) or by letting + * ImageMagick sniff one. * - * TIFF rather than PDF, and a blob this test writes itself rather than a fixture: PDF - * decoding needs the Ghostscript delegate and is denied by Debian's and Ubuntu's - * stock policy.xml, neither of which queryFormats() reports - so gating on a PDF - * would be red on a plain apt-installed ImageMagick rather than skipped. - * - * The stream is asserted before the decode result, so that an environment which - * cannot decode the blob still exercises the handle-release behaviour under test and - * reports the decode as the failure it is. + * The stream is asserted before the decode result for the same reason as above. */ public function testClosesTheStreamOnSuccess(): void { - list($file, $stream) = $this->makeFile($this->tiffBlob(), 'image/tiff'); + list($file, $stream) = $this->makeFile($this->psdBlob(), 'application/x-photoshop'); - $result = (new TIFF())->getThumbnail($file, 32, 32, false); + $result = (new Photoshop())->getThumbnail($file, 32, 32, false); - # outcome-neutral message: this assertion runs first, so it also fires where the - # decode did not succeed, and must not then claim the leak was on the success path $this->assertFalse(\is_resource($stream), 'the stream must be closed once getThumbnail() returns'); - $this->assertNotFalse($result, 'a TIFF should decode through the TIFF provider'); + $this->assertNotFalse($result, 'a PSD should decode through the Photoshop provider'); } /** - * Skips when this build has no TIFF support, and is loud about everything else. - * - * That line is deliberate rather than convenient. A skip costs the success-path - * fclose() assertion, and a guard quietly withholding these assertions is how they - * came to never run in CI in the first place - so an absent feature may skip, while a - * broken setup (revoked coder rights, an unparsable policy.xml, a wand that will not - * construct) has to be visible. - * - * It takes two checks, because neither sees the other's case. Depending on how - * ImageMagick was built, a missing libtiff either leaves TIFF unregistered - which - * queryFormats() catches - or leaves it registered and failing later at the delegate, - * which the message check catches. A policy denial says "not allowed by the security - * policy" instead and stays loud. - * - * Known limit: a module- or coder-domain policy denial can itself surface as - * MissingDelegateError, textually identical to an absent delegate, so such a build - * skips. Rather than guess, this only rejects messages naming a policy outright. + * A PSD this build writes itself, so nothing here depends on a fixture. * - * TIFF rather than PDF because no stock policy revokes it, whereas Debian and Ubuntu - * deny PDF out of the box. An allowlist-style policy.xml denying all coders bar a - * handful would still fail here - an accepted cost, since being loud about a - * misconfiguration is the point. - * - * Both directions are exercised because writing a TIFF does not establish that one can - * be read back, and reading is what the assertion needs. Measured: with TIFF coder - * rights revoked, getImageBlob() still returned a blob and only the read-back raised. - * - * This deliberately does not try to prove the whole path - getThumbnail() also needs - * PNG encoding and GD to load the result. Both are hard product requirements - * (composer.json requires ext-gd) and PNG output underpins every ownCloud preview, so - * a build failing those should fail this test rather than skip it. + * PSD rather than TIFF, PDF or SVG because ImageMagick implements it natively: + * coders/psd.so links no image library, while coders/tiff.so links libtiff, PDF needs + * the Ghostscript delegate that Debian and Ubuntu deny by default, and SVG needs a + * renderer that owncloudci/php:8.3 does not have at all. So this needs no availability + * guard and cannot skip - and it must not skip, because that would silently retire the + * success-path fclose() assertion, which is the entire subject of this file. A guard + * quietly withholding these assertions is how the OC10-164 preview tests came to never + * run in CI in the first place. */ - private function tiffBlob(): string { - if (\count(\Imagick::queryFormats('TIFF')) === 0) { - $this->markTestSkipped('This ImageMagick build registers no TIFF coder'); - } + private function psdBlob(): string { $image = new \Imagick(); try { $image->newImage(64, 48, new \ImagickPixel('white')); - $image->setImageFormat('tiff'); - $blob = $image->getImageBlob(); - - $probe = new \Imagick(); - try { - $probe->readImageBlob($blob); - } finally { - $probe->clear(); - } - return $blob; - } catch (\Exception $e) { - # \Exception rather than \ImagickException: ImagickPixelException extends - # \Exception directly and is a sibling, not a subclass - $message = $e->getMessage(); - $missingDelegate = \stripos($message, 'no encode delegate') !== false - || \stripos($message, 'no decode delegate') !== false; - $namesAPolicy = \stripos($message, 'policy') !== false - || \stripos($message, 'not authorized') !== false; - if ($missingDelegate && !$namesAPolicy) { - $this->markTestSkipped('This ImageMagick build has no TIFF delegate: ' . $message); - } - throw $e; + $image->setImageFormat('psd'); + return $image->getImageBlob(); } finally { $image->clear(); } @@ -187,12 +134,11 @@ private function tiffBlob(): string { * A storage that cannot open the file returns false rather than throwing, and * stream_get_contents(false) raises a TypeError - an \Error, so it would escape the * \Exception handler in getThumbnail() and surface as a 500 instead of a missing - * preview. + * preview. No mime type is stubbed here on purpose: this returns before one is read. */ public function testReturnsFalseWhenTheFileCannotBeOpened(): void { $file = $this->createMock(File::class); $file->method('fopen')->willReturn(false); - $file->method('getMimeType')->willReturn('application/x-photoshop'); $file->method('getSize')->willReturn(1024); $file->method('getPath')->willReturn('/test/unopenable'); From eb8292a9a336c6d99131eee93048c7ea39537dd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 16:17:37 +0200 Subject: [PATCH 11/14] test: assert the gate's own condition, and stub the third mock's mime type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two narrow corrections. The pin on the payload's detected type asserted one exact classification, application/octet-stream, while the gate it protects only refuses text/*, image/svg*, application/xml and image/x-mvg. A libmagic that matched these bytes to some other binary magic entry would still reach the decode exactly as intended and fail the assertion, which is the build-dependence this file has been shedding. It now mirrors isDangerousToDecode()'s own condition. The mime type is also stubbed on the cannot-be-opened mock, so that case does not depend on where in getThumbnail() the mime type is first read. That stub does not make the file runnable on a tree without #41835's fopen guard, and the comment no longer claims it does - measured on #41827's branch, which carries neither that guard nor the finally, the suite reports 1 error and 1 failure because every case here asserts what those two added. Failing there is correct, and it is why this lands on master rather than folded into #41834: CI builds the head-into-base merge commit, which always contains both. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index 03e3e24d6ff..c8b217c9241 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -76,10 +76,17 @@ public function testClosesTheStreamWhenDecodingThrows(): void { # on #41834's tree a build whose libmagic called these bytes text/* would refuse # them at the mime gate, keep this test green, and quietly stop covering the decode # this test is named for. - $this->assertStringStartsWith( - 'application/octet-stream', - \OC::$server->getMimeTypeDetector()->detectString($content), - 'the payload must not read as text, or the decode is never reached on the pinned branch' + # Mirrors isDangerousToDecode() rather than pinning one exact classification: any + # binary type reaches the decode, so asserting "octet-stream" specifically would + # fail on a libmagic that matched these bytes to some other binary magic entry + # while the behaviour under test was still correct. + $detected = \strtolower(\trim(\explode(';', \OC::$server->getMimeTypeDetector()->detectString($content), 2)[0])); + $refusedBeforeDecoding = \strpos($detected, 'text/') === 0 + || \strpos($detected, 'image/svg') === 0 + || \in_array($detected, ['application/xml', 'image/x-mvg'], true); + $this->assertFalse( + $refusedBeforeDecoding, + "the payload must reach the decode rather than the pre-decode mime gate, got: $detected" ); list($file, $stream) = $this->makeFile($content, 'application/x-photoshop'); @@ -134,11 +141,20 @@ private function psdBlob(): string { * A storage that cannot open the file returns false rather than throwing, and * stream_get_contents(false) raises a TypeError - an \Error, so it would escape the * \Exception handler in getThumbnail() and surface as a 500 instead of a missing - * preview. No mime type is stubbed here on purpose: this returns before one is read. + * preview. + * + * The mime type is stubbed even though the guard returns before reading it, so that the + * case does not depend on where in getThumbnail() the mime type is first touched. + * + * It does not make the file runnable on a tree without that guard, and nothing can: + * every case here asserts behaviour the guard and the finally introduced, so on a + * branch predating them they fail by design. That is why this belongs on master rather + * than folded into #41834 - CI builds the head-into-base merge, which always has both. */ public function testReturnsFalseWhenTheFileCannotBeOpened(): void { $file = $this->createMock(File::class); $file->method('fopen')->willReturn(false); + $file->method('getMimeType')->willReturn('application/x-photoshop'); $file->method('getSize')->willReturn(1024); $file->method('getPath')->willReturn('/test/unopenable'); From 05dfb1abf9d45b20ac37c4bf85799af128c1ee40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 16:22:52 +0200 Subject: [PATCH 12/14] docs: scope the master-only claim, and name the deny-list this mirrors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two comment corrections, no code change. "Every case here asserts behaviour the guard and the finally introduced" is wrong for testClosesTheStreamOnSuccess: fclose() on the success path predates #41835, which only moved it into the finally, so that case passes on a tree without either. The measurement already said so - one error and one failure on #41834's branch, two cases and not three - and the claim should have been scoped to those two. The pre-decode check is also now attributed to its source, OC\Preview\Bitmap:: isDangerousToDecode(), which #41834 adds and which is private and so cannot be called from a test. Mirroring it is still preferable to pinning one exact libmagic classification, but the duplication has a cost worth stating: if that deny-list gains an entry, this copy must gain it too, or the payload starts being refused at the gate while the assertion stays green and the decode goes uncovered. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index c8b217c9241..980251245a5 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -76,10 +76,16 @@ public function testClosesTheStreamWhenDecodingThrows(): void { # on #41834's tree a build whose libmagic called these bytes text/* would refuse # them at the mime gate, keep this test green, and quietly stop covering the decode # this test is named for. - # Mirrors isDangerousToDecode() rather than pinning one exact classification: any - # binary type reaches the decode, so asserting "octet-stream" specifically would - # fail on a libmagic that matched these bytes to some other binary magic entry - # while the behaviour under test was still correct. + # Mirrors the deny-list in OC\Preview\Bitmap::isDangerousToDecode(), which #41834 + # adds and which is private, so it cannot be called from here. Mirroring beats + # pinning one exact classification: any binary type reaches the decode, so asserting + # "octet-stream" specifically would fail on a libmagic that matched these bytes to + # another binary magic entry while the behaviour under test was still correct. + # + # The copy is the cost. If that deny-list gains an entry - its own comment + # anticipates more text-ish types - this has to gain it too, or the payload starts + # being refused at the gate while this assertion stays green and the decode goes + # uncovered. Grep for isDangerousToDecode when changing either. $detected = \strtolower(\trim(\explode(';', \OC::$server->getMimeTypeDetector()->detectString($content), 2)[0])); $refusedBeforeDecoding = \strpos($detected, 'text/') === 0 || \strpos($detected, 'image/svg') === 0 @@ -146,10 +152,13 @@ private function psdBlob(): string { * The mime type is stubbed even though the guard returns before reading it, so that the * case does not depend on where in getThumbnail() the mime type is first touched. * - * It does not make the file runnable on a tree without that guard, and nothing can: - * every case here asserts behaviour the guard and the finally introduced, so on a - * branch predating them they fail by design. That is why this belongs on master rather - * than folded into #41834 - CI builds the head-into-base merge, which always has both. + * It does not make the file runnable on a tree without that guard. This case and the + * undecodable one assert what the guard and the finally introduced, so on a branch + * predating them they fail by design - measured on #41834's branch: one error, one + * failure. testClosesTheStreamOnSuccess is not among them, since fclose() on the + * success path predates #41835, which only moved it into the finally. That is why this + * belongs on master rather than folded into #41834: CI builds the head-into-base merge + * commit, which always carries both. */ public function testReturnsFalseWhenTheFileCannotBeOpened(): void { $file = $this->createMock(File::class); From f27769a8580cdec1af1d48e6a47338232cd9fcf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 16:32:42 +0200 Subject: [PATCH 13/14] docs: name the change that actually drifts, and drop merge-strategy prose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more comment corrections. The maintenance note pointed the wrong maintainer at the mirror. A new text/ entry in isDangerousToDecode()'s deny-list is already matched by the text/ prefix here, so mirroring it would be busywork; the change that actually drifts is an entry of the application/xml or image/x-mvg shape, which the prefix does not catch. It now says that. isDangerousToDecode()'s own comment also enumerates what it already covers rather than anticipating additions, so that clause is gone. The claim that CI building the head-into-base merge commit is why this belongs on master rather than folded into #41834 was a non-sequitur - that same fact means folding it in would have been green too, since the failures only appear on the bare branch. The real reason is that the branch tree lacks #41835 and so cannot run the file locally, which the surrounding lines already say. Merge-strategy reasoning does not belong in a test docblock in any case; it goes in the pull request. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index 980251245a5..c722f46a313 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -82,10 +82,11 @@ public function testClosesTheStreamWhenDecodingThrows(): void { # "octet-stream" specifically would fail on a libmagic that matched these bytes to # another binary magic entry while the behaviour under test was still correct. # - # The copy is the cost. If that deny-list gains an entry - its own comment - # anticipates more text-ish types - this has to gain it too, or the payload starts - # being refused at the gate while this assertion stays green and the decode goes - # uncovered. Grep for isDangerousToDecode when changing either. + # The copy is the cost, and only one kind of change drifts: an entry the `text/` + # prefix below does not already match, i.e. another of the application/xml or + # image/x-mvg shape. Add such an entry there without adding it here and the payload + # can start being refused at the gate while this assertion stays green, leaving the + # decode uncovered. Grep for isDangerousToDecode when changing either. $detected = \strtolower(\trim(\explode(';', \OC::$server->getMimeTypeDetector()->detectString($content), 2)[0])); $refusedBeforeDecoding = \strpos($detected, 'text/') === 0 || \strpos($detected, 'image/svg') === 0 @@ -156,9 +157,7 @@ private function psdBlob(): string { * undecodable one assert what the guard and the finally introduced, so on a branch * predating them they fail by design - measured on #41834's branch: one error, one * failure. testClosesTheStreamOnSuccess is not among them, since fclose() on the - * success path predates #41835, which only moved it into the finally. That is why this - * belongs on master rather than folded into #41834: CI builds the head-into-base merge - * commit, which always carries both. + * success path predates #41835, which only moved it into the finally. */ public function testReturnsFalseWhenTheFileCannotBeOpened(): void { $file = $this->createMock(File::class); From 11dc0f610220e2364f903f743907ebfd907d493a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 16:35:50 +0200 Subject: [PATCH 14/14] docs: state the drift rule against all three of the mirrored rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous wording named only the text/ prefix and treated a drifting deny-list entry as necessarily an exact match. The mirror has three rules, and an entry written as a prefix - application/postscript alongside the existing image/svg, say - drifts just as badly while a reader following that wording concludes no mirroring is needed. It also over-warned in the other direction: an added image/svg+xml is not matched by text/ but is already matched by the image/svg prefix, so it does not drift. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index c722f46a313..f4e6f9c93c8 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -82,11 +82,11 @@ public function testClosesTheStreamWhenDecodingThrows(): void { # "octet-stream" specifically would fail on a libmagic that matched these bytes to # another binary magic entry while the behaviour under test was still correct. # - # The copy is the cost, and only one kind of change drifts: an entry the `text/` - # prefix below does not already match, i.e. another of the application/xml or - # image/x-mvg shape. Add such an entry there without adding it here and the payload - # can start being refused at the gate while this assertion stays green, leaving the - # decode uncovered. Grep for isDangerousToDecode when changing either. + # The copy is the cost. What drifts is a deny-list entry that none of the three rules + # below already match, whether it is written as an exact match or as a prefix. Add + # one there without adding it here and the payload can start being refused at the + # gate while this assertion stays green, leaving the decode uncovered. Grep for + # isDangerousToDecode when changing either. $detected = \strtolower(\trim(\explode(';', \OC::$server->getMimeTypeDetector()->detectString($content), 2)[0])); $refusedBeforeDecoding = \strpos($detected, 'text/') === 0 || \strpos($detected, 'image/svg') === 0