fix: pin the Imagick coder per provider without a temporary file (OC10-164) - #41834
Open
oc-tmueller wants to merge 7 commits into
Open
oc-tmueller wants to merge 7 commits into
oc-tmueller wants to merge 7 commits into
Conversation
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>
|
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>
5 tasks
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-backedITempManager::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, becausesetFormat()+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, sogetImageBlob()was faithfully re-encoding back to the pinned input format, andsetImageFormat('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 alongsidesetImageFormat('png')on the image.Verified on two builds, comparing pinned geometry against an unpinned read as ground truth:
Why removing the temp file matters
/dev/shmis 64 MB by default in bothowncloud/server:10.15.0andowncloudci/php:8.3(measured), deployments routinely set it smaller, andpreview_max_filesize_imagedefaults to 50 MB. A full tmpfs makesfile_put_contents()short-write, andreadImage('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 fromis_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
ramtempdirectoryconfig surface, theOCP\ITempManageraddition (a BC break in a patch release), and the tmpfs leak thatcleanOld()could not sweep.What changed
Bitmap::getResizedPreview()takes the file's own mime type, callssetFormat($this->getImagickFormat($mimeType))beforereadImageBlob(), and resets both output formats afterwards.abstract protected getImagickFormat(string $mimeType): stringwith one implementation per provider.SVG.phpgets the same pin,Office.phpkeeps pinning through its constructor argument.queryFormats()inBitmap: 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.phpis guarded, because a build with no SVG coder cannot decode SVG either way and what it pins is DOMSanitizer output, not raw bytes.HeicpinsHEICfor bothimage/heicandimage/heif. They are one container handled by one coder module, and pinningHEIFbroke.heifpreviews on every build that registers onlyHEIC— includingowncloudci/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
CoderPinningTestasserts both halves of the pin, with six new fixtures (tests/datahad no.ai/.heic/.psd/.sgi/.tiff/.ttfsample at all). The HEIC fixture is AVIF-encoded on purpose — ImageMagick classifies theavifbrand 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" — butowncloudci/php:8.3registers no SVG coder, so the whole file skipped and the assertions never ran in CI. Each case now requires only the coder it exercises. Onowncloudci/php:8.3the result is 15 tests executing rather than skipping, including theimage/heifcase.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/EPSaccept 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'spolicy.xml, which denies MVG/MSL/MSVG regardless of entry point.TTF/PFBaccept 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 bytestFontNeverInvokesADangerousCoderForForeignContent.abstract protected getImagickFormat()is a load-time fatal for any out-of-treeOC\Preview\Bitmapsubclass.Bitmapislib/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+PreviewManagerTeston a freshowncloudci/php:8.3: 73 tests, 171 assertions, 0 failures. The only skips are pre-existingPDFTest/SVGTestones.php-cs-fixerwith the ownCloud standard: 0 of 41 files need fixing.php -lunder PHP 7.4 for every changed file.