fix: pin the Imagick coder per provider instead of letting it sniff (OC10-164)

[oc-tmueller]

Summary

Follow-up to #41827 (this PR's base branch - stacked, not against master, since it touches the same files). Closes the residual bypass class discussed on OC10-164: mechanism-only writeup at #41827 (comment).

isDangerousToDecode() is a deny-list over the libmagic-sniffed type, but the decode that follows re-derives the format independently: readImageBlob()/readImage() with no format set consults Imagick's own ~130-entry magic table. application/postscript/application/pdf are deliberately not denied (Postscript/PDF need them), so 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, regardless of which provider read them.

Confirmed directly against the patched code: libmagic classifies "%!PS-Adobe-3.0..." as application/postscript (not denied by the mime gate), and a bare readImageBlob() on that content picks the PS coder - independent of which provider's mime-type check let it through.

Changes

• getImagickFormat(string $mimeType): string on every Bitmap subclass, mapping each provider's own detected mime type(s) to the exact Imagick coder it needs.
• Bitmap::getResizedPreview() now reads through a temporary file with a "FORMAT:path" prefix rather than a bare blob read - SVG.php and Office.php (the only other two Imagick read paths in core) get the same treatment.

The pin has to be path-based, not setFormat()+readImageBlob(). I initially implemented it that way (matching the literal shape suggested in the OC10-164 discussion), and it does reliably reject a mismatched format - but for every coder pinned here it also silently skips the actual rasterization step: setImageFormat('png') ends up with no effect and getImageBlob() returns the original, undecoded source bytes. Confirmed for all eight Bitmap subclasses, not just the lazily-rendered ones (fonts, HEIC) where I first suspected it. Path-based reads (new Imagick('FORMAT:' . $path) / readImage('FORMAT:' . $path)) don't have this problem and decode correctly for all of them.

Known, accepted residual gap. PDF/Postscript/Illustrator are the Ghostscript-backed providers PostScript-ish content is not foreign to, and Ghostscript does not respect the pin the way the other coders do - so pinning doesn't close cross-coder confusion for those three specifically. This is bounded to their own domain (feeding PostScript to the PDF/Postscript/Illustrator providers isn't "foreign", it's their whole purpose) and mitigated separately by the ImageMagick policy shipped in owncloud-docker/php#309, which denies the MSL/MVG/MSVG coders regardless of entry point.

Font's failure mode for foreign content is a safe blank placeholder, not a hard reject. FreeType fails to parse non-font bytes and Imagick returns an empty canvas without ever reaching a script coder or delegate - so it has its own, separately-worded test rather than sharing the assertFalse() used for SGI/Photoshop/TIFF/Heic.

Adds tests/data/testimage.{ai,heic,psd,sgi,tiff,ttf} - genuine samples of each format, since none existed before this PR.

Sequencing

Per the discussion on OC10-164: independent follow-up, not a blocker for #41827. The 10.16 backport (#41828) stays unmerged until this approach is agreed here first - no changes made there in this PR.

Test plan

• make test-php-style
• make test-php-unit TEST_PHP_SUITE=tests/lib/Preview/ - 72 tests, 226 assertions, 9 skipped (unrelated: no Movie/Office provider in this environment, plus one HEIF-coder-specific skip - see note below), 0 failures
• New CoderPinningTest: legitimate content still decodes for all 8 Bitmap subclasses (real fixtures, not synthetic), and PostScript-shaped content is rejected when fed to a non-Ghostscript-backed provider
• SanitizeTest, SVGSanitizeTest, SVGTest, PDFTest, BitmapTest all still pass
• OfficeTest is skipped in my environment (no LibreOffice binary) - verified the 'PDF:' . $path . '[0]' syntax directly against a real PDF instead (subimage selector + format prefix combination Imagick handles correctly), but the actual LibreOffice-generated-PDF path is unverified by an automated test here
• Note: one HEIF-specific test case is conditionally skipped where the CI ImageMagick build lacks a distinct HEIF coder from HEIC - confirmed separately that the real owncloud/server image (built on Ubuntu + libheif1) registers both as distinct coders (identify -list format)

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[oc-tmueller]

Code review

Found 1 issue:

1. SVG.php pins the coder using the disk-backed getTemporaryFile() instead of the new RAM-backed getRamTemporaryFile() that Bitmap.php uses for the identical write-then-read-then-unlink scratch file. Before this PR that path was a pure in-memory readImageBlob($output), so every SVG preview now does a real disk write + read + unlink per request — the opposite of what changelog/unreleased/41832 claims ("using a tmpfs-backed temporary file ... so the pin adds no disk I/O to preview generation") and of the new ramtempdirectory docs in config/config.sample.php ("currently just bitmap/vector preview generation"). It also means the RAM-backed rationale that closed fix: replace Imagick coder pinning with a magic-byte pre-check (OC10-164) #41833 only holds for the bitmap path, not the vector one.

core/lib/private/Preview/SVG.php

Lines 65 to 67 in 34769f5

	# silently skips the actual rasterization step, same as in Bitmap.php.
	$tmpPath = \OC::$server->getTempManager()->getTemporaryFile();
	\file_put_contents($tmpPath, $output);

Compare the bitmap path, which does use the tmpfs-backed call:

core/lib/private/Preview/Bitmap.php

Lines 114 to 116 in 34769f5

	# Imagick a path to pin against and never needs to survive on disk.
	$tmpPath = \OC::$server->getTempManager()->getRamTemporaryFile();
	\file_put_contents($tmpPath, $content);

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[oc-tmueller]

Performance: disk-backed vs. RAM-backed coder pinning

Measured Bitmap::getResizedPreview() directly (via reflection, warm-up discarded) across three states, same container, sequential runs, testimage.pdf, 200 iterations each:

Scenario	mean	median	p95	min	max
(a) before pinning (baseline, 51ca68bd2c)	152.2ms	125.9ms	311.4ms	69.2ms	1237.1ms
(b) this PR as first written, disk-backed pin (a5408ab8ed)	297.3ms	242.9ms	676.3ms	101.7ms	1166.4ms
(c) current state, RAM-backed pin (tmpfs via getRamTemporaryFile())	183.4ms	127.2ms	462.3ms	70.6ms	2409.8ms

The disk-backed temp file roughly doubled mean/median latency vs. the unpinned baseline. The RAM-backed version brings median and min essentially back to baseline (127ms vs. 126ms, 70.6ms vs. 69.2ms) - the coder pin itself is no longer the dominant cost.

Caveats:

• Docker's page cache makes (b)'s disk numbers a lower bound - a real, uncached production disk would be slower, widening the gap this fix closes.
• PDF-via-Ghostscript's own process startup dominates the tail latency (p95/max) for all three scenarios, so the disk-vs-RAM signal is clearest in median/min rather than mean/max.
• A planned testimage.tiff secondary comparison was dropped: that fixture doesn't exist at the pre-pinning baseline commit, so a fair three-way comparison wasn't possible there.

[oc-tmueller]

Closing in favour of #41834, which implements the same coder pin without a temporary file.

The reason this PR needed one was the note in its own commit message and code comments: that setFormat() + readImageBlob() pins correctly but "silently skips the actual rasterization step", so getImageBlob() returns the original undecoded bytes. That diagnosis was wrong, and I only found out when looking for a way to avoid /dev/shm.

setFormat() does fully decode. It also sets the wand's output format, so getImageBlob() was faithfully re-encoding back to the pinned input format, and setImageFormat('png') alone could not override it. For TIFF and SGI that re-encode is byte-identical to the input, which is exactly why it looked like untouched passthrough. PSD was the tell: 15016 bytes out for a 14988-byte input. Adding one call — setFormat('png') on the wand — makes the in-memory pin work end to end, verified against unpinned geometry as ground truth on ImageMagick 6.9.11-60/imagick 3.8.1/PHP 8.3 and 7.1.1-36/imagick 3.7.0/PHP 7.4.

That removes the whole reason for getRamTemporaryFile(), and with it the concerns raised on this PR:

• /dev/shm is 64 MB by default in both owncloud/server and owncloudci/php (measured), deployments routinely set it lower, and preview_max_filesize_image defaults to 50 MB. A full tmpfs short-writes, and readImage('TIFF:…') frequently succeeds on a truncated TIFF/PSD/SGI stream — so a corrupt preview would have been written to the preview cache and served from then on, undetectable via is_dir()/is_writable().
• No new public OCP\ITempManager method, so no BC break in a patch release and nothing new to add to a maintenance branch for the 10.16 backport.
• The diff drops from +396/-14 to +89/-4 and is PHP 7.4-clean, so fix: prevent arbitrary file write via unsanitized SVG/MVG bitmap previews [10.16] (OC10-164) #41828 should backport without syntax changes.

#41834 also fixes something this PR's tests got wrong: they gated every case on Imagick::queryFormats('SVG') as a proxy for "this build has the extended coder set", but owncloudci/php:8.3 registers no SVG coder at all — so CoderPinningTest skipped in its entirety and its security assertions never actually ran in CI. Guards are now per-coder, and 15 tests execute on that image.

Two review points from here are carried over to #41834: Heic now pins HEIC for both image/heic and image/heif (pinning HEIF broke .heif previews on every build without a distinct HEIF coder), and the abstract protected getImagickFormat() BC risk is called out explicitly in the new PR description.

No changes are needed to #41827, and #41828 stays held per the agreed sequencing.