Email adjuster to attach/embed by scanning the email body

Hey i did rebase this MR and did try to fix the comments made by @AdamPS.

I don't implement the CID counter and instead relied on the getContentId() from DataPart

Great thanks. The tests are failing with a syntax error so this needs work.

Fixed linter issues.

Thanks @metallized, that's some more good ideas.

It's a good reminder to look at image:, which as far as I can see is specific to swiftmailer. It could be good to support it for back-compatibility however I would see it as deprecated. I propose we should get the main function of this issue working here, and raise a separate issue for image:.

This issue 🌱 [Meta] PHP DOM (libxml2) misinterprets HTML5 Active covers HTML5 support in core, and it proposes to use html5-php. The interface seems close to \DOMDocument, so sticking with \DOMDocument is likely the best direction for now.

Details comments are in the MR.

@AdamPS, i'll wait your answer to commit the changes.

The UTF-8 problem seems unpleasant, thanks for debugging it. There seems to a choice of exactly how to solve it, see discussion. Most of the options seem a bit hacky, with possible problems.

1) According to 🐛 Deprecated function: mb_convert_encoding(): Handling HTML entities via mbstring is deprecated Fixed , the mb_convert_encoding() option could be deprecated in PHP 8.2.

2) In tijsverkoyen/css-to-inline-styles (which is already a dependency of this module), in createDomDocumentFromHtml() it uses

mb_encode_numericentity($html, [0x80, 0x10FFFF, 0, 0x1FFFFF], 'UTF-8')

This seems even more hacky than most options however I guess there is a precedent for it.

3) We could potentially avoid all the problems and create a future-proof solution using html5-php, as mentioned in #13.

For the other question about this code:

public function embedFromPath(string $path, string $name = NULL, string $mimeType = NULL) {
    $contentId = bin2hex(random_bytes(16));
    $name = empty($name) ? $contentId . '@symfony_mailer' : $contentId . '@' . $name;
    $this->inner->embedFromPath($path, $name, $mimeType);

    return $name;
  }

Basically like that, however if the caller passed $name we should use it. So I would say more like this, I think better than random_bytes() is to use a hash of $path.

if (!$name) {
  $name = sha1($path);
}

I just spotted one more thing. The original patch had $processed_images to avoid embedding the same image twice. We still need that I think - it can be map from filename to cid. If the image was already processed, then we skip the call to embedFromPath() but still call setAttribute().

I think all gets fixed, about #17, i think is not necesary cause in first place we need to get the cid from uri so we can create a $processedImage, i think we need a helper method or function to create the cid string so we skip the call to Symfony\Component\Mime\Email::embedFromPath.

Great thanks I'll take a look

So HTML5 solved all the problems??

Interesting, core has HTML::load() and Html::serialize. I guess that would be the alternative if we find problems with the HTML5 library.

Yeah solved all problems.

Let me replace the use of direct HTML5 for href=" https://www.drupal.org/project/bootstrap/issues/3254714 → ">Core HTML.

All done, also working encoding.

@AdamPS do you need something more, can we update this?

Sorry I haven't forgotten, just been a bit busy. I'll get to it soon.

Generally it's looking good thanks. This is a key issue and it's important that we do all we can to get it right😃. I read through the past comments and here's what I spotted:

1) #17/#18 seems to be still unresolved.

embedFromPath() can keep a map with key = path and value = name. If the key already exists then skip the call to $this->inner->embedFromPath.

2) Originally I suggested a CID counter. Unfortunately later I forgot about this idea, and suggested sha1. The counter seems better as it's faster and generates shorter names. Any reason why we can't use the original counter idea?

3) We should have tests for new features please (true, existing test coverage is poor, at least we can stop adding new code without tests😃).

• Create a new functional test class AdjusterTest which should have a test for each adjuster. Of course you don't have to add the existing ones - just create one for the new adjuster.
• Create a new function SymfonyMailerKernelTest::emailInterfaceTest() which should call each function on EmailInterface/BaseEmailInterface. Again just need to call the new function and try to find a way to test that it worked.

4) Security implications. This adjuster could potentially be used to bypass access checking. The file might be in the private file system, or it could even be settings.php. Perhaps security should be a separate issue - in which case this one might need to be postponed on the other.

First of all, i want to apologize for sending so many commits (i couldn't get phpunit to work on my local, but finallly i did it), i did answer to your comments on gitlab.

1) #17/#18 seems to be still unresolved.

Don't get me wrong but why are you concerned about this, symfony do this on Email::prepareParts() on line 502, anyway i did a extra validation on BaseEmailTrait::embedFromPath() what do you think about this?

2) Originally I suggested a CID counter. Unfortunately later I forgot about this idea, and suggested sha1. The counter seems better as it's faster and generates shorter names. Any reason why we can't use the original counter idea?

See my previous answer.

3) We should have tests for new features please (true, existing test coverage is poor, at least we can stop adding new code without tests😃).

Create a new functional test class AdjusterTest which should have a test for each adjuster. Of course you don't have to add the existing ones - just create one for the new adjuster.
Create a new function SymfonyMailerKernelTest::emailInterfaceTest() which should call each function on EmailInterface/BaseEmailInterface. Again just need to call the new function and try to find a way to test that it worked.

What do you think about what i did?

4) Security implications. This adjuster could potentially be used to bypass access checking. The file might be in the private file system, or it could even be settings.php. Perhaps security should be a separate issue - in which case this one might need to be postponed on the other.

I add a validation to make sure is a image file, what do you think about this?

Great many thanks

1)

Don't get me wrong but why are you concerned about this, symfony do this on Email::prepareParts() on line 502

I am concerned about the case that the email contains the the same URI in two places. If we call embedFromPath() twice then it will create 2 identical entries in $this->attachments(), see line 391 in Email.php.

3) Tests look good thanks.

4) Eventually the security should be similar to the existing Drupal access system - there would be hooks that return an access result, and an adjuster to configure policy in the GUI. In the beginning we can start with a simple rule that is safe. I made a suggestion in the MR based on testing the URL protocol.

I made all your suggestions, please review.

Great thanks. I made one comment on the new function.

Please see my answer on gitlab, i think we should stay close to the swiftmailer implementation, we can even remove the absolutepath transformation and scheme/protocol validations.

Great I like the mime type validation, and being similar to swiftmailer.

In BaseEmailTrait::embedFromPath(), attachNoPath(), attachFromPath() there is a $mimeType parameter. This is passed through to the Symfony Mailer library methods which will guess a mime type if one hasn't been set. I propose that we should guess consistently and only do it once (rather than doing some guessing in the module, some in the library, and sometimes guessing the same file twice).

It seems better to guess in the module, using the same Drupal code used throughout the website, so your new code is good. We can guess the mime type in all three BaseEmailTrait methods (except if a value is passed already), and pass the answer to the library. We can use the same mime type for allowEmbed().

we can even remove the absolutepath transformation

I agree - we already have AbsoluteUrlEmailAdjuster. If necessary we can make the new adjuster run after AbsoluteUrlEmailAdjuster.

and remove the scheme/protocol validations.

I don't think so. If an email contains a link to a private image, this is safe - the recipient can only see the image if they log in as a user with the correct permissions. If an email contains an embedded private image, then the recipient can immediately see the image without any access checking. The InlineImagesEmailAdjuster tries to embed all images, so this creates a dangerous combination.

We should have tests to verify the access checking. Embedding should fail for a private:\\ image, settings.php, or an image denied by htaccess. Embedding should succeed for a public:\\ image, or core/themes/stark/screenshot.png.

Great I like the mime type validation, and being similar to swiftmailer.

In BaseEmailTrait::embedFromPath(), attachNoPath(), attachFromPath() there is a $mimeType parameter. This is passed through to the Symfony Mailer library methods which will guess a mime type if one hasn't been set. I propose that we should guess consistently and only do it once (rather than doing some guessing in the module, some in the library, and sometimes guessing the same file twice).

It seems better to guess in the module, using the same Drupal code used throughout the website, so your new code is good. We can guess the mime type in all three BaseEmailTrait methods (except if a value is passed already), and pass the answer to the library. We can use the same mime type for allowEmbed().

What you think about what i did?

I don't think so. If an email contains a link to a private image, this is safe - the recipient can only see the image if they log in as a user with the correct permissions. If an email contains an embedded private image, then the recipient can immediately see the image without any access checking. The InlineImagesEmailAdjuster tries to embed all images, so this creates a dangerous combination.

We should have tests to verify the access checking. Embedding should fail for a private:\\ image, settings.php, or an image denied by htaccess. Embedding should succeed for a public:\\ image, or core/themes/stark/screenshot.png.

And what if a site admin/module wants/needs to embed that files?, i think we should not restrict this, at the end is the admin user/contrib or custom modules who decides what to embed.

And what if a site admin/module wants/needs to embed those files?, i think we should not restrict this, in the end is the admin/modules who decides what to embed.

But it is not only admins that can choose the content of an email. E.g. with simplenews, anyone with permission to edit a newsletter node can insert links to private images - even ones that they don't have permission to see. Maybe with contact module even anon users can insert links in emails.

This module must not break access control of images, that would be a security issue and I cannot commit it. If we restrict too much, then true it is limiting, but I can commit it. We can add ways to manage the access in detail in another issue. Still the default must be secure.

Many thanks, the mime type code is good, I made some detailed comments.

I'm sorry but the security issue cannot be ignored. We have a stable release and the security team would insist on a fix or shut the module down. If the recipient of the email would not be allowed to see the image using a link then also they must not be allowed to see it using embed. We should have tests to verify this. Otherwise, an anonymous user who can guess the URL of a private image could email it to themself (e.g. using webform).

Certainly I don't insist on a solution using scheme/protocol validations - it could also be a different solution, which could even be better. Ideally we would email a private image to the email address of a user who has permission to see it. And we would email an image within /modules or /themes except if it was blocked by .htaccess. That would mean that we fetch the image in a way that runs through the normal Drupal access checking, using the permissions of the current user.

Hi, i make all the fixes (i think ;)), about the security issues i will wait to suggestions about how to do it, i try with the Drupal path validator service, but doesn't works as need it.

We need to make some research about how other modules do it. Note that in`SwiftMailer`, they does not do any security validation about image paths.

If you want to mark this as posponed its OK.

Embedding images works well with the new email adjuster, except that the adjuster plugin runs before the email is wrapped. Any images from the wrapped HTML (from email-wrap.html.twig) will then not be embedded. Setting the plugin weight to 850 so that it runs after the mailer_wrap_and_convert plugin solves that.

Setting the plugin weight to 850 so that it runs after the mailer_wrap_and_convert plugin solves that.

That seems to make sense. It also makes this plug-in after AbsoluteUrlEmailAdjuster, which I think should be fine.

Even if this is only scoped to an API addition for mail plugins to call embedFromPath, this is a great improvement. I vote to get this merged. It works. If the UI components are still being debated, can we move those into a follow-up so we can get the basics of this feature added to the code base?

@heddn
This issue is for the UI components as indicated in the title. It's not ready to commit due to legitimate security concerns that are clearly explained in earlier comments.

However I agree it's a good idea to split the API change into a separate issue. It can take parts of this patch and likely could be committed.

I've rebased the MR here and moved the API additions over into 📌 embedFromPath API addition (for #3284140) Needs work .

I referred to the code in the PR and created a patch to scan the pictures in the email and replace them with cid. It works normally for me. According to the code in the PR above, there may be a more elegant way to solve it. I don't have time to continue researching this issue at the moment.
my symfony mailer version is 1.2.2. This patch is only used to solve the problem that the embed image is broken in the email body.

Thanks for the patch. The patch is working except if I add an image to the body with a token then the image is not being processed because the token replace is being called later in the process. Any ideas in how to solve this?

The weight of InlineImagesEmailAdjuster should be greater than the WrapAndConvertEmailAdjuster one. This way the inline images adjuster can be used in wrap templates too.

Maybe set it to 1000.

Thanks! I was using the latest patch in this issue, when I switch to the MR it works

hey @AdamPS, do you have any ideas about resolving the security issue when embedding files?, what about a whitelist/blacklist files, also i think that developers can skip this restrictions maybe an `embedFromPath` and `uncheckedEmbedFromPath`?

Anything else i can help you?

@metallized Thanks please see 📌 embedFromPath API addition (for #3284140) Needs work

The API changes are now checked in so work can continue here.

@adamps, Is there anything left here?

@metallized Yes we still need the adjuster and the test for it. The needs updating a little as the API is different. This issue no longer needs the calls to setAttribute() or setHtmlBody() because that's handled automatically in the Email class.