1251408 - Replying or forwarding a message with external image doesn't not show the image in the composition

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

Attachment: Test e-mail with external image

Replying or forwarding a message with external image (for example sent with img moz-do-not-send="true") doesn't not show the image in the composition although the image is shown when received (assuming that images from that site are allowed).

Or take any HTML e-mail with external images and reply.

Is this some origin/security problem?

This does not happen when you check:
Tools > Options, Privacy tab, Allow remote content in messages.

Surely if the images are visible when viewing the message they should be visible when replying/forwarding.

Comment 1

[Jorg K (CEST = GMT+2)]

Magnus, can you take a look at this. You worked on this area. It's really quite annoying that you don't see the images in your reply.

Comment 2

[Jorg K (CEST = GMT+2)]

Sadly, the last time this has worked was in TB 24. It stopped working with TB 31, not working in TB 38, TB 46 or latest trunk. And no one has noticed or complained.

Comment 3

[Jorg K (CEST = GMT+2)]

This is bad enough to allow tracking for TB 45.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1).

This does the trick. I've we're composing, we also test the whitelist of sites.

Comment 5

[Jorg K (CEST = GMT+2)]

I'm just looking at the blame of nsMsgContentPolicy.cpp. A few bugs in the area:
Bug 374578, bug 605140 and most prominently bug 953426. Looks like in bug 953426 (which landed on TB 31) you forgot to apply the "per host" privileges to the message being composed.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 8723858 [details] [diff] [review]
Proposed solution (v1).

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

I think you should add the permission manager tests to ComposeShouldLoad instead of falling through, to keep the logic more simple.

nsMsgContentPolicy.cpp changes are really required to include a test though (see mail/test/mozmill/content-policy/)

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2).

(In reply to Magnus Melin from comment #6)
> I think you should add the permission manager tests to ComposeShouldLoad
> instead of falling through, to keep the logic more simple.
Agreed. The patch looks big due to reshuffling of if-blocks, but it's really only this:
+        // Test whitelist.
+        uint32_t permission;
+        mPermissionManager->TestPermission(aContentLocation, "image", &permission);
+        if (permission == nsIPermissionManager::ALLOW_ACTION)
+          *aDecision = nsIContentPolicy::ACCEPT;

> nsMsgContentPolicy.cpp changes are really required to include a test though
> (see mail/test/mozmill/content-policy/)
How about doing this next week? Right now I'm far away from home (2000 km). Would be nice to have this while we're still on TB 47 so uplift is simpler.

Comment 8

[Magnus Melin [:mkmelin]]

Generally it's not a good idea to land tests independently from the code fix, since they should verify the code works. In this case the test would have to land later anyway so there's no win in a multi-landing.

Comment 9

[Magnus Melin [:mkmelin]]

Enjoy your trip though, this has apparently been broken for years so there's no immediate hurry :)

Comment 10

[Jorg K (CEST = GMT+2)]

Well, the immediate hurry is that if it doesn't land now, it will be harder to get into 45.x.

Comment 11

[Jorg K (CEST = GMT+2)]

Meanwhile, I've done a try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=54cf548777a4

Comment 12

[Jorg K (CEST = GMT+2)]

Bad luck, test have just gone bust, so that run doesn't tell us a thing.

Comment 16

[Jorg K (CEST = GMT+2)]

A quick check already found three duplicates. The hurry *is* that's it's been broken for year.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2) and modified test.

OK, the solution is unchanged from the previous version, and I added a test.

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 8726163 [details] [diff] [review]
Proposed solution (v2) and modified test.

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

Looks good, I'll land it in a bit. r=mkmelin

Comment 19

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/278bc390b99b -> FIXED

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8726163 [details] [diff] [review]
Proposed solution (v2) and modified test.

[Approval Request Comment]
Regression caused by (bug #): Bug 953426.
User impact if declined: Very annoying behaviour that images don't show on reply/forward.
Testing completed (on c-c, etc.): Manual + adapted test.
Risk to taking this patch (and alternatives if risky):
Minimal clear code change, little risk.

Comment 21

[Jorg K (CEST = GMT+2)]

Aurora (TB 46)
https://hg.mozilla.org/releases/comm-aurora/rev/7fcbe733ca61

Comment 22

[Kent James (:rkent)]

Comment on attachment 8726163 [details] [diff] [review]
Proposed solution (v2) and modified test.

http://hg.mozilla.org/releases/comm-esr45/rev/daf28448957b