Merge pull request #115 from APaikens/MailAttachmentReset-patch

Mail attachment reset patch

Author: arhimede

src/Email.php

@@ -297,7 +297,7 @@ private function generateBody(): AbstractPart
             }
         }
 
-        return $part ?? new TextPart($this->text, $this->textCharset);
+        return $part ?? new TextPart((string) $this->text, $this->textCharset);
     }
 
     private function prepareParts(): array

src/Service/MailService.php

@@ -70,6 +70,12 @@ public function send(): ResultInterface
             //trigger error event
             $this->getEventManager()->triggerEvent($this->createMailEvent(MailEvent::EVENT_MAIL_SEND_ERROR, $result));
             throw new MailException($result->getMessage());
+        } finally {
+            // Reset the Symfony Message body after every send attempt (success or failure).
+            // attachFiles() embeds DataParts into Message::$body; without this reset those
+            // DataParts survive a failed send and get wrapped into the next email's body,
+            // causing attachments from a failed send to leak into subsequent emails.
+            $this->getMessage()->setBody(null);
         }
 
         if ($result->isValid()) {

test/Service/MailServiceTest.php

@@ -17,8 +17,11 @@
 use PHPUnit\Framework\MockObject\Exception;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
+use ReflectionClass;
 use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport;
 use Symfony\Component\Mailer\Transport\TransportInterface;
+use Symfony\Component\Mime\Message;
+use Symfony\Component\Mime\Part\AbstractPart;
 use Symfony\Component\Mime\Part\DataPart;
 use Symfony\Component\Mime\Part\Multipart\MixedPart;
 use Symfony\Component\Mime\Part\TextPart;
@@ -292,4 +295,136 @@ public function testAttachFilesNoNestedMixedParts(): void
             $this->assertNotInstanceOf(MixedPart::class, $part);
         }
     }
+
+    /**
+     * Found an attachment leak where DataParts
+     * embedded into Message::$body by attachFiles() survived a failed send
+     * and were wrapped into the next email. The fix must live in dot-mail:
+     * after send() (success OR failure) the underlying Message::$body must
+     * be reset so the next send rebuilds the body from scratch.
+     */
+    public function testMessageBodyIsResetAfterFailedSendToPreventAttachmentLeak(): void
+    {
+        $this->mailService->setSubject('First');
+        $this->message->html('First body');
+        $this->mailService->addAttachment(
+            $this->fileSystem->url() . '/data/mail/attachments/testPdfAttachment.pdf'
+        );
+
+        $this->transportInterface
+            ->method('send')
+            ->willThrowException(new RuntimeException('SMTP failure'));
+
+        try {
+            $this->mailService->send();
+            $this->fail('Expected MailException was not thrown');
+        } catch (MailException) {
+            // expected
+        }
+
+        $this->assertNull(
+            $this->readPrivateMessageBody($this->message),
+            'After a failed send the Symfony Message::$body must be reset; '
+            . 'otherwise attachFiles() will wrap the leaked MixedPart into the next email.'
+        );
+    }
+
+    /**
+     * End-to-end regression for the same leak: after a failed send with
+     * attachments, configure a fresh email with no attachments, and verify
+     * the message handed to the transport contains no DataPart from the
+     * previous send.
+     */
+    public function testAttachmentsDoNotLeakIntoSubsequentSendAfterFailure(): void
+    {
+        $this->mailService->setSubject('First');
+        $this->message->html('First email');
+        $this->mailService->addAttachment(
+            $this->fileSystem->url() . '/data/mail/attachments/testPdfAttachment.pdf'
+        );
+
+        $capturedSecondBody = null;
+        $callCount          = 0;
+        $this->transportInterface
+            ->method('send')
+            ->willReturnCallback(function ($email) use (&$capturedSecondBody, &$callCount) {
+                $callCount++;
+                if ($callCount === 1) {
+                    throw new RuntimeException('first send fails');
+                }
+                $capturedSecondBody = $email->getBody();
+                return null;
+            });
+
+        try {
+            $this->mailService->send();
+        } catch (MailException) {
+            // expected
+        }
+
+        // Reconfigure: brand new email with NO attachments.
+        $this->mailService->setAttachments([]);
+        $this->mailService->setSubject('Second');
+        $this->message->html('Second email - must not carry first email\'s attachment');
+
+        $this->mailService->send();
+
+        $this->assertInstanceOf(
+            AbstractPart::class,
+            $capturedSecondBody,
+            'Transport did not receive a second message body'
+        );
+        $this->assertFalse(
+            $this->bodyContainsDataPart($capturedSecondBody),
+            'Second email leaked a DataPart from the failed first send'
+        );
+    }
+
+    /**
+     * Confirms the success path also resets the body, so a future refactor
+     * does not silently regress the success case.
+     */
+    public function testMessageBodyIsResetAfterSuccessfulSend(): void
+    {
+        $this->mailService->setSubject('Hello');
+        $this->message->html('Body');
+        $this->mailService->addAttachment(
+            $this->fileSystem->url() . '/data/mail/attachments/testPdfAttachment.pdf'
+        );
+
+        $this->transportInterface->expects($this->once())->method('send');
+
+        $this->mailService->send();
+
+        $this->assertNull(
+            $this->readPrivateMessageBody($this->message),
+            'Message body should be null after a successful send so the next email starts clean.'
+        );
+    }
+
+    private function readPrivateMessageBody(Message $message): ?AbstractPart
+    {
+        $reflection = new ReflectionClass(Message::class);
+        $property   = $reflection->getProperty('body');
+        $property->setAccessible(true);
+
+        return $property->getValue($message);
+    }
+
+    private function bodyContainsDataPart(AbstractPart $part): bool
+    {
+        if ($part instanceof DataPart) {
+            return true;
+        }
+
+        if ($part instanceof MixedPart) {
+            foreach ($part->getParts() as $child) {
+                if ($this->bodyContainsDataPart($child)) {
+                    return true;
+                }
+            }
+        }
+
+        return false;
+    }
 }