If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Updating to 2.39B allow images not working

RESOLVED FIXED in seamonkey2.43

Status

SeaMonkey
MailNews: Message Display
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: Steve Manser, Assigned: frg)

Tracking

SeaMonkey 2.39 Branch
seamonkey2.43

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8685966 [details]
151111 SeaMonkey Mail issue.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_1) AppleWebKit/601.2.7 (KHTML, like Gecko) Version/9.0.1 Safari/601.2.7

Steps to reproduce:

Upgraded from 2.38 to 2.39 (UK British localisation), go into mail, view mail with images, see yellow bar showing blocked content


Actual results:

Choose to show content from email address, has no effect so have to select 'Show remote content for this message' to see it, however you have to do this for every message as it is not retained choice


Expected results:

When I selected show content from this email address it should have done this and then allowed content from that address for any future emails from this source (worked beautifully in SeaMonkey 2.38.0 (UK British))

Comment 1

2 years ago
Symptoms sound similar to "Bug 1135387 - Checkmark "Allow remote content" without effect", but that problem appeared long before 2.39

@reporter:
please test relation to Bug 1135387 and contribute a step by step instruction containing every key press and every mouse click how to reproduce your problem due to <https://developer.mozilla.org/en-US/docs/Mozilla/QA/Bug_writing_guidelines>  (similar to report in Bug 1139273)
Flags: needinfo?(smanser)
See Also: → bug 1135387

Comment 2

2 years ago
I read several comments that 2.38 was the only version where  "Allow remote content" worked correctly. 

I wonder whether in 2.39 the old problem reappeared or a new one appeared (with old symptoms)

Comment 3

2 years ago
Seem for me that the problem has to do with the format of "Permission Data Record".
I can see several scenarios:
If I do an "Allow remote content for ..." no "Permission Data Record" can be found neither under 2.39 as under 2.38 .
If I do an "Allow remote content for ..." under 2.38 no "Permission Data Record" can be found under 2.39 but can be seen under 2.38 .
I can see under 2.39 some "Permission Data Records" with "Load Images" entries that are not honored.
I have at least one "Permission Data Record" with "Load Images" entries that works under 2.39
(Sorry but I do not know under with version this entries where added.)

Comment 4

2 years ago
Worked fine until v 2.39, and for years at that.  Creating Remote Content group in Address Book years ago, then 2.39 upgrade and it does not hold.  Have to recheck Allow Remote Content every mail message.  
Very frustrating.
(Assignee)

Comment 5

2 years ago
Could someone check if he/she sees a similar error in the web console when trying to unblock the content:

>> Timestamp: 3/30/2016 7:24:44 PM
>> Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) 
>> [nsIPermissionManager.add]
>> Source File: chrome://messenger/content/mailWindowOverlay.js
>> Line: 2794

Permission api did change in 2.39 and it looks like the code here is wrong. If I have some time I will look into it this weekend. 

Confirming bug. Tested on 2.45a1 x64 Windows 7.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 6

2 years ago
Looked at it a bit more. I think this fails with https:// and other non http hosts currently. Thunderbird code is completely different here. Seamonkey needs either a big fix or a straight port.

Comment 7

2 years ago
Confirmed in 2.39 W32 (XP sp3).  Also confirmed problem not present in 2.38.  Unfortunately this bug makes SeaMonkey Mail too frustrating for me to use everyday.  Have reverted to 2.38 and disabled auto updates until this bug is fixed.
Thanks for looking into it, guys.  Keep up the good work.  (Wish I could help, but I've been out of the software development biz for too many years.)
(Assignee)

Comment 8

2 years ago
TB fixed this in bug 1193200. They added a workaround using "chrome://messenger/content/?email=" to allow email addresses to be whitelisted using the permissions api.

Ian, do we want to also do this or should we just make the website image permissions stick by replacing the internal use of hosts with origins and remove the option to whitelist email addresses? The later needs to be done anyway.

The fixed Data Manager will currently not like chrome:// urls but I plan to fix this and some quirks later anyway.
Flags: needinfo?(iann_bugzilla)
See Also: → bug 1193200
(Assignee)

Updated

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

Updated

2 years ago
Assignee: nobody → frgrahl

Comment 10

2 years ago
See also Bug 995737 - Adapt seamonkey for the addressbook remote content policy change; use the permission manager instead of address book property - which was fixed in SM 2.38.  Remote content option is remembered in 2.38 but not in 2.39.  Regression?
(Assignee)

Comment 11

2 years ago
I am quite sure the issues fixed in bug 995737 and popping up in 2.39 again will go away if the issue here is fixed. Will test when a patch is ready.
See Also: → bug 995737
(Assignee)

Comment 12

2 years ago
Created attachment 8739685 [details] [diff] [review]
1223741-mailnews-part.patch

Port for the mail part of Bug 1193200. I do not use Seamonkey for mail. Tried it with my gmx account and imap. Unblocking seems to work and stick between sessions.

I added a debug preference. Let me know if this is ok and if yes can stay in. Helped me to see what content needs to be unblocked and what goes on. 

The fixed Data Manager in 2.45a1 just ignores the chrome:// permissions right now. I will think about it but will pursue this in a separate bug.
Attachment #8739685 - Flags: review?(mnyromyr)
(Assignee)

Updated

2 years ago
Attachment #8739685 - Flags: review?(mnyromyr) → review?(philip.chee)
(Assignee)

Comment 13

2 years ago
Created attachment 8742123 [details] [diff] [review]
1223741-mailnews-part-V2.patch

Version 2 patch with logging reworked.

The Data Manager in 2.45 will not choke on the xul permission but as stated will be patched up to display it and other non httpx permissions properly in a followup bug.
Attachment #8739685 - Attachment is obsolete: true
Attachment #8742123 - Flags: review?(philip.chee)
Attachment #8739685 - Flags: review?(philip.chee)

Comment 14

a year ago
Comment on attachment 8742123 [details] [diff] [review]
1223741-mailnews-part-V2.patch

>     let menuitem = document.createElement("menuitem");
>     menuitem.setAttribute("label",
>+    messengerBundle.getFormattedString("remoteContentAllow",
>+      [origin.replace("chrome://messenger/content/?email=", "")]));
Nit: indentation

> function allowRemoteContentForURI(aItem)
> {
>-  var host = aItem.getAttribute("host");
>-  if (!host)
>+  log.debug("Start allowRemoteContentForURI");
>+
>+  var origin = aItem.getAttribute("value");
>+
>+  if (!origin)
>     return;
> 
>-  var scheme = host.includes("@") ? "mailto:" : "http://";
>-  var uri = Services.io.newURI(scheme + host, null, null);
Would there still be items that need http:// ?
Flags: needinfo?(iann_bugzilla) → needinfo?(frgrahl)

Updated

a year ago
Status: NEW → ASSIGNED

Updated

a year ago
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Comment 15

a year ago
Created attachment 8744717 [details] [diff] [review]
1223741-mailnews-part-V2a.patch

Indention fix only.

>> Would there still be items that need http:// ?

Imho no. Previously the set did consist of only hosts. With the fix we switch it to origins made out of URIs. If some weirdo URI is passed to setRemoteContentMsg and a host couldn't be found uri.spec is used. The permissions api seems to eat almost everything so even files and chrome works as long as it has a ://. If it does fail here the content will not be unblocked but if someone finds such a case we might meed another workaround like with mailto anyway and not just a http://. For any other case we need to use the original scheme which must not be replaced by something else or unblocking will not work.
Attachment #8742123 - Attachment is obsolete: true
Attachment #8742123 - Flags: review?(philip.chee)
Flags: needinfo?(frgrahl)
Attachment #8744717 - Flags: review?(philip.chee)

Comment 16

a year ago
Comment on attachment 8744717 [details] [diff] [review]
1223741-mailnews-part-V2a.patch

> +    let origin;
use var here.

> +    try
> +    {
> +      origin = aContentURI.scheme + "://" + aContentURI.hostPort;
> +    }
> +    // No hostport so likely a special url. Try to use the whole url and see
> +    // what comes of it.
> +    catch (e)
> +    {
> +      origin = aContentURI.spec;
> +    }

This could be written as:
var origin = aContentURI.spec;
try
{
  origin = aContentURI.scheme + "://" + aContentURI.hostPort;
}
catch (e) { };

> +  let authorEmailAddressURI = Services.io.newURI(
> +    "chrome://messenger/content/?email=" + authorEmailAddress, null, null);
Use "var" here. Suggest use shorter variable names e.g. emailURL, Principal.
var emailURI = Services.io.newURI(
  "chrome://messenger/content/?email=" + authorEmailAddress, null, null);

> +  let mailPrincipal = Services.scriptSecurityManager
> +                              .createCodebasePrincipal(authorEmailAddressURI, {});
var principal = Services.scriptSecurityManager
                        .createCodebasePrincipal(emailURI, {});

> -    hosts.unshift(authorEmailAddress);
> +  origins.splice(0, 0, mailPrincipal.origin);
Please use unshift here.

>      let menuitem = document.createElement("menuitem");
>      menuitem.setAttribute("label",
> -      messengerBundle.getFormattedString("remoteContentAllow", [host]));
> -    menuitem.setAttribute("host", host);

> +    messengerBundle.getFormattedString("remoteContentAllow",
> +      [origin.replace("chrome://messenger/content/?email=", "")]));
I think you need to indent the above two lines two more spaces.

> +    menuitem.setAttribute("value", origin);

I'd like to see a new patch with all the debug statements removed before checkin. Thanks.
Attachment #8744717 - Flags: review?(philip.chee) → review-
(Assignee)

Comment 17

a year ago
Created attachment 8745485 [details] [diff] [review]
1223741-mailnews-part-V3.patch

>> This could be written as:
>> var origin = aContentURI.spec; 
>> ...

Changed it but was intentional originally. I am always uneasy with these constructs because I am not sure if there aren't rare cases where the original variable is somehow touched or altered during the assignment when it fails. 

>> I'd like to see a new patch with all the debug statements removed before checkin.

I am a fan of them unless they are in performance critical code which wasn't the case here but removed. You are the boss :)

If the patch is ok it should go into c-a too. It itself would be safe for the other branches too but I didn't test it with the broken Data Manager in them yet.
Attachment #8744717 - Attachment is obsolete: true
Attachment #8745485 - Flags: review?(philip.chee)

Comment 18

a year ago
Comment on attachment 8745485 [details] [diff] [review]
1223741-mailnews-part-V3.patch

> +  let emailURI = Services.io.newURI(
s/let/var/
> +    "chrome://messenger/content/?email=" + authorEmailAddress, null, null);
> +  let principal = Services.scriptSecurityManager
s/let/var/

>      menuitem.setAttribute("label",
> -      messengerBundle.getFormattedString("remoteContentAllow", [host]));
> -    menuitem.setAttribute("host", host);
> +      messengerBundle.getFormattedString("remoteContentAllow",
> +        [origin.replace("chrome://messenger/content/?email=", "")]));
I've decided that this lacks a certain clarity. Lets use something like this:

let host = origin.replace("chrome://messenger/content/?email=", "");
let hostString = messengerBundle.getFormattedString("remoteContentAllow", [host]);
menuitem.setAttribute("label", hostString);

r=me with the above nits fixed.
Attachment #8745485 - Flags: review?(philip.chee) → review+
(Assignee)

Comment 19

a year ago
Created attachment 8747458 [details] [diff] [review]
1223741-mailnews-part-V3a.patch

nits fixed.

review+ from Philip Chee carried over.
Attachment #8745485 - Attachment is obsolete: true
Flags: needinfo?(smanser)
Attachment #8747458 - Flags: review+
(Assignee)

Comment 20

a year ago
checked-in

https://hg.mozilla.org/comm-central/rev/b120a5b361b8
Keywords: leave-open
(Assignee)

Comment 21

a year ago
Comment on attachment 8745485 [details] [diff] [review]
1223741-mailnews-part-V3.patch

[Approval Request Comment]
Regression caused by (bug #): 1165263
User impact if declined: remote content can not be unblocked.
Testing completed (on m-c, etc.): c-a
Risk to taking this patch (and alternatives if risky): none its borken already
String changes made by this patch: none
Attachment #8745485 - Flags: approval-comm-aurora?
(Assignee)

Comment 22

a year ago
Comment on attachment 8747458 [details] [diff] [review]
1223741-mailnews-part-V3a.patch

[Approval Request Comment]
Regression caused by (bug #): Bug 1165263
User impact if declined: remote content can not be unblocked.
Testing completed (on m-c, etc.): c-a
Risk to taking this patch (and alternatives if risky): none its broken already
String changes made by this patch: none
Attachment #8747458 - Flags: approval-comm-aurora?
(Assignee)

Updated

a year ago
Attachment #8745485 - Flags: approval-comm-aurora?

Updated

a year ago
Attachment #8747458 - Flags: approval-comm-aurora? → approval-comm-aurora+
(Assignee)

Comment 23

a year ago
Checked into c-a:

https://hg.mozilla.org/releases/comm-aurora/rev/b9e78456cca7

I am leaving the bug open to see if it can be pushed to beta and release too. Need to check that the unfixed data manager does not choke on it.
(Assignee)

Comment 24

a year ago
Created attachment 8747465 [details]
Capture.PNG

[Approval Request Comment]
Regression caused by (bug #): Bug 1165263
User impact if declined: remote content can not be unblocked.
Testing completed (on m-c, etc.): c-r c-b
Risk to taking this patch (and alternatives if risky): none its broken already
String changes made by this patch: none

The unfixed Data Manager displays a messenger host for the permission and lists it under a messenger domain. The permission can not be removed but I do not consider this a big problem because permission handling is practically broken for any non http scheme in 2.43 and 2.44 anyway :)

Tested with 2.43 VS2015 x64 build under Windows 10.
Attachment #8747465 - Flags: approval-comm-release?
Attachment #8747465 - Flags: approval-comm-beta?

Comment 25

a year ago
Comment on attachment 8747465 [details]
Capture.PNG

a=me CLOSED TREE in case you need it.
Attachment #8747465 - Flags: approval-comm-release?
Attachment #8747465 - Flags: approval-comm-release+
Attachment #8747465 - Flags: approval-comm-beta?
Attachment #8747465 - Flags: approval-comm-beta+
(Assignee)

Comment 26

a year ago
Done
https://hg.mozilla.org/releases/comm-beta/rev/fc1290dc036d
https://hg.mozilla.org/releases/comm-release/rev/3dec13956227
https://hg.mozilla.org/releases/comm-esr45/rev/e9db074af6c9
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Keywords: leave-open
Resolution: --- → FIXED
(Assignee)

Updated

9 months ago
Target Milestone: --- → seamonkey2.43
You need to log in before you can comment on or make changes to this bug.