launchpad-reviewers team mailing list archive

[Jürgen Gmach <mp+412672@xxxxxxxxxxxxxxxxxx>]

Jürgen Gmach has proposed merging ~jugmac00/launchpad:prevent-email-disclosure into launchpad:master.

Commit message:
Prevent email address disclosure for mirror notifications

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jugmac00/launchpad/+git/launchpad/+merge/412672

This is WIP:

There are a couple of more possibly problematic spots.

https://git.launchpad.net/launchpad/tree/lib/lp/soyuz/mail/binarypackagebuild.py#n166

https://git.launchpad.net/launchpad/tree/lib/lp/translations/scripts/po_import.py#n145

https://git.launchpad.net/launchpad/tree/lib/lp/translations/scripts/translations_to_branch.py#n297

-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~jugmac00/launchpad:prevent-email-disclosure into launchpad:master.

diff --git a/lib/lp/registry/model/distributionmirror.py b/lib/lp/registry/model/distributionmirror.py
index 8d3bd82..2454187 100644
--- a/lib/lp/registry/model/distributionmirror.py
+++ b/lib/lp/registry/model/distributionmirror.py
@@ -385,13 +385,14 @@ class DistributionMirror(SQLBase):
         message = template % replacements
         subject = "Launchpad: Verification of %s failed" % self.name
 
-        mirror_admin_address = get_contact_email_addresses(
+        mirror_admin_addresses = get_contact_email_addresses(
             self.distribution.mirror_admin)
-        simple_sendmail(fromaddress, mirror_admin_address, subject, message)
+        for admin_address in mirror_admin_addresses:
+            simple_sendmail(fromaddress, admin_address, subject, message)
 
         if notify_owner:
-            owner_address = get_contact_email_addresses(self.owner)
-            if len(owner_address) > 0:
+            owner_addresses = get_contact_email_addresses(self.owner)
+            for owner_address in owner_addresses:
                 simple_sendmail(fromaddress, owner_address, subject, message)
 
     def newProbeRecord(self, log_file):
diff --git a/lib/lp/registry/tests/test_distributionmirror.py b/lib/lp/registry/tests/test_distributionmirror.py
index 08dae43..8f1423d 100644
--- a/lib/lp/registry/tests/test_distributionmirror.py
+++ b/lib/lp/registry/tests/test_distributionmirror.py
@@ -170,14 +170,22 @@ class TestDistributionMirror(TestCaseWithFactory):
         mirror.disable(notify_owner=True, log=log)
         # A notification was sent to the owner and other to the mirror admins.
         transaction.commit()
-        self.assertEqual(len(stub.test_emails), 2)
+        self.assertEqual(len(stub.test_emails), 3)
+
+        # In order to prevent data disclosure, emails have to be sent to one
+        # person each, ie it is not allowed to have multiple recipients in an
+        # email's `to` field.
+        for email in stub.test_emails:
+            number_of_to_addresses = len(email[1])
+            self.assertLess(number_of_to_addresses, 2)
+
         stub.test_emails = []
 
         mirror.disable(notify_owner=True, log=log)
         # Again, a notification was sent to the owner and other to the mirror
         # admins.
         transaction.commit()
-        self.assertEqual(len(stub.test_emails), 2)
+        self.assertEqual(len(stub.test_emails), 3)
         stub.test_emails = []
 
         # For mirrors that have been probed more than once, we'll only notify
@@ -187,7 +195,7 @@ class TestDistributionMirror(TestCaseWithFactory):
         mirror.disable(notify_owner=True, log=log)
         # A notification was sent to the owner and other to the mirror admins.
         transaction.commit()
-        self.assertEqual(len(stub.test_emails), 2)
+        self.assertEqual(len(stub.test_emails), 3)
         stub.test_emails = []
 
         # We can always disable notifications to the owner by passing
@@ -195,7 +203,7 @@ class TestDistributionMirror(TestCaseWithFactory):
         mirror.enabled = True
         mirror.disable(notify_owner=False, log=log)
         transaction.commit()
-        self.assertEqual(len(stub.test_emails), 1)
+        self.assertEqual(len(stub.test_emails), 2)
         stub.test_emails = []
 
         mirror.enabled = False
@@ -217,12 +225,12 @@ class TestDistributionMirror(TestCaseWithFactory):
         transaction.commit()
         stub.test_emails = []
 
-        # Disabling the mirror results in a single notification to the
-        # mirror admins.
+        # Disabling the mirror results in one notification for each of
+        # the three mirror admins.
         self.factory.makeMirrorProbeRecord(mirror)
         mirror.disable(notify_owner=True, log="It broke.")
         transaction.commit()
-        self.assertEqual(len(stub.test_emails), 1)
+        self.assertEqual(len(stub.test_emails), 3)
 
     def test_permissions_for_resubmit(self):
         self.assertRaises(