1670791 - Useless empty attachment added for Filelink'ed attachments

Status: RESOLVED FIXED

(Thunderbird :: FileLink, defect, P2)

Description

[Johannes Endres]

Attachment: Error message

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

Steps to reproduce:

1. Add an attachment
2. Upload the attachment to a Filelink provider ("Convert to..." from the context menu)
   => the download link is inserted into the message
3. Send the message

Actual results:

Thunderbird added an empty attachment, containing a X-Mozilla-Cloud-Part header.

In Thunderbird 78.3.2 (64-Bit) on Windows 10 double clicking this attachment shows an error message (I only have the german version).

A user of my Filelink provider Addon tells me that Thunderbird 78.3.2 (64-Bit) on Linux crashes when trying to open the empty attachment.

Expected results:

Thunderbird shouls not add attachments even the same version of Thunderbird can't handle.

or

Thunderbird should handle this kind of attachments.

Comment 1

[Thomas D. (:thomas8)]

This is the translation of the German error message of comment 0:

mail/locales/en-US/chrome/messenger/messenger.properties

381 emptyAttachment=This attachment appears to be empty.\nPlease check with the person who sent this.\nOften company firewalls or antivirus programs will destroy attachments.

Comment 2

[Magnus Melin [:mkmelin]]

IIRC it's added so that you know the message has an attachment (in search etc.)
But it would be much preferable to have something that can be opened. That could be an html file containing showing the links, and maybe an <meta http-equiv="refresh"> redirect to get to the file.

Comment 3

[Johannes Endres]

One might argue that from the receiver's point of view the message doesn't have attachments, but just contains download links. And how do we make clear which "attachment" corresponds to which download file?

And <meta http-equiv="refresh"> might not be a good idea: A phisher could put a legitimate link into the message body and a malicious one into the "attachment". And as a reaction receiving clients might disable this kind of attachments -> bad UX for receivers.

And why click on an attachment if the relevant link is right there in the message?

Comment 4

[Magnus Melin [:mkmelin]]

And how do we make clear which "attachment" corresponds to which download file?

Seems best to add one "attachment" for each filelink. If you attach cv.docx as filelink, add a cv.docx-filelink.html, or something like that. If someone clicks it that opens in the browser which opens the file.

And why click on an attachment if the relevant link is right there in the message?

If we don't have an attachment, then I'd assume some people won't grasp where the "attachment" is...

I wouldn't be too worried about phishing or not. It's up to the user if they want to use the attachment or not. What is clear is the current not open-able attachment is annoying.

Comment 5

[Johannes Endres]

(In reply to Magnus Melin [:mkmelin] from comment #4)

If we don't have an attachment, then I'd assume some people won't grasp where the "attachment" is...

But why would they be looking for an attachment at all? In the message with the FileLink'ed attachment it reads "I've uploaded 1 file...." For the recipient there is no hint that there was an attachment in the sender's message compose window.

I wouldn't be too worried about phishing or not.

Oh, I am! It's my day job.

Comment 6

[Magnus Melin [:mkmelin]]

(In reply to Johannes Endres from comment #5)

But why would they be looking for an attachment at all?

I guess it depends on how one uses it, but seems likely the user would e.g. remember "oh it was attached to this email from x" - be that physically attached or as filelink. It shouldn't really matter how it's technically there. For the user it's an attachment. Personally I would many times just like to skip the links and all that in the message.

Comment 7

[Johannes Endres]

(In reply to Magnus Melin [:mkmelin] from comment #6)

I guess it depends on how one uses it, but seems likely the user would e.g. remember "oh it was attached to this email from x" - be that physically attached or as filelink. It shouldn't really matter how it's technically there. For the user it's an attachment.

I dispute that this is true for the recipient. For a sender it makes sense to think "I attached that to message x" and not to remember if she converted it to FileLink. But the receiver just gets a message with a link in it. There is no reason for her to see this as an attachment. Only users of Thunderbird (and FileLink within Thunderbird) will have the technical knowledge that putting this link into the mail involved adding an attachment on the sender side.

What's more important: PLEASE don't put any magic into the attachment that (tries to) start the download without presenting the link to the recipient and have her confirm it. This would be an open invitation for phishers to combine a legit link in the message body with a malicious download. Thanks.

Comment 8

[Elhem Enohpi]

I used FileLink for the first time this week, and found the psuedo-attachment to be unexpected. I even mentioned it in a bug report for the Thunderbird-Send addon.

People are familiar with WeTransfer, DropBox, etc. to transmit large files, and just putting a link in the e-mail. It's not really an "attachment". I wouldn't expect there also to be an attachment, especially an empty one called "part 1.2" that doesn't seem related. I thought it was a bug. I was worried that the recipient would be confused by having both a link in the e-mail body to download a file, and an attachment to the message, which is not actually the file. I certainly was.

Even if you change it so that the attachment opens the download link, it's still confusing and unexpected. There's already a link in the e-mail, so what is this attachment? Is it something different? Do I need to get them both?

My suggestion would be to keep it simple, and just put the link in the e-mail, as people are already used to with other large-file transfer services. If there's a file attachment to the message, I expect it to be a file that I can save to disk, not a download link I need to open. A pseudo-attachment in addition to a download link I think is unconventional and not useful enough to balance the confusion that it may cause.

Thanks for considering this...

Comment 9

[John Bieling (:TbSync)]

How about this:

1. Let's keep the fake attachment on Drafts, so we can continue to rename/delete/convert them. Let's give them a proper name in the preview area, by adding name="..." to the content-type header of the fake attachment.
2. Let's remove the fake attachment on outgoing mail.

I would do that here:
https://searchfox.org/comm-central/rev/e441dd8b6f04570c5b9529ed5a6cce43efe8770e/mailnews/compose/src/MimeMessage.jsm#382

by looking at the delivery mode and only keep the attachment, if the mode is Ci.nsIMsgCompDeliverMode.SaveAsDraft,
Ci.nsIMsgCompDeliverMode.AutoSaveAsDraft or Ci.nsIMsgCompDeliverMode.SaveAsTemplate.

The other modes (where I would discard the fake attachments) are Now, Later, Save, SaveAs, SendUnsent and Background

There are a lot of default actions associated with file types in the preview area. When using the proper names for fake attachments, a linked PNG file would allow (and fail) to open it. To get around that, we could append ".filelink" to the name of fake attachments (and also disable attachment detachment/deletion?).

I have not yet found where I could manipulate the icon of the fake attachment in the preview area. For consistency, we should use the same cloudFileIcon we use in the composer. Does someone know where that could be done?

Or should we hide fake attachments on drafts in the preview altogether?

Comment 10

[Magnus Melin [:mkmelin]]

Especially if you send both filelink and other attachments, I think it's confusing to have an "attachment" disappear from the attachment area.
Also for receiving files, you would not be able to find emails with "attachments".

I don't understand the reasoning above regarding scams or what not. I still think just making it an html file with proper linking inside would solve all possible issues. Many times you actually don't want the template text included in the email at all, and then the attachments - when implemented like this - would be a completely working solution I think.

Comment 11

[S.Brooks]

I do send both attachments and filelinks. I value the psuedo-null-attachment because it enables me to sort my sent emails by attachment to find the emails where I sent someone a file (often so I can forward the same email to a different user to provide them with the same content). I don't care if it is actually an attachment or a cloud download link - the important thing for me is that I can sort my emails by attachment to quickly locate individual emails in which I provided documents regardless of the mode of transport (attachment or download link). If the pseudo attachment was removed I'd still want the "has attachment paperclip flag" to be ON in the send folder somehow

Comment 12

[John Bieling (:TbSync)]

Fixed by Bug 942615

Comment 13

[John Bieling (:TbSync)]

Edited the wrong bug tab. This is of course still being worked on.

Comment 14

[John Bieling (:TbSync)]

We will keep the pseudo attachment, but as suggested in Comment 2, I will add an HTML file instead of the zero-size-no-name attachment, which has a clickable link. Any suggestions how to craft the file name?

Just original.txt -> original.txt.html or something which indicates it is a info/placeholder file, like original.txt_cloudLink.html ?

Comment 15

[Magnus Melin [:mkmelin]]

Just adding .html to the filename seems pretty standard (otherwise l10n complications).

Comment 16

[John Bieling (:TbSync)]

Attachment: Bug 1670791 - Replace nameless zero-size cloudFile attachment by a small html info attachment. r=mkmelin

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9eb234b212ad
Replace nameless zero-size cloudFile attachment by a small html info attachment. r=mkmelin

Comment 18

[John Bieling (:TbSync)]

Attachment: Bug 1670791 - Fix minor spelling errors. r=#thunderbird-reviewers

Comment 19

[Magnus Melin [:mkmelin]]

comm/mailnews/compose/test/unit/test_messageHeaders.js is apparently failing due to this

Comment 20

[John Bieling (:TbSync)]

Due to the first or second patch? The first patch has passed this try run:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=0e4057029fed0846cbc0df3e8087ad7984eb1f21

The second one does not change code.

I really do not understand our try system. I will check locally. Is it failing for you locally? Linux?

Comment 21

[John Bieling (:TbSync)]

Oh no, my try run covered only mochitests! I will create a fix as soon as possible.

Comment 22

[Magnus Melin [:mkmelin]]

The landed one yes, it fails locally as well.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8ef84b85773f
Fix minor spelling errors. r=mkmelin

Comment 24

[John Bieling (:TbSync)]

Attachment: Bug 1670791 - Fix unit tests. r=mkmelin

Comment 25

[newsfan]

Drive-by comment: By "re-open" draft you mean "edit draft" (which was the common nomenclature so far)? There is another way to "re-open" a draft, that is "edit as new message" (leads to the a new draft ID and a new draft being saved). Looks like templates are also covered now (https://hg.mozilla.org/comm-central/rev/9eb234b212ad#l21.22). So did you test "edit template" and "new message from template" as well as "edit as new message" on a draft?

Comment 26

[newsfan]

And comment #9 mentions Ci.nsIMsgCompDeliverMode.AutoSaveAsDraft which is not in the patch after all.

Comment 27

[John Bieling (:TbSync)]

Comment #9 is obsolete, as we decided to not remove the attachment. We only have to test against Ci.nsIMsgSend.* and not against Ci.nsIMsgCompDeliverMode.*.

• https://searchfox.org/comm-central/rev/3a69ba6ee7510697701bd515eca869b2c74bbff0/mailnews/compose/public/nsIMsgSend.idl#159-169
• https://searchfox.org/comm-central/rev/3a69ba6ee7510697701bd515eca869b2c74bbff0/mailnews/compose/public/nsIMsgCompose.idl#43

As you can see, Ci.nsIMsgSend.AutoSaveAsDraft does not exist. Comment #9 probably was plain wrong anyhow.

Regarding your drive-by comment: They all use the same code path when opening a message compose window. And sorry for disturbing you with using the wrong nomenclature. I use "re-opened" draft in 3 comments where I think it is appropriate and helps to understand what the code is doing.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b2970637ff0d
Fix unit tests. r=mkmelin

Comment 29

[Johannes Endres]

This seems to cause more problems, see bug 1830941