CRM-21521: fix nested multipart handling

When for example a multipart-report email body's first part is multipart-related, extract the text correctly instead of crashing. This logic should handle all kinds of weird multipart nesting, up to MAIL_MAX_RECURSION levels deep.
civicrm · Dec 8, 2017 · b80b567 · b80b567
1 parent ba2bee9
commit b80b567
Show file tree

Hide file tree

Showing 3 changed files with 433 additions and 49 deletions.
diff --git a/CRM/Utils/Mail/EmailProcessor.php b/CRM/Utils/Mail/EmailProcessor.php
@@ -35,6 +35,7 @@
 // before the 4.1 release
 define('EMAIL_ACTIVITY_TYPE_ID', NULL);
 define('MAIL_BATCH_SIZE', 50);
+define('MAIL_MAX_RECURSION', 10);
 
 /**
  * Class CRM_Utils_Mail_EmailProcessor.
@@ -286,55 +287,7 @@ public static function _process($civiMail, $dao, $is_create_activities) {
                 $text = $mail->body->text;
               }
               elseif ($mail->body instanceof ezcMailMultipart) {
-                if ($mail->body instanceof ezcMailMultipartReport) {
-                  $part = $mail->body->getMachinePart();
-                  if ($part instanceof ezcMailDeliveryStatus) {
-                    foreach ($part->recipients as $rec) {
-                      if (isset($rec["Diagnostic-Code"])) {
-                        $text = $rec["Diagnostic-Code"];
-                        break;
-                      }
-                      elseif (isset($rec["Description"])) {
-                        $text = $rec["Description"];
-                        break;
-                      }
-                      // no diagnostic info present - try getting the human readable part
-                      elseif (isset($rec["Status"])) {
-                        $text = $rec["Status"];
-                        $textpart = $mail->body->getReadablePart();
-                        if ($textpart != NULL and isset($textpart->text)) {
-                          $text .= " " . $textpart->text;
-                        }
-                        else {
-                          $text .= " Delivery failed but no diagnostic code or description.";
-                        }
-                        break;
-                      }
-                    }
-                  }
-                  elseif ($part != NULL and isset($part->text)) {
-                    $text = $part->text;
-                  }
-                  elseif (($part = $mail->body->getReadablePart()) != NULL) {
-                    $text = $part->text;
-                  }
-                }
-                elseif ($mail->body instanceof ezcMailMultipartRelated) {
-                  foreach ($mail->body->getRelatedParts() as $part) {
-                    if (isset($part->subType) and $part->subType == 'plain') {
-                      $text = $part->text;
-                      break;
-                    }
-                  }
-                }
-                else {
-                  foreach ($mail->body->getParts() as $part) {
-                    if (isset($part->subType) and $part->subType == 'plain') {
-                      $text = $part->text;
-                      break;
-                    }
-                  }
-                }
+                $text = self::getTextFromMultipart($mail->body);
               }
 
               if (
@@ -475,4 +428,107 @@ public static function _process($civiMail, $dao, $is_create_activities) {
     }
   }
 
+  /**
+   * @param \ezcMailMultipart $multipart
+   * @param int $recursionLevel
+   *
+   * @return array
+   */
+  protected static function getTextFromMultipart($multipart, $recursionLevel = 0) {
+    if ($recursionLevel >= MAIL_MAX_RECURSION) {
+      return NULL;
+    }
+    $recursionLevel += 1;
+    $text = NULL;
+    if ($multipart instanceof ezcMailMultipartReport) {
+      $text = self::getTextFromMulipartReport($multipart, $recursionLevel);
+    }
+    elseif ($multipart instanceof ezcMailMultipartRelated) {
+      $text = self::getTextFromMultipartRelated($multipart, $recursionLevel);
+    }
+    else {
+      foreach ($multipart->getParts() as $part) {
+        if (isset($part->subType) and $part->subType === 'plain') {
+          $text = $part->text;
+        }
+        elseif ($part instanceof ezcMailMultipart) {
+          $text = self::getTextFromMultipart($part);
+        }
+        if ($text) {
+          break;
+        }
+      }
+    }
+    return $text;
+  }
+
+  /**
+   * @param \ezcMailMultipartRelated $related
+   * @param int $recursionLevel
+   *
+   * @return array
+   */
+  protected static function getTextFromMultipartRelated($related, $recursionLevel) {
+    $text = NULL;
+    foreach ($related->getRelatedParts() as $part) {
+      if (isset($part->subType) and $part->subType === 'plain') {
+        $text = $part->text;
+      }
+      elseif ($part instanceof ezcMailMultipart) {
+        $text = self::getTextFromMultipart($part, $recursionLevel);
+      }
+      if ($text) {
+        break;
+      }
+    }
+    return $text;
+  }
+
+  /**
+   * @param \ezcMailMultipartReport $multipart
+   * @param $recursionLevel
+   *
+   * @return array
+   */
+  protected static function getTextFromMulipartReport($multipart, $recursionLevel) {
+    $text = NULL;
+    $part = $multipart->getMachinePart();
+    if ($part instanceof ezcMailDeliveryStatus) {
+      foreach ($part->recipients as $rec) {
+        if (isset($rec["Diagnostic-Code"])) {
+          $text = $rec["Diagnostic-Code"];
+          break;
+        }
+        elseif (isset($rec["Description"])) {
+          $text = $rec["Description"];
+          break;
+        }
+        // no diagnostic info present - try getting the human readable part
+        elseif (isset($rec["Status"])) {
+          $text = $rec["Status"];
+          $textpart = $multipart->getReadablePart();
+          if ($textpart !== NULL and isset($textpart->text)) {
+            $text .= " " . $textpart->text;
+          }
+          else {
+            $text .= " Delivery failed but no diagnostic code or description.";
+          }
+          break;
+        }
+      }
+    }
+    elseif ($part !== NULL and isset($part->text)) {
+      $text = $part->text;
+    }
+    elseif (($part = $multipart->getReadablePart()) !== NULL) {
+      if (isset($part->text)) {
+        $text = $part->text;
+      }
+      elseif ($part instanceof ezcMailMultipart) {
+        $text = self::getTextFromMultipart($part, $recursionLevel);
+      }
+    }
+    return $text;
+  }
+
 }
diff --git a/tests/phpunit/CRM/Utils/Mail/EmailProcessorTest.php b/tests/phpunit/CRM/Utils/Mail/EmailProcessorTest.php
@@ -69,6 +69,21 @@ public function testProcessingMultipartRelatedEmail() {
     $this->checkMailingBounces(1);
   }
 
+  /**
+   * Tests that a nested multipart email does not cause pain & misery & fatal errors.
+   *
+   * Sample anonymized from an email that broke bounce processing at Wikimedia
+   */
+  public function testProcessingNestedMultipartEmail() {
+    $this->setUpMailing();
+    $mail = 'test_nested_message.eml';
+
+    copy(__DIR__ . '/data/bounces/' . $mail, __DIR__ . '/data/mail/' . $mail);
+    $this->callAPISuccess('job', 'fetch_bounces', array());
+    $this->assertFalse(file_exists(__DIR__ . '/data/mail/' . $mail));
+    $this->checkMailingBounces(1);
+  }
+
   /**
    * Test that a deleted email does not cause a hard fail.
    *