Merge pull request #9262 from nupplaphil/phpmailer_fix

E-Mail Message-ID & Newline fix

src/Object/Email.php

@@ -137,10 +137,10 @@ class Email implements IEmail
 		foreach ($this->additionalMailHeader as $name => $values) {
 			if (is_array($values)) {
 				foreach ($values as $value) {
-					$headerString .= $name . ': ' . $value . '\n';
+					$headerString .= "$name : $value\n";
 				}
 			} else {
-				$headerString .= $name . ': ' . $values . '\n';
+				$headerString .= "$name : $values\n";
 			}
 		}
 		return $headerString;

src/Util/Emailer.php

@@ -134,6 +134,17 @@ class Emailer
 			return true;
 		}
 
+		// @see https://github.com/friendica/friendica/issues/9142
+		$countMessageId = 0;
+		foreach ($email->getAdditionalMailHeader() as $name => $value) {
+			if (strtolower($name) == 'message-id') {
+				$countMessageId += count($value);
+			}
+		}
+		if ($countMessageId > 0) {
+			$this->logger->warning('More than one Message-ID found - RFC violation', ['email' => $email]);
+		}
+
 		$email_textonly = false;
 		if (!empty($email->getRecipientUid())) {
 			$email_textonly = $this->pConfig->get($email->getRecipientUid(), 'system', 'email_textonly');
@@ -197,15 +208,34 @@ class Emailer
 			return true;
 		}
 
-		$res = mail(
+		$res = $this->mail(
 			$hookdata['to'],
 			$hookdata['subject'],
 			$hookdata['body'],
 			$hookdata['headers'],
 			$hookdata['parameters']
 		);
+
 		$this->logger->debug('header ' . 'To: ' . $email->getToAddress() . '\n' . $messageHeader);
 		$this->logger->debug('return value ' . (($res) ? 'true' : 'false'));
+
 		return $res;
 	}
+
+	/**
+	 * Wrapper around the mail() method (mainly used to overwrite for tests)
+	 * @see mail()
+	 *
+	 * @param string $to         Recipient of this mail
+	 * @param string $subject    Subject of this mail
+	 * @param string $body       Message body of this mail
+	 * @param string $headers    Headers of this mail
+	 * @param string $parameters Additional (sendmail) parameters of this mail
+	 *
+	 * @return boolean true if the mail was successfully accepted for delivery, false otherwise.
+	 */
+	protected function mail(string $to, string $subject, string $body, string $headers, string $parameters)
+	{
+		return mail($to, $subject, $body, $headers, $parameters);
+	}
 }

tests/Util/EmailerSpy.php

@@ -0,0 +1,23 @@
+<?php
+
+namespace Friendica\Test\Util;
+
+use Friendica\Util\Emailer;
+
+class EmailerSpy extends Emailer
+{
+	public static $MAIL_DATA;
+
+	protected function mail(string $to, string $subject, string $body, string $headers, string $parameters)
+	{
+		self::$MAIL_DATA = [
+			'to' => $to,
+			'subject' => $subject,
+			'body' => $body,
+			'headers' => $headers,
+			'parameters' => $parameters,
+		];
+
+		return true;
+	}
+}

tests/Util/HookMockTrait.php

@@ -0,0 +1,30 @@
+<?php
+
+namespace Friendica\Test\Util;
+
+use Friendica\Core\Hook;
+use Mockery\MockInterface;
+
+trait HookMockTrait
+{
+
+	/**
+	 * @var MockInterface The Interface for mocking a renderer
+	 */
+	private $hookMock;
+
+	/**
+	 * Mocking a method 'Hook::call()' call
+	 *
+	 */
+	public function mockHookCallAll(string $name, &$capture)
+	{
+		if (!isset($this->hookMock)) {
+			$this->hookMock = \Mockery::mock('alias:' . Hook::class);
+		}
+
+		$this->hookMock
+			->shouldReceive('callAll')
+			->withArgs([$name, \Mockery::capture($capture)]);
+	}
+}

tests/src/Util/EMailerTest.php

@@ -0,0 +1,133 @@
+<?php
+
+namespace Friendica\Test\src\Util;
+
+use Friendica\App\BaseURL;
+use Friendica\Core\Config\IConfig;
+use Friendica\Core\L10n;
+use Friendica\Core\PConfig\IPConfig;
+use Friendica\Object\EMail\IEmail;
+use Friendica\Test\MockedTest;
+use Friendica\Test\Util\EmailerSpy;
+use Friendica\Test\Util\HookMockTrait;
+use Friendica\Test\Util\SampleMailBuilder;
+use Friendica\Test\Util\VFSTrait;
+use Mockery\MockInterface;
+use Psr\Log\NullLogger;
+
+/**
+ * Annotation necessary because of Hook calls
+ *
+ * @runTestsInSeparateProcesses
+ * @preserveGlobalState disabled
+ */
+class EMailerTest extends MockedTest
+{
+	use VFSTrait;
+	use HookMockTrait;
+
+	/** @var IConfig|MockInterface */
+	private $config;
+	/** @var IPConfig|MockInterface */
+	private $pConfig;
+	/** @var L10n|MockInterface */
+	private $l10n;
+	/** @var BaseURL|MockInterface */
+	private $baseUrl;
+
+	/** @var string */
+	private $defaultHeaders;
+
+	protected function setUp()
+	{
+		parent::setUp();
+
+		$this->setUpVfsDir();
+
+		$this->config  = \Mockery::mock(IConfig::class);
+		$this->config->shouldReceive('get')->withArgs(['config', 'sender_email'])->andReturn('test@friendica.local')->once();
+		$this->config->shouldReceive('get')->withArgs(['config', 'sitename', 'Friendica Social Network'])->andReturn('Friendica Social Network')->once();
+		$this->config->shouldReceive('get')->withArgs(['system', 'sendmail_params', true])->andReturn(true);
+
+		$this->pConfig = \Mockery::mock(IPConfig::class);
+		$this->l10n    = \Mockery::mock(L10n::class);
+		$this->baseUrl = \Mockery::mock(BaseURL::class);
+		$this->baseUrl->shouldReceive('getHostname')->andReturn('friendica.local');
+		$this->baseUrl->shouldReceive('get')->andReturn('http://friendica.local');
+
+		$this->defaultHeaders = [];
+	}
+
+	protected function tearDown()
+	{
+		EmailerSpy::$MAIL_DATA = [];
+
+		parent::tearDown();
+	}
+
+	public function testEmail()
+	{
+		$this->pConfig->shouldReceive('get')->withArgs(['1', 'system', 'email_textonly'])->andReturn(false)->once();
+
+		$builder = new SampleMailBuilder($this->l10n, $this->baseUrl, $this->config, new NullLogger());
+
+		$testEmail = $builder
+			->withRecipient('recipient@friendica.local')
+			->withMessage('Test Subject', "Test Message<b>Bold</b>", 'Test Text')
+			->withSender('Sender', 'sender@friendica.local')
+			->forUser(['uid' => 1])
+			->addHeader('Message-ID', 'first Id')
+			->build(true);
+
+		$emailer = new EmailerSpy($this->config, $this->pConfig, $this->baseUrl, new NullLogger(), $this->l10n);
+
+		$this->assertTrue($emailer->send($testEmail));
+
+		$this->assertContains("X-Friendica-Host : friendica.local", EmailerSpy::$MAIL_DATA['headers']);
+		$this->assertContains("X-Friendica-Platform : Friendica", EmailerSpy::$MAIL_DATA['headers']);
+		$this->assertContains("List-ID : <notification.friendica.local>", EmailerSpy::$MAIL_DATA['headers']);
+		$this->assertContains("List-Archive : <http://friendica.local/notifications/system>", EmailerSpy::$MAIL_DATA['headers']);
+		$this->assertContains("Reply-To: Sender <sender@friendica.local>", EmailerSpy::$MAIL_DATA['headers']);
+		$this->assertContains("MIME-Version: 1.0", EmailerSpy::$MAIL_DATA['headers']);
+		// Base64 "Test Text"
+		$this->assertContains(chunk_split(base64_encode('Test Text')), EmailerSpy::$MAIL_DATA['body']);
+		// Base64 "Test Message<b>Bold</b>"
+		$this->assertContains(chunk_split(base64_encode("Test Message<b>Bold</b>")), EmailerSpy::$MAIL_DATA['body']);
+		$this->assertEquals("Test Subject", EmailerSpy::$MAIL_DATA['subject']);
+		$this->assertEquals("recipient@friendica.local", EmailerSpy::$MAIL_DATA['to']);
+		$this->assertEquals("-f sender@friendica.local", EmailerSpy::$MAIL_DATA['parameters']);
+	}
+
+	public function testTwoMessageIds()
+	{
+		$this->pConfig->shouldReceive('get')->withArgs(['1', 'system', 'email_textonly'])->andReturn(false)->once();
+
+		/** @var IEmail $preparedEmail */
+		$preparedEmail = null;
+		/** @var IEmail $sentEMail */
+		$sentEMail = null;
+
+		$this->mockHookCallAll('emailer_send_prepare', $preparedEmail);
+		$this->mockHookCallAll('emailer_send', $sentEMail);
+
+		$builder = new SampleMailBuilder($this->l10n, $this->baseUrl, $this->config, new NullLogger());
+
+		$testEmail = $builder
+			->withRecipient('recipient@friendica.local')
+			->withMessage('Test Subject', "Test Message<b>Bold</b>", 'Test Text')
+			->withSender('Sender', 'sender@friendica.loca')
+			->forUser(['uid' => 1])
+			->addHeader('Message-ID', 'first Id')
+			->addHeader('Message-Id', 'second Id')
+			->build(true);
+
+		$emailer = new EmailerSpy($this->config, $this->pConfig, $this->baseUrl, new NullLogger(), $this->l10n);
+
+		// even in case there are two message ids, send the mail anyway
+		$this->assertTrue($emailer->send($testEmail));
+
+		// check case sensitive key problem
+		$this->assertArrayHasKey('Message-ID', $testEmail->getAdditionalMailHeader());
+		$this->assertArrayHasKey('Message-Id', $testEmail->getAdditionalMailHeader());
+	}
+}

tests/src/Util/Emailer/MailBuilderTest.php

@@ -29,6 +29,7 @@ use Friendica\Test\MockedTest;
 use Friendica\Test\Util\SampleMailBuilder;
 use Friendica\Test\Util\VFSTrait;
 use Friendica\Util\EMailer\MailBuilder;
+use Mockery\MockInterface;
 use Psr\Log\NullLogger;
 
 /**
@@ -39,11 +40,11 @@ class MailBuilderTest extends MockedTest
 {
 	use VFSTrait;
 
-	/** @var IConfig */
+	/** @var IConfig|MockInterface */
 	private $config;
-	/** @var L10n */
+	/** @var L10n|MockInterface */
 	private $l10n;
-	/** @var BaseURL */
+	/** @var BaseURL|MockInterface */
 	private $baseUrl;
 
 	/** @var string */