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
2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

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

Comment 7

2 years 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

2 years 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

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

Comment 10

2 years 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

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

Comment 12

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

Updated

2 years ago
Duplicate of this bug: 609406
(Assignee)

Updated

2 years ago
Duplicate of this bug: 610376
(Assignee)

Updated

2 years ago
Duplicate of this bug: 642566
(Assignee)

Comment 16

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

Comment 17

2 years 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

2 years 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

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

Comment 20

2 years 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

2 years ago
Aurora (TB 46)
https://hg.mozilla.org/releases/comm-aurora/rev/7fcbe733ca61
status-thunderbird46: affected → fixed

Comment 22

2 years ago
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

2 years 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.