1193200 - Remote content exceptions: Cannot add email address to blocklist with Block and Allow buttons; adding web site seems to succeed but also invisible

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #457296 +++

STR

1) Tools > Options > Privacy > Mail Content > Exceptions - Remote Content
2) type email address [e-mail] into input box
3) Block or Allow

Actual Result

nothing, not added to block list

Expected Result

email address added to block list

fyi, I had the general option "[ ] Allow remote content in messages" NOT checked but that shouldn't matter, and it makes no difference, bug occurs either way.

Comment 1

[Thomas D. (:thomas8)]

Adding a website seems to respond but also nothing seen in the blocklist thereafter.

Comment 2

[Thomas D. (:thomas8)]

Seen on 42.0a1 (2015-08-09) Win 8.1-64

Comment 3

[Thomas D. (:thomas8)]

Works on release version 38.1.0, but not on trunk

Also note the trunk version where it fails has an empty block list (whereas my release version has previous entries)

Comment 4

[Alice0775 White]

Good:
https://hg.mozilla.org/mozilla-central/rev/eab21ec484bb
https://hg.mozilla.org/comm-central/rev/78682db44be6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Thunderbird/42.0a1 ID:20150712030206


Bad:
https://hg.mozilla.org/mozilla-central/rev/8e5c888d0d89
https://hg.mozilla.org/comm-central/rev/1c60cc0002ba
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:42.0) Gecko/20100101 Thunderbird/42.0a1 ID:20150722030208


Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=eab21ec484bb&tochange=8e5c888d0d89
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=78682db44be6&tochange=1c60cc0002ba

Regressed by:
d4cc20fc3973	Michael Layzell — Bug 1185343 - Part 1: Use new table moz_perms for permissions, and leave unmigrated permission data in moz_hosts for back-compat. r=ehsan

Comment 5

[Nika Layzell [:nika] (ni? for response)]

The ability to store mailto: URIs was removed from the permission manager in bug 1165263 as mailto: URIs aren't supported by the nsIPrincipal.origin property which is now being used for permission manager storage (as mailto: URIs don't have a meaningful origin). I'm not sure if there is a way to adapt the new permission manager internals to work for white lists for Thunderbird.

Comment 6

[Kent James (:rkent)]

I did a post to tb-planning suggesting that we create the tracking-thunderbird_esr45 flags so that we can track issues like this. We really are not watching the tracking flags for TB 39 - 44 at the moment.

Comment 7

[Jorg K (CEST = GMT+2)]

This is annoying since the on message with remote content you get:
"To protect your privacy, Thunderbird has blocked remote content in this message." - [Options].
Well, setting options ain't working any more.

All the exceptions that show up in TB 41 don't show up in TB 42 any more.
(Tools > Options > Privacy > Mail Content > Exceptions).

Kent, someone needs to get in touch with the M-C core guys to work out a solution here.
M-C was changed (comment #5) and TB was left out standing in the rain.

Comment 8

[Kent James (:rkent)]

(In reply to Jorg K (Thunderbird C-C contributions suspended) from comment #7)
 
> Kent, someone needs to get in touch with the M-C core guys to work out a
> solution here.
> M-C was changed (comment #5) and TB was left out standing in the rain.

Someone from the mailnews side needs to understand this and propose ways to fix this. m-c was quite intentional about dropping mailto: ulrs as I understand it.

Comment 9

[Jorg K (CEST = GMT+2)]

Neil, perhaps you know someone to look into it. Maybe yourself.

Comment 10

[[e-mail]]

I don't even know why remote content was changed to use the permission manager in the first place.

Comment 11

[Jorg K (CEST = GMT+2)]

There seem to be two problems here:

(In reply to Jorg K (GMT+2) from comment #7)
> This is annoying since the on message with remote content you get:
> "To protect your privacy, Thunderbird has blocked remote content in this
> message." - [Options].
> Well, setting options ain't working any more.

Actually, it's not quite so bad.
There are two types of exception: mailto: and "normal" URL.
Due to the M-C core changes, the mailto: type isn't supported any more, so we should remove this from the menu:
http://dxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#3167
https://dxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#3182

If in a message with remote content, I select a "normal" URL from the popup, the message is actually displayed correctly (in some/most cases).

The second problem is that the "Exceptions - Remote content" panel isn't working, so it's not showing what was added.

I'll have a look at both problems.

(Note that even in Firefox this has caused problems when switching between versions 41 and 42, see bug 1185343.)

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: WIP: Part 1: Don't add "mailto:" addresses to menu and don't check whitelist for senders

I've made a start.

Since M-C doesn't support mailto: URIs any more, don't offer to add them and also don't query permissions with such URI (since it crashes!!).

Part 2 will be getting the panel working again.

Comment 13

[Magnus Melin [:mkmelin]]

Somewhat simplified, permission manager used "host" (ignoring protocol), now it's using "origin", which is kind of protocol+host, right? 

So if that's correct it shouldn't be that hard to make it work again for mailto:
It's just that we had to kind of hack it before to make hosts with @ be assumed to be mailto: uris.

Comment 14

[Jorg K (CEST = GMT+2)]

Not sure what you're saying.

Before there was table "moz_hosts" with column "host". There we stored for for example |[e-mail]| and |aktuell.conrad.de|.

Now it's table "moz_perms" with column "origin". There the full URL gets stored, for example |http://henk.com.au|.

Comment #5 says that mailto: URIs *CANNOT* be stored.

So IMHO we just have to *remove* the facility to store the sender and be done with it.

Magnus: If you have another plan, please go ahead. I understood that you wanted to look at it anyway.

Please let me know how you want to proceed.

Note:
Storing mailto: URI does actually not provide any security. Attackers could send e-mail with a false address.

Note 2: On the Earlybird 42 I am using, I can store site URLs but not mailto: URIs. If I allow the remote content from the picture-providing URL, the message gets displayed correctly. Doing the same in my Daily compiled version, I get a crash in ShouldAcceptRemoteContentForSender(). So I removed the call in the patch since, as no mailto: URIs are stored, it provides no benefit.

Note 3: For the record: The panel that also needs to be fixed is here:
https://dxr.mozilla.org/comm-central/source/mail/components/preferences/permissions.js

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: WIP: Part 2: Fix permission manager, provide dummy host.

Looks like the permissions don't return the host name any more. We get:

key=|QueryInterface|, value=|function QueryInterface() {[native code]}|
key=|type|, value=|image|
key=|principal|, value=|[xpconnect wrapped (nsISupports, nsIPrincipal, nsISerializable) @ 0xd084d50 (native @ 0x8a46260)]|
key=|capability|, value=|1|
key=|expireType|, value=|0|
key=|expireTime|, value=|0|
key=|matches|, value=|function matches() {[native code]}|
key=|matchesURI|, value=|function matchesURI() {[native code]}|

Putting in a dummy host makes the panel work again.

Micheal, can you please tell me how to dig out the host/origin? Please look at the patch, it's obvious what we're trying to do. Is there some documentation or sample to look at?

Thanking you in advance.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: Fix permission manager. One line change.

Michael, sorry, I figured it out:
Instead of permission.host it's permission.principal.origin.
It's a one line change.

Now we need to figure out what we want to do with part 1, that is, whether we want to drop mailto: URIs or come up with some hack to squeeze them into the new scheme.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: WIP: Part 1: Don't add "mailto:" addresses to menu, don't check whitelist for senders, add http: and https:

OK, now we have:

Part 1:
- Don't offer to add mailto: exceptions.
- Always add the http:// and the https:// URL at the same time
  (since pictures can come via https)
- don't call ShouldAcceptRemoteContentForSender() since it crashes.

Part 2:
- Fix the exceptions panel (one line change).

Magnus, let me know how you want to proceed. I won't stick feedback? onto the patches.

If you're happy with the approach, I will fix part 1 (WIP) a little more and then present both for review (or merge them).

If you have another plan, please let me know.

Comment 18

[Jorg K (CEST = GMT+2)]

I just saw: addPermission in preferences.js (line 72) needs to change to add http: and https: versions. Not problem, I'll add it as soon as I have feedback on the plan of attack.

Comment 19

[Jorg K (CEST = GMT+2)]

More problems: Removals also don't work. The site is removed from the list, but next time you bring the panel up. it's still there. Anway, all small fry, each problem perhaps a few lines to fix.

The major question here is: What do we do with mailto: URIs. Not support them any more or what?

Comment 20

[Magnus Melin [:mkmelin]]

We "just" have to figure out how to have the principal.origin set for mailto: uris. 

I wouldn't put any work into removing the functionality, that should be the very last resort.

Comment 21

[Jorg K (CEST = GMT+2)]

"Just" seems impossible. They removed "mailto:" URIs. The end.

The only thing you can do is hack it:
[e-mail] becomes: http://mailto-abc.def.com under the covers.

Do you want that?

Mind you, ShouldAcceptRemoteContentForSender() is in mailnews/base, so we need to ask Neil and Ian.

Comment 22

[Jorg K (CEST = GMT+2)]

Neil just told me on IRC: Before we looked in the address book:
http://hg.mozilla.org/comm-central/rev/ec82de81c636
So we might have to go back to that.

Comment 23

[Jorg K (CEST = GMT+2)]

That was part of bug 457296.

Comment 24

[Magnus Melin [:mkmelin]]

Well it was removed because it's evidently unclear exactly how to do it. That doesn't mean we can't add back support in the new world.

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: WIP: Part 1: Hack "mailto:" addresses, add http: and https:

Magnus, frankly, can you write more than one line.

So far I presented two options, there are two more to investigate:
Presented:
Option 1: Remove the feature to allow remote content by sender.
          This is the most secure option.
Option 2: Hack this feature. Instead of storing "[e-mail]"
          we store: "http://mailto-xx.yy.com". See this patch.

Other options:
Option 3: Go back to storing a flag in the address book.
Option 4: Modify the M-C permission manager to accept "mailto:[e-mail]".

I think option 4 is not an option. They won't allow us to drill open their box. See comment #5.

I will stop any further work on this now and await clear instructions (of more than one line). Normally in software development, you specify what you want and then implement it. You don't implement a variety of solutions and then let someone chose later. I agree that it's early on in the process, so one needs to investigate a few paths, which I've done now.

Comment 26

[:aceman]

(In reply to Jorg K (GMT+2) from comment #21)
> The only thing you can do is hack it:
> [e-mail] becomes: http://mailto-abc.def.com under the covers.
> 
> Do you want that?
What about http://mailto://[e-mail] ? Does the manager accept URLs with @ ? It is an allowed format for input into the browser urlbar (e.g. http://stackoverflow.com/questions/7835992/how-does-http-userpasshost-com-authentication-work). Are such URLs also supported in the new perms manager?

Comment 27

[Jorg K (CEST = GMT+2)]

I changed my patch from
let uri = Services.io.newURI(
  host.includes("@") ? "http://mailto-" + host.replace(/@/, ".") : "http://" + host, null, null);
to
let uri = Services.io.newURI(
  host.includes("@") ? "http://mailto-" + host : "http://" + host, null, null);

I tried to add "[e-mail]", so passing: "http://[e-mail]".
The result that that got stored was "http://xe.com". So no, you can't pass a "@" and have it stored.

Obviously my hack is incomplete. For [e-mail] we would need to store:
http://mailto07-abc.def.xx.com, the "07" being the position where we changed "@" to ".", so we can reverse it later.

Comment 28

[:aceman]

(In reply to Jorg K (GMT+2) from comment #27)
> I tried to add "[e-mail]", so passing: "http://[e-mail]".
> The result that that got stored was "http://xe.com". So no, you can't pass a
> "@" and have it stored.
Looks like they have some bugs in the manager :) It should have rejected strings it can't store, not loose data.

> Obviously my hack is incomplete. For [e-mail] we would need to store:
> http://mailto07-abc.def.xx.com, the "07" being the position where we changed
> "@" to ".", so we can reverse it later.
Yeah, or we could just encode the full email (base64 or something).

Anyway, I understand the sender address can be spoofed so basing any permissions on it can be wrong.
Previously, the user could whitelist the sender address and all remote resources in the email were loaded, regardless of origin.

Is there is still the possibility today to whitelist individual remote resources (or unique protocol+server combinations) in a message? Does the user have to hunt around the images and right-click each of them? Or do we present a list of URLs found?

Comment 29

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #28)
> Anyway, I understand the sender address can be spoofed so basing any
> permissions on it can be wrong.
> Previously, the user could whitelist the sender address and all remote
> resources in the email were loaded, regardless of origin.
Yes.

> Is there is still the possibility today to whitelist individual remote
> resources (or unique protocol+server combinations) in a message? Does the
> user have to hunt around the images and right-click each of them? Or do we
> present a list of URLs found?
The latter. You should try it one day, works already in TB31 to TB38 and TB41 ;-)

The user is presented with a menu:
Edit remote content options
Allow remote content for [e-mail]
Allow remote content for xx.com
(and possibly more: Allow remote content for yy.com).

So if we withdraw the e-mail option, then the user needs to click on (possibly multiple) lines in the menu until the message is rendered with all images.

Comment 30

[:aceman]

(In reply to Jorg K (GMT+2) from comment #29)
> The user is presented with a menu:
> Edit remote content options
> Allow remote content for [e-mail]
> Allow remote content for xx.com
> (and possibly more: Allow remote content for yy.com).
That is good we list all URLs. Some of the images could be hidden and the user may miss them if he had to look for them. (Actually, supporting these webbugs may not be such great idea:))

> So if we withdraw the e-mail option, then the user needs to click on
> (possibly multiple) lines in the menu until the message is rendered with all
> images.

So what about adding a menuitem "Allow remote content in this message", which internally would whitelist all the listed URLs? It would still be stricter than whitelist the email, as if the next message is spoofed (same sender), but the URLs change, the malware URLs will not be loaded.

Comment 31

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #30)
> (Actually, supporting these webbugs may not be such great idea:))
Huh? Webbug??

> So what about adding a menuitem "Allow remote content in this message",
> which internally would whitelist all the listed URLs? It would still be
> stricter than whitelist the email, as if the next message is spoofed (same
> sender), but the URLs change, the malware URLs will not be loaded.
Good idea.

Are you in favour of removing the e-mail option?

So Aceman, do you want to take over? I have other work in the M-C editor. I just started to look at the bug since there was no action.

Comment 32

[:aceman]

(In reply to Jorg K (GMT+2) from comment #31)
> (In reply to :aceman from comment #30)
> > (Actually, supporting these webbugs may not be such great idea:))
> Huh? Webbug??
https://en.wikipedia.org/wiki/Web_beacon

> > So what about adding a menuitem "Allow remote content in this message",
> > which internally would whitelist all the listed URLs? It would still be
> > stricter than whitelist the email, as if the next message is spoofed (same
> > sender), but the URLs change, the malware URLs will not be loaded.
> Good idea.
> 
> Are you in favour of removing the e-mail option?
Yes, if whitelisting by URL becomes equally easy for the user, while it seems to also improve security/privacy.

> So Aceman, do you want to take over?
If the UX guys comment whether this would be acceptable, I can look at it.

> I have other work in the M-C editor.
Great, we need somebody to look into that dark place ;)

Comment 33

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #32)
> > Are you in favour of removing the e-mail option?
> Yes, if whitelisting by URL becomes equally easy for the user, while it
> seems to also improve security/privacy.
I find the current UI quite shocking. So many options. So far, I've always selected the e-mail option, but in the future, I'd select Aceman's new option and be done with it. Most likely the remote images sent to me by [e-mail] also reside on the company' server at images.some-company.com. So when you get the next e-mail from them, the content loads automatically. That's as good as allowing the sender's address, only safer.

> > So Aceman, do you want to take over?
> If the UX guys comment whether this would be acceptable, I can look at it.
I don't see why the UX guys would object to changing a few options on the very same menu.
Magnus has to approve, see comment #25. Summarising the options:

Option 1a: Remove the feature to allow remote content by sender.
          This is the most secure option.
          Add "Allow remote content in this message" to add all referenced websites in one go.
Option 2: Hack this feature. Instead of storing "[e-mail]"
          we store: "http://mailtoNN-xx.yy.com" (NN is the position of the removed @).
Option 3: Go back to storing a flag in the address book.
Option 4: Modify the M-C permission manager to accept "mailto:[e-mail]".

Looks like Aceman and I prefer option 1a. Clean, quick and safe.

Comment 34

[Richard Marti (:Paenglab)]

I'm also for option 1a.

With a one click entry to whitelist all sites in the message helps with newsletters which are sent from different people of one company. Then it works already when a new employee begins to send this newsletter.

Comment 35

[Jorg K (CEST = GMT+2)]

Aceman, when you take over, please keep my comment #18 and comment #19 in mind (copied again):

addPermission in preferences.js (line 72) needs to change to add http: and https: versions.

Removals also don't work. The site is removed from the list, but next time you bring the panel up. it's still there.

Feel free to obsolete my patches.

Comment 36

[Jorg K (CEST = GMT+2)]

Just to mention what Magnus said in the Tuesday meeting (Sept. 8, 2015):
He prefers to fix M-C so we can store mailto: URIs.
Independent of that of that, there are changes we need to make, and the "Allow remote content in this message" option is a good idea.

We could temporarily remove the e-mail option until M-C gets fixed and land a working 80%-solution now.

Comment 37

[Nika Layzell [:nika] (ni? for response)]

The reason why M-C doesn't support mailto: URIs anymore is because they don't have a meaningful origin. Namely, two mailto: principals are not considered equal, and thus it isn't valid to generate an origin string for them. See bug 1172022 for a bit more context.

In the old permission manager, permissions were based on host strings, which due to a loophole could also include mailto: URIs (although they were not hosts), and in the new one they are based on origins (or principals, as they are usually referred to as internally). So, unless you define a meaningful origin for mailto: URIs (which doesn't make too much sense, because mailto: URIs don't have an origin in the html5 spec sense), or add a new URI type which does have a meaningful origin, and is only exposed internally in thunderbird, I don't think you can use the permission manager for this feature.

Comment 38

[:aceman]

(In reply to Jorg K (GMT+2) from comment #36)
> Just to mention what Magnus said in the Tuesday meeting (Sept. 8, 2015):
> He prefers to fix M-C so we can store mailto: URIs.
What would be the benefit of this aside from preserving current whitelists (if the storage format is compatible or can be auto-converted)? Also the cost of maintaining this support outside of m-c could be high. So does he propose to store some encoded version of emails in the manager, as we discussed previously?

Comment 39

[Jorg K (CEST = GMT+2)]

Given comment #37 and after doing some reading about the theory behind the "origin"
https://tools.ietf.org/html/rfc6454
https://en.wikipedia.org/wiki/Same-origin_policy
https://code.google.com/p/html5security/wiki/CrossOriginRequestSecurity
I've come to the conclusion that we won't squeeze the mailto: URIs into M-C, since it doesn't seem to make sense.

IMHO, given that this is about security, option 1a (which is also simple to implement) is the only viable option.

Comment 40

[:aceman]

Magnus, can you describe what your plan is?

Comment 41

[Magnus Melin [:mkmelin]]

My plan is to investigate what m-c changes it takes to get it working again.

Some comments:
 - from what I've seen, the theory that emails from [e-mail] would usually link to images on example.org is just nonsense.  
 - The current UI can list a lot of hosts yes, IFF there's something suspicious going on. That it also meant to be informational, so you can decide if it's just tracking or actually something more useful. With a "for all hosts in this email allow forever" option you'd easily end up allowing extensive tracking as one of the big trackers would end up there within a few mails.
 - In the grand scheme of things, I don't think web bugs actually are that much to be scared of. You go online in the browser and you get identified + tracked across almost any site you visit. That someone knows when and that you read a mail seems minor compared to that.

Comment 42

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #25)
> Magnus, frankly, can you write more than one line.

What, you don't like concise specs? ;)
Seriously though, my time is limited so it may often stand between a quick comment or nothing at all. Certainly feel free to ask more if something's unclear.

Comment 43

[:aceman]

(In reply to Magnus Melin from comment #41)
> My plan is to investigate what m-c changes it takes to get it working again.
> 
> Some comments:
>  - from what I've seen, the theory that emails from [e-mail] would
> usually link to images on example.org is just nonsense.  
I think we do not propose to implement anything from this theory.

>  - The current UI can list a lot of hosts yes, IFF there's something
> suspicious going on. That it also meant to be informational, so you can
> decide if it's just tracking or actually something more useful. With a "for
> all hosts in this email allow forever" option you'd easily end up allowing
> extensive tracking as one of the big trackers would end up there within a
> few mails.
Yes. How is that different to just allowing the senders? You then still allow the trackers.
Yeah, a new sender (that is not yet whitelisted), will have the URL blocked, even if we previously allowed them from some other sender.

Maybe we should store email+URL pairs for maximum security? :)

Comment 44

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #41)
> My plan is to investigate what m-c changes it takes to get it working again.

IMHO, given the background, no m-c will make it work again ever. I'll shout you a beer, should I be wrong. Conceptually they are talking about origins, see comment #39, and mailto: URIs don't fit in, so they dropped them. They won't let us implement a kludge.

I repeat that I am for solution 1a, which is doable NOW with a few C-C changes. My patches already show where work is required.

Comment 45

[Thomas D. (:thomas8)]

Some tentative comments

1) From an UX and security perspective, we want to keep the current menuitem "Show remote content in this message". My understanding is that this bypasses restrictions only for the current message, without permanently creating exceptions for all messages which allow remote content for any email addresses, origins, or what not. (Unfortunately, Jorg skipped that menu in Bug 1193200 Comment 29). To be clear, this option is NOT the same as "Allow remote content in this message", which in the above discussion was suggested to permanently allow all urls/origins from a given message which allows them for all messages. If you go for the allow-all-sites option, please make it complementary, and do NOT make it the default:

"Show remote content in this message" (default, see bug 1062961 about the currently wrong button design)
"Allow remote content in this message"

I also wonder if the latter is clear enough to describe what it does, a blanket permission for all sites referenced in the message. Perhaps something like this?
"Allow remote content for all sites in this message" or
"Allow remote content for all contained sites"

2) email-addressed based remote content exceptions
(+) are much easier to understand for a user ux-wise. For many everyday cases, they work correctly.
(+) avoid allowing all tracking sites for all messages. I might be fine with [e-mail] (my pharmacy) knowing that and when I read their message, but that doesn't mean the same for [e-mail] (e.g. a business contact, future employer etc.). Imo there's a significant difference in privacy between allowing all sites for *all* senders, and allowing all sites only for just *one* sender (in the case of email address based exceptions).
(-) On the downside, whitelisting senders opens the door for spoofing email addresses. However, if you fall for the first spoof and whitelist all malicious sites from there, the damage is also quite extensive, much bigger than allowing just a single email address which might be spoofed at some point.

I'm not claiming to be deep in the details about this.

I don't agree with Jorg's assumption that m-c wouldn't let us tweak something if we talk to them and explain why we want/need that, and I see no indications thereof.

Thanks Jorg for working on this. We need to find a balance between fixing this asap and getting things right in the long term without rushing to conclusions. Jorg will ensure the asap ;)

Comment 46

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #43)
> (In reply to Magnus Melin from comment #41)
> > My plan is to investigate what m-c changes it takes to get it working again.
> [...]
> Maybe we should store email+URL pairs for maximum security? :)

That might be an interesting option to explore. Could somebody describe the UX of that?

Comment 47

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. from comment #45)
> I don't agree with Jorg's assumption that m-c wouldn't let us tweak
> something if we talk to them and explain why we want/need that, and I see no
> indications thereof.
Read comment #5 and comment #37 and
https://tools.ietf.org/html/rfc6454
https://en.wikipedia.org/wiki/Same-origin_policy
https://code.google.com/p/html5security/wiki/CrossOriginRequestSecurity
Origin is about defining were content originates, an e-mail address is not an origin.
M-C people have *already* analysed what TB needs and decided to drop the support nonetheless (and rightly so!).

> Thanks Jorg for working on this.
I am not working on this any more. Apparently my suggestion to drop what doesn't work is meeting opposition, and I really don't have the inclination to conduct a discussion as in bug 368915.

Obviously this needs to be fixed *NOW*, meaning before 21st Sept. 2015 (https://wiki.mozilla.org/RapidRelease/Calendar#Future_branch_dates) so the fix will land on TB 43 Aurora. As simple fix (option 1a, comment #33) will take a few hours to implement, (fruitless) M-C investigation and discussion and "exploring options" can take months. Note that TB 45 enters Aurora on 14th Dec. 2015.

TB 42 Earlybird and TB 43 Daily are already unusable due to this problem.

Comment 48

[Magnus Melin [:mkmelin]]

Guys, this is bug is about making it work as before. Nothing more, nothing less. After it's done, you can discuss that in followups.

Comment 49

[Thomas D. (:thomas8)]

Another question arising from this is how we can ensure that we get notified in the future when M-C unilaterally drops stuff which we depend on so it breaks us?

Comment 50

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. from comment #49)
> we can ensure that we get notified in the future when M-C unilaterally drops
> stuff which we depend on so it breaks us?

This happens *all* the time causing bustages, build failures and bugs like this one. Normally the TB team doesn't get notified. See also:
https://mail.mozilla.org/pipermail/tb-planning/2015-January/003606.html
https://mail.mozilla.org/pipermail/tb-planning/2015-January/003610.html

Generally it's a good idea to watch the related mailing lists, like dev-platform and firefox-dev.

Comment 51

[:aceman]

We should have tests and those will detect breakage like this (that does not cause full crash or compile errors). It seems to me we have about 2 tests in the tree that are seem to now be failing due to this problem. So those did notify us about feature removal.

Comment 52

[Magnus Melin [:mkmelin]]

Attachment: bug1193200_remote_content_bustage-tb.patch

Sync the thunderbird part of the permissions ui to the firefox version. 
Only a few lines are not copy-paste. (The for handling mailto scheme.)

With this and the m-c patch I'll attach. things work like before.

Comment 53

[Magnus Melin [:mkmelin]]

Attachment: bug1193200_remote_content_bustage-mc.patch

Make mailto: URIs have an origin (mailto:[e-mail])

Of course mailto: URIs don't have an origin when thinking http, but I think that's beside the point - this is an internal implementation detail that only ever affects someone trying to use a mailto uri in the first place. It's similar to that chrome:// uris have an origin too...

This makes the permission manager usable for storing per-sender permissions in thunderbird again. (Broke by bug 1165263.)

Comment 54

[Jorg K (CEST = GMT+2)]

Magnus, looks like I was wrong and I owe you a beer.

Can you add https at the same time as adding http, please. I noticed that when adding a site to the permissions manager, the e-mail still doesn't display correctly since the images come from a https URL.

The migration the permissions manager does from moz_hosts to moz_perms also doubles up the amount of records (and currently drops the mailto: URIs) adding http and https at the same time.

With the M-C changes in place we should look at the migration process again and possibly not drop the maito URIs.

Comment 55

[Nika Layzell [:nika] (ni? for response)]

(In reply to Magnus Melin from comment #53)
> Created attachment 8659475 [details] [diff] [review]
> bug1193200_remote_content_bustage-mc.patch
> 
> Make mailto: URIs have an origin (mailto:[e-mail])
> 
> Of course mailto: URIs don't have an origin when thinking http, but I think
> that's beside the point - this is an internal implementation detail that
> only ever affects someone trying to use a mailto uri in the first place.
> It's similar to that chrome:// uris have an origin too...
> 
> This makes the permission manager usable for storing per-sender permissions
> in thunderbird again. (Broke by bug 1165263.)

It doesn't look like you fixed the definition of subsumes to consider two mailto URIs with the same origin as the same. Does `principalA.equals(principalB) iff principalA.origin == principalB.origin` hold with your patch? If it doesn't, then that's pretty bad (it's an invariant assumed by the principal and other code in the permission manager).

You'll also need to modify the migration process, as it will drop mailto URIs on the ground right now IIRC.

I'm not the person who's opinion matters on this (that may be bholley or someone else), but I'm a tad uncomfortable making mailto: URIs have origins. Theoretically it shouldn't ever be visible to M-C, but still, it seems wrong.

Comment 56

[Magnus Melin [:mkmelin]]

Jorg: we probably shouldn't add http if it was https in the first place... I doubt it's really common to have http in one message and then https in another. But yes, the ui code to add "hosts" may need some further tweaks to get the protocol right. 

Michael: thx, I'll have to add a check for mailto in UpgradeHostToOriginAndInsert. 
Will verify the sumsumes thing, but that should check out. ([e-mail] would always have origin mailto:[e-mail] and [e-mail] would always have origin mailto:[e-mail])

Comment 57

[Jorg K (CEST = GMT+2)]

Magnus, somehow the hosts get passed in here ...
function onRemoteContentOptionsShowing(aEvent) {
   let hosts = aEvent.target.value ? aEvent.target.value.split(" ") : [];
... without the protocol. So we prepend "http://". During my testing I noticed after adding the host, some messages still don't display correctly, I assume that this is due to the fact that the images would get loaded via https, which is becoming more commonplace these days. This is of course very puzzling behaviour for the user: The user selects the option "Allow remote content from xx.com", the message reloads, and the option "Allow remote content from xx.com" is still offered, although the user has already selected it. Can we fix this please, since this is part of the regression.

Comment 58

[Magnus Melin [:mkmelin]]

Attachment: bug1193200_remote_content_bustage-tb.patch

Also update the permission adder menu to use origins instead of hosts. Fixes the inbability to add permissions for https issue that Jorg mentioned.

Comment 59

[Magnus Melin [:mkmelin]]

Attachment: bug1193200_remote_content_bustage-mc.patch

Updated to add migration. See comment 53 for the details.

Comment 60

[Bobby Holley (:bholley)]

Comment on attachment 8660917 [details] [diff] [review]
bug1193200_remote_content_bustage-mc.patch

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

I'm really sorry, but I'm not willing to do this. mailto: URIs don't really fit into the origin abstraction, and I'm not willing to break that abstraction and risk regressions in the already regression-prone permission manager for this use-case.

Thunderbird should store trusted senders in regular browser storage, and not interact with the permission manager.

Thank you all for your hard work on Thunderbird. Sorry for making your lives harder. :-(

Comment 61

[Jorg K (CEST = GMT+2)]

Magnus, sorry to hear this, but no beer for you now ;-(

Please just drop the function. Allowing all remote content that comes in through an approved e-mail address is not secure!

Comment 62

[:aceman]

So can we somehow fake the "origin" and make the permissions manager (without changes) store our mailto string? Or is it determining something automatically and that will fail on mailto strings?

Comment 63

[Bobby Holley (:bholley)]

Can you explain why you want to use the permission manager here?

Comment 64

[Magnus Melin [:mkmelin]]

(In reply to Bobby Holley (:bholley) from comment #63)
> Can you explain why you want to use the permission manager here?

Sure. We want to use the permission manager because we use the same (well, almost copied) ui as firefox for allowing users to whitelist/blacklist domains that can automatically load remote images in mails. If you have Thunderbird installed, see Options | Pricacy | Mail content. We also allow whitelisting/blacklisting remote content for a specific sender. That data has also been stored in the permission manager. 

Having the same UI for what's end-user wise essentially the same thing makes sense.
If we can't use the permission manager that would mean we'd have to hack around that in the UI, which is always error prone as code evolves. 

In any case, it's way more code than the tiny 5-line change to nsPrincipal.cpp

Since these are uris we form ourselves it should be possible to use a faked known origin, like chrome://messenger/content/?email=[e-mail], but I'd still much prefer it if you could reconsider allowing mailto:'s, as that's just semantically correct.

Comment 65

[Bobby Holley (:bholley)]

(In reply to Magnus Melin from comment #64)
> In any case, it's way more code than the tiny 5-line change to
> nsPrincipal.cpp

Understood, but the impact of that change in nsPrincipal.cpp (and the associated weakening of abstractions) has outsize importance from my perspective.

> Since these are uris we form ourselves it should be possible to use a faked
> known origin, like chrome://messenger/content/?email=[e-mail]

Everything loaded from chrome:// has the system origin.

Comment 66

[Magnus Melin [:mkmelin]]

(In reply to Bobby Holley (:bholley) from comment #65)
> Everything loaded from chrome:// has the system origin.

Yes, but that origin can be stored in the permission manager 
Just to be clear, we don't really load if from any particular url in the end. We just form the uri, check with the permission manager and based on what it said we either block or not. http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgContentPolicy.cpp#119

Comment 67

[Bobby Holley (:bholley)]

(In reply to Magnus Melin from comment #66)
> (In reply to Bobby Holley (:bholley) from comment #65)
> > Everything loaded from chrome:// has the system origin.
> 
> Yes, but that origin can be stored in the permission manager

Which - the System Principal, or a codebase principal for chrome:// foo? The latter is totally unsupported - I'm not sure what would happen if you tried to make one.

> Just to be clear, we don't really load if from any particular url in the
> end. We just form the uri, check with the permission manager and based on
> what it said we either block or not.

But the whole point is that the permission manager is using principals and origins now - you can't just give it a URI and expect it to be treated as a URI.

Comment 68

[(no longer active)]

Magnus, the reason why we changed the permission manager in Firefox was that the previous behavior was creating security risks for our users.  The requirements of storing permissions for websites are different than storing an ACL for users, which I think is what Thunderbird needs.  I suggest you take a look at the contentprefs service: <http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIContentPrefService2.idl>.  Through that service you can manage your own hierarchy of choice, and I believe that is a better tool for your use case.

I'm really sorry, but please pay attention to the fact that the requirements of permission manager are different.

Comment 69

[Magnus Melin [:mkmelin]]

(In reply to Bobby Holley (:bholley) from comment #67)
> Which - the System Principal, or a codebase principal for chrome:// foo? The
> latter is totally unsupported - I'm not sure what would happen if you tried
> to make one.

Well what's stored internally is the origin serialization. For chrome:// that's the full uri.
I actually played with this yesterday and it works. I'd just have to clean up the patch.

> But the whole point is that the permission manager is using principals and
> origins now - you can't just give it a URI and expect it to be treated as a URI

I'm not, I'm creating a chrome URI and letting the permission manager figure out how to store permissions for that. Do you see a problem with that?

Comment 70

[(no longer active)]

(In reply to Magnus Melin from comment #69)
> > But the whole point is that the permission manager is using principals and
> > origins now - you can't just give it a URI and expect it to be treated as a URI
> 
> I'm not, I'm creating a chrome URI and letting the permission manager figure
> out how to store permissions for that. Do you see a problem with that?

If we ever decide to disallow chrome:// URIs in the permission manager (since it conceptually makes no sense to give privileges to chrome code) then your code will break again.

Comment 71

[Magnus Melin [:mkmelin]]

Well as I see it, unless there's concrete plans to do so we might as well just cross that bridge if we get to it.

Comment 72

[Magnus Melin [:mkmelin]]

Attachment: bug1193200_remote_content_bustage-tb.patch

This seems to work for me, and the tests are happy.
Using chrome://messenger/content/?email=[e-mail] to store permissions

Fairly many changes, but it's just syncing up with ff. To see what mods i had to make, apply and run diff -u8 mail/components/preferences/permissions.js  mozilla/browser/components/preferences/permissions.js

Comment 73

[Bobby Holley (:bholley)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #70)
> > I'm not, I'm creating a chrome URI and letting the permission manager figure
> > out how to store permissions for that. Do you see a problem with that?
> 
> If we ever decide to disallow chrome:// URIs in the permission manager
> (since it conceptually makes no sense to give privileges to chrome code)
> then your code will break again.

More the the point: The origin serialization used by the permission comes from accessing .origin on an nsIPrincipal. Generally anything coming from a chrome:// URI gets System Principal, so there's no URI in that origin to speak of.

it looks to me like we may end up creating codebase URIs for stuff outside of chrome://content, so you might be in luck. But these are treacherous waters, and I can't promise to keep TB's usage here in mind for any further simplifications of the security model.

Comment 74

[(no longer active)]

(In reply to Magnus Melin from comment #71)
> Well as I see it, unless there's concrete plans to do so we might as well
> just cross that bridge if we get to it.

I don't understand your reasoning.  But at any rate, comment 68 is an alternative solution.  It's up to you whether you want to use that, or keep relying on things that may break in the future.

Comment 75

[Magnus Melin [:mkmelin]]

My reasoning is that it would an even more fragile hack to try to hook that up in the same ui, and if breakage actually happens it's not like coding that up now would somehow be less work.

Thx for th pointer about nsIContentPrefService2.idl though!

Comment 76

[Mitra Ardron]

When the security implications of any particular solution are considered, we should bear in mind that in practice the only way to use TB effectively is to set "Allow remote content in messages" because the current process of per-website settings just makes reading any kind of richer email too painful.

Comment 77

[:aceman]

I don't think so. I do think whitelisting per website is better security. You whitelist your known/expected hosts (e.g. paypal or your bank) and nothing else. Not the sender, that can easily be spoofed. If you have whitelisted a sender from your bank, but the spoofed message suddenly contains unknown new hosts. If going by sender, we would load all the malicious sites. That is not good.

If by richer email you mean ton of links/images to various hosts, I'd consider such message malicious. In my experience serious services do limit hosts to one of their own (or at least domain).

Comment 78

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #77)
> I don't think so. I do think whitelisting per website is better security.
> You whitelist your known/expected hosts (e.g. paypal or your bank) and
> nothing else. Not the sender, that can easily be spoofed. If you have
> whitelisted a sender from your bank, but the spoofed message suddenly
> contains unknown new hosts. If going by sender, we would load all the
> malicious sites. That is not good.

Therefore I repeat for the 53rd time. I'd drop the feature of whitelisting by e-mail like a hot potato.

> If by richer email you mean ton of links/images to various hosts, I'd
> consider such message malicious. In my experience serious services do limit
> hosts to one of their own (or at least domain).

Sadly not quite. Most of the "serious" services I use draw pictures from two to three domains. Therefore Aceman's option "Allow remote content in this message" is still the best option. It rocks! Put that first on the menu and users will use it instead of "Allow content from sender". That will get them the same effect, but with 1000% security added. And, as Richard pointed out (comment #34), next time the newsletter comes from a different sender, the content still displays, since the sites are whitelisted, not the sender. Now that we've got the box open is the time to implement this!

Therefore I still think option 1a (comment #33) is best.

Frankly, I'm already getting tired of the discussion. Just implement something that works and lets me use Earlybird 43 from next week.

Comment 79

[Jorg K (CEST = GMT+2)]

Correction:
Most of the "serious" services I use draw pictures from two to three domains/subdomains,
for exaple: images-eu.ssl-images-amazon.com, images-eu.amazon.com and www.amazon.de.
They are a pain to allow individually, therefore I lazily use the insecure "by e-mail" option.

Comment 80

[:aceman]

Comment on attachment 8663026 [details] [diff] [review]
bug1193200_remote_content_bustage-tb.patch

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

OK, so this only does changes in c-c and fakes the email to a string that is accepted by the permission manager. We can try it. With not much time to the next ESR (45) this seems to be the quickest solution. Hopefully the permission manager does not break it till then.

I haven't run with this yet, but have read the code.

::: mail/base/content/mailWindowOverlay.js
@@ +3034,5 @@
>      let popup = document.getElementById("remoteContentOptions");
> +    let principal = Services.scriptSecurityManager
> +      .createCodebasePrincipal(aContentURI, {});
> +    let origins = popup.value ? popup.value.split(" ") : [];
> +    if (origins.indexOf(principal.origin) == -1)

!origins.includes()?

@@ -3187,5 @@
>    // ... and in with the new.
> -  for (let host of hosts) {
> -    let uri = Services.io.newURI(
> -      host.includes("@") ? "mailto:" + host : "http://" + host, null, null);
> -

If this code is removed, where in the new code we detect whether we are handling an email or a host? Only the email needs the fake "chrome://messenger/content/?email=" prepended. Or not?

@@ +3193,5 @@
>      let menuitem = document.createElement("menuitem");
>      menuitem.setAttribute("label",
> +      messengerBundle.getFormattedString("remoteAllow",
> +        [origin.replace("chrome://messenger/content/?email=", "")]));
> +    menuitem.setAttribute("value", origin);

What happens if a malicious email contains <img src="chrome://messenger/content/?email=X@Y"> ? How will it be displayed in the menu and can/want we somehow prevent it?

::: mail/components/preferences/permissions.js
@@ +51,5 @@
>      getCellValue: function(aRow, aColumn) {},
> +    cycleHeader: function(column) {},
> +    getRowProperties: function(row){ return ""; },
> +    getColumnProperties: function(column){ return ""; },
> +    getCellProperties: function(row,column){

Why do you remove the 'a' argument prefixes? The functions above still have them. Also missing space after "row,".

@@ +70,5 @@
>        stringKey = "cannot";
>        break;
> +    case nsICookiePermission.ACCESS_ALLOW_FIRST_PARTY_ONLY:
> +      stringKey = "canAccessFirstParty";
> +      break;

Do we actually allow to set these cookie perms in this dialog? Are we using this same code for 2 dialogs ("accept cookies" and "allow remote content")?  Have you added the XUL button for this value (http://mxr.mozilla.org/comm-central/source/mail/components/preferences/permissions.xul#44)?


Do we disallow inputting emails into the cookie exceptions dialog?

@@ +81,5 @@
>  
>    addPermission: function (aCapability)
>    {
>      var textbox = document.getElementById("url");
> +    var input_url = textbox.value.replace(/^\s*/, ""); // trim any leading space

What about trailing spaces?

@@ +172,5 @@
> +    // longer be in order.
> +    if (this._lastPermissionSortColumn == "statusCol") {
> +      this._resortPermissions();
> +    }
> +    this._tree.treeBoxObject.invalidate();

Do we need to do these internal changes manually? Doesn't the widget do it automatically? Or are we using a dumb widget that only shows rows and we must order them?

@@ +435,5 @@
>    {
> +    for (let i = 0; i < this._permissions.length; ++i) {
> +      // Thunderbird compares origins, not principals here.
> +      if (this._permissions[i].principal.origin == aPrincipal.origin) {
> +        this._permissions.splice(i, 1);

So we delete the first found permission for aPrincipal (which seems to be one URL). Is there always only one perm existing for one principal? It appears to me like we could e.g. set one permission for the access and another for the cookies handling for the same principal. Then how do we address removing only one of them in this function?

::: mail/locales/en-US/chrome/messenger/preferences/permissions.dtd
@@ +10,5 @@
>  <!ENTITY removepermission.label       "Remove Site">
>  <!ENTITY removepermission.accesskey   "R">
>  <!ENTITY removeallpermissions.label   "Remove All Sites">
>  <!ENTITY removeallpermissions.accesskey "e">
>  <!ENTITY address.label                "Address of website:">

This shows "Address of website" above the input field, but you also allow to input email. Can it be improved?

@@ +23,5 @@
>  
> +<!ENTITY button.cancel.label          "Cancel">
> +<!ENTITY button.cancel.accesskey      "C">
> +<!ENTITY button.ok.label              "Save Changes">
> +<!ENTITY button.ok.accesskey          "S">

"S" is already used above, is it a problem?

::: mailnews/base/src/nsMsgContentPolicy.cpp
@@ +109,5 @@
>  
>    nsCOMPtr<nsIIOService> ios = do_GetService("@mozilla.org/network/io-service;1", &rv);
>    NS_ENSURE_SUCCESS(rv, false);
>    nsCOMPtr<nsIURI> mailURI;
> +  emailAddress.Insert("chrome://messenger/content/?email=", 0);

I'd wish there would be a constant defined somewhere for this magic string.

::: mailnews/base/util/mailnewsMigrator.js
@@ +131,5 @@
>    while (enumerator.hasMoreElements())
>    {
>      let migrateAddress = function(aEmail) {
> +      let uri = Services.io.newURI(
> +        "chrome://messenger/content/?email=" + aEmail, null, null);

Will you add a migrator from the stored perms and senders in the old mailto: format? Or are those already lost (not retrievable from DB) once the new TB starts (with mailto: support them removed)?

Comment 81

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #80)
> If this code is removed, where in the new code we detect whether we are
> handling an email or a host? Only the email needs the fake
> "chrome://messenger/content/?email=" prepended. Or not?

The chrome uri is passed in as aContentURI. So we don't have to do anything here.

> What happens if a malicious email contains <img
> src="chrome://messenger/content/?email=X@Y"> ? How will it be displayed in
> the menu and can/want we somehow prevent it?

Content can't load chrome uris, but if it could, that would just be a not found image.


> Why do you remove the 'a' argument prefixes? The functions above still have
> them. Also missing space after "row,".

I'm just copy-pasting firefox's code. I'd rather not fix style issues as it just makes it harder to port code as we go.

(more comments later)

Comment 82

[:aceman]

(In reply to Magnus Melin from comment #81)
> > What happens if a malicious email contains <img
> > src="chrome://messenger/content/?email=X@Y"> ? How will it be displayed in
> > the menu and can/want we somehow prevent it?
> 
> Content can't load chrome uris, but if it could, that would just be a not
> found image.
Yes, it will look broken in the message display. I mean what will be shown in the list of hosts/urls to allow (the menuitems below "allow from <email>"). If the user can be tricked into allowing the X@Y sender when just allowing all the presented 'hosts'.

> 
> 
> > Why do you remove the 'a' argument prefixes? The functions above still have
> > them. Also missing space after "row,".
> 
> I'm just copy-pasting firefox's code. I'd rather not fix style issues as it
> just makes it harder to port code as we go.
> 
> (more comments later)

Comment 83

[:aceman]

(In reply to Magnus Melin from comment #81)
> > Why do you remove the 'a' argument prefixes? The functions above still have
> > them. Also missing space after "row,".
> 
> I'm just copy-pasting firefox's code. I'd rather not fix style issues as it
> just makes it harder to port code as we go.

The 4 lines I asked about had only the removal of 'a' as the sole change. Why does that 'bad style' need to be copied?

Comment 84

[Jorg K (CEST = GMT+2)]

Magnus, sadly I didn't understand what you said during today's meeting.
Why is "Allow remote content from sender" better then "All all remote content in this message"?

I can only see that it's worse:
- sender can be spoofed.
- sender can change to a colleague sending the same newsletter, or see below, anonymous e-mail
  via Amazon, eBay, etc.

I have beauties like: [e-mail] (no joke!) in my list, which I added just to load a few pictures from Amazon. When I correspond with the next Amazon Marketplace seller, I will have to add another one of those.

These "anonymised" e-mail addresses are commonplace these days, and the only way to allow the content in the message in one hit is to allow the strange e-mail address.

Regardless of what you implement, we need a "Allow all content in this e-mail", meaning, add all the referenced sites to the exceptions.

Comment 85

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #83)
> The 4 lines I asked about had only the removal of 'a' as the sole change.
> Why does that 'bad style' need to be copied?

What I meant was that IMO it's not time well spent doing anything about it. What I did was copy over the firefox files and apply the changes we needed. As we're not likely to do any "real work" on this dialog all such changes only make it harder to see what the real differences actually were - for all future syncs.

> Why is "Allow remote content from sender" better then "All all remote content in this message"?

Better or not is up to the user. Blindly whitelisting "all remote content in this message" would pretty soon have many of the major trackers whitelisted. The use may care or not.

Comment 86

[:aceman]

(In reply to Magnus Melin from comment #85)
> > Why is "Allow remote content from sender" better then "All all remote content in this message"?
> 
> Better or not is up to the user. Blindly whitelisting "all remote content in
> this message" would pretty soon have many of the major trackers whitelisted.
> The use may care or not.

Yes. So can you please add the option and let the user decide?

Comment 87

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #85)
> > Why is "Allow remote content from sender" better then "All all remote content in this message"?
> 
> Better or not is up to the user. Blindly whitelisting "all remote content in
> this message" would pretty soon have many of the major trackers whitelisted.
> The use may care or not.

Thanks Magnus, now I get it, sorry the sound when you spoke at the meeting was just terrible.

Yes, if you use the new option without caution, trackers originating from one e-mail could be are enabled and then track the next e-mail from another sender.

My use case for the option was stated in comment #84: Mailing lists and all sources of obfuscated e-mail addressed (Amazon, eBay, etc).

Comment 88

[alta88]

(In reply to Magnus Melin from comment #85)
> (In reply to :aceman from comment #83)
> > The 4 lines I asked about had only the removal of 'a' as the sole change.
> > Why does that 'bad style' need to be copied?
> 
> What I meant was that IMO it's not time well spent doing anything about it.
> What I did was copy over the firefox files and apply the changes we needed.
> As we're not likely to do any "real work" on this dialog all such changes
> only make it harder to see what the real differences actually were - for all
> future syncs.
> 
> > Why is "Allow remote content from sender" better then "All all remote content in this message"?
> 
> Better or not is up to the user. Blindly whitelisting "all remote content in
> this message" would pretty soon have many of the major trackers whitelisted.
> The use may care or not.

magnus' approach is exactly correct.  in fact, these sort of ports of 'browsing' bits would be automated in a perfect world, and mail specific patches applied on top.  style or other nits in libraries or ported upstream code should not be touched.

(In reply to :aceman from comment #86)
> (In reply to Magnus Melin from comment #85)
> > > Why is "Allow remote content from sender" better then "All all remote content in this message"?
> > 
> > Better or not is up to the user. Blindly whitelisting "all remote content in
> > this message" would pretty soon have many of the major trackers whitelisted.
> > The use may care or not.
> 
> Yes. So can you please add the option and let the user decide?

sorry, but that is a new feature and not related to the mail address permission regression, and whether it is added or not should not at all block this fix.

Comment 89

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #80)
> >        stringKey = "cannot";
> >        break;
> > +    case nsICookiePermission.ACCESS_ALLOW_FIRST_PARTY_ONLY:
> > +      stringKey = "canAccessFirstParty";
> > +      break;
> 
> Do we actually allow to set these cookie perms in this dialog? Are we using

No, but apparently needed not to break if someone set that at the time it was possible.

> this same code for 2 dialogs ("accept cookies" and "allow remote content")? 
> Have you added the XUL button for this value
> (http://mxr.mozilla.org/comm-central/source/mail/components/preferences/
> permissions.xul#44)?

Already exits.

> Do we disallow inputting emails into the cookie exceptions dialog?

Good catch, fixed.

> @@ +81,5 @@
> >  
> >    addPermission: function (aCapability)
> >    {
> >      var textbox = document.getElementById("url");
> > +    var input_url = textbox.value.replace(/^\s*/, ""); // trim any leading space
> 
> What about trailing spaces?

Now using trim() instead.
 
> Do we need to do these internal changes manually? Doesn't the widget do it
> automatically? Or are we using a dumb widget that only shows rows and we
> must order them?

Didn't investigate, I assume there's some reason :)

> So we delete the first found permission for aPrincipal (which seems to be
> one URL). Is there always only one perm existing for one principal? It

No permissions have type. And we only add/remove for the "image" type.

> This shows "Address of website" above the input field, but you also allow to
> input email. Can it be improved?

As it's shared with cookies, I'd just let it be.

> @@ +23,5 @@
> >  
> > +<!ENTITY button.cancel.label          "Cancel">
> > +<!ENTITY button.cancel.accesskey      "C">
> > +<!ENTITY button.ok.label              "Save Changes">
> > +<!ENTITY button.ok.accesskey          "S">
> 
> "S" is already used above, is it a problem?

Yes for cookies, apparently a bug in firefox too. Changed.

> Will you add a migrator from the stored perms and senders in the old mailto:
> format? Or are those already lost (not retrievable from DB) once the new TB
> starts (with mailto: support them removed)?

I don't plan to do further migration. The rejected m-c patch had migration code, but it's not really feasible to do it elsewhere.

I don't want to expand the scope of this bug for an add-all option. That should be done in a separate bug, especially as it's not completely clear how that should behave.

Comment 90

[Magnus Melin [:mkmelin]]

Attachment: bug1193200_remote_content_bustage-tb.patch

Updated for review comments

Comment 91

[:aceman]

Comment on attachment 8665075 [details] [diff] [review]
bug1193200_remote_content_bustage-tb.patch

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

I don't like that the magic string "chrome://messenger/content/?email=" is hardcoded all around the place, but you probably can't put it in some nice location and make a constant. Maybe at least in the permissions.js file.

I have now tested it on a real message with remote content and the management does work. I stored allow for an url, then it no longer appeared in the menu in the message. It appeared in the permissions manager. Also deleting it in manager worked fine. Adding other emails/hosts to manager worked fine. Restarting TB keeps the stored hosts.

There is some small bitrot in mailWindowOverlay.js.

I've pushed it to try to see if it fixes the tests: https://hg.mozilla.org/try-comm-central/rev/489cf34f731d

You haven't touched mozmill/content-policy/test-general-content-policy.js, there is code like this:

  let authorEmailAddress = addresses.value[0];
  let uri = Services.io.newURI("mailto:" + authorEmailAddress, null, null);
  Services.perms.add(uri, "image", Services.perms.ALLOW_ACTION);

Looks like that may need a treatment too.

::: mail/locales/en-US/chrome/messenger/preferences/permissions.dtd
@@ +23,5 @@
>  
> +<!ENTITY button.cancel.label          "Cancel">
> +<!ENTITY button.cancel.accesskey      "C">
> +<!ENTITY button.ok.label              "Save Changes">
> +<!ENTITY button.ok.accesskey          "S">

Somehow the cookies dialog does not have the Save Changes button, but is does have S for Search. In TB there is also no "Allow for Session" button, so maybe currently they would not clash. Only once we expose them.

Comment 92

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #91)
> Somehow the cookies dialog does not have the Save Changes button, 

It does for me, you're looking at the cookie exception dialog, not the other one, right?

Comment 93

[Magnus Melin [:mkmelin]]

Attachment: bug1193200_remote_content_bustage-tb.patch

Made it a const in permission.js and fixed the forgotten test

Comment 94

[Magnus Melin [:mkmelin]]

Attachment: bug1193200_remote_content_bustage-tb.patch

And unbitrot

Comment 95

[:aceman]

Comment on attachment 8666387 [details] [diff] [review]
bug1193200_remote_content_bustage-tb.patch

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

Yes, sorry, I was looking at the Show cookies dialog. Now I can see the Allow for Session button and the changed accesskey.

I'm not sure I can do reviews, but as this does not add new features, just solves a bustage and updates dialogs to state in Firefox, maybe I'm enough :)

The mozmill test still fails for me locally on the line:
Services.perms.remove(authorEmailAddress, "image");

SUMMARY-UNEXPECTED-FAIL | test-general-content-policy.js | test-general-content-policy.js::test_generalContentPolicy
  EXCEPTION: Could not convert JavaScript argument arg 0 [nsIPermissionManager.remove]
    at: nonesuch line 293
       checkAllowForSenderWithPerms test-general-content-policy.js:293 3

r=me with that fixed :)

Comment 96

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #95)
> I'm not sure I can do reviews, but as this does not add new features, just
> solves a bustage and updates dialogs to state in Firefox, maybe I'm enough :)

Kent said in the last Tuesday meeting that he wants you to do reviews and not just give f+.

Comment 97

[Kent James (:rkent)]

(In reply to Jorg K (GMT+2) from comment #96)
> (In reply to :aceman from comment #95)
> > I'm not sure I can do reviews, but as this does not add new features, just
> > solves a bustage and updates dialogs to state in Firefox, maybe I'm enough :)
> 
> Kent said in the last Tuesday meeting that he wants you to do reviews and
> not just give f+.

The rule that we are operating under, AFAIK, is that a module peer (or owner) can decide who is qualified to give a review. So if Magnus asks for a review from aceman, then aceman is deemed qualified.

Comment 98

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/6abae4c3529b -> FIXED

Unfortunately there seems to be another problem in test-general-content-policy.js so it's not yet a complete success.
At least locally forwarding a mail in the mozmill test crashes, but outside mozmill I can't reproduce. (checkComposeWindow for forward). I don't know if it's related to this bug or not.

Comment 99

[aleth [:aleth]]

(In reply to Magnus Melin from comment #89)
> > Will you add a migrator from the stored perms and senders in the old mailto:
> > format? Or are those already lost (not retrievable from DB) once the new TB
> > starts (with mailto: support them removed)?
> 
> I don't plan to do further migration. The rejected m-c patch had migration
> code, but it's not really feasible to do it elsewhere.

Does this have release note implications?

Comment 100

[:aceman]

That was also my question in comment 80. I think I got no clear answer, but if there is no migrator, then old entries (not having the right magic format) will basically be lost (invisible), maybe even automatically removed by the permissions manager core.

If that is the case, it should be mentioned in release notes. Users will need to "train" (store) the senders again. Unless it is possible to create a migrator in a new bug.

Comment 101

[Kent James (:rkent)]

Should this be landed in comm-beta for the next beta (42)? Per the status it looks to me like it should.

Comment 102

[Jorg K (CEST = GMT+2)]

That would be supercalifragilisticexpialidocious.
To me, TB 42 and TB 43 are unusable due to this bug ... and I'd like to move on from the TB 41 beta I'm using now.

Comment 103

[Kent James (:rkent)]

These patches have strings. Not sure how that interacts these days with beta and aurora. Fallen, OK to push these with string changes to comm-beta and comm-aurora?

Comment 104

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushing string changes to aurora and beta is generally frowned upon, because this creates extra work for localizers, especially on beta where some locales don't ususally push to because they only use the web tools that sync with aurora.

It is still possible to do this, but then you'll need set the late-l10n keyword, make a posting on m.d.l10n, and keep an eye on the dashboard to see which localizers may have not taken care and write them a private email to remind them.

If the strings can be avoided that would be great, maybe it is possible to create a patch that fixes the bug but uses existing strings?

Comment 105

[Kent James (:rkent)]

New strings are "Cancel" and "Save Changes". Surely those are available elsewhere.

Comment 106

[Kent James (:rkent)]

Comment on attachment 8666387 [details] [diff] [review]
bug1193200_remote_content_bustage-tb.patch

https://hg.mozilla.org/releases/comm-aurora/rev/64ba3a65e399 (no strings variant)

Comment 107

[Kent James (:rkent)]

Comment on attachment 8666387 [details] [diff] [review]
bug1193200_remote_content_bustage-tb.patch

pushed https://hg.mozilla.org/releases/comm-beta/rev/ca6e9c82d923 (no new strings variant)

Comment 108

[Kent James (:rkent)]

The pushes to aurora and beta inadvertently modified strings. That has been corrected, so that the net change does not change strings, here:

https://hg.mozilla.org/releases/comm-aurora/rev/3439f2f0debd
https://hg.mozilla.org/releases/comm-beta/rev/27787d7bf72a

Comment 109

[aleth [:aleth]]

Despite the uplift, there's still a comm-beta mozmill failure that seems related to this:

TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-policy\test-general-content-policy.js | test-general-content-policy.js::test_generalContentPolicy

04:13:30     INFO -  SUMMARY-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-policy\test-general-content-policy.js | test-general-content-policy.js::test_generalContentPolicy
04:13:30     INFO -    EXCEPTION: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIPermissionManager.remove]

Comment 110

[Magnus Melin [:mkmelin]]

Evidently a beta porting mixup. 
Pushed https://hg.mozilla.org/releases/comm-beta/rev/137d56c94bd3 - should be fixed now.