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

RESOLVED FIXED in Thunderbird 47.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
a year ago
a year ago

People

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

Tracking

38 Branch
Thunderbird 47.0

Thunderbird Tracking Flags

(thunderbird44 wontfix, thunderbird45+ fixed, thunderbird46 fixed, thunderbird47 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
Created attachment 8723773 [details]
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.
(Assignee)

Comment 1

a year ago
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.
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 2

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

Comment 3

a year ago
This is bad enough to allow tracking for TB 45.
status-thunderbird44: --- → wontfix
status-thunderbird45: --- → affected
status-thunderbird46: --- → affected
status-thunderbird47: --- → affected
tracking-thunderbird45: --- → +
(Assignee)

Comment 4

a year ago
Created attachment 8723858 [details] [diff] [review]
Proposed solution (v1).

This does the trick. I've we're composing, we also test the whitelist of sites.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Attachment #8723858 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 5

a year ago
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

a year ago
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/)
Attachment #8723858 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

a year ago
Attachment #8723773 - Attachment mime type: message/rfc822 → text/plain
(Assignee)

Comment 7

a year ago
Created attachment 8725355 [details] [diff] [review]
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.
Attachment #8723858 - Attachment is obsolete: true
Attachment #8725355 - Flags: review?(mkmelin+mozilla)

Comment 8

a year ago
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

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

Comment 10

a year ago
Well, the immediate hurry is that if it doesn't land now, it will be harder to get into 45.x.
(Assignee)

Comment 11

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

Comment 12

a year ago
Bad luck, test have just gone bust, so that run doesn't tell us a thing.
(Assignee)

Updated

a year ago
Duplicate of this bug: 609406
(Assignee)

Updated

a year ago
Duplicate of this bug: 610376
(Assignee)

Updated

a year ago
Duplicate of this bug: 642566
(Assignee)

Comment 16

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

Comment 17

a year ago
Created attachment 8726163 [details] [diff] [review]
Proposed solution (v2) and modified test.

OK, the solution is unchanged from the previous version, and I added a test.
Attachment #8725355 - Attachment is obsolete: true
Attachment #8725355 - Flags: review?(mkmelin+mozilla)
Attachment #8726163 - Flags: review?(mkmelin+mozilla)

Comment 18

a year ago
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
Attachment #8726163 - Flags: review?(mkmelin+mozilla) → review+

Comment 19

a year ago
https://hg.mozilla.org/comm-central/rev/278bc390b99b -> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-thunderbird47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0
(Assignee)

Comment 20

a year ago
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.
Attachment #8726163 - Flags: approval-comm-beta?
Attachment #8726163 - Flags: approval-comm-aurora+
(Assignee)

Comment 21

a year ago
Aurora (TB 46)
https://hg.mozilla.org/releases/comm-aurora/rev/7fcbe733ca61
status-thunderbird46: affected → fixed
Comment on attachment 8726163 [details] [diff] [review]
Proposed solution (v2) and modified test.

http://hg.mozilla.org/releases/comm-esr45/rev/daf28448957b
Attachment #8726163 - Flags: approval-comm-beta? → approval-comm-beta+

Updated

a year ago
status-thunderbird45: affected → fixed
(Assignee)

Updated

a year ago
Attachment #8726163 - Flags: approval-comm-beta+ → approval-comm-esr45+
You need to log in before you can comment on or make changes to this bug.