revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-09-20 09:18:48 +02:00
Code Issues Releases Wiki Activity

When we fail to process mail, tell the user about it

Browse source 
Summary:
Ref T4371. Ref T4699. Fixes T3994.

Currently, we're very conservative about sending errors back to users. A concern I had about this was that mistakes could lead to email loops, massive amounts of email spam, etc. Because of this, I was pretty hesitant about replying to email with more email when I wrote this stuff.

However, this was a long time ago. We now have Message-ID deduplication, "X-Phabricator-Sent-This-Mail", generally better mail infrastructure, and rate limiting. Together, these mechanisms should reasonably prevent anything crazy (primarily, infinite email loops) from happening.

Thus:

  - When we hit any processing error after receiving a mail, try to send the author a reply with details about what went wrong. These are limited to 6 per hour per address.
  - Rewrite most of the errors to be more detailed and informative.
  - Rewrite most of the errors in a user-facing voice ("You sent this mail..." instead of "This mail was sent..").
  - Remove the redundant, less sophisticated code which does something similar in Differential.

Test Plan:
  - Using `scripts/mail/mail_receiver.php`, artificially received a pile of mail.
  - Hit a bunch of different errors.
  - Saw reasonable error mail get sent to me.
  - Saw other reasonable error mail get rate limited.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3994, T4371, T4699

Differential Revision: https://secure.phabricator.com/D8692
This commit is contained in:
epriestley 2014-04-03 18:43:18 -07:00
parent f9a92c7631
commit d9cdbdb9fa
12 changed files with 234 additions and 125 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
src/__phutil_library_map__.php
View file

@ -371,7 +371,6 @@ phutil_register_library_map(array(
'DifferentialDoorkeeperRevisionFeedStoryPublisher' => 'applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php',
'DifferentialDraft' => 'applications/differential/storage/DifferentialDraft.php',
'DifferentialEditPolicyField' => 'applications/differential/customfield/DifferentialEditPolicyField.php',
'DifferentialExceptionMail' => 'applications/differential/mail/DifferentialExceptionMail.php',
'DifferentialFieldParseException' => 'applications/differential/exception/DifferentialFieldParseException.php',
'DifferentialFieldValidationException' => 'applications/differential/exception/DifferentialFieldValidationException.php',
'DifferentialGetWorkingCopy' => 'applications/differential/DifferentialGetWorkingCopy.php',
@ -398,7 +397,6 @@ phutil_register_library_map(array(
'DifferentialLintStatus' => 'applications/differential/constants/DifferentialLintStatus.php',
'DifferentialLocalCommitsView' => 'applications/differential/view/DifferentialLocalCommitsView.php',
'DifferentialMail' => 'applications/differential/mail/DifferentialMail.php',
'DifferentialMailPhase' => 'applications/differential/constants/DifferentialMailPhase.php',
'DifferentialManiphestTasksField' => 'applications/differential/customfield/DifferentialManiphestTasksField.php',
'DifferentialPHIDTypeDiff' => 'applications/differential/phid/DifferentialPHIDTypeDiff.php',
'DifferentialPHIDTypeRevision' => 'applications/differential/phid/DifferentialPHIDTypeRevision.php',
@ -1693,6 +1691,7 @@ phutil_register_library_map(array(
'PhabricatorMetaMTADAO' => 'applications/metamta/storage/PhabricatorMetaMTADAO.php',
'PhabricatorMetaMTAEmailBodyParser' => 'applications/metamta/parser/PhabricatorMetaMTAEmailBodyParser.php',
'PhabricatorMetaMTAEmailBodyParserTestCase' => 'applications/metamta/parser/__tests__/PhabricatorMetaMTAEmailBodyParserTestCase.php',
'PhabricatorMetaMTAErrorMailAction' => 'applications/metamta/action/PhabricatorMetaMTAErrorMailAction.php',
'PhabricatorMetaMTAMail' => 'applications/metamta/storage/PhabricatorMetaMTAMail.php',
'PhabricatorMetaMTAMailBody' => 'applications/metamta/view/PhabricatorMetaMTAMailBody.php',
'PhabricatorMetaMTAMailBodyTestCase' => 'applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php',
@ -2952,7 +2951,6 @@ phutil_register_library_map(array(
'DifferentialDoorkeeperRevisionFeedStoryPublisher' => 'DoorkeeperFeedStoryPublisher',
'DifferentialDraft' => 'DifferentialDAO',
'DifferentialEditPolicyField' => 'DifferentialCoreCustomField',
'DifferentialExceptionMail' => 'DifferentialMail',
'DifferentialFieldParseException' => 'Exception',
'DifferentialFieldValidationException' => 'Exception',
'DifferentialGitSVNIDField' => 'DifferentialCustomField',
@ -4487,6 +4485,7 @@ phutil_register_library_map(array(
'PhabricatorMetaMTAController' => 'PhabricatorController',
'PhabricatorMetaMTADAO' => 'PhabricatorLiskDAO',
'PhabricatorMetaMTAEmailBodyParserTestCase' => 'PhabricatorTestCase',
'PhabricatorMetaMTAErrorMailAction' => 'PhabricatorSystemAction',
'PhabricatorMetaMTAMail' => 'PhabricatorMetaMTADAO',
'PhabricatorMetaMTAMailBodyTestCase' => 'PhabricatorTestCase',
'PhabricatorMetaMTAMailTestCase' => 'PhabricatorTestCase',

9
src/applications/differential/constants/DifferentialMailPhase.php
View file

@ -1,9 +0,0 @@
<?php
final class DifferentialMailPhase {
const WELCOME = 1;
const UPDATE = 2;
const COMMENT = 3;
}

45
src/applications/differential/mail/DifferentialExceptionMail.php
View file

@ -1,45 +0,0 @@
<?php
final class DifferentialExceptionMail extends DifferentialMail {
public function __construct(
DifferentialRevision $revision,
Exception $exception,
$original_body) {
$this->revision = $revision;
$this->exception = $exception;
$this->originalBody = $original_body;
}
protected function renderBody() {
// Never called since buildBody() is overridden.
}
protected function renderSubject() {
return "Exception: unable to process your mail request";
}
protected function renderVaryPrefix() {
return '';
}
protected function buildBody() {
$exception = $this->exception;
$original_body = $this->originalBody;
$message = $exception->getMessage();
return <<<EOBODY
Your request failed because an exception was encoutered while processing it:
EXCEPTION: {$message}
-- Original Body -------------------------------------------------------------
{$original_body}
EOBODY;
}
}

20
src/applications/differential/mail/DifferentialReplyHandler.php
View file

@ -169,25 +169,7 @@ class DifferentialReplyHandler extends PhabricatorMailReplyHandler {
$editor->setContentSource($content_source);
}
try {
$editor->applyTransactions($this->getMailReceiver(), $xactions);
} catch (Exception $ex) {
if ($this->receivedMail) {
$error_body = $this->receivedMail->getRawTextBody();
} else {
$error_body = $body;
}
$exception_mail = new DifferentialExceptionMail(
$this->getMailReceiver(),
$ex,
$error_body);
$exception_mail->setActor($this->getActor());
$exception_mail->setToPHIDs(array($this->getActor()->getPHID()));
$exception_mail->send();
throw $ex;
}
$editor->applyTransactions($this->getMailReceiver(), $xactions);
}
}

13
src/applications/metamta/action/PhabricatorMetaMTAErrorMailAction.php Normal file
View file

@ -0,0 +1,13 @@
<?php
final class PhabricatorMetaMTAErrorMailAction extends PhabricatorSystemAction {
public function getActionConstant() {
return 'email.error';
}
public function getScoreThreshold() {
return 6 / phutil_units('1 hour in seconds');
}
}

19
src/applications/metamta/constants/MetaMTAReceivedMailStatus.php
View file

@ -16,4 +16,23 @@ final class MetaMTAReceivedMailStatus
const STATUS_HASH_MISMATCH = 'err:bad-hash';
const STATUS_UNHANDLED_EXCEPTION = 'err:exception';
public static function getHumanReadableName($status) {
$map = array(
self::STATUS_DUPLICATE => pht('Duplicate Message'),
self::STATUS_FROM_PHABRICATOR => pht('Phabricator Mail'),
self::STATUS_NO_RECEIVERS => pht('No Receivers'),
self::STATUS_ABUNDANT_RECEIVERS => pht('Multiple Receivers'),
self::STATUS_UNKNOWN_SENDER => pht('Unknown Sender'),
self::STATUS_DISABLED_SENDER => pht('Disabled Sender'),
self::STATUS_NO_PUBLIC_MAIL => pht('No Public Mail'),
self::STATUS_USER_MISMATCH => pht('User Mismatch'),
self::STATUS_POLICY_PROBLEM => pht('Policy Error'),
self::STATUS_NO_SUCH_OBJECT => pht('No Such Object'),
self::STATUS_HASH_MISMATCH => pht('Bad Address'),
self::STATUS_UNHANDLED_EXCEPTION => pht('Unhandled Exception'),
);
return idx($map, $status, pht('Processing Exception'));
}
}

83
src/applications/metamta/receiver/PhabricatorMailReceiver.php
View file

@ -19,12 +19,32 @@ abstract class PhabricatorMailReceiver {
PhabricatorMetaMTAReceivedMail $mail,
PhabricatorUser $sender) {
if (!$sender->isUserActivated()) {
$failure_reason = null;
if ($sender->getIsDisabled()) {
$failure_reason = pht(
'Your account (%s) is disabled, so you can not interact with '.
'Phabricator over email.',
$sender->getUsername());
} else if ($sender->getIsStandardUser()) {
if (!$sender->getIsApproved()) {
$failure_reason = pht(
'Your account (%s) has not been approved yet. You can not interact '.
'with Phabricator over email until your account is approved.',
$sender->getUsername());
} else if (PhabricatorUserEmail::isEmailVerificationRequired() &&
!$sender->getIsEmailVerified()) {
$failure_reason = pht(
'You have not verified the email address for your account (%s). '.
'You must verify your email address before you can interact '.
'with Phabricator over email.',
$sender->getUsername());
}
}
if ($failure_reason) {
throw new PhabricatorMetaMTAReceivedMailProcessingException(
MetaMTAReceivedMailStatus::STATUS_DISABLED_SENDER,
pht(
"Sender '%s' does not have an activated user account.",
$sender->getUsername()));
$failure_reason);
}
}
@ -46,33 +66,34 @@ abstract class PhabricatorMailReceiver {
return $user;
} else {
$reasons[] = pht(
"The email was sent from '%s', but this address does not correspond ".
"to any user account.",
'This email was sent from "%s", but that address is not recognized by '.
'Phabricator and does not correspond to any known user account.',
$raw_from);
}
// If we missed on "From", try "Reply-To" if we're configured for it.
$reply_to_key = 'metamta.insecure-auth-with-reply-to';
$allow_reply_to = PhabricatorEnv::getEnvConfig($reply_to_key);
if ($allow_reply_to) {
$raw_reply_to = $mail->getHeader('Reply-To');
$reply_to = self::getRawAddress($raw_reply_to);
$raw_reply_to = $mail->getHeader('Reply-To');
if (strlen($raw_reply_to)) {
$reply_to_key = 'metamta.insecure-auth-with-reply-to';
$allow_reply_to = PhabricatorEnv::getEnvConfig($reply_to_key);
if ($allow_reply_to) {
$reply_to = self::getRawAddress($raw_reply_to);
$user = PhabricatorUser::loadOneWithEmailAddress($reply_to);
if ($user) {
return $user;
$user = PhabricatorUser::loadOneWithEmailAddress($reply_to);
if ($user) {
return $user;
} else {
$reasons[] = pht(
'Phabricator is configured to authenticate users using the '.
'"Reply-To" header, but the reply address ("%s") on this '.
'message does not correspond to any known user account.',
$raw_reply_to);
}
} else {
$reasons[] = pht(
"Phabricator is configured to try to authenticate users using ".
"'Reply-To', but the reply to address ('%s') does not correspond ".
"to any user account.",
$raw_reply_to);
'(Phabricator is not configured to authenticate users using the '.
'"Reply-To" header, so it was ignored.)');
}
} else {
$reasons[] = pht(
"Phabricator is not configured to authenticate users using ".
"'Reply-To' (`metamta.insecure-auth-with-reply-to`), so the ".
"'Reply-To' header was not examined.");
}
// If we don't know who this user is, load or create an external user
@ -82,7 +103,7 @@ abstract class PhabricatorMailReceiver {
if ($allow_email_users) {
$from_obj = new PhutilEmailAddress($from);
$xuser = id(new PhabricatorExternalAccountQuery())
->setViewer($user)
->setViewer(PhabricatorUser::getOmnipotentUser())
->withAccountTypes(array('email'))
->withAccountDomains(array($from_obj->getDomainName(), 'self'))
->withAccountIDs(array($from_obj->getAddress()))
@ -90,15 +111,19 @@ abstract class PhabricatorMailReceiver {
return $xuser->getPhabricatorUser();
} else {
$reasons[] = pht(
"Phabricator is not configured to allow unknown external users to ".
"send mail to the system using just an email address ".
"(`phabricator.allow-email-users`), so an implicit external acount ".
"could not be created.");
'Phabricator is also not configured to allow unknown external users '.
'to send mail to the system using just an email address.');
$reasons[] = pht(
'To interact with Phabricator, add this address ("%s") to your '.
'account.',
$raw_from);
}
$reasons = implode("\n\n", $reasons);
throw new PhabricatorMetaMTAReceivedMailProcessingException(
MetaMTAReceivedMailStatus::STATUS_UNKNOWN_SENDER,
pht('Unknown sender: %s', implode(' ', $reasons)));
$reasons);
}
/**

35
src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php
View file

@ -55,6 +55,7 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver {
parent::validateSender($mail, $sender);
$parts = $this->matchObjectAddressInMail($mail);
$pattern = $parts['pattern'];
try {
$object = $this->loadObjectFromMail($mail, $sender);
@ -62,8 +63,9 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver {
throw new PhabricatorMetaMTAReceivedMailProcessingException(
MetaMTAReceivedMailStatus::STATUS_POLICY_PROBLEM,
pht(
"This mail is addressed to an object you are not permitted ".
"to see: %s",
'This mail is addressed to an object ("%s") you do not have '.
'permission to see: %s',
$pattern,
$policy_exception->getMessage()));
}
@ -71,9 +73,9 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver {
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']));
'This mail is addressed to an object ("%s"), but that object '.
'does not exist.',
$pattern));
}
$sender_identifier = $parts['sender'];
@ -83,8 +85,12 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver {
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`)."));
'This mail is addressed to the public email address of an object '.
'("%s"), but public replies are not enabled on this Phabricator '.
'install. An administrator may have recently disabled this '.
'setting, or you may have replied to an old message. Try '.
'replying to a more recent message instead.',
$pattern));
}
$check_phid = $object->getPHID();
} else {
@ -92,9 +98,12 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver {
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."));
'This mail is addressed to the private email address of an object '.
'("%s"), but you are not the user who is authorized to use the '.
'address you sent mail to. Each private address is unique to the '.
'user who received the original mail. Try replying to a message '.
'which was sent directly to you instead.',
$pattern));
}
$check_phid = $sender->getPHID();
}
@ -105,8 +114,10 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver {
throw new PhabricatorMetaMTAReceivedMailProcessingException(
MetaMTAReceivedMailStatus::STATUS_HASH_MISMATCH,
pht(
"The hash in this object's address does not match the expected ".
"value."));
'This mail is addressed to an object ("%s"), but the address is '.
'not correct (the security hash is wrong). Check that the address '.
'is correct.',
$pattern));
}
}

34
src/applications/metamta/storage/PhabricatorMetaMTAMail.php
View file

@ -222,6 +222,15 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
return $this;
}
public function setIsErrorEmail($is_error) {
$this->setParam('is-error', $is_error);
return $this;
}
public function getIsErrorEmail() {
return $this->getParam('is-error', false);
}
public function getWorkerTaskID() {
return $this->getParam('worker-task');
}
@ -549,6 +558,19 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
return $this->save();
}
if ($this->getIsErrorEmail()) {
$all_recipients = array_merge($add_to, $add_cc);
if ($this->shouldRateLimitMail($all_recipients)) {
$this->setStatus(self::STATUS_VOID);
$this->setMessage(
pht(
'This is an error email, but one or more recipients have '.
'exceeded the error email rate limit. Declining to deliver '.
'message.'));
return $this->save();
}
}
$mailer->addHeader('X-Phabricator-Sent-This-Message', 'Yes');
$mailer->addHeader('X-Mail-Transport-Agent', 'MetaMTA');
@ -839,5 +861,17 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
return $actors;
}
private function shouldRateLimitMail(array $all_recipients) {
try {
PhabricatorSystemActionEngine::willTakeAction(
$all_recipients,
new PhabricatorMetaMTAErrorMailAction(),
1);
return false;
} catch (PhabricatorSystemActionRateLimitException $ex) {
return true;
}
}
}

80
src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php
View file

@ -102,12 +102,25 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
$receiver->receiveMail($this, $sender);
} catch (PhabricatorMetaMTAReceivedMailProcessingException $ex) {
switch ($ex->getStatusCode()) {
case MetaMTAReceivedMailStatus::STATUS_DUPLICATE:
case MetaMTAReceivedMailStatus::STATUS_FROM_PHABRICATOR:
// Don't send an error email back in these cases, since they're
// very unlikely to be the sender's fault.
break;
default:
$this->sendExceptionMail($ex);
break;
}
$this
->setStatus($ex->getStatusCode())
->setMessage($ex->getMessage())
->save();
return $this;
} catch (Exception $ex) {
$this->sendExceptionMail($ex);
$this
->setStatus(MetaMTAReceivedMailStatus::STATUS_UNHANDLED_EXCEPTION)
->setMessage(pht('Unhandled Exception: %s', $ex->getMessage()))
@ -169,8 +182,9 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
throw new PhabricatorMetaMTAReceivedMailProcessingException(
MetaMTAReceivedMailStatus::STATUS_FROM_PHABRICATOR,
"Ignoring email with 'X-Phabricator-Sent-This-Message' header to avoid ".
"loops.");
pht(
"Ignoring email with 'X-Phabricator-Sent-This-Message' header to ".
"avoid loops."));
}
/**
@ -203,8 +217,8 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
return;
}
$message = sprintf(
'Ignoring email with message id hash "%s" that has been seen %d '.
$message = pht(
'Ignoring email with "Message-ID" hash "%s" that has been seen %d '.
'times, including this message.',
$message_id_hash,
$messages_count);
@ -237,18 +251,70 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
if (!$accept) {
throw new PhabricatorMetaMTAReceivedMailProcessingException(
MetaMTAReceivedMailStatus::STATUS_NO_RECEIVERS,
"No concrete, enabled subclasses of `PhabricatorMailReceiver` can ".
"accept this mail.");
pht(
'Phabricator can not process this mail because no application '.
'knows how to handle it. Check that the address you sent it to is '.
'correct.'.
"\n\n".
'(No concrete, enabled subclass of PhabricatorMailReceiver can '.
'accept this mail.)'));
}
if (count($accept) > 1) {
$names = implode(', ', array_keys($accept));
throw new PhabricatorMetaMTAReceivedMailProcessingException(
MetaMTAReceivedMailStatus::STATUS_ABUNDANT_RECEIVERS,
"More than one `PhabricatorMailReceiver` claims to accept this mail.");
pht(
'Phabricator is not able to process this mail because more than '.
'one application is willing to accept it, creating ambiguity. '.
'Mail needs to be accepted by exactly one receiving application.'.
"\n\n".
'Accepting receivers: %s.',
$names));
}
return head($accept);
}
private function sendExceptionMail(Exception $ex) {
$from = $this->getHeader('from');
if (!strlen($from)) {
return;
}
if ($ex instanceof PhabricatorMetaMTAReceivedMailProcessingException) {
$status_code = $ex->getStatusCode();
$status_name = MetaMTAReceivedMailStatus::getHumanReadableName(
$status_code);
$title = pht('Error Processing Mail (%s)', $status_name);
$description = $ex->getMessage();
} else {
$title = pht('Error Processing Mail (%s)', get_class($ex));
$description = pht('%s: %s', get_class($ex), $ex->getMessage());
}
$body = pht(<<<EOBODY
Your email to Phabricator was not processed, because an error occurred while
trying to handle it:
%s
-- Original Message Body -----------------------------------------------------
%s
EOBODY
,
wordwrap($description, 78),
$this->getRawTextBody());
$mail = id(new PhabricatorMetaMTAMail())
->setIsErrorEmail(true)
->setSubject($title)
->addRawTos(array($from))
->setBody($body)
->saveAndSend();
}
}

12
src/applications/people/storage/PhabricatorUser.php
View file

@ -90,6 +90,18 @@ final class PhabricatorUser
return true;
}
/**
* Returns `true` if this is a standard user who is logged in. Returns `false`
* for logged out, anonymous, or external users.
*
* @return bool `true` if the user is a standard user who is logged in with
* a normal session.
*/
public function getIsStandardUser() {
$type_user = PhabricatorPeoplePHIDTypeUser::TYPECONST;
return $this->getPHID() && (phid_get_type($this->getPHID()) == $type_user);
}
public function getConfiguration() {
return array(
self::CONFIG_AUX_PHID => true,

4
src/applications/system/engine/PhabricatorSystemActionEngine.php
View file

@ -20,7 +20,9 @@ final class PhabricatorSystemActionEngine extends Phobject {
}
}
self::recordAction($actors, $action, $score);
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
self::recordAction($actors, $action, $score);
unset($unguarded);
}
public static function loadBlockedActors(