Skip to content

fix: pin the Imagick coder per provider without a temporary file (OC10-164) - #41834

Merged
oc-tmueller merged 7 commits into
fix/oc10-164-bitmap-preview-arbitrary-file-writefrom
fix/oc10-164-in-memory-coder-pin
Sep 16, 2026
Merged

oc-tmueller merged 7 commits into
fix/oc10-164-bitmap-preview-arbitrary-file-writefrom
fix/oc10-164-in-memory-coder-pin

Conversation

@oc-tmueller

Copy link
Copy Markdown
Contributor

Summary

Implements Florian's coder-pinning fix for OC10-164 entirely in memory. Replaces #41832, which does the same pinning but needs a temporary file per preview (and a new /dev/shm-backed ITempManager::getRamTemporaryFile() to make that affordable).

Stacked on #41827's branch rather than master, since it touches the same files. This is a sibling of #41832, not a follow-up — #41832 will be closed.

Why #41832 needed a temp file, and why it turns out it doesn't

#41832 pins via readImage('TIFF:/path'), which needs a real filesystem path, because setFormat() + readImageBlob() appeared to pin correctly but skip rasterization — getImageBlob() handed back the original, undecoded bytes.

That diagnosis was wrong. 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.

The fix is one extra call — setFormat('png') on the wand alongside setImageFormat('png') on the image.

Verified on two builds, comparing pinned geometry against an unpinned read as ground truth:

ImageMagick 6.9.11-60 / imagick 3.8.1 / PHP 8.3 ImageMagick 7.1.1-36 / imagick 3.7.0 / PHP 7.4
tiff/psd/sgi/ai/heic/ttf decode geometry matches unpinned geometry matches unpinned
MVG / MSL / PostScript via a foreign pin rejected rejected

Why removing the temp file matters

/dev/shm is 64 MB by default in both owncloud/server:10.15.0 and owncloudci/php:8.3 (measured), deployments routinely set it smaller, and preview_max_filesize_image defaults to 50 MB. A full tmpfs makes file_put_contents() short-write, and readImage('TIFF:…') frequently succeeds on a truncated TIFF/PSD/SGI stream — so a partially rendered image would be written to the preview cache and served from then on. There is no way to detect that from is_dir()/is_writable(), since a full tmpfs at mode 1777 is still writable.

Not writing the file at all removes that failure mode rather than hardening it, and with it the ramtempdirectory config surface, the OCP\ITempManager addition (a BC break in a patch release), and the tmpfs leak that cleanOld() could not sweep.

What changed

  • Bitmap::getResizedPreview() takes the file's own mime type, calls setFormat($this->getImagickFormat($mimeType)) before readImageBlob(), and resets both output formats afterwards.
  • New abstract protected getImagickFormat(string $mimeType): string with one implementation per provider.
  • SVG.php gets the same pin, Office.php keeps pinning through its constructor argument.
  • Deliberately not guarded by queryFormats() in Bitmap: if a build does not register a provider's coder, throwing is correct — the only alternative is the content-sniffing the pin exists to prevent. SVG.php is guarded, because a build with no SVG coder cannot decode SVG either way and what it pins is DOMSanitizer output, not raw bytes.
  • Heic pins HEIC for both image/heic and image/heif. They are one container handled by one coder module, and pinning HEIF broke .heif previews on every build that registers only HEIC — including owncloudci/php:8.3.

Diff is +89/-4 across 11 source files, and it is PHP 7.4-clean, so the pending 10.16 backport (#41828) needs no syntax changes.

Tests

CoderPinningTest asserts both halves of the pin, with six new fixtures (tests/data had no .ai/.heic/.psd/.sgi/.tiff/.ttf sample at all). The HEIC fixture is AVIF-encoded on purpose — ImageMagick classifies the avif brand as HEIC, and an HEVC sample needs a libde265 delegate that is not present everywhere.

These skips are now per-coder, which fixes a real gap. The tests this file is modelled on gated every case 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, so the whole file skipped and the assertions never ran in CI. Each case now requires only the coder it exercises. On owncloudci/php:8.3 the result is 15 tests executing rather than skipping, including the image/heif case. SanitizeTest's guard likewise moves to the PDF/TTF coders its providers actually use; it deliberately does not require an SVG coder, since the point of those cases is that the content never reaches Imagick.

Known residuals (unchanged, pre-existing)

  • AI/PDF/EPS accept PostScript content — same Ghostscript family, so it is not foreign to them. Bounded to those three providers and mitigated by fix: harden ImageMagick policy and install rsvg-convert (OC10-164) owncloud-docker/php#309's policy.xml, which denies MVG/MSL/MSVG regardless of entry point.
  • TTF/PFB accept non-font bytes, but the TTF coder is what runs: the output is the same 800x480 font specimen sheet a real TTF produces, so there is no coder handoff. Covered by testFontNeverInvokesADangerousCoderForForeignContent.
  • abstract protected getImagickFormat() is a load-time fatal for any out-of-tree OC\Preview\Bitmap subclass. Bitmap is lib/private, and the compile-time guarantee that every provider declares its coder seems worth more — flagging it as the one deliberate BC risk.

Verification

  • tests/lib/Preview/ + TempManagerTest + PreviewManagerTest on a fresh owncloudci/php:8.3: 73 tests, 171 assertions, 0 failures. The only skips are pre-existing PDFTest/SVGTest ones.
  • php-cs-fixer with the ownCloud standard: 0 of 41 files need fixing.
  • php -l under PHP 7.4 for every changed file.
  • Pin matrix re-run against the patched mapping on both builds: all formats to PNG, MVG/MSL/PostScript rejected by every non-Ghostscript pin.

oc-tmueller and others added 3 commits September 15, 2026 21:52
isDangerousToDecode() (af3c147) 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) <noreply@anthropic.com>
Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
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) <noreply@anthropic.com>
Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
…-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) <noreply@anthropic.com>
Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
@oc-tmueller
oc-tmueller requested a review from a team as a code owner September 15, 2026 20:21
@update-docs

update-docs Bot commented Sep 15, 2026

Copy link
Copy Markdown

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.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
oc-tmueller and others added 2 commits September 16, 2026 12:09
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) <noreply@anthropic.com>
Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
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) <noreply@anthropic.com>
Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
…(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<timestamp> 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) <noreply@anthropic.com>
Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
@oc-tmueller
oc-tmueller merged commit d828f95 into fix/oc10-164-bitmap-preview-arbitrary-file-write Sep 16, 2026
28 checks passed
@oc-tmueller
oc-tmueller deleted the fix/oc10-164-in-memory-coder-pin branch September 16, 2026 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant