249912 - XSENDER support broken in IMAP

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Hans-Andreas Engel]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040622
Build Identifier: 

The information obtained via the XSENDER server extension is only used for the
first downloaded message of each imap folder.  For all following messages, this
information is discarded.

Note that there is currently no explicit UI to display if a message has been
authenticated with XSENDER.  However, one can use "Message Source" and check for
flag 0x0200 in the X-Mozilla-Status header line.



Reproducible: Always
Steps to Reproduce:

Comment 1

[Hans-Andreas Engel]

The XSENDER capability provides "a mechanism to retrieve envelope information
indicating that the message was received via Authenticated SMTP."

http://help.netscape.com/kb/corporate/19980422-6.html

Comment 2

[Hans-Andreas Engel]

To fix this bug, |m_FromHeaderSeen = PR_FALSE;| must be set in
nsImapProtocol::BeginMessageDownLoad. On a historical note, this was actually
done in the code added to nsImapProtocol.cpp in r1.9 (however, this code was
within an |#if 0|), see 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/imap/src/nsImapProtocol.cpp&rev=1.9#587
The |#if 0| was then removed in r1.16, but at the same time, |fFromHeaderSeen =
PR_FALSE;| was unfortunately also removed from the code.

When the XSENDER authentication was successful, an X-Mozilla-Status flag
|(MSG_FLAG_SENDER_AUTHED | MSG_FLAG_READ)| is added to the message.  It there a
reason behind setting the |MSG_FLAG_READ| bit?  IMO it should only be
|MSG_FLAG_SENDER_AUTHED|.  I have tested this and have found that the
|MSG_FLAG_READ| bit make no difference.  Unless there is a good reason for it, I
suggest to use only |MSG_FLAG_SENDER_AUTHED|.

Comment 3

[Hans-Andreas Engel]

Attachment: record the XSENDER information for every message

This patch
* adds |m_FromHeaderSeen = PR_FALSE;| to BeginMessageDownLoad,
* inlines AddXMozillaStatusLine(), and
* only sets the MSG_FLAG_SENDER_AUTHED flag.

Comment 4

[Stephen Donner [:stephend] Not actively reading bugmail]

Setting status to NEW.

Comment 5

[Hans-Andreas Engel]

To test this with an IMAP server which does not support XSENDER, one can add the
following code to |nsImapProtocol::HandleMessageDownLoadLine()|.  This emulates
an XSENDER authentication of all messages.

if(!m_fromHeaderSeen && !PL_strncmp("From: ", localMessageLine, 6)) {
  m_fromHeaderSeen = PR_TRUE;
  HandleMessageDownLoadLine("X-Mozilla-Status: 0200\r\n", PR_FALSE);
}

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

To be honest, I'm not sure if Kaie still actively reviews Mozilla code.

Kaie, if you do, sorry for the false statement.

Comment 7

[Hans-Andreas Engel]

Comment on attachment 152361 [details] [diff] [review]
record the XSENDER information for every message

Stephen, thank you for the hint.  I picked the first non-superreviewer from the
list of mailnews peers,
  <http://mozilla.org/owners.html#Mail/News>.

Now, I am asking Jean-Francois Ducarroz for review.  He is actively reviewing
bugs, see for example bug 90161 (review on 2004-06-18).

Comment 8

[Jean-Francois Ducarroz]

Comment on attachment 152361 [details] [diff] [review]
record the XSENDER information for every message

Looks good. R=ducarroz

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

I tried to apply this patch (to land it for you since 1.8a3's open), but got:

C:\moz_src\mozilla\mailnews\imap\src>patch <patch.txt
(Stripping trailing CRs from patch.)
patching file nsImapProtocol.cpp
(Stripping trailing CRs from patch.)
patching file nsImapProtocol.h
Hunk #1 succeeded at 235 with fuzz 1.
patch unexpectedly ends in middle of line
patch unexpectedly ends in middle of line

probably a result of

004-07-13 21:16	scott%scott-macgregor.org 	mozilla/ mailnews/ imap/ src/
nsImapProtocol.cpp 	1.571 	4/6  	Bug #250698 --> make
nsIMsgCopyService.CopyFileMessage useful for arbitrary folders

Patch by Myk
r=bienvenu
sr=sspitzer
2004-07-13 21:16	scott%scott-macgregor.org 	mozilla/ mailnews/ imap/ src/
nsImapMailFolder.cpp 	1.662 	4/2

Hans, can you update your tree so a patch could apply cleanly?

Thanks! 

Comment 10

[Hans-Andreas Engel]

Stephen, thank you very much for checking in my patch!  It is great that you are
closely watching this and other IMAP issues.


The patch (attachment 152361 [details] [diff] [review]) seems to work fine with the most recent CVS
versions, namely 
  nsImapProtocol.cpp r1.571  and
  nsImapProtocol.h   r1.180:

$ cd mailnews/imap/src/
$ patch -p0 < ~/fix_XSENDER_p1
patching file nsImapProtocol.cpp
patching file nsImapProtocol.h
$

I created a new patch using
$ cvs diff -up8N nsImapProtocol.cpp nsImapProtocol.h > fix_XSENDER_p2
However, this patch is identical to the old one (since all recent changes were
at line # >= 4000 while this patch is for line # ~ 2500). 


Apparently, you are applying the patch on a Windows machine.  Probably you are
suffering from some inconsistent DOS vs. unix line endings.  Maybe running
unix2dos on the patch will solve this issue?

Or, if you are using cygwin cvs, there could be the following artifact, see
<http://www.gigascale.org/softdevel/faq/23.html>:
    The reason that this is a problem with CVS is that CVS handles 
    end of line translation between Unix and Windows.  If, under
    Cygwin, the directories are mounted binmode, then Cygwin CVS 
    will not do the proper translations.

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

Timeless landed this.

Thanks, Hans and Timeless.