1412639 - Image from eml file not shown in Composer

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[webwirtschaft]

Attachment: eml file with image

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.102 Safari/537.36 Vivaldi/1.93.955.42

Steps to reproduce:

- create new user profile and set up mail account
- run attached eml file
--> file opens in a separate mail window
- press Ctrl+E



Actual results:

- mail opens in composer
- image is not shown
- in the status bar, I see "thunderbird has blocked a file from loading into this message"
- clicking on "Preferences" I see "Unblock mailbox:///media/username/..."
--> Unless stated <a href="https://www.mozilla.org/en-US/thunderbird/52.0/releasenotes/">here</a>, this does'nt look like a data URI. It is the beginning of the local address of the eml file. 

Related: https://bugzilla.mozilla.org/show_bug.cgi?id=1366349 with  http://somup.com/cb1ZqzViwY


Expected results:

The image should be shown in composer and sent with the message.

Comment 1

[webwirtschaft]

Attachment: clickable message to load image in mail composer

Comment 2

[Jorg K (CEST = GMT+2)]

If you move the attached message into a folder, you have no problem. I can confirm that opening the saved message and pressing Ctrl+E (Edit as new message) brings up the blocked content warning. (BTW: It's "Options", not "Preferences" in the English UI).

I think this is a corner case. The problem will be here in the comparison:
MsgComposeCommands.js:5559
  if (src.startsWith(originalMsgNeckoURI.value.spec)) {

I get:
src:
mailbox:///D:/Desktop/image_not_shown_in_composer.eml?fetchCompleteMessage=true&editasnew=true&number=0&part=1.2&filename=testbild.png
Necko:
mailbox:///D:/Desktop/image_not_shown_in_composer.eml?type=application/x-message
-display&fetchCompleteMessage=true&editasnew=true&number=0

So src doesn't start with the Necko URI since the "type=application/x-message-display&" is in the way.

I'll fix it now.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1412639-message-from-file.patch (v1)

Comment 4

[webwirtschaft]

Thank you very much for looking into this! I hope you hit the right spot...because as you can see in attachment 8923158 [details], my English UI (Linux) says "Preferences", not "Options". 

Anyway it would be of high value to have this fixed. I'm currently evaluating Thunderbird for use at the company I work for, where there are sent daily around 200 Mails from eml files. And that image problem is the main obstacle right now when using the latest version of Thunderbird. 

And yes, the eml file I created with Thunderbird Composer, saved it as a template and then took the code from the file system's template folder just to make sure the eml format would be exactly what Thunderbird expects.

Comment 5

[:aceman]

Comment on attachment 8923215 [details] [diff] [review]
1412639-message-from-file.patch (v1)

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5557,5 @@
>        let originalMsgNeckoURI = {};
>        msgSvc.GetUrlForUri(gOriginalMsgURI, originalMsgNeckoURI, null);
> +      // Remove indication that this was opened from a file.
> +      originalMsgNeckoURI.value.spec =
> +        originalMsgNeckoURI.value.spec.replace("type=application/x-message-display&", "");

Would there be a safer way to check for the argument so that id does not match e.g. 'apptype=application/x-message-display&'. Also what if the trailing & is not there (this argument being the last one in the URI)? I'd think Necko (or some other URI manipulation API) would have a ton of robust helpers to analyse and cut out the URI components.

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #5)
> Would there be a safer way to check for the argument so that id does not
> match e.g. 'apptype=application/x-message-display&'.
I don't understand the question. Which argument? Which ID?

> Also what if the trailing & is not there (this argument being the last one
> in the URI)?
Looking at
https://dxr.mozilla.org/comm-central/search?q=application%2Fx-message-display&redirect=false
I should test for &type=application/x-message-display.
I'll update the patch to do that once you clarified the point above.

> I'd think Necko (or some other URI manipulation API) would have a ton
> of robust helpers to analyse and cut out the URI components.
Not so. There are getter functions for all the different URL parts:
https://dxr.mozilla.org/mozilla-central/rev/66f9b72b87297adf712b14be58df13c2333bb3a9/netwerk/base/nsIURI.idl#16
but not to pull the query part apart.

See the DXR search that there's always a find involved.

Comment 7

[Jorg K (CEST = GMT+2)]

(In reply to webwirtschaft from comment #4)
> Anyway it would be of high value to have this fixed.
It will be fixed, but the fix won't be in the stable version TB 52.x ESR for a while, say some weeks or up to two months.

Comment 8

[:aceman]

(In reply to Jorg K (GMT+2) from comment #6)
> (In reply to :aceman from comment #5)
> > Would there be a safer way to check for the argument so that id does not
> > match e.g. 'apptype=application/x-message-display&'.
> I don't understand the question. Which argument? Which ID?

Say the URI would be mailbox:///D:/Desktop/image_not_shown_in_composer.eml?apptype=application/x-message
-display&fetchCompleteMessage=true&editasnew=true&number=0

Your replace would still match it and mangle it to mailbox:///D:/Desktop/image_not_shown_in_composer.eml?app&fetchCompleteMessage=true&editasnew=true&number=0

The URIs wouldn't match which may be what you want, but it seems semantically ugly and by just luck. Next time your would want to know if the string was in the URI or not, and the answer would be wrong.

> > Also what if the trailing & is not there (this argument being the last one
> > in the URI)?
> Looking at
> https://dxr.mozilla.org/comm-central/search?q=application%2Fx-message-
> display&redirect=false
> I should test for &type=application/x-message-display.
> I'll update the patch to do that once you clarified the point above.

Notice your URI has ? preceding the type, not &. But I ask for a way for both to work, if the type gets moved to elsewhere in the URI, e.g. mailbox:///D:/Desktop/image_not_shown_in_composer.eml?fetchCompleteMessage=true&type=application/x-message-display&editasnew=true&number=0

Your current .replace() would also not work on: mailbox:///D:/Desktop/image_not_shown_in_composer.eml?fetchCompleteMessage=true&editasnew=true&number=0&type=application/x-message-display

Also, do you only want to ignore the 'type' when the value is 'application/x-message-display', or any type?
 
> > I'd think Necko (or some other URI manipulation API) would have a ton
> > of robust helpers to analyse and cut out the URI components.
> Not so. There are getter functions for all the different URL parts:
> https://dxr.mozilla.org/mozilla-central/rev/
> 66f9b72b87297adf712b14be58df13c2333bb3a9/netwerk/base/nsIURI.idl#16
> but not to pull the query part apart.
> 
> See the DXR search that there's always a find involved.

Yes, usually it is the websites to handle the contents of the query part of the URI. But if we handle URIs so extensively for internal purposes, I would expect some handlers to split the query into individual arguments/variables. Maybe Firefox has no need for that.

Maybe we should invent some helpers. E.g. all of the above cases would be solved by a single call originalMsgNeckoURI.value.spec.removeQueryArg("type"), or similar, if the .removeQueryArg() helper was robust enough.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8923215 [details] [diff] [review]
1412639-message-from-file.patch (v1)

OK, I'll come up with something more robust.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1412639-message-from-file.patch (v2)

I didn't feel like doing some tricky regexp stuff which is most likely slower since it has the parse the expression, etc. The most probable case is that the message is *not* opened form a file and the query part isn't found.

Comment 11

[Jorg K (CEST = GMT+2)]

Oh, this will still find "type=xx" in "apptype=xx" and mutilate it accordingly.
Let me know if you want a more expensive "Rolls Royce" solution.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1412639-message-from-file.patch (v3)

Here's the Rolls Royce solution.

Comment 13

[:aceman]

In the first patch you modified the originalMsgNeckoURI.value.spec, in the latter versions you don't do it. Which version is correct?
Also you didn't answer whether we want to remove any 'type' and its value.

Comment 14

[Jorg K (CEST = GMT+2)]

It's unimportant whether we change originalMsgNeckoURI.value.spec.
I want to remove a fixed query part "type=application/x-message-display" safely, irrespective of whether ? or & leads and what follows. The later two patches do that, the last one doesn't destroy "apptype=..." as requested.

Comment 15

[:aceman]

Comment on attachment 8923556 [details] [diff] [review]
1412639-message-from-file.patch (v3)

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5482,5 @@
> + * @param aURL    the URL from which to remove the query part
> + * @param aQuery  the query part to remove
> + * @return        the URL with the query part removed
> + */
> +function removeQueryPart(aURL, aQuery)

I still find the code somehow ugly, but if it works let's leave it at that. I may look at some improvements later when I can check what other uses of this we have in the code. E.g. there is a similar argument extraction in loadBlockedImage().

@@ +5491,5 @@
> +    return aURL;
> +
> +  // Now look a bit closer.
> +  index = aURL.indexOf("?" + aQuery);
> +  if (index < 0)

If there is no '?' in the whole URL, maybe we should abort? There is no query in the URL so no reason to look for &. There may be one, but it isn't in the query component of the URL. I would try to use nsIURI.query somehow to get the already robust logic for splitting the URL, but we can leave it for followup improvements.

@@ +5507,5 @@
>  function InitEditor()
>  {
> +  // Debug test.
> +  dump(removeQueryPart("...?remove=xx&other=yy", "remove=xx")+"===\n");
> +  dump(removeQueryPart("...?aa=aa&remove=xx&other=yy", "remove=xx")+"===\n");

What about 'removeQueryPart("...?aa=aa&remove=xxzz&other=yy", "remove=xx"'? I think this one is mangled again but shouldn't. Please fix suffixed query case and we should be done.

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #15)
> If there is no '?' in the whole URL, maybe we should abort? There is no
> query in the URL so no reason to look for &. There may be one, but it isn't
> in the query component of the URL.
I don't understand. The query part of a URL is anything after the ?, so
www.xx.com?xx=yy&xx=tt
           ^^^^^^^^^^^
> I would try to use nsIURI.query somehow
> to get the already robust logic for splitting the URL, but we can leave it
> for followup improvements.
Yes, you can use .query, but the problem is to manipulate the result. You'd have to go
aURL.query = ...
IMHO using .query doesn't help at all since you still need to cut the part you want to remove from the middle of the query since the query can have multiple parts.

> What about 'removeQueryPart("...?aa=aa&remove=xxzz&other=yy", "remove=xx"'?
> I think this one is mangled again but shouldn't. Please fix suffixed query
> case and we should be done.
OK.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1412639-message-from-file.patch (v4)

Rolls Royce super-charged ;-)

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1412639-message-from-file.patch (v5)

Since I know that you hate signing off on something less than elegant, let's drive the full JS power at it with split, splice and join ;-)

Comment 19

[:aceman]

Comment on attachment 8923714 [details] [diff] [review]
1412639-message-from-file.patch (v5)

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

Strangely, I now tried the attachment and I do NOT get the notification bar about the image.
I just get a message in error console:
Security Error: Content at moz-nullprincipal:{ebcb3c81-3550-4b4a-ac2a-ce968b81644e} may not load or link to mailbox:///.../attachment=8923157.eml?fetchCompleteMessage=true&editasnew=true&number=0&part=1.2&filename=testbild.png.
And the image does not show in the composer.
With the patch, the image (orange square) does show up correctly. But I still get the same security error (twice!) in the console. Why is it there?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5497,5 @@
> +  let indexPart = queryParts.indexOf(aQuery);
> +  if (indexPart < 0)
> +    return aURL;
> +  queryParts.splice(indexPart, 1);
> +  return aURL.substr(0, indexQM + 1) + queryParts.join("&");

Yes, this is what I would like :) I know this is slower than your version, but easier to read and I think on the short URLs (with few & chars) the speed does not matter.

Comment 20

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #19)
> I now tried the attachment and I do NOT get the notification bar about the image.
That's a bug somehow, no? What happens if you include a file-based image, like paste a piece of HTML that has <img src=file://... Does that show the notification bar? BTW, the blocked content notification is hidden by the "you don't have a matching identity" notification if you go "edit as new" on the attached message.

> But I still get the same security error (twice!) in the console. Why is it there?
That's how it works in general. We load the content and some parts are blocked resulting in that console error. They are then loaded via an error event listener.
  // XXX: the error event fires twice for each load. Why??
  editor.document.body.addEventListener("error", function(event) {

Comment 21

[:aceman]

(In reply to Jorg K (GMT+2) from comment #20)
> (In reply to :aceman from comment #19)
> > I now tried the attachment and I do NOT get the notification bar about the image.
> That's a bug somehow, no? What happens if you include a file-based image,
> like paste a piece of HTML that has <img src=file://... Does that show the
> notification bar? BTW, the blocked content notification is hidden by the
> "you don't have a matching identity" notification if you go "edit as new" on
> the attached message.

OK, then I see it now in the composer after closing the identity notification. And really on Linux the button says Preferences.

So all is fine then, thanks for fixing it.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/28b76ee8a4ee
remove 'type=application/x-message-display' before same origin check in image processing. r=aceman

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8923714 [details] [diff] [review]
1412639-message-from-file.patch (v5)

I'll include this in TB 57 beta 1 and hopefully it can make TB 52.4.1 in a few weeks.

Comment 24

[Jorg K (CEST = GMT+2)]

SM needs to fix this, too.

Comment 25

[Jorg K (CEST = GMT+2)]

Beta (TB 57):
https://hg.mozilla.org/releases/comm-beta/rev/c92346f64d497d2a84b65e1da7515144e8df3302

Comment 26

[Jorg K (CEST = GMT+2)]

TB 52.5 ESR (should be tracking 57+):
https://hg.mozilla.org/releases/comm-esr52/rev/64782d6f7602

Comment 27

[webwirtschaft]

Works in Thunderbird 52.5 on Windows and Linux. Thank you very much!