From 57f92f1170026626150d65adf2901d28fbf1397b Mon Sep 17 00:00:00 2001 From: MontyWei Date: Mon, 19 Oct 2026 19:51:42 +0800 Subject: [PATCH] Serve inline images labelled octet-stream by what their bytes are (#220) Parts labelled application/octet-stream or image/octet-stream are identified from their bytes; only a raster type on the allow-list can come out, so SVG and HTML stay downloads. Part of #213. Co-authored-by: MontyWei <62734446+montywei@users.noreply.github.com> --- CHANGELOG.md | 11 ++++ src/Controller/Mail/AttachmentController.php | 13 ++++ src/Domain/Helper/InlineDisposition.php | 59 +++++++++++++++++++ src/Service/Mail/ImageProxyFetcher.php | 55 +---------------- tests/Domain/Helper/InlineDispositionTest.php | 46 +++++++++++++++ 5 files changed, 132 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba4a03a6..4648b450 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,17 @@ reached Outlook at all, and now does; labels nested under a Gmail label are dele it. No schema change. [#40](https://github.com/karatektus/pl_mail/issues/40). +**Inline images labelled `application/octet-stream` show up.** Many mailers do not say what an +inline image is — they label it `application/octet-stream`, or `image/octet-stream`, which is not +a registered type at all and is what Vaultwarden's notification mails put on their logo — and the +attachment route served every such part as a download, so the image never appeared in the message. For those labels only, the file's own +bytes now decide, held to the same allow-list as everything else: a PNG, JPEG, GIF or WebP is +served inline, while SVG, HTML and anything unrecognised are still downloads. The image proxy +already did this for unlabelled remote images and now shares the same check, so a remote image +labelled `image/octet-stream` is now also identified from its bytes and served under its real +type, rather than passed through under the label it came with. No schema change. +Part of [#213](https://github.com/karatektus/pl_mail/issues/213). + **A mail can go to Bcc recipients only.** Sending with nobody in To was refused with "Add at least one recipient before sending", even with addresses in Cc or Bcc: the window counted all three, but the server only counted To. It now counts all three as well. A mail with no recipient at all is diff --git a/src/Controller/Mail/AttachmentController.php b/src/Controller/Mail/AttachmentController.php index 3ff0a941..6d2d2d11 100644 --- a/src/Controller/Mail/AttachmentController.php +++ b/src/Controller/Mail/AttachmentController.php @@ -90,6 +90,9 @@ final class AttachmentController extends AbstractController return $response; } + /** Enough of a file for its magic bytes; libmagic reads no further for raster formats. */ + private const int SNIFF_BYTES = 4096; + #[Route('/mail/attachment/{id}', name: 'app_mail_attachment', methods: ['GET'], requirements: ['id' => '\d+'])] public function serve(MessagePart $part, Request $request): Response { @@ -98,6 +101,16 @@ final class AttachmentController extends AbstractController $absolutePath = $this->attachmentResolver->absolutePathFor($part); $contentType = $part->contentType ?? 'application/octet-stream'; + // A part labelled `application/octet-stream` or `image/octet-stream` is + // very often an inline image whose sender did not say so. For those + // labels only, the bytes decide — and only a type on the allow-list can + // come out of InlineDisposition::sniff(), so this cannot make anything + // inline that the list would refuse. + if (true === InlineDisposition::isUnlabelled($contentType)) { + $head = @file_get_contents($absolutePath, false, null, 0, self::SNIFF_BYTES); + $contentType = (false === $head ? null : InlineDisposition::sniff($head)) ?? $contentType; + } + // An allow-list, not a prefix test. `image/svg+xml` starts with // "image/" and executes script when loaded as a document, and the type // comes from the MIME headers of an incoming mail — see diff --git a/src/Domain/Helper/InlineDisposition.php b/src/Domain/Helper/InlineDisposition.php index b0e6e622..5e55ef25 100644 --- a/src/Domain/Helper/InlineDisposition.php +++ b/src/Domain/Helper/InlineDisposition.php @@ -116,6 +116,65 @@ final class InlineDisposition return '' === $clean ? $default : $clean; } + /** + * Did the sender decline to say what this is? + * + * S3 hands back `application/octet-stream` for any object uploaded without + * a content type, and a great deal of the web's images live in exactly + * that state. Amazon's return-label QR codes are one: a real 8KB PNG, + * served unlabelled, refused by the image proxy for years while the reader + * saw a blank space where their barcode should be. + * + * Mail does the same to inline parts: `application/octet-stream` is common, + * and so is `image/octet-stream`, which is not a registered type at all — + * Vaultwarden's notification mails put it on their logo. Neither is on the + * list above, so those images were served as downloads and an `` + * could not show them. + * + * Treated as "unknown", not as "not an image" — the difference between the + * two is the whole bug. For these labels, and only these, the bytes decide; + * see sniff(). + */ + public static function isUnlabelled(?string $contentType): bool + { + return in_array(self::normalise($contentType), [ + '', + 'application/octet-stream', + 'binary/octet-stream', + 'image/octet-stream', + ], true); + } + + /** + * What the bytes actually are, when that is a type on the list above; + * null for anything else. + * + * This is not trusting the sender — the declared type is discarded and the + * content is identified from its own magic bytes, then held to the same + * allow-list, which excludes SVG. SVG matters here: it is the one image + * format that can carry script, and serving one inline would be serving + * script from our own origin. So an unlabelled part can only ever come out + * as one of a fixed set of raster types, whatever the sender wrote. + * The start of the content is enough; there is no need to pass a whole + * attachment. + */ + public static function sniff(string $bytes): ?string + { + if ('' === $bytes) { + return null; + } + + $sniffed = (new \finfo(FILEINFO_MIME_TYPE))->buffer($bytes); + + if (false === $sniffed) { + return null; + } + + $sniffed = self::normalise($sniffed); + + return true === self::allows($sniffed) ? $sniffed : null; + } + /** The bare type, lowercased, with any parameters and padding removed. */ public static function normalise(?string $contentType): string { diff --git a/src/Service/Mail/ImageProxyFetcher.php b/src/Service/Mail/ImageProxyFetcher.php index 014d7895..8f60c0b9 100644 --- a/src/Service/Mail/ImageProxyFetcher.php +++ b/src/Service/Mail/ImageProxyFetcher.php @@ -227,7 +227,7 @@ final readonly class ImageProxyFetcher // download it. if ( false === str_starts_with($contentType, 'image/') - && false === self::isUnlabelled($contentType) + && false === InlineDisposition::isUnlabelled($contentType) ) { $response->cancel(); @@ -257,8 +257,8 @@ final readonly class ImageProxyFetcher // Unlabelled: decide from the bytes, and serve what the bytes // actually are rather than what the sender claimed. - if (true === self::isUnlabelled($contentType)) { - $sniffed = self::sniff($body); + if (true === InlineDisposition::isUnlabelled($contentType)) { + $sniffed = InlineDisposition::sniff($body); if (null === $sniffed) { $this->logger->info('ImageProxyFetcher: unlabelled response is not an image', [ @@ -276,55 +276,6 @@ final readonly class ImageProxyFetcher } } - /** - * Did the sender decline to say what this is? - * - * S3 hands back `application/octet-stream` for any object uploaded without - * a content type, and a great deal of the web's images live in exactly - * that state. Amazon's return-label QR codes are one: a real 8KB PNG, - * served unlabelled, refused by this proxy for years while the reader saw - * a blank space where their barcode should be. - * - * Treated as "unknown", not as "not an image" — the difference between the - * two is the whole bug. - */ - private static function isUnlabelled(string $contentType): bool - { - return '' === $contentType - || 'application/octet-stream' === $contentType - || 'binary/octet-stream' === $contentType; - } - - /** - * What the bytes actually are, or null if they are not an image we serve. - * - * THIS IS NOT TRUSTING THE SENDER — it is the opposite. The declared type - * is discarded and the content is identified from its own magic bytes, - * then checked against InlineDisposition's list, which is the same - * deliberate allow-list attachments are held to and which excludes SVG. - * SVG matters here: it is the one image format that can carry script, and - * a proxy that served one would be serving script from our own origin. - * - * So an unlabelled response can only ever be served as one of a fixed set - * of raster types, whatever the sender said or did not say. - */ - private static function sniff(string $body): ?string - { - if ('' === $body) { - return null; - } - - $sniffed = (new \finfo(FILEINFO_MIME_TYPE))->buffer($body); - - if (false === $sniffed) { - return null; - } - - $sniffed = strtolower(trim(explode(';', $sniffed)[0])); - - return true === InlineDisposition::allows($sniffed) ? $sniffed : null; - } - /** * Rules 1–3. Returns the host and the ONE address the connection may use. * diff --git a/tests/Domain/Helper/InlineDispositionTest.php b/tests/Domain/Helper/InlineDispositionTest.php index b09679bf..31e67a3a 100644 --- a/tests/Domain/Helper/InlineDispositionTest.php +++ b/tests/Domain/Helper/InlineDispositionTest.php @@ -105,6 +105,52 @@ final class InlineDispositionTest extends TestCase self::assertSame('image/png', InlineDisposition::servedType('IMAGE/PNG; name=x.png')); } + #[DataProvider('labels')] + public function testWhichLabelsLetTheBytesDecide(?string $contentType, bool $expected): void + { + self::assertSame($expected, InlineDisposition::isUnlabelled($contentType)); + } + + /** @return iterable */ + public static function labels(): iterable + { + yield 'octet-stream' => ['application/octet-stream', true]; + // Not a registered type, and what Vaultwarden's mails and plenty of + // others put on their inline logos (#213). + yield 'image/octet-stream' => ['image/octet-stream', true]; + yield 'binary' => ['binary/octet-stream', true]; + yield 'with a name' => ['Application/Octet-Stream; name="logo.png"', true]; + yield 'empty' => ['', true]; + yield 'null' => [null, true]; + + // A type the sender did state is taken at its word — sniffing these + // would let a declared `text/html` be re-read as something else. + yield 'png' => ['image/png', false]; + yield 'svg' => ['image/svg+xml', false]; + yield 'html' => ['text/html', false]; + yield 'pdf' => ['application/pdf', false]; + } + + #[DataProvider('contents')] + public function testSniffingOnlyEverYieldsATypeOnTheList(string $bytes, ?string $expected): void + { + self::assertSame($expected, InlineDisposition::sniff($bytes)); + } + + /** @return iterable */ + public static function contents(): iterable + { + yield 'png' => [(string) base64_decode('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNkYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg==', true), 'image/png']; + yield 'gif' => [(string) base64_decode('R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7', true), 'image/gif']; + + // The reason sniff() exists as a gate and not a lookup: an unlabelled + // part that turns out to be markup is still a download. + yield 'svg with a script' => ['', null]; + yield 'html' => ["\n", null]; + yield 'pdf' => ["%PDF-1.4\n%\xe2\xe3\xcf\xd3\n1 0 obj\n<<>>\nendobj\n", null]; + yield 'empty' => ['', null]; + } + /** A path in an attachment name used to be a 500 out of makeDisposition(). */ public function testAFilenameWithAPathStillMakesAValidHeader(): void {