diff --git a/resources/sql/patches/20130513.receviedmailstatus.sql b/resources/sql/patches/20130513.receviedmailstatus.sql new file mode 100644 index 0000000000..1d83f1ee54 --- /dev/null +++ b/resources/sql/patches/20130513.receviedmailstatus.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_metamta.metamta_receivedmail + ADD status VARCHAR(32) NOT NULL; diff --git a/scripts/mail/mail_handler.php b/scripts/mail/mail_handler.php index 409b25081d..19e315364d 100755 --- a/scripts/mail/mail_handler.php +++ b/scripts/mail/mail_handler.php @@ -34,9 +34,6 @@ $received->setBodies(array( 'text' => $text_body, 'html' => $parser->getMessageBody('html'), )); -$received->setMessageIDHash( - PhabricatorHash::digestForIndex($received->getMessageID()) -); $attachments = array(); foreach ($parser->getAttachments() as $attachment) { diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c6666fb2f6..68f7d0e425 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -653,6 +653,7 @@ phutil_register_library_map(array( 'ManiphestView' => 'applications/maniphest/view/ManiphestView.php', 'MetaMTAConstants' => 'applications/metamta/constants/MetaMTAConstants.php', 'MetaMTANotificationType' => 'applications/metamta/constants/MetaMTANotificationType.php', + 'MetaMTAReceivedMailStatus' => 'applications/metamta/constants/MetaMTAReceivedMailStatus.php', 'ObjectHandleLoader' => 'applications/phid/handle/ObjectHandleLoader.php', 'OwnersPackageReplyHandler' => 'applications/owners/OwnersPackageReplyHandler.php', 'PHUI' => 'view/phui/PHUI.php', @@ -1107,6 +1108,8 @@ phutil_register_library_map(array( 'PhabricatorMetaMTAReceiveController' => 'applications/metamta/controller/PhabricatorMetaMTAReceiveController.php', 'PhabricatorMetaMTAReceivedListController' => 'applications/metamta/controller/PhabricatorMetaMTAReceivedListController.php', 'PhabricatorMetaMTAReceivedMail' => 'applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php', + 'PhabricatorMetaMTAReceivedMailProcessingException' => 'applications/metamta/exception/PhabricatorMetaMTAReceivedMailProcessingException.php', + 'PhabricatorMetaMTAReceivedMailTestCase' => 'applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php', 'PhabricatorMetaMTASendController' => 'applications/metamta/controller/PhabricatorMetaMTASendController.php', 'PhabricatorMetaMTASendGridReceiveController' => 'applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php', 'PhabricatorMetaMTAViewController' => 'applications/metamta/controller/PhabricatorMetaMTAViewController.php', @@ -2391,6 +2394,7 @@ phutil_register_library_map(array( 'ManiphestTransactionType' => 'ManiphestConstants', 'ManiphestView' => 'AphrontView', 'MetaMTANotificationType' => 'MetaMTAConstants', + 'MetaMTAReceivedMailStatus' => 'MetaMTAConstants', 'OwnersPackageReplyHandler' => 'PhabricatorMailReplyHandler', 'PHUIBoxExample' => 'PhabricatorUIExample', 'PHUIBoxView' => 'AphrontTagView', @@ -2832,6 +2836,8 @@ phutil_register_library_map(array( 'PhabricatorMetaMTAReceiveController' => 'PhabricatorMetaMTAController', 'PhabricatorMetaMTAReceivedListController' => 'PhabricatorMetaMTAController', 'PhabricatorMetaMTAReceivedMail' => 'PhabricatorMetaMTADAO', + 'PhabricatorMetaMTAReceivedMailProcessingException' => 'Exception', + 'PhabricatorMetaMTAReceivedMailTestCase' => 'PhabricatorTestCase', 'PhabricatorMetaMTASendController' => 'PhabricatorMetaMTAController', 'PhabricatorMetaMTASendGridReceiveController' => 'PhabricatorMetaMTAController', 'PhabricatorMetaMTAViewController' => 'PhabricatorMetaMTAController', diff --git a/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php b/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php new file mode 100644 index 0000000000..22eb948bf7 --- /dev/null +++ b/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php @@ -0,0 +1,9 @@ +isFormPost()) { $received = new PhabricatorMetaMTAReceivedMail(); - $header_content = array(); + $header_content = array( + 'Message-ID' => Filesystem::readRandomBytes(12), + ); $from = $request->getStr('sender'); $to = $request->getStr('receiver'); $uri = '/mail/received/'; @@ -42,11 +44,6 @@ final class PhabricatorMetaMTAReceiveController 'text' => $request->getStr('body'), )); - // Make up some unique value, since this column isn't nullable. - $received->setMessageIDHash( - PhabricatorHash::digestForIndex( - Filesystem::readRandomBytes(12))); - $received->save(); $received->processReceivedMail(); diff --git a/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php b/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php index ae75b348c2..1a6a1ad837 100644 --- a/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php +++ b/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php @@ -39,8 +39,6 @@ final class PhabricatorMetaMTASendGridReceiveController 'text' => $request->getStr('text'), 'html' => $request->getStr('from'), )); - $received->setMessageIDHash( - PhabricatorHash::digestForIndex($received->getMessageID())); $file_phids = array(); foreach ($_FILES as $file_raw) { diff --git a/src/applications/metamta/exception/PhabricatorMetaMTAReceivedMailProcessingException.php b/src/applications/metamta/exception/PhabricatorMetaMTAReceivedMailProcessingException.php new file mode 100644 index 0000000000..5fb1209885 --- /dev/null +++ b/src/applications/metamta/exception/PhabricatorMetaMTAReceivedMailProcessingException.php @@ -0,0 +1,20 @@ +statusCode; + } + + public function __construct($status_code /* ... */) { + $args = func_get_args(); + $this->statusCode = $args[0]; + + $args = array_slice($args, 1); + call_user_func_array(array('parent', '__construct'), $args); + } + +} diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php index afc47c2ba3..f891275bf6 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php @@ -5,11 +5,12 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { protected $headers = array(); protected $bodies = array(); protected $attachments = array(); + protected $status = ''; protected $relatedPHID; protected $authorPHID; protected $message; - protected $messageIDHash; + protected $messageIDHash = ''; public function getConfiguration() { return array( @@ -25,18 +26,31 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { // Normalize headers to lowercase. $normalized = array(); foreach ($headers as $name => $value) { - $normalized[strtolower($name)] = $value; + $name = $this->normalizeMailHeaderName($name); + if ($name == 'message-id') { + $this->setMessageIDHash(PhabricatorHash::digestForIndex($value)); + } + $normalized[$name] = $value; } $this->headers = $normalized; return $this; } + public function getHeader($key, $default = null) { + $key = $this->normalizeMailHeaderName($key); + return idx($this->headers, $key, $default); + } + + private function normalizeMailHeaderName($name) { + return strtolower($name); + } + public function getMessageID() { - return idx($this->headers, 'message-id'); + return $this->getHeader('Message-ID'); } public function getSubject() { - return idx($this->headers, 'subject'); + return $this->getHeader('Subject'); } public function getCCAddresses() { @@ -156,35 +170,15 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { public function processReceivedMail() { - // If Phabricator sent the mail, always drop it immediately. This prevents - // loops where, e.g., the public bug address is also a user email address - // and creating a bug sends them an email, which loops. - $is_phabricator_mail = idx( - $this->headers, - 'x-phabricator-sent-this-message'); - if ($is_phabricator_mail) { - $message = "Ignoring email with 'X-Phabricator-Sent-This-Message' ". - "header to avoid loops."; - return $this->setMessage($message)->save(); - } - - $message_id_hash = $this->getMessageIDHash(); - if ($message_id_hash) { - $messages = $this->loadAllWhere( - 'messageIDHash = %s', - $message_id_hash); - $messages_count = count($messages); - if ($messages_count > 1) { - $first_message = reset($messages); - if ($first_message->getID() != $this->getID()) { - $message = sprintf( - 'Ignoring email with message id hash "%s" that has been seen %d '. - 'times, including this message.', - $message_id_hash, - $messages_count); - return $this->setMessage($message)->save(); - } - } + try { + $this->dropMailFromPhabricator(); + $this->dropMailAlreadyReceived(); + } catch (PhabricatorMetaMTAReceivedMailProcessingException $ex) { + $this + ->setStatus($ex->getStatusCode()) + ->setMessage($ex->getMessage()) + ->save(); + return $this; } list($to, @@ -460,4 +454,61 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { return $user; } + /** + * If Phabricator sent the mail, always drop it immediately. This prevents + * loops where, e.g., the public bug address is also a user email address + * and creating a bug sends them an email, which loops. + */ + private function dropMailFromPhabricator() { + if (!$this->getHeader('x-phabricator-sent-this-message')) { + return; + } + + throw new PhabricatorMetaMTAReceivedMailProcessingException( + MetaMTAReceivedMailStatus::STATUS_FROM_PHABRICATOR, + "Ignoring email with 'X-Phabricator-Sent-This-Message' header to avoid ". + "loops."); + } + + /** + * If this mail has the same message ID as some other mail, and isn't the + * first mail we we received with that message ID, we drop it as a duplicate. + */ + private function dropMailAlreadyReceived() { + $message_id_hash = $this->getMessageIDHash(); + if (!$message_id_hash) { + // No message ID hash, so we can't detect duplicates. This should only + // happen with very old messages. + return; + } + + $messages = $this->loadAllWhere( + 'messageIDHash = %s ORDER BY id ASC LIMIT 2', + $message_id_hash); + $messages_count = count($messages); + if ($messages_count <= 1) { + // If we only have one copy of this message, we're good to process it. + return; + } + + $first_message = reset($messages); + if ($first_message->getID() == $this->getID()) { + // If this is the first copy of the message, it is okay to process it. + // We may not have been able to to process it immediately when we received + // it, and could may have received several copies without processing any + // yet. + return; + } + + $message = sprintf( + 'Ignoring email with message id hash "%s" that has been seen %d '. + 'times, including this message.', + $message_id_hash, + $messages_count); + + throw new PhabricatorMetaMTAReceivedMailProcessingException( + MetaMTAReceivedMailStatus::STATUS_DUPLICATE, + $message); + } + } diff --git a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php new file mode 100644 index 0000000000..d49c08ce18 --- /dev/null +++ b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php @@ -0,0 +1,52 @@ + true, + ); + } + + public function testDropSelfMail() { + $mail = new PhabricatorMetaMTAReceivedMail(); + $mail->setHeaders( + array( + 'X-Phabricator-Sent-This-Message' => 'yes', + )); + $mail->save(); + + $mail->processReceivedMail(); + + $this->assertEqual( + MetaMTAReceivedMailStatus::STATUS_FROM_PHABRICATOR, + $mail->getStatus()); + } + + + public function testDropDuplicateMail() { + $mail_a = new PhabricatorMetaMTAReceivedMail(); + $mail_a->setHeaders( + array( + 'Message-ID' => 'test@example.com', + )); + $mail_a->save(); + + $mail_b = new PhabricatorMetaMTAReceivedMail(); + $mail_b->setHeaders( + array( + 'Message-ID' => 'test@example.com', + )); + $mail_b->save(); + + $mail_a->processReceivedMail(); + $mail_b->processReceivedMail(); + + $this->assertEqual( + MetaMTAReceivedMailStatus::STATUS_DUPLICATE, + $mail_b->getStatus()); + } + + + +} diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index 519ecb9e8c..e33710a5a1 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1294,6 +1294,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'php', 'name' => $this->getPatchPath('20130508.releephtransactionsmig.php'), ), + '20130513.receviedmailstatus.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('20130513.receviedmailstatus.sql'), + ), ); } }