Email adjuster to attach/embed by scanning the email body

Great many thanks for the patch. Here are some comments

1. preg_match_all() is not the best way to parse HTML - please use $dom->getElementsByTagName('img') as in the patch you started from. Then use $img->setAttribute() instead of str_replace. Get rid of $processed_images variable.
2. (=IS item 2) The cid doesn't need to be universally unique, just unique within this email. Probably we should get the Email object to handle assignment of a unique cid or else every caller would need to duplicate the same code. EDIT So the interface is embedFromPath($path) returning a CID. The function supports passing the same $path and returns the same CID. The CID counts up numerically it doesn't need to be a UUID.
3. (=IS item 3) Call to realpath() is discouraged and only works for local filesystem. Probably it's unnecessary as embedFromPath() can handle it??

Thanks for the follow up.

1. Core uses \DOMDocument in 4 places including Html::transformRootRelativeUrlsToAbsolute This module uses CssToInlineStyles which uses \DOMDocument. So for the moment it seems to be most consistent with what we already do. preg_match_all() could be fooled by comments, whitespace, etc, so that's no good.

2. Yes that was my idea to change the interface - we have done this in other places hence BaseEmailTrait with a protected inner \Symfony\Component\Mime\Email object. You are right we would lose chaining however I can't see a better way. We can't safely put a counter in InlineImagesEmailAdjuster because the CID might clash with another class that also inserts inline images. However the Email class can use a simple counter because it can see all the CIDs and images, and it can even return the same CID if the same image is used to avoid transporting the embedded image twice.

3. I think we can drop both if statements and require a URL scheme. Mailing of local files without using stream wrappers seems like something we don't want to encourage for security as it makes it hard to prevent emailing of a sensitive/private image. When we later write security checking (I thought I raised an issue but I can't see it so maybe I didn't) it could be based on checking the URL scheme. Of course people can still use file: in their email, however we can write a policy to block it.

Interesting, /vendor/symfony/mime/Email.php uses preg_match_all(). I'm still not sure if it's robust in all unusual cases, and it feels safer to copy what Drupal does.

In existing attachFromPath(), I think the $name parameter seems to default to none. What we can say is that if there is a name, it must be unique among all attachments. Please can you fix to something like this?
* (optional) The file name, which if set must be unique among all attachments.

One way to avoid changing the interface of embedFromPath() is to add a new method getUniqueName(string $path). Downside is that it makes the calling code more complex adding a line of code.

Probably I still prefer to change the interface:
* (optional) The file name, which must be unique among all attachments. Leave blank to generate a unique name.
and
* @return string The file name.

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

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 #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5 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.

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 #3342481: Deprecated function: mb_convert_encoding(): Handling HTML entities via mbstring is deprecated, 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().

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.

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.

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.

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

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.

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.

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.

@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.

@metallized Thanks please see #3382624: embed attachments API addition

The API changes are now checked in so work can continue 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.

Thanks. I believe the fix shouldn't need to change Email.php or BaseEmailTrait.php now.

Thanks. The MR still seems mixed up - please see #53. This MR should only contain the new adjuster, schema changes and tests.

New fixed need to go into 2.x please. We can backport to 1.x after.

Adding credit

From #45

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

That's a good point. However in v2.x the weight has to be < 600 to run before AttachmentAccessEmailProcessor. I've raised a follow up #3527701: Allow inline of images in the wrapped template..

Thanks everyone