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

RESOLVED FIXED in Thunderbird 31.0

Status

enhancement
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: mkmelin, Assigned: mkmelin)

Tracking

Trunk
Thunderbird 31.0
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tb31features])

Attachments

(3 attachments, 8 obsolete attachments)

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.
Attachment #8351720 - Flags: ui-review?(bwinton)
Attachment #8351720 - Flags: feedback?(mconley)
Posted image screenshots
Some screenshots
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.
Attachment #8351720 - Flags: ui-review?(bwinton) → ui-review-
Posted patch proposed fix, v2 (obsolete) — Splinter Review
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.
Attachment #8351720 - Attachment is obsolete: true
Attachment #8351720 - Flags: feedback?(mconley)
Attachment #8356755 - Flags: ui-review?(bwinton)
Attachment #8356755 - Flags: review?(mconley)
Posted image Screenshot of the new privacy options (obsolete) —
The new privacy options.
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).
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?
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.
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.
Posted patch proposed fix, v3 (obsolete) — Splinter Review
Unbitrotted, now needs to be applied on top of the remote content policy patch from bug 457296.
Attachment #8356755 - Attachment is obsolete: true
Attachment #8356755 - Flags: ui-review?(bwinton)
Attachment #8356755 - Flags: review?(mconley)
Attachment #8358792 - Flags: ui-review?(bwinton)
Attachment #8358792 - Flags: superreview?(mbanner)
Attachment #8358792 - Flags: review?(mconley)
Posted patch proposed fix, v4 (obsolete) — Splinter Review
Minor update.
Attachment #8358792 - Attachment is obsolete: true
Attachment #8358792 - Flags: ui-review?(bwinton)
Attachment #8358792 - Flags: superreview?(mbanner)
Attachment #8358792 - Flags: review?(mconley)
Attachment #8365649 - Flags: ui-review?(bwinton)
Attachment #8365649 - Flags: superreview?(mbanner)
Attachment #8365649 - Flags: review?(mconley)
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?)
Attachment #8365649 - Flags: ui-review?(bwinton)
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/mkmelin@iki.fi-a616b5b5299d/
Attachment #8365649 - Flags: ui-review?(bwinton)
Posted patch proposed fix, v5 (obsolete) — Splinter Review
Fix the test and bitrot
Attachment #8378957 - Flags: ui-review?(bwinton)
Attachment #8378957 - Flags: superreview?(mbanner)
Attachment #8378957 - Flags: review?(mconley)
Attachment #8365649 - Attachment is obsolete: true
Attachment #8365649 - Flags: ui-review?(bwinton)
Attachment #8365649 - Flags: superreview?(mbanner)
Attachment #8365649 - Flags: review?(mconley)
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.
Attachment #8378957 - Flags: superreview?(standard8)
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/mkmelin@iki.fi-a616b5b5299d/ but I don't know for now long they are kept - usually not very long IIRC.
Added an info link beneath the remote content
Attachment #8356756 - Attachment is obsolete: true
Posted patch proposed fix, v6 (obsolete) — Splinter Review
Unbitrotted and with the info link.
Attachment #8378957 - Attachment is obsolete: true
Attachment #8378957 - Flags: ui-review?(bwinton)
Attachment #8378957 - Flags: review?(mconley)
Attachment #8394094 - Flags: ui-review?(bwinton)
Attachment #8394094 - Flags: review?(mconley)
Just a drive by comment,  but there is going to be no issue here with Lightning adding it's icon to options?
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 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 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!
Attachment #8394094 - Flags: ui-review?(bwinton) → ui-review+
Posted patch proposed fix, v7 (obsolete) — Splinter Review
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.
Attachment #8394094 - Attachment is obsolete: true
Attachment #8394094 - Flags: review?(mconley)
Attachment #8406324 - Flags: ui-review+
Attachment #8406324 - Flags: superreview?(neil)
Attachment #8406324 - Flags: review?(mconley)
^^^ this patch needs part 2 of bug 457296 applied first
Comment on attachment 8406324 [details] [diff] [review]
proposed fix, v7

(Why not use a Set for your hosts?)
Attachment #8406324 - Flags: superreview?(neil) → superreview+
A Set would lose the host order. Granted maybe it's not that important, but still.
mconley: ping on the review, would be nice to get this in for 31
I'll see if I can get it done tonight.
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. :)
Attachment #8406324 - Flags: review?(mconley)
(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.
(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.
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.
Attachment #8406324 - Attachment is obsolete: true
Attachment #8412070 - Flags: ui-review+
Attachment #8412070 - Flags: superreview+
Attachment #8412070 - Flags: review?(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.
Attachment #8412070 - Flags: review?(mconley) → review+
https://hg.mozilla.org/comm-central/rev/45cc03b40060 -> FIXED

Note to self: create the "more info" support page.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(mkmelin+mozilla)
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
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+
Flags: needinfo?(mkmelin+mozilla)
Whiteboard: [tb31features]
Roland, could you get the https://support.mozilla.org/kb/remote-content-in-messages url approved?
Flags: needinfo?(rtanglao)
did a major update, will set to L10N ready once jsavage approves
: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
Flags: needinfo?(jsavage)
Looks good to me, Roland. I've set it ready for L10N.
Flags: needinfo?(jsavage)
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?
Duplicate of this bug: 370165
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.
Blocks: 995737
Blocks: 1193200
Depends on: 1209910
For posterity and code/BMO archaeology, let's be more descriptive in summary about the nifty UI additions implemented here.
Summary: expose remote content per-host privileges → Expose remote content per-host privileges in notification bar's [Option] button, and implement Options > Privacy tab
Can this feature be extended to allow remote content for a whole domain (including all subdomains) like

*.linkedin.com
You need to log in before you can comment on or make changes to this bug.