1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-09-20 01:08:50 +02:00

Don't bounce mail messages if any recipient was reserved

Summary:
Ref T13222. If we receive a message and nothing processes it, we normally try to send the user an error message like "hey, nothing handled this, maybe you got the address wrong".

Just skip the "send them an error message" part if any recipient was reserved, so if you "Reply All" to a message that is "From: noreply@phabricator" you don't get a relatively unhelpful error.

This also makes sure that the "void" address doesn't generate bounces if the "From" is a valid user email address (e.g., with `metamta.can-send-as-user`). That is:

  - Phabricator needs to send a mail with only "CC" users.
  - Phabricator puts the "void" address in "To" as a placeholder.
  - The "void" address happens to route back to Phabricator.

We don't want that mail to bounce to anywhere. Normally, it won't:

  - From is usually "noreply@phabricator", which isn't a user, so we won't send anything back: we only send mail to verified user email addresses.
  - The message will have "X-Phabricator-Sent-This-Message: true" so we won't process it at all.

...but this is another layer of certainty.

Test Plan: Used `bin/mail receive-test` to receive mail to an invalid, unreserved address (bounce/error email) and an invalid, reserved address (no bounce/error email).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19987
This commit is contained in:
epriestley 2019-01-17 06:38:11 -08:00
parent 6b6c991ad4
commit ff220acae6
2 changed files with 29 additions and 1 deletions

View file

@ -16,6 +16,7 @@ final class MetaMTAReceivedMailStatus
const STATUS_UNHANDLED_EXCEPTION = 'err:exception';
const STATUS_EMPTY = 'err:empty';
const STATUS_EMPTY_IGNORED = 'err:empty-ignored';
const STATUS_RESERVED = 'err:reserved-recipient';
public static function getHumanReadableName($status) {
$map = array(
@ -32,6 +33,7 @@ final class MetaMTAReceivedMailStatus
self::STATUS_UNHANDLED_EXCEPTION => pht('Unhandled Exception'),
self::STATUS_EMPTY => pht('Empty Mail'),
self::STATUS_EMPTY_IGNORED => pht('Ignored Empty Mail'),
self::STATUS_RESERVED => pht('Reserved Recipient'),
);
return idx($map, $status, pht('Processing Exception'));

View file

@ -161,12 +161,16 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
->setFilterMethod('isEnabled')
->execute();
$reserved_recipient = null;
$targets = $this->newTargetAddresses();
foreach ($targets as $key => $target) {
// Never accept any reserved address as a mail target. This prevents
// security issues around "hostmaster@" and bad behavior with
// "noreply@".
if (PhabricatorMailUtil::isReservedAddress($target)) {
if (!$reserved_recipient) {
$reserved_recipient = $target;
}
unset($targets[$key]);
continue;
}
@ -212,8 +216,26 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
throw $receiver_exception;
}
if (!$any_accepted) {
if (!$sender) {
if ($reserved_recipient) {
// If nothing accepted the mail, we normally raise an error to help
// users who mistakenly send mail to "barges@" instead of "bugs@".
// However, if the recipient list included a reserved recipient, we
// don't bounce the mail with an error.
// The intent here is that if a user does a "Reply All" and includes
// "From: noreply@phabricator" in the receipient list, we just want
// to drop the mail rather than send them an unhelpful bounce message.
throw new PhabricatorMetaMTAReceivedMailProcessingException(
MetaMTAReceivedMailStatus::STATUS_RESERVED,
pht(
'No application handled this mail. This mail was sent to a '.
'reserved recipient ("%s") so bounces are suppressed.',
(string)$reserved_recipient));
} else if (!$sender) {
// NOTE: Currently, we'll always drop this mail (since it's headed to
// an unverified recipient). See T12237. These details are still
// useful because they'll appear in the mail logs and Mail web UI.
@ -244,6 +266,10 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
// Don't send an error email back in these cases, since they're
// very unlikely to be the sender's fault.
break;
case MetaMTAReceivedMailStatus::STATUS_RESERVED:
// This probably is the sender's fault, but it's likely an accident
// that we received the mail at all.
break;
case MetaMTAReceivedMailStatus::STATUS_EMPTY_IGNORED:
// This error is explicitly ignored.
break;