N°9574 - Fix CKEditor CSS displayed as part of the email message in Gmail (#898)

* fix(email): generate plain text before inlining HTML CSS

* N°9574 - Add unit tests

* Apply suggestions from code review

Co-authored-by: Molkobain <lajarige.guillaume@free.fr>

---------

Co-authored-by: Molkobain <lajarige.guillaume@free.fr>

sources/Core/Email/EmailSymfony.php

@@ -30,6 +30,7 @@ use Symfony\Component\Mailer\Exception\TransportExceptionInterface;
 use Symfony\Component\Mailer\Transport;
 use Symfony\Component\Mailer\Mailer;
 use Symfony\Component\Mime\Email as SymfonyEmail;
+use Symfony\Component\Mime\HtmlToTextConverter\DefaultHtmlToTextConverter;
 use Symfony\Component\Mime\Part\DataPart;
 use Symfony\Component\Mime\Part\Multipart\RelatedPart;
 use Symfony\Component\Mime\Part\Multipart\MixedPart;
@@ -416,13 +417,13 @@ class EMailSymfony extends Email
 
 		$this->m_aData['body'] = ['body' => $sBody, 'mimeType' => $sMimeType];
 
-		$oTextPart = new TextPart(strip_tags($sBody), 'utf-8', 'plain', 'base64');
-
 		// Embed inline images and store them in attachments (so BuildSymfonyMessageFromInternal can pick them)
 		if ($sPrimaryMimeType === 'text/html') {
 			$aAdditionalParts = $this->EmbedInlineImages($sBody);
+			$oTextPart = new TextPart((new DefaultHtmlToTextConverter())->convert($sBody, 'utf-8'), 'utf-8', 'plain', 'base64');
 			$oHtmlPart = new TextPart($sBody, 'utf-8', 'html', 'base64');
-			$oAlternativePart = new AlternativePart($oHtmlPart, $oTextPart);
+			// It's important de order parts from least prefered to most prefered as per RFC 2046 {@see https://www.rfc-editor.org/rfc/rfc2046.html#section-5.1.4}
+			$oAlternativePart = new AlternativePart($oTextPart, $oHtmlPart);
 			// Default root part is the HTML body
 			$oRootPart = $oAlternativePart;
 

tests/php-unit-tests/unitary-tests/sources/core/Email/EmailSymfonyTest.php

@@ -1,6 +1,11 @@
 <?php
 
+use Combodo\iTop\Core\Email\EMailSymfony;
 use Combodo\iTop\Test\UnitTest\ItopTestCase;
+use Symfony\Component\Mime\Part\DataPart;
+use Symfony\Component\Mime\Part\Multipart\AlternativePart;
+use Symfony\Component\Mime\Part\Multipart\RelatedPart;
+use Symfony\Component\Mime\Part\TextPart;
 
 class EmailSymfonyTest extends ItopTestCase
 {
@@ -135,4 +140,162 @@ HTML;
 
 		$this->assertSame($sExpectedBody, $sActualBody);
 	}
+
+	/**
+	 * Returns the parts of the AlternativePart produced by SetBody() for an HTML email.
+	 *
+	 * Handles both the simple case (AlternativePart at root) and the inline-images case
+	 * where the root is a RelatedPart whose first child is the AlternativePart.
+	 *
+	 * @return AbstractPart[]
+	 */
+	private function GetAlternativePartsFromHtmlEmail(EMailSymfony $oEmail): array
+	{
+		$oSymfonyMessage = $this->GetNonPublicProperty($oEmail, 'm_oMessage');
+		$oBody = $oSymfonyMessage->getBody();
+
+		// With inline images the root is a RelatedPart; the AlternativePart is its first child.
+		if ($oBody instanceof RelatedPart) {
+			$oBody = $oBody->getParts()[0];
+		}
+
+		$this->assertInstanceOf(AlternativePart::class, $oBody, 'Body should be a multipart/alternative for HTML emails');
+
+		return $oBody->getParts();
+	}
+
+	/**
+	 * RFC 2046 §5.1.4: parts in multipart/alternative must be ordered from least to most preferred.
+	 * Email clients display the last part they support, so text/plain must come first and text/html last.
+	 *
+	 * @see https://www.rfc-editor.org/rfc/rfc2046.html#section-5.1.4
+     * @covers \Combodo\iTop\Core\Email\EmailSymfony::SetBody()
+	 * @since N°9574
+	 */
+	public function testSetBodyAlternativePartOrderForHtmlEmailIsPlainThenHtml(): void
+	{
+		$oEmail = new EMailSymfony();
+		$oEmail->SetBody('<p>Hello there!</p>', 'text/html');
+
+		[$oFirstPart, $oSecondPart] = $this->GetAlternativePartsFromHtmlEmail($oEmail);
+
+		$this->assertSame('plain', $oFirstPart->getMediaSubtype(), 'First part must be text/plain (least preferred per RFC 2046)');
+		$this->assertSame('html', $oSecondPart->getMediaSubtype(), 'Last part must be text/html (most preferred per RFC 2046)');
+	}
+
+	/**
+	 * @dataProvider provideSetBodyPlainTextDoesNotContainCss
+	 *
+	 * @covers \Combodo\iTop\Core\Email\EmailSymfony::SetBody()
+	 * @since N°9574
+	 */
+	public function testSetBodyPlainTextDoesNotContainCss(string $sHtml, ?string $sCustomStyles): void
+	{
+		$oEmail = new EMailSymfony();
+		$oEmail->SetBody($sHtml, 'text/html', $sCustomStyles);
+
+		// We locate the plain text part by subtype to be order-agnostic and isolate this assertion from the order bug.
+		$aParts = $this->GetAlternativePartsFromHtmlEmail($oEmail);
+		$oPlainPart = null;
+		foreach ($aParts as $oPart) {
+			if ($oPart instanceof TextPart && $oPart->getMediaSubtype() === 'plain') {
+				$oPlainPart = $oPart;
+				break;
+			}
+		}
+		$this->assertNotNull($oPlainPart, 'No text/plain part found in the message');
+
+		$sPlainText = $oPlainPart->getBody();
+
+		$this->assertStringNotContainsString('<style>', $sPlainText, 'Style tag must not appear in plain text');
+		$this->assertStringNotContainsString('color:', $sPlainText, 'CSS color rule must not appear in plain text');
+		$this->assertStringNotContainsString('font-size:', $sPlainText, 'CSS font-size rule must not appear in plain text');
+		$this->assertStringNotContainsString('@media', $sPlainText, 'CSS @media rule must not appear in plain text');
+		$this->assertStringContainsString('Hello there!', $sPlainText, 'Actual content must be preserved in plain text');
+	}
+
+	/**
+	 * The HTML part must contain the body content and the CSS inlined by Emogrifier.
+	 * This guards against regressions where the wrong body (e.g. the plain-text version)
+	 * would end up in the HTML part.
+	 *
+	 * @covers \Combodo\iTop\Core\Email\EmailSymfony::SetBody()
+	 * @since N°9574
+	 */
+	public function testSetBodyHtmlPartContainsBodyAndInlinedCss(): void
+	{
+		$oEmail = new EMailSymfony();
+		$oEmail->SetBody('<html><body><p>Hello there!</p></body></html>', 'text/html', 'p { color: red; }');
+
+		$aParts = $this->GetAlternativePartsFromHtmlEmail($oEmail);
+
+		$oHtmlPart = null;
+		foreach ($aParts as $oPart) {
+			if ($oPart instanceof TextPart && $oPart->getMediaSubtype() === 'html') {
+				$oHtmlPart = $oPart;
+				break;
+			}
+		}
+		$this->assertNotNull($oHtmlPart, 'No text/html part found in the message');
+
+		$sHtmlContent = $oHtmlPart->getBody();
+		$this->assertStringContainsString('Hello there!', $sHtmlContent, 'HTML part must preserve the original text content');
+		$this->assertStringContainsString('color: red', $sHtmlContent, 'HTML part must contain the CSS inlined by Emogrifier');
+	}
+
+	/**
+	 * With inline images, SetBody() wraps the AlternativePart in a RelatedPart.
+	 * The AlternativePart must still be correctly ordered (plain first, HTML last)
+	 * and the plain-text part must not contain CSS.
+	 *
+	 * @see https://www.rfc-editor.org/rfc/rfc2046.html#section-5.1.4
+	 * @covers \Combodo\iTop\Core\Email\EmailSymfony::SetBody()
+	 * @since N°9574
+	 */
+	public function testSetBodyWithInlineImagesHasCorrectPartStructure(): void
+	{
+		// Anonymous subclass so we can inject a fake inline image part without a real inline image in DB
+		$oEmail = new class () extends EMailSymfony {
+			protected function EmbedInlineImages(string &$sBody): array
+			{
+				return [new DataPart('fake-image-data', 'image.png', 'image/png')];
+			}
+		};
+		$oEmail->SetBody('<html><head><style>p { color: red; }</style></head><body><p>Hello there!</p></body></html>', 'text/html');
+
+		$oSymfonyMessage = $this->GetNonPublicProperty($oEmail, 'm_oMessage');
+		$oBody = $oSymfonyMessage->getBody();
+
+		// Root must be a RelatedPart when inline images are present
+		$this->assertInstanceOf(RelatedPart::class, $oBody, 'Root part must be multipart/related when inline images are present');
+
+		// The AlternativePart must be the first child of the RelatedPart
+		$aRelatedParts = $oBody->getParts();
+		$this->assertInstanceOf(AlternativePart::class, $aRelatedParts[0], 'First child of RelatedPart must be the AlternativePart');
+
+		// Order and CSS checks are delegated to the shared helper, which now handles RelatedPart
+		[$oFirstPart, $oSecondPart] = $this->GetAlternativePartsFromHtmlEmail($oEmail);
+		$this->assertSame('plain', $oFirstPart->getMediaSubtype(), 'First part must be text/plain (least preferred per RFC 2046)');
+		$this->assertSame('html', $oSecondPart->getMediaSubtype(), 'Last part must be text/html (most preferred per RFC 2046)');
+	}
+
+	public function provideSetBodyPlainTextDoesNotContainCss(): array
+	{
+		$sCustomStyles = 'p { color: blue; font-size: 14px; }';
+
+		return [
+			'<style> tag in HTML, no custom styles' => [
+				'<html><head><style>body { color: red; font-size: 12px; } @media print { p { color: black; } }</style></head><body><p>Hello there!</p></body></html>',
+				null,
+			],
+			'<style> tag in HTML with custom styles' => [
+				'<html><head><style>body { color: red; font-size: 12px; } @media print { p { color: black; } }</style></head><body><p>Hello there!</p></body></html>',
+				$sCustomStyles,
+			],
+			'custom styles only, no <style> tag' => [
+				'<html><body><p>Hello there!</p></body></html>',
+				$sCustomStyles,
+			],
+		];
+	}
 }