609406 - inserted but unattached external (http) image is not displayed in the reply compose window

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[al_9x]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101027 Lightning/1.0b2 Thunderbird/3.1.6

When composing new mail, an external (http) image, inserted without attaching, is immediately shown in the message.

But when replying, only the placeholder is shown

Reproducible: Always

Comment 1

[Ludovic Hirlimann [:Usul]]

Same issue in -safe-mode ?

This look a lot like bug 560877 or bug 560210. Can you provide  an example email showing the issue please ?

Comment 2

[al_9x]

(In reply to comment #1)
> Same issue in -safe-mode ?

yes

> 
> This look a lot like bug 560877 or bug 560210. Can you provide  an example
> email showing the issue please ?

I don't think it's the same, the inserted http image is not lost it just isn't displayed in the compose window, only the accurately sized placeholder is shown.

It happens with any message (including news) you reply to, you can easily reproduce it in a new profile:

1. Create a new profile (TB 3.1.6)
2. add news.mozilla.org news account
3. check "Compose in HTML" in the account properties
4. subscribe to any group (dev.apps.thunderbird)
5. right click on any message ("Outlook import testing needed") and reply to sender
6. in the composer, Insert -> Image
7. enter a real http image url
8. uncheck "attach this image to this message"
9. Select "don't use alternate text"
10. click ok, only the placeholder is shown in the composer.

Comment 3

[Mark Banner (:standard8)]

I can confirm the STR though it isn't an issue if the sender of the email is whitelisted as allowing remote content.

I suspect we can improve the content policy here, though I'm not sure how yet.

Comment 4

[al_9x]

(In reply to comment #3)
> I can confirm the STR though it isn't an issue if the sender of the email is
> whitelisted as allowing remote content.
> 
> I suspect we can improve the content policy here, though I'm not sure how yet.

This is not the senders content that's being blocked but the user's own.  One should not have to whitelist anyone to be able to compose a message.

TB should differentiate (and not block) user composed content.

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> (In reply to comment #3)
> > I can confirm the STR though it isn't an issue if the sender of the email is
> > whitelisted as allowing remote content.
> > 
> > I suspect we can improve the content policy here, though I'm not sure how yet.
> 
> This is not the senders content that's being blocked but the user's own.  One
> should not have to whitelist anyone to be able to compose a message.

Agreed. I was mentioning the whitelisting as a note for work around, and as a reminder to developers of what the factors are that affect this bug.

Comment 6

[Jorg K (CEST = GMT+2)]

Forward duplicate to bug where this will be fixed.

Comment 7

[Elhem Enohpi]

I'm still having trouble with this bug. I don't think it's a duplicate of bug 1251408. Could it be reopened please?

Bug 1251408 pertains to replying to messages containing remote content, where the sender or domains have been whitelisted. When reading the message, the remote content is displayed, but when replying to it, it's not. That has been fixed.

This bug however, covers the problem that it's not possible for the user to insert and see a remote image, when replying to a message where that message's sender is _not_ whitelisted - even if that sender did not include any remote content. It's basically a consequence of the fix for bug 522019.

As the reporter said in comment 4, "This is not the senders content that's being blocked but the user's own. One should not have to whitelist anyone to be able to compose a message. TB should differentiate (and not block) user composed content".

In other words, I reply to a message from Bob, which did not contain any remote content from him. I insert a link to a remote image, and uncheck "attach this image". I see a thumbnail of the image in "image preview". But when I click "Ok", I can't see the image in the compose window.

My use case is that I often want to illustrate some point, by including an image from somewhere on the web (not my own server etc.). I prefer to send it as remote content, rather than attaching it, to avoid sending multi-megabyte e-mails, and give the person the option to not download/view it on their phone for example.

A work-around is to whitelist the person you're replying to. But since that person didn't send any remote content, and the "remote content blocked" bar did not appear, there's no convenient way to do that. You have to copy their e-mail address, go into options/preferences-->privacy-->mail content-->exceptions-->add an exception. Adding an exception for your own e-mail address doesn't work (and is a bad idea anyway, since so much spam is spoofed as coming from your own address). You have to do this for every person you might want to send a linked image to, in a reply.

The best solution would be to always show remote content inserted by the user in compose, while still blocking any in the quoted material if it hasn't been whitelisted. Not sure if that's feasible though, I assume that block/show remote content is on a per-message basis, not per-url.

Another idea would be, after inserting a remote image while composing a reply to a non-whitelisted sender, to show the "remote content blocked" bar in the compose window, and give the option to whitelist the sender (actually now the recipient), or whitelist this particular message. That would work for me.

It wouldn't work to whitelist your own address, and have the compose window consider you as the sender. That would have the effect that any message you reply to, would have remote content quoted from the original sender shown, effectively reintroducing Bug 522019.

Comment 8

[Jorg K (CEST = GMT+2)]

You're right.

In a reply, I don't see, for example, https://s.w.org/images/notableusers/blondie-2x.png, where https://s.w.org is not whitelisted. I can see it in a new message.

Comment 9

[Jorg K (CEST = GMT+2)]

Just revisiting this bug. As a workaround I found that you can insert the remote image with the attach box checked. That will make it show. Then you go back to the properties and uncheck the box. It doesn't get hidden now.

Comment 10

[Jorg K (CEST = GMT+2)]

Ignore that previous comment, I was using a Daily where the content policy check didn't work. Now it works again and http-based images that are inserted into a reply just do now show. This is due to the processing here:
https://dxr.mozilla.org/comm-central/rev/cbd97df08042e6eee4a2823ade9dda017154bfa6/mailnews/base/src/nsMsgContentPolicy.cpp#624
Quote:
    // Special case image elements. When replying to a message, we want to allow
    // the user to add remote images to the message. But we don't want remote
    // images that are a part of the quoted content to load. Fortunately, after
    // the quoted message has been inserted into the document, mail compose
    // flags remote content elements that came from the original message with a
    // moz-do-not-send attribute.
This logic is just plain wrong. If you insert you image with a moz-do-not-send attribute, it won't show.

Going back in history, this has been wrong since this code was imported into Mercurial in 2008. Here's the first version:
https://hg.mozilla.org/comm-central/annotate/e4f4569d451a/mailnews/base/src/nsMsgContentPolicy.cpp#l519

I just tried to remove the moz-do-not-send attribute on a freshly inserted image with ThunderHTMLedit, and lo and behold, the image shows.

Comment 11

[Elhem Enohpi]

Right, because moz-do-not-send is set for *every* remote image, not only the ones inserted from the quoted original message. So it's not a valid test.

Since we only want to block remote images in quoted content, would it not be enough to test there for !insertingQuotedContent, and an image element, without the extra check for moz-do-not-send?

If not, no need to explain the reasons, because I probably wouldn't understand :-) Just a thought.

Comment 12

[Jorg K (CEST = GMT+2)]

Sadly that won't work since insertingQuotedContent==true is only returned during the brief moment when the content of the compose window is prepared after you clicked "Reply". Later on, while you're sitting in front of the screen thinking about what to reply, this is returned 'false' and all pictures would be permitted if you - say - double click one or cause it to re-render in any other way, not that I can think of one right now.

The solution would be to a) remove the faulty moz-do-not-send processing, and b) while insertingQuotedContent==true create an internal list of blocked content to keep blocking that content, even when insertingQuotedContent==false later. New User-inserted content wouldn't be in the list and therefore not blocked.

I'll look into it further, this bug is now on my list and I should be able to fix it for TB 52 due out in March 2017.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: WIP: 609406-dont-block-image-in-reply.patch

Remove faulty moz-do-not-send processing in nsMsgContentPolicy::ComposeShouldLoad(). I still need to implement a list to keep track of initially blocked content.

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8820300 [details] [diff] [review]
WIP: 609406-dont-block-image-in-reply.patch

Hi Magnus, what do you thinks of this patch?

Clearly, checking moz-do-not-send and only showing an image without it is wrong.

So with this patch the following happens: When the reply content is inserted into the message, 'insertingQuotedContent' is true, hence the remote images are blocked (unless permitted by the whitelist which is checked later).

When the user inserts an image into the reply, it is displayed as it should.

There is one side-effect of the patch. The initially blocked images in the message become visible, when the user double-clicks one or opens the properties of the image.

I think we could even classify this as a desired result. Someone replies to a message with remote content and doesn't see this content (which is a silly situation to start with, but anyway, there might be an invisible tracking image in the message). So now the user actually wants to see what how the image he/she is sending out looks like. So they double-click it.

The alternative would be a sophisticated list to keep track of images that were originally in the composition and were blocked.

The problem is that the nsMsgContentPolicy object is only instantiated once per session, not once per composition. So we'd have to glue the list into the compose window somehow (with a service on the compose window that can answer whether the image was originally blocked or not), or have a list owned by the nsMsgContentPolicy that's indexed by the compose window. Then you have the problem of removing the images from the list when the compose window dies.

Thoughts welcome.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 8820300 [details] [diff] [review]
WIP: 609406-dont-block-image-in-reply.patch

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

I think this is correct. Doesn't seem correct that having the moz-do-not-send attr would result in "accepted". 

But then, I don't know if I understand this bug. An image you just pasted wouldn't have that attribute, so how does this help?

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #15)
> But then, I don't know if I understand this bug. An image you just pasted
> wouldn't have that attribute, so how does this help?
It depends on how you insert the image. If you paste it, it has no moz-do-not-send attribute and shows.

If you insert it via the dialogue with "do not attach", it gets the attribute and doesn't show.

The problem is whether we want to accept the side-effect: Double-clicking an initially blocked image will start to show it.

mozmake SOLO_TEST=content-policy/test-general-content-policy.js mozmill-one
still passes, so if you'd like to upgrade your f+ to an r+ we can land it as proposed.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 8820300 [details] [diff] [review]
WIP: 609406-dont-block-image-in-reply.patch

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

Well it only shows the image if you hit "ok" in the properties dialog. 

Looked it over again, and this seems fine. You should however update the comment above (remove the part starting "Fortunately,...)

Content policy changes should come with a test though as it's a tricky area.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 609406-dont-block-image-in-reply.patch (v1b)

Fixed comment.

Magnus, what sort of test would you like? That images in replies are generally blocked is already covered in test-general-content-policy.js which still passes.

So do you want a test that images freshly inserted into a reply are visible? The code is pretty clear and a manual test shows that it works.

The worst regression we could have it that they go invisible again which is the current state, so no harm done.

Comment 19

[Jorg K (CEST = GMT+2)]

C-C (TB 54): https://hg.mozilla.org/comm-central/rev/06666873a81d1b12cd0f82cf8406015499db1609
C-A (TB 53): https://hg.mozilla.org/releases/comm-aurora/rev/b962bde0c77fdfc1ca8b70ac7163d4e8bca2ea59
C-B (TB 52): https://hg.mozilla.org/releases/comm-beta/rev/efc36765d7aa0d5dbdcb2e8ace2fae9e92bb574a

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8830507 [details] [diff] [review]
609406-dont-block-image-in-reply.patch (v1b)

Pretty old and annoying bug. Will follow up with test.

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #17)
> Well it only shows the image if you hit "ok" in the properties dialog. 
Right, but opening the properties does load the image to show the preview and your privacy is gone.

Anyway, I checked this with TB 45. It also loads the image to show the preview, and if you go "OK" on the panel, the image shows as well. So we've actually not made it any different to how it was already, only that now inserted images will show up immediately as they should.

Let me know what kind of test you want and I'll add it.

Comment 22

[Magnus Melin [:mkmelin]]

I wouldn't worry about that. You can't both allow curiosity and total privacy. Anyway the privacy issue is more with tracking images.

The test could open a reply to an email containing remote content, then verify that images are blocked, add two images, one through paste and one through the dialog - and both should be shown. test-compose-mailto.js does similar things but not with a reply. Oh, and the same for forward.

Comment 23

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #22)
> The test could open a reply to an email containing remote content, then
> verify that images are blocked, ...
That's already covered in test-general-content-policy.js which still passes ;-)

I'll do the other one.

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 609406-dont-block-image-test.patch (v1).

Copy/paste was easier than I thought in this case ;-)

Sorry about the hasty landing of the code. I wanted this in TB 52 beta since I promised the reporter to fix it ;-)

Comment 25

[Wayne Mery (:wsmwk)]

> Sorry about the hasty landing of the code. I wanted this in TB 52 beta since I promised the reporter to fix it ;-)

I'm not at all keen on this as a rationale for landing on beta with absolutely no user testing for a 6 year old bug.
But having voiced my opinion, I'm also not going to waste time arguing about it. The beta needs to go out.

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 8830797 [details] [diff] [review]
609406-dont-block-image-test.patch (v1).

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

::: mail/test/mozmill/content-policy/test-general-content-policy.js
@@ +479,5 @@
> +function subtest_insertImageIntoReplyForward(aReplyType) {
> +  let msgDbHdr = addToFolder("Test insert image into reply or forward",
> +                             "Stand by for image insertion ;-)",
> +                             folder);
> +  ++gMsgNo;

nit: would prefer gMsgNo++; style

@@ +529,5 @@
> +
> +  // Now wait for the paste.
> +  replyWindow.waitFor(function() {
> +    let img = replyWindow.e("content-frame").contentDocument.getElementById("tmp-img");
> +    return true;

huh??

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 609406-dont-block-image-test.patch (v2).

I (badly) copied your code ;-)
https://dxr.mozilla.org/comm-central/rev/756fa9e390ab08f995382c9a9dfc6bf4b72421ca/mail/test/mozmill/composition/test-blocked-content.js#100

Comment 28

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/06666873a81d1b12cd0f82cf8406015499db1609 (comment #19)
https://hg.mozilla.org/comm-central/rev/1d0683ac7b096db2c99988c9d8cdf4423abdf87f (test)

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8834119 [details] [diff] [review]
609406-dont-block-image-test.patch (v2).

Let's take the test to the branches, too.

Comment 30

[Jorg K (CEST = GMT+2)]

Aurora (TB 53):
https://hg.mozilla.org/releases/comm-aurora/rev/b962bde0c77fdfc1ca8b70ac7163d4e8bca2ea59 (comment #19)
https://hg.mozilla.org/releases/comm-aurora/rev/d622687456eb6da504f9b057b682b9bb88b38064 (test)

Comment 31

[Jorg K (CEST = GMT+2)]

Beta (TB 52):
https://hg.mozilla.org/releases/comm-beta/rev/efc36765d7aa0d5dbdcb2e8ace2fae9e92bb574a (comment #19)
https://hg.mozilla.org/releases/comm-beta/rev/c2efb67bd9f341aac7e8efd00ef0156f50289e2f (test)