James insert a Return-Path: null in outgoing email

• Key: JAMES-570
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Blocker
• Assignee: Norman Maurer
• Reporter: Norman Maurer
• Affects Version/s: 2.3.0
• Fix Version/s: 2.3.0

Description

At the moment james insert a Return-Path: null to outgoing emails. That is not correct. On outgoing emails no Return-Path should be insert.

From RFC (http://www.faqs.org/rfcs/rfc821.html):
When the receiver-SMTP makes the "final delivery" of a
message it inserts at the beginning of the mail data a
return path line. The return path line preserves the
information in the <reverse-path> from the MAIL command.
Here, final delivery means the message leaves the SMTP
world. Normally, this would mean it has been delivered to
the destination user, but in some cases it may be further
processed and transmitted by another mail system.


We have also to get sure what todo if there is allready one ( should never happen). Should we keep the header or strip it.

Comments

[Soren Hilmer]

The current SMTP specification is in (http://www.faqs.org/rfcs/rfc2821.html).

It has a lot more detail on this subject in section 4.4

[Norman Maurer]

Its also importent to know that some mailserver strip the return path if there is allready one and some not to reproduce this bug. For example postfix and sendmail seems to strip it. Qmail not.

[Stefano Bagnara]

The problem seems to be related to code in MimeMessageWrapped added by Noel on 05/05/2005 (r168238)

"revision 109187 introduced a bug where if we had a real Return-Path header its value would not be the first in the String[]. This tries to eliminate that problem. We should still test to see if this hack is necessary at all, or if InternetHeaders.setHeader("Return-Path", <value>) is smart enough in JavaMail 1.3.2 to put the header at the top, instead of in the middle."

I this also the comment to r109187 is usefult:

Additional adjustments to fix JAMES-264. InternetHeaders is an awkward class to work with due its own internal handling of header order. This change takes advantage of how InternetHeaders works, pre-placing a null placeholder for a possible Return-Path header so that when setHeader is called, it will use that placeholder, and remove any other Return-Path headers. It also ensures that there are no other placeholders, providing a bit more control as we manipulate the Delivered-To headers (and possibly other headers that JavaMail may want to change internally, such as Message-ID, Date and Mime-Version).

InternetHeaders currently works differently if created with empty constructor or from stream. In the first case it create an empty sorted list of headers to keep positions, otherwise it does it the wrong way.

Changing the MailHeaders(InputStream) constructor to use the empty super constructor and calling load(is) seems to fix this difference.

Norman is currently working on writing unit testing and to find a solution to this issues.

[Stefano Bagnara]

This bug has been introduced by the fix to JAMES-264

[Stefano Bagnara]

InternetHeaders have this bugs:
1) if you load an header from stream then following addHeader will not follow "default" sorting.
2) if you add a Return-Path to headers loaded from a stream with a Return-Path in the bad place it will add the new Return-Path just before the old one, and not at the start of the message
3) if you replace a Return-Path to headers loaded from a stream with a Return-Path in the bad place it will put the new Return-Path in the same position of the old one.

So we changed MailHeaders to fix this issues:
1) Changed the default constructor to use the right one from the super class and then load the stream
2) Changed the setHeader and addHeader to correctly handle the Return-Path header

Then we removed all the custom routine introduced by Noel to fix the Return-Path issues that had the problem described by this issue and was no more needed with this update.

This fix also allowed to remove workaround code from MimeMessageTest (it worked around the Return-Path: null). I probably have been too lazy inserting this workaround instead of trying to understand why it was there when testing.

Btw all the tests now passes, feel free to test it and backport to 2.3

[Norman Maurer]

Backported to 2.3

[Danny Angus]

Closing issue fixed in released version.