Open Bug 391717 Opened 12 years ago Updated Last year

filter with From:/To: criteria doesn't work if message's address is null or missing (neither IsInAB nor IsntInAB filter operator matches the address)

Categories

(MailNews Core :: Filters, defect)

1.8 Branch
defect
Not set

Tracking

(Not tracked)

People

(Reporter: wsmwk, Assigned: aceman, NeedInfo)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 1 obsolete file)

thunderbird trunk or 2.0 - don't remember which.  excerpt of message is below. If needed I can supply the full message to debug this.

filter criteria included
 subject begins with               [SPAM]
 from    isn't in my address book  personal
 from    doesn't contain           lehigh.edu

Don't know if which "from" criteria bombed - maybe both have bugs.

As this is yet one more way for spammers to defeat thunderbird I wonder if "from is the only field at risk ... perhaps other fields like "subject" are at risk of exploitation with respect to mozilla filters.

X-Envelope-From: <>
X-Envelope-To: <blah@lehigh.edu>
Received: from blah
	(envelope-from <>
Return-Path: <>
Received: from UnknownHost [211.92.245.59] by maila19.webcontrolcenter.com with SMTP;
   Thu, 9 Aug 2007 11:11:32 -0700
Received: from szjh-jsfoovenfe ([202.105.152.14])
          by smtp.xz.chinaunicom.com (Lotus Domino Release 6.5.3FP1)
          with ESMTP id 2007081001551082-20835 ;
          Fri, 10 Aug 2007 01:55:10 +0800 
From: "faysmakeyouqte" <>
To: "webmaster" <webmaster@ninggangyueqi.com>
Subject: %         ~
 =?GB2312?B?yMPO0sPH0K/K1rrP1/eyor34o6FfY2poZGZncHlxdWFsaXR5WDcyYUpJ?=
Date: Fri, 10 Aug 2007 01:55:20 +0800
X-Mailer: Microsoft Outlook Express 6.00.2800.1106
X-MimeOLE: Produced By Microsoft MimeOLE V6.00.2800.1106
follow up to bug 404264 comment 31

to summarize, for message header
  From: "faysmakeyouqte" <>

message doesn't star, incorrectly IMO, using
 from    isn't in my address book  personal


nothing in error console (didn't expect anything)
so don't think related to bug 404264 
(In reply to comment #1)
> so don't think related to bug 404264 

I agree. Looking at the code:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/base/search/src/nsMsgSearchTerm.cpp&rev=1.121.4.10&mark=882-883#876

In the failure case, we should probably be altering pResult to PR_TRUE. Additionally, we should now be able to drop that check as HasCardForEmailAddress (or cardForEmailAddress on trunk) will work correctly per bug 404264.

OS: Windows XP → All
Hardware: PC → All
nominating TB3
Flags: wanted-thunderbird3?
Whiteboard: [good first bug]
I'll probably be able to get to this with my other filter/search work.
Product: Core → MailNews Core
rkent - this may have some bearing on your current "search addresses" work.
found another occasion to want this
Summary: filter with from criteria doesn't work if message's From: address is null or missing → filter with From:/To: criteria doesn't work if message's address is null or missing
We return "no match" regardless of whether you have "is in AB" or "isn't in AB".

So can we decide whether "" is in the addressbook or not? It seems this bug and bug 1016213 wants it to NOT BE in the addressbook. Also the fix in bug 404264 makes CardForEmailAddress return null card for empty address which would mean empty is NOT in addressbook.
Assignee: nobody → acelists
Duplicate of this bug: 1016213
I think it's reasonable to assume "" is not in the addressbook, for practical purposes.
The check in nsMsgSearchTerm::MatchInAddressBook we are now poking at was added in bug 202169:
  // Some junkmails have empty From: fields.
  if (aAddress.IsEmpty())
    return rv;

There is not much talk in the bug whether it actually fixes the problem of the reporter, which looks to be the same as in this bug. So did it regress? Or did the code behave differently in those times.

Anyway, removing this check doesn't seem enough.
If there is really no address in the message (like "From:"), then MatchInAddressBook gets passed a aAddress of "-" (why???).

But i the message contains at least some characters in the From address (like "From:  <>"), aAddress isn't "" or null as we'd expect, but it is the user's email (TB user). I think I have seen this substitution in some other bug.
Blocks: 202169
new bug 1034408 is similar
(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #12)
> new bug 1034408 is similar

I think it is not. This one is about empty data received and a non-empty filter acting on it wrongly.

The other one is about empty rule in filter on non-empty data.
Summary: filter with From:/To: criteria doesn't work if message's address is null or missing → filter with From:/To: criteria doesn't work if message's address is null or missing (neither IsInAB nor IsntInAB filter operator matches the address)
Attached patch patch (obsolete) — Splinter Review
I implemented this but I have encountered one catch:
For rules accessing multiple headers (e.g. ToOrCC or AllAddresses) having a msg with e.g. To in addressbook, From in addressbook, but Bcc not defined (empty), would still evaluate to a match for the rule "AllAddresses,IsntInAb".
I'm not sure if that is a problem.
Attachment #8669476 - Flags: review?(rkent)
Comment on attachment 8669476 [details] [diff] [review]
patch

bugmail9 does not exist, I suspect you forgot to add it to the patch when you created a new file?
Attachment #8669476 - Flags: review?(rkent) → review-
Attached patch patch v2Splinter Review
Yes, sorry.
Attachment #8669476 - Attachment is obsolete: true
Attachment #8675325 - Flags: review?(rkent)
I'm trying to understand the issues around comment 14. Referring back to the implementation bug 310359, the wording of "All Addresses" was discussed a bit, but the intention is clearly that this is "Any Address".

So a test "If any address is not in my address book, then flag message as HasNewPerson" is intended to work, with the code AllAddresses,IsntInAb.

For that context, you need to skip any blank values, not consider them as "Not In My Addressbook" and therefore causing the search term to fire. This is particularly true for the case of BCC, as in almost all cases it will be blank, and you don't that to be considered an address that is not in the address book.

So the situation is a little more complex than comment 10 admits.

I THINK my reasoning is correct, but I'm having a hard time visualizing all of the possibilities.

In any case, I think it would be better to do nothing than to break an existing use case. I think what I am concluding is that we need special handling of the All Addresses IsntInAB case.

Does that make sense?
Comment on attachment 8675325 [details] [diff] [review]
patch v2

I have not heard an answer to my concerns, so I am removing the review request until then.
Attachment #8675325 - Flags: review?(rkent)
It seems a patch has been provided but it was awaiting a review which didn't come through. Could you guys confirm if there is any activity pending on this ticket?
Flags: needinfo?(vseerror)
Flags: needinfo?(rkent)
ref comment 10 and comment 17
Flags: needinfo?(vseerror)
Flags: needinfo?(rkent)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)
mkmelin, acelists, could you please provide an update please. Thanks.
I read this a few times, and I think Kent's completely  in comment 17. We need special handling of the All Addresses IsntInAB case.
Flags: needinfo?(mkmelin+mozilla)
completely correct
You need to log in before you can comment on or make changes to this bug.