Mails sent from roundup contain duplicate headers.

To: ...
Subject: INVALID HEADER in mail TO YOU from <roundup-admin@tryton.org>
Date: Thu, 19 Feb 2015 08:06:59 +0000 (UTC)



Our content checker found
    Duplicate header field: "Reply-To"
    Duplicate header field: "Message-ID"


in an email to you from:
  roundup-admin@tryton.org

Content type: BadHdrDupl
Our internal reference code for your message is 01019-04/QweNCjtxuziY

First upstream SMTP client IP address: [94.23.24.155] moretus.b2ck.com
According to a 'Received:' trace, the message apparently originated at:
  [94.23.24.155], moretus.b2ck.com moretus.b2ck.com [IPv6:::1]


We fixed that on our roundup (1.4) since considerable time with the
patch in http://codereview.tryton.org/11001002 and it works without
errors to our knowledge.

* Mathias Behrle: " [issue2550869] Duplicate mail headers" (Thu, 19 Feb 2015
  11:34:39 +0000):

Since I got 

From: Mathias Behrle <issues@roundup-tracker.org>
To: mbehrle@m9s.biz
Reply-To: Roundup tracker <issues@roundup-tracker.org>
Reply-To: Roundup tracker <issues@roundup-tracker.org>

with

X-Amavis-Alert: BAD HEADER SECTION, Duplicate header field: "Reply-To"
X-Roundup-Version: 1.5.0

this still applies also for version 1.5.

-- 

    Mathias Behrle
    MBSolutions
    Gilgenmatten 10 A
    D-79114 Freiburg

    Tel: +49(761)471023
    Fax: +49(761)4770816
    http://www.m9s.biz
    UStIdNr: DE 142009020
    PGP/GnuPG key availabable from any keyserver, ID: 0x8405BBF6

Hi Mathias,

thanks for the report and the suggested fix.
I checked out one of our roundup emails and it had the doubled headers.
So I consider the problem confirmed.

However before removing the lines like you have done in your patch,
I think I must find out why they are there in the first place?
There must be a situation where they where necessary, maybe with an 
elder email python module or a different change elsewhere.

Best,
Bernhard

* Bernhard Reiter: " [issue2550869] Duplicate mail headers (Reply-To,
  Message-ID, In-Reply-To) when sending out email." (Thu, 19 Feb 2015 14:33:10
  +0000):

Hi Bernhard,

> thanks for the report and the suggested fix.
> I checked out one of our roundup emails and it had the doubled headers.
> So I consider the problem confirmed.
> 
> However before removing the lines like you have done in your patch,
> I think I must find out why they are there in the first place?
> There must be a situation where they where necessary, maybe with an 
> elder email python module or a different change elsewhere.

That was exactly my thought, too. I just couldn't find out, if there are
special cases where those headers need ot be (re)set. At least we are running
our roundup without the lines in question. In case they should be needed by some
legacy module and the lines should be considered necessary, at least there could
be a added a test to not add them twice.

Cheers,
Mathias
-- 

    Mathias Behrle
    MBSolutions
    Gilgenmatten 10 A
    D-79114 Freiburg

    Tel: +49(761)471023
    Fax: +49(761)4770816
    http://www.m9s.biz
    UStIdNr: DE 142009020
    PGP/GnuPG key availabable from any keyserver, ID: 0x8405BBF6

On Thursday 19 February 2015 at 16:48:03, Mathias Behrle wrote:
> In case they should be needed by some
> legacy module and the lines should be considered necessary, at least there
> could be a added a test to not add them twice.

Yes, this is the way to go once we find out.
I believe the next steps would be: 
* see in hg history when this was added.
* check out at which other place the headers are set currently.
* check f the behaviour of this module changed in the past.
* (maybe) try an elder verison of round. Or try the oldest.
* (optional) write/extend a test case that checks for double header lines.

The code causing the header violation comes from Ralf in the following commit.
In the python 2.7.6 I am testing even rev:4542 never did this correctly, so I am not
sure why Ralf ever added it  in the first place?

Ralf, do you remember why you wrote the lines? :)

changeset:   4542:46239c21a1eb
user:        Ralf Schlatterbeck <schlatterbeck@users.sourceforge.net>
date:        Fri Oct 07 18:04:00 2011 +0000
files:       CHANGES.txt roundup/roundupdb.py test/db_test_base.py test/gpgmelib.py 
test/test_mailgw.py
description:
Sending of PGP-Encrypted mail to all users or selected users (via roles)...

...is now working. (Ralf)



Digging further I found that the email comparison function in test_mailgw.py
does not catch this, because it only compares one of the duplicated headers.
And that roundup still uses the deprecated rfc822 module. We should probably
test if the email module does not do this better anyway.

Created issue2550870 for the migration from rfc822 to the email module.

Adding issue869-better-test-20150225.diff which patches test_mailgw so that test function
that does a more thorough comparision of the email headers noticing more than one entry.
(But also case insensitivity differences.)

This finds the reported issue, but also additional ones e.g. MIME-Version seems to
be there twice sometimes.

Thanks John for the get_body() function, here is my first commit.
This leads to 119 failures, but I believe this is the right next
step as the test itself was to lax, missing problems which the test
authors probably did not understand.

changeset:   4965:a850f8bae536
tag:         tip
user:        Bernhard Reiter <bernhard@intevation.de>
date:        Mon Mar 02 17:08:40 2015 +0100
files:       test/test_mailgw.py
description:
Moved test_mailgw to email module and make message comparison sharper to
flag differences in headers with the same name.

Before removing the code that set the double header reply-to,
it would be cool to have an automated test that tests the crypt branch
of the code. Would needed to be a test with PGP_ENCRYPT enabled in the
config.

Manual testing revealed that re-setting the headers is necessary
in the "crypt == True" code branch. Thus a conditional fixes the issue.

changeset:   4969:010ce624b320
Fixing duplicated email headers message-id, reply-to, in-reply-to. (part of issue2550869).


Details: 
The testing was done in the following way.
A print statement was added to db_test_base.py:
--- a/test/db_test_base.py      Sun Feb 08 00:35:41 2015 +1100
+++ b/test/db_test_base.py      Tue Mar 10 21:16:50 2015 +0100
@@ -2050,6 +2050,7 @@
                 messages = [m], nosy = [db.user.lookup("fred"), john])

             db.issue.nosymessage(i, m, {})
+            print str(res)
             res.sort(key=lambda x: x['mail_to'])
             self.assertEqual(res[0]["mail_to"], ["fred@example.com"])
             self.assertEqual(res[1]["mail_to"], ["john@test.test"])
then
  python run_tests.py any PGP
saving the output list in file y, making a python statement out of it:
a=
for i,v in enumerate(a):
 for j,w in enumerate(v):
   print j,w
running python file >v1
and comparing the variants manuelly (e.g. by diff or kompare).

The remaining test failures are other headers now,
thus I've created a new issue for it 2550877.