- Issue created by @heddn
- Open on Drupal.org βCore: 9.5.x + Environment: PHP 7.4 & MySQL 5.7last update
over 1 year ago Not currently mergeable. - @heddn opened merge request.
- last update
over 1 year ago 6 pass - Status changed to RTBC
over 1 year ago 3:55pm 22 August 2023 - heddn Nicaragua
Since the API changes here were already RTBCed over in β¨ Email adjuster to attach/embed by scanning the email body Needs work , moving directly to RTBC.
- Status changed to Needs work
over 1 year ago 12:10pm 25 August 2023 - π¬π§United Kingdom adamps
Great thanks @heddn. Definitely good to split this into a separate issue as it's much easier to review one step at a time.
Even with the reduced scope, this area is pretty tricky when you look into it closely. Here are my comments and points for discussion - probably best if we try to agree what to do before spending time changing code.
1) Let leave
allowEmbed()
out of this issue. Until we have completed the "attachment security" task then we are just guessing about the interface. Probably it would be calledallowAttach()
as it's equally possible to have security bugs with attaching files.What we can and should do is add a comment to all the attach/embed functions that the caller is responsible for access checking of attachments to avoid sending a private file or even worse settings.php to someone who shouldn't see it. Code call this function using a $filename taken from unsanitized user input.
2) What about
embedNoPath()
? This would correspond to Symfonyembed()
. Implementation will depend on the points below....3) embed/attach are almost the same except for the $inline flag, and even symfony will swap the inline flag in
prepareParts()
if you got it wrong. We're keeping an array$this->filenames[$path]
, probably we should do that too in attachFromPath(). And as in point 1), the security will likely be shared. So maybe we should try to combine the code? even combine the interface, with an $embed flag??4) There is confusion on names. The $name parameter is not in general a file name. Inspecting into symfony code, the gets written to the 'Content-Type' parameter 'name', so we could call it an attachment name or a part name.
Symfony defaults to the base file name for attachFromPath() - which will break if it clashes with an embed. Our patch currently defaults to a hash of the file name for embedFromPath(), but that wouldn't work for embedNoPath(). I had suggested several times that we just use an incrementing integer but this wasn't taken up.
The name is used in a complex way in
Email::prepareParts()
. It's strange that we end up writing code to pick a unique name only for symfony to run a lot of code swapping it for the content id which is a different, random unique name. I don't know what we can do as Symfony makes a lot of things private, and difficult/expensive to query the attachments / names / content ids.5) Let's combine this with β¨ Simpler way to get attachments Needs work , and then write a test that covers all 5 attachment methods.
- heddn Nicaragua
What I'd like to aim for in an MVP is something that allows:
public function build(EmailInterface $email): void { $email->embedFromPath('themes/custom/my_theme/logo.png', 'logo', 'image/png'); }
then in my email.html.twig:
<img src="cid:logo" alt="'Logo'|t" title="'Logo'|t">
Right now, there isn't enough of the symfony underlying API available to do this without hacking the Email object through reflection. If we need to add dire warnings that adding random files to emails is a security concern, let's do that. Anything so we can get the API addition into the mix and improve things over time.
- π¬π§United Kingdom adamps
I agree we can cover security with a comment - that's my point 1).
Points 2) and 5) are simple - let's complete the attachments API by adding all the remaining gaps at once rather than an issue for each. This would also allow writing better tests.
Points 3) and 4) are questions about what is the right API. Sorry it was a bit unfocused and unclear, as I'm still thinking things through.
- π¬π§United Kingdom adamps
I think it's really important to understand how this API will be used. #7 matches the Symfony API well - but is it really what we want in Drupal? The theme-designer needs to tell the coder every file to embed - changing the code every time they want to add a file. It could work in custom code but it's awkward. Clearly it won't work in a contrib module where each site will want different files.
Instead we might prefer any of these:
<img src="themes/custom/my_theme/logo.png" embed="true" alt="'Logo'|t" title="'Logo'|t">
- symfony syntax:
<img src="{{ email.image('@public/logo.png') }}" alt="Logo">
- Field formatter for image field with option to embed
- (moving on to attach rather than embed) Email adjuster to attach a file
So I propose the Symfony API is not good for Drupal and instead we should have something like this:
- The application sets
$name
which is a human-readable value that the mail client often displays with the attachment. It defaults to the file basename for theFromPath
functions. - The framework assigns the CID, which is a machine-oriented ID that must be unique. I propose this has no relationship to $name because they have quite different needs and the application in general can't easily pick a unique value given that also hooks, plug-ins, templates etc. might embed/attach. Instead the embed functions return a CID.
- All attach and embed functions may fail due to access checking, in which case they return FALSE. Therefore they can't return $this. However this issue isn't about access checking, so we can defer the non-BC change to the existing attach methods to a separate issue.
getAttachments()
should return attachments not data parts - it should be easy to get back all of the data from the original attach call which is not available on a data part. Also this should be performance efficient. Probably we keep our own record of the attachments instead of just$this->filenames
My other points are coding details:
- We should use a function for common code in the 4 attach functions - this can include tracking the attachments, calculating CID & mime type.
- We can delete
allowEmbed()
and put the single line of code in embedFromPath(). In a separate issue we can writeallowAttach()
with general purpose access checking.
- π¬π§United Kingdom adamps
Anything so we can get the API addition into the mix and improve things over time.
Yes but we can't change the interface later, so we need to get that right now. This is an important decision that will hopefully even get adopted into Core for use on 100000s sites. So let's not rush, and instead take a little time to get it rightπ.
- π΅π±Poland volman
+1
Lack of image embedding is blocking us from migrating to Symfony Mailer.
What are the prospects of moving forward with this? - Merge request !101Add embedUri() and use it for test emails. Missing tests. β (Merged) created by adamps
- Status changed to Needs review
4 months ago 12:40pm 30 August 2024 - π¬π§United Kingdom adamps
I feel that the the decision whether to embed is likely a site policy decision.
When the body is generated by code, or written by a content editor, then it should contain img src exactly as normal. There could be an attribute that requests an embed, but the policy might ignore it - for security, the file is too large, etc.
Then we can have a policy (Adjuster) with configurable options that decides which images to embed.
This allows a simpler UI where the caller of embedFromPath() doesn't have to care about the CID. Inistead Email::getSymfonyEmail() does search and replace for all the cid
This is missing tests, however otherwise fairly complete. Please let me know what you think.
- π¬π§United Kingdom adamps
This would also fix β¨ Simpler way to get attachments Needs work
- π¬π§United Kingdom adamps
adamps β changed the visibility of the branch 3382624-embedfrompath-api-addition to hidden.
-
adamps β
committed 66be8a3d on 1.x
Issue #3382624 by metallized: embedFromPath API addition (for #3284140)
-
adamps β
committed 66be8a3d on 1.x
Automatically closed - issue fixed for 2 weeks with no activity.