From eabe3a4d3314588e71e38fa75a19813e28bd282f Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 May 2013 16:32:19 -0700 Subject: [PATCH] Begin improving the soundness of received mail Summary: We/I broke a couple of things here recently (see D5911) and are doing some work here in general (see D5912, etc.). Generally, this code is pretty oldschool and not especially well architected for modern application-oriented Phabricator. It hardcodes a lot of stuff which should be applications' responsibilites. Take the first steps toward making it more solid to reduce the risk here. In particular: - Factor out the "self mail" and "duplicate mail" checks and add unit tests. - Make Message-ID hash handling automatic. Test Plan: Ran unit tests. Reviewers: btrahan, chad Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D5915 --- .../patches/20130513.receviedmailstatus.sql | 2 + scripts/mail/mail_handler.php | 3 - src/__phutil_library_map__.php | 6 + .../constants/MetaMTAReceivedMailStatus.php | 9 ++ .../PhabricatorMetaMTAReceiveController.php | 9 +- ...icatorMetaMTASendGridReceiveController.php | 2 - ...MetaMTAReceivedMailProcessingException.php | 20 +++ .../PhabricatorMetaMTAReceivedMail.php | 117 +++++++++++++----- ...PhabricatorMetaMTAReceivedMailTestCase.php | 52 ++++++++ .../patch/PhabricatorBuiltinPatchList.php | 4 + 10 files changed, 180 insertions(+), 44 deletions(-) create mode 100644 resources/sql/patches/20130513.receviedmailstatus.sql create mode 100644 src/applications/metamta/constants/MetaMTAReceivedMailStatus.php create mode 100644 src/applications/metamta/exception/PhabricatorMetaMTAReceivedMailProcessingException.php create mode 100644 src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php 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'), + ), ); } }