When applying DMARC mitigations, CookHeaders now adds the original From:

to Cc: rather than Reply-To: in some cases to make MUA 'reply' and
'reply all' more consistent with the non-DMARC cases.

4 files changed, 84 insertions, 20 deletions

diff --git a/Mailman/Handlers/CookHeaders.py b/Mailman/Handlers/CookHeaders.py
index 5b20f23c..0139f3c3 100755
--- a/Mailman/Handlers/CookHeaders.py
+++ b/Mailman/Handlers/CookHeaders.py
@@ -1,4 +1,4 @@
-# Copyright (C) 1998-2014 by the Free Software Foundation, Inc.
+# Copyright (C) 1998-2015 by the Free Software Foundation, Inc.
 #
 # This program is free software; you can redistribute it and/or
 # modify it under the terms of the GNU General Public License
@@ -15,7 +15,10 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301,
 # USA.
 
-"""Cook a message's Subject header."""
+"""Cook a message's Subject header.
+Also do other manipulations of From:, Reply-To: and Cc: depending on
+list configuration.
+"""
 
 from __future__ import nested_scopes
 import re
@@ -68,7 +71,7 @@ def change_header(name, value, mlist, msg, msgdata, delete=True, repl=True):
     if ((msgdata.get('from_is_list') == 2 or
         (msgdata.get('from_is_list') == 0 and mlist.from_is_list == 2)) and 
         not msgdata.get('_fasttrack')
-       ) or name.lower() in ('from', 'reply-to'):
+       ) or name.lower() in ('from', 'reply-to', 'cc'):
         msgdata.setdefault('add_header', {})[name] = value
     elif repl or not msg.has_key(name):
         if delete:
@@ -154,6 +157,23 @@ def process(mlist, msg, msgdata):
     # augment it.  RFC 2822 allows max one Reply-To: header so collapse them
     # if we're adding a value, otherwise don't touch it.  (Should we collapse
     # in all cases?)
+    # MAS: We need to do some things with the original From: if we've munged
+    # it for DMARC mitigation.  We have goals for this process which are
+    # not completely compatible, so we do the best we can.  Our goals are:
+    # 1) as long as the list is not anonymous, the original From: address
+    #    should be obviously exposed, i.e. not just in a header that MUAs
+    #    don't display.
+    # 2) the original From: address should not be in a comment or display
+    #    name in the new From: because it is claimed that multiple domains
+    #    in any fields in From: are indicative of spamminess.  This means
+    #    it should be in Reply-To: or Cc:.
+    # 3) the behavior of an MUA doing a 'reply' or 'reply all' should be
+    #    consistent regardless of whether or not the From: is munged.
+    # Goal 3) implies sometimes the original From: should be in Reply-To:
+    # and sometimes in Cc:, and even so, this goal won't be achieved in
+    # all cases with all MUAs.  In cases of conflict, the above ordering of
+    # goals is priority order.
+
     if not fasttrack:
         # A convenience function, requires nested scopes.  pair is (name, addr)
         new = []
@@ -171,13 +191,20 @@ def process(mlist, msg, msgdata):
         # the original Reply-To:'s to the list we're building up.  In both
         # cases we'll zap the existing field because RFC 2822 says max one is
         # allowed.
+        o_rt = False
         if not mlist.first_strip_reply_to:
             orig = msg.get_all('reply-to', [])
             for pair in getaddresses(orig):
+                # There's an original Reply-To: and we're not removing it.
                 add(pair)
-        # We also need to put the old From: in Reply-To: in all cases.
-        if o_from:
+                o_rt = True
+        # We also need to put the old From: in Reply-To: in all cases where
+        # it is not going in Cc:.  This is when reply_goes_to_list == 0 and
+        # either there was no original Reply-To: or we stripped it.
+        if o_from and mlist.reply_goes_to_list == 0 and not o_rt: 
             add(o_from)
+            # Flag that we added it.
+            o_from = None
         # Set Reply-To: header to point back to this list.  Add this last
         # because some folks think that some MUAs make it easier to delete
         # addresses from the right than from the left.
@@ -206,13 +233,19 @@ def process(mlist, msg, msgdata):
         # because even though the list address is in From:, the Reply-To:
         # poster will override it. Brain dead MUAs may then address the list
         # twice on a 'reply all', but reasonable MUAs should do the right
-        # thing.
-        if (mlist.personalize == 2 and mlist.reply_goes_to_list <> 1 and
-            not mlist.anonymous_list):
+        # thing.  We also add the original From: to Cc: if it wasn't added
+        # to Reply-To:
+        add_list = (mlist.personalize == 2 and
+                    mlist.reply_goes_to_list <> 1 and
+                    not mlist.anonymous_list)
+        if add_list or o_from:
             # Watch out for existing Cc headers, merge, and remove dups.  Note
             # that RFC 2822 says only zero or one Cc header is allowed.
             new = []
             d = {}
+            # If we're adding the original From:, add it first.
+            if o_from:
+                add(o_from)
             # AvoidDuplicates may have set a new Cc: in msgdata.add_header,
             # so check that.
             if (msgdata.has_key('add_header') and
@@ -222,8 +255,9 @@ def process(mlist, msg, msgdata):
             else:
                 for pair in getaddresses(msg.get_all('cc', [])):
                     add(pair)
-            i18ndesc = uheader(mlist, mlist.description, 'Cc')
-            add((str(i18ndesc), mlist.GetListEmail()))
+            if add_list:
+                i18ndesc = uheader(mlist, mlist.description, 'Cc')
+                add((str(i18ndesc), mlist.GetListEmail()))
             change_header('Cc',
                           COMMASPACE.join([formataddr(pair) for pair in new]),
                           mlist, msg, msgdata)
diff --git a/Mailman/Handlers/WrapMessage.py b/Mailman/Handlers/WrapMessage.py
index 4fd88ed9..fba6bc2a 100644
--- a/Mailman/Handlers/WrapMessage.py
+++ b/Mailman/Handlers/WrapMessage.py
@@ -1,4 +1,4 @@
-# Copyright (C) 2013-2014 by the Free Software Foundation, Inc.
+# Copyright (C) 2013-2015 by the Free Software Foundation, Inc.
 #
 # This program is free software; you can redistribute it and/or
 # modify it under the terms of the GNU General Public License
@@ -49,6 +49,9 @@ def process(mlist, msg, msgdata):
             if a_h.get('Reply-To'):
                 del msg['reply-to']
                 msg['Reply-To'] = a_h.get('Reply-To')
+            if a_h.get('Cc'):
+                del msg['cc']
+                msg['Cc'] = a_h.get('Cc')
         return
 
     # There are various headers in msg that we don't want, so we basically
diff --git a/NEWS b/NEWS
index c344264e..7066b1e5 100755
--- a/NEWS
+++ b/NEWS
@@ -40,6 +40,10 @@ Here is a history of user visible changes to Mailman.
 
   Bug fixes and other patches
 
+    - When applying DMARC mitigations, CookHeaders now adds the original From:
+      to Cc: rather than Reply-To: in some cases to make MUA 'reply' and
+      'reply all' more consistent with the non-DMARC cases.  (LP: #1407098)
+
     - The Subject: of the list welcome message wasn't always in the user's
       preferred language.  Fixed.  (LP: #1400988)
 
diff --git a/tests/test_handlers.py b/tests/test_handlers.py
index c3df0cb8..8f6219e8 100644
--- a/tests/test_handlers.py
+++ b/tests/test_handlers.py
@@ -1,4 +1,4 @@
-# Copyright (C) 2001-2014 by the Free Software Foundation, Inc.
+# Copyright (C) 2001-2015 by the Free Software Foundation, Inc.
 #
 # This program is free software; you can redistribute it and/or
 # modify it under the terms of the GNU General Public License
@@ -614,8 +614,11 @@ From: aperson@dom.ain
         msgdata = {}
         CookHeaders.process(mlist, msg, msgdata)
         eq(msgdata['add_header']['Reply-To'],
-            'aperson@dom.ain, _xtest@dom.ain')
+            '_xtest@dom.ain')
+        eq(msgdata['add_header']['Cc'],
+            'aperson@dom.ain')
         eq(msg.get_all('reply-to'), None)
+        eq(msg.get_all('cc'), None)
 
     def test_reply_to_list_with_strip(self):
         eq = self.assertEqual
@@ -647,8 +650,12 @@ Reply-To: bperson@dom.ain
         msgdata = {}
         CookHeaders.process(mlist, msg, msgdata)
         eq(msgdata['add_header']['Reply-To'],
-            'aperson@dom.ain, _xtest@dom.ain')
+            '_xtest@dom.ain')
+        eq(msgdata['add_header']['Cc'],
+            'aperson@dom.ain')
         eq(msg.get_all('reply-to'), ['bperson@dom.ain'])
+        eq(msg.get_all('cc'), None)
+
 
     def test_reply_to_explicit(self):
         eq = self.assertEqual
@@ -678,8 +685,11 @@ From: aperson@dom.ain
         msgdata = {}
         CookHeaders.process(mlist, msg, msgdata)
         eq(msgdata['add_header']['Reply-To'],
-            'mlist@dom.ain, aperson@dom.ain')
+            'mlist@dom.ain')
+        eq(msgdata['add_header']['Cc'],
+            'aperson@dom.ain')
         eq(msg.get_all('reply-to'), None)
+        eq(msg.get_all('cc'), None)
 
     def test_reply_to_explicit_with_strip(self):
         eq = self.assertEqual
@@ -715,8 +725,11 @@ Reply-To: bperson@dom.ain
 
         CookHeaders.process(self._mlist, msg, msgdata)
         eq(msgdata['add_header']['Reply-To'],
-            'mlist@dom.ain, aperson@dom.ain')
+            'mlist@dom.ain')
+        eq(msgdata['add_header']['Cc'],
+            'aperson@dom.ain')
         eq(msg.get_all('reply-to'), ['bperson@dom.ain'])
+        eq(msg.get_all('cc'), None)
 
     def test_reply_to_extends_to_list(self):
         eq = self.assertEqual
@@ -750,7 +763,11 @@ Reply-To: bperson@dom.ain
 
         CookHeaders.process(mlist, msg, msgdata)
         eq(msgdata['add_header']['Reply-To'],
-            'bperson@dom.ain, aperson@dom.ain, _xtest@dom.ain')
+            'bperson@dom.ain, _xtest@dom.ain')
+        eq(msgdata['add_header']['Cc'],
+            'aperson@dom.ain')
+        eq(msg.get_all('reply-to'), ['bperson@dom.ain'])
+        eq(msg.get_all('cc'), None)
 
     def test_reply_to_extends_to_explicit(self):
         eq = self.assertEqual
@@ -784,7 +801,11 @@ Reply-To: bperson@dom.ain
         msgdata = {}
         CookHeaders.process(mlist, msg, msgdata)
         eq(msgdata['add_header']['Reply-To'],
-            'mlist@dom.ain, bperson@dom.ain, aperson@dom.ain')
+            'mlist@dom.ain, bperson@dom.ain')
+        eq(msgdata['add_header']['Cc'],
+            'aperson@dom.ain')
+        eq(msg.get_all('reply-to'), ['bperson@dom.ain'])
+        eq(msg.get_all('cc'), None)
 
     def test_list_headers_nolist(self):
         eq = self.assertEqual
@@ -980,7 +1001,8 @@ Content-Transfer-Encoding: 7bit
 Content-Disposition: inline
 
 footer
---BOUNDARY--""")
+--BOUNDARY--
+""")
 
     def test_image(self):
         eq = self.assertEqual
@@ -1749,7 +1771,8 @@ class TestReplybot(TestBase):
 class TestSpamDetect(TestBase):
     def test_short_circuit(self):
         msgdata = {'approved': 1}
-        rtn = SpamDetect.process(self._mlist, None, msgdata)
+        msg = email.message_from_string('')
+        rtn = SpamDetect.process(self._mlist, msg, msgdata)
         # Not really a great test, but there's little else to assert
         self.assertEqual(rtn, None)