From 545517e85f9ae5c27334b16d6e9cedecc41063ed Mon Sep 17 00:00:00 2001 From: Philipp Date: Tue, 22 Sep 2020 20:52:48 +0200 Subject: [PATCH 1/5] Add protected mail function for testability --- src/Util/Emailer.php | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/Util/Emailer.php b/src/Util/Emailer.php index 0594937ec..b402abf25 100644 --- a/src/Util/Emailer.php +++ b/src/Util/Emailer.php @@ -197,15 +197,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); + } } From b1a402a787821e33d16c279eef371335e07b017e Mon Sep 17 00:00:00 2001 From: Philipp Date: Tue, 22 Sep 2020 21:08:34 +0200 Subject: [PATCH 2/5] Fix newline email error --- src/Object/Email.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Object/Email.php b/src/Object/Email.php index 9f7876312..008e68c4b 100644 --- a/src/Object/Email.php +++ b/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; From e2b736d0a9b351965cc2310696dbbaeb4f19ebb2 Mon Sep 17 00:00:00 2001 From: Philipp Date: Tue, 22 Sep 2020 22:48:34 +0200 Subject: [PATCH 3/5] Fix phpmailer (Case sensitive Check) --- src/Util/Emailer.php | 12 ++ tests/Util/EmailerSpy.php | 23 ++++ tests/Util/HookMockTrait.php | 30 +++++ tests/src/Util/EMailerTest.php | 127 +++++++++++++++++++++ tests/src/Util/Emailer/MailBuilderTest.php | 7 +- 5 files changed, 196 insertions(+), 3 deletions(-) create mode 100644 tests/Util/EmailerSpy.php create mode 100644 tests/Util/HookMockTrait.php create mode 100644 tests/src/Util/EMailerTest.php diff --git a/src/Util/Emailer.php b/src/Util/Emailer.php index b402abf25..203f3b4f7 100644 --- a/src/Util/Emailer.php +++ b/src/Util/Emailer.php @@ -134,6 +134,18 @@ 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]); + return false; + } + $email_textonly = false; if (!empty($email->getRecipientUid())) { $email_textonly = $this->pConfig->get($email->getRecipientUid(), 'system', 'email_textonly'); diff --git a/tests/Util/EmailerSpy.php b/tests/Util/EmailerSpy.php new file mode 100644 index 000000000..effd2ed08 --- /dev/null +++ b/tests/Util/EmailerSpy.php @@ -0,0 +1,23 @@ + $to, + 'subject' => $subject, + 'body' => $body, + 'headers' => $headers, + 'parameters' => $parameters, + ]; + + return true; + } +} diff --git a/tests/Util/HookMockTrait.php b/tests/Util/HookMockTrait.php new file mode 100644 index 000000000..68a17c5bd --- /dev/null +++ b/tests/Util/HookMockTrait.php @@ -0,0 +1,30 @@ +hookMock)) { + $this->hookMock = \Mockery::mock('alias:' . Hook::class); + } + + $this->hookMock + ->shouldReceive('callAll') + ->withArgs([$name, \Mockery::capture($capture)]); + } +} diff --git a/tests/src/Util/EMailerTest.php b/tests/src/Util/EMailerTest.php new file mode 100644 index 000000000..636965ec4 --- /dev/null +++ b/tests/src/Util/EMailerTest.php @@ -0,0 +1,127 @@ +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 MessageBold", 'Test Text') + ->withSender('Sender', 'sender@friendica.loca') + ->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 : ", EmailerSpy::$MAIL_DATA['headers']); + $this->assertContains("List-Archive : ", EmailerSpy::$MAIL_DATA['headers']); + $this->assertContains("Reply-To: Sender ", 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 MessageBold" + $this->assertContains(chunk_split(base64_encode("Test MessageBold")), 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 testWrongReturnTwoMessageIds() + { + /** @var IEmail $returnMail */ + $returnMail = null; + + $this->mockHookCallAll('emailer_send_prepare', $returnMail); + + $builder = new SampleMailBuilder($this->l10n, $this->baseUrl, $this->config, new NullLogger()); + + $testEmail = $builder + ->withRecipient('recipient@friendica.local') + ->withMessage('Test Subject', "Test MessageBold", '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); + + $this->assertFalse($emailer->send($testEmail)); + + // check case sensitive key problem + $this->assertArrayHasKey('Message-ID', $testEmail->getAdditionalMailHeader()); + $this->assertArrayHasKey('Message-Id', $testEmail->getAdditionalMailHeader()); + } +} diff --git a/tests/src/Util/Emailer/MailBuilderTest.php b/tests/src/Util/Emailer/MailBuilderTest.php index 202ad587b..b89022421 100644 --- a/tests/src/Util/Emailer/MailBuilderTest.php +++ b/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 */ From 3eaaf716e91f4e4acd542443122157fe959c0626 Mon Sep 17 00:00:00 2001 From: Philipp Date: Wed, 23 Sep 2020 19:38:20 +0200 Subject: [PATCH 4/5] just log double message IDs, don't discard the email --- src/Util/Emailer.php | 1 - tests/src/Util/EMailerTest.php | 20 +++++++++++++------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/Util/Emailer.php b/src/Util/Emailer.php index 203f3b4f7..ed6c7b331 100644 --- a/src/Util/Emailer.php +++ b/src/Util/Emailer.php @@ -143,7 +143,6 @@ class Emailer } if ($countMessageId > 0) { $this->logger->warning('More than one Message-ID found - RFC violation', ['email' => $email]); - return false; } $email_textonly = false; diff --git a/tests/src/Util/EMailerTest.php b/tests/src/Util/EMailerTest.php index 636965ec4..fa771f3ed 100644 --- a/tests/src/Util/EMailerTest.php +++ b/tests/src/Util/EMailerTest.php @@ -74,7 +74,7 @@ class EMailerTest extends MockedTest $testEmail = $builder ->withRecipient('recipient@friendica.local') ->withMessage('Test Subject', "Test MessageBold", 'Test Text') - ->withSender('Sender', 'sender@friendica.loca') + ->withSender('Sender', 'sender@friendica.local') ->forUser(['uid' => 1]) ->addHeader('Message-ID', 'first Id') ->build(true); @@ -87,7 +87,7 @@ class EMailerTest extends MockedTest $this->assertContains("X-Friendica-Platform : Friendica", EmailerSpy::$MAIL_DATA['headers']); $this->assertContains("List-ID : ", EmailerSpy::$MAIL_DATA['headers']); $this->assertContains("List-Archive : ", EmailerSpy::$MAIL_DATA['headers']); - $this->assertContains("Reply-To: Sender ", EmailerSpy::$MAIL_DATA['headers']); + $this->assertContains("Reply-To: Sender ", 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']); @@ -98,12 +98,17 @@ class EMailerTest extends MockedTest $this->assertEquals("-f sender@friendica.local", EmailerSpy::$MAIL_DATA['parameters']); } - public function testWrongReturnTwoMessageIds() + public function testTwoMessageIds() { - /** @var IEmail $returnMail */ - $returnMail = null; + $this->pConfig->shouldReceive('get')->withArgs(['1', 'system', 'email_textonly'])->andReturn(false)->once(); - $this->mockHookCallAll('emailer_send_prepare', $returnMail); + /** @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()); @@ -118,7 +123,8 @@ class EMailerTest extends MockedTest $emailer = new EmailerSpy($this->config, $this->pConfig, $this->baseUrl, new NullLogger(), $this->l10n); - $this->assertFalse($emailer->send($testEmail)); + // 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()); From fae7256d82e588d0d7adb6cc177f3286efde5c23 Mon Sep 17 00:00:00 2001 From: Philipp Date: Wed, 23 Sep 2020 19:46:24 +0200 Subject: [PATCH 5/5] wrong annotation for preserverGlobalState .. --- tests/src/Util/EMailerTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/Util/EMailerTest.php b/tests/src/Util/EMailerTest.php index fa771f3ed..209195f7e 100644 --- a/tests/src/Util/EMailerTest.php +++ b/tests/src/Util/EMailerTest.php @@ -19,7 +19,7 @@ use Psr\Log\NullLogger; * Annotation necessary because of Hook calls * * @runTestsInSeparateProcesses - * @preserveGlobalState false + * @preserveGlobalState disabled */ class EMailerTest extends MockedTest {