v1.x version ready for testing please
Fix in 2.x first then backport
I understand this change was introduced for security reasons, but could you clarify specifically what vulnerability or risk this hasAccess() check is addressing?
This change/problem was introduced in 📌 embedFromPath API addition (for #3284140) Needs work . The vulnerability is where non-trusted users can attach files - they could attach a file they shouldn't be able to access and email it to themselves (this isn't just theoretical as there has already been a security issue like this in another module). Particularly this applies to ✨ Email adjuster to attach/embed by scanning the email body Needs work , which currently is in 2.x but not yet in 1.x.
The solution is intended to work like this. Public files are allowed; http/https file are allowed if the URL can be opened with fopen; everything else is not allowed by default. Code that knows the attachment is from a trusted source can use the API to allow access to any file.
For legacy emails, AFAIK there has never been any access checking. The code that sets the attachment is responsible for ensuring that it is safe. Therefore we could reasonably do the same, as in #7. This was how it worked in 1.5, and I AFAIK it is safe.
On my current site, even public files are no longer being sent as attachments
and whether the implementation is fully correct, given that public files are also affected.
This is not correct, it would be a bug. Can anyone else confirm. There is an automatic test that should be covering this case, however the test could be wrong.
We could remove the access check for legacy emails (sent using Drupal core mailer)?? This makes some sense for compatibility.
adamps → created an issue. See original summary → .
Thanks for the report. The access checking is a new feature designed to improve security. The change record → was linked from the release note. By default private files are not allowed.
Please can you take a look at the CR and see if you could write code to call Attachment::setAccess()?
If not, then do you have any suggestions how we could make it work for your site and still keep some security?
adamps → changed the visibility of the branch 3347378-incorrect-information-on to hidden.
It's strange that the tests still worked.
In 2.x I have made various improvements with auto-wiring and the constructors are almost all @internal. I raised a Drupal Core issue to describe it ✨ Improvements to AutowireTrait Active .
2.x is a partial re-write with various non-BC changes. It should have a beta release soon, after which there should be no more that are non-BC (except perhaps if there is no other possible way to fix a critical bug). Please do join in and test it if you can, so your feedback/bugs can be addressed before the beta.
Thanks. The problem is that mailer_tranport is using that class from symfony_mailer without declaring a dependency and also symfony_mailer depends on mailer_tranport.
This affects fresh installs only - I have various sites running with the current release. So 2 potential workarounds:
- upgrade from 1.x
- hack some files to allow install - might be enough just to comment out in two places in mailer_transport module
use AutowireTrait;- then put back after installing
- For commerce orders, the user is context if logged in, else use the order as before.
- I moved the explanation field to the declaration entity rather than evidence because evidence is also used on donors, where explanation is not appropriate.
- I now set zero weight every call to
->setDisplayOptions(), copying Commerce - it means the fields can be rearranged in code to determine their order without also having to edit the weights.
From Drupal Symfony Mailer perspective, there have been no problems reported with the solution reached in the discussion here. Thank you to all those who helped.
Thanks that was quick😃
Everything regarding cancellation is now part of 🌱 The problem of changes Active . This issue is now for finishing up anything about Donor entities.
Continuing the previous approach, I would like to commit this now please, deferring your review until we have something useable and self-consistent.
Then I will move on to 📌 Figure out contexts and donors Active followed by 📌 Finish the declaration forms Active and ✨ Fix GiftAidOverviewController Active , perhaps something else smaller. That would be the point for review. For sure it won't be feature complete, but at least all the features that are present should be useable, and any known issues be documented.
We can use this issue specifically for cancellation, which is the main topic. A lot of the discussion on 📌 Figure out contexts and donors Active is relevant to here.
Then the other part: change of address and adding of misc correspondence is part of 📌 Figure out contexts and donors Active .
The Drupal Symfony Mailer project implemented this functionality in a more flexible way. However, since it is reusing patterns unique to that module, the mechanism isn't easily portable to current core architecture.
I suggested how the patterns from DSM could be introduced in Core in
✨
Create new mailer service based on symfony
Active
. In addition to being more flexible, it feels simpler, just $email->setTheme() without adding 3 new services and a service tag. So far my suggestion doesn't seem popular, and I've got no problem with that, of course you will make your own way.
You can ignore my other points from #25. Contrib DSM can just stick with the approach that it already uses and that works.
I created a 1.6.1 release containing this fix.
Great thanks
I altered the default evidence types slightly:
- Separate PDF from image because they need different widget/formatter settings
- Combine the in-person and remote versions of each type to avoid doubling the number of types (already there are 15 files). The admin can easily clone them, or add a field "in person" if they wish.
A complication is that there are various different types of header with different data/parameters. See Symfony\Component\Mime\Header\Headers.
OK thanks
Great. Here are my comments.
1) If we apply this to the existing mailer then it seems not to meet the back-compatibility requirements of a minor release.
- Change in behaviour: introducing this code could change the theme used when rendering emails. This in turn affects which templates and CSS files are used.
- Change in UI: before it was the mailsystem one and now it is the Core one.
- Change in config model: before the mailsystem setting now the Core one (requires a change to installation profiles etc).
Secondly it potentially creates forward-compatibility problems because then these changes aren't experimental, and we might need to make changes as we develop the rest of the mailer.
If we add it only to the new mailer, and we have a global setting whether to use old mailer or new, then this solves the problem as sites have a choice when to adopt it. Sites using the old mailer anyway have an existing working solution using mailsystem and might even prefer that we leave them in peace until one big change when they switch mailsystem.
2)
The Drupal Symfony Mailer project implemented this functionality in a more flexible way. However, since it is reusing patterns unique to that module, the mechanism isn't easily portable to current core architecture.
Yes I understand your point. I feel that the key here is to ensure that we can re-implement the DSM flexibility on top of the mechanism proposed here.
The main question I have is regarding MailThemeNegotiator::applies(), which requires the choice of theme to depend only on MailTemplateID. This doesn't "unambiguously identifies the email to be sent" as the IS states but identifies a category (the email also varies with the params etc). I can imagine scenarios where we would require more flexibility: e.g. the site has multiple brands with different main themes. We might check the user who will receive the email, see which brand they registered with, and choose the matching theme.
I feel we want instead is the email object - and I am aware this doesn't exist yet, however we could create a placeholder class for the purpose of this issue that we fill out later.
3) This patch is making key design choices about the new mailer in advance of an overall design (AFAIK). I feel it could be really useful to try to establish a design in advance, but otherwise I guess we can discuss design questions here.
a) The concept of a MailTemplateId. In contrib DSM v2 we have a string value named "tag" - which follows after the concepts in the underlying symfony code. This choice was actually influenced by a comment from @znerol on another issue😃 . It's a "dotted-string" format, with the first part being the module, hence allowing a hierarchy such as contact.page.copy and simplenews.subscriber.validate. The two alternatives are basically compatible so it's not a big deal, but I feel it could really help the community if we could make Core and Contrib consistent as much as possible. If you agree with my comment 2 then anyway this patch doesn't need to contain MailTemplateId so we can defer the discussion.
b) I agree with the basic approach of executing the mail building/sending code in a new context with the correct theme. When we take the whole design into account, I found 4 contexts that we need to switch: render, theme, language and user. This led DSM to choose a general approach, with a Mailer class which does all 4 switches. We could make MailThemeManager::switchTheme() public, and then it works more like AccountSwitcher (perhaps we call it MailThemeSwitcher?). I agree it could work with 4 separate switches stacked inside one another, each with try/finally and creating a new closure, but it seems less clear.
The language switcher is also missing, see 📌 Allow to change the current language Needs work , which should handle translations, see #3029003: TranslationManager has method setDefaultLangcode not defined on any interface. → .
Good news, 📌 [PP-1] Add symfony mailer transports to DIC Postponed has been committed, principally thanks to @znerol for many patches and comments. Some parts were left to follow on issues ✨ Add support for 3rd-party symfony mailer transports Active , 📌 Make symfony mail delivery layer more useful to contrib Active which I have added to the IS.
In the meantime, Contrib module Drupal Symfony Mailer → has a second alpha release of the 2.x branch. This branch includes various changes to adapt to various feedback in the various Core issues discussions, becoming closer to / more compatible with the likely Core Mailer.
The next step listed in the IS is ✨ Create new mailer service based on symfony Active , which I raised, and basically it says to adopt some of the key parts of DSM into Core. At least to copy at an architectural level - @znerol and others would no doubt introduce dozens of little improvements to fit better into Core. Also it's probably too big for one issue anyway, so we'd also defer various parts to other issues. However setting aside these important details for a moment, there is a key question:
Is ✨ Create new mailer service based on symfony Active the direction that Core wants to move forward? The design has some real-world proving in Contrib at least, and I've done my best to describe the reasons and advantages for the current design in various issues. Still Core might prefer something different for good reasons.
As I already said in #2, mailer_sendmail_commands is a security mechanism to avoid the situation where the UI admin can run any arbitrary shell command and the same mechanism exists in Drupal Core. You have observed that it could be re-used to handle sensitive data if changed in a particular way - and I agree. However it would only work for sendmail transports, and the information is still in plain text (just in a different place), so it's a limited solution.
There is already an issue for encrypting sensitive data, #3309471: Encrypt stored remote Transport credentials → which proposes using standard mechanisms and contrib modules designed for this purpose.
When I said "OK it's an interesting idea" - I meant it was interesting to avoid duplicating the very same command in the yaml file and also settings.php. However on the plus side the duplicating provides some extra security ensuring that the command matches. In the end the situation is that the decision has been made - the code is in contrib and core, it's documented in various places and likely 100s of sites are using it. It would be a lot of work to change it, and the benefit seems small. I propose that we postpone this for now and we can re-open if there is a lot of interest, or if Core decides to change.
@o'briat
I would say that DSM is neutral regarding config. It uses the standard config API, which allows for overrides, swappable config store back-ends etc, so nobody is forced to behave in a particular way. Sites could choose to use settings.php overrides either to avoid putting sensitive data into the database or to get different setting in dev without editing config. Site could use one of contrib modules that allows moving sensitive config to storage. DSM hasn't yet done anything to directly integrate with such a module. This isn't because of any design problem, rather just that nobody found time to do it yet. The issue is #3309471: Encrypt stored remote Transport credentials → if anybody wants to get involved.
The issue you linked to,
✨
Settings custom send mail command on multiple env
Active
, is about $settings['mailer_sendmail_commands'], which is entirely different, as I already described in my comment #2 in that issue. It is a security mechanism to avoid the situation where the UI admin can run any arbitrary shell command and the same mechanism exists in Drupal Core. You have observed that it could be re-used to handle sensitive data if changed in a particular way - and I agree. However it would only work for sendmail transports, and the information is still in plain text (just in a different place), so it's a limited solution. I feel that instead let's stick with the standard options I describe in the first paragraph.
I'm going to make a release soon, so I'll commit this now. Thanks everyone for the contributions. Please do test and give any feedback if anyone gets a chance.
We're missing any kind of functional test for mailer policy, so there's not yet a framework to a new test into. Unfortunately I don't have time to fix that right now, so let's just get this in and I'll continue work towards a 2.x stable release.
I'm going to make a release soon, so I'll commit this now. Please do test and let me know when you have a chance.
Thanks everyone
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 ✨ Allow inline of images in the wrapped template. Active .
Adding credit
adamps → changed the visibility of the branch 3284140-email-adjuster-to to hidden.
General "help me learn about Drupal Symfony Mailer" questions are best asked elsewhere please😃. This issue queue is more for things that need the attention of the module developer specifically.
This seems more like a general Drupal question so better asked elsewhere please😃. This issue queue is more for things that need the attention of the module developer specifically.
This is more like a "help my fix my site" question so better asked elsewhere please😃. This issue queue is more for things that need the attention of the module developer specifically.
I've just tested on v2.x (based on the command in #9) and it works.
adamps → changed the visibility of the branch 3518428-translating-fallback to hidden.
Thanks. It's a single line change so I made a direct commit.
Great thanks
This should fix it.
In the end we did ✨ Allow to customize Sender Active instead however thanks for the patches
Actually the problem was worse than I expected. All of the headers were coming out in lower-case because Symfony converts to lower-case.
Great thanks
MR ready for 2.x please review and test
adamps → changed the visibility of the branch 3270408-email-body-policy to hidden.
Rejoining with ✨ Email body policy form should show available TWIG variables Needs work
Many thanks @abyss your MR was a big help.
I am working on this and also the TWIG variables, so let's combine the issues again.
Many thanks to @abyss for the valuable work on the MR in the other issues.
Good job I tested it as it didn't work. In future please test fixes if you want to get credit😃.
v1 MR is ready for review and test please
I have fixed ✨ Set langcode with LegacyEmailBuilder Needs work and removed __disable_customize__ so your problems should be solved in 2.x.
I've now added a test. I'll commit as I'm getting too many issues in progress. Anyone else please do test to confirm.
Fixed in v2 next backport to v1
I fixed it in 🐛 Updating from 1.5 to 1.6 broke rerouting for webforms Needs work which was changing the same function anyway
Here's a patch for 2.x that worked for me.
It's not really systematic😃. We already waited for module handler to load. Now we also wait for EntityTypeManager. Maybe in the future we would have to add another one. However it works for now on my site at least.
I added a warning to the release note.