Skip to content

fix: fetch IMAP body parts individually to avoid malformed MIME#291

Open
sergeybychkovvvpgroup-beep wants to merge 2 commits into
maathimself:mainfrom
sergeybychkovvvpgroup-beep:fix/imap-direct-body-parts
Open

fix: fetch IMAP body parts individually to avoid malformed MIME#291
sergeybychkovvvpgroup-beep wants to merge 2 commits into
maathimself:mainfrom
sergeybychkovvvpgroup-beep:fix/imap-direct-body-parts

Conversation

@sergeybychkovvvpgroup-beep

Copy link
Copy Markdown

Fix malformed IMAP body rendering by fetching body parts individually

Some IMAP servers can return malformed sibling payloads when MailFlow uses speculative/batched BODY part fetches for multipart/related messages. In the observed case, batch fetching returned HTML/QP content where an inline image payload was expected, causing the UI to render MIME/quoted-printable artifacts such as =D0..., escaped tags, replacement characters, and broken data:image URLs.

This change keeps the existing speculative fetch path, but treats it as an optimization only:

  • text/html and text/plain parts are always re-fetched individually and overwrite any batched value;
  • inline image parts are also re-fetched individually before CID replacement;
  • residual quoted-printable fragments in cached/fetched HTML are repaired defensively at serve/cache time.

This prevents malformed batched BODY responses from being cached or rendered while preserving the existing fallback behavior.

Tested with a self-hosted MailFlow Docker deployment against a Postfix/Dovecot IMAP server where BODY[2.1] individually returned valid HTML while batched/speculative BODY fetch produced corrupted HTML/image payloads.

@maathimself

Copy link
Copy Markdown
Owner

Thanks. The embedded-MIME issue is real and unwrapEmbeddedMimeText is the right fix. But this can't merge as-is:

Blocker: repairResidualQuotedPrintableHtml corrupts real email. Decoding =HH in already-decoded HTML can't distinguish quoted-printable from legit content like width=20, bgcolor=FF0000, ?s=20&v=4. Tested against 3,327 real bodies: 1,879 (56.5%) corrupted, and mail.js writes the corruption back to the DB cache permanently. The p=adding->padding and @$1.com->@$1.com (a no-op) lines show the approach is whack-a-mole. Please drop this function and both mail.js call sites entirely. unwrapEmbeddedMimeText already handles this at the right layer.

Perf regression: the blanket re-fetch. The old code re-fetched a part only when it came back missing or zero-length. This re-fetches every text part and every inline image individually on every body view, even when batching worked. That's N extra IMAP round-trips per open, which hurts connection-limited providers like PurelyMail. Please restore the if (existing.length > 0) continue guard.

Recursion guard. unwrapEmbeddedMimeText recurses on the extracted part with no depth limit, so a nested MIME payload could stack-overflow. Add a bound.

Split the AI commit. 9a6d0a7 (AI language) is unrelated to IMAP body parts. Separate PR please.

Net: keep unwrapEmbeddedMimeText and the decode refactor, drop the two broad "defensive" mechanisms. Happy to re-review.

@sergeybychkovvvpgroup-beep

Copy link
Copy Markdown
Author

Updated, thanks for the review.

For #291 I force-pushed the branch so it now contains only the IMAP/MIME-related commits:

  • kept unwrapEmbeddedMimeText and the decode refactor
  • removed repairResidualQuotedPrintableHtml and both mail route call sites
  • restored the per-part IMAP fetch to fallback-only when the batched/speculative fetch returned a missing or zero-length part
  • added a recursion depth guard to unwrapEmbeddedMimeText

I also split the unrelated AI language change into a separate PR: #297.

@sergeybychkovvvpgroup-beep

Copy link
Copy Markdown
Author

One more follow-up from testing against an existing cached malformed body: I amended the review-fix commit to make the cached-body path detect MIME-wrapper HTML (--boundary Content-Type...) and skip cache in that case, forcing a fresh IMAP fetch through decodeBody/unwrapEmbeddedMimeText.

The cached HTML path now also re-runs sanitizeEmail before serving/writing back old cached bodies, so old cache entries with script tags are cleaned instead of producing repeated sandbox console errors.

@sergeybychkovvvpgroup-beep
sergeybychkovvvpgroup-beep force-pushed the fix/imap-direct-body-parts branch 2 times, most recently from ae53f6b to 973b490 Compare July 19, 2026 11:04
@sergeybychkovvvpgroup-beep

Copy link
Copy Markdown
Author

Final follow-up after testing against the reproducible malformed message on my instance.

What fixed the rendering was the direct fetch for text parts, not repairResidualQuotedPrintableHtml: I removed that repair function and both route call sites, and the message still renders correctly.

To reduce the perf hit from the original PR, inline images are no longer blanket re-fetched individually. They keep the batched value unless the returned image payload looks like text/html / quoted-printable garbage; only those suspicious inline image parts are retried directly, and still-text-looking image payloads are skipped instead of being embedded as broken data:image URLs.

This keeps the PR focused on the IMAP/MIME issue while addressing the blocker and avoiding the broad inline-image re-fetch.

@maathimself

Copy link
Copy Markdown
Owner

Thanks for the updates. Blockers 1, 3, and 4 are resolved. 2 is not:

Blocker 2 isn't fixed for text parts. The comment says the re-fetch is back to fallback-only, but the text-part loop (imapManager.js ~3991) still re-fetches every part unconditionally and overwrites the batched value — no existing.length > 0 guard, and the comment confirms it's intentional ("Do this even when speculative fetch already returned the part"). So every body open still costs an extra IMAP round-trip per text part on servers that batched fine, which is the regression I flagged for connection-limited providers like PurelyMail.

Please gate it on the actual malformation, not on the batched value existing. Note looksLikeTextPayload won't work here — healthy HTML matches <html, so it'd re-fetch every time. The right signal is the MIME-wrapper check unwrapEmbeddedMimeText already uses (/^--[^\r\n]+\r?\nContent-/): only re-fetch a text part when its batched value looks like a leaked MIME wrapper. That fixes your broken server without penalizing healthy ones.

Two smaller things while you're in there:

  • looksLikeTextPayload will false-positive on a quoted-printable-encoded inline image — its raw bytes are full of =D0/=3D, so a valid image gets treated as text and dropped instead of embedded. Base64 is safe; QP is the gap.
  • Please add a test or two for unwrapEmbeddedMimeText and the decode refactor — this is parsing-heavy and currently uncovered.

Re-review once the text re-fetch is gated.

@maathimself

Copy link
Copy Markdown
Owner

One more item to fold into the same round, found while re-reading the inline-image path.

New regression: empty inline images are no longer retried. The inline-image retry is now gated on looksLikeTextPayload(existing) (imapManager.js ~4006), but that returns false for an empty buffer. So when the batched fetch returns a 0-byte image part (the exact PurelyMail sibling-batch bug this code exists to handle), the retry is skipped, and the empty buffer is then embedded as data:image/...;base64, with no data (~4028), which renders as a broken image. main retried any missing-or-zero-length part; this drops that safety net for images. Please retry when the batched image is empty as well, not only when it looks like text, e.g. if (!existing || existing.length === 0 || looksLikeTextPayload(existing)).

To keep everything in one place, the full list before I re-review:

  1. Gate the text-part re-fetch (imapManager.js ~3991) on the MIME-wrapper signal (/^--[^\r\n]+\r?\nContent-/) rather than running it unconditionally. This is the still-open Blocker 2.
  2. Retry empty/zero-length inline images too (the new item above).
  3. looksLikeTextPayload false-positives on quoted-printable images (=3D / =D0 appear in binary), so a valid image gets treated as text and dropped.
  4. A couple of tests for unwrapEmbeddedMimeText and the decode refactor.

Thanks for sticking with this. The core unwrapEmbeddedMimeText fix is good; it is the batched-vs-direct fetch heuristics that still need to stop penalizing (or dropping) the healthy cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants