Closed Bug 1276419 Opened 8 years ago Closed 8 years ago

embedded image with source URL with ? leads to corrupt src="mailbox:///.../Drafts?number=..." in a composition. (This gets sent without being converted to reference via CID).

Categories

(MailNews Core :: Composition, defect)

Product:
MailNews Core
Component:
Composition
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(thunderbird49 fixed, thunderbird50 fixed, thunderbird_esr4548+ fixed, thunderbird51 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 51.0
Tracking Flags:
Tracking Status
thunderbird49 --- fixed
thunderbird50 --- fixed
thunderbird_esr45 48+ fixed
thunderbird51 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

(Keywords: dataloss, Whiteboard: [dupetome])

Attachments

(3 files, 7 obsolete files)

Test message showing the problem
1.46 KB, text/plain
Details
Much simpler test message showing the problem
414 bytes, message/rfc822
Details
Killer messages - adieu!! (v3b) - same as v3a without the print.
2.58 KB, patch
jorgk-bmo
: review+
jorgk-bmo
: approval-comm-aurora+
jorgk-bmo
: approval-comm-beta+
rkent
: approval-comm-esr45+
Details | Diff | Splinter Review 
In bug 1216914 we came across messages which contain
img src="mailbox:///.../Drafts?number=..."

One example is answering a message that contains:
<img src=3D"https://www.amazon.de/gp/r.html?C=3D3FB5FYNAY98WO&R=3D3MHCT=
GXCZTDTX&T=3DE&U=3Dhttps%3A%2F%2Fimages-eu.ssl-images-amazon.com%2Fimages%2=
FG%2F01%2Fnav%2Ftransp.gif&A=3DBF66D29TTSPDIYLGNA4KSWIVXLAA&H=3DAXIHIYWBCBU=
UPCNNDOYD01CYHC4A" />

In the sent answer we find:
<img
src="mailbox:///D:/MAIL-THUNDERBIRD/jorgk.default/Mail/jazz.jorgk.com/Drafts?number=10&amp;C=3FB5FYNAY98WO&amp;R=3MHCTGXCZTDTX&amp;T=E&amp;U=https%3A%2F%2Fimages-eu.ssl-images-amazon.com%2Fimages%2FG%2F01%2Fnav%2Ftransp.gif&amp;A=BF66D29TTSPDIYLGNA4KSWIVXLAA&amp;H=AXIHIYWBCBUUPCNNDOYD01CYHC4A"
        moz-do-not-send="true">

Sadly I can't reproduce it now, but I have proof that it happened ;-)
STR:
Import test message.
Reply to the text message.
Type something, save draft, type some more, save draft.
Upon second save of draft a broken image placeholder shows.
Sent the message (or send later).
In the sent message we find:

<img src="mailbox://.../Drafts?number=116 ...

This should not have been sent (this is a "killer" message which will delete Drafts.msf when viewed, see bug 1216914).
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Summary: img src="mailbox:///.../Drafts?number=..." gets inserted into a composition and sent. → img src="mailbox:///.../Drafts?number=..." in a composition gets sent without being converted to reference via CID
OK, I removed the mime parts and stripped down to this:

====
Content-Type: text/html; charset=windows-1252
Content-Transfer-Encoding: 7bit

<html>
<body>
  <img src="https://www.amazon.de/gp/r.html?C=xx.jpg">
</body>
</html>
====

What triggers the problem is the "?" in the image location. Looks like we detect the URL as one TB can control and replace with:
src="mailbox:///D:/.../Drafts?number=166&amp;C=xx.jpg"

Of course when we later try to access this and convert it to a CID, the resource can't be found and therefore the reference is sent out unchanged (just guessing).

With
  <img src="https://www.amazon.de/gp/xx.jpg">
the problem doesn't occur. The image source is left untouched regardless of how may times the draft gets saved.
Problem is here. We consider anything that has a "?" as ours:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#392

Before we do that, we should check that the URL is one of ours, so it should start with one of:
mailbox:
mailbox-message:
imap:
imap-message:
Attached patch Killer messages - adieu!! (v1a). (obsolete) — — Splinter Review 
Simple fix for a severe problem that together with other problems has caused deleted MSF files, reappearing superseded drafts and endless attaching problems.

All due to the killer messages created by this code.

Aceman, please note that it is correct to check for mailbox and imap here, see comment further down in the file:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#423
// mailbox urls will have ?number=xxx; imap urls won't.
Attachment #8777965 - Flags: review?(acelists)
Comment on attachment 8777965 [details] [diff] [review]
Killer messages - adieu!! (v1a).

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

Nice detective work :)

The patch works for me on the sample message (I have never noticed it on my own messages as I do not reply to HTML spam :))

Please make the commit message say "...URLs in composed messages...".

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cb20afe109ee494269bc6658778ad127932ebdd8

Sidenote: have you noticed that when looking at the draft in drafts folder, the "remote content" warning is on top of the "this is a draft" notification, so the Edit button on it can't be seen/clicked. Is that a bug?

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +384,5 @@
>          // do we care about anything besides images?
>          nsAutoString objURL;
>          image->GetSrc(objURL);
> +
> +        // First we need to make sure that the URL is one the we control

the -> that ?
Attachment #8777965 - Flags: review?(acelists) → review+
I'm CCing our great bug triagers so that they can now look out for bugs that may be a manifestation of this bug and thus could get closed/fixed now.
Severity: normal → major
Component: Message Compose Window → Composition
Keywords: dataloss
OS: Unspecified → All
Product: Thunderbird → MailNews Core
Hardware: Unspecified → All
Whiteboard: [dupe to me]
Attached patch Killer messages - adieu!! (v1b). (obsolete) — — Splinter Review 
Thanks. Nits fixed.

(In reply to :aceman from comment #5)
> The patch works for me on the sample message (I have never noticed it on my
> own messages as I do not reply to HTML spam :))
The original message was from Amazon.

> Please make the commit message say "...URLs in composed messages...".
Done, slightly different wording.

> Try run:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=cb20afe109ee494269bc6658778ad127932ebdd8
Thanks, I'll check it out. I'll push it tomorrow or mark it "checkin-needed".

> Sidenote: have you noticed that when looking at the draft in drafts folder,
> the "remote content" warning is on top of the "this is a draft"
> notification, so the Edit button on it can't be seen/clicked. Is that a bug?
I knew that. Call it an annoying feature.

> > +        // First we need to make sure that the URL is one the we control
> the -> that ?
the -> (gone)
// First we need to make sure that the URL is one we control
;-)
Attachment #8777965 - Attachment is obsolete: true
Attachment #8777990 - Flags: review+
Summary: img src="mailbox:///.../Drafts?number=..." in a composition gets sent without being converted to reference via CID → embedded image with source URL with ? leads to corrupt src="mailbox:///.../Drafts?number=..." in a composition. (This gets sent without being converted to reference via CID).
Bug 1114008 is for same issue in <img ... moz-do-not-send="true" ...> case.
So, "This gets sent without being converted to reference via CID" part of this bug is not applicable to Bug 1114008.
Comment on attachment 8777990 [details] [diff] [review]
Killer messages - adieu!! (v1b).

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

Drive-by comment.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +388,5 @@
> +        // First we need to make sure that the URL is one we control
> +        // so we don't accidentally manipulate a URL like:
> +        // http://www.site.com/retrieve.html?C=image.jpg.
> +        if (objURL.Find("mailbox") != 0 && objURL.Find("imap") != 0)
> +          continue;

This was not really the correct thing to do. I've been trying to eliminate cases where behavior is hard wired to particular account types, and here you have added another one. Breaking more windows.

You need to understand what exactly is the URL behavior that matters to you, and use protocol information to check that behavior. If you want to see if it "belongs to us" you might try getting the protocol handler and see if it QIs to nsIMsgProtocolInfo, but I don't know if that is really the behavior that you want.
Thanks for the comment. I'll look into it. Nothing landed so far.

Also, I want to do a bit more investigation into why img src="mailbox:///.../Drafts?number=..." was left in the message upon send since clearly no attachment could be accessed via the faulty link. If an error message had been display, this would have been detected much earlier. Maybe the system didn't even try to replace this with a CID since there was no attachment in the first place.
Yeah, if there is a more universal way to fix this, it will be better than hardcoding these strings.

Anyway, this fix makes it that no more killer messages will be created.
Is there a bug about sanitizing already existing killer messages in users' folders? Those are already there and as I understand it any attempt to read them may kill a msf file. These messages may linger there forever. Or was that the other bug you did Jorg, where you prevented the drafts.msf being killed? Is that universal for all folders? Does it check that even if the URL starts with "mailbox:" that it does not really point to a real attachment (the part after ? is invalid)? If yes, great.
(In reply to :aceman from comment #11)
> Or was that the other bug you did Jorg, where you prevented
> the drafts.msf being killed?
Yes, bug 1216914 stopped the deletion of MSF files caused by "killer messages", so no clean-up required.
Nice, thanks.
See Also: → 1216914
(In reply to :aceman from comment #11)
> Anyway, this fix makes it that no more killer messages will be created.
> Is there a bug about sanitizing already existing killer messages in users' folders?

Even when ”already existing killer messages in users' folders” never exists, any Thunderbird user can pretty easily generate such killer message by requesting "Image URL with query string" upon "Insert Image in Tb's HTML mode mail composition", as pretty easily known by phenomenon reported to Bug 1114008.
As pretty easily known by the Bug 1114008, "URL for an image like next" is never incorrect image URL on Earth,
  https://secure.gravatar.com/avatar/879385da7489a39b50513dde5c68ee5a?d=mm&size=64
If it's incorrect, why this URL is used as official data of the Bug 1114008 which was generated by B.M.O?
(In reply to WADA from comment #14)
> As pretty easily known by the Bug 1114008, "URL for an image like next" is
> never incorrect image URL on Earth,
>  
> https://secure.gravatar.com/avatar/
> 879385da7489a39b50513dde5c68ee5a?d=mm&size=64
> If it's incorrect, why this URL is used as official data of the Bug 1114008
> which was generated by B.M.O?

I'm not sure what you mean here. What is incorrect in that URL?
Hmm, perhaps it's not clear what I mean by killer message:
I mean a message where a URL
  <img src="https://www.amazon.de/gp/r.html?C=xx.jpg">
erroneously got replaced by
  <img src="mailbox:///D:/.../Drafts?number=166&amp;C=xx.jpg">

Viewing a message containing the latter led to the deletion of MSF files, hence the name "killer message".

There is nothing wrong with the original message or the link contained in it.
Attached patch WIP: Patch according to Kent (obsolete) — — Splinter Review 
Hi Kent, I tried to do what you suggested, but it doesn't work.

Here some debug:
=== URL |https://www.amazon.de/gp/r.html?C=xx.jpg|
=== Protocol |https|
=== not one of ours

=== URL |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.SPAM%3E63?header=quotebody&part=1.2&filename=lijbmghmkilicioj.png|
=== Protocol |imap|

and sadly not working:
=== URL |mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Drafts?number=28&header=quotebody&part=1.2&filename=lijbmghmkilicioj.png|
=== Protocol |mailbox|
=== not one of ours

So the QI to nsIMsgProtocolInfo fails for a mailbox. But that's the one we want to detect as our own and process.

So I need a better idea for a more generic solution. Needless to say that the generic solution is much more complicated than comparing strings. Also note that the "broken windows" are already here:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#423
where the code already explicitly knows about mailbox and imap URLs.

I'm happy to put lipstick on the pig if you help me choose the right colour ;-)
Attachment #8778256 - Flags: feedback?(rkent)
Acemen, do you know what's going on? Should the mailbox protocol QI/cast to nsIMsgProtocolInfo? Kent told me in an e-mail (quote): "Aceman did one of these in another bug, he could be a good resource."

Looking at
https://hg.mozilla.org/comm-central/diff/75b05edca192/mailnews/compose/src/nsMsgSend.cpp#l1.182
(from bug 1240327) "mailbox" and "imap" are counted separately, so I think Kent is just mistaken here.

Given that at
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#423
we handle mailbox and imap, frankly, what Kent is asking for here is not appropriate since this code would need to be looked at for other account types.

What do you think?
Flags: needinfo?(acelists)
I can't find what Kent could mean.
However there is e.g. https://dxr.mozilla.org/comm-central/source/mailnews/local/src/nsMailboxService.cpp#511 which seems to know the "mailbox" protocol.

Maybe you need to QI on nsIProtocolHandler not, nsIProtocolInfo?

Or when you did the GetProtocolHandler, isn't it enough that it succeeded? Or does that also match on the "http" protocol which we want to skip?
Flags: needinfo?(acelists)
(In reply to :aceman from comment #19)
> Maybe you need to QI on nsIProtocolHandler not, nsIProtocolInfo?
That's what I already have.

> Or when you did the GetProtocolHandler, isn't it enough that it succeeded?
> Or does that also match on the "http" protocol which we want to skip?
It matches any protocol: http, https, data, etc.

I'll do a patch and see whether Kent likes it.
And what about the way you linked to in the nsMsgSend?
+          // Additional account types need a mechanism to report that they are
+          // message protocols. If there is an nsIMsgProtocolInfo component
+          // registered for this scheme, we'll consider it a mailbox
+          // attachment.
+          nsAutoCString contractID;
+          contractID.Assign(
+            NS_LITERAL_CSTRING("@mozilla.org/messenger/protocol/info;1"));
+          nsAutoCString scheme;
+          uri->GetScheme(scheme);
+          contractID.Append(scheme);
+          nsCOMPtr<nsIMsgProtocolInfo> msgProtocolInfo =
+            do_CreateInstance(contractID.get());
Attached patch Killer messages - adieu!! (v2a). (obsolete) — — Splinter Review 
Kent, this one is for you. It is modelled on you work here:
https://hg.mozilla.org/comm-central/diff/75b05edca192/mailnews/compose/src/nsMsgSend.cpp#l1.182

Note that the protocol handler for "mailbox" *cannot* be QI'ed onto nsIMsgProtocolInfo as you had suggested.

Obviously you're the guy who created additional message protocols. So you can see whether this does what you need. What happens if you reply to a message that contains an embedded image (not embedded as data URI)? Does this code work for you? I'll attach a message I use for testing. When replied to from a local folder the content looks like this (using ThunderHTMLedit):
<img
src="mailbox:///D:/MAIL-THUNDERBIRD/jorgk.default/Mail/jorgk.jorgk.com/Sent.sbd/Test%20attaching?number=5&amp;header=quotebody&amp;part=1.2&amp;filename=lijbmghmkilicioj.png" alt="">

You could load it into a special account and see what happens.
Attachment #8778256 - Attachment is obsolete: true
Attachment #8778256 - Flags: feedback?(rkent)
Attachment #8778404 - Flags: review?(rkent)
(In reply to Jorg K (GMT+2, PTO during summer) from comment #10)
> Also, I want to do a bit more investigation into why img
> src="mailbox:///.../Drafts?number=..." was left in the message upon send
> since clearly no attachment could be accessed via the faulty link. If an
> error message had been display, this would have been detected much earlier.
> Maybe the system didn't even try to replace this with a CID since there was
> no attachment in the first place.
I did this investigation looking where the CID is generated. It happens in this big loop:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSend.cpp#1698

For a URL which we erroneously destroyed no attachment is present, so we never try to replace the bad URL with a CID. Hence it gets send out.

Case closed with one of the two patches presented here.
No longer blocks: 1114008
(current practice for this whiteboard string is specify as one word https://mzl.la/2aXH5ot )
Whiteboard: [dupe to me] → [dupetome]
Attached patch Killer messages - adieu!! (v2b). (obsolete) — — Splinter Review 
Same effect as v2a, perhaps a little more elegant.
The print statements are left in for the reviewer, they will of course be removed before landing.
Attachment #8778404 - Attachment is obsolete: true
Attachment #8778404 - Flags: review?(rkent)
Attachment #8778592 - Flags: review?(rkent)
Kent, you mentioned imap: vs. imap-message: and mailbox: vs. mailbox-message:
I came across both of those in bug 597369 when I tried to implement a nsIMsgWindow::messageURL (which was rejected). My patch now looks at the URL's scheme, so the *-message ones should not be occurring there.
Comment on attachment 8778592 [details] [diff] [review]
Killer messages - adieu!! (v2b).

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

Needs some changes.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +82,5 @@
>  #include "nsISelection.h"
>  #include "nsJSEnvironment.h"
>  #include "nsIObserverService.h"
> +#include "nsIProtocolHandler.h"
> +#include "nsIMsgProtocolInfo.h"

You won't need nsIMsgProtocolInfo after my changes, see below.

@@ +395,5 @@
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        nsAutoCString scheme;
> +        ioService->ExtractScheme(NS_ConvertUTF16toUTF8(objURL), scheme);
> +
> +        if (scheme.Equals("data") || scheme.Find("http") == 0) {

I don't like this attempt at optimization for code that is low impact. It adds bloat, introduces new possibilities for errors, and masks whether the standard mechanism actually works (and it doesn't in this case!). Please remove.

@@ +399,5 @@
> +        if (scheme.Equals("data") || scheme.Find("http") == 0) {
> +          // Skip schemes we're not interested in quickly.
> +          printf("=== Ignoring scheme |%s|\n", scheme.get());
> +          continue;
> +        } else if (!(scheme.Equals("mailbox") || scheme.Equals("imap"))) {

Remove this premature optimization.

@@ +406,5 @@
> +          ioService->GetProtocolHandler(scheme.get(), getter_AddRefs(handler));
> +          if (!handler)
> +            continue;
> +          // See whether it's one of our mail protocols.
> +          nsCOMPtr<nsIMsgProtocolInfo> mailHandler = do_QueryInterface(handler);

This does not work, as mailbox protocols do not support this. The feature that you are actually looking for is whether the protocol can handle attachments. If you replace this with nsIMsgMessageFetchPartService then it works, which both mailbox and imap support.
Attachment #8778592 - Flags: review?(rkent) → review-
Attached patch Killer messages - adieu!! (v3a). (obsolete) — — Splinter Review 
Prints will be removed before landing. Maybe you already know that it works like this, I tried with "mailbox", "imap", "https" and "data".
Attachment #8777990 - Attachment is obsolete: true
Attachment #8778592 - Attachment is obsolete: true
Attachment #8779840 - Flags: review?(rkent)
Comment on attachment 8779840 [details] [diff] [review]
Killer messages - adieu!! (v3a).

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

Yes this is fine, sans print statements.
Attachment #8779840 - Flags: review?(rkent) → review+
[Approval Request Comment]
Regression caused by (bug #): Has always been wrong.
User impact if declined: Before this bug was the start of a chain of destruction:
The message content caused MSF files to disappear (bug 1216914) which in turn caused the "endless attaching" bug (see bug 532395 comment #223). Since the chain has already been broken, this bug is less important.
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
Not risky, added small but important test.
Attachment #8778407 - Attachment is obsolete: true
Attachment #8779840 - Attachment is obsolete: true
Attachment #8780384 - Flags: review+
Attachment #8780384 - Flags: approval-comm-esr45?
Attachment #8780384 - Flags: approval-comm-beta?
Attachment #8780384 - Flags: approval-comm-aurora+
https://hg.mozilla.org/comm-central/rev/6949ba7846d2
(landed on closed tree since I need it for Aurora, sorry).

Aurora (TB 50):
https://hg.mozilla.org/releases/comm-aurora/rev/4aa478f37383
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-thunderbird49: --- → affected
status-thunderbird50: --- → fixed
status-thunderbird51: --- → fixed
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
Attachment #8780384 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 8780384 [details] [diff] [review]
Killer messages - adieu!! (v3b) - same as v3a without the print.

I wish we could try this in the beta first, but looking at the code I don't see any way this could cause a serious regression.
Attachment #8780384 - Flags: approval-comm-esr45? → approval-comm-esr45+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: