Problem/Motivation

Steps to reproduce

  1. Enable SMTP module and configure it.
  2. Enable workbench or any other module that triggers an email notification on node save.
  3. Save the node and you will get an error i.e.
    The website encountered an unexpected error.

Proposed resolution

In SMTPMailSystem.php, the below code causes this issue:

case 'cc':
          $cc_recipients = explode(',', $value);
          foreach ($cc_recipients as $cc_recipient) {
            $cc_comp = $this->getComponents($cc_recipient);
            $mailer->AddCC($cc_comp['email'], $cc_comp['name']);
          }
          break;

        case 'bcc':
          $bcc_recipients = explode(',', $value);
          foreach ($bcc_recipients as $bcc_recipient) {
            $bcc_comp = $this->getComponents($bcc_recipient);
            $mailer->AddBCC($bcc_comp['email'], Unicode::mimeHeaderEncode($bcc_comp['name']));
          }
          break;

There is no empty check for $value that results in the below error:

Drupal\Core\Entity\EntityStorageException: Invalid address: (cc): in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 846 of /var/www/d8/docroot/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).
    

PHPMailer\PHPMailer\Exception: Invalid address: (cc): in PHPMailer\PHPMailer\PHPMailer->addOrEnqueueAnAddress() (line 1081 of /var/www/d8/vendor/phpmailer/phpmailer/src/PHPMailer.php).

The attached patch solves the problem. It checks for the NULL/Empty value and if it's empty then it doesn't execute the PHPMailer code.

CommentFileSizeAuthor
#13 3174535-13-invalid-address.patch2.08 KBandrew answer
#9 3174535-9-invalid-address.patch2.46 KBtr
#9 3174535-9-invalid-address-test-only.patch1.34 KBtr
#8 3174535-invalid-address-8.patch12.46 KBpushpinderchauhan
#6 3174535-invalid-address-6.patch12.45 KBpushpinderchauhan
#2 3174535-invalid-address-2.patch1.9 KBpushpinderchauhan

Comments

er.pushpinderrana created an issue. See original summary.

pushpinderchauhan’s picture

pushpinderchauhan commented
StatusFileSize
new 3174535-invalid-address-2.patch1.9 KB
pushpinderchauhan’s picture

pushpinderchauhan commented
Assigned: pushpinderchauhan » Unassigned
Status: Active » Needs review
pushpinderchauhan’s picture

pushpinderchauhan commented
Issue summary: View changes
tr’s picture

tr commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Seems like the wrong fix.
The headers are looped over like this:
foreach ($headers as $key => $value)

A valid email message MUST have a $value for each $key. I don't know why $value isn't set for some keys, but the $value should be checked at this point, even before the switch begins. It shouldn't be checked in each individual case, like you do in the patch - note there are other cases in that switch that should have $value checked which you didn't fix. If you check $value before the switch, that's basically one line of code that needs to be added. If $value is not set, $headers[$key] should probably be unset so we don't send invalid headers to PHPMailer.

But I think it's also worth the effort to trace back and see how the Cc: header is getting set with no value - it's quite possible that your module which is sending email is passing wrong values, and that should be fixed.

Likewise, to make this module more robust, a test should be added to check that sending a header without a $value will work without error.

pushpinderchauhan’s picture

pushpinderchauhan commented
Status: Needs work » Needs review
StatusFileSize
new 3174535-invalid-address-6.patch12.45 KB

Thanks, TR. I agree with you. The origin of the issue is the content_moderation_notifications module which is passing empty cc & bcc keys to header array. I'll take care of that module issue later and post a patch for that too.

Eariler I wasn't sure whether $value is mandatory or not but after digging in the code it seems we may apply explicit check for this. Giving it a try with another patch.

Status: Needs review » Needs work

The last submitted patch, 6: 3174535-invalid-address-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

pushpinderchauhan’s picture

pushpinderchauhan commented
Status: Needs work » Needs review
StatusFileSize
new 3174535-invalid-address-8.patch12.46 KB
tr’s picture

tr commented
Version: 8.x-1.0 » 8.x-1.x-dev
StatusFileSize
new 3174535-9-invalid-address-test-only.patch1.34 KB
new 3174535-9-invalid-address.patch2.46 KB

Thanks for the test, that's very important.

When you add a test you should post a patch with only the test in it - that patch should FAIL, and that will demonstrate that the test is really finding the bug. Also post a second patch with both the test AND the fix, which will demonstrate that the proposed fix actually fixes the bug.

It's hard to review your patch in #8 because so many lines are changed. The fix I had in mind is more like this:

    foreach ($headers as $key => $value) {
      if ($value == NULL || $value == '') {
        // $value should always be set. If not, remove the header field and
        // skip to the next header field.
        unset($headers[$key]);
        continue;
      }
      switch (strtolower($key)) {

This checks the $value without having to reformat all the switch code, and adds a comment saying what is being done and why. This makes it much easier to read and to see what is being done, which makes it easier to understand and maintain.

Here is a new patch using the above fix, and correcting some minor coding standards problems with your test. I've also uploaded the test separately to see if it will fail without the fix.

The last submitted patch, 9: 3174535-9-invalid-address-test-only.patch, failed testing. View results

tr’s picture

tr commented

The results in #9 are exactly what were expected:

  1. The test-only patch FAILS, which demonstrates that there is currently a problem with the SMTP module when the "Cc:" header is empty.
  2. The fix + test patch PASSES, which demonstrates that the fix corrects the problem and makes the test work.
pushpinderchauhan’s picture

pushpinderchauhan commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Great... Thanks for the detailed explanation and your support in this issue. It really helps!

Since you have posted the patch now and it works perfectly so moving it to RTBC.

andrew answer’s picture

andrew answer commented
StatusFileSize
new 3174535-13-invalid-address.patch2.08 KB

Patch re-rolled for last dev version.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 3174535-13-invalid-address.patch, failed testing. View results

andrew answer’s picture

andrew answer commented

Test failed, but if I'll apply https://www.drupal.org/project/smtp/issues/3191046#comment-14026470 together with this patch, it will be finished successfully.

andrew answer’s picture

andrew answer commented
Status: Needs work » Needs review
andrew answer’s picture

andrew answer commented
Status: Needs review » Reviewed & tested by the community

I confirm that two patches works together; +1 to release.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 3174535-13-invalid-address.patch, failed testing. View results

japerry’s picture

japerry
Location KOMK
commented
Status: Needs work » Fixed

Thanks for the patch and the tests! Committed.

rivimey’s picture

rivimey
she/her
commented

Have no idea if it is in fact related, but I've just entered a bug #3304401: From address considered invalid which relates to email address validation and was introduced in the v1.1 release.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.