AdamPS → credited metallized → .
Released a new version with this fix, thanks to all.
Done.
I think this is a temporary fix, please tell me what else we can do.
Please review
metallized → made their first commit to this issue’s fork.
As far as I can see, the only part of the loop that we can break is this:
symfony_mailer.config_override -> plugin.manager.email_builderMailerConfigOverride could fetch the email builder manager to a local variable inside buildCache().
I'm not able to test this as I don't have any multilingual sites, so I hope someone else can help.
I tried to remove the plugin.manager.email_builder argument from symfony_mailer.config_override service, but the problem is that creating manually a instance of EmailBuilderManager needs a entityTypeManagerInterface argument, so now the circular reference is here, i think we must remove the EntityTypeManagerInterface from EmailBuilderManager.phpon line 72.
What i unsderstand is that the locale requiresentity_type.manager to load translated configs and this modules try to override those configs, requiring the same dependency entity_type.manager so the circular reference happens.
Don't know if is this correct, please review and let know how can i help you to make a fix, i am using Drupal 10 and locale modules.
I can confirm this is not related only to Drush 12, the reason is about the locale core module enable and Drush 12 on Drupal 10 sites
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.
I think this is realted to Drush 12 support 🐛 Drush 12 Support Closed: duplicate
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.
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.
Hi in Drush 11 there is not errors, once I enable symfony_mailer through the UI, any command in Drush (12) fail.
I made all your suggestions, please review.
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?
metallized → created an issue.
@AdamPS do you need something more, can we update this?
All done, also working encoding.
Yeah solved all problems.
Let me replace the use of direct HTML5 for href=" https://www.drupal.org/project/bootstrap/issues/3254714 → ">Core HTML.
Can anyone review this so we can change to RTBC, thanks.
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.
@AdamPS, i'll wait your answer to commit the changes.
Fixed linter issues.
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
metallized → made their first commit to this issue’s fork.
This was fixed on dev, can anyone please review, thanks.
Hey, if you get lost just follow this guide → .
I still getting this
FILE: /home/drupal/serial/src/SerialSQLStorage.php
----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------
150 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
FILE: /home/drupal/serial/src/Plugin/Field/FieldFormatter/SerialDefaultFormatter.php
------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------
33 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
------------------------------------------------------------------------------------------------------
Time: 275ms; Memory: 10MB
the output of phpcs -i:
The installed coding standards are MySource, PEAR, PSR1, PSR2, PSR12, Squiz, Zend, Drupal, DrupalPractice, VariableAnalysis and SlevomatCodingStandard
New release published.
Thanks!
Changed status
Created MR so that it can be merge directly.
Moving to RTBC.
RTBC +1
Hey @colan can you grant me access as co-maintainer also, I want at least publish a Drupal 10 working update, thanks.
Is this should be tagged as `8.x-1.x-dev` or `8.x-1.x-alpha-2`?
I have not tested the patch but according to the Upgrade Status module, this module is not yet compatible with Drupal 10.
Maybe we need first tag this as "Review" to trigger the "Project Update Bot"