diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a564ba0f3b..d1c833760c 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1185,6 +1185,7 @@ phutil_register_library_map(array( 'PhabricatorObjectItemView' => 'view/layout/PhabricatorObjectItemView.php', 'PhabricatorObjectListView' => 'view/control/PhabricatorObjectListView.php', 'PhabricatorObjectMailReceiver' => 'applications/metamta/receiver/PhabricatorObjectMailReceiver.php', + 'PhabricatorObjectMailReceiverTestCase' => 'applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php', 'PhabricatorObjectSelectorDialog' => 'view/control/PhabricatorObjectSelectorDialog.php', 'PhabricatorOffsetPagedQuery' => 'infrastructure/query/PhabricatorOffsetPagedQuery.php', 'PhabricatorOwnerPathQuery' => 'applications/owners/query/PhabricatorOwnerPathQuery.php', @@ -2920,6 +2921,7 @@ phutil_register_library_map(array( 'PhabricatorObjectItemView' => 'AphrontTagView', 'PhabricatorObjectListView' => 'AphrontView', 'PhabricatorObjectMailReceiver' => 'PhabricatorMailReceiver', + 'PhabricatorObjectMailReceiverTestCase' => 'PhabricatorTestCase', 'PhabricatorOffsetPagedQuery' => 'PhabricatorQuery', 'PhabricatorOwnersConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorOwnersController' => 'PhabricatorController', diff --git a/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php b/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php index 3245e19a76..494213eb59 100644 --- a/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php +++ b/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php @@ -72,7 +72,10 @@ final class PhabricatorManiphestTaskTestDataGenerator public function getProjectPHIDs() { $projects = array(); for ($i = 0; $i < rand(1, 4);$i++) { - $projects[] = $this->loadOneRandom("PhabricatorProject")->getPHID(); + $project = $this->loadOneRandom("PhabricatorProject"); + if ($project) { + $projects[] = $project->getPHID(); + } } return $projects; } diff --git a/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php b/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php index 7f1e477ad1..062fcbdfbe 100644 --- a/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php +++ b/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php @@ -8,5 +8,11 @@ final class MetaMTAReceivedMailStatus const STATUS_NO_RECEIVERS = 'err:no-receivers'; const STATUS_ABUNDANT_RECEIVERS = 'err:multiple-receivers'; const STATUS_UNKNOWN_SENDER = 'err:unknown-sender'; + const STATUS_DISABLED_SENDER = 'err:disabled-sender'; + const STATUS_NO_PUBLIC_MAIL = 'err:no-public-mail'; + const STATUS_USER_MISMATCH = 'err:bad-user'; + const STATUS_POLICY_PROBLEM = 'err:policy'; + const STATUS_NO_SUCH_OBJECT = 'err:not-found'; + const STATUS_HASH_MISMATCH = 'err:bad-hash'; } diff --git a/src/applications/metamta/receiver/PhabricatorMailReceiver.php b/src/applications/metamta/receiver/PhabricatorMailReceiver.php index 575aca6ff9..b44e67ac8b 100644 --- a/src/applications/metamta/receiver/PhabricatorMailReceiver.php +++ b/src/applications/metamta/receiver/PhabricatorMailReceiver.php @@ -6,6 +6,22 @@ abstract class PhabricatorMailReceiver { abstract public function isEnabled(); abstract public function canAcceptMail(PhabricatorMetaMTAReceivedMail $mail); + public function validateSender( + PhabricatorMetaMTAReceivedMail $mail, + PhabricatorUser $sender) { + + if ($sender->getIsDisabled()) { + throw new PhabricatorMetaMTAReceivedMailProcessingException( + MetaMTAReceivedMailStatus::STATUS_DISABLED_SENDER, + pht( + "Sender '%s' has a disabled user account.", + $sender->getUsername())); + } + + + return; + } + /** * Identifies the sender's user account for a piece of received mail. Note * that this method does not validate that the sender is who they say they diff --git a/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php b/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php index 2fb0632be5..f429106f7d 100644 --- a/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php +++ b/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php @@ -2,15 +2,101 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver { + /** + * Return a regular expression fragment which matches the name of an + * object which can receive mail. For example, Differential uses: + * + * D[1-9]\d* + * + * ...to match `D123`, etc., identifying Differential Revisions. + * + * @return string Regular expression fragment. + */ abstract protected function getObjectPattern(); - final public function canAcceptMail(PhabricatorMetaMTAReceivedMail $mail) { - $regexp = $this->getAddressRegexp(); + + /** + * Load the object receiving mail, based on an identifying pattern. Normally + * this pattern is some sort of object ID. + * + * @param string A string matched by @{method:getObjectPattern} + * fragment. + * @param PhabricatorUser The viewing user. + * @return void + */ + abstract protected function loadObject($pattern, PhabricatorUser $viewer); + + + public function validateSender( + PhabricatorMetaMTAReceivedMail $mail, + PhabricatorUser $sender) { + parent::validateSender($mail, $sender); foreach ($mail->getToAddresses() as $address) { - $address = self::stripMailboxPrefix($address); - $local = id(new PhutilEmailAddress($address))->getLocalPart(); - if (preg_match($regexp, $local)) { + $parts = $this->matchObjectAddress($address); + if ($parts) { + break; + } + } + + try { + $object = $this->loadObject($parts['pattern'], $sender); + } catch (PhabricatorPolicyException $policy_exception) { + throw new PhabricatorMetaMTAReceivedMailProcessingException( + MetaMTAReceivedMailStatus::STATUS_POLICY_PROBLEM, + pht( + "This mail is addressed to an object you are not permitted ". + "to see: %s", + $policy_exception->getMessage())); + } + + if (!$object) { + throw new PhabricatorMetaMTAReceivedMailProcessingException( + MetaMTAReceivedMailStatus::STATUS_NO_SUCH_OBJECT, + pht( + "This mail is addressed to an object ('%s'), but that object ". + "does not exist.", + $parts['pattern'])); + } + + $sender_identifier = $parts['sender']; + + if ($sender_identifier === 'public') { + if (!PhabricatorEnv::getEnvConfig('metamta.public-replies')) { + throw new PhabricatorMetaMTAReceivedMailProcessingException( + MetaMTAReceivedMailStatus::STATUS_NO_PUBLIC_MAIL, + pht( + "This mail is addressed to an object's public address, but ". + "public replies are not enabled (`metamta.public-replies`).")); + } + $check_phid = $object->getPHID(); + } else { + if ($sender_identifier != $sender->getID()) { + throw new PhabricatorMetaMTAReceivedMailProcessingException( + MetaMTAReceivedMailStatus::STATUS_USER_MISMATCH, + pht( + "This mail is addressed to an object's private address, but ". + "the sending user and the private address owner are not the ". + "same user.")); + } + $check_phid = $sender->getPHID(); + } + + $expect_hash = self::computeMailHash($object->getMailKey(), $check_phid); + + if ($expect_hash != $parts['hash']) { + throw new PhabricatorMetaMTAReceivedMailProcessingException( + MetaMTAReceivedMailStatus::STATUS_HASH_MISMATCH, + pht( + "The hash in this object's address does not match the expected ". + "value.")); + } + } + + + final public function canAcceptMail(PhabricatorMetaMTAReceivedMail $mail) { + foreach ($mail->getToAddresses() as $address) { + if ($this->matchObjectAddress($address)) { return true; } } @@ -18,6 +104,20 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver { return false; } + private function matchObjectAddress($address) { + $regexp = $this->getAddressRegexp(); + + $address = self::stripMailboxPrefix($address); + $local = id(new PhutilEmailAddress($address))->getLocalPart(); + + $matches = null; + if (!preg_match($regexp, $local, $matches)) { + return false; + } + + return $matches; + } + private function getAddressRegexp() { $pattern = $this->getObjectPattern(); diff --git a/src/applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php b/src/applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php new file mode 100644 index 0000000000..31ad44f974 --- /dev/null +++ b/src/applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php @@ -0,0 +1,128 @@ + true, + ); + } + + public function testDropUnconfiguredPublicMail() { + list($task, $user, $mail) = $this->buildMail('public'); + + $env = PhabricatorEnv::beginScopedEnv(); + $env->overrideEnvConfig('metamta.public-replies', false); + + $mail->save(); + $mail->processReceivedMail(); + + $this->assertEqual( + MetaMTAReceivedMailStatus::STATUS_NO_PUBLIC_MAIL, + $mail->getStatus()); + } + +/* + + TODO: Tasks don't support policies yet. Implement this once they do. + + public function testDropPolicyViolationMail() { + list($task, $user, $mail) = $this->buildMail('public'); + + // TODO: Set task policy to "no one" here. + + $mail->save(); + $mail->processReceivedMail(); + + $this->assertEqual( + MetaMTAReceivedMailStatus::STATUS_POLICY_PROBLEM, + $mail->getStatus()); + } + +*/ + + public function testDropInvalidObjectMail() { + list($task, $user, $mail) = $this->buildMail('404'); + + $mail->save(); + $mail->processReceivedMail(); + + $this->assertEqual( + MetaMTAReceivedMailStatus::STATUS_NO_SUCH_OBJECT, + $mail->getStatus()); + } + + public function testDropUserMismatchMail() { + list($task, $user, $mail) = $this->buildMail('baduser'); + + $mail->save(); + $mail->processReceivedMail(); + + $this->assertEqual( + MetaMTAReceivedMailStatus::STATUS_USER_MISMATCH, + $mail->getStatus()); + } + + public function testDropHashMismatchMail() { + list($task, $user, $mail) = $this->buildMail('badhash'); + + $mail->save(); + $mail->processReceivedMail(); + + $this->assertEqual( + MetaMTAReceivedMailStatus::STATUS_HASH_MISMATCH, + $mail->getStatus()); + } + + private function buildMail($style) { + + // TODO: Clean up test data generators so that we don't need to guarantee + // the existence of a user. + $this->generateNewTestUser(); + + $task = id(new PhabricatorManiphestTaskTestDataGenerator())->generate(); + $user = $this->generateNewTestUser(); + + $is_public = ($style === 'public'); + $is_bad_hash = ($style == 'badhash'); + $is_bad_user = ($style == 'baduser'); + $is_404_object = ($style == '404'); + + if ($is_public) { + $user_identifier = 'public'; + } else if ($is_bad_user) { + $user_identifier = $user->getID() + 1; + } else { + $user_identifier = $user->getID(); + } + + if ($is_bad_hash) { + $hash = PhabricatorObjectMailReceiver::computeMailHash('x', 'y'); + } else { + $hash = PhabricatorObjectMailReceiver::computeMailHash( + $task->getMailKey(), + $is_public ? $task->getPHID() : $user->getPHID()); + } + + if ($is_404_object) { + $task_identifier = 'T'.($task->getID() + 1); + } else { + $task_identifier = 'T'.$task->getID(); + } + + $to = $task_identifier.'+'.$user_identifier.'+'.$hash.'@example.com'; + + $mail = new PhabricatorMetaMTAReceivedMail(); + $mail->setHeaders( + array( + 'Message-ID' => 'test@example.com', + 'From' => $user->loadPrimaryEmail()->getAddress(), + 'To' => $to, + )); + + return array($task, $user, $mail); + } + + +} diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php index ff1ec8f631..e4e360fc30 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php @@ -176,6 +176,7 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { $receiver = $this->loadReceiver(); $sender = $receiver->loadSender($this); + $receiver->validateSender($this, $sender); } catch (PhabricatorMetaMTAReceivedMailProcessingException $ex) { $this @@ -197,62 +198,11 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { $from = idx($this->headers, 'from'); - // TODO -- make this a switch statement / better if / when we add more - // public create email addresses! - $create_task = PhabricatorEnv::getEnvConfig( - 'metamta.maniphest.public-create-email'); - - if ($create_task && $to == $create_task) { + // TODO: Move this into `ManiphestCreateMailReceiver`. + if ($receiver instanceof ManiphestCreateMailReceiver) { $receiver = new ManiphestTask(); - $user = $this->lookupSender(); - if ($user) { - $this->setAuthorPHID($user->getPHID()); - } else { - $allow_email_users = PhabricatorEnv::getEnvConfig( - 'phabricator.allow-email-users'); - - if ($allow_email_users) { - $email = new PhutilEmailAddress($from); - - $xuser = id(new PhabricatorExternalAccount())->loadOneWhere( - 'accountType = %s AND accountDomain IS NULL and accountID = %s', - 'email', - $email->getAddress()); - - if (!$xuser) { - $xuser = new PhabricatorExternalAccount(); - $xuser->setAccountID($email->getAddress()); - $xuser->setAccountType('email'); - $xuser->setDisplayName($email->getDisplayName()); - $xuser->save(); - } - - $user = $xuser->getPhabricatorUser(); - } else { - $default_author = PhabricatorEnv::getEnvConfig( - 'metamta.maniphest.default-public-author'); - - if ($default_author) { - $user = id(new PhabricatorUser())->loadOneWhere( - 'username = %s', - $default_author); - - if (!$user) { - throw new Exception( - "Phabricator is misconfigured, the configuration key ". - "'metamta.maniphest.default-public-author' is set to user ". - "'{$default_author}' but that user does not exist."); - } - - } else { - // TODO: We should probably bounce these since from the user's - // perspective their email vanishes into a black hole. - return $this->setMessage("Invalid public user '{$from}'.")->save(); - } - } - - } + $user = $sender; $receiver->setAuthorPHID($user->getPHID()); $receiver->setOriginalEmailSource($from); @@ -275,11 +225,7 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { // means we're creating a conpherence...! if ($user_phids) { - // we must have a valid user who created this conpherence - $user = $this->lookupSender(); - if (!$user) { - return $this->setMessage("Invalid public user '{$from}'.")->save(); - } + $user = $sender; $conpherence = id(new ConpherenceReplyHandler()) ->setMailReceiver(new ConpherenceThread()) @@ -293,31 +239,7 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { return $this->save(); } - if ($user_id == 'public') { - if (!PhabricatorEnv::getEnvConfig('metamta.public-replies')) { - return $this->setMessage("Public replies not enabled.")->save(); - } - - $user = $this->lookupSender(); - - if (!$user) { - return $this->setMessage("Invalid public user '{$from}'.")->save(); - } - - $use_user_hash = false; - } else { - $user = id(new PhabricatorUser())->load($user_id); - if (!$user) { - return $this->setMessage("Invalid private user '{$user_id}'.")->save(); - } - - $use_user_hash = true; - } - - if ($user->getIsDisabled()) { - return $this->setMessage("User '{$user_id}' is disabled")->save(); - } - + $user = $sender; $this->setAuthorPHID($user->getPHID()); $receiver = self::loadReceiverObject($receiver_name); @@ -327,24 +249,6 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { $this->setRelatedPHID($receiver->getPHID()); - if ($use_user_hash) { - // This is a private reply-to address, check that the user hash is - // correct. - $check_phid = $user->getPHID(); - } else { - // This is a public reply-to address, check that the object hash is - // correct. - $check_phid = $receiver->getPHID(); - } - - $expect_hash = PhabricatorObjectMailReceiver::computeMailHash( - $receiver->getMailKey(), - $check_phid); - - if ($expect_hash != $hash) { - return $this->setMessage("Invalid mail hash!")->save(); - } - if ($receiver instanceof ManiphestTask) { $editor = new ManiphestTransactionEditor(); $editor->setActor($user); @@ -431,39 +335,6 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { return array_filter($raw_addresses); } - private function lookupSender() { - $from = idx($this->headers, 'from'); - $from = $this->getRawEmailAddress($from); - - $user = PhabricatorUser::loadOneWithEmailAddress($from); - - // If Phabricator is configured to allow "Reply-To" authentication, try - // the "Reply-To" address if we failed to match the "From" address. - $config_key = 'metamta.insecure-auth-with-reply-to'; - $allow_reply_to = PhabricatorEnv::getEnvConfig($config_key); - - if (!$user && $allow_reply_to) { - $reply_to = idx($this->headers, 'reply-to'); - $reply_to = $this->getRawEmailAddress($reply_to); - if ($reply_to) { - $user = PhabricatorUser::loadOneWithEmailAddress($reply_to); - } - } - - $allow_email_users = PhabricatorEnv::getEnvConfig( - 'phabricator.allow-email-users'); - - if (!$user && $allow_email_users) { - $xusr = id(new PhabricatorExternalAccount())->loadOneWhere( - 'accountType = %s AND accountDomain IS NULL and accountID = %s', - 'email', $from); - - $user = $xusr->getPhabricatorUser(); - } - - 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 diff --git a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php index 1e1be9c566..8f3e72a342 100644 --- a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php +++ b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php @@ -87,4 +87,31 @@ final class PhabricatorMetaMTAReceivedMailTestCase extends PhabricatorTestCase { $mail->getStatus()); } + + public function testDropDisabledSenderMail() { + $env = PhabricatorEnv::beginScopedEnv(); + $env->overrideEnvConfig( + 'metamta.maniphest.public-create-email', + 'bugs@example.com'); + + $user = $this->generateNewTestUser() + ->setIsDisabled(true) + ->save(); + + $mail = new PhabricatorMetaMTAReceivedMail(); + $mail->setHeaders( + array( + 'Message-ID' => 'test@example.com', + 'From' => $user->loadPrimaryEmail()->getAddress(), + 'To' => 'bugs@example.com', + )); + $mail->save(); + + $mail->processReceivedMail(); + + $this->assertEqual( + MetaMTAReceivedMailStatus::STATUS_DISABLED_SENDER, + $mail->getStatus()); + } + }