Attachment downloaded as base64 encoded file when not multipart (no email body)

RESOLVED FIXED in Thunderbird 67.0

Status

defect
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: raztus, Assigned: infofrommozilla)

Tracking

Thunderbird 67.0

Thunderbird Tracking Flags

(thunderbird_esr6067+ fixed, thunderbird66 fixed, thunderbird67 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

5 months ago

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

Received this email in Thunderbird with only an attachment (no message body):

Content-Type: text/plain; name=test.txt
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename=test.txt
From: from@localhost.com
To: me@mydomain.com
Subject: Test
Message-ID: <c7a2664e-6803-c144-1c70-96dc0941e3a1@localhost.com>
Date: Mon, 04 Feb 2019 19:35:59 +0000
MIME-Version: 1.0

VGhpcyBpcyB0aGUgdGV4dApmaWxlIDMu

Actual results:

The inline-display of the attachment (when enabled) displayed the text file correctly. However, once opened from a temp file, or downloaded elsewhere, the file "test.txt" contained the base64 encoded value.

Expected results:

The file "test.txt" should have contained the base64 decoded as plain-text.

Reporter

Comment 1

5 months ago

Related to Bug 1361422.

Comment 2

5 months ago

Alfred, could you please take a look.

Flags: needinfo?(infofrommozilla)
Assignee

Comment 4

5 months ago

https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/mime/src/mimeleaf.cpp#96

      // If we need the object as "raw" for saving or forwarding,
      // don't decode text parts of message types. Other output formats,
      // like "display" (nsMimeMessageBodyDisplay), need decoding.
      (obj->options->format_out == nsMimeOutput::nsMimeMessageRaw &&
       obj->parent &&
       (!PL_strcasecmp(obj->parent->content_type, MESSAGE_NEWS) ||
        !PL_strcasecmp(obj->parent->content_type, MESSAGE_RFC822)) &&
       !PL_strncasecmp(obj->content_type, "text/", 5)))

That begs me the question, what is the purpose of this code?
When do we need the coded form of an attachment?
An example for testing would be helpful?

Comment 5

5 months ago
Posted file Fwd Test.eml

Hmm, all good questions. Doesn't the comment suggest that we need the un-decoded for for forwarding, for example. I'll try this with the patch. Yes, we're lacking test coverage in this area.

I tried the patch. Yes, with the message in comment #0 it works, but it doesn't work, when it's forwarded. You get the enclosed message and the attachment doesn't open correctly.

Comment 6

5 months ago
Comment on attachment 9042870 [details] [diff] [review]
Decode  attachment (base64/qp) from not multipart email, when saved.Bug1525120.patch

Works in the case presented here, but now when the message is forwarded. Would you like to investigate a little further?
Attachment #9042870 - Flags: review?(jorgk) → feedback+
Assignee

Comment 7

5 months ago

(In reply to Alfred Peters from comment #4)

      // If we need the object as "raw" for saving or forwarding,

[...]

That begs me the question, what is the purpose of this code?

I do remember: Bug 1350080

When do we need the coded form of an attachment?
An example for testing would be helpful?

attachment 8850706 [details]

(In reply to Jorg K (GMT+1) from comment #6)

Works in the case presented here, but now when the message is forwarded.

Now obj->options->is_child is also TRUE and we are back at the beginning.

Would you like to investigate a little further?

Sure, of course. In fact we need a better solution for Bug 1350080.

Assignee

Comment 8

4 months ago

The example form comment 0 and attachment 8850706 [details] are really hard to keep apart.

The real point is: when should we not decode the body?
Answer: if we save it together to a Content-type: header.

The solution is to recognize this case as correctly as possible.

obj->parent->output_p should indicate something has been written from the parent object. This could only have been the headers.

Attachment #9042870 - Attachment is obsolete: true
Attachment #9044508 - Flags: review?(jorgk)

Comment 9

4 months ago
Comment on attachment 9044508 [details] [diff] [review]
Do not decode attachments (base64/qp), if they are saved with headers.

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

This works for the cases connected to the bug. Here's a try run, we're lucky, since last week's big bustage is over.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d71e80e04e75a85330fcafdd04d33c558b661aee

::: mailnews/mime/src/mimeleaf.cpp
@@ -101,3 @@
>         (!PL_strcasecmp(obj->parent->content_type, MESSAGE_NEWS) ||
> -        !PL_strcasecmp(obj->parent->content_type, MESSAGE_RFC822)) &&
> -       !PL_strncasecmp(obj->content_type, "text/", 5)))

What was this doing and why is it OK to remove? Can you give an example?
Attachment #9044508 - Flags: review?(jorgk) → feedback+
Assignee

Comment 10

4 months ago

(In reply to Alfred Peters from comment #8)

The real point is: when should we not decode the body?
Answer: if we save it together to a Content-type: header.

Content-Transfer-Encoding: header of cause.

(In reply to Jorg K (GMT+1) from comment #9)

Comment on attachment 9044508 [details] [diff] [review]
Do not decode attachments (base64/qp), if they are saved with headers.

Review of attachment 9044508 [details] [diff] [review]:

This works for the cases connected to the bug.

I tested it with the test cases from Bug 1350080 Bug 1361422 and this bug of cause.
I tested view, save and forward the attachments/mails.

::: mailnews/mime/src/mimeleaf.cpp
@@ -101,3 @@

    (!PL_strcasecmp(obj->parent->content_type, MESSAGE_NEWS) ||
  •    !PL_strcasecmp(obj->parent->content_type, MESSAGE_RFC822)) &&
    
  •   !PL_strncasecmp(obj->content_type, "text/", 5)))
    

What was this doing and why is it OK to remove? Can you give an example?

That's from the fix to the first regression (Bug 1361422) from Bug 1350080.

This doesn't work because a TXT file and an email both have content-type 'text/*'

The Content-type: header also does not matter. No matter what we save, if we do that with Content-Transfer-Encoding: base64 or quoted-printable, then we need the encoded content.

Comment 11

4 months ago

I'll look at it again tonight.

You're aware of our new preference implemented here, right?
https://hg.mozilla.org/comm-central/rev/43c9c600cf84#l1.17
I needed to switch this of get the inline display.

Assignee

Comment 12

4 months ago

(In reply to Jorg K (GMT+1) from comment #11)

You're aware of our new preference implemented here, right?
https://hg.mozilla.org/comm-central/rev/43c9c600cf84#l1.17

Already since last week.
At first I wondered about the new option in the debugger 🤔.
Then I wondered why the view did not work anymore 😳.
And then came the aha effect 🙄.

Comment 13

4 months ago

OK, tested with ...

All still working, try run is green, so let's go with this.

Doing the testing I noticed, painfully, that we still haven't fixed the issue that the mailbox name will be used as file name in come cases (mentioned in bug 1350080 comment #1) and bug 1352740 for some tests. I've filed bug 1528932 for the former.

Comment 14

4 months ago
Comment on attachment 9044508 [details] [diff] [review]
Do not decode attachments (base64/qp), if they are saved with headers.

We can take this to the next beta, but given the brittleness of MIME, I won't be in a hurry to go to ESR with this.
Attachment #9044508 - Flags: review+
Attachment #9044508 - Flags: approval-comm-beta+

Comment 15

4 months ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/59f55da59da5
Fix condition for decoding attachments (base64/qp) when saving or forwarding. r=jorgk

Status: UNCONFIRMED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED

Comment 16

4 months ago

I've been agonising over the commit message. The suggestion "Do not decode attachments (base64/qp) if they are saved with headers" didn't sound right since attachments are typically not saved with headers, that's the whole point. They may be processed with headers for forwarding.

Can you enlighten me where output_p is set and what it means. Yes, I should have asked earlier. Maybe we should risk a follow-up and add some comments.

Target Milestone: --- → Thunderbird 67.0

Updated

4 months ago
Assignee: nobody → infofrommozilla
Component: Untriaged → MIME
Product: Thunderbird → MailNews Core

Comment 18

4 months ago

Alfred, please answer comment #16.

Flags: needinfo?(infofrommozilla)
Assignee

Comment 19

4 months ago

(In reply to Jorg K (GMT+1) from comment #16)

Can you enlighten me where output_p is set and what it means. Yes, I should have asked earlier. Maybe we should risk a follow-up and add some comments.

It is expensive to determine that. Man has to keep manual book, about the object addresses.

In which case exactly do you want to know that?

For saving a TXT attachment, this is done here:

https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/mime/src/mimeobj.cpp#220

Flags: needinfo?(infofrommozilla)

Comment 20

4 months ago

OK, let's drop the "where output_p is set" and concentrate on "what it means". What does it mean?

Assignee

Comment 21

4 months ago

(In reply to Jorg K (GMT+1) from comment #20)

OK, let's drop the "where output_p is set" and concentrate on "what it means". What does it mean?

Hard to say.

  1. See Comment 8

  2. In which case exactly do you want to know that?
    For saving a TXT attachment:
    https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/mime/src/mimeobj.cpp#208

  3. http://geek-and-poke.com/geekandpoke/2009/7/25/the-art-of-programming-part-2.html

Sorry because i could not be helpful.

Comment 22

4 months ago

I read comment #8. I'd just like to enhance the comment here
https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/mailnews/mime/src/mimeobj.h#161
and where we use is now.

Since our decoding logic depends on it now, it would be good to have a better understanding.

Assignee

Comment 23

4 months ago

(In reply to Jorg K (GMT+1) from comment #22)

I read comment #8. I'd just like to enhance the comment here
https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/mailnews/mime/src/mimeobj.h#161
and where we use is now.

I see.

  • bool output_p; /* Whether it should be written. */
  • bool output_p; /* Whether it should be written.
  •                     If it is the parent of an encoded (base64/qp)
    
  •                     attachment, this flag shows, if the headers of that
    
  •                     attachment are already written. The flag therefore
    
  •                     decides whether the attachment must be written
    
  •                     encoded. */
    

Comment 24

4 months ago

I tried to add a comment here and it occurred to me that we can drop the special casing for message attachments, no? I've done some indentation fixes as well.

Attachment #9046122 - Flags: review?(infofrommozilla)
Assignee

Comment 25

4 months ago
Comment on attachment 9046122 [details] [diff] [review]
1525120-follow-up.patch

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

Yes, on closer consideration that should be enough.
Attachment #9046122 - Flags: review?(infofrommozilla) → review+

Comment 26

4 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fd2a34cf700c
Follow-up: Drop special casing for message attachments. r=AlfredPeters

Comment 28

4 months ago
Comment on attachment 9044508 [details] [diff] [review]
Do not decode attachments (base64/qp), if they are saved with headers.

Let's consider this for TB 60.7 if it goes well on TB 66 and 67 beta. No need to rush MIME changes ;-)
Attachment #9044508 - Flags: approval-comm-esr60?

Updated

3 months ago
Attachment #9044508 - Flags: approval-comm-esr60? → approval-comm-esr60+
You need to log in before you can comment on or make changes to this bug.