Closed Bug 1525120 Opened 5 years ago Closed 5 years ago

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

Categories

(MailNews Core :: MIME, defect)

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

Tracking

(thunderbird_esr6067+ fixed, thunderbird66 fixed, thunderbird67 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 67.0
Tracking Flags:
Tracking Status
thunderbird_esr60 67+ fixed
thunderbird66 --- fixed
thunderbird67 --- fixed

People

(Reporter: raztus, Assigned: infofrommozilla)

Details

Attachments

(3 files, 1 obsolete file)

Fwd Test.eml
1.49 KB, text/plain
Details
Do not decode attachments (base64/qp), if they are saved with headers.
1.46 KB, patch
jorgk-bmo
: review+
jorgk-bmo
: feedback+
jorgk-bmo
: approval-comm-beta+
jorgk-bmo
: approval-comm-esr60+
Details | Diff | Splinter Review
1525120-follow-up.patch
2.12 KB, patch
infofrommozilla
: review+
Details | Diff | Splinter Review

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.

Related to Bug 1361422.

Alfred, could you please take a look.

Flags: needinfo?(infofrommozilla)

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?

Attached 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 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+

(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.

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 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+

(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.

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.

(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 🙄.

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 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+

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: 5 years ago
Resolution: --- → FIXED

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
Assignee: nobody → infofrommozilla
Component: Untriaged → MIME
Product: Thunderbird → MailNews Core

Alfred, please answer comment #16.

Flags: needinfo?(infofrommozilla)

(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)

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

(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.

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.

(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. */

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)
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+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fd2a34cf700c
Follow-up: Drop special casing for message attachments. r=AlfredPeters
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?
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.

Attachment

General

Creator:
Created:
Updated:
Size: