This is a different solution to the idea proposed in Bug 303754 but it's trying to tackle the same problem. We might end up won't fixing 303754 and using this solution instead. * By default, thunderbird blocks remote content for every message * The remote content bar in the message pane would now have two buttons: Show Images and something like "Always Show From This Sender" * Clicking on Show Images shows the images JUST this one time. Clicking away and clicking back blocks the remote content again. This is different from the current behavior which always shows the messages for a message once you click "Show images" * "Always Show From This Sender" sets a property on a card in the address book to allow remote content from this sender. Eventually we could even have a field in the UI for the card that lets a user undo this change.

Work in progress. Address book changes to support the new policy for remote images. Add a new property for local address book cards for allowing remote images in HTML mail for this sender. I used the generic nsIAbMDBCard::set/getStringPropert to store this property on a local card. When editing a vCard, this property is hidden from the card. When looking at an LDAP card, this property is disabled. The new remote content bar in the UI now has two buttons: * Load Images * Always Load Images from Sender I really need to land the content policy code in Bug 330443 before I can hook up this new load behavior to avoid code conflict issues.

screen shot of the remote content policy bar with a 2nd button for always loading remote images from this sender. I'm also thinking of making the second button a text link that you click on. something like: [image] To protect your privacy, Thunderbird has blocked remote images in this message [Load Images] *smaller text* Click here to always load remote images from %S where %S is the author of the message.

When you click on the always show button, if the user isn't in your address book already, we bring up the new card dialog. Here you can see the new property for Allow remote images in HTML mail which I added to the Internet groupbox...

Here's an alternative UI I've been using, I think I like it more than having two buttons. I've got a pretty functional work in progress. Will post an updated patch shortly.

I wonder if the link should say: Click here to always load remote images from *address* or Always load remote images from *address* the "click here" could be implicit with making it look like a link.

This patch does the following: * Removes the privacy / general tab in the options UI. Remove blocking remote image prefs (including ab based white listing). It also removes the UI for enabling javascript (that's another desired bug that I'll link to here) * added a new method to nsIAbMDBCardDirectory, cardForEmailAddress, that returns the card associated with a particular e-mail address. Modified the implementation of nsIAbMDBCardDirectory::hasCardForEmailAddress to leverage cardForEmailAddress. * abCardOverlay.xul now has a checkbox for the allow remote images in HTML mail for this person in the internet properties groupbox. * nsAbMDBDirectory.h had a bunch of tabs in it. * abNewCardDialog.dtd (thunderbird only) had a bunch of duplicated entity names with abCardDialogOverlay.dtd so I removed those. * abCardOverlay.js. Add support for the new boolean property for allowing remote images in HTML mail. Hide it in vCards, disable it for LDAP directories like we do for other fields. use the generic setStringAttribute on a mdb card for setting the remote content value. * allowRemoteContentForSender in mailWindowOverlay.js walks through the local address books, the first match it finds for the sender, it sets the new address book property for allowing remote content and reloads the messsage.

Mark, do you think you'll have some time in the next couple days to take a look at these address book changes? See comment 6 for a description. If they look ok to you then I'll port the thunderbird specific AB changes to seamonkey as part of this patch so the fix goes in for both.

(In reply to comment #0) > Eventually we could even have a field > in the UI for the card that lets a user undo this change. This should be done soon because it is easy to accidentally add a malicious site. It is imperative to be able to remove it. (In reply to comment #3) > Created an attachment (id=243052) [edit] > the ab card dialog This seems to address my concern above. (In reply to comment #4) > Created an attachment (id=243718) [edit] > > Here's an alternative UI I've been using, I think I like it more than having > two buttons. It's the lesser of two evils: 1. Two buttons uses a lot of width and the second button is huge. 2. One button and one link for two very similar functions is less consistent. (In reply to comment #5) > I wonder if the link should say: If you want "normal" users to "get it", it might need to say "Click here...". PS. Would it make sense to add a similar UI (message header and address book) for JavaScript?

Comment on attachment 243865 [details] [diff] [review] breaking out just the address book related changes General comment: I almost r- this in the first instance because I thought the extra item in the dialog would make it significantly longer than it is now (thus making the 800x600 problem we have even worse). However it only makes the dialog 3pts higher at this stage so I guess its not too bad. Main comments: You are missing the locale changes from this part of the patch. Not too much of a problem because I got them from the main one, but we'll need to include them in the final of course. > * abCardOverlay.js. Add support for the new boolean property for allowing > remote images in HTML mail. Hide it in vCards, disable it for LDAP directories > like we do for other fields. use the generic setStringAttribute on a mdb card > for setting the remote content value. I'm not sure I'm quite happy with this. Firstly, if we enable writeable LDAP then the field will be enabled. Secondly, its also going to be enabled for outlook directories, and possibly mac specific (if/when they get implemented). In the LDAP case, I'd agree its probably not a good idea to have a remote query going on (unless we've cached them, which we don't really do at the moment). The mac and outlook cases may be relevant depending on what fields (if any) are available for us to use or add. Also using a nsIAbMDBCard function won't help extensions adding new types of address books (or sync - e.g. palmsync) or even ourselves if we decide to implement a new local address book type of our own later. So I think the allowRemoteContent field should be available from nsIAbCard not just nsIAbMDBCard. If we're going to say that remote address books can't have the allowRemoteContent field, then we should hide it if the directory is given as remote. What we do in the outlook case I'm not sure, hopefully David can advise what options we might be able to have. It may be that we need to add something to nsIAbDirectory to say that this directory type doesn't support various fields. Anyway, I'm willing to discuss those points if you think I'm wrong anywhere. HasCardForEmailAddress/CardForEmailAddress - I'm not really happy with these being just nsIAbMDBDirectory methods, but as that's the current implementation I think we could go with it now, but file a follow up bug to make that more generic. The code in general looks reasonable, I'll have a look at in detail when you update the patch.

Thanks for looking at this patch and for the feedback Mark. I had originally implemented this as a property on nsIAbCard and then after talking it over with Seth Spitzer, he reminded me of the generic property attributes on a local address book card. Since this was a property for local address books only, we thought it made sense to use the generic attribute method which was added for extensions to extend local ab properties...it should be pretty easy for me to go back to the original implementation based on nsIAbCard. I will put a new patch up that does just that. I would note that we've encouraged extension authors to add new local AB properties using the generic attribute methods on nsIAbMDBCard. Solving the issues you mentioned for outlook, mac and writeable ldap directories may also need to account for extensions as well (might be outside the scope of this bug...).

Mark, here's a patch that implements a new property on nsIAbCard for allowing remote content in HTML mail from this person. As we discussed, this patch hides the new checkbox property for all remote directories. I also found a bunch of tabs in nsAddrDatabase.h while adding support for the new property. I've checked in the entity strings for seamonkey and Thunderbird into the trunk and the branch in order to make sure the strings are there for seamonkey's l10n freeze so you shouldn't need that part from the earlier patch anymore. Not sure what to do for someone who has hacked their prefs to see an outlook directory yet. It looks like we handle this condition poorly already for fields that don't map like whether the user prefers html mail over text...Hmm.

(In reply to comment #10) > I would note that we've encouraged extension authors to add new local AB > properties using the generic attribute methods on nsIAbMDBCard. Solving the > issues you mentioned for outlook, mac and writeable ldap directories may also > need to account for extensions as well (might be outside the scope of this > bug...). Given where we are with the current status of address book (no mac, outlook not really "supported" - no UI, writeable ldap not there yet), I'm happy to leave these to other bugs.

Comment on attachment 244017 [details] [diff] [review] address book only changes. Using nsIAbCard I haven't had chance to test this and am unlikely to until Tuesday. As I prefer to test before I set r+, I'm letting you know in case you want to get this through earlier and want to request review from someone else. Generally the patch looks ok and if I'd had time to test it I'd probably be setting r+ assuming the comments below were fixed: interface nsIAddrDatabase : nsIAddrDBAnnouncer { ... + + [noscript] void addAllowRemoteContent(in nsIMdbRow row, in boolean value); It'd be nice for this to be next to the other add functions. Index: mailnews/addrbook/public/nsIAbCard.idl =================================================================== RCS file: /cvsroot/mozilla/mailnews/addrbook/public/nsIAbCard.idl,v retrieving revision 1.34 diff -u -r1.34 nsIAbCard.idl --- mailnews/addrbook/public/nsIAbCard.idl 20 May 2006 08:18:20 -0000 1.34 +++ mailnews/addrbook/public/nsIAbCard.idl 30 Oct 2006 00:46:44 -0000 @@ -37,7 +37,7 @@ #include "nsISupports.idl" -[scriptable, uuid(97448252-F189-11d4-A422-001083003D0C)] +[scriptable, uuid(673B53D2-15EB-450f-8F90-28BFFA43714E)] interface nsIAbPreferMailFormat { const unsigned long unknown = 0; const unsigned long plaintext = 1; You're updating the wrong uuid - there's two in that file and you want the second one. + /** + * allowRemoteContent in HTML mail sent by this contact + */ I think this would be clearer worded as: allowRemoteContent to be displayed in HTML mail received from this contact. NS_IMETHODIMP nsAbCardProperty::GetCardValue(const char *attrname, PRUnichar **value) NS_IMETHODIMP nsAbCardProperty::SetCardValue(const char *attrname, const PRUnichar *value) I think we should be updating these functions as well for allowRemoteContent. + *aCardExists = card.get() ? PR_TRUE : PR_FALSE; Can't this just be: *aCardExists = card ? PR_TRUE : PR_FALSE;

Updated patch based on Mark's review comments. Thanks Mark.

I filed Bug 359352 to track moving HasCardForEmailAddress and CardForEmailAddress to nsIAbCard

there was a typo in the previous address book patch.

Comment on attachment 244956 [details] [diff] [review] address book only changes. Includes seamonkey changes A lot of the lines you have added have extra spaces on the end. Also, a lot of the blank lines have unnecessary spaces in them as well. interface nsIAddrDatabase : nsIAddrDBAnnouncer { /** @@ -290,4 +291,6 @@ void InitCardFromRow(in nsIAbCard aNewCard,in nsIMdbRow aCardRow); void SetListAddressTotal(in nsIMdbRow aListRow, in PRUint32 aTotal); nsIMdbRow FindRowByCard(in nsIAbCard aCard); + + [noscript] void addAllowRemoteContent(in nsIMdbRow row, in boolean value); This should really go just under the addPopularityIndex function a few lines further up. interface nsIAbCard : nsISupports { // Card properties attribute wstring firstName; @@ -135,4 +135,9 @@ string convertToBase64EncodedXML(); wstring convertToXMLPrintData(); string convertToEscapedVCard(); + + /** + * allowRemoteContent to be displayed in HTML mail received from this contact. + */ + attribute boolean allowRemoteContent; Please put this attribute under the mailListURI attribute or the popularityIndex one. Index: mailnews/addrbook/src/nsAbCardProperty.cpp + PRBool allowRemoteContent; + srcCard->GetAllowRemoteContent(&allowRemoteContent); + SetAllowRemoteContent(allowRemoteContent); As we're not checking the return values, you should initialise this to PR_FALSE like you've done elsewhere in the patch. + // hide remote content in HTML field for remote directories + if (directory.isRemote) + document.getElementById('allowRemoteContent').hidden = true; Please also add the above into the OnLoadNewCard functions for both SM & TB. r=me with the above fixed.

(In reply to comment #17) > (From update of attachment 244956 [details] [diff] [review] [edit]) > A lot of the lines you have added have extra spaces on the end. Also, a lot of > the blank lines have unnecessary spaces in them as well. This has been a problem for me ever since I upgraded to visual studio 8. For some reason it likes to add lots of trailing spaces. Especially when I'm deleting or moving code around. Anyone else have this problem?

(In reply to comment #17) > + // hide remote content in HTML field for remote directories > + if (directory.isRemote) > + document.getElementById('allowRemoteContent').hidden = true; > > Please also add the above into the OnLoadNewCard functions for both SM & TB. Mark, I'm not sure we need that part. When we open a new card dialog, even if you have an ldap directory selected, the new card dialog only lets you add the card to an address book and not a directory, in which case we still want to show this property...

(In reply to comment #19) > (In reply to comment #17) > > + // hide remote content in HTML field for remote directories > > + if (directory.isRemote) > > + document.getElementById('allowRemoteContent').hidden = true; > > > > Please also add the above into the OnLoadNewCard functions for both SM & TB. > > Mark, I'm not sure we need that part. When we open a new card dialog, even if > you have an ldap directory selected, the new card dialog only lets you add the > card to an address book and not a directory, in which case we still want to > show this property... Yep, I see your point, leave it out for now, I'll have to look at a way round it for when we get write-able directories.

Updated against Mark's latest review comments. This is the patch I'll land on the trunk and (after a small baking delay) on the branch.

Mscott, Had 2 auto updates since your checkin on 2006-11-10 16:23 Still not seeing the extra dialog on blocked images. Is this still work in progress?

* Thunderbird UI changes to show the link text for allowing remote content for the sender. If the user chooses to allow remote content for the sender, we first look through all the local address books looking for a card, if we find one, we just set the new card attribute and reload the message. If we don't have a card, we bring up the new card dialog with the allow remote content in html attribute pre-checked. * Thunderbird Options UI changes. Remove the UI for disabling the remote content blocker (can still be tweaked in about:config). Remove the UI for disabling JS (can still be tweaked in about:config). Remove the UI for using a local addresss book as a white list for remote content (can no longer be tweaked as a pref). * Core content policy changes. Stop reading in the white listing prefs. New method for allowing remote content for sender which walks through the local address books looking for the allow remote content property for the sender. The content is only allowed if the property is set for the sender.

The content policy code and the front end code that tries to set the property, only looks for address books that implement nsIAbMDBDDirectory as those are the only types that currently support the allowRemoteContentInHTML attribute on nsIAbCard. I'm not positive that's the right thing to do and may need changed in the future.

Comment on attachment 245415 [details] [diff] [review] content policy change + thunderbird UI changes probably should relnote this since we're removing the whitelist by ab functionality.

I filed Bug 360288 last week for seamonkey to allow the user to load remote images from the sender on a per message basis from the message pane. Until that gets fixed, seamonkey users will have to set the field on the address book cards themselves if they alway want to load remote images from the sender.

The content policy change caused bustage on SeaMonkey trunk due to stricter c++ rules. I have hopefully corrected by doing this change: Index: mailnews/base/src/nsMsgContentPolicy.h =================================================================== RCS file: /cvsroot/mozilla/mailnews/base/src/nsMsgContentPolicy.h,v retrieving revision 1.14 diff -u -p -r1.14 nsMsgContentPolicy.h --- mailnews/base/src/nsMsgContentPolicy.h 13 Nov 2006 18:59:35 -0000 1.14 +++ mailnews/base/src/nsMsgContentPolicy.h 13 Nov 2006 19:56:59 -0000 @@ -82,7 +82,7 @@ protected: nsAdoptingCString mTrustedMailDomains; PRBool IsTrustedDomain(nsIURI * aContentLocation); - nsresult nsMsgContentPolicy::AllowRemoteContentForSender(nsIMsgDBHdr * aMsgHdr, PRBool * aAllowForSender); + nsresult AllowRemoteContentForSender(nsIMsgDBHdr * aMsgHdr, PRBool * aAllowForSender); nsresult AllowRemoteContentForMsgHdr(nsIMsgDBHdr * aMsgHdr, nsIURI * aRequestingLocation, nsIURI * aContentLocation, PRInt16 *aDecision); nsresult MailShouldLoad(nsIURI * aRequestingLocation, nsIURI * aContentLocation, PRInt16 * aDecision); nsresult ComposeShouldLoad(nsIDocShell * aRootDocShell, nsIURI * aContentLocation, PRInt16 * aDecision);

Thanks Mark. That broke the linux thunderbird build too.

(In reply to comment #28) > Thanks Mark. That broke the linux thunderbird build too. > As well as Win xp. the "click here to always load" dialog is now working as of version 3 alpha 1 (20061114) only after manually changing the pref to block remote images to true. It seems the following block of prefs was missing in all-thunderbird.js // Privacy Controls for Handling Remote Content ///////////////////////////////////////////////////////////////// pref("mailnews.message_display.allow.plugins", true); // disable plugins by default pref("mailnews.message_display.disable_remote_image", true); pref("mailnews.message_display.disable_remote_images.useWhitelist", true); pref("mailnews.message_display.disable_remote_images.whiteListAbURI","moz-abmdbdirectory://abook.mab"); ///////////////////////////////////////////////////////////////// This resulted in not blocking remote images (was set to block) continuing to allow javascript in mail/new (was set to enable) and continuing to allow plugins (was set to enable in all-thunderbird.js) after the update.

good catch Joe. In the check in to mailnews.js, I changed: pref("mailnews.message_display.disable_remote_image", false); when it should be true.

Comment on attachment 245415 [details] [diff] [review] content policy change + thunderbird UI changes I'm planning on landing this on the branch later today.

Here's the same patch but against the 1.8.1 branch.

This checkin seems to have an unwanted side-effect for newsgroups with remote images. The remote image is always blocked, but no dialog appears, or "allow" option. See: news://news.mozilla.org:119/mailman.1605.1164261688.2900.test-multimedia@lists.mozilla.org The remote image is blocked, with no notification, or option to allow. Should I file a new bug on this? Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061120 Thunderbird/3.0a1 ID:2006112003

This patch: * Removes unneeded if statement

Comment on attachment 246531 [details] [diff] [review] Fix msgHdrForCurrentMessage patch (Checked into trunk and 1.8.1 branch) Checking in (trunk) mailWindowOverlay.js; new revision: 1.142; previous revision: 1.141 done Checking in (1.8.1 branch) mailWindowOverlay.js; new revision: 1.95.2.42; previous revision: 1.95.2.41 done

This patch: * Removes code associated with the address books and whitelists from privacy.js

Comment on attachment 246548 [details] [diff] [review] Remove unneeded code from privacy.js (Checked into trunk & 1.8.1 branch) thanks for cleaning up my mess iann.

Comment on attachment 246548 [details] [diff] [review] Remove unneeded code from privacy.js (Checked into trunk & 1.8.1 branch) Checking in (trunk) privacy.js; new revision: 1.12; previous revision: 1.11 done Checking in (1.8.1 branch) privacy.js; new revision: 1.4.2.8; previous revision: 1.4.2.7 done

Well, the removal of whitelisting capability pretty much kills any options on the part of the user to select their desired level of security. Don't mean to spam this bug, but where does the user's preference come into play here. The situation right now is: 1. There is no UI to control remote images in newsgroups 2. If you would like to allow that, you must know about the now hidden pref 3. If you use the pref "mailnews.message_display.disable_remote_image" you also enable remote images in Mail, defeating the entire purpose of this patch. Of course there is always the possibilty of using a caps policy as a substitute for whitelisting.(But nobody but the most experiences user will go there) I understand that sometimes decisions have to be made for "the greater good" (like the decision to disable low level encryption, and remove the UI) I just think that if we continue to make determinations on what the user wants/needs, this should be done with a little more concern for the end user, and in a more flexible manner.

(In reply to comment #40) > 1. There is no UI to control remote images in newsgroups that's bug and we should fix it. You should get the same phishing bar you get if the message was from e-mail.

The problem with newsgroups is that nsNntpService::DecomposeNewsMessageURI does not work properly yet - see bug 179162(In reply to comment #41) > (In reply to comment #40) > > 1. There is no UI to control remote images in newsgroups > > that's bug and we should fix it. You should get the same phishing bar you get > if the message was from e-mail. > The problem with newsgroups is that nsNntpService::DecomposeNewsMessageURI does not work properly yet - see bug 179162

This patch: * Changes to using boolean for allowRemoteContent argument to abCardOverlay * Only reloads the message if allowRemoteContent has been updated to true * Changes to using label instead of description for allowRemoteContent link

Comment on attachment 246854 [details] [diff] [review] Back port changes from SM to TB (Checked into trunk & 1.8.1 branch) thanks again Ian.

Comment on attachment 246854 [details] [diff] [review] Back port changes from SM to TB (Checked into trunk & 1.8.1 branch) Checking in (trunk) base/content/mailWindowOverlay.js; new revision: 1.145; previous revision: 1.144 base/content/mailWindowOverlay.xul; new revision: 1.185; previous revision: 1.184 components/addrbook/content/abCardOverlay.js; new revision: 1.16; previous revision: 1.15 done Checking in (1.8.1 branch) base/content/mailWindowOverlay.js; new revision: 1.95.2.43; previous revision: 1.95.2.42 base/content/mailWindowOverlay.xul; new revision: 1.116.2.59; previous revision: 1.116.2.58 components/addrbook/content/abCardOverlay.js; new revision: 1.8.2.8; previous revision: 1.8.2.7 done

From a comment I read on the forums, I wonder if this new attribute for allowing remote content for html mail should be hooked up to the ldif export/import code...

*** Bug 259621 has been marked as a duplicate of this bug. ***

FYI, I pulled out the problem with the remote content UI not showing up for news articles with remote images into Bug 367529.

Is it possible to revert to the previous whitelist method? That is, enabling them for all entries in your address book rather than selecting individual users? I think this new method is a really great feature, but in my case I have a rather large yet selective personal address book. I'd much prefer to simply enable it for all users in that address book rather than editing each and every address book card to toggle the new option. I tried adding these preferences, but no luck: user_pref("mailnews.message_display.disable_remote_images.useWhitelist", true); user_pref("mailnews.message_display.disable_remote_images.whiteListAbURI", "moz-abmdbdirectory://personal.mab"); Am I just doing something wrong, or was this ability completely removed in favor of the new method?

Although I enjoy how it's operating in TB2.0 I'm in agreement with Jared for I also have extensive address book(s). However, any such changes please insure it includes Collected Addresses in order to incorporate all those addresses that are not wanted in the PAB. It would be nice to have a global setting, perhaps in the AB or in Options to allow all addresses in the various books to be checked to allow images.