N°9574 - fix(email): Prevent CSS leaking into plain-text emails

[hakonharnes]

Base information

Question	Answer
Related to a SourceForge thread / Another PR / A GitHub Issue / Combodo ticket?	TBD
Type of change?	Bug fix

Symptom (bug) / Objective (enhancement)

When an HTML email notification is sent, the generated text/plain alternative can contain inlined stylesheet contents.

After updating from iTop 3.2.2-1 to 3.2.3, public log update emails may expose CKEditor CSS such as @keyframes ck-input-shake, @keyframes ck-dialog-fade-in, .ck-reset_all, etc. before the actual email text.

Before fix:

After fix:

Reproduction procedure (bug)

1. On iTop 3.2.3
2. Configure an email notification for ticket public log updates
3. Update the public log of a ticket
4. Receive the notification email
5. Open/read the email in a client that displays the text/plain alternative (e.g. Gmail)
6. See CKEditor CSS content before the expected notification body

Expected result: the email body only contains the notification content.

Actual result: stylesheet content is visible before the notification content.

Cause (bug)

The Symfony mailer implementation builds the text/plain alternative from $sBody after InlineCssIntoBodyContent() has injected CSS into the HTML body.

The previous implementation used:

$oTextPart = new TextPart(strip_tags($sBody), 'utf-8', 'plain', 'base64');

At that point, $sBody can contain <style>...</style> content. strip_tags() removes the tags but keeps their text content, so CSS rules leak into the plain-text part.

Also, the multipart/alternative parts were ordered as HTML first and plain text second:

new AlternativePart($oHtmlPart, $oTextPart)

For multipart/alternative, the preferred representation must be last, so the expected order is text/plain first, then text/html.

This was introduced with the Symfony mailer migration in 428d2c6356a33c92e5b78c23559cfdd531a63b0f.

Proposed solution (bug and enhancement)

Build the plain-text body from the original HTML before CSS inlining, using Symfony's DefaultHtmlToTextConverter.

Then inline CSS only for the HTML part.

Finally, build multipart/alternative in the standard order:

new AlternativePart($oTextPart, $oHtmlPart)

This prevents CSS from leaking into the plain-text part and makes HTML the preferred alternative for clients that support it.

Checklist before requesting a review

• I have performed a self-review of my code
• I have tested all changes I made on an iTop instance
• I have added a unit test, otherwise I have explained why I couldn't
• Is the PR clear and detailed enough so anyone can understand without digging in the code?

Unit test note: I did not add a unit test because this mail rendering path depends on the iTop email/MIME assembly and notification flow. The fix has been verified manually on an iTop instance by comparing the generated/received email before and after the change.

[steffunky]

Hi Håkon,
Thanks for your pull-request, I've created a bug in our internal bug tracker N°9574 - CSS leaks into plain-text emails, I'll wait for our product owners to review it.

In the meantime, this issue seems to be a regression in iTop 3.2.3, could you base your PR on support/3.2 branch so it'll be included in next 3.2.x version ?

The fix seems pretty straightforward and uses SymfonyMailer better that what we did when migrating from Laminas, there's juste one questions, did you try you fix with synchronous or asynchronous email sending ?

I see that $this->m_aData['body'], which is used to store the email in base in case of asynchronous mail and that is then passed to the same SetBody method when the background task runs, is populated with the HTML containing injected CSS.
So that would bring back the old behavior of giving to the $oTextPart, text that comes from HTML with injected CSS stripped of tags.
I see that DefaultHtmlToTextConverter removes the <script> from the HTML so maybe it's not that problematic but that would mean $sTextBody maybe doesn't need to be calculated before inlining css either

[jf-cbd]

Hey @hakonharnes, thank you for your PR (and for reporting that regression !). We'll perform a few tests to see if we want it in 3.2.4 or if it requires a patch (e.g. 3.2.3-1).

[hakonharnes]

In the meantime, this issue seems to be a regression in iTop 3.2.3, could you base your PR on support/3.2 branch so it'll be included in next 3.2.x version ?

Done!

did you try you fix with synchronous or asynchronous email sending ?

Yes, I tested both synchronous and asynchronous email sending.

The old patch worked, but I adjusted the patch after your async comment. The plain-text alternative is now generated at the point where the MIME parts are built, using Symfony's DefaultHtmlToTextConverter, from the final HTML body.

That means both paths behave the same:

• synchronous sending converts the final HTML body to text/plain
• asynchronous sending serializes/restores the body, then also converts the final HTML body to text/plain when the email is built by the background task

This avoids depending on whether CSS was already inlined before serialization. In both sync and async mode, the CKEditor CSS no longer appears in the plain-text part of the email.

I amended it to the original commit so commit history is clean. Let me know if you need more adjustments or testing on my end.

[Molkobain]

Hello @hakonharnes thanks for the PR! I just added unit tests to cover more emails cases, hopefully it will prevent this kind of regressions in the future.

[Molkobain]

@greptileai review please

[greptile-apps]

Greptile Summary

This PR fixes CSS leaking into the text/plain alternative of HTML emails by replacing strip_tags() with Symfony's DefaultHtmlToTextConverter (which correctly excludes <style> content), and corrects the multipart/alternative part order to comply with RFC 2046 (plain first, HTML last). The fix is well-reasoned, well-tested, and addresses both the CSS leaking regression and the long-standing incorrect part ordering.

Confidence Score: 4/5

Safe to merge — the fix is correct and well-tested; only minor style-level issues remain.

No P0 or P1 findings. Two P2 findings: a comment typo and the plain-text part being generated from the post-image-embed body. Neither affects correctness of the primary bug fix.

sources/Core/Email/EmailSymfony.php — minor comment typo and plain-text capture ordering.

Important Files Changed

Filename	Overview
sources/Core/Email/EmailSymfony.php	Core fix: replaces strip_tags() with DefaultHtmlToTextConverter for the plain-text alternative, and corrects the RFC 2046 part order (plain first, HTML last). Two minor P2 style issues: typo in comment and text part built from the already-image-embedded body.
tests/php-unit-tests/unitary-tests/sources/core/Email/EmailSymfonyTest.php	Good test coverage added: part ordering, CSS absence in plain text (with and without custom styles), HTML part integrity, and the inline-images RelatedPart wrapping scenario.

Sequence Diagram

sequenceDiagram
    participant C as Caller
    participant SB as SetBody()
    participant IC as InlineCssIntoBodyContent()
    participant EI as EmbedInlineImages()
    participant HC as DefaultHtmlToTextConverter

    C->>SB: SetBody(html, text/html, customStyles?)
    SB->>IC: InlineCssIntoBodyContent(sBody, customStyles)
    note over IC: Returns body unchanged if customStyles is null
    IC-->>SB: sBody (CSS-inlined or unchanged)
    SB->>EI: EmbedInlineImages(sBody) [modifies by ref]
    EI-->>SB: aAdditionalParts, sBody with cid: srcs
    SB->>HC: convert(sBody, utf-8)
    HC-->>SB: plain text (style tags stripped)
    note over SB: Build TextPart(plain), TextPart(html)
    note over SB: AlternativePart(plain, html) — RFC 2046 order
    alt inline images present
        note over SB: Wrap in RelatedPart(alternative, ...images)
    end
    SB-->>C: m_oMessage body set

Loading

_{Reviews (1): Last reviewed commit: "N°9574 - Add unit tests" | Re-trigger Greptile}

[Molkobain]

Force pushed to rebase branch on support/3.2.3

[Molkobain]

@hakonharnes we would like to send you stickers as a thank you for your contributions.
Could you send me your fullname and postal address to guillaume.lajarige at combodo.com please?

Nevermind, I just saw that we already have them!