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

RESOLVED FIXED in Thunderbird 51.0

Status

MailNews Core
Composition
--
major
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

({dataloss})

Trunk
Thunderbird 51.0
dataloss

Thunderbird Tracking Flags

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

Details

(Whiteboard: [dupetome])

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

a year ago
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 ;-)
(Assignee)

Comment 1

10 months ago
Created attachment 8777814 [details]
Test message showing the problem

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)

Updated

10 months ago
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
(Assignee)

Comment 2

10 months ago
Created attachment 8777932 [details]
Much simpler test message showing the problem

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

Comment 3

10 months ago
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:
(Assignee)

Comment 4

10 months ago
Created attachment 8777965 [details] [diff] [review]
Killer messages - adieu!! (v1a).

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 5

10 months ago
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+

Comment 6

10 months ago
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]
(Assignee)

Comment 7

10 months ago
Created attachment 8777990 [details] [diff] [review]
Killer messages - adieu!! (v1b).

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

Updated

10 months ago
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).
Blocks: 1114008
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 9

10 months ago
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.
(Assignee)

Comment 10

10 months ago
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.

Comment 11

10 months ago
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.
(Assignee)

Comment 12

10 months ago
(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.

Comment 13

10 months ago
Nice, thanks.
See Also: → bug 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?

Comment 15

10 months ago
(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?
(Assignee)

Comment 16

10 months ago
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.
(Assignee)

Comment 17

10 months ago
Created attachment 8778256 [details] [diff] [review]
WIP: Patch according to Kent

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

Comment 18

10 months ago
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)

Comment 19

10 months ago
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)
(Assignee)

Comment 20

10 months ago
(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.

Comment 21

10 months ago
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());
(Assignee)

Comment 22

10 months ago
Created attachment 8778404 [details] [diff] [review]
Killer messages - adieu!! (v2a).

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

Comment 23

10 months ago
Created attachment 8778407 [details]
Simple message with embedded image for Kent's testing.
(Assignee)

Comment 24

10 months ago
(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.
(Assignee)

Updated

10 months ago
No longer blocks: 1114008
(Assignee)

Updated

10 months ago
Duplicate of this bug: 1114008
(current practice for this whiteboard string is specify as one word https://mzl.la/2aXH5ot )
Whiteboard: [dupe to me] → [dupetome]
(Assignee)

Comment 27

10 months ago
Created attachment 8778592 [details] [diff] [review]
Killer messages - adieu!! (v2b).

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

Comment 28

10 months ago
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 29

10 months ago
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-
(Assignee)

Comment 30

10 months ago
Created attachment 8779840 [details] [diff] [review]
Killer messages - adieu!! (v3a).

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 31

10 months ago
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+
(Assignee)

Comment 32

10 months ago
Created attachment 8780384 [details] [diff] [review]
Killer messages - adieu!! (v3b) - same as v3a without the print.

[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+
(Assignee)

Comment 33

10 months ago
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
Last Resolved: 10 months ago
status-thunderbird49: --- → affected
status-thunderbird50: --- → fixed
status-thunderbird51: --- → fixed
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
(Assignee)

Updated

9 months ago
Attachment #8780384 - Flags: approval-comm-beta? → approval-comm-beta+
(Assignee)

Comment 34

9 months ago
Beta (TB 49):
https://hg.mozilla.org/releases/comm-beta/rev/0d45c9451a46
status-thunderbird49: affected → fixed

Comment 35

9 months ago
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+

Comment 36

9 months ago
https://hg.mozilla.org/releases/comm-esr45/rev/02896946d380305658a672a70dd2274d33e24697
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: ? → +

Updated

9 months ago
tracking-thunderbird_esr45: + → 48+
You need to log in before you can comment on or make changes to this bug.