Skip to content

[12.x] Fix DataPart API misuse in email embed methods - second parameter is filename, not cid #57606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
+54 −39
Closed

[12.x] Fix DataPart API misuse in email embed methods - second parameter is filename, not cid #57606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 14 additions & 19 deletions src/Illuminate/Mail/Message.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

use Illuminate\Contracts\Mail\Attachable;
use Illuminate\Support\Collection;
use Illuminate\Support\Str;
use Illuminate\Support\Traits\ForwardsCalls;
use Symfony\Component\Mime\Address;
use Symfony\Component\Mime\Email;
Expand Down Expand Up @@ -344,31 +343,27 @@ public function embed($file)
if ($file instanceof Attachment) {
return $file->attachWith(
function ($path) use ($file) {
$cid = $file->as ?? Str::random();
$part = (new DataPart(new File($path), $file->as, $file->mime))->asInline();

$this->message->addPart(
(new DataPart(new File($path), $cid, $file->mime))->asInline()
);
$this->message->addPart($part);

return "cid:{$cid}";
return "cid:{$part->getContentId()}";
},
function ($data) use ($file) {
$this->message->addPart(
(new DataPart($data(), $file->as, $file->mime))->asInline()
);
$part = (new DataPart($data(), $file->as, $file->mime))->asInline();
$this->message->addPart($part);

return "cid:{$file->as}";
return "cid:{$part->getContentId()}";
}
);
}

$cid = Str::random(10);
$fileObject = new File($file);
$part = (new DataPart($fileObject, $fileObject->getFilename()))->asInline();

$this->message->addPart(
(new DataPart(new File($file), $cid))->asInline()
);
$this->message->addPart($part);

return "cid:$cid";
return "cid:{$part->getContentId()}";
}

/**
Expand All @@ -381,11 +376,11 @@ function ($data) use ($file) {
*/
public function embedData($data, $name, $contentType = null)
{
$this->message->addPart(
(new DataPart($data, $name, $contentType))->asInline()
);
$part = (new DataPart($data, $name, $contentType))->asInline();

$this->message->addPart($part);

return "cid:$name";
return "cid:{$part->getContentId()}";
}

/**
Expand Down
2 changes: 2 additions & 0 deletions tests/Integration/Mail/Fixtures/embed.blade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
Embed file: {{ basename(__FILE__) }}

Embed content: {{ $message->embed(__FILE__) }}
18 changes: 11 additions & 7 deletions tests/Integration/Mail/SendingMarkdownMailTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,23 +52,28 @@ public function testEmbed()
$email = app('mailer')->getSymfonyTransport()->messages()[0]->getOriginalMessage()->toString();

$cid = explode(' cid:', (new Stringable($email))->explode("\r\n")
->filter(fn ($line) => str_contains($line, 'Embed content: cid:'))
->filter(fn ($line) => str_contains($line, ' content: cid:'))
->first())[1];

$filename = explode('Embed file: ', (new Stringable($email))->explode("\r\n")
->filter(fn ($line) => str_contains($line, ' file:'))
->first())[1];

$this->assertStringContainsString(<<<EOT
Content-Type: application/x-php; name=$cid\r
Content-Type: application/x-php; name=$filename\r
Content-Transfer-Encoding: base64\r
Content-Disposition: inline; name=$cid; filename=$cid\r
Content-Disposition: inline; name=$filename;\r
filename=$filename\r
Content-ID: <$cid>\r
EOT, $email);
}

public function testEmbedData()
{
Mail::to('test@mail.com')->send($mailable = new EmbedDataMailable());

$mailable->assertSeeInHtml('Embed data content: cid:foo.jpg');
Copy link

@habibalkhabbaz habibalkhabbaz Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it’s removed?

Copy link
Author

@yinheli yinheli Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cid is now automatically generated by DataPart as a random value, so it's been removed. Alternatively, should we just check for the existence of cid instead?

$mailable->assertSeeInHtml('Embed data content: cid:');

Any better suggestions?

Copy link

@habibalkhabbaz habibalkhabbaz Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my point of view, yes we should check cid existence at least as you mentioned instead of removing the check totally.

Thanks

$mailable->assertSeeInText('Embed data content: ');
$mailable->assertDontSeeInText('Embed data content: cid:foo.jpg');
$mailable->assertSeeInHtml('Embed data content: cid:');

$email = app('mailer')->getSymfonyTransport()->messages()[0]->getOriginalMessage()->toString();

Expand All @@ -87,8 +92,7 @@ public function testEmbedMultilineImage()

$this->assertStringContainsString('Embed multiline content: <img', $html);
$this->assertStringContainsString('alt="multiline image"', $html);
$this->assertStringContainsString('data:image/png;base64', $html);
$this->assertStringNotContainsString('cid:foo.jpg', $html);
$this->assertStringContainsString('<img src="cid:', $html);
}

public function testMessageAsPublicPropertyMayBeDefinedAsViewData()
Expand Down
1 change: 1 addition & 0 deletions tests/Integration/Notifications/Fixtures/markdown.blade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
Embed file: {{ basename(__FILE__) }}
Embed content: {{ $message->embed(__FILE__) }}
16 changes: 12 additions & 4 deletions tests/Integration/Notifications/SendingMailableNotificationsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,24 @@ public function testMarkdownNotification()

$user->notify(new MarkdownNotification());

$email = app('mailer')->getSymfonyTransport()->messages()[0]->getOriginalMessage()->toString();
$message = app('mailer')->getSymfonyTransport()->messages()[0]->getOriginalMessage();
$email = $message->toString();
$textBody = $message->getTextBody();

$cid = explode(' cid:', (new Stringable($email))->explode("\r\n")
$cid = explode(' cid:', (new Stringable($textBody))->explode("\n")
->filter(fn ($line) => str_contains($line, 'Embed content: cid:'))
->first())[1];

$filename = explode(' file: ', (new Stringable($textBody))->explode("\n")
->filter(fn ($line) => str_contains($line, 'Embed file: '))
->first())[1];

$this->assertStringContainsString(<<<EOT
Content-Type: application/x-php; name=$cid\r
Content-Type: application/x-php; name=$filename\r
Content-Transfer-Encoding: base64\r
Content-Disposition: inline; name=$cid; filename=$cid\r
Content-Disposition: inline; name=$filename;\r
filename=$filename\r
Content-ID: <$cid>\r
EOT, $email);
}

Expand Down
23 changes: 14 additions & 9 deletions tests/Mail/MailMessageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,13 +165,14 @@ public function testEmbedPath(): void
$cid = $this->message->embed($path);

$this->assertStringStartsWith('cid:', $cid);
$name = Str::after($cid, 'cid:');
$contentId = Str::after($cid, 'cid:');
$attachment = $this->message->getSymfonyMessage()->getAttachments()[0];
$headers = $attachment->getPreparedHeaders()->toArray();
$this->assertSame('bar', $attachment->getBody());
$this->assertSame("Content-Type: image/jpeg; name={$name}", $headers[0]);
$this->assertSame($contentId, $attachment->getContentId());
$this->assertStringContainsString('Content-Type: image/jpeg', $headers[0]);
$this->assertSame('Content-Transfer-Encoding: base64', $headers[1]);
$this->assertSame("Content-Disposition: inline; name={$name}; filename={$name}", $headers[2]);
$this->assertStringContainsString('Content-Disposition: inline', $headers[2]);

unlink($path);
}
Expand All @@ -182,7 +183,9 @@ public function testDataEmbed(): void

$attachment = $this->message->getSymfonyMessage()->getAttachments()[0];
$headers = $attachment->getPreparedHeaders()->toArray();
$this->assertSame('cid:foo.jpg', $cid);
$this->assertStringStartsWith('cid:', $cid);
$contentId = Str::after($cid, 'cid:');
$this->assertSame($contentId, $attachment->getContentId());
$this->assertSame('bar', $attachment->getBody());
$this->assertSame('Content-Type: image/png; name=foo.jpg', $headers[0]);
$this->assertSame('Content-Transfer-Encoding: base64', $headers[1]);
Expand All @@ -201,9 +204,11 @@ public function toMailAttachment()
}
});

$this->assertSame('cid:baz', $cid);
$this->assertStringStartsWith('cid:', $cid);
$contentId = Str::after($cid, 'cid:');
$attachment = $this->message->getSymfonyMessage()->getAttachments()[0];
$headers = $attachment->getPreparedHeaders()->toArray();
$this->assertSame($contentId, $attachment->getContentId());
$this->assertSame('bar', $attachment->getBody());
$this->assertSame('Content-Type: image/png; name=baz', $headers[0]);
$this->assertSame('Content-Transfer-Encoding: base64', $headers[1]);
Expand All @@ -225,14 +230,14 @@ public function toMailAttachment()
});

$this->assertStringStartsWith('cid:', $cid);
$name = Str::after($cid, 'cid:');
$this->assertSame(16, mb_strlen($name));
$contentId = Str::after($cid, 'cid:');
$attachment = $this->message->getSymfonyMessage()->getAttachments()[0];
$this->assertSame($contentId, $attachment->getContentId());
$headers = $attachment->getPreparedHeaders()->toArray();
$this->assertSame('bar', $attachment->getBody());
$this->assertSame("Content-Type: image/jpeg; name={$name}", $headers[0]);
$this->assertStringContainsString('Content-Type: image/jpeg', $headers[0]);
$this->assertSame('Content-Transfer-Encoding: base64', $headers[1]);
$this->assertSame("Content-Disposition: inline; name={$name};\r\n filename={$name}", $headers[2]);
$this->assertStringContainsString('Content-Disposition: inline', $headers[2]);

unlink($path);
}
Expand Down