Issue #3226117 by MegaChriz, marthinal, longwave, rschwab, sinn, dcam,...

Issue #3226117 by MegaChriz, marthinal, longwave, rschwab, sinn, dcam, cilefen, ankithashetty, itaran, larowlan, catch: [Needs backport] Uncaught RfcComplianceException when email From name contains a comma

core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php

@@ -87,7 +87,9 @@ public function mail(array $message) {
     $headers = new Headers();
     foreach ($message['headers'] as $name => $value) {
       if (in_array(strtolower($name), self::MAILBOX_LIST_HEADERS, TRUE)) {
-        $value = explode(',', $value);
+        // Split values by comma, but ignore commas encapsulated in double
+        // quotes.
+        $value = str_getcsv($value, ',');
       }
       $headers->addHeader($name, $value);
     }
@@ -104,12 +106,7 @@ public function mail(array $message) {
     $mail_headers = str_replace("\r\n", "\n", $headers->toString());
     $mail_subject = str_replace("\r\n", "\n", $mail_subject);
 
-    $request = \Drupal::request();
-
-    // We suppress warnings and notices from mail() because of issues on some
-    // hosts. The return value of this method will still indicate whether mail
-    // was sent successfully.
-    if (!$request->server->has('WINDIR') && strpos($request->server->get('SERVER_SOFTWARE'), 'Win32') === FALSE) {
+    if (substr(PHP_OS, 0, 3) != 'WIN') {
       // On most non-Windows systems, the "-f" option to the sendmail command
       // is used to set the Return-Path. There is no space between -f and
       // the value of the return path.
@@ -117,7 +114,7 @@ public function mail(array $message) {
       // we assume to be safe.
       $site_mail = $this->configFactory->get('system.site')->get('mail');
       $additional_headers = isset($message['Return-Path']) && ($site_mail === $message['Return-Path'] || static::_isShellSafe($message['Return-Path'])) ? '-f' . $message['Return-Path'] : '';
-      $mail_result = @mail(
+      $mail_result = $this->doMail(
         $message['to'],
         $mail_subject,
         $mail_body,
@@ -130,7 +127,7 @@ public function mail(array $message) {
       // Return-Path header.
       $old_from = ini_get('sendmail_from');
       ini_set('sendmail_from', $message['Return-Path']);
-      $mail_result = @mail(
+      $mail_result = $this->doMail(
         $message['to'],
         $mail_subject,
         $mail_body,
@@ -142,6 +139,36 @@ public function mail(array $message) {
     return $mail_result;
   }
 
+  /**
+   * Wrapper around PHP's mail() function.
+   *
+   * We suppress warnings and notices from mail() because of issues on some
+   * hosts. The return value of this method will still indicate whether mail was
+   * sent successfully.
+   *
+   * @param string $to
+   *   Receiver, or receivers of the mail.
+   * @param string $subject
+   *   Subject of the email to be sent.
+   * @param string $message
+   *   Message to be sent.
+   * @param array $additional_headers
+   *   (optional) Array to be inserted at the end of the email header.
+   * @param string $additional_params
+   *   (optional) Can be used to pass additional flags as command line options.
+   *
+   * @see mail()
+   */
+  protected function doMail(string $to, string $subject, string $message, $additional_headers = [], string $additional_params = ''): bool {
+    return @mail(
+      $to,
+      $subject,
+      $message,
+      $additional_headers,
+      $additional_params
+    );
+  }
+
   /**
    * Disallows potentially unsafe shell characters.
    *

core/modules/system/tests/src/Kernel/Mail/MailTest.php

@@ -40,6 +40,12 @@ protected function setUp(): void {
     parent::setUp();
     $this->installEntitySchema('user');
     $this->installEntitySchema('file');
+
+    // Set required site configuration.
+    $this->config('system.site')
+      ->set('mail', 'mailtest@example.com')
+      ->set('name', 'Drupal')
+      ->save();
   }
 
   /**
@@ -115,12 +121,6 @@ public function testCancelMessage() {
   public function testFromAndReplyToHeader() {
     $language = \Drupal::languageManager()->getCurrentLanguage();
 
-    // Set required site configuration.
-    $this->config('system.site')
-      ->set('mail', 'mailtest@example.com')
-      ->set('name', 'Drupal')
-      ->save();
-
     // Reset the state variable that holds sent messages.
     \Drupal::state()->set('system.test_mail_collector', []);
     // Send an email with a reply-to address specified.
@@ -153,6 +153,15 @@ public function testFromAndReplyToHeader() {
     // Errors-to header must not be set, it is deprecated.
     $this->assertFalse(isset($sent_message['headers']['Errors-To']));
 
+    // Test that From names containing commas work as expected.
+    $this->config('system.site')->set('name', 'Foo, Bar, and Baz')->save();
+    // Send an email and check that the From-header contains the site name.
+    \Drupal::service('plugin.manager.mail')->mail('mail_cancel_test', 'from_test', 'from_test@example.com', $language);
+    $captured_emails = \Drupal::state()->get('system.test_mail_collector');
+    $sent_message = end($captured_emails);
+    // From header contains the quoted site name with commas.
+    $this->assertEquals('"Foo, Bar, and Baz" <mailtest@example.com>', $sent_message['headers']['From']);
+
     // Test RFC-2822 rules are respected for 'display-name' component of
     // 'From:' header. Specials characters are not allowed, so randomly add one
     // of them to the site name and check the string is wrapped in quotes. Also

core/modules/user/tests/src/Kernel/UserMailNotifyTest.php

@@ -82,6 +82,7 @@ public function userMailsProvider() {
    */
   public function testUserMailsSent($op, array $mail_keys) {
     $this->installConfig('user');
+    $this->config('system.site')->set('mail', 'test@example.com')->save();
     $this->config('user.settings')->set('notify.' . $op, TRUE)->save();
     $return = _user_mail_notify($op, $this->createUser());
     $this->assertTrue($return);
@@ -173,6 +174,7 @@ public function testUserRecoveryMailLanguage() {
 
     // Recovery email should respect user preferred langcode by default if
     // langcode not set.
+    $this->config('system.site')->set('mail', 'test@example.com')->save();
     $params['account'] = $user;
     $default_email = \Drupal::service('plugin.manager.mail')->mail('user', 'password_reset', $user->getEmail(), $preferredLangcode, $params);
     $this->assertTrue($default_email['result']);

core/tests/Drupal/KernelTests/Core/Action/EmailActionTest.php

@@ -31,6 +31,8 @@ protected function setUp(): void {
    * Tests the email action plugin.
    */
   public function testEmailAction() {
+    $this->config('system.site')->set('mail', 'test@example.com')->save();
+
     /** @var \Drupal\Core\Action\ActionManager $plugin_manager */
     $plugin_manager = $this->container->get('plugin.manager.action');
     $configuration = [

core/tests/Drupal/Tests/Core/Mail/Plugin/Mail/PhpMailTest.php

+<?php
+
+namespace Drupal\Tests\Core\Mail\Plugin\Mail;
+
+use Drupal\Core\DependencyInjection\ContainerBuilder;
+use Drupal\Core\Mail\Plugin\Mail\PhpMail;
+use Drupal\Tests\UnitTestCase;
+
+/**
+ * @coversDefaultClass \Drupal\Core\Mail\Plugin\Mail\PhpMail
+ * @group Mail
+ */
+class PhpMailTest extends UnitTestCase {
+
+  /**
+   * The configuration factory.
+   *
+   * @var \Drupal\Core\Config\ConfigFactoryInterface|\PHPUnit\Framework\MockObject\MockObject
+   */
+  protected $configFactory;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp(): void {
+    parent::setUp();
+
+    // Use the provided config for system.mail.interface settings.
+    $this->configFactory = $this->getConfigFactoryStub([
+      'system.mail' => [
+        'interface' => [],
+      ],
+      'system.site' => [
+        'mail' => 'test@example.com',
+      ],
+    ]);
+
+    $container = new ContainerBuilder();
+    $container->set('config.factory', $this->configFactory);
+    \Drupal::setContainer($container);
+  }
+
+  /**
+   * Creates a mocked PhpMail object.
+   *
+   * The method "doMail()" gets overridden to avoid a mail() call in tests.
+   *
+   * @return \Drupal\Core\Mail\Plugin\Mail\PhpMail|\PHPUnit\Framework\MockObject\MockObject
+   *   A PhpMail instance.
+   */
+  protected function createPhpMailInstance(): PhpMail {
+    $mailer = $this->getMockBuilder(PhpMail::class)
+      ->onlyMethods(['doMail'])
+      ->getMock();
+
+    $mailer->expects($this->once())->method('doMail')
+      ->willReturn(TRUE);
+
+    return $mailer;
+  }
+
+  /**
+   * Tests sending a mail using a From address with a comma in it.
+   *
+   * @covers ::testMail
+   */
+  public function testMail() {
+    // Setup a mail message.
+    $message = [
+      'id' => 'example_key',
+      'module' => 'example',
+      'key' => 'key',
+      'to' => 'to@example.org',
+      'from' => 'from@example.org',
+      'reply-to' => 'from@example.org',
+      'langcode' => 'en',
+      'params' => [],
+      'send' => TRUE,
+      'subject' => '',
+      'body' => '',
+      'headers' => [
+        'MIME-Version' => '1.0',
+        'Content-Type' => 'text/plain; charset=UTF-8; format=flowed; delsp=yes',
+        'Content-Transfer-Encoding' => '8Bit',
+        'X-Mailer' => 'Drupal',
+        'Return-Path' => 'from@example.org',
+        'From' => '"Foo, Bar, and Baz" <from@example.org>',
+        'Reply-to' => 'from@example.org',
+      ],
+    ];
+
+    $mailer = $this->createPhpMailInstance();
+    $this->assertTrue($mailer->mail($message));
+  }
+
+}