fix: pin the Imagick coder per provider without a temporary file (OC10-164) - #41834
Merged
oc-tmueller merged 7 commits intoSep 16, 2026
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>
oc-tmueller
merged commit Sep 16, 2026
d828f95
into
fix/oc10-164-bitmap-preview-arbitrary-file-write
28 checks passed
oc-tmueller
added a commit
that referenced
this pull request
Sep 22, 2026
* test: stub the mime type in BitmapStreamTest so it survives the coder pin 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) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: decode a self-written TIFF rather than gating on the PDF coder 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) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: probe the TIFF read path, not just the write path, before asserting 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 "<?xml" and then fails on a document with no <svg> root. The comment now says that, and warns that another XML payload is not automatically substitutable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: skip only when TIFF is absent, and assert the stream before the decode 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) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: separate an absent TIFF delegate from a denied one by ImageMagick's message 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) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: keep both TIFF guards, since neither covers the other's case 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) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: fail the undecodable case on bytes no coder claims The payload was '<?xml version="1.0"?><notanimage>x</notanimage>', which is not environment-independent. ImageMagick's IsSVG() claims any blob opening with "<?xml", so readImageBlob() reported "no decode delegate for this image format `SVG'" - it threw only because these images register no SVG renderer. Where librsvg or the internal MSVG renderer is present, the lenient parser returns a blank canvas rather than throwing, and the case would fail for reasons unrelated to the stream. That is the same environment coupling this file has been shedding elsewhere; the failure path had it too. It now uses bytes no coder claims. ImageMagick sniffs the format as "" and fails with "no decode delegate for this image format `'" on every build regardless of which delegates are compiled in. libmagic reads them as application/octet-stream rather than text, so they also survive #41827's mime gate and still reach the decode on that branch instead of being turned away earlier. Verified in a container: the old payload sniffs as SVG, the new one as "", and both the master tree and the tree merged with #41827 stay green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: correct the recorded reasons in BitmapStreamTest's comments 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) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: pin why the undecodable case throws, and assert the handle first 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) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: decode a PSD, dropping the TIFF availability guard entirely 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) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: assert the gate's own condition, and stub the third mock's mime type 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) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: scope the master-only claim, and name the deny-list this mirrors 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) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: name the change that actually drifts, and drop merge-strategy prose 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) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: state the drift rule against all three of the mirrored rules 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) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --------- Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
oc-tmueller
added a commit
that referenced
this pull request
Sep 22, 2026
…0-164) (#41834) * fix: pin the Imagick coder per bitmap preview provider (OC10-164) 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> * fix: pin the Imagick coder for SVG previews too (OC10-164) SVG::getThumbnail() is the one Imagick read path in core that is not a Bitmap provider, and it had the same gap: ImagickFactory sets svg:sanitize, svg:embed and svg:decode, but the read that follows let Imagick pick the coder from the content, so those options could be reasoning about a different coder than the one that ran. Pin SVG explicitly, and reset both the image and wand output formats to png32 afterwards for the same reason as Bitmap.php - setFormat() pins the output format as well, so setImageFormat() alone would leave getImageBlob() re-encoding back to SVG. Unlike Bitmap.php the pin is guarded by queryFormats(). A build that registers no SVG coder cannot be pinned to it and cannot decode SVG at all either way, so throwing would trade a clear "no decode delegate" failure for a confusing "Unable to set format" one; owncloudci/php:8.3 is such a build. The value at risk is also lower here: what gets pinned is DOMSanitizer's serialized output, not the raw file bytes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: cover Imagick coder pinning with real per-format fixtures (OC10-164) Adds CoderPinningTest, which asserts both halves of the pin: every provider still decodes its own format, and PostScript content is rejected by the providers it is foreign to (SGI, Photoshop, TIFF, Heic) rather than being handed to the Ghostscript delegate by ImageMagick's own content-sniffing. Six fixtures had to be added - tests/data had no .ai/.heic/.psd/.sgi/.tiff/ .ttf sample at all, so there was nothing to decode per provider. The HEIC fixture is AVIF-encoded on purpose: ImageMagick classifies the avif brand as HEIC, and an HEVC-encoded sample needs a libde265 delegate that is not present everywhere. Skips are per-coder rather than blanket. The tests these are modelled on gated on Imagick::queryFormats('SVG') as a stand-in for "this build has the extended coder set", but owncloudci/php:8.3 registers no SVG coder at all, so that guard skipped every case and the assertions never ran in CI. Each case now requires only the one coder it exercises, which is also why the image/heif case runs here: it pins HEIC, so it no longer depends on a distinct HEIF coder being registered. testPinnedDecodeReturnsPngAndNotThePinnedInputFormat covers the one non-obvious part of the mechanism - setFormat() pins the output format as well, and for TIFF the re-encode is byte-identical to the input, so dropping the second setFormat() call would be easy to reintroduce and hard to notice. SanitizeTest needs the mime type plumbed through, since providers now pin based on it. Its skip guard moves to the PDF/TTF coders its two providers actually use - it deliberately does not require an SVG coder, because the whole point of those cases is that the content never reaches Imagick. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: add changelog entry for the OC10-164 in-memory coder pin (#41834) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: report build-dependent coder-pinning results correctly (OC10-164) CoderPinningTest reported the wrong thing on any ImageMagick build differing from the one it was written against, which matters for the pending PHP 7.4 backport: tests/phpunit-autotest.xml sets failOnRisky, and PHPUnit 9.6 defaults beStrictAboutTestsThatDoNotTestAnything to true, so a test executing zero assertions is a hard failure rather than a warning. testFontNeverInvokesADangerousCoderForForeignContent kept its only assertion inside `if ($result !== false)`. On any build where FreeType refuses the PostScript payload outright - the safest outcome, and the one the test exists to assert about - it executed no assertion at all and failed as risky. Both outcomes now collapse into one branch-free assertion. requireCoder() proves a coder is registered, not that the delegate behind it can decode a given fixture. coders/heic.c registers HEIC, HEIF and AVIF whenever libheif is present, but decoding the AVIF fixture additionally needs an AV1 decoder inside libheif, so a build without one failed instead of skipping. requireDecodableFixture() reads the fixture unpinned first and skips when the build cannot decode those bytes at all, since the pinned read failing then says nothing about the pin. The fixture stays AVIF-branded deliberately: an AVIF-branded file served by the Heic provider, which pins HEIC for it, is exactly the case worth a real sample. The negative tests could also pass for the wrong reason. isDangerousToDecode() is a deny-list over the sniffed type and it denies text/*, so a libmagic build reporting the payload as text/plain would reject it at that gate before the coder pin ever ran. assertPayloadReachesTheCoderPin() asserts the sniffed type, so such a build fails loudly with an actionable message instead of passing vacuously. The payload itself was duplicated in both tests and is now a constant. Both fixtures the Font and Illustrator cases used are replaced by files already in the tree. testimage.ttf was Microsoft Verdana, carrying an "All Rights Reserved" notice and a trademark notice, so the Font case now reads the Apache-2.0 core/fonts/OpenSans-Regular.ttf instead - same sfnt tag, same DSIG table, same coder path. testimage.ai was byte-identical to testimage.pdf, and ImageMagick's AI coder is a Ghostscript alias for the PDF one, so the Illustrator case reads testimage.pdf directly. Fixture paths now resolve through OC::$SERVERROOT, the existing idiom in tests/lib. CoderPinningTest and SanitizeTest both call Imagick::queryFormats() unguarded, which raises a class-not-found Error rather than skipping on a build without ext-imagick. Both get the @requires annotation the neighbouring provider tests already use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: gate preview tests on the coder each provider pins (OC10-164) Three neighbouring preview tests guarded on the wrong thing, in ways the coder pin makes load-bearing. PDFTest gated on Imagick::queryFormats('SVG'), a coder the PDF provider never touches. On any build registering no SVG coder - owncloudci/php:8.3 among them - all four cases skipped while reporting "No PDF provider present", so the PDF preview assertions never ran in CI even though the PDF coder was present. It now requires PDF, the coder PDF::getImagickFormat() actually pins. SVGTest names the right coder but compared the count to exactly 1, which skips whenever a build registers SVG alongside SVGZ or MSVG. It now checks for zero, matching the idiom the rest of the directory uses. BitmapTest had no coder guard at all. It drives Postscript against testimage.eps, which now hard-requires the EPS coder rather than reaching one through ImageMagick's own sniffing, so on a reduced build it would fail instead of skipping. It now requires EPS. SanitizeTest's guard goes the other way and is removed entirely. isDangerousToDecode() rejects that content before ImagickFactory::create() and before setFormat(), so those eight cases never reach a coder - requiring PDF and TTF could only ever let a reduced build skip the OC10-164 regression assertions silently, which is the failure mode this whole series is trying to remove. The changelog entry also now records that pinning costs previews for files whose extension does not match their content, since media types come from the extension. That is the intended trade-off, but it is user-visible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: cover decoding when the stored mime type is not the provider's (OC10-164) The coder pin reads $file->getMimeType(), not the mime type that selected the provider. Those differ whenever a caller overrides the selection type through getThumbnail(['mimeType' => ...]): apps/files_trashbin/ajax/preview.php does, because a trashed file's .d<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> --------- Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
oc-tmueller
added a commit
that referenced
this pull request
Sep 23, 2026
…0-164) (#41834) * fix: pin the Imagick coder per bitmap preview provider (OC10-164) 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> * fix: pin the Imagick coder for SVG previews too (OC10-164) SVG::getThumbnail() is the one Imagick read path in core that is not a Bitmap provider, and it had the same gap: ImagickFactory sets svg:sanitize, svg:embed and svg:decode, but the read that follows let Imagick pick the coder from the content, so those options could be reasoning about a different coder than the one that ran. Pin SVG explicitly, and reset both the image and wand output formats to png32 afterwards for the same reason as Bitmap.php - setFormat() pins the output format as well, so setImageFormat() alone would leave getImageBlob() re-encoding back to SVG. Unlike Bitmap.php the pin is guarded by queryFormats(). A build that registers no SVG coder cannot be pinned to it and cannot decode SVG at all either way, so throwing would trade a clear "no decode delegate" failure for a confusing "Unable to set format" one; owncloudci/php:8.3 is such a build. The value at risk is also lower here: what gets pinned is DOMSanitizer's serialized output, not the raw file bytes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: cover Imagick coder pinning with real per-format fixtures (OC10-164) Adds CoderPinningTest, which asserts both halves of the pin: every provider still decodes its own format, and PostScript content is rejected by the providers it is foreign to (SGI, Photoshop, TIFF, Heic) rather than being handed to the Ghostscript delegate by ImageMagick's own content-sniffing. Six fixtures had to be added - tests/data had no .ai/.heic/.psd/.sgi/.tiff/ .ttf sample at all, so there was nothing to decode per provider. The HEIC fixture is AVIF-encoded on purpose: ImageMagick classifies the avif brand as HEIC, and an HEVC-encoded sample needs a libde265 delegate that is not present everywhere. Skips are per-coder rather than blanket. The tests these are modelled on gated on Imagick::queryFormats('SVG') as a stand-in for "this build has the extended coder set", but owncloudci/php:8.3 registers no SVG coder at all, so that guard skipped every case and the assertions never ran in CI. Each case now requires only the one coder it exercises, which is also why the image/heif case runs here: it pins HEIC, so it no longer depends on a distinct HEIF coder being registered. testPinnedDecodeReturnsPngAndNotThePinnedInputFormat covers the one non-obvious part of the mechanism - setFormat() pins the output format as well, and for TIFF the re-encode is byte-identical to the input, so dropping the second setFormat() call would be easy to reintroduce and hard to notice. SanitizeTest needs the mime type plumbed through, since providers now pin based on it. Its skip guard moves to the PDF/TTF coders its two providers actually use - it deliberately does not require an SVG coder, because the whole point of those cases is that the content never reaches Imagick. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: add changelog entry for the OC10-164 in-memory coder pin (#41834) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: report build-dependent coder-pinning results correctly (OC10-164) CoderPinningTest reported the wrong thing on any ImageMagick build differing from the one it was written against, which matters for the pending PHP 7.4 backport: tests/phpunit-autotest.xml sets failOnRisky, and PHPUnit 9.6 defaults beStrictAboutTestsThatDoNotTestAnything to true, so a test executing zero assertions is a hard failure rather than a warning. testFontNeverInvokesADangerousCoderForForeignContent kept its only assertion inside `if ($result !== false)`. On any build where FreeType refuses the PostScript payload outright - the safest outcome, and the one the test exists to assert about - it executed no assertion at all and failed as risky. Both outcomes now collapse into one branch-free assertion. requireCoder() proves a coder is registered, not that the delegate behind it can decode a given fixture. coders/heic.c registers HEIC, HEIF and AVIF whenever libheif is present, but decoding the AVIF fixture additionally needs an AV1 decoder inside libheif, so a build without one failed instead of skipping. requireDecodableFixture() reads the fixture unpinned first and skips when the build cannot decode those bytes at all, since the pinned read failing then says nothing about the pin. The fixture stays AVIF-branded deliberately: an AVIF-branded file served by the Heic provider, which pins HEIC for it, is exactly the case worth a real sample. The negative tests could also pass for the wrong reason. isDangerousToDecode() is a deny-list over the sniffed type and it denies text/*, so a libmagic build reporting the payload as text/plain would reject it at that gate before the coder pin ever ran. assertPayloadReachesTheCoderPin() asserts the sniffed type, so such a build fails loudly with an actionable message instead of passing vacuously. The payload itself was duplicated in both tests and is now a constant. Both fixtures the Font and Illustrator cases used are replaced by files already in the tree. testimage.ttf was Microsoft Verdana, carrying an "All Rights Reserved" notice and a trademark notice, so the Font case now reads the Apache-2.0 core/fonts/OpenSans-Regular.ttf instead - same sfnt tag, same DSIG table, same coder path. testimage.ai was byte-identical to testimage.pdf, and ImageMagick's AI coder is a Ghostscript alias for the PDF one, so the Illustrator case reads testimage.pdf directly. Fixture paths now resolve through OC::$SERVERROOT, the existing idiom in tests/lib. CoderPinningTest and SanitizeTest both call Imagick::queryFormats() unguarded, which raises a class-not-found Error rather than skipping on a build without ext-imagick. Both get the @requires annotation the neighbouring provider tests already use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: gate preview tests on the coder each provider pins (OC10-164) Three neighbouring preview tests guarded on the wrong thing, in ways the coder pin makes load-bearing. PDFTest gated on Imagick::queryFormats('SVG'), a coder the PDF provider never touches. On any build registering no SVG coder - owncloudci/php:8.3 among them - all four cases skipped while reporting "No PDF provider present", so the PDF preview assertions never ran in CI even though the PDF coder was present. It now requires PDF, the coder PDF::getImagickFormat() actually pins. SVGTest names the right coder but compared the count to exactly 1, which skips whenever a build registers SVG alongside SVGZ or MSVG. It now checks for zero, matching the idiom the rest of the directory uses. BitmapTest had no coder guard at all. It drives Postscript against testimage.eps, which now hard-requires the EPS coder rather than reaching one through ImageMagick's own sniffing, so on a reduced build it would fail instead of skipping. It now requires EPS. SanitizeTest's guard goes the other way and is removed entirely. isDangerousToDecode() rejects that content before ImagickFactory::create() and before setFormat(), so those eight cases never reach a coder - requiring PDF and TTF could only ever let a reduced build skip the OC10-164 regression assertions silently, which is the failure mode this whole series is trying to remove. The changelog entry also now records that pinning costs previews for files whose extension does not match their content, since media types come from the extension. That is the intended trade-off, but it is user-visible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: cover decoding when the stored mime type is not the provider's (OC10-164) The coder pin reads $file->getMimeType(), not the mime type that selected the provider. Those differ whenever a caller overrides the selection type through getThumbnail(['mimeType' => ...]): apps/files_trashbin/ajax/preview.php does, because a trashed file's .d<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> --------- Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
oc-tmueller
added a commit
that referenced
this pull request
Sep 24, 2026
…0-164) (#41834) * fix: pin the Imagick coder per bitmap preview provider (OC10-164) 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> * fix: pin the Imagick coder for SVG previews too (OC10-164) SVG::getThumbnail() is the one Imagick read path in core that is not a Bitmap provider, and it had the same gap: ImagickFactory sets svg:sanitize, svg:embed and svg:decode, but the read that follows let Imagick pick the coder from the content, so those options could be reasoning about a different coder than the one that ran. Pin SVG explicitly, and reset both the image and wand output formats to png32 afterwards for the same reason as Bitmap.php - setFormat() pins the output format as well, so setImageFormat() alone would leave getImageBlob() re-encoding back to SVG. Unlike Bitmap.php the pin is guarded by queryFormats(). A build that registers no SVG coder cannot be pinned to it and cannot decode SVG at all either way, so throwing would trade a clear "no decode delegate" failure for a confusing "Unable to set format" one; owncloudci/php:8.3 is such a build. The value at risk is also lower here: what gets pinned is DOMSanitizer's serialized output, not the raw file bytes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: cover Imagick coder pinning with real per-format fixtures (OC10-164) Adds CoderPinningTest, which asserts both halves of the pin: every provider still decodes its own format, and PostScript content is rejected by the providers it is foreign to (SGI, Photoshop, TIFF, Heic) rather than being handed to the Ghostscript delegate by ImageMagick's own content-sniffing. Six fixtures had to be added - tests/data had no .ai/.heic/.psd/.sgi/.tiff/ .ttf sample at all, so there was nothing to decode per provider. The HEIC fixture is AVIF-encoded on purpose: ImageMagick classifies the avif brand as HEIC, and an HEVC-encoded sample needs a libde265 delegate that is not present everywhere. Skips are per-coder rather than blanket. The tests these are modelled on gated on Imagick::queryFormats('SVG') as a stand-in for "this build has the extended coder set", but owncloudci/php:8.3 registers no SVG coder at all, so that guard skipped every case and the assertions never ran in CI. Each case now requires only the one coder it exercises, which is also why the image/heif case runs here: it pins HEIC, so it no longer depends on a distinct HEIF coder being registered. testPinnedDecodeReturnsPngAndNotThePinnedInputFormat covers the one non-obvious part of the mechanism - setFormat() pins the output format as well, and for TIFF the re-encode is byte-identical to the input, so dropping the second setFormat() call would be easy to reintroduce and hard to notice. SanitizeTest needs the mime type plumbed through, since providers now pin based on it. Its skip guard moves to the PDF/TTF coders its two providers actually use - it deliberately does not require an SVG coder, because the whole point of those cases is that the content never reaches Imagick. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: add changelog entry for the OC10-164 in-memory coder pin (#41834) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: report build-dependent coder-pinning results correctly (OC10-164) CoderPinningTest reported the wrong thing on any ImageMagick build differing from the one it was written against, which matters for the pending PHP 7.4 backport: tests/phpunit-autotest.xml sets failOnRisky, and PHPUnit 9.6 defaults beStrictAboutTestsThatDoNotTestAnything to true, so a test executing zero assertions is a hard failure rather than a warning. testFontNeverInvokesADangerousCoderForForeignContent kept its only assertion inside `if ($result !== false)`. On any build where FreeType refuses the PostScript payload outright - the safest outcome, and the one the test exists to assert about - it executed no assertion at all and failed as risky. Both outcomes now collapse into one branch-free assertion. requireCoder() proves a coder is registered, not that the delegate behind it can decode a given fixture. coders/heic.c registers HEIC, HEIF and AVIF whenever libheif is present, but decoding the AVIF fixture additionally needs an AV1 decoder inside libheif, so a build without one failed instead of skipping. requireDecodableFixture() reads the fixture unpinned first and skips when the build cannot decode those bytes at all, since the pinned read failing then says nothing about the pin. The fixture stays AVIF-branded deliberately: an AVIF-branded file served by the Heic provider, which pins HEIC for it, is exactly the case worth a real sample. The negative tests could also pass for the wrong reason. isDangerousToDecode() is a deny-list over the sniffed type and it denies text/*, so a libmagic build reporting the payload as text/plain would reject it at that gate before the coder pin ever ran. assertPayloadReachesTheCoderPin() asserts the sniffed type, so such a build fails loudly with an actionable message instead of passing vacuously. The payload itself was duplicated in both tests and is now a constant. Both fixtures the Font and Illustrator cases used are replaced by files already in the tree. testimage.ttf was Microsoft Verdana, carrying an "All Rights Reserved" notice and a trademark notice, so the Font case now reads the Apache-2.0 core/fonts/OpenSans-Regular.ttf instead - same sfnt tag, same DSIG table, same coder path. testimage.ai was byte-identical to testimage.pdf, and ImageMagick's AI coder is a Ghostscript alias for the PDF one, so the Illustrator case reads testimage.pdf directly. Fixture paths now resolve through OC::$SERVERROOT, the existing idiom in tests/lib. CoderPinningTest and SanitizeTest both call Imagick::queryFormats() unguarded, which raises a class-not-found Error rather than skipping on a build without ext-imagick. Both get the @requires annotation the neighbouring provider tests already use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: gate preview tests on the coder each provider pins (OC10-164) Three neighbouring preview tests guarded on the wrong thing, in ways the coder pin makes load-bearing. PDFTest gated on Imagick::queryFormats('SVG'), a coder the PDF provider never touches. On any build registering no SVG coder - owncloudci/php:8.3 among them - all four cases skipped while reporting "No PDF provider present", so the PDF preview assertions never ran in CI even though the PDF coder was present. It now requires PDF, the coder PDF::getImagickFormat() actually pins. SVGTest names the right coder but compared the count to exactly 1, which skips whenever a build registers SVG alongside SVGZ or MSVG. It now checks for zero, matching the idiom the rest of the directory uses. BitmapTest had no coder guard at all. It drives Postscript against testimage.eps, which now hard-requires the EPS coder rather than reaching one through ImageMagick's own sniffing, so on a reduced build it would fail instead of skipping. It now requires EPS. SanitizeTest's guard goes the other way and is removed entirely. isDangerousToDecode() rejects that content before ImagickFactory::create() and before setFormat(), so those eight cases never reach a coder - requiring PDF and TTF could only ever let a reduced build skip the OC10-164 regression assertions silently, which is the failure mode this whole series is trying to remove. The changelog entry also now records that pinning costs previews for files whose extension does not match their content, since media types come from the extension. That is the intended trade-off, but it is user-visible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: cover decoding when the stored mime type is not the provider's (OC10-164) The coder pin reads $file->getMimeType(), not the mime type that selected the provider. Those differ whenever a caller overrides the selection type through getThumbnail(['mimeType' => ...]): apps/files_trashbin/ajax/preview.php does, because a trashed file's .d<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> --------- Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
oc-tmueller
added a commit
that referenced
this pull request
Sep 24, 2026
…iews (OC10-164) (#41827) * fix: prevent arbitrary file write via unsanitized SVG/MVG bitmap previews (OC10-164) Bitmap::getResizedPreview() sanitized SVG content before handing it to Imagick::readImageBlob(), but fell back to the ORIGINAL, unsanitized bytes whenever the sanitizer returned an empty string - which it does for any content libxml cannot parse, not just for genuinely malformed SVG. A malformed SVG (or any non-XML payload such as a raw MVG script) therefore reached ImageMagick unsanitized, where an <image xlink:href="MSL:..."> or an MVG "fill 'url(...)'" primitive can execute an MSL script that reads and writes arbitrary files as the web user (CVSS 8.8). Bitmap providers (PDF, Font, Postscript, ...) only ever need to decode real bitmap/vector image formats, never SVG or script-shaped text content - that belongs exclusively to the dedicated SVG provider. getResizedPreview() now rejects any content whose libmagic-detected media type is text/*, image/svg+xml, application/xml, or image/x-mvg before ever calling into Imagick, instead of trying to sanitize and falling back on failure. It also now goes through ImagickFactory::create() so the svg:sanitize/svg:embed/ svg:decode hardening options apply here as they already did in the SVG provider. SVG::sanitizeSVGContent() return type changes from string to ?string so it can report "could not sanitize" (null) separately from "sanitized to an empty document" (''); its own provider now bails out on null instead of silently passing empty content to Imagick. The removal of the sanitize-with-fallback path in Bitmap changes the behaviour asserted by SanitizeTest: SVG content fed to a Bitmap provider (PDF, Font) now yields false instead of a rendered PNG, since Bitmap providers no longer attempt to handle SVG-shaped content at all. Added regression cases for a malformed SVG with an MSL xlink:href, a raw MVG script, and a well-formed SVG - all must return false from a Bitmap provider. Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: add changelog entry for OC10-164 bitmap preview fix (#41827) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: match "image/svg" without the +xml suffix in the OC10-164 mime gate Confirmed while testing the 10.16 backport: PHP 7.4's bundled fileinfo extension reports the same SVG content as "image/svg", not "image/svg+xml" - the exact-match check silently let it through on that runtime while still catching it on PHP 8.3. Match by prefix instead so the gate added in af3c147 ("fix: prevent arbitrary file write via unsanitized SVG/MVG bitmap previews (OC10-164)") is not dependent on which libmagic build a given PHP runtime happens to link. Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: pin the Imagick coder per provider without a temporary file (OC10-164) (#41834) * fix: pin the Imagick coder per bitmap preview provider (OC10-164) 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> * fix: pin the Imagick coder for SVG previews too (OC10-164) SVG::getThumbnail() is the one Imagick read path in core that is not a Bitmap provider, and it had the same gap: ImagickFactory sets svg:sanitize, svg:embed and svg:decode, but the read that follows let Imagick pick the coder from the content, so those options could be reasoning about a different coder than the one that ran. Pin SVG explicitly, and reset both the image and wand output formats to png32 afterwards for the same reason as Bitmap.php - setFormat() pins the output format as well, so setImageFormat() alone would leave getImageBlob() re-encoding back to SVG. Unlike Bitmap.php the pin is guarded by queryFormats(). A build that registers no SVG coder cannot be pinned to it and cannot decode SVG at all either way, so throwing would trade a clear "no decode delegate" failure for a confusing "Unable to set format" one; owncloudci/php:8.3 is such a build. The value at risk is also lower here: what gets pinned is DOMSanitizer's serialized output, not the raw file bytes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: cover Imagick coder pinning with real per-format fixtures (OC10-164) Adds CoderPinningTest, which asserts both halves of the pin: every provider still decodes its own format, and PostScript content is rejected by the providers it is foreign to (SGI, Photoshop, TIFF, Heic) rather than being handed to the Ghostscript delegate by ImageMagick's own content-sniffing. Six fixtures had to be added - tests/data had no .ai/.heic/.psd/.sgi/.tiff/ .ttf sample at all, so there was nothing to decode per provider. The HEIC fixture is AVIF-encoded on purpose: ImageMagick classifies the avif brand as HEIC, and an HEVC-encoded sample needs a libde265 delegate that is not present everywhere. Skips are per-coder rather than blanket. The tests these are modelled on gated on Imagick::queryFormats('SVG') as a stand-in for "this build has the extended coder set", but owncloudci/php:8.3 registers no SVG coder at all, so that guard skipped every case and the assertions never ran in CI. Each case now requires only the one coder it exercises, which is also why the image/heif case runs here: it pins HEIC, so it no longer depends on a distinct HEIF coder being registered. testPinnedDecodeReturnsPngAndNotThePinnedInputFormat covers the one non-obvious part of the mechanism - setFormat() pins the output format as well, and for TIFF the re-encode is byte-identical to the input, so dropping the second setFormat() call would be easy to reintroduce and hard to notice. SanitizeTest needs the mime type plumbed through, since providers now pin based on it. Its skip guard moves to the PDF/TTF coders its two providers actually use - it deliberately does not require an SVG coder, because the whole point of those cases is that the content never reaches Imagick. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: add changelog entry for the OC10-164 in-memory coder pin (#41834) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: report build-dependent coder-pinning results correctly (OC10-164) CoderPinningTest reported the wrong thing on any ImageMagick build differing from the one it was written against, which matters for the pending PHP 7.4 backport: tests/phpunit-autotest.xml sets failOnRisky, and PHPUnit 9.6 defaults beStrictAboutTestsThatDoNotTestAnything to true, so a test executing zero assertions is a hard failure rather than a warning. testFontNeverInvokesADangerousCoderForForeignContent kept its only assertion inside `if ($result !== false)`. On any build where FreeType refuses the PostScript payload outright - the safest outcome, and the one the test exists to assert about - it executed no assertion at all and failed as risky. Both outcomes now collapse into one branch-free assertion. requireCoder() proves a coder is registered, not that the delegate behind it can decode a given fixture. coders/heic.c registers HEIC, HEIF and AVIF whenever libheif is present, but decoding the AVIF fixture additionally needs an AV1 decoder inside libheif, so a build without one failed instead of skipping. requireDecodableFixture() reads the fixture unpinned first and skips when the build cannot decode those bytes at all, since the pinned read failing then says nothing about the pin. The fixture stays AVIF-branded deliberately: an AVIF-branded file served by the Heic provider, which pins HEIC for it, is exactly the case worth a real sample. The negative tests could also pass for the wrong reason. isDangerousToDecode() is a deny-list over the sniffed type and it denies text/*, so a libmagic build reporting the payload as text/plain would reject it at that gate before the coder pin ever ran. assertPayloadReachesTheCoderPin() asserts the sniffed type, so such a build fails loudly with an actionable message instead of passing vacuously. The payload itself was duplicated in both tests and is now a constant. Both fixtures the Font and Illustrator cases used are replaced by files already in the tree. testimage.ttf was Microsoft Verdana, carrying an "All Rights Reserved" notice and a trademark notice, so the Font case now reads the Apache-2.0 core/fonts/OpenSans-Regular.ttf instead - same sfnt tag, same DSIG table, same coder path. testimage.ai was byte-identical to testimage.pdf, and ImageMagick's AI coder is a Ghostscript alias for the PDF one, so the Illustrator case reads testimage.pdf directly. Fixture paths now resolve through OC::$SERVERROOT, the existing idiom in tests/lib. CoderPinningTest and SanitizeTest both call Imagick::queryFormats() unguarded, which raises a class-not-found Error rather than skipping on a build without ext-imagick. Both get the @requires annotation the neighbouring provider tests already use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: gate preview tests on the coder each provider pins (OC10-164) Three neighbouring preview tests guarded on the wrong thing, in ways the coder pin makes load-bearing. PDFTest gated on Imagick::queryFormats('SVG'), a coder the PDF provider never touches. On any build registering no SVG coder - owncloudci/php:8.3 among them - all four cases skipped while reporting "No PDF provider present", so the PDF preview assertions never ran in CI even though the PDF coder was present. It now requires PDF, the coder PDF::getImagickFormat() actually pins. SVGTest names the right coder but compared the count to exactly 1, which skips whenever a build registers SVG alongside SVGZ or MSVG. It now checks for zero, matching the idiom the rest of the directory uses. BitmapTest had no coder guard at all. It drives Postscript against testimage.eps, which now hard-requires the EPS coder rather than reaching one through ImageMagick's own sniffing, so on a reduced build it would fail instead of skipping. It now requires EPS. SanitizeTest's guard goes the other way and is removed entirely. isDangerousToDecode() rejects that content before ImagickFactory::create() and before setFormat(), so those eight cases never reach a coder - requiring PDF and TTF could only ever let a reduced build skip the OC10-164 regression assertions silently, which is the failure mode this whole series is trying to remove. The changelog entry also now records that pinning costs previews for files whose extension does not match their content, since media types come from the extension. That is the intended trade-off, but it is user-visible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: cover decoding when the stored mime type is not the provider's (OC10-164) The coder pin reads $file->getMimeType(), not the mime type that selected the provider. Those differ whenever a caller overrides the selection type through getThumbnail(['mimeType' => ...]): apps/files_trashbin/ajax/preview.php does, because a trashed file's .d<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> --------- Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: keep a missing stored mime type from turning a preview into a 500 Pinning the Imagick coder made $file->getMimeType() load-bearing: it now feeds getResizedPreview()'s string-typed $mimeType parameter. That value comes from FileInfo::getMimetype(), which returns whatever Cache::get() stored, and that in turn is MimeTypeLoader::getMimetypeById() - documented to return null for an id with no row in oc_mimetypes. Nothing constrains oc_filecache.mimetype with a foreign key, so a dangling id is reachable. Uncast, such a file raised a TypeError. A TypeError is an \Error, so it escaped getThumbnail()'s catch (\Exception) and surfaced as a 500 instead of degrading to a media-type icon - the same failure mode the unchecked fopen() had. Casting keeps the degradation graceful without weakening the pin: seven of the eight providers return a constant coder and ignore the argument entirely, and Font, the only one that branches on it, falls through to TTF exactly as it already does for a trashed .pfb reporting application/octet-stream. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: make the preview gate tests fail when the gate is gone Two independent weaknesses, both found by re-reviewing this range rather than by any failure. SanitizeTest could only half detect a reverted mime gate. Removing the gate left the four PDF cases passing: PDF pins a Ghostscript-backed coder, which rejects SVG and MVG bytes on its own, so the assertion held on the pin alone. Only the four Font cases failed, because the TTF coder renders arbitrary bytes as a specimen sheet and so yields a preview. That left the two providers whose pin offers no protection - the Ghostscript-backed ones, exactly where the gate is the only defence - with no coverage at all. Fixed by adding a payload that is denied by the gate yet decodable by the pin: PostScript with its %!PS-Adobe header sniffs as application/postscript, which the gate has to allow so real ones still preview, but drop the header and libmagic reports text/plain while an affirmed PDF:/EPS: pin still hands it to Ghostscript. Verified both directions - the two new cases pass with the gate and fail without it, taking the revert detectors from four of eight to six of ten. Also added a control assertion mirroring the deny-list, so a build whose libmagic classifies one of these payloads differently fails with an actionable message instead of going quiet, and renamed the parameter from $svgContent, which no longer describes what it carries. PDFTest and BitmapTest gated on Imagick::queryFormats(), which reports only that a coder is registered. coders/pdf.c and coders/ps.c register PDF, AI and EPS unconditionally and wire the Ghostscript delegate separately, and MagickQueryFormats() never consults policy.xml - which on stock Debian and Ubuntu denies the PDF/PS/EPS/XPS coders. On either build the guard answered "present" and the cases failed where they meant to skip. Both now probe the fixture they actually decode, via a shared helper on Provider. That helper is requireDecodableFixtureFile(), named for the path it takes so it cannot be confused with CoderPinningTest's blob-taking equivalent: handing one a path would throw inside the probe and be converted into a skip, silently retiring a test. It reads the fixture outside the probe, so an unreadable one still fails, and catches \Throwable inside it, because imagick reports some delegate and policy conditions at warning severity and failOnWarning="true" turns those into PHPUnit warnings rather than ImagickException. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: give the Font coder-pin case an observable it can actually fail on testFontNeverInvokesADangerousCoderForForeignContent() asserted that the returned preview stayed under 2048 bytes, meaning to catch the PostScript payload coming back rendered by Ghostscript. It could not: OC_Image::data() re-encodes through GD, and the image has already been downscaled to fit 32x32 by then, so both the safe and the unsafe outcome land within a few hundred bytes of each other. Removing the coder pin left the case green. The thumbnail's shape does distinguish them. The payload now declares a portrait bounding box, ImageMagick's TTF coder draws a fixed 800x480 specimen sheet, and on owncloudci/php:8.3 that is 32x19 through the TTF pin against 25x32 with the pin removed. Verified both directions: green as shipped, and failing with the pin removed, which takes CoderPinningTest's detectors from four to five. The page stays blank - the bounding box is the whole difference from FOREIGN_POSTSCRIPT, since nothing here inspects a pixel. assertFalse() on the result would not work either: the placeholder FreeType returns for non-font bytes is a valid image on this build, so it would fail against a correctly pinned decode. Kept as one branch-free expression, because failOnRisky turns a zero-assertion test into a hard failure on any build that refuses the bytes. The case now also requires that this build can rasterize PostScript at all. Without Ghostscript, or under the stock Debian policy denying the PS coder, the pin-removed mutant throws instead of rendering, getThumbnail() returns false and the assertion would hold with no pin in place - green while protecting nothing. Both capability probes here and on Provider catch \Exception rather than \ImagickException. imagick reports some delegate and policy conditions at warning severity, and PHPUnit 9 turns PHP warnings into PHPUnit\Framework\Error\Warning through convertWarningsToExceptions, which defaults to true and is unset in tests/phpunit-autotest.xml; failOnWarning only decides whether an emitted warning fails the run. That class reaches \Exception via PHPUnit\Framework\Exception, so one catch covers both - and unlike \Throwable it still lets an \Error fail rather than become a green skip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: make the PostScript coder-pin cases fail when the pin is gone Three ways these cases could stay green with Bitmap::getResizedPreview()'s setFormat() pin removed. testFontNeverInvokesADangerousCoderForForeignContent() asserted the preview came back under 2048 bytes. OC_Image::data() re-encodes through GD after the image has been downscaled to fit 32x32, so the safe and unsafe outcomes both land within a few hundred bytes and the ceiling could never be crossed. Shape does separate them: ImageMagick's TTF coder draws a fixed 800x480 specimen sheet, landscape, while an unpinned read goes to the PS coder and rasterizes a full page, portrait - 32x19 against 25x32 on owncloudci/php:8.3. assertFalse() would not work either, since the placeholder FreeType returns for non-font bytes is a valid image here. That shape is a property of the build, not of the payload: measured, this build's PS coder ignores %%BoundingBox, EPSF-3.0 branding and setpagedevice alike and always renders 612x792. So requirePortraitPostScriptRender() now asserts it, and does so after reproducing the first-frame selection and bestfit downscale the assertion actually observes - a raster only slightly taller than wide collapses to exactly 32x32 there, which would have waved through a build that cannot discriminate. The payload keeps a portrait bounding box even so, because upstream coders/ps.c does derive Ghostscript's -g geometry from it on other builds, and a portrait box is portrait under both behaviours where a square one would silently stop exercising the pin. testRejectsPostScriptContentFromAForeignProvider() had no PostScript precondition at all. Under a policy.xml denying PS/EPS/PDF the unpinned read throws just as the pinned one does, so all four rows held with no pin in place. They now require a renderable payload first, via a plain capability helper the portrait one builds on; with the pin removed and PS denied the class skips 8 instead of falsely passing 4. FOREIGN_POSTSCRIPT_PORTRAIT is gone - one constant serves both tests again. Verified on owncloudci/php:8.3: 19 pass as shipped; 5 fail with the pin removed; 8 skip and none pass falsely with the pin removed and PS denied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: state which extensions the Imagick coder pin actually affects The entry named tif, psd, sgi, heic and ai - the most visible subset of the trade-off rather than its extent. The list now comes from resources/config/mimetypemapping.dist.json instead of memory, so image/sgi contributes bw, int, inta, rgb and rgba alongside sgi, and image/heif contributes heif. svgz is not mapped there at all, so it is deliberately absent. It also says which of those providers a stock install actually has. Only SGI and Heic are in PreviewManager's defaults; PDF, Postscript, Illustrator, Photoshop, TIFF and Font need enabling, so without that note the list reads as a much wider regression than most admins will see. Fonts get their own paragraph rather than being listed or omitted. The font coder accepts any bytes, so a mismatched .ttf still yields a thumbnail - just one the font coder drew instead of the file's content - and a real .otf gains a preview it never had, since an unpinned read has no decode delegate for CFF outlines. Neither "affected" nor "unaffected" describes that honestly. Office and SVG stay excluded: the Office pin covers the PDF LibreOffice just produced rather than user bytes, and non-XML content never reached a coder in SVG before this change either. Also softened the "no filesystem access" claim to what this change is responsible for. The pin adds no temporary file of its own, but it does not stop the Ghostscript-backed coders staging their own input, and the original wording read as a promise about the whole preview path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: always report a media type when detecting from file content Detection::detectString() is documented as returning a string, but returned finfo_buffer()'s value unchecked, which is typed string|false, and passed finfo_open()'s result to it without checking that either. Both matter to the bitmap preview media type gate added earlier in this pull request, which compares the detected type against a list it refuses to hand to ImageMagick: - finfo_open() returns false when libmagic cannot load its magic database. finfo_buffer() then raises a TypeError, and because that is an \Error rather than an \Exception it escapes the handler in Bitmap::getThumbnail(), so a preview failed the whole request with a server error instead of falling back to a media type icon. - a false from finfo_buffer() reached the gate as the empty string, which matches no entry in the list, so the content was admitted. Both now fall back to application/octet-stream. The finfo_open() warning is suppressed the same way finfo_file()'s already is a few lines above, since the fallback is what the caller acts on. The function_exists() guard also tested finfo_file(), which this branch never calls; it now tests finfo_buffer(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: apply the hardened Imagick options before decoding ImagickFactory::create() passed its argument to the Imagick constructor, which reads the image immediately, and only then set svg:sanitize, svg:embed and svg:decode. Anything loaded that way was therefore decoded with none of the hardening the options exist to provide - so the one form that takes a path was the only unhardened way through the factory, while the comment added to Office::getThumbnail() earlier in this pull request presents it as the hardened route. The instance is now always constructed empty, the options applied, and the file read afterwards. Verified in a container that this is behaviour preserving for the only caller that passes a path: Office's "PDF:<path>[0]" produces identical geometry and identical output bytes either way, a foreign image pinned to the PDF coder is still rejected, and readImage() does not pin the wand's output format the way setFormat() does - so setImageFormat() alone remains sufficient there. The parameter is narrowed from mixed to ?string at the same time. Imagick's constructor also accepts an array of paths, but no caller has ever passed one, and carrying that form would mean a second, untested branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: record what the bitmap preview media type gate depends on The gate can only be as good as the detection behind it, and that needs either ext-fileinfo or the "file" binary. Neither is actually required to run ownCloud: OC_Util::checkServer() does not list ext-fileinfo among its hard dependencies, and OC_Util::fileInfoLoaded() only raises a recommendation in the admin panel. On an install with neither, every payload is reported as application/octet-stream and the gate admits all of them, leaving the per-provider coder pin as the only remaining layer. Recorded rather than closed: failing closed here would cost every bitmap preview on a configuration ownCloud supports, and the condition pre-dates this check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: stop requiring imagick for the preview gate regression cases SanitizeTest carried a class level "@requires extension imagick" that contradicts the comment directly below it, which argues against guarding these cases so that a reduced build cannot silently skip the regression assertions. No case in the class reaches Imagick at all: the media type gate rejects every payload before ImagickFactory::create() is called. All ten cases still run and pass without it. Also drops TestCase::assertImage() and tests/lib/Preview/white-32x32.png. Both arrived together with an earlier change, and their only caller was the SanitizeTest assertion this pull request replaced, so nothing references either any more. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: changelog for the media type detection and coder pin follow-ups Records the two preview hardening fixes from this review round, and states the residual the coder pin deliberately leaves open: which provider serves a preview is steerable by the request, so a file can still be routed to the PDF coder whatever its content. That is not a change in behaviour - content sniffing reached the same coder before the pin - and a distribution's ImageMagick policy is the process-wide control for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: fall back to a media type when no temporary file can be written detectString()'s branch for installs without ext-fileinfo writes the content to a temporary file so that detect() can inspect it. It did not check either step: TempManager::getTemporaryFile() returns false when its directory is not writable, and fopen(false, ...) raises a ValueError on PHP 8. Being an \Error rather than an \Exception, that escapes the handler in Bitmap::getThumbnail(), so the request failed with a server error instead of falling back to a media type icon. Nothing in core called detectString() before the media type gate added in this pull request, so this branch was not previously reachable from a request. It now runs on every bitmap preview on such an install, which is what makes it worth closing alongside the finfo_open() guard above it - the same failure mode, on the sibling path. Not covered by a test: reaching this branch needs a build without ext-fileinfo, which cannot be simulated from within the test suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: justify the PostScript coder pin by measurement, not coder internals The comment argued that pinning EPS covers .ps because EPS and PS share ReadPSImage(). They do share it, but not unconditionally - it branches on the requested coder - so the comment asserted an equivalence it did not establish. Replaced with what was actually measured on the shipped image: plain PostScript and EPSF-tagged content, both declaring a bounding box smaller than the page, render to identical geometry read unpinned, pinned EPS and pinned PS. The pin therefore does not change what a .ps file previews as, which is the property the comment needed to support. No functional change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: do not let a disabled popen turn a preview into a server error detect() falls back to the "file" binary when nothing else identified the content, and OC_Helper::canExecute() only establishes that the binary exists - not that we are allowed to start a process. popen() is a common disable_functions entry, and on PHP 8 a disabled function is undefined, so calling it raises an \Error. popen() is also documented to return false when the process cannot be forked, which makes the fgets() and pclose() that follow raise a TypeError. All of those are \Error rather than \Exception, so they escape the handler in Bitmap::getThumbnail() and fail the request instead of degrading to a media type icon - the same hole the previous commit closed two frames up, on the path that commit routes into. An install without ext-fileinfo reaches this code on every bitmap preview, so the guard there was incomplete without this one. Verified that the guard is transparent when popen is available: the branch still returns the type the "file" binary reports. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: record which build the PostScript pin was measured on The rewritten comment rests the pin's safety on a measurement, so it should say where that measurement came from: ImageMagick 6.9.11-60 with Ghostscript 9.55.0, as shipped in owncloudci/php:8.3. Versions read from the build rather than recalled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --------- Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
oc-tmueller
added a commit
that referenced
this pull request
Sep 24, 2026
…iews [10.16] Backport of #41827. That PR carries the per-provider Imagick coder pin from #41834 as well, because #41834 was merged into its branch, so the squash commit on master contains both changes and so does this backport. Bitmap::getResizedPreview() sanitized SVG content before handing it to Imagick::readImageBlob(), but fell back to the ORIGINAL, unsanitized bytes whenever the sanitizer returned an empty string - which it does for any content libxml cannot parse, not only for genuinely malformed SVG. A malformed SVG, or any non-XML payload such as a raw MVG script, therefore reached ImageMagick unsanitized, where an <image xlink:href="MSL:..."> or an MVG "fill 'url(...)'" primitive can execute an MSL script that reads and writes arbitrary files as the web user. getResizedPreview() now rejects content whose libmagic-detected media type is text/*, image/svg*, application/xml or image/x-mvg before calling into Imagick at all, and each provider pins the exact coder it serves instead of letting ImageMagick re-derive the format from the content. Adapted for PHP 7.4, the only version 10.16 supports. Master justifies several of these guards by PHP 8 raising an \Error that escapes catch (\Exception); on 7.4 the same calls only warn, so every such claim was re-derived on the target runtime rather than carried over: - finfo_buffer(false, ...) warns and returns false on 7.4, and detectString() returned that false to the new deny-list, where it collapses to '' and matches no entry. Here the missing guard admitted the content the list exists to reject; it is on PHP 8 that it turns a missing preview into a 500. - the same holds for popen()/fgets()/pclose() in detect() and for fopen(false, ...) in the branch taken without ext-fileinfo. - the (string) cast on $file->getMimeType() is required on 7.4 too: passing null to a userland string-typed parameter is a TypeError on 7.4 as well. Measured, because the surrounding guards are not. 10.16 keeps its own "$stream === false" check and $image->loadFromData($bp); the is_resource() form and the (string) cast on that call are master-only, from #41855 and #41449, and the three-way merge preserved both correctly. The measurements the coder-pin comments rest on were re-taken on owncloudci/php:7.4, this branch's own CI image. It ships the same ImageMagick 6.9.11-60 and Ghostscript 9.55.0 as the 8.3 image and every figure reproduced: plain PostScript and EPSF-branded content both render 612x792 unpinned, pinned EPS and pinned PS alike; a %!PS-Adobe payload read unpinned reaches the PS coder at 612x792 while the TTF pin gives 800x480. That image also registers no SVG coder and no HEIF coder distinct from HEIC - which is precisely what PDFTest's old SVG-based guard got wrong and what Heic's single HEIC pin is there for. Verified in owncloudci/php:7.4: tests/lib/Preview/ plus tests/lib/Files/Type/DetectionTest.php at 68 tests / 207 assertions, against 41 tests / 123 assertions before, with 12 environment skips (Movie, Office, SVG). The PDF cases run here for the first time. php -l clean under 7.4 on all 21 changed files. 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
added a commit
that referenced
this pull request
Sep 25, 2026
…iews [10.16] (OC10-164) (#41863) * fix: prevent arbitrary file write via unsanitized SVG/MVG bitmap previews [10.16] Backport of #41827. That PR carries the per-provider Imagick coder pin from #41834 as well, because #41834 was merged into its branch, so the squash commit on master contains both changes and so does this backport. Bitmap::getResizedPreview() sanitized SVG content before handing it to Imagick::readImageBlob(), but fell back to the ORIGINAL, unsanitized bytes whenever the sanitizer returned an empty string - which it does for any content libxml cannot parse, not only for genuinely malformed SVG. A malformed SVG, or any non-XML payload such as a raw MVG script, therefore reached ImageMagick unsanitized, where an <image xlink:href="MSL:..."> or an MVG "fill 'url(...)'" primitive can execute an MSL script that reads and writes arbitrary files as the web user. getResizedPreview() now rejects content whose libmagic-detected media type is text/*, image/svg*, application/xml or image/x-mvg before calling into Imagick at all, and each provider pins the exact coder it serves instead of letting ImageMagick re-derive the format from the content. Adapted for PHP 7.4, the only version 10.16 supports. Master justifies several of these guards by PHP 8 raising an \Error that escapes catch (\Exception); on 7.4 the same calls only warn, so every such claim was re-derived on the target runtime rather than carried over: - finfo_buffer(false, ...) warns and returns false on 7.4, and detectString() returned that false to the new deny-list, where it collapses to '' and matches no entry. Here the missing guard admitted the content the list exists to reject; it is on PHP 8 that it turns a missing preview into a 500. - the same holds for popen()/fgets()/pclose() in detect() and for fopen(false, ...) in the branch taken without ext-fileinfo. - the (string) cast on $file->getMimeType() is required on 7.4 too: passing null to a userland string-typed parameter is a TypeError on 7.4 as well. Measured, because the surrounding guards are not. 10.16 keeps its own "$stream === false" check and $image->loadFromData($bp); the is_resource() form and the (string) cast on that call are master-only, from #41855 and #41449, and the three-way merge preserved both correctly. The measurements the coder-pin comments rest on were re-taken on owncloudci/php:7.4, this branch's own CI image. It ships the same ImageMagick 6.9.11-60 and Ghostscript 9.55.0 as the 8.3 image and every figure reproduced: plain PostScript and EPSF-branded content both render 612x792 unpinned, pinned EPS and pinned PS alike; a %!PS-Adobe payload read unpinned reaches the PS coder at 612x792 while the TTF pin gives 800x480. That image also registers no SVG coder and no HEIF coder distinct from HEIC - which is precisely what PDFTest's old SVG-based guard got wrong and what Heic's single HEIC pin is there for. Verified in owncloudci/php:7.4: tests/lib/Preview/ plus tests/lib/Files/Type/DetectionTest.php at 68 tests / 207 assertions, against 41 tests / 123 assertions before, with 12 environment skips (Movie, Office, SVG). The PDF cases run here for the first time. php -l clean under 7.4 on all 21 changed files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: make BitmapStreamTest survive the OC10-164 coder pin [10.16] Backport of #41838. Without it the previous commit leaves this file red: testClosesTheStreamOnSuccess fed a PNG through the Photoshop provider, and the PSD pin refuses exactly that mismatch. The fixture is replaced by a PSD Imagick writes itself, so the case needs no fixture and cannot skip - it must not, since the success-path fclose() assertion is the whole subject of the file. The undecodable payload becomes bytes no coder claims, with a control assertion mirroring isDangerousToDecode(): both existing assertions are satisfied by any early return, so a build whose libmagic read the old payload as text/* would have had it refused at the new mime gate, stayed green, and silently stopped covering the decode. Measured on owncloudci/php:7.4 as application/octet-stream, which does reach the decode. Diverges from master in three places, all because this branch is PHP 7.4: - the unopenable-file case keeps its 10.16 shape, asserting that no warning is emitted. Master asserts the return value, which cannot fail here: stream_get_contents(false) only warns on 7.4 and the result is false either way, so the returned value cannot tell an unopenable file from an undecodable one. Only the warning can. - master's note that an unstubbed getMimeType() mock yields a TypeError does not hold on this branch: getThumbnail() casts the value, so an unstubbed mock gives ''. That is worse rather than better for a test - seven of the eight providers answer '' with the same constant they answer anything with, so the case would pass while proving nothing about which coder ran. Font is the one provider that branches on the mime type. The comment says that instead. - owncloudci/php:7.4 rather than :8.3 named as the build with no SVG renderer, confirmed on it. Verified in owncloudci/php:7.4: 3 tests / 7 assertions / 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: report a preview file that cannot be opened without logging noise [10.16] Backport of #41855, folded into this backport at the maintainer's request rather than opened as its own 10.16 PR. SVG::getThumbnail() read the handle from $file->fopen('r') without checking it, and released it only on the success path. Bitmap::getThumbnail() checked for false but not for null. Both now use !is_resource(), and the SVG provider closes the handle in a finally so a read that throws from the wrapper stack - the encryption module does, on a missing or damaged key - cannot hold the descriptor and the view's shared lock for the rest of the request. Reworded and re-tested for PHP 7.4, where the consequence differs from master's: - master's changelog and comments say an unopened file made the request fail with a server error. On 7.4 it does not. stream_get_contents() warns and hands on false, the prefix check turns that into a bare XML declaration, and Imagick rejects it - so the preview already degraded to a media type icon, after two misleading log lines. It is on PHP 8 that those calls raise a TypeError, an \Error that escapes the catch (\Exception) as a 500. The changelog now describes the logging noise, which is what this fix removes here. - SVGStreamTest's unopenable case therefore asserts that no warning is emitted, not that the return value is false: master's assertFalse() passes with the guard reverted on this runtime, so it could never go red. Renamed accordingly, and the handler honours error_reporting() so that diagnostics the code under test silences with @ cannot fail it, the same line OC\Log\ErrorHandler::onError() draws. Verified red before green - see the PR description for the counts. - BitmapStreamTest's equivalent case keeps its existing 7.4 assertion and gains #41855's data provider, so the null handle is covered here too. Verified in owncloudci/php:7.4: tests/lib/Preview/ plus tests/lib/Files/Type/DetectionTest.php at 72 tests / 219 assertions / 0 failures, 12 environment skips (Movie, Office, SVG - this image registers no SVG coder). php -l clean under 7.4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: point the changelog entries at the reopened backport PR The backport PR was reopened as #41863, because #41828 had been opened under the wrong GitHub account and a PR's author cannot be changed. The three changelog entries linked #41828, which is now closed, so they move to #41863. The master PR links are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --------- Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged
oc-tmueller
added a commit
that referenced
this pull request
Sep 25, 2026
Bump version.php to 11.0.1 and materialize the changelog fragments for the first patch release of the 11.0 line. $OC_Version becomes [11, 0, 1, 0]: the 4th digit is the internal DB-upgrade patch level, not the public patch number, and nothing in this release needs it moved. The 15 fragments in changelog/unreleased/ move into changelog/11.0.1_2026-09-25/ (git mv, so the renames stay tracked) and CHANGELOG.md is regenerated with calens. unreleased/ keeps its .gitkeep and is now empty, ready for the next cycle. Security: - #41827 reject SVG/script content before it reaches ImageMagick bitmap previews - #41834 pin the Imagick coder for each preview provider - #41856 prevent path traversal via appconfig public_/remote_ keys Bugfixes: - #41676 reduce priority of checkPropFind event - #41779 do not echo secrets when setting config values via occ - #41782 restore index usage for filecache writes on Oracle - #41807 show federated users in the share dialog when local users also match - #41808 avoid a deprecation notice when hashing the file cache path on Oracle - #41824 ship only the app payload in the release tarballs - #41835 release the file handle when a bitmap preview cannot be decoded - #41855 show a media type icon when a preview file cannot be opened - #41869 restrict federated address book sync to the trusted server Changes: - #41775 update PHP dependencies - #41785 require rhukster/dom-sanitizer as a tagged release - #41808 restore Oracle database support in the command line installer The #41824 fragment is carried over from #41825, which prepared this release on a release/v11.0.1 branch that the org maintenance ruleset freezes after its first push; that branch cannot take the changelog commit, so this supersedes it. Once merged, v11.0.1 gets tagged on the merged commit and the release bundles are built and published from owncloud/server-release. 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
added a commit
that referenced
this pull request
Sep 25, 2026
Bump version.php to 10.16.5 and materialize the changelog fragments for this release. $OC_Version becomes [10, 16, 5, 0]: the 4th digit is the internal DB-upgrade patch level, not the public patch number, and nothing in this release needs it moved. The 11 fragments in changelog/unreleased/ move into changelog/10.16.5_2026-09-25/ (git mv, so the renames stay tracked) and CHANGELOG.md is regenerated with calens. unreleased/ keeps its .gitkeep and is now empty, ready for the next cycle. Security: - #41784 update PHP dependencies to close published advisories - #41803 prevent path traversal via appconfig public_/remote_ keys - #41827 reject SVG/script content before it reaches ImageMagick bitmap previews - #41834 pin the Imagick coder for each preview provider Bugfixes: - #41782 restore index usage for filecache writes on Oracle - #41808 avoid a deprecation notice when hashing the file cache path on Oracle - #41814 show federated users in the share dialog when local users also match - #41819 speed up Oracle schema introspection - #41835 release the file handle when a bitmap preview cannot be decoded - #41855 report a preview file that cannot be opened without logging noise Changes: - #41788 update PHP dependencies Same shape as 852062d ("feat: release 10.16.4"), which did the changelog and the version bump in one commit. Once merged, v10.16.5 gets tagged on the merged commit and the release bundles are built and published from owncloud/server-release. 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
added a commit
that referenced
this pull request
Sep 25, 2026
* feat: release 10.16.5 Bump version.php to 10.16.5 and materialize the changelog fragments for this release. $OC_Version becomes [10, 16, 5, 0]: the 4th digit is the internal DB-upgrade patch level, not the public patch number, and nothing in this release needs it moved. The 11 fragments in changelog/unreleased/ move into changelog/10.16.5_2026-09-25/ (git mv, so the renames stay tracked) and CHANGELOG.md is regenerated with calens. unreleased/ keeps its .gitkeep and is now empty, ready for the next cycle. Security: - #41784 update PHP dependencies to close published advisories - #41803 prevent path traversal via appconfig public_/remote_ keys - #41827 reject SVG/script content before it reaches ImageMagick bitmap previews - #41834 pin the Imagick coder for each preview provider Bugfixes: - #41782 restore index usage for filecache writes on Oracle - #41808 avoid a deprecation notice when hashing the file cache path on Oracle - #41814 show federated users in the share dialog when local users also match - #41819 speed up Oracle schema introspection - #41835 release the file handle when a bitmap preview cannot be decoded - #41855 report a preview file that cannot be opened without logging noise Changes: - #41788 update PHP dependencies Same shape as 852062d ("feat: release 10.16.4"), which did the changelog and the version bump in one commit. Once merged, v10.16.5 gets tagged on the merged commit and the release bundles are built and published from owncloud/server-release. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: correct three defects in the 10.16.5 release notes Found reviewing the release commit, all three in text that ships inside the tarball, so this is the last point at which they are free to fix. **Six updated dependencies were unnamed.** Diffing composer.lock at v10.16.4 against this branch gives 17 changed production packages; the entry listed 11. Missing: sabre/dav (4.7.0 to 4.7.1), sabre/event (5.1.7 to 5.1.9), sabre/vobject (4.5.8 to 4.6.1), pimple/pimple (v3.6.1 to v3.6.2), nikic/php-parser (v5.7.0 to v5.9.0) and dg/composer-cleaner (v2.2.1 to v2.2.2). All but sabre/event are direct entries in composer.json's require, and sabre/vobject is a minor bump of the vCard/iCalendar parser behind CalDAV and CardDAV — an administrator auditing what moved in that stack for 10.16.5 would have seen nothing. Five of the six came from #41787, whose URL was missing from the entry as well; nikic/php-parser then went on to v5.9.0 in #41830, which was already listed. Earlier releases on this branch do list sabre bumps here (10.10.0, 10.11.0, 10.12.0), so the omission also broke the house convention. **Two entries linked a pull request that never reached this branch.** #41819 was merged into ci/oracle-db-in-github-actions-10.16, an intermediate branch that no longer exists on the remote; the change reached 10.16 through #41815 (9408736). Likewise the 41835 entry cited only master's #41835, while the 10.16 delivery was #41837 (6443822). Both now cite the 10.16 pull request first, matching what 41827, 41834 and 41855 already do with #41863 — so calens also makes the branch's own pull request the primary link. CHANGELOG.md is regenerated with calens and, as on the rest of this branch, written without a trailing newline: `ocrelease changelog` captures calens' stdout through execa, which strips it, and every released section on 10.16 was produced that way. Adding one here would only create churn at the next release. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * chore: fold the federated address book sync fix into the 10.16.5 release notes Its fix merged into 10.16 after the release notes were first prepared, so its changelog fragment was still sitting in changelog/unreleased and would have been deferred to the next release while the fix itself shipped in this one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --------- Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
oc-tmueller
added a commit
that referenced
this pull request
Sep 25, 2026
* feat: release 11.0.1 Bump version.php to 11.0.1 and materialize the changelog fragments for the first patch release of the 11.0 line. $OC_Version becomes [11, 0, 1, 0]: the 4th digit is the internal DB-upgrade patch level, not the public patch number, and nothing in this release needs it moved. The 15 fragments in changelog/unreleased/ move into changelog/11.0.1_2026-09-25/ (git mv, so the renames stay tracked) and CHANGELOG.md is regenerated with calens. unreleased/ keeps its .gitkeep and is now empty, ready for the next cycle. Security: - #41827 reject SVG/script content before it reaches ImageMagick bitmap previews - #41834 pin the Imagick coder for each preview provider - #41856 prevent path traversal via appconfig public_/remote_ keys Bugfixes: - #41676 reduce priority of checkPropFind event - #41779 do not echo secrets when setting config values via occ - #41782 restore index usage for filecache writes on Oracle - #41807 show federated users in the share dialog when local users also match - #41808 avoid a deprecation notice when hashing the file cache path on Oracle - #41824 ship only the app payload in the release tarballs - #41835 release the file handle when a bitmap preview cannot be decoded - #41855 show a media type icon when a preview file cannot be opened - #41869 restrict federated address book sync to the trusted server Changes: - #41775 update PHP dependencies - #41785 require rhukster/dom-sanitizer as a tagged release - #41808 restore Oracle database support in the command line installer The #41824 fragment is carried over from #41825, which prepared this release on a release/v11.0.1 branch that the org maintenance ruleset freezes after its first push; that branch cannot take the changelog commit, so this supersedes it. Once merged, v11.0.1 gets tagged on the merged commit and the release bundles are built and published from owncloud/server-release. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: name every updated PHP dependency in the 11.0.1 changelog composer/semver (3.4.4 to 3.5.0) and nikic/php-parser (v5.8.0 to v5.9.0) both moved since v11.0.0, both are direct entries in composer.json's require, and neither was named in the fragment. Diffing composer.lock at v11.0.0 against this branch gives 26 changed production packages; the fragment listed 24. Each was missed by the pass that bumped it: #41864 edited this fragment in the same commit that raised composer/semver, and #41829 did the same for nikic/php-parser. The list is not direct-only either — it already names transitive dependencies such as guzzlehttp/psr7 and symfony/mime — so both are omissions, not a scoping decision. An administrator reconciling the 11.0.1 notes against advisory ranges would read both as unchanged. The four remaining differences are require-dev only (myclabs/deep-copy, phpunit/phpunit, sebastian/exporter, sebastian/recursion-context) and stay out, matching the fragment's existing production-only scope; the tarball installs --no-dev. Also restores CHANGELOG.md's trailing newline. `ocrelease changelog` writes calens' stdout as captured by execa, which strips it; the last six "chore: update changelog" revisions of the file all end in one, and so does calens' own output, so the next regeneration would have put it back as a one-line no-op diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --------- Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.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
1 participant
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.