953426 - Expose remote content per-host privileges in notification bar's [Option] button, and implement Options > Privacy tab

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Magnus Melin [:mkmelin]]

Attachment: bugXXX_remote_conent_privs.patch

We should allow per host remote images. I know I have many list-type mails I don't really care if the sender knows I read the message or not. Google thinks so too and are now showing remote images by default - http://gmailblog.blogspot.fi/2013/12/images-now-showing.html

The attached patch lets you allow remote content from certain cites. It's more secure than allowing content for certain senders, as the email address could easily be spoofed. Here the privileges apply to where the images are actually loaded from.

The patch is functionally complete but some icons+tests still missing. I'd like input especially on if we want a Privacy tab under options (like Firefox).

This could easily be extended to cover the missing parts of bug 457296, but I didn't expose that yet so we can fix one part at a time.

Screenshots next.

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: screenshots

Some screenshots

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8351720 [details] [diff] [review]
bugXXX_remote_conent_privs.patch

(ui-review based on screenshots.)

I think, if we're going to add a Privacy tab, we can steal the icon from the Firefox privacy tab, and we seem to have enough space in the preferences dialog to add that tab.

As for the content, since we have a ton of space in the Mail Content subtab, what do you think about merging both Mail Content and Web Content, or perhaps displaying the list of exceptions right in the dialog?

Finally, would you mind filing followup bugs to make the Do Not Track options a little more like the Firefox ones?

(I'm saying ui-r- mostly so that I can take another look at it later.)

Thanks,
Blake.

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Went ahead and adopted some more of the firefox prefs - it's easier and safer to do the copy pastes....

Icons are also now fixed. I should note that firefox doesn't have a @x2 icon for privacy. I just scaled the one they have.

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: Screenshot of the new privacy options

The new privacy options.

Comment 5

[Mark Banner (:standard8)]

Just a couple of drive-by comments:

Looking at "Allow remote content in messages" this kinda disagrees with what the "Exceptions" button is giving which sites you want to allow or disable. So I would almost suggest having something like "choose which sites to receive remote content from".

I also wonder if we shouldn't have a link to learn more to explain why enabling remote content might not be desired (or include that in the text somewhere).

Comment 6

[Magnus Melin [:mkmelin]]

The idea is to expose the pref to allow all remote content, for such prefs what I've used has been the standard verbiage. I don't think it disagrees with the Exceptions button. If the pref is set, you'd add sites you want to block - if not, you'd add sites you want to allow. Exceptions to the default rule.

For many users having remote content on by default could be the desired mode - it all depends on how you look at things, there are far larger privacy problems out there than that someone might know your email address existed, and that you just read the mail.

I'm not opposed to explaining it though. Should we add a warning when the option is ticked?

Comment 7

[Vincent (caméléon)]

This would maybe allow to get completely rid of the contact properties which allow to load external image from trust senders. I remember that Mike Conley tell that this properties will certainly not be used in its new address book concept.

Comment 8

[Magnus Melin [:mkmelin]]

Yes, the same Exceptions dialog would list emails too. This is not yet exposed completely, it needs at least the content policy patch from bug 457296 for it to work. I intend to dust that off soon.

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3

Unbitrotted, now needs to be applied on top of the remote content policy patch from bug 457296.

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v4

Minor update.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8365649 [details] [diff] [review]
proposed fix, v4

(There are, like, four patches in bug 457296, one of which might have landed.  Could you re-request ui-review once it lands, or give me a try-build and I'll review that?)

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 8365649 [details] [diff] [review]
proposed fix, v4

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

Granted the bugs got a bit tied up... This bug is independent, but bug 457296 needs the UI from this one, and for ui-r it probably is best to review look at them together.

Try run: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=a616b5b5299d (I notice I'll have to fix one test)
Builds: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-a616b5b5299d/

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v5

Fix the test and bitrot

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8378957 [details] [diff] [review]
proposed fix, v5

So it is unclear which way round these should go. If this one is independent, should this be landed first? If so, the build failures need fixing.

I still think we should be explicitly detailing why we block remote content by default (it isn't going to be obvious to everyone). Otherwise people might just think that's strange, and turn it back on again.

Comment 15

[Magnus Melin [:mkmelin]]

Yes, if they don't go in together, this one should land first. The test failures are fixed in the latest patch.

The try builds for testing are still there at http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-a616b5b5299d/ but I don't know for now long they are kept - usually not very long IIRC.

Comment 16

[Magnus Melin [:mkmelin]]

Attachment: Screenshot of the new privacy options

Added an info link beneath the remote content

Comment 17

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v6

Unbitrotted and with the info link.

Comment 18

[Matt]

Just a drive by comment,  but there is going to be no issue here with Lightning adding it's icon to options?

Comment 19

[Magnus Melin [:mkmelin]]

No, it's not an issue.

bwinton/moconley: ping on the ui-r/r. If either of you want to take both ui-r + r, please go a head :)
Bug 457296 is now all ready to land, but it needs the UI from this bug (or something like it)

Comment 20

[[e-mail]]

Comment on attachment 8394094 [details] [diff] [review]
proposed fix, v6

>+  /// Grab all remote URLs in the doc. Inspired by pageInfo.js.
>+  let grabAll = function(elem) {
Surely a better way to achieve this would be for nsMsgContentPolicy to notify of remote hosts as it blocks them - an extra argument to onMsgHasRemoteContent perhaps?

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8394094 [details] [diff] [review]
proposed fix, v6

This seems to fit in with the rest of the preferences UI, so ui-r=me!

Comment 22

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v7

Letting content policy tell us the blocked locations, per Neils suggestion. It's slightly less self-contained in that we need to keep track of all blocked hosts for a msg but... I guess it's better.

Comment 23

[Magnus Melin [:mkmelin]]

^^^ this patch needs part 2 of bug 457296 applied first

Comment 24

[[e-mail]]

Comment on attachment 8406324 [details] [diff] [review]
proposed fix, v7

(Why not use a Set for your hosts?)

Comment 25

[Magnus Melin [:mkmelin]]

A Set would lose the host order. Granted maybe it's not that important, but still.

Comment 26

[Magnus Melin [:mkmelin]]

Successful try at https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=104163c0533d

Comment 27

[Magnus Melin [:mkmelin]]

mconley: ping on the review, would be nice to get this in for 31

Comment 28

[Mike Conley (:mconley) (:⚙️)]

I'll see if I can get it done tonight.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8406324 [details] [diff] [review]
proposed fix, v7

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

This all looks good - just a few minor things. Nothing serious. See below.

::: mail/base/content/mailWindowOverlay.js
@@ +3122,5 @@
> +
> +  let messengerBundle = document.getElementById("bundle_messenger");
> +
> +  // Out with the old...
> +  for (let i = aEvent.target.childNodes.length - 1; i >= 0; i--) {

Let's just alias aEvent.target.childNodes here so as to reduce the verbosity.

It's also advisable, performance-wise, to do these adds and removes while the target is outside of the document. I'd recommend removing the event target from the DOM, doing the removes, doing the adds, and then re-inserting the target into the DOM.

::: mail/base/content/mailWindowOverlay.xul
@@ +986,5 @@
> +              label="&editRemoteContentSettingsUnix.label;"
> +              accesskey="&editRemoteContentSettingsUnix.accesskey;"
> +#endif
> +              oncommand="editRemoteContentSettings();"/>
> +<!--    <menuitem id="remoteContentOptionAllowForAddress"

Let's remove this dead code.

::: mail/components/preferences/permissions.js
@@ +72,5 @@
>    addPermission: function (aCapability)
>    {
>      var textbox = document.getElementById("url");
> +    let scheme = textbox.value.contains("@") ? "mailto:" : "http://";
> +    var host = textbox.value.replace(/^\s*([-\w]*:\/*)?/, ""); // trim any leading space and scheme

let, not var.

::: mail/components/preferences/privacy.js
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +let gPrivacyPane = {
> +
> +  init: function ()

This can just be a one liner:

init: function() {},

@@ +43,5 @@
> +  },
> +
> +  /**
> +   * Enables/disables the "keep until" label and menulist in response to the
> +   * "accept cookies" checkbox being checked or unchecked.

Can you document what this returns?

@@ +94,5 @@
> +    {
> +      case 0:
> +        return "always";
> +      case 1:
> +        return "never";

1 and 2 are supposed to both be "never"?

@@ +140,5 @@
> +      "chrome://messenger/content/preferences/permissions.xul", "", params);
> +  },
> +
> +  /**
> +   * Update the Tracking preferences based on controls.

Can you please document what this returns?

@@ +142,5 @@
> +
> +  /**
> +   * Update the Tracking preferences based on controls.
> +   */
> +  setTrackingPrefs: function PPP_setTrackingPrefs()

PPP_setTrackingPrefs is not needed. Neither is PPP_getTrackingPrefs below.

@@ +160,5 @@
> +    return dntRadioGroup.selectedItem.value;
> +  },
> +
> +  /**
> +   * Obtain the tracking preference value and reflect it in the UI.

Please document what this returns.

::: mail/locales/en-US/chrome/messenger/messenger.dtd
@@ +564,5 @@
>  <!ENTITY remoteContentOptionsAllowForMsg.label "Show remote content in this message">
>  <!ENTITY remoteContentOptionsAllowForMsg.accesskey "S">
>  <!ENTITY remoteContentOptionAllowForAddress.label "Show remote content from this sender">
>  <!ENTITY remoteContentOptionAllowForAddress.accesskey "f">
> +<!ENTITY editRemoteContentSettings.label "Edit remote content options…">

Settings...options...preferences... so much overloading. :)

::: 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:">

I guess we're OK not bumping the string ID because this should _only_ apply to the en-US locale?

::: mail/locales/en-US/chrome/messenger/preferences/preferences.properties
@@ +74,5 @@
>  #### Cookies
>  cookiepermissionstitle=Exceptions - Cookies
>  cookiepermissionstext=You can specify which web sites are always or never allowed to use cookies.  Type the exact address of the site you want to manage and then click Block, Allow for Session, or Allow.
> +
> +

Let's get rid of the newlines you added in here.

::: mail/locales/en-US/chrome/messenger/preferences/privacy.dtd
@@ +13,5 @@
> +
> +<!-- Web Content -->
> +<!ENTITY captionWebContent.label "Web Content">
> +
> +<!ENTITY acceptCookies.label "Accept cookies from sites">

We're re-using the same string keys as before... can I assume this is OK for localizers because the string is moving from one file to another?

::: mail/test/mozmill/content-policy/test-general-content-policy.js
@@ +312,5 @@
> +  let src = mozmill.getMail3PaneController().window.content.document
> +                   .getElementById("testelement").src;
> +
> +  if (!src.startsWith("http"))
> +    return; // just test http in this test

You might want to "info" your skipping of these tests, if any exist.

::: mail/test/mozmill/pref-window/test-donottrack-prefs.js
@@ +24,5 @@
>  }
>  
>  /**
> + * Helper that opens the privacy pane, checks that aInitiallySelected is
> + * currently selected, selects aRadioId and closes the preferences dialog.

aInitiallySelected -> aInitiallySelectedId

::: mail/themes/osx/mail/preferences/preferences.css
@@ +49,5 @@
>    -moz-image-region: rect(0px, 288px, 32px, 256px);
>  }
>  
> +radio[pane=panePrivacy] {
> +  -moz-image-region: rect(0px 320px 32px 288px);

It freaks me out to see these without commas. I know the ones below it doesn't have commas, but I'd prefer if this did.

::: mailnews/mime/public/nsIMimeMiscStatus.idl
@@ +53,5 @@
>    // the message body in the UI. 
>    void onEndMsgDownload(in nsIMsgMailNewsUrl url);
>  
>    attribute nsISupports securityInfo;
> +  

Lots of trailing whitespace in here. Let's not add any more. :)

Comment 30

[Magnus Melin [:mkmelin]]

(In reply to Mike Conley (:mconley) from comment #29)
> > +<!--    <menuitem id="remoteContentOptionAllowForAddress"
> 
> Let's remove this dead code.

Bug 457296 removes it. I'll land them all in one push.

> > +      case 1:
> > +        return "never";
> 
> 1 and 2 are supposed to both be "never"?

Apparently yes. http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/preferences/privacy.js#438
This stuff is copy/pasted as much as possible from firefox.

> > +<!ENTITY address.label                "Address of website:">
> 
> I guess we're OK not bumping the string ID because this should _only_ apply
> to the en-US locale?

Yes, no need to bump as there is no semantic change, just a typo kind of fix.

> We're re-using the same string keys as before... can I assume this is OK for
> localizers because the string is moving from one file to another?

Yeah since it moves to another file it should be ok.

> > +  if (!src.startsWith("http"))
> > +    return; // just test http in this test
> 
> You might want to "info" your skipping of these tests, if any exist.

I don't see a standard way to do that in mozmill.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

(In reply to Magnus Melin from comment #30)
> I don't see a standard way to do that in mozmill.

Bah, Mozmill. I'd forgotten. :/ Nevermind then.

Comment 32

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v8

Addressing the review comments.
I did not do the "adds and removes while the target is outside of the document" yet. How would I do that? As I'd need to keep track of the position anyway

- replaceChild to change the target to some placeholder take node
- do the building
- replace the fake with the real thing again?

I haven't seen anything like that elsewhere, and this isn't very performance critical code, it's likely to be 2-4 nodes added/removed per message with remote content that the user actually clicks open the prefs for.

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8412070 [details] [diff] [review]
proposed fix, v8

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

Assuming tests pass, I think this looks good to go (beyond my small nit which you can ignore if you'd like). Thanks for your patience, Magnus.

::: mail/components/preferences/privacy.js
@@ +42,5 @@
> +
> +  /**
> +   * Enables/disables the "keep until" label and menulist in response to the
> +   * "accept cookies" checkbox being checked or unchecked.
> +   * @return 0 if cookies are accepted, 2 if they are not

Would be good to also include _where_ these magic numbers come from.

Comment 34

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/45cc03b40060 -> FIXED

Note to self: create the "more info" support page.

Comment 35

[Magnus Melin [:mkmelin]]

Created the info page - https://support.mozilla.org/kb/remote-content-in-messages
Someone still needs to approve (and improve) it. I didn't find how to restrict it to tb31+

Comment 36

[Magnus Melin [:mkmelin]]

Roland, could you get the https://support.mozilla.org/kb/remote-content-in-messages url approved?

Comment 37

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

did a major update, will set to L10N ready once jsavage approves

Comment 38

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

:jsavage if you could take a quick look at this article i would be most grateful. I'd like to set it ready for L10n on Wed 16 July 2014

https://support.mozilla.org/en-US/kb/remote-content-in-messages

Comment 39

[Joni Chan]

Looks good to me, Roland. I've set it ready for L10N.

Comment 40

[Vincent (caméléon)]

In the screen-shoot, I see that image from the same domain (mailchimp.com) need two different authorizations, one for gallery.mailchimp.com, and another one for cdn-images.mailchimp.com.

It seems to me that it is quite complicated for our users. Do you think an authorization for the entire domain (mailchimp.com) would be possible?

Comment 42

[Magnus Melin [:mkmelin]]

That should be possible. I didn't verify it but if you (in this case, manually) add mailchimp.com to to the whitelist it should cover all subdomains too.

CDNs are often on an entirely different domain though, so not much luck there.

Comment 43

[Thomas D. (:thomas8)]

For posterity and code/BMO archaeology, let's be more descriptive in summary about the nifty UI additions implemented here.

Comment 44

[Thomas Stein]

Can this feature be extended to allow remote content for a whole domain (including all subdomains) like

*.linkedin.com