Closed Bug 523796 Opened 15 years ago Closed 11 years ago

Eml file content change sending it as an attached file (if there is no [CRLF] at end of .eml file, Tb tries to send in Base64, then bug 326303 occurs)

Categories

(MailNews Core :: MIME, defect)

Product:
MailNews Core
Component:
MIME
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 23.0

People

(Reporter: guidros, Assigned: hiro)

References

(Blocks 3 open bugs)

Details

(Keywords: testcase)

Attachments

(4 files, 2 obsolete files)

file to be send
2.35 KB, text/plain
Details
Fixed encoding pickup
2.35 KB, text/plain
Details
Tests covering bug 523796 and bug 291899
4.15 KB, patch
Details | Diff | Splinter Review
Hiros patch with nit fixed
1.53 KB, patch
Usul
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/532.0 (KHTML, like Gecko) Chrome/3.0.195.27 Safari/532.0
Build Identifier: 2.0.0.23 (20090812)

If I attach an eml file that is an email with multipart/alternative content, saved by Outlook Express, opening the attached file with Thunderbird in the destination client the email body is not shown.

Reproducible: Always

Steps to Reproduce:
1.Save the following text in a file called "file.eml":

X-Account-Key: account3
Return-Path: <useram@domaingal.com>
Received: from cluster.serviceorg.com (cluster.serviceorg.com [10.213.4.42])
	by mail.serviceorg.com (8.12.11.20060614/8.12.11) with ESMTP id kARABqg0023308
	for <userpf@domaingal.com>; Mon, 27 Nov 2006 11:11:54 +0100
Received: from cluster.serviceorg.com (localhost.localdomain [127.0.0.1])
	by localhost.serviceorg.com (Postfix) with ESMTP id 30C6248047
	for <userpf@domaingal.com>; Mon, 27 Nov 2006 11:11:52 +0100 (CET)
Received: from spiderwall.localdomain 
	(gateway-gal.domaingal.com [10.10.254.4])by 
	cluster.serviceorg.com (Postfix) with SMTP id 5530848056for 
	<userpf@domaingal.com>; Mon, 27 Nov 2006 11:11:48 +0100
	 (CET)
Received: (qmail 27268 invoked from network); 27 Nov 2006 10:11:48 -0000
Received: from pc-seg.dominio.locale (HELO PCSEG) 
	(10.10.0.100) by 12.45.87.123 with SMTP; 27 Nov 2006 10:11:47 -0000
Message-ID: <002601c7120c$7220d3d0$6400670a@dominio.locale>
From: "name surname" <useram@domaingal.com>
To: <destuser@domaingal.com>
Subject: determ- iter
Date: Mon, 27 Nov 2006 11:11:46 +0100
MIME-Version: 1.0
Content-Type: multipart/alternative;
	boundary="----=_NextPart_000_0023_01C71214.D3D49A00"
X-Priority: 3
X-MSMail-Priority: Normal
X-Mailer: Microsoft Outlook Express 6.00.2900.2869
X-MimeOLE: Produced By Microsoft MimeOLE V6.00.2900.2962
X-imss-version: 2.044
X-imss-result: Passed
X-imss-scanInfo: M:P L:E SM:0
X-imss-tmaseResult: TT:0 TS:0.0000 TC:00 TRN:0 TV:3.6.1039(14838.003)
X-imss-scores: Clean:64.23839 C:2 M:3 S:5 R:5
X-imss-settings: Baseline:1 C:4 M:4 S:4 R:4 (0.0000 0.0000)
X-Scanned-By: MIMEDefang 2.54 on 10.213.4.47

This is a multi-part message in MIME format.

------=_NextPart_000_0023_01C71214.D3D49A00
Content-Type: text/plain;
	charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable

Here there is plain text of mail body.

------=_NextPart_000_0023_01C71214.D3D49A00
Content-Type: text/html;
	charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<HTML><HEAD>
<META http-equiv=3DContent-Type content=3D"text/html; =
charset=3Diso-8859-1">
<META content=3D"MSHTML 6.00.2900.2963" name=3DGENERATOR>
<STYLE></STYLE>
</HEAD>
<BODY bgColor=3D#ffffff>
Here there is html content
</BODY></HTML>

------=_NextPart_000_0023_01C71214.D3D49A00--

2.In Thunderbird create a new message
3.Fill the To field with your email address, subject with word "test" and body with word "test" (every other text it's the same) then attach "file.eml".
4.Send the message.
5.When you receive the message open it and double click the "file.eml" file attached from Thunderbird or save the file and open it with Thunderbird.
Actual Results:  
Opening the "file.eml" RECEIVED you'll not see the body and the source content of the file received is changed like this:

X-Account-Key: account3
Return-Path: <useram@domaingal.com>
Received: from cluster.serviceorg.com (cluster.serviceorg.com [10.213.4.42])
	by mail.serviceorg.com (8.12.11.20060614/8.12.11) with ESMTP id kARABqg0023308
	for <userpf@domaingal.com>; Mon, 27 Nov 2006 11:11:54 +0100
Received: from cluster.serviceorg.com (localhost.localdomain [127.0.0.1])
	by localhost.serviceorg.com (Postfix) with ESMTP id 30C6248047
	for <userpf@domaingal.com>; Mon, 27 Nov 2006 11:11:52 +0100 (CET)
Received: from spiderwall.localdomain 
	(gateway-gal.domaingal.com [10.10.254.4])by 
	cluster.serviceorg.com (Postfix) with SMTP id 5530848056for 
	<userpf@domaingal.com>; Mon, 27 Nov 2006 11:11:48 +0100
	 (CET)
Received: (qmail 27268 invoked from network); 27 Nov 2006 10:11:48 -0000
Received: from pc-seg.dominio.locale (HELO PCSEG) 
	(10.10.0.100) by 12.45.87.123 with SMTP; 27 Nov 2006 10:11:47 -0000
Message-ID: <002601c7120c$7220d3d0$6400670a@dominio.locale>
From: "name surname" <useram@domaingal.com>
To: <destuser@domaingal.com>
Subject: determ- iter
Date: Mon, 27 Nov 2006 11:11:46 +0100
MIME-Version: 1.0
Content-Type: multipart/alternative;
	boundary="----=_NextPart_000_0023_01C71214.D3D49A00"
X-Priority: 3
X-MSMail-Priority: Normal
X-Mailer: Microsoft Outlook Express 6.00.2900.2869
X-MimeOLE: Produced By Microsoft MimeOLE V6.00.2900.2962
X-imss-version: 2.044
X-imss-result: Passed
X-imss-scanInfo: M:P L:E SM:0
X-imss-tmaseResult: TT:0 TS:0.0000 TC:00 TRN:0 TV:3.6.1039(14838.003)
X-imss-scores: Clean:64.23839 C:2 M:3 S:5 R:5
X-imss-settings: Baseline:1 C:4 M:4 S:4 R:4 (0.0000 0.0000)
X-Scanned-By: MIMEDefang 2.54 on 10.213.4.47

VGhpcyBpcyBhIG11bHRpLXBhcnQgbWVzc2FnZSBpbiBNSU1FIGZvcm1hdC4NCg0KLS0tLS0t
PV9OZXh0UGFydF8wMDBfMDAyM18wMUM3MTIxNC5EM0Q0OUEwMA0KQ29udGVudC1UeXBlOiB0
ZXh0L3BsYWluOw0KCWNoYXJzZXQ9Imlzby04ODU5LTEiDQpDb250ZW50LVRyYW5zZmVyLUVu
Y29kaW5nOiBxdW90ZWQtcHJpbnRhYmxlDQoNCkhlcmUgdGhlcmUgaXMgcGxhaW4gdGV4dCBv
ZiBtYWlsIGJvZHkuDQoNCi0tLS0tLT1fTmV4dFBhcnRfMDAwXzAwMjNfMDFDNzEyMTQuRDNE
NDlBMDANCkNvbnRlbnQtVHlwZTogdGV4dC9odG1sOw0KCWNoYXJzZXQ9Imlzby04ODU5LTEi
DQpDb250ZW50LVRyYW5zZmVyLUVuY29kaW5nOiBxdW90ZWQtcHJpbnRhYmxlDQoNCjwhRE9D
VFlQRSBIVE1MIFBVQkxJQyAiLS8vVzNDLy9EVEQgSFRNTCA0LjAgVHJhbnNpdGlvbmFsLy9F
TiI+DQo8SFRNTD48SEVBRD4NCjxNRVRBIGh0dHAtZXF1aXY9M0RDb250ZW50LVR5cGUgY29u
dGVudD0zRCJ0ZXh0L2h0bWw7ID0NCmNoYXJzZXQ9M0Rpc28tODg1OS0xIj4NCjxNRVRBIGNv
bnRlbnQ9M0QiTVNIVE1MIDYuMDAuMjkwMC4yOTYzIiBuYW1lPTNER0VORVJBVE9SPg0KPFNU
WUxFPjwvU1RZTEU+DQo8L0hFQUQ+DQo8Qk9EWSBiZ0NvbG9yPTNEI2ZmZmZmZj4NCkhlcmUg
dGhlcmUgaXMgaHRtbCBjb250ZW50DQo8L0JPRFk+PC9IVE1MPg0KDQotLS0tLS09X05leHRQ
YXJ0XzAwMF8wMDIzXzAxQzcxMjE0LkQzRDQ5QTAwLS0=
------=_NextPart_000_0023_01C71214.D3D49A00--


Expected Results:  
You should see the body content of the file.eml attached to the email sended.
Attached file file to be send — 
File to be send in order to reproduce the bug.
Component: General → MIME
Keywords: testcase
Product: Thunderbird → MailNews Core
QA Contact: general → mime
Version: unspecified → 1.8 Branch
I believe I can fix this problem. Yet I need more time to test my patch.
I notice that MIME specifies an attachment of "message" type shouldn't be encoded with BASE64. After searched the bug list, I found Bug 161806 is related to how encoding is selected. So I'll upload a patch for that bug. Then I'll write a test case to ensure the MIME specification is applied.
Attachment #418472 - Flags: superreview?(neil)
Attachment #418472 - Flags: review?(daniel)
Attached file Fixed encoding pickup — 
Just what I have in mind so far if anybody is interested. After I finish it, I'll upload it to Bug 161806.
Comment on attachment 418472 [details] [diff] [review]
Don't encode an attachment with different encodings

I know this patch touches a file located in mailnews/compose/ but the proposed fix is really not related to editor and I am no mailnews peer. Please find another reviewer. Sorry.
Attachment #418472 - Flags: review?(daniel) → review-
Do you mind telling me who I should add to review list? David :Bienvenu (bienvenu at nventure dot com)? Should I change super reviewer as well?
Attachment #418472 - Flags: superreview?(neil)
Attachment #418472 - Flags: superreview?(bienvenu)
Attachment #418472 - Flags: review?(bienvenu)
Attachment #418472 - Flags: review-
(In reply to comment #6)
> Do you mind telling me who I should add to review list? David :Bienvenu
> (bienvenu at nventure dot com)? Should I change super reviewer as well?

https://developer.mozilla.org/en/Mailnews_and_Mail_code_review_requirements documents that :-)
Assignee: nobody → ZaneUJi
patch should build on trunk too. I wouldn't expect it to be applied to v2.x.
Is this considered dataloss or blocking.?
OS: Windows XP → All
Hardware: x86 → All
Version: 1.8 Branch → Trunk
I think this problem must be considered dataloss cause the middle user see nothing in the received email.

(In reply to comment #8)
> patch should build on trunk too. I wouldn't expect it to be applied to v2.x.
> Is this considered dataloss or blocking.?
(In reply to comment #1)
> file to be send

({CRLF}=0x0D0A, {EOF}=End of file, i.e. no data) 
Last line is next(all other lines has {CRLF}) 
> ------=_NextPart_000_0023_01C71214.D3D49A00--{EOF}
If {CRLF} is added, message/rfc822 part was created ormally.
> ------=_NextPart_000_0023_01C71214.D3D49A00--{CRLF}
> {EOF}
If no {CRLF} at end of .eml file of simple plain/text mail, problem is reprduced with Tb3.
Mime-type handling related issue?
It looks:
  loss of end of file due to no {CRLF} after last line
  => base64 encoding of whole mail payload of the mail in .eml file.
Confirming by test result in my comment #10.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Probably problems are next.
(1) When no {CRLF} at end of .eml file,
    Tb tries to send message/rfc822 part in base64.
(2) When base64 is applied to message/rfc822 part, bug 326303 happens,
    and garbled part is generated.
> --------------020402020109030905040000
> Content-Type: message/rfc822; name="bug-523796-3.eml"
> Content-Transfer-Encoding: base64
> 
> (all message header lines of mail in .eml file, without base64 encoding)
>
> (Base64 encoded data of payload of mail in .eml file)
> --------------020402020109030905040000--  (close boundary)
If .eml file contains wrong data such as 0x00, 0xFF, sending in Base64 is required. But, in this bug's case, there is no such data in .eml file, and user's setting is "send in plain text, not in Base64". So, problem of (1) should be resolved by this bug.
(In reply to comment #13)
> If .eml file contains wrong data such as 0x00, 0xFF, sending in Base64 is
> required. But, in this bug's case, there is no such data in .eml file, and
> user's setting is "send in plain text, not in Base64". So, problem of (1)
> should be resolved by this bug.

You really dig deep.

At the moment, I know it shouldn't be encoded(Comment 3). Even if my patch is applied, this bug could be closed but there are still problems. That is why I didn't upload a test case.

When I finish the patch (Comment 4) for Bug 161806, a message/rfc822 attachment will not be encoded. Then I'll upload a test case.

To fix Bug 161806, I have to patch mime_encode_qp_buffer in addition to the code I uploaded in Comment 4. The problem with mime_encode_qp_buffer is that it converts single CR and LF to line break (CRLF) rather than "=0D" and "=0A". I believe the original developer just misunderstood MIME specification. It just forbidden putting CR in a string like "A B C D \x0D E F" or LF like "A B C D \x0A E F", but "A B C D =0D E F" and "A B C D =0A E F" are legal. Here is what I believe is correct:
diff --git a/mailnews/mime/src/mimeenc.cpp b/mailnews/mime/src/mimeenc.cpp
--- a/mailnews/mime/src/mimeenc.cpp
+++ b/mailnews/mime/src/mimeenc.cpp
@@ -997,18 +997,18 @@ mime_encode_qp_buffer (MimeEncoderData *
 
 
   NS_ASSERTION(data->in_buffer_count == 0, "1.1 <rhp@netscape.com> 19 Mar 1999 12:00");
 
   /* Populate the out_buffer with quoted-printable data, one line at a time.
   */
   for (; in < end; in++)
   {
-    if (*in == '\r' || *in == '\n')
-    {
+    if (*in == '\r' && in + 1 < end && in[1] == '\n')
+    {/*RFC2049*/
 
     /* Whitespace cannot be allowed to occur at the end of
     the line, so we backup and replace the whitespace with
     its code.
       */
       if (white)
       {
         out--;
Attachment #418475 - Attachment mime type: application/octet-stream → text/plain
Zane, do you still think this patch is the right thing to do, or will this be fixed by the other patch(es) you're working on?
Summary: Eml file content change sending it as an attached file → Eml file content change sending it as an attached file (if there is no [CRLF] at end of .eml file, Tb tries to send in Base64, then bug 326303 occurs)
Yes, I do. The patch submitted here addresses an inconsistency problem in encoding attachments. No matter what happens, we should encode the header of the attachment if the rest of it is encoded. In my opinion, it's another problem whether an attachment should be encoded.
(In reply to comment #16)
> No matter what happens, we should encode the header of the attachment if the rest of it is encoded.

Are you resoving bug 326303 at same time?
The patch I submitted in comment 3 should fix bug 326303. I had known nothing about bug 326303 until you mentioned it.
Setting dependency of bug 326303, for ease of tracking.
Blocks: 326303
Following is copy of bug 161806 comment #5.
> The current detection in nsMsgAttachmentHandler::AnalyzeDataChunk() is too weak
>(snip)
> This code detects any mixed combination of CR, LF and CRLF as text file line
> endings. If the remaining chars are considered printable, such a (binary!)
> file will be sent as text/plain with all CR, LF converted to CRLF.

As seen in bug 161806 comment #7, "mixed CR/LF/CRLF case" is processed by bug 269390 and was fixed (now sent in base64).
However, as I wrote in bug 291899 comment #10(application/postscript is treated as text type by Tb), binary data is still sent as plain text, if text type attachment and if file has single line-ending only(test data is for [CRLF] only case. Sorry but I don't check no line-ending case which is partially similar to this bug).

Zane U. Ji, can your patch resolve problem like bug 291899 too?
I can but I can't eat an elephant in one day. I'll try to move forward step by step.

The code indicates that unprintable data won't be encoded when "mail.strictly_mime" preference is set to "false", which is the default value. I found a description for "mail.strictly_mime" at:
http://kb.mozillazine.org/Mail_and_news_settings.
But it seems that description is for "mail.strictly_mime_headers". So I am really confused. Why that configuration is checked before encoding unprintable data? Shouldn't those data always be encoded? Does somebody try to create a non-mime standard email? What is the meaning of that?

What I can come up with is that the code is wrong. The logical operator between "mail.strictly_mime" and unprintable data should be "OR" rather than "AND":
diff --git a/mailnews/compose/src/nsMsgAttachmentHandler.cpp b/mailnews/compose/src/nsMsgAttachmentHandler.cpp
--- a/mailnews/compose/src/nsMsgAttachmentHandler.cpp
+++ b/mailnews/compose/src/nsMsgAttachmentHandler.cpp
@@ -365,22 +360,24 @@ nsMsgAttachmentHandler::PickEncoding(con
     }
     else if (mime_delivery_state) {
       if (((nsMsgComposeAndSend *)mime_delivery_state)->mCompFields->GetForceMsgEncoding())
         force_p = PR_TRUE;
     }
 
     if (force_p || (m_max_column > 900))
       encode_p = PR_TRUE;
-    else if (UseQuotedPrintable() && m_unprintable_count)
+    else if (UseQuotedPrintable() || m_unprintable_count)
       encode_p = PR_TRUE;
 
       else if (m_null_count)  /* If there are nulls, we must always encode,
         because sendmail will blow up. */
         encode_p = PR_TRUE;
Attachment #418472 - Flags: superreview?(bienvenu)
Attachment #418472 - Flags: superreview-
Attachment #418472 - Flags: review?(bienvenu)
Attachment #418472 - Flags: review-
Comment on attachment 418472 [details] [diff] [review]
Don't encode an attachment with different encodings

Sorry for the delay - we'd need some sort of unit test for this...I think that should be relatively straightforward, a simple xpcshell test that attempts to send a message with the problem attachment.

and a comment about why this might fail would be useful.
If this bug will be fixed, we will be able to see bug 580017 reliably and consistently. Setting dependency to that bug for ease of tracking and search.
Blocks: 269826
Depends on: 580017
Base64 encoding(and quoted-printable too) of message/rfc822 part(composite media type) was EXPRESSLY forbidden by RFC 2045.

http://tools.ietf.org/html/rfc2045#section-6.4
> 6.4. Interpretation and Use
>(snip)
>    Certain Content-Transfer-Encoding values may only be used on certain
>    media types.  In particular, it is EXPRESSLY FORBIDDEN to use any
>    encodings other than "7bit", "8bit", or "binary" with any composite
>    media type, i.e. one that recursively includes other Content-Type
>    fields.  Currently the only composite media types are "multipart" and
>    "message".  All encodings that are desired for bodies of type
>    multipart or message must be done at the innermost level, by encoding
>    the actual body that needs to be encoded.
>(snip)
>    NOTE ON ENCODING RESTRICTIONS:  Though the prohibition against using
>    content-transfer-encodings on composite body data may seem overly
>    restrictive, it is necessary to prevent nested encodings, in which
>    data are passed through an encoding algorithm multiple times, and
>    must be decoded multiple times in order to be properly viewed.
>    Nested encodings add considerable complexity to user agents:  Aside
>    from the obvious efficiency problems with such multiple encodings,
>    they can obscure the basic structure of a message.  In particular,
>    they can imply that several decoding operations are necessary simply
> 
>    to find out what types of bodies a message contains.  Banning nested
>    encodings may complicate the job of certain mail gateways, but this
>    seems less of a problem than the effect of nested encodings on user
>    agents.

Tb's behavior of "trying to send message/rfc822 part in Base64" is apparent RFC 2045 violation.
This patch depends on the fix for bug 725926.
This bug does not depend on bug 580017. It it not related at all.
No longer depends on: 580017
Well, I made another fix when I wrote the tests, but I have lost the fix..
Attached patch Proposed fix (obsolete) — — Splinter Review 
I recreated the fix.
Assignee: ZaneUJi → hiikezoe
Status: NEW → ASSIGNED
Attachment #613518 - Flags: review?(dbienvenu)
Comment on attachment 613518 [details] [diff] [review]
Proposed fix

redirecting review
Attachment #613518 - Flags: review?(mozilla) → review?(Pidgeot18)
Attachment #613518 - Flags: review?(irving)
Comment on attachment 613518 [details] [diff] [review]
Proposed fix

Review of attachment 613518 [details] [diff] [review]:
-----------------------------------------------------------------

Based on my test, this patch does the right thing when attaching text files that don't end with newlines, so base64 isn't necessary even when the attachment isn't a .eml file.

Looks good to me. I'd prefer to have the spaces added, but if you'd rather not change it, it can be checked in as is. If you do add the spaces you do not need to ask for new reviews.

::: mailnews/compose/src/nsMsgAttachmentHandler.cpp
@@ +315,5 @@
>    if (pPrefBranch)
>      pPrefBranch->GetBoolPref ("mail.file_attach_binary", &forceB64);
>  
>    if (!mMainBody && (forceB64 || mime_type_requires_b64_p (m_type.get()) ||
> +    m_have_cr+m_have_lf+m_have_crlf != 1))

Since we're changing this line, we should put spaces around the arithmetic operators to meet coding standards:

 m_have_cr + m_have_lf + m_have_crlf != 1))
Attachment #613518 - Flags: review?(irving)
Attachment #613518 - Flags: review?(Pidgeot18)
Attachment #613518 - Flags: review+
Hiro can you attach a new patch carry the r+ and set the checking-needed ?
Because Hiro didn't respond for 3 month now, I updated Hiro's last patch with the spaces around the arithmetic operators. I hope thats OK.
Comment on attachment 734408 [details] [diff] [review]
Hiros patch with nit fixed

(In reply to Nomis101 from comment #32)
> Created attachment 734408 [details] [diff] [review]
> Hiros patch with nit fixed
> 
> Because Hiro didn't respond for 3 month now, I updated Hiro's last patch
> with the spaces around the arithmetic operators. I hope thats OK.


Yes !!!
Attachment #734408 - Flags: review+
Attachment #418472 - Attachment is obsolete: true
Attachment #613518 - Attachment is obsolete: true
What about the tests?
https://hg.mozilla.org/comm-central/rev/739363e1ea82
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
Is this resolved without test? Patch is submitted.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: