Currently Thunderbird uses the Address Book interface to decide if a sender can load remote images from a message. This is not the desired behavior for the system of interactions needed. We need a separate white list of email addresses stored that the user has specified as able to load remote images sent in a message. This separate list should probably be for email only (but across all accounts) bug 296258 describes the problem of sharing this with RSS This separate list allows us to remove the problematic Card Editor UI described in bug 363856. We would no longer need the Card Editor for adding addresses to the list as described in bug 363948 where Thunderbird, for what appears to be no reason, opens up the address book card editor when you click the "Always load remote images..." link. Clicking the "Always load..." link from the Privacy Alert Box should add the listed email address with no questions asked, remove the alert box, and begin loading the remote images. Also this requires us to add in a piece of UI for removing this email address from the Always list directly in the message reader. see for example: http://clarkbw.net/designs/msg-reader/v2/msg-reader-remote-images.html This list will likely require additional UI bit to the Address Book UI for managing the list of addresses that are allowed to load remote images. I'm not entirely sure the format this is delivered in. From this interface if there are addresses in this list that we have contacts for it would be nice for us to relate those identities to the addresses listsed. Another option would have us adding UI bits to the Preferences -> Privacy page where we could load up the addresses allowed to load remote images such that people could remove some or all of them. It might make the most sense to just launch the Address Book interface (if that's the format we concentrate on) from the preferences page. Just to keep in mind while working on this that we're using the word Contact and not Card anymore (bug 455246)

One more note for the Address Book UI bits is that it would also be helpful to grab the list of messages sent by any of these remote image loader addresses in case people can't remember who foo@freestuff.com is and what they sent.

Taking and putting on beta 2 radar.

I'm glad to see that this is on the radar. I've been missing remote images for some time while tracking the nightlies.

I vote for this one too. We need some UI to have white/black list that also supports wildcards and/or regex (to allow/block whole domains for example).

This patch has basic support for session storage of remote image preferences, basically just starting to get the APIs in place. I'm not dealing with domain options here, they should be easy to add in later. Things to do in this patch: - If no storage file on initialisation, grab existing data from the address books. - Implement storage mechanism - Implement UI for adding & removing email addresses - Remove existing address book system.

Any particular reason not to keep the storage mechanism the same?

(In reply to comment #6) > Any particular reason not to keep the storage mechanism the same? You mean as attributes in address book alongside the cards? The issue is that is that we can't do it for non-local address books easily, e.g. consider OS X which doesn't have a field to map to, and LDAP where we'd currently have to do a remote fetch. I'm currently thinking of maybe a json file or sql. We want something simple, I guess we don't need to obscure addresses because they aren't obscured for the address books currently anyway.

Since we're going to be doing this to fix at least one other blocker, setting as blocking so we can track it.

We had a discussion on this offline. Whilst this would make managing some things easier (e.g. for remote/external address books where we don't have an option to store the field), it is unclear if doing this really has major benefits. Due to trying to drive Thunderbird 3 out, we are therefore not going to implement this option at this time, but fix up the current issues with remote images. Once we get to the next version after TB 3 we may be redoing how address book works in general, which may further reduce the need for this, but as that isn't defined yet, leaving the bug open for now.

Something else should be considered, if possible: Some services use different fake-email-addresses with the same domain, i.e. donontreplay@domain.com,news@domain.com, etc. It would be interesting to think about wildcards in such a list, e.g. *@domain.com which should at least be possible by a simple UI-question like "Do yo want images of any e-mails from domain.com to be loaded automatically?"

that would be nice to incorporate into this somehow. we'll have to balance that feature carefully so people don't turn on domain loading by mistake when they meant just one email address.

In reply to comment #11 and comment #12: One of my newsletters not only uses remote images, it also uses email addresses containing (AFAICT) a pseudorandom string which varies from one issue to the next, e.g. "From: La Première <2352.hxgcwn@campaign.citobi.be>" -- this means that until or unless I can whitelist that domain (which I've never seen used for something else) or that newsletter (regardless of the exact from-address used) I'll have to allow images again and again for every issue of the newsletter. Maybe "Allow remote images (in this single email)" could be added as a Filter action in addition to, or instead of, whitelisting with wildcards. If this idea is deemed useful, I suppose it will be handled in a followup bug.

Sounds like the filter suggestion is a good one (imo), but yes, a followup is a good idea; Might even be possible to do each of these disjointly (any one of those options can probably be started first).

I have the same problem as Tony. Twitter and Google+ both use the same type of unique From: address and needing to click something on every single email just to see the images isn't great. The only alternative right now seems to be showing remote content by default from everyone, also not great.

(In reply to Scott Reynen from comment #15) > I have the same problem as Tony. Twitter and Google+ both use the same type > of unique From: address and needing to click something on every single email > just to see the images isn't great. The only alternative right now seems to > be showing remote content by default from everyone, also not great. As far as I can tell, Twitter, G+ and Facebook don't vary the From: address over time; it looks like they generate a From: address related to each ID on their service and then send all messages about that ID using the same address. You should only need to whitelist one sender for G+ and Facebook, and three or so for Twitter because they use separate senders for different kinds of notification (e.g. follow, mention and DM)

add support for email addresses to nsPermissionManager. Changes are restricted to one method that extracts the appropriate string key (either host name, email address or "<file>" special case) from the incoming URI, and to the lookup method that previously matched by successively removing the most local part from the domain; this lookup now first removes the email local part if any and then progresses up the domain name. Note for Thunderbird devs: this patch needs to be applied relative to the mozilla subtree of your build environment, not comm-central.

Proposed patch to have Thunderbird's content policy call a modified core PermissionManager to look up the remote content whitelist. Still to come: UI changes to add and manage the white list entries; pending approval of the proposed modifications to nsPermissionManager.cpp in patch Part 1

Comment on attachment 576598 [details] [diff] [review] Part 1 - modify nsPermissionManager to support mailto: URIs I shouldn't be reviewing this code.

Comment on attachment 576598 [details] [diff] [review] Part 1 - modify nsPermissionManager to support mailto: URIs I'm open to suggestions as to how this can move forward...

Thanks to Jim Porter for pointing me in this direction. This would resolve 486479, too.

Re Comment #16, I posted a Bug #784069 similar to this for Facebook. The (OBSCURE) solution was to add the address you receive, find it in the AddressBook and uncheck "Always Prefer Display Name" If you have other problems with this, and given the large number of long-time bugs related to this problem (fuller list on Bug #784069) you might want to set preference mailnews.message_display.disable_remote_image to false so that it allows ALL images as the risk of a malicious image is probably less than the impact from TB bugs.

Another (nicer) way to do this would be to add a flag to each AddressBook (including Read-Only ones like MacOSX or LDAP) that allows setting a global "Allow remote images" for anyone in the book - I think that is the most likely requirement anyway, for example I would immediately set that for all my address books (OSX, personal etc) except for the "Spammers" address book (which is used by one of my filters).

Comment on attachment 576598 [details] [diff] [review] Part 1 - modify nsPermissionManager to support mailto: URIs Review of attachment 576598 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/cookie/nsPermissionManager.cpp @@ +1044,5 @@ > if (!innerURI) return NS_ERROR_FAILURE; > > + bool isScheme = false; > + nsresult rv = NS_OK; > + if (NS_SUCCEEDED(aURI->SchemeIs("mailto", &isScheme)) && isScheme) { I'm a bit dubious about hard-coding mailto into core gecko, but at the same time, I think it is probably reasonable if we're working with different forms of uris. ::: extensions/cookie/test/unit/test_permmanager_mailto.js @@ +1,4 @@ > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +function run_test() { There should probably be a test for file:// as well, if there isn't already.

Comment on attachment 576599 [details] [diff] [review] Part 2 - modify Thunderbird content policy to call (modified) PermissionManager Review of attachment 576599 [details] [diff] [review]: ----------------------------------------------------------------- I think we've got some mozmill tests for this, which would obviously need to be updated, and we'd need UI, of course, but in general I think the idea is sane. ::: mailnews/base/src/nsMsgContentPolicy.cpp @@ +452,5 @@ > > // Case #3, the domain for the remote image is in our white list > bool trustedDomain = IsTrustedDomain(aContentLocation); > > + // Case 4 is expensive as we're looking up items in the permissions database. So if I'm hoping it isn't so expensive ;-)

Hey Josh, you're listed as a peer of this component. How's my driving here? Anything big that I'm missing?

Comment on attachment 576599 [details] [diff] [review] Part 2 - modify Thunderbird content policy to call (modified) PermissionManager This patch might still apply. Pulling it out of retirement.

A comment on the goal pursued here: IMO it is quite flawed security to enable image loading based on sender addresses. As we all know, sender addresses in e-mails can be easily spoofed. Indeed, they often _are_ spoofed in spam mails, using addresses "similar" to the recipient's address, i.e., known to the recipient. At the very least, the domains from which images can be loaded should be limited in a useful way ...

(In reply to Jens Müller from comment #28) > A comment on the goal pursued here: IMO it is quite flawed security to > enable image loading based on sender addresses. This is quite true. But I should point out two things: 1) This is what we already do with the current address book, reading a boolean "AllowRemoteContact" property off of a contact card 2) The goal pursued here, at least for me, is to rebuild the Thunderbird address book. That means moving stuff out of the address book that isn't in there, which includes the remote content permissions. So what we're proposing here is arguably no worse than what we already have, and helps move us forward on making a better address book.

As they say, the Pefect is the enemy of the Good, and currently Firefox is unusable on a Mac that uses Mac Address Book without disabling the "disable_remote_image" section i.e. because we don't have imperfect security we have NO security. (See Comment#22 and Comment#23)

Comment on attachment 713924 [details] [diff] [review] WIP Patch 1 Review of attachment 713924 [details] [diff] [review]: ----------------------------------------------------------------- I sincerely apologize for the totally unreasonable delay in reviewing this. If you've got any more cookie/permission work that needs reviewing, I promise to hop right on it. ::: extensions/cookie/nsPermissionManager.cpp @@ +133,5 @@ > + // query part if one exists, so we eliminate everything after a ?. > + bool isMailTo = false; > + if (NS_SUCCEEDED(uri->SchemeIs("mailto", &isMailTo)) && isMailTo) { > + rv = uri->GetPath(aHost); > + if (NS_SUCCEEDED(rv)) { Why not fail properly here instead of falling through? @@ +134,5 @@ > + bool isMailTo = false; > + if (NS_SUCCEEDED(uri->SchemeIs("mailto", &isMailTo)) && isMailTo) { > + rv = uri->GetPath(aHost); > + if (NS_SUCCEEDED(rv)) { > + PRInt32 spart = aHost.FindChar('?', 0); int32_t

(In reply to klonos from comment #4) > I vote for this one too. We need some UI to have white/black list that also > supports wildcards and/or regex (to allow/block whole domains for example). +1! I also assume that the whitelist UI being worked on in this bug will (need to) support wildcards and/or regex (to allow/block whole domains for example). If not, please open a followup bug for that, or reopen duplicate bug 302192. Note that we already have a similar pref (without UI): mail.trusteddomains (implemented by bug 300834) Adding domains to that whitelist (via config editor) will bypass various privacy controls including blocking of remote images: (From attachment 189846 [details] [diff] [review], code comment on mail.trusteddomains pref) > Specific domains can be white listed to bypass various privacy controls in > Thunderbird such as blocking remote images, the phishing detector, etc. This > is particularly useful for business deployments where images or links > reference servers inside a corporate intranet. For multiple domains, separate > them with a comma. i.e. pref("mail.trusteddomains", > "mozilla.org,mozillafoundation.org");

Wow, it's taken me a long time to get back to this. Ok, thanks for the feedback Josh, here's an updated patch.

Comment on attachment 820048 [details] [diff] [review] Patch v1 - checked in Review of attachment 820048 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/cookie/test/unit/test_permmanager_mailto.js @@ +7,5 @@ > + const kType = "test-mailto"; > + const kCapability = 1; > + > + // make a mailto: URI with parameters > + let uri = Services.io.newURI("mailto:" + kTestAddr + "?subject=test", null, Huh, do we not need to import Services.jsm any more?

Comment on attachment 820048 [details] [diff] [review] Patch v1 - checked in Thanks Josh! Yep, we seem to get Services.jsm for free. :D Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/a5a25f0e61ee. I can't seem to set check-in+ on this file, so I'll just do it in the attachment name...

So the platform bit has landed, and here's what's left: 1) We need to get Thunderbird to query nsIPermissionManager to check if we can display remote images - Irving's patch seems to do this pretty well. 2) We need some code for *adding* those permissions - we can hook that up to the current UI. 3) We probably need some new UI for *revoking* those permissions. I'll see about #1 and #2 first.

Not necessarily this bug, but I've been dreaming we could add a menu item to the blocked remote content menu that could bring up the permission manager... It would be very useful to be able to whitelist images from sites you regularly get mails from. (Whitelist based on what domain the image is on, not the sender.)

This is an updated version of Irivings patch (part2), with an added test.

A small update.

This makes us able to actually remove permissions for an added email-address without trickery.

And with this, the address book should be freed from remote content stuff. (SeaMonkey will need some UI work, I'm not volunteering: remove the checkboxes, and make the privileges list viewable from some suitable place, if there isn't one already)

Comment on attachment 8365585 [details] [diff] [review] patch - part 3 (additional fix to permission manager) - checked in https://hg.mozilla.org/integration/mozilla-inbound/rev/fa9b60c88118

Backed out for Windows bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac0b47d6465 https://tbpl.mozilla.org/php/getParsedLog.php?id=34154950&tree=Mozilla-Inbound

Relanded with if/else instead as the ternary confused windows - https://hg.mozilla.org/integration/mozilla-inbound/rev/0a388fbfac0f

Which release did the backend change land in? (In reply to Magnus Melin from comment #46) > Relanded with if/else instead as the ternary confused windows - > https://hg.mozilla.org/integration/mozilla-inbound/rev/0a388fbfac0f You were really lucky that it didn't confuse gcc - your two string literals happened to be the same length, so gcc was able to evaluate the type of the ternary as being const char (&)[8].

(In reply to neil@parkwaycc.co.uk from comment #48) > Which release did the backend change land in? The mailnews content policy backend changes haven't landed yet - that's part 2 attached to this bug. (The main mozilla-central change landed on trunk ~ 2013-11-18)

The mail/ ui is mainly bug 953426 - which is also waiting for review.

Comment on attachment 8365582 [details] [diff] [review] patch - part2 (modify content policy), v2 Can I get a fresh try build with just the changes from this bug, so I can play around with it please?

(In reply to Mark Banner (:standard8) (away until 3 Mar) from comment #51) > Comment on attachment 8365582 [details] [diff] [review] > patch - part2 (modify content policy), v2 > > Can I get a fresh try build with just the changes from this bug, so I can > play around with it please? The builds from http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/mkmelin@iki.fi-a616b5b5299d/ should still be "fresh". It also includes bug 953426, but that UI has parts semi-required for this bug. (Can be decoupled, but not without bitrot.) Apart from removing the "load remote images" option from the address book, this bug has pretty little UI impact: just slightly changes the menu for "load for this e-mail address".

Comment on attachment 8365582 [details] [diff] [review] patch - part2 (modify content policy), v2 Re-flagging for review

Comment on attachment 8365586 [details] [diff] [review] patch - part 4 (migrate and remove the remote content settings from ab) Review of attachment 8365586 [details] [diff] [review]: ----------------------------------------------------------------- I think this looks good to me - though you might (definitely) want someone from mailnews to look at it too to make sure I didn't miss a spot where allow remote content is still read in from the cards. ::: mailnews/base/util/mailnewsMigrator.js @@ +115,5 @@ > + * The address book used to contain information about wheather to allow remote > + * content for a given contact. Now we use the permission manager for that. > + * Do a one-time migration for it. > + */ > +function MigrateABRemoteContentSettings() *sigh*, it'd be so lovely if we could do this off the main-thread, or somehow delay it to sometime after start-up. Maybe find some idle time to do this maintenance - but I guess we'd better do it all in one shot as soon as we can. @@ +117,5 @@ > + * Do a one-time migration for it. > + */ > +function MigrateABRemoteContentSettings() > +{ > + if (Services.prefs.prefHasUserValue("mail.ab_remote_content.migrated")) We should have a const for the current, most recent migration number, and then bail out iff this pref matches that const. If not, we should do the migration steps that come after - much like we do with migrateUI. @@ +138,5 @@ > + if (addrbook.readOnly) > + continue; > + > + let childCards = addrbook.childCards; > + while (childCards.hasMoreElements()) Trailing whitespace in here.

(In reply to Mike Conley (:mconley) from comment #54) > Comment on attachment 8365586 [details] [diff] [review] > patch - part 4 (migrate and remove the remote content settings from ab) > We should have a const for the current, most recent migration number, and > then bail out iff this pref matches that const. If not, we should do the > migration steps that come after - much like we do with migrateUI. We have a const (kABRemoteContentPrefVersion), but like the other functions in this file we just check if it's set at all. These are one-time migrations - migrateUI is designed a bit differently but the but the end result it the same.

Updating, fixing bitrot. Carrying fwd r=mconley

Comment on attachment 8394091 [details] [diff] [review] patch - part 4 (migrate and remove the remote content settings from ab) Review of attachment 8394091 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine, though please add a unit test before landing. Additionally, I'm assuming you're going to give the SeaMonkey folks a heads-up before landing this. ::: mailnews/base/util/mailnewsMigrator.js @@ +115,5 @@ > + * The address book used to contain information about wheather to allow remote > + * content for a given contact. Now we use the permission manager for that. > + * Do a one-time migration for it. > + */ > +function MigrateABRemoteContentSettings() This should really have a unit test, to ensure we don't break the migration functionality.

With unit test, for checkin.

Comment on attachment 8365585 [details] [diff] [review] patch - part 3 (additional fix to permission manager) - checked in >+ nsCString scheme = NS_LITERAL_CSTRING( >+ (aHost.FindChar('@') == -1) ? "http://" : "mailto:"); You are extraordinarily lucky that "http://" and "mailto:" are the same length.

Actually, not so lucky - see comment 46, windows still didn't like that.

rebased part 4 due to the changes to approach in bug 953426.

Does this also implement Bug 608844 - Never show remote content from site? If not, is backend easily extended to allow it?

Yes, this implements bug 608844 too.

part2 https://hg.mozilla.org/comm-central/rev/ec82de81c636 part4 https://hg.mozilla.org/comm-central/rev/af30e12503ab -> FIXED

FTR: As hinted in comment 50, the UI/frontend for this was implemented in: Bug 953426 - Expose remote content per-host privileges ...which implemented the current [Options] button on remote content info bar, with a dropdown with plenty of remote content options.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #66) > FTR: As hinted in comment 50, the UI/frontend for this was implemented in: > > Bug 953426 - Expose remote content per-host privileges > > ...which implemented the current [Options] button on remote content info > bar, with a dropdown with plenty of remote content options. More precisely, it was bug 562048 which removed [Show Remote Content] button and replaced it with rudimentary [Options] button, whose dropdown menu was then extended to expose remote content per-host privileges in Bug 953426 and Bug 1209910.

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

Not very discoverable, but IIRC if you add linkedin.com it whitelists all of the domain (including subdomains)