476400 - Double-clicking deleted attachment breaks ability to send attachments, and this breakage spreads to other Thunderbird users via email (double-click of deleted attachment invokes open dialog, then entry for text/x-moz-deleted is created in mimeTypes.rdf)

Status: RESOLVED FIXED

(MailNews Core :: Attachments, defect)

Description

[Mason Mark]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_6; en-us) AppleWebKit/525.27.1 (KHTML, like Gecko) Version/3.2.1 Safari/525.27.1
Build Identifier: 2.0.0.19 and 3.0b1

If the user double-clicks the icon of a deleted attachment, Thunderbird tries to open it as a real attachment. This can cause two problems, one annoying and one very serious.

The first consequence is that the user's mimeTypes.rdf will become borked, such that the MIME type "text/x-moz-deleted" gets associated with the file type in question. That breaks Thunderbird's ability to send attachments of the file type in question (because they are sent with text/x-moz-deleted, they render as gibberish text in the recipient's Thunderbird, and cannot be interacted with).

But the second and much more serious consequence is that, because of the way text/x-moz-deleted works, this damage to mimeTypes.rdf can spread via email, and gradually cause every Thunderbird installation in a given organization to become unable to send attachments.

This is bug 2 of 3 that are descended from bug 471027. Fixing any of these three bugs would prevent the serious harm described below, although they should all be fixed. 

--- The real-world scenario where I discovered this bug ---
A problem that originated with a single user, and initially seemed like a somewhat minor bug, propagated via email to nearly every Thunderbird installation in a company, causing them all to become unable to send email attachments. Furthermore, the problem re-emerges and re-propagates, even after IT staff go in to repair the damage.

This propagation-via-email aspect is what makes this such a major bug.

Originally filed as bug 471027, the discussion on the bug made it clear that there were actually multiple bugs working together to cause harm. Fixing any one of these bugs would stop the propagation of the bug from machine to machine, so I am filing them separately. 
---




Reproducible: Always

Steps to Reproduce:
(NOTE: I use Excel .xls file as the example here, but it could be any file type.)

1) From Thunderbird instance A, find a message (send yourself one if necessary) with an .xls attachment. Open the message and delete the attachment. 

2) Forward the message with the deleted attachment to Thunderbird instance B.

3) In Thunderbird instance B, double-click the attachment in the received mail. (The attachment will have type "text/x-moz-deleted".)

4) In the resulting open prompt, choose "Open with Microsoft Office Excel (default)" (or choose to open it with some other program), and check the "Do this automatically for files like this from now on".


Actual Results:  
In step 4 above, Thunderbird B creates a bogus entry in mimeTypes.rdf that associates the ".xls" file extension with mimetype "text/x-moz-deleted. This is the incorrect behavior that this bug report reflects.

5) Now Thunderbird B is borked. It will now send .xls attachments with mimetype "text/x-moz-deleted", and this causes many mailers to see the attachment as gibberish inline text. Thunderbird, in particular, will not only be unable to display the message properly, but if the recipient performs the same actions as step 3 above, their Thunderbird will now also be affected in the same way.

6) Each other Thunderbird user to whom Thunderbird B sends an Excel attachment will experience the same problem. In the company where I found this issue, over the course of a few weeks almost everybody had their Thunderbird become non-functional in this way, via opening an attachment file from another user
already affected by this bug.




Expected Results:  
There are multiple bugs at work here, but this one is occurring in step 4, when the deleted attachment is double-clicked. Thunderbird should not treat a deleted attachment as a real attachment and go through its normal routine of trying to open it.



This bug is descended from bug 471027, which was filed when I encountered an entire company network of Thunderbird installations which were gradually breaking each other via email, such that sending attachments no longer worked. Based on the discussion of that bug, it was determined that three different bugs were working in tandem to cause a loss of functionality that could spread via email. Therefore, I am filing separate reports.

The first, related bug  is bug 476133, "Incorrect x-moz-deleted MIME type for sent attachments harms recipient's Thunderbird, and then breakage propagates via email to every Thunderbird on the user's network". That bug points out that it is always an error, and a serious/dangerous error because of how these multiple bugs interact, for Thunderbird to ever send a real attachment with MIME type text/x-moz-deleted. That should always be prevented via a sanity check, even if mimeTypes.rdf or any other mechanism tells TB to use text/x-moz-deleted for a given attachment type.

As noted in bug 471027, fixing any one of these interacting bugs would prevent the most serious consequence, which is the loss of functionality which can propagate from Thunderbird to Thunderbird via email. 

Also, this bug is similar to bug 286454. Basically, the underlying issue is the same: TB should not react to a double-click of a deleted attachment by trying to open it as a real attachment. However, that bug describes different and less serious unwanted behavior that results from this underlying issue. The incorrect behavior described in this bug is more serious, since it can not only break functionality, but also propagate by email and gradually cripple an entire organization's email capabilities (the ability to send each other attachments).

Comment 1

[WADA:World Anti-bad-Duping Agency]

Confirming, based on test result of Bug 471027 Comment #4.
Adding some words in bug summary for ease of search. Sorry for lengthy summary.

By the way, "and this breakage spreads to other Thunderbird users via email" part is Bug 476133, isn't it?
(Please keep "single problem per single bug". And please note that explanation about "how severe for you the problem is" is not neccessary in bug at bugzilla.mozilla.org.)

Comment 2

[Mason Mark]

> By the way, "and this breakage spreads to other Thunderbird users via email"
> part is Bug 476133, isn't it? (Please keep "single problem per single bug".

Well, OK. The way I think of it, both bugs contribute to the damage spreading by email. 

Bug 476133 describes incorrect behavior at the time of sending email. This bug 476400 describes incorrect behavior at the time of double-clicking a deleted attachment.

Both of these bugs have unwanted consequences for the user, but ones which on their own would probably be considered minor.

However, when both of these bugs are present, then the breakage can spread from PC to PC via email. That makes it a major bug.

That is why I refer to the "spreading by email" in both bug reports. Fixing either one of these two bugs would stop the spread-by-email behavior.

Once spread-by-email is gone, I think the other problems are just "normal" bugs, not "major" or "critical".

> And please note that explanation
> about "how severe for you the problem is" is not neccessary in bug at
> bugzilla.mozilla.org.)

What part of this bug report are you referring to? In most software projects I am involved with, it is necessary to explain the reason why an issue is marked "serious" or "critical". 

I am trying to give complete and unambiguous information for these bugs, including both what TB is doing wrong (software/programmer perspective), and then what the consequences are (end user perspective). Sorry if it is a bit verbose.

Comment 3

[WADA:World Anti-bad-Duping Agency]

(Off-Topic)

(In reply to comment #2)
> it is necessary to explain the reason why an issue is marked "serious" or "critical".

There is no need to explain it, because Severity(blocker, critical, major, ...) is clearly defined at bugzilla.mozilla.org.

> I am trying to give complete and unambiguous information for these bugs,

Pointing to Bug 471027 is sufficient, isn't it? I believe you already described/explained problem detail and impact of problem on customers/users clearly/sufficiently/deeply in that bug. (it's one of reasons I re-opened Bug 471027 which you closed as INVALID once.)

Comment 4

[WADA:World Anti-bad-Duping Agency]

(In addition to comment #0)
> If the user double-clicks the icon of a deleted attachment, Thunderbird tries
> to open it as a real attachment.

As written in Bug 471027 Comment #2 & Bug 471027 Comment #2, "Open" option for "deleted attachment" in any menu/context menu is peoperly/successfully disabled(grayed out).

Possibly regression by some changes, because following bug was found.
> Bug 80254 Double-clicking an attachment should ALWAYS bring up dialog
>           asking what to do (open / save), no matter what the extension is
No change is made by Bug 80254, but existence of such request is sometimes indicator of changes by other bug.

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Don't try to open deleted attachments for double click.

Comment 6

[WADA:World Anti-bad-Duping Agency]

Magnus, thanks for taking this bug.

> +      if (target.localName == "listitem" &&
> +          target.attachment.contentType != "text/x-moz-deleted")

I think followings will be required for text/x-moz-deleted.
 1-a. Don't invoke file open dialog for text/x-moz-deleted.
      (menu for it is already disabled. double-click case only. this bug)
 1-b. Don't add attachment of text/x-moz-deleted by Drag&Drop to compose window.
      (sorry but bug is not opened for this issue yet)
 2-a. Don't create mimeTypes.rdf entry for text/x-moz-deleted.
 2-b. If mimeTypes.rdf entry for text/x-moz-deleted is found,
      remove it upon read or save of mimeTypes.rdf.
 3.   Don't use mimeTypes.rdf entry for text/x-moz-deleted upon attaching
      of any attachment's file-extension.

Above 2-a/2-b/3 won't occur if 1-a will be resolved. And problem of 1-b won't produce critical situation if 1-a will be resolved.
However, 1-b is better to be avoided, and 2-b is better to be implemented for victims who already have problem due to 1-a.  

Can you add code for 1-b easily? Should we open separate bug for 1-b?

Hard code of "text/x-moz-deleted" in each code is better to be avoided. So I think black list like definition of wrong mime-type is better.
Magnus, what do you think?
(for 2-a/2-b/3 will be required or not, "black list" is required or not.)

Comment 7

[[e-mail]]

Comment on attachment 361320 [details] [diff] [review]
proposed fix

I do wonder whether it's worth bulletproofing openAttachment instead.

Comment 8

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Yeah, maybe that's safer... 

Also don't allow deleted attachments to be draggged. (The rest is removing tmpurl and wrapping.)

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 361599 [details] [diff] [review]
proposed fix, v2

I've got to start choosing right patches... !!

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Comment 11

[Magnus Melin [:mkmelin]]

(In reply to comment #6)
> Magnus, what do you think?
> (for 2-a/2-b/3 will be required or not, "black list" is required or not.)

I wouldn't think it's worth doing.

Comment 12

[[e-mail]]

Comment on attachment 361600 [details] [diff] [review]
proposed fix, v2

> var attachmentAreaDNDObserver = {
>   onDragStart: function (aEvent, aAttachmentData, aDragAction)
>   {
>     var target = aEvent.target;
>     if (target.localName == "listitem")
>     {
>+      var attachmentContentType = target.getAttribute("attachmentContentType");
Eww, worms and cans spring to mind... we should really be using target.attachment.* here and not set all of these attributes!

>+      var tmpurlWithExtraInfo = attachmentUrl + "&type=" + attachmentContentType +
>+                                "&filename=" + attachmentDisplayName;
I wonder whether the display name needs an encodeURIComponent...

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3

Changed to using target.attachment.xxx and added encodeURIComponent for the display name.

Carrying fwd sr=neil

Comment 15

[Magnus Melin [:mkmelin]]

jminta: r ping

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 362078 [details] [diff] [review]
proposed fix, v3

Joey: is this something you're going to get to any time soon? Or should I try someone else?

Comment 17

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v4

Unbitrotter, carrying fwd sr=neil
Phil, want to take the review?

Comment 18

[Phil Ringnalda (:philor)]

Comment on attachment 375635 [details] [diff] [review]
proposed fix, v4

Looks fine to me, other than indenting a space too deep in

>+    messenger.openAttachment(this.contentType,
>+                              this.url,
>+                              encodeURIComponent(this.displayName),
>+                              this.uri,
>+                              this.isExternalAttachment);

Comment 19

[Magnus Melin [:mkmelin]]

changeset:   2562:00602ae6cd39
http://hg.mozilla.org/comm-central/rev/00602ae6cd39

->FIXED

Comment 20

[WADA:World Anti-bad-Duping Agency]

Checked with next build on MS Win-XP.
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b5pre) Gecko/20090511 Shredder/3.0b3pre
Double-click of deleted-attachment, Drag&Drop to compose window.
=> Nothing happened. (needless to say, all context menu is grayed-out as before.)

VERIFIED on MS Win.

Comment 21

[Phil Lacy]

Comment on attachment 375635 [details] [diff] [review]
proposed fix, v4

>+        var info = attachment.url + "&type=" + attachment.contentType +
>+                   "&filename=" + encodeURIComponent(attachment.displayName);

The attachment.url already has '&filename=...' and it is not escaped per bug 485514. Shall we remove it here or in bug 485514 since this is done deal.

Comment 22

[WADA:World Anti-bad-Duping Agency]

To Mason Mark(bug opener):

Can you reproduce problem with Tb latest-trunk?

Comment 23

[Magnus Melin [:mkmelin]]

Phil: maybe it in bug 485514?