Uncaught RfcComplianceException when email From name contains a comma

I think the false positive in your test is because PhpMail::mail() isn't tested; kernel and functional tests set the mail plugin to test_mail_collector which subclasses PhpMail and overrides that method. This is by design; we don't want test runs trying to send real emails.

Perhaps we need to split the actual calls to mail() to their own method and only override that in TestMailCollector.

Opened an MR with the patch from #2 plus changes to PhpMail::mail() and TestMailCollector from #5. MailTest now fails, let's see if anything else breaks.

We should probably fix each of these tests to use valid email addresses.

I think I fixed the cases where the tests tried to send emails with no site email address set; this should just leave the one broken case where the comma triggers an error.

  /**
   * A list of headers that can contain multiple email addresses.
   *
   * @see \Symfony\Component\Mime\Header\Headers::HEADER_CLASS_MAP
   */
  private const MAILBOX_LIST_HEADERS = ['from', 'to', 'reply-to', 'cc', 'bcc'];

Can 'from' really contain multiple email addresses? Even if it can, this seems like an edge case we don't really want to support and we should just pass the 'from' header straight through. I special cased 'from' to do this in the MR.

edit: https://serverfault.com/questions/554520/smtp-allows-for-multiple-from-a... suggests this is technically valid but unlikely to be used in practice, especially I think in our case where a single website is responsible for sending the email - I can't see a reason why we would have multiple senders.

Fixing attribution.

Whilst I agree that a single from address makes sense, there's always https://xkcd.com/1172/

But I'm not sure what other options we have here.

There might be a rare case where someone altered the From header in hook_mail or hook_mail_alter. But by default we set $message['headers']['From'] ourselves in MailManager; we even set it up as a single MailboxHeader there and then render it to string to preserve backward compatibility in the hooks. So as #13 says there may be an edge case that we break here, but I don't see what other option we have, as Symfony seems to provide no way of parsing a header that might contain multiple values - we can only set up headers for sending.

I've added a test and a possible fix for this issue.

In the branch 3226117-phpmail-from-not-rfc-compliant-tests_only, I did the following:

• For \Drupal\Core\Mail\Plugin\Mail::mail(), I added a test using message with a From address with a comma in it.
• I wrapped the call to PHP's mail() function (used in \Drupal\Core\Mail\Plugin\Mail::mail()) in a separate method called doMail(). This way I could override that part of the code in the test and avoid an actual call to PHP's mail() function.

I created a merge request for this branch just so that the testbot runs the tests. Not sure if there's an other way on drupal.org for demonstrating a bug without using patches.

Additionally, in the branch 3226117-phpmail-from-not-rfc-compliant I came up with a possible fix for the issue:

• Instead of using explode() to split From addresses by comma, use str_getcsv() instead. That function supports ignoring comma's encapsulated in double quotes. Besides that, it seems to have the same effect as explode().

I found this solution by searching on "php split values by comma but not if they are in double quotes" and I had found the following: https://stackoverflow.com/questions/41449352/php-split-comma-separated-v...
I saw no value in retaining the quotes however, so I think just using str_getcsv() was fine.

Oops, when checking @longwave's work, I clicked on " https://git.drupalcode.org/project/drupal/-/merge_requests/992/diffs?commit_id=3bc0c9265b3475636e53b52731046f57e8ffc3dd">3bc0c926" in #9, figuring there were only two changes.

I see I should have looked at https://git.drupalcode.org/project/drupal/-/merge_requests/992/diffs instead.

I'll see if I can merge the two branches.

I added two commits from @longwave to the branch 3226117-phpmail-from-not-rfc-compliant. Not sure if the changes from the commit "Split PhpMail::mail() in two." (I made a similar split) is still needed. I'm assuming that the changes from commit "Disallow multiple email addresses in 'from' header." is redundant now, based on the comment from @larowlan in #13.

Not sure if this is related but after upgrading to 9.2.x I'm seeing:

The website encountered an unexpected error. Please try again later.
Error: Call to undefined method Symfony\Component\Mime\Header\Headers::addHeader() in Drupal\Core\Mail\Plugin\Mail\PhpMail->mail() (line 92 of core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php).

This happens only when attempting to submit forms whilst logged in.

Same pb for me

Verified that @MegaChriz MR 1070 fixes the Uncaught RfcComplianceException issue with sending email from sites that have a comma in the name.

Back to needs work for a comment on the MR.

I've done the following:

• Added a comment to explain the usage of str_getcsv();
• Locally, rebased branch "3226117-phpmail-from-not-rfc-compliant" on 9.3.x;
• git push --force

Is a forced git push the recommended way of getting an issue branch mergeable again?

Tested MR !1070 with Drupal 9.3.3: works like a charm. Thanks so much for this work.

Created a patch from https://git.drupalcode.org/project/drupal/-/merge_requests/1070 diffs (without tests) to apply while PR haven't merged yet.

Back to RTBC given that @catch's comment was addressed and @z3cka reported that the patch works for them.

Thank you @itaran for creating that patch!

The patch is not the one the testbot should keep testing because it contains the fix without the tests. The test failures also look unrelated to the changes, so setting back to RTBC. Also requeued the tests on the issue fork.

Can we get a D10 version of this too?

The D10 version can use union types for the new method type-hint.

Adding the needs-reroll tag for adding a D10 version

Created an MR for the D10 version, thanks!

Does the following line in \Drupal\Core\Mail\Plugin\Mail\PhpMail::mail() cause the test failure on PHP 8.1?
$return_path_set = strpos(ini_get('sendmail_path'), ' -f');

There is an issue after refactoring of core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php.

Steps to reproduce:

• Create an user without email. Set status "Blocked"
• Update user. Set status "Active". Save

Error is shown because Drupal tries to send mail, but user doesn't have it:
Got error 'PHP message: TypeError: Drupal\\Core\\Mail\\Plugin\\Mail\\PhpMail::doMail(): Argument #1 ($to) must be of type string, null given, called in /var/www/html/build/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php on line 120 in /var/www/html/build/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php on line 164 #0 /var/www/html/build/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php(120): Drupal\\Core\\Mail\\Plugin\\Mail\\PhpMail->doMail()\n#1 /var/www/html/build/core/lib/Drupal/Core/Mail/MailManager.php(311): Drupal\\Core\\Mail\\Plugin\\Mail\\PhpMail->mail()\n#2 /var/www/html/build/core/lib/Drupal/Core/Mail/MailManager.php(180): Drupal\\Core\\Mail\\MailManager->doMail()\n#3 /var/www/html/build/core/lib/Drupal/Core/Render/Renderer.php(564): Drupal\\Core\\Mail\\MailManager->Drupal\\Core\\Mail\\{closure}()\n#4 /var/www/html/build/core/lib/Drupal/Core/Mail/MailManager.php(181): Drupal\\Core\\Render\\Renderer->executeInRenderContext()\n#5 /var/www/html/build/core/modules/user/user.module(1067): Drupal\\Core\\Mail\\MailManager->mail()

#38 works well for me and webforms, thanks!

#38. doesn't work for me

Thank you very much for raising this issue and for your code contributions.

We encountered the same error:

Drupal\Core\Entity\EntityStorageException: Email "[SITENAME]" does not comply with addr-spec of RFC 2822. in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 811 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

when trying to submit a Webform with an email handler using the site name in the from field.
Since the site name contained a comma, it seems to have triggered this exception, see related issue:
#3219222-12: After 9.2.0 update, "Email "" does not comply with addr-spec of RFC 2822"
(Tested with webform:6.1.3)

After applying the patch from #38 with composer on Drupal core 9.4.3 (PHP: 7.4), the error seems to have been fixed and the Webform could be properly submitted with the site name containing a comma.

Additionally, we've given the email a quick test with the reroute_email module, to confirm the from and reply-to fields of the array generated for the email would have the correct values, with the comma.

Without the patch, a temporary solution in this particular case would be to remove the comma from the site name, or for example replaced with a pipe (|).
 

Not sure exactly why the tests failed on PHP 8.1 for the patch, but is there anything else holding off on this change?
We would greatly appreciate additional reviews and feedback on the code changes, in particular if you would see anything missing in the tests, or that would need to be updated.

Thanks in advance.

I'm not sure if testing the #38 is still viable for Drupal core 9.4.7, but we're also getting this issue with a comma in the sitename.

I can try to test something if that's the most applicable patch and still relevant to 9.4.7.

Confirming that #38 but the one for D9.3.x worked on 9.4.8(drupal-3226117-phpmail-from-not-rfc-compliant-38.patch).

The patch for 9.4.x didn't work(28-38.diff).

Hi guys. Thanks for the work on this issue. I refactored the patch:

#38 I'm not sure why we need to create users without email...

#33 Needs a D10 version

#35 I've created the patch from this MR

We are testing twice. See testCancelMessage() and PhpMailTest.

Let's see what the Testbot says.

I've generalized the issue title because this issue also affects Webform email handlers if the "From name" is configured for user-entered form data, which may contain a comma, eg "Lastname, Firstname"

Patch in #49 show the failing test and fix

Committed this to 10.1.x and cherry-picked to 10.0.x

With the union type, this won't pass on PHP 7.3 so can we get an adapted version of the patch for the 9.x branches?

Thanks

The only change in #57 is to the doMail() signature.

I don’t understand which test run failed but it looks like a random fail.

#57 Works for me on drupal 9.4.8 using both php 7.3 and 8.0.

I committed to 9.5.x and backported to 9.4.x as the risk of disruption is low

But then I thought to check if anything in contrib overrode the constructor without calling the parent, as they'd now be broken by the setting of $this->request.

https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22exten...

I found two instances

* https://git.drupalcode.org/project/ubercart_funds/-/blob/8.x-1.x/src/Plu... - this only has a D7 release, and currently would be broken as it doesn't set the config factory, safe to ignore
* https://git.drupalcode.org/project/mimemail/-/blob/8.x-1.x/src/Plugin/Ma... - this has 24k installs and would be broken too
* https://git.drupalcode.org/project/sendgrid_mailer/-/blob/1.1.x/src/Plug... - 60 installs, would be broken

Mimemail doesn't have a D10 version and I've pointed to the impending break in #3288663: [meta] Add support for Drupal 10.
Send Grid Mail does, so I commented on their D10 compatibility issue here #3289557: Automated Drupal 10 compatibility fixes.

I think its OK for this API change in the next major, even though constructors are not part of an API.

However, I don't think its ok to break 24k sites in D9.4 (and 9.5). So I then reverted this change.

Can we reroll this without the property assignment in the constructor to allow for those downstream modules that are technically doing it wrong. Even though we could also make this BC compliant and not break their D10 version, I think a major version is the appropriate time to communicate that they need to call the parent constructor.

Thanks

Addresses #66 and also fixes the doc block of the doMail function now that its parameter is not a union type.

The change to support 9.4.x/9.5.x looks good to me.

+++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php
@@ -106,10 +108,7 @@ public function mail(array $message) {
+    if (!$request->server->has('WINDIR') && !str_contains($request->server->get('SERVER_SOFTWARE'), 'Win32')) {

I suspect this check should be updated anyway - there are language-level constants that should be more reliable now: https://www.php.net/manual/en/reserved.constants.php#constant.php-os

Probably worth opening a followup to change this, then we don't need $request at all?

In fact we use this pattern in a number of places elsewhere in core:

  if (substr(PHP_OS, 0, 3) == 'WIN') {

Tests are failing

This one incorporates the feedback from longwave in #70 and #71. $request is now removed in favor of the PHP_OS constant.

Thank you @rschwab and @longwave for the idea to remove the request

Bumping in the hopes this can make the next release for 9.x.

Backported to 9.5.x - thanks all

Hi @larowlan

I think this was released in 9.5.4 but I am observing it in 9.5.5.

The log message is:

Error sending email: Email xxxx does not comply with addr-spec of RFC 2822

Remove comma from the 'From' field and the mail sends.

@tonypaulbarker yeah this should be fixed, can you confirm you're seeing the code changes of the above patch in your codebase?

@larowlan yes the update seems to have all applied fine and I see the new code in the codebase.

I will see if I can reproduce the issue on a fresh copy, run some tests and do some debugging and see if I can spot anything.