From dc376f52a5a309c653a52d990b0c1044dbc3f3c3 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Thu, 5 Sep 2019 14:01:23 +0200 Subject: [PATCH] [Mailer] Check email validity before opening an SMTP connection --- .../Transport/SendgridApiTransportTest.php | 8 +++--- src/Symfony/Component/Mailer/SentMessage.php | 2 ++ .../Mailer/Transport/Smtp/SmtpTransport.php | 20 +++++++------- .../Transport/Smtp/Stream/SocketStream.php | 2 +- src/Symfony/Component/Mime/Email.php | 14 +++++++--- src/Symfony/Component/Mime/Message.php | 9 +++++++ src/Symfony/Component/Mime/RawMessage.php | 9 +++++++ .../Component/Mime/Tests/EmailTest.php | 27 +++++++++---------- 8 files changed, 61 insertions(+), 30 deletions(-) diff --git a/src/Symfony/Component/Mailer/Bridge/Sendgrid/Tests/Transport/SendgridApiTransportTest.php b/src/Symfony/Component/Mailer/Bridge/Sendgrid/Tests/Transport/SendgridApiTransportTest.php index ea5e91af1a..301824684b 100644 --- a/src/Symfony/Component/Mailer/Bridge/Sendgrid/Tests/Transport/SendgridApiTransportTest.php +++ b/src/Symfony/Component/Mailer/Bridge/Sendgrid/Tests/Transport/SendgridApiTransportTest.php @@ -50,7 +50,8 @@ class SendgridApiTransportTest extends TestCase $email = new Email(); $email->from('foo@example.com') ->to('bar@example.com') - ->bcc('baz@example.com'); + ->bcc('baz@example.com') + ->text('content'); $response = $this->createMock(ResponseInterface::class); @@ -74,14 +75,15 @@ class SendgridApiTransportTest extends TestCase ], ], 'from' => ['email' => 'foo@example.com'], - 'content' => [], + 'content' => [ + ['type' => 'text/plain', 'value' => 'content'], + ], ], 'auth_bearer' => 'foo', ]) ->willReturn($response); $mailer = new SendgridApiTransport('foo', $httpClient); - $mailer->send($email); } } diff --git a/src/Symfony/Component/Mailer/SentMessage.php b/src/Symfony/Component/Mailer/SentMessage.php index 5ed2acabaf..f90b25e363 100644 --- a/src/Symfony/Component/Mailer/SentMessage.php +++ b/src/Symfony/Component/Mailer/SentMessage.php @@ -29,6 +29,8 @@ class SentMessage */ public function __construct(RawMessage $message, SmtpEnvelope $envelope) { + $message->ensureValidity(); + $this->raw = $message instanceof Message ? new RawMessage($message->toIterable()) : $message; $this->original = $message; $this->envelope = $envelope; diff --git a/src/Symfony/Component/Mailer/Transport/Smtp/SmtpTransport.php b/src/Symfony/Component/Mailer/Transport/Smtp/SmtpTransport.php index 71f684431a..40a393a1c7 100644 --- a/src/Symfony/Component/Mailer/Transport/Smtp/SmtpTransport.php +++ b/src/Symfony/Component/Mailer/Transport/Smtp/SmtpTransport.php @@ -104,18 +104,15 @@ class SmtpTransport extends AbstractTransport public function send(RawMessage $message, SmtpEnvelope $envelope = null): ?SentMessage { - $this->ping(); - if (!$this->started) { - $this->start(); - } - try { $message = parent::send($message, $envelope); } catch (TransportExceptionInterface $e) { - try { - $this->executeCommand("RSET\r\n", [250]); - } catch (TransportExceptionInterface $_) { - // ignore this exception as it probably means that the server error was final + if ($this->started) { + try { + $this->executeCommand("RSET\r\n", [250]); + } catch (TransportExceptionInterface $_) { + // ignore this exception as it probably means that the server error was final + } } throw $e; @@ -163,6 +160,11 @@ class SmtpTransport extends AbstractTransport protected function doSend(SentMessage $message): void { + $this->ping(); + if (!$this->started) { + $this->start(); + } + try { $envelope = $message->getEnvelope(); $this->doMailFromCommand($envelope->getSender()->getAddress()); diff --git a/src/Symfony/Component/Mailer/Transport/Smtp/Stream/SocketStream.php b/src/Symfony/Component/Mailer/Transport/Smtp/Stream/SocketStream.php index 09c03660c3..debeeb4b01 100644 --- a/src/Symfony/Component/Mailer/Transport/Smtp/Stream/SocketStream.php +++ b/src/Symfony/Component/Mailer/Transport/Smtp/Stream/SocketStream.php @@ -26,7 +26,7 @@ final class SocketStream extends AbstractStream private $url; private $host = 'localhost'; private $port = 465; - private $timeout = 15; + private $timeout = 5; private $tls = true; private $sourceIp; private $streamContextOptions = []; diff --git a/src/Symfony/Component/Mime/Email.php b/src/Symfony/Component/Mime/Email.php index 9c1a1c9eff..7ecea4711a 100644 --- a/src/Symfony/Component/Mime/Email.php +++ b/src/Symfony/Component/Mime/Email.php @@ -399,6 +399,15 @@ class Email extends Message return $this->generateBody(); } + public function ensureValidity() + { + if (null === $this->text && null === $this->html && !$this->attachments) { + throw new LogicException('A message must have a text or an HTML part or attachments.'); + } + + parent::ensureValidity(); + } + /** * Generates an AbstractPart based on the raw body of a message. * @@ -421,10 +430,9 @@ class Email extends Message */ private function generateBody(): AbstractPart { + $this->ensureValidity(); + [$htmlPart, $attachmentParts, $inlineParts] = $this->prepareParts(); - if (null === $this->text && null === $this->html && !$attachmentParts) { - throw new LogicException('A message must have a text or an HTML part or attachments.'); - } $part = null === $this->text ? null : new TextPart($this->text, $this->textCharset); if (null !== $htmlPart) { diff --git a/src/Symfony/Component/Mime/Message.php b/src/Symfony/Component/Mime/Message.php index d141cb525e..d927177302 100644 --- a/src/Symfony/Component/Mime/Message.php +++ b/src/Symfony/Component/Mime/Message.php @@ -123,6 +123,15 @@ class Message extends RawMessage yield from $body->toIterable(); } + public function ensureValidity() + { + if (!$this->headers->has('From')) { + throw new LogicException('An email must have a "From" header.'); + } + + parent::ensureValidity(); + } + private function generateMessageId(string $email): string { return bin2hex(random_bytes(16)).strstr($email, '@'); diff --git a/src/Symfony/Component/Mime/RawMessage.php b/src/Symfony/Component/Mime/RawMessage.php index 8a01049e51..79a27e9fcc 100644 --- a/src/Symfony/Component/Mime/RawMessage.php +++ b/src/Symfony/Component/Mime/RawMessage.php @@ -11,6 +11,8 @@ namespace Symfony\Component\Mime; +use Symfony\Component\Mime\Exception\LogicException; + /** * @author Fabien Potencier */ @@ -51,6 +53,13 @@ class RawMessage implements \Serializable $this->message = $message; } + /** + * @throws LogicException if the message is not valid + */ + public function ensureValidity() + { + } + /** * @internal */ diff --git a/src/Symfony/Component/Mime/Tests/EmailTest.php b/src/Symfony/Component/Mime/Tests/EmailTest.php index 59c7e8948f..eb792c212b 100644 --- a/src/Symfony/Component/Mime/Tests/EmailTest.php +++ b/src/Symfony/Component/Mime/Tests/EmailTest.php @@ -251,62 +251,62 @@ class EmailTest extends TestCase $att = new DataPart($file = fopen(__DIR__.'/Fixtures/mimetypes/test', 'r')); $img = new DataPart($image = fopen(__DIR__.'/Fixtures/mimetypes/test.gif', 'r'), 'test.gif'); - $e = new Email(); + $e = (new Email())->from('me@example.com'); $e->text('text content'); $this->assertEquals($text, $e->getBody()); $this->assertEquals('text content', $e->getTextBody()); - $e = new Email(); + $e = (new Email())->from('me@example.com'); $e->html('html content'); $this->assertEquals($html, $e->getBody()); $this->assertEquals('html content', $e->getHtmlBody()); - $e = new Email(); + $e = (new Email())->from('me@example.com'); $e->html('html content'); $e->text('text content'); $this->assertEquals(new AlternativePart($text, $html), $e->getBody()); - $e = new Email(); + $e = (new Email())->from('me@example.com'); $e->html('html content', 'iso-8859-1'); $e->text('text content', 'iso-8859-1'); $this->assertEquals('iso-8859-1', $e->getTextCharset()); $this->assertEquals('iso-8859-1', $e->getHtmlCharset()); $this->assertEquals(new AlternativePart(new TextPart('text content', 'iso-8859-1'), new TextPart('html content', 'iso-8859-1', 'html')), $e->getBody()); - $e = new Email(); + $e = (new Email())->from('me@example.com'); $e->attach($file); $e->text('text content'); $this->assertEquals(new MixedPart($text, $att), $e->getBody()); - $e = new Email(); + $e = (new Email())->from('me@example.com'); $e->attach($file); $e->html('html content'); $this->assertEquals(new MixedPart($html, $att), $e->getBody()); - $e = new Email(); + $e = (new Email())->from('me@example.com'); $e->attach($file); $this->assertEquals(new MixedPart($att), $e->getBody()); - $e = new Email(); + $e = (new Email())->from('me@example.com'); $e->html('html content'); $e->text('text content'); $e->attach($file); $this->assertEquals(new MixedPart(new AlternativePart($text, $html), $att), $e->getBody()); - $e = new Email(); + $e = (new Email())->from('me@example.com'); $e->html('html content'); $e->text('text content'); $e->attach($file); $e->attach($image, 'test.gif'); $this->assertEquals(new MixedPart(new AlternativePart($text, $html), $att, $img), $e->getBody()); - $e = new Email(); + $e = (new Email())->from('me@example.com'); $e->text('text content'); $e->attach($file); $e->attach($image, 'test.gif'); $this->assertEquals(new MixedPart($text, $att, $img), $e->getBody()); - $e = new Email(); + $e = (new Email())->from('me@example.com'); $e->html($content = 'html content '); $e->text('text content'); $e->attach($file); @@ -314,13 +314,12 @@ class EmailTest extends TestCase $fullhtml = new TextPart($content, 'utf-8', 'html'); $this->assertEquals(new MixedPart(new AlternativePart($text, $fullhtml), $att, $img), $e->getBody()); - $e = new Email(); + $e = (new Email())->from('me@example.com'); $e->html($content = 'html content '); $e->text('text content'); $e->attach($file); $e->attach($image, 'test.gif'); $fullhtml = new TextPart($content, 'utf-8', 'html'); - $inlinedimg = (new DataPart($image, 'test.gif'))->asInline(); $body = $e->getBody(); $this->assertInstanceOf(MixedPart::class, $body); $this->assertCount(2, $related = $body->getParts()); @@ -336,7 +335,7 @@ class EmailTest extends TestCase fwrite($r, $content); rewind($r); - $e = new Email(); + $e = (new Email())->from('me@example.com'); $e->html($r); // embedding the same image twice results in one image only in the email $e->embed($image, 'test.gif');