452879 - Junk message with from address matching to are whitelisted

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Kent James (:rkent)]

Attachment: Whitelisted email

Because a user frequently has himself in his address book, junk emails sent with From matching To may be whitelisted - such as this sample that I just received.

We should probably exclude the To address when checking for whitelist From.

Comment 2

[Dawn Endico]

Probably also want to de-whitelist the user's reply-to addresses. Also maybe addresses for other identities? Neither of those would help my problem but they seem to make sense.

It looks like this is the code that needs to be changed.
http://mxr.mozilla.org/comm-central/source/mailnews/base/resources/content/junkCommands.js#180

Get the user's email and reply-to addresses from nsIMsgIdentity
http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgIdentity.idl

For example:
http://mxr.mozilla.org/comm-central/source/mailnews/base/resources/content/mailCommands.js#59

Comment 4

[era+mozilla]

See also bug #484359 which rather suggests that the user's own address should not be whitelisted.  The To==From is completely legitimately used e.g. by some mailing lists, which you probably want to continue to be able to whitelist.

Comment 5

[Kent James (:rkent)]

(In reply to comment #4)
> See also bug #484359 which rather suggests that the user's own address should
> not be whitelisted.
That is what I always intended to implement.  But I concede that my explanation in previous comments is confusing.

Comment 7

[Kent James (:rkent)]

Attachment: Backend/hidden preference fix

This patch adds two new preferences that control whether we suppress whitelisting for the email address or the domain of users found in the identities for the server. This new option is currently disabled by default. The intention is that in a followup bug we will deal with the issue of what should be enabled by default, and whether we need any UI for this.

I did sr Standard8 because it deals mostly with interactions with address book, plus some string and frozen linkage issues. Feel free to add bienvenu if needed.

Comment 8

[Kent James (:rkent)]

I'm adding bienvenu to the cc: in case he would rather sr.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 369716 [details] [diff] [review]
Backend/hidden preference fix

>+    nsCOMPtr<nsIMsgAccountManager>
>+      accountManager(do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv));
>+    NS_ENSURE_SUCCESS(rv,rv);

nit: space after comma please.

>+      nsCOMPtr<nsIMsgIdentity> identity;
>+      rv = identities->QueryElementAt(index, NS_GET_IID(nsIMsgIdentity),
>+                                      getter_AddRefs(identity));

This should be using do_QueryElementAt

>+#include "nsTarray.h"

Should be "nsTArray.h" (capitalise the first A)

>+// should we allow whitelisting of the email addresses for a server's identities?
>+pref("mail.server.default.whiteListIdentityUser", true);
>+// should we allow whitelisting of the domain for a server's identities?
>+pref("mail.server.default.whiteListIdentityDomain", true);

These feel the wrong way round, but its probably just me, and I'd want to add "dont" in and we'd have a negative pref which would probably be even worse.

r/sr=Standard8 with the previous comments fixed.

Comment 10

[era+mozilla]

> >+// should we allow whitelisting of the email addresses for a server's identities?
> >+pref("mail.server.default.whiteListIdentityUser", true);
> >+// should we allow whitelisting of the domain for a server's identities?
> >+pref("mail.server.default.whiteListIdentityDomain", true);
>
> These feel the wrong way round, but its probably just me, and I'd want to add
> "dont" in and we'd have a negative pref which would probably be even worse.

I can't pretend I understand this but if the preference means "do not whitelist the user's identity / the user's domain" then it should definitely have a name which doesn't mean the opposite.  If this is correct, and if you want to avoid "dont" then how about something like inhibitWhiteListingIdentityXxxx?

Comment 11

[Kent James (:rkent)]

I agree that the terms can be confusing, that's why I added the code comment:

// The terms to describe this get wrapped up in chains of negatives. A full
// definition of the boolean whiteListIdentityUser is "Don't suppress address
// book whitelisting if the sender matches an identity's email address"

The chain of negatives is:

- Junk itself is a negative term
- "whitelist" actually means "don't apply junk analysis"

So suppress (or inhibit as Era suggests) whitelisting adds a chain of three negatives. And I also agree with Standard8 that it feels wrong (which means that I feel the preference should have been something we default false, not default true, such that adding the additional capability of this code is enabled when the preference is true, not false.) So the polarity of the preference adds a fourth possible negative to the chain.

And to make matters worse, it is very easy to interpret "whiteListIdentityUser" to mean that we always whitelist the user identity when true, when in fact what we do is simply allow the existing addressbook whitelisting to work.

I would have no problem with inhibitWhiteListingIdentityUser (/Domain) if there is a sense that this is a better way. That is reversing the polarity by the way, so the existing name does not actually mean the opposite.

Comment 12

[Mark Banner (:standard8)]

(In reply to comment #11)
> I would have no problem with inhibitWhiteListingIdentityUser (/Domain) if there
> is a sense that this is a better way.

I think inhibit is definitely a clearer option.

Comment 13

[Kent James (:rkent)]

Attachment: Fixed nits, reversed sense of preferences

I've incorporated the comments, patch to checkin.

Comment 14

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/71bdea990aae