1255071 - fowarding text/html attachment with lines longer than 900 characters leads to base64 encoding. Original CTE should just be handed on.

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Wayne Mery (:wsmwk)]

+++ This bug was initially created as a clone of Bug #1225904 +++

For an email containing html...

Content-Type: text/html;
	charset="us-ascii"

... forward "Message as attachment", is sent/received unintelligible.  THunderbird shows the message as 

Subject: AllegiantAir.com - Itinerary #S836582
From: "Allegiant Travel Company" ....
To: ...

PCFET0NUWVBFIGh0bWw+DQogICAgPGh0bWw+DQoNCiAgICANCiAgICA8aGVhZD4NCiAgICAg 


45.0a1 2015-12-04 works, 2015-12-05 does not. http://hg.mozilla.org/comm-central/pushloghtml?startdate=2015-12-04+03%3A05%3A00&enddate=2015-12-05+03%3A05%3A00  so regression of bug 1225904. I'll foward the email to jorg


Prior to bug 1225904, attached message is sent as 

Content-Type: message/rfc822;
 name="AllegiantAir_com - Itinerary #S836582.eml"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="AllegiantAir_com - Itinerary #S836582.eml"

After bug 1225904, attached message is sent as 

Content-Type: message/rfc822;
 name="AllegiantAir_com - Itinerary #S836582.eml"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="AllegiantAir_com - Itinerary #S836582.eml"

Comment 1

[Jorg K (CEST = GMT+2)]

Can you please attach the sample that fails.

Comment 2

[Wayne Mery (:wsmwk)]

(In reply to Jorg K (GMT+1) from comment #1)
> Can you please attach the sample that fails.

emailed to jorgk

Are testcases possible?

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: Test e-mail showing the problem

What happens here is very easy to understand. In bug 1225904 I changed the behaviour of e-mail sending. If the body or attachment of an e-mail is longer than 900 characters, it will be base64 encoded. Before we had some terrible code that tried to chop up long lines. That we don't do any more.

The sample e-mail has a HTML body with a line that's 980 characters line.

According to RFC2822 the line length must not be longer than 1000 characters, including the CRLF, so 998 net characters. Just to be on the safe side, we made it 900 characters.

So the fact that the attachment was sent as base64 is well understood.

(To be continued with the next attachment).

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: Here is the sent message.

When the message is sent, we get these headers:

Content-Type: multipart/mixed;
 boundary="------------173C12B971CF9C605E7BDCFD"

This is a multi-part message in MIME format.
--------------173C12B971CF9C605E7BDCFD
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit



--------------173C12B971CF9C605E7BDCFD
Content-Type: message/rfc822;
 name="Itinerary.eml"
Content-Transfer-Encoding: base64 <===== RIGHT
Content-Disposition: attachment;
 filename="Itinerary.eml"

X-Mozilla-Keys:                                                                                 
From: "Travel Company" <[e-mail]>
To: <[e-mail]>
Subject: Itinerary
Date: Wed, 10 Feb 2016 16:45:36 -0600
MIME-Version: 1.0
Content-Type: text/html;
	charset="us-ascii"
Content-Transfer-Encoding: 7bit <======= WRONG!!

PCFET0NUWVBFIGh0bWw+DQo8aHRtbD4NCjxib2R5Pg0KICAgICAgICANCjxpPkluIGFjY29y
ZGFuY2Ugd2l0aCBGQUEvVFNBIFNlY3VyaXR5IERpcmVjdGl2ZXMsIHBhc3NlbmdlcnMgYXJl

Looks like we ran into a MIME problem here.

The CTE is noted as base64 for the attachment, but is noted as 7bit for the message. That's wrong.

If I change the 7bit to base64 manually, and I won't attach that, you can take my word for it, it is displayed correctly.

So the fix is finding the spot were the CTE is set incorrectly and fix it.

Comment 5

[Jorg K (CEST = GMT+2)]

I think that's worth tracking in TB 45. Fix shouldn't be too hard and can be included in ESR45.

Lifting the limit to 998 characters is not an option, I think.

Wayne, thanks a lot for finding this, good job ;-)

Comment 6

[Jorg K (CEST = GMT+2)]

Note: Creating a new message with the long-line-message as attachment suffers the same problem.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1)

Magnus, we broke it together, so let's fix it together ;-)

In comment #4 I said I wanted to correct the second header which belongs to the attached message.
Content-Transfer-Encoding: base64 <===== RIGHT
Content-Transfer-Encoding: 7bit <======= WRONG!!

I've looked at it for a while and found that we can't do that, since the attached message is just handed on.

So instead I now don't encode "message/rfc822" attachment as base64. We simply rely on/hope that their lines are already right.

Note that Joshua already predicted this bug here in bug 1225904 comment #17 (quote):
===
> +  // According to RFC 821 we must always have lines shorter than 998 bytes.
This is possibly going to do bad things if the message/ or multipart/ type parts happen to violate this condition. [...] ... message/rfc822 is probably going to hit this path if you try to include a .eml file from the disk as an attachment.
===

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1b)

Actually, increase the arbitrary limit from 900 to 990, that's still a few bytes below the 998. Fix whitespace issues.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 8728946 [details] [diff] [review]
Proposed solution (v1b)

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

Could you add a test for it? Looks like test-eml-actions.js gives you almost all things needed.

::: mailnews/compose/src/nsMsgAttachmentHandler.cpp
@@ +457,5 @@
> +
> +  // We don't do this for message/rfc822 attachments, since we can't
> +  // change the original Content-Transfer-Encoding of the message we're
> +  // attaching. We rely on the original message complying with RFC 821,
> +  // if it doesn't we won't either. Not ideal.

base64 encoding isn't reallly allowed there anyway... 
https://tools.ietf.org/html/rfc2046#section-5.2.1

   No encoding other than "7bit", "8bit", or "binary" is permitted for
   the body of a "message/rfc822" entity.

@@ +458,5 @@
> +  // We don't do this for message/rfc822 attachments, since we can't
> +  // change the original Content-Transfer-Encoding of the message we're
> +  // attaching. We rely on the original message complying with RFC 821,
> +  // if it doesn't we won't either. Not ideal.
> +  if (!m_type.EqualsLiteral(MESSAGE_RFC822) && m_max_column > 990 && !isUsingQP)

should compare case insensitively, unless you're sure it's normalized earlier somewhere

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #9)
> Could you add a test for it? Looks like test-eml-actions.js gives you almost
> all things needed.
Sure, but what do you want the test to do?
Forward the sample message with the 980-character long line and make sure it's legible when it arrives?
Do we really need such a test? We can't have a test for every half-line of code that someone can possibly change. We understand why this was caused and we fixed it.

Anyway, easy enough to copy/paste a test together if you tell me what you want to see. My test-forward-utf8.js also derived from test-eml-actions.js is an easy source.

Well, the test may be good for when Joshua turns this upside down ;-)

Comment 11

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+1) from comment #10)
> (In reply to Magnus Melin from comment #9)
> > Could you add a test for it? Looks like test-eml-actions.js gives you almost
> > all things needed.
> Sure, but what do you want the test to do?
> Forward the sample message with the 980-character long line and make sure
> it's legible when it arrives?
> Do we really need such a test? We can't have a test for every half-line of
> code that someone can possibly change. We understand why this was caused and
> we fixed it.

Look at it this way: had such a test existed before this bug wouldn't have occurred. If we don't add a test it's quite likely a rewrite or other change might break it again. 

I think the test would forward-as-attachment, then test that the result has the expected (readable) text included. Quote similar to the test_forward_base64_eml function in that file.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1b) with new test.

Here's the test, I did a great cut&paste job ;-)

Note that this doesn't need to be forwarded from a folder, it fails/works from a saved message.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1b) with new test (v1b).

I've changed my mind and made the line in the test 998 characters long. This will at least test the half-line change I made:

+  if (!m_type.LowerCaseEqualsLiteral(MESSAGE_RFC822) &&
+      m_max_column > 990 && !isUsingQP)

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 8729810 [details] [diff] [review]
Proposed solution (v1b) with new test (v1b).

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

::: mail/test/mozmill/composition/long-html-line.eml
@@ +10,5 @@
> +<!DOCTYPE html>
> +<html>
> +<body>
> +
> +We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. We like writing long lines. This is 998 long.

Why did you make it 998 long exactly? Yes the implementation now would handle it properly since it's looking for 990, but if a new implementation actually used the max (998) the test would be wrong, no?
(I don't really like the 990 either, we could use the real 998 instead of kind of random magic number.)

::: mail/test/mozmill/composition/test-forward-rfc822-attach.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * Tests that UTF-8 messages are correctly forwarded.
> + */

Too much copy paste?

@@ +7,5 @@
> + */
> +
> +// mozmake SOLO_TEST=composition/test-forward-rfc822-attach.js mozmill-one
> +
> +var MODULE_NAME = "test-forward-utf8";

and here

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1b) with new test (v1c).

I've fixed the copy/paste errors.

> Why did you make it 998 long exactly? Yes the implementation now would
> handle it properly since it's looking for 990, but if a new implementation
> actually used the max (998) the test would be wrong, no?
> (I don't really like the 990 either, we could use the real 998 instead of
> kind of random magic number.)

As I tried to say before, there is no "right" test. You wanted to test that message attachments are not base64 encoded, so that's what the test does.

I chose a number greater than 990 so without the type-check, it would trigger the base64 encoding.

I chose 998 since this is the largest legal value.

990 is arbitrary as 900 was arbitrary before. It doesn't really matter as long as e-mail with long lines does get base64-encoded, unless it's rfc822.

The 900 was picked from
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgAttachmentHandler.cpp#397
and you signed off on it ;-)

Maybe better to change that, too, for consistency.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1c) with new test (v1c).

Stuck the "magic number" into a define. Look, there was a comment talking about 900 ;-)

Comment 17

[Jorg K (CEST = GMT+2)]

Magnus, if you want other magic numbers, please let me know.
IMHO having 990 consistently in the code and 998 in the test is fine.

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 8729881 [details] [diff] [review]
Proposed solution (v1c) with new test (v1c).

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

r=mkmelin

::: mail/test/mozmill/composition/test-forward-rfc822-attach.js
@@ +62,5 @@
> +  let sis = Cc["@mozilla.org/scriptableinputstream;1"]
> +              .createInstance(Ci.nsIScriptableInputStream);
> +  sis.init(streamListener.inputStream);
> +  const MAX_MESSAGE_LENGTH = 65536;
> +  return sis.read(MAX_MESSAGE_LENGTH);

should probably sis.close() after

::: mailnews/compose/src/nsMsgSend.h
@@ +49,5 @@
>  
>     - it is of a non-text type (in which case we will use base64); or
>     - The "use QP" option has been selected and high-bit characters exist; or
>     - any NULLs exist in the document; or
> +   - any line is longer than 990 bytes.

- and it's not of type message/rfc822

@@ +139,5 @@
>  //
>  #define TEN_K                 10240
>  #define MIME_BUFFER_SIZE      4096 // must be greater than 1000
>                                     // SMTP (RFC821) limit
> +#define LINELENGTH_ENCODING_THRESHOLD 990

please add documenting comment

Comment 19

[Jorg K (CEST = GMT+2)]

Thanks!

(In reply to Magnus Melin from comment #18)
> > +  return sis.read(MAX_MESSAGE_LENGTH);
> should probably sis.close() after
I've copied that from here:
https://dxr.mozilla.org/comm-central/source/mail/test/mozmill/composition/test-charset-upgrade.js#101
https://dxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_detachToFile.js#124
and two more. No one closes. What to do?

Comment 20

[Magnus Melin [:mkmelin]]

Feel free to add it there too. Streams do like to get closed when not used :)  
Not *that* important for tests but i assume unclosed streams could eventually cause problems.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1d) with new test (v1d).

Added close() to test and fixed comments in nsMsgSend.h

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1d) with new test (v1d).

(added missing closing parenthesis in comment)

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8730022 [details] [diff] [review]
Proposed solution (v1d) with new test (v1d).

[Approval Request Comment]
Regression caused by (bug #): Bug 1225904
User impact if declined: Some messages cannot be attached or forwarded as attachment.
Testing completed (on c-c, etc.): Manual and with new test.
Risk to taking this patch (and alternatives if risky):
Low. Only added test for message/rfc822 attachment in one spot.

Comment 24

[Jorg K (CEST = GMT+2)]

Landed on Aurora (TB 47) for further testing and because we need it for TB 45.
https://hg.mozilla.org/releases/comm-aurora/rev/5dc3ace8c027

C-C is currently closed.

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1d) with new test (v1d) and updated long line test.

Since we increased the "long line" limit from 900 to 990 one test failed.
Since 400 Japanese characters come to more than 900 but less than 990 bytes, I increased the test to 450 Japanese characters.

Comment 26

[Jorg K (CEST = GMT+2)]

Aurora (TB 47):
https://hg.mozilla.org/releases/comm-aurora/rev/2699d102dc00 - backout
https://hg.mozilla.org/releases/comm-aurora/rev/d96d1ff6d19d - relanded with fixed test.
Sorry about that ;-(

Comment 27

[Jorg K (CEST = GMT+2)]

I'll land it on C-C myself using a transplant from Aurora.

Comment 28

[Lawrence Mandel [:lmandel] (use needinfo)]

AFAICT Firefox is not affected. Dropping status flag.

Comment 29

[Kent James (:rkent)]

Comment on attachment 8730127 [details] [diff] [review]
Proposed solution (v1d) with new test (v1d) and updated long line test.

http://hg.mozilla.org/releases/comm-beta/rev/f19c1ba0792a
http://hg.mozilla.org/releases/comm-esr45/rev/02a2f33d611d

Comment 30

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/3c244d6d2812

Comment 31

[Jorg K (CEST = GMT+2)]

Grrrr. I grabbed the wrong changeset from Aurora to transplant (comment #24).
Now I transplanted the right one from comment #26:
https://hg.mozilla.org/comm-central/rev/601b90ee6cc7 - backout
https://hg.mozilla.org/comm-central/rev/b0835f56f7b3 - relanded.