test: make BitmapStreamTest survive the OC10-164 coder pin - #41838
Merged
Merged
Conversation
… 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>
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>
…ting 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>
… 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>
…ck'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>
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>
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>
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>
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>
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>
… 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>
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>
…rose 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>
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>
jvillafanez
approved these changes
Sep 22, 2026
oc-tmueller
added a commit
that referenced
this pull request
Sep 24, 2026
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>
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>
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
2 participants
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
BitmapStreamTest, which I added in #41835, mocksOCP\Files\Filewithout stubbinggetMimeType(). Harmless on master today — but #41827 hasBitmapproviders derive the Imagick coder from it, andgetResizedPreview()declares that parameter asstring.nullthere is aTypeError, and aTypeErroris an\Error, so it escapesgetThumbnail()'scatch (\Exception)rather than degrading to "no preview".#41827 is currently red because of this, on every database. Its
pull_requestCI builds the head-into-base merge commit, which is the only tree that has both changes:The merge itself is clean and correct: the
fopenguard, the pin's$file->getMimeType()argument and exactly onefclose()inside thefinallyall coexist. Only the test was wrong.Targets
master, not folded into #41827, so it merges independently of that security review and keeps #41827 cherry-pickable into #41828.What changed
Filemocks stubgetMimeType().Photoshopprovider that was already here — provider, declared media type and content all agree, so it stays a success whether the provider pins a coder or lets ImageMagick sniff one.Two things worth recording, because they were counter-intuitive
Availability guards on a coder are the wrong tool. The success case originally decoded a PNG through
Photoshop, which only works while ImageMagick may sniff. Making it agree with the pin needed a real format, and every guard I tried was wrong in a different way:queryFormats(PDF)policy.xmland the Ghostscript delegate. Debian/Ubuntu deny PS/EPS/PDF/XPS by default, so this would have been red in CI, not skippedqueryFormats(TIFF)coders/tiff.cregisters unconditionally and wires delegates only when built against libtiffgetImageBlob()still returned 3330 bytes; only the read raisedfclose()assertion — and a guard quietly withholding assertions is how the OC10-164 preview tests came to never run in CI in the first placeThe answer was to need no guard:
coders/psd.solinks no image library (PSD is implemented natively), whilecoders/tiff.solinkslibtiff.so.5. The test is now unconditional and ~45 lines shorter.Not every "undecodable" payload is equivalent.
<?xml?><notanimage>throws everywhere I measured, but for a different reason each time —no decode delegate 'SVG'on stock CI,not allowed by the security policy 'MVG'with the extra coders under stock Ubuntu policy, MVG's ownmust specify image sizewith the policy opened. More importantly libmagic reads it astext/xml, which #41827's pre-decode gate refuses — so on that branch it would stop reaching the decode at all. Binary junk reads asapplication/octet-streamand fails at the decode on both trees.Verification
masterOK (3 tests, 6 assertions), no skipsmaster+ #41827 mergedfinallyremovedIndependently reproduced on the merged tree from a separate checkout, matching to the assertion.
make test-php-style0 of 2435;php -lclean under PHP 7.4.Note the file fails on #41827's bare branch (1 error, 1 failure) — correctly, since that tree has neither the
fopenguard nor thefinallythese cases assert.testClosesTheStreamOnSuccesspasses there, asfclose()on the success path predates #41835.No changelog entry: test-only, no user-visible change.
🤖 Generated with Claude Code