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

@@ -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

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

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 `<img>`
+     * 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
     {

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.
      *

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<string, array{?string, bool}> */
+    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<string, array{string, ?string}> */
+    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' => ['<svg xmlns="http://www.w3.org/2000/svg"><script>alert(1)</script></svg>', null];
+        yield 'html'              => ["<!DOCTYPE html>\n<html><body><script>alert(1)</script></body></html>", 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
     {