Don't count "Cc: x@y.com" as a legitimate recipient if the user who has "x@y.com" attached to their account has not verified the address

Summary:
Fixes T13317. On `admin.phacility.com`, an enterprising user added `noreply@admin.phacility.com` to their account. This caused them to become CC'd on several support issues over the last year, because we send mail "From" this address and it can get CC'd via reply/reply all/whatever else.

The original driving goal here is that if I reply to a task email and CC you on my reply, that should count as a CC in Phabricator, since this aligns with user intent and keeps them in the loop.

This misfire on `noreply@` is ultimately harmless (being CC'd does not grant the user access permission, see T4411), but confusing and undesirable. Instead:

  - Don't allow reserved addresses ("noreply@", "ssladmin@", etc) to trigger this subscribe-via-CC behavior.
  - Only count verified addresses as legitimate user recipients.

Test Plan:
  - Added a `bin/mail receive-test --cc ...` flag to make this easier to test.
  - Sent mail as `bin/mail receive-test --to X --as alice --cc bailey@verified.com`. Bailey was CC'd both before and after the change.
  - Sent mail as `bin/mail receive-test --to X --as alice --cc unverified@imaginary.com`, an address which Bailey has added to her account but not verified.
    - Before change: Bailey was CC'd on the task anyway.
    - After change: Bailey is not CC'd on the task.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13317

Differential Revision: https://secure.phabricator.com/D20593

src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php

@@ -29,6 +29,12 @@ final class PhabricatorMailManagementReceiveTestWorkflow
             'param'   => 'object',
             'help'    => pht('Simulate mail delivery "To:" the given object.'),
           ),
+          array(
+            'name' => 'cc',
+            'param' => 'address',
+            'help' => pht('Simulate a mail delivery "Cc:" address.'),
+            'repeat' => true,
+          ),
         ));
   }
 
@@ -84,6 +90,8 @@ final class PhabricatorMailManagementReceiveTestWorkflow
       $from = $user->loadPrimaryEmail()->getAddress();
     }
 
+    $cc = $args->getArg('cc');
+
     $console->writeErr("%s\n", pht('Reading message body from stdin...'));
     $body = file_get_contents('php://stdin');
 
@@ -91,6 +99,7 @@ final class PhabricatorMailManagementReceiveTestWorkflow
     $header_content = array(
       'Message-ID' => Filesystem::readRandomCharacters(12),
       'From'       => $from,
+      'Cc' => implode(', ', $cc),
     );
 
     if (preg_match('/.+@.+/', $to)) {

src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php

@@ -104,24 +104,43 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
   }
 
   public function loadAllRecipientPHIDs() {
-    $addresses = array_merge(
-      $this->getToAddresses(),
-      $this->getCCAddresses());
+    $addresses = $this->newTargetAddresses();
 
-    return $this->loadPHIDsFromAddresses($addresses);
-  }
+    // See T13317. Don't allow reserved addresses (like "noreply@...") to
+    // match user PHIDs.
+    foreach ($addresses as $key => $address) {
+      if (PhabricatorMailUtil::isReservedAddress($address)) {
+        unset($addresses[$key]);
+      }
+    }
 
-  public function loadCCPHIDs() {
-    return $this->loadPHIDsFromAddresses($this->getCCAddresses());
-  }
-
-  private function loadPHIDsFromAddresses(array $addresses) {
-    if (empty($addresses)) {
+    if (!$addresses) {
       return array();
     }
-    $users = id(new PhabricatorUserEmail())
-      ->loadAllWhere('address IN (%Ls)', $addresses);
-    return mpull($users, 'getUserPHID');
+
+    $address_strings = array();
+    foreach ($addresses as $address) {
+      $address_strings[] = phutil_string_cast($address->getAddress());
+    }
+
+    // See T13317. If a verified email address is in the "To" or "Cc" line,
+    // we'll count the user who owns that address as a recipient.
+
+    // We require the address be verified because we'll trigger behavior (like
+    // adding subscribers) based on the recipient list, and don't want to add
+    // Alice as a subscriber if she adds an unverified "internal-bounces@"
+    // address to her account and this address gets caught in the crossfire.
+    // In the best case this is confusing; in the worst case it could
+    // some day give her access to objects she can't see.
+
+    $recipients = id(new PhabricatorUserEmail())
+      ->loadAllWhere(
+        'address IN (%Ls) AND isVerified = 1',
+        $address_strings);
+
+    $recipient_phids = mpull($recipients, 'getUserPHID');
+
+    return $recipient_phids;
   }
 
   public function processReceivedMail() {