117297 - Email address added twice in ADD CCs due to case-insensitive query

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect, P1)

Description

[Gilbert Fang]

My bugzilla account is my email address with initial-capitalized.
when I input my email address in the Add CC field , it is all lower-case.
The result is the two addresses (actully one ) are all added in the CCs.
And  I get the notifications twice with the two address seperatly.

Comment 1

[Gervase Markham [:gerv]]

Well, this definitely shouldn't happen:

Email sent to: [e-mail], [e-mail], [e-mail],
[e-mail], [e-mail]
Excluding: [e-mail], [e-mail], [e-mail] 

I think we need to be lower-casing email addresses somewhere in processmail, but
I can't immediately see where would be best.

bbaetz seems to be the local expert here...

Gerv

Comment 2

[Bradley Baetz (:bbaetz)]

We should probably just invert the DBNameToIdAndCheck before adding to the hash
in process_bug, at at least the area round line 667. Or maybe we should do this
in processmail.

/me shrugs. I'll think about it

Eventually processmail really needs to become a sub which can be passed all this
in directly.

Comment 3

[Matthew Tuck [:CodeMachine]]

Why doesn't the CCs table have the relevant uniqueness constraint on IDs?  Do
these two different accounts with different case actually exist?

Comment 4

[Bradley Baetz (:bbaetz)]

It does, which is why gerv is here only once in the cc list. Its probably passed
to -forcecc, or read from the activity log, or something

Comment 5

[Bradley Baetz (:bbaetz)]

A quick hack is just to lowercase @emaillist in ProcessOneBug (and the
forcelist, too). Or this could be done in the alreadyseen bit, I guess, but then
you'd have to change multiple places (I'm not going to ask why we have multiple
places doing this). Comments?

Comment 6

[Dave Miller [:justdave]]

Sorry to push this back, but in the grand scheme of things this is kind of a
minor issue.  If we get a patch in the next couple days we can put it back on 2.16

Comment 7

[Dave Miller [:justdave]]

Changing default owner of Email Notifications component to JayPee, a.k.a.
J. Paul Reed ([e-mail]).  Jake will be offline for a few months.

Comment 8

[Dave Miller [:justdave]]

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

Comment 9

[Dave Miller [:justdave]]

There are some hairy problems with case-sensitivity in email addresses.

domain names (the part after the @) are by definition case-insensitive.

user names (the part before the @) are by definition case-SENSITIVE by the RFC,
but in practice some hosts treat it case-insensitively.

Technically, usernames in two different cases on the same host could be two
separate users.  Then again it might not be.  Some hosts (AOL is a good example)
let you change the case of your email address on the fly whenever you feel like
it, because they don't treat it case-sensitively.  On other hosts (pretty much
anything running a variant of UNIX) the addresses are very much case-sensitive.

So we have a minor security hole here if we don't treat the username portion as
case-sensitive.  On the other hand, how likely is it that you'd have two users
with the same username, but using different cases on the same host?

In either case we should send the email address to processmail in the same case
it was entered when the user signed up, whatever that case is.  We should only
do case-insensitive matches when verifying entered data (CC list,
authenticating, etc), but use it as originally entered any time we display/use it.

Comment 10

[Thomas Stromberg]

This bug bit me today when upgrading our internal  deployment where all of the
2000 imported e-mail accounts have mixed case.

And as far as UNIX goes for e-mail reception, everything I know follows the RFC
(Postfix & Sendmail was just tested.. not to mention Exchange), in being case
insensitive for e-mails.

While it may not be the most elegant, I nominate that we if nothing else just
lowercase the e-mails as they get sent out. If anyone out there has an
RFC-non-complaint mailserver, let them have less e-mail because of it. Do you
have an example of a mailserver which breaks RFC in this manner?

I'll post an ugly hack patch later today to solve this, since this is an ugly
thing for us in production right now. 

Comment 11

[Thomas Stromberg]

Attachment: processmail patch to case-insensitive check who you've mailed.

This patch changes processmail so that it checks case sensitivity in the
following manners:

ProcessOneBug(): When checking to see if it's already seen that user.
filterExcludeList(): When checking to see that it's already seen that user.
filterExcludeList(): When checking to see if the excludes are correct. This
fixes another issue where if a user has a mixed case login to bugzilla, but
logs in with a lowercase equivalent for that session - he will get e-mailed any
bug he enters.

Comment 12

[Bradley Baetz (:bbaetz)]

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

Comment 13

[Jouni Heikniemi]

Comment on attachment 81878 [details] [diff] [review]
processmail patch to case-insensitive check who you've mailed. 

>+	# don't modify the original so it sends out with the right case
>+	# based on who came first.
>+	$checkperson = lc($person);

r=jouni, but convert those tabs into appropriate sequences of spaces.

Comment 14

[timeless]

Comment on attachment 81878 [details] [diff] [review]
processmail patch to case-insensitive check who you've mailed. 

why not use =1 instead of ++? we really do mean =1..
>+        if ( !($AlreadySeen{$checkperson}) ) {
>             push(@allEmail,$person);
>-            $AlreadySeen{$person}++;
>+            $AlreadySeen{$checkperson}++;
>         }

>+	$checkperson = lc($person);
>+        if ( !($alreadySeen{$checkperson}) ) {
>             push(@uniqueResult,$person);
>-            $alreadySeen{$person}++;
>+            $alreadySeen{$checkperson}++;
>         }

Comment 15

[Jouni Heikniemi]

-> [e-mail], the patch author

Justdave: you had some doubts in comment 9. Is this patch an acceptable fix for
you? If so, I'll check this into the trunk.

Comment 16

[Dave Miller [:justdave]]

Comment on attachment 81878 [details] [diff] [review]
processmail patch to case-insensitive check who you've mailed. 

a=justdave

looks good to me.

Comment 17

[Jouni Heikniemi]

Thanks for the patch, Thomas!

Checking in processmail;
/cvsroot/mozilla/webtools/bugzilla/processmail,v  <--  processmail
new revision: 1.84; previous revision: 1.83
done

Comment 18

[Bradley Baetz (:bbaetz)]

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

Comment 19

[Bradley Baetz (:bbaetz)]

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

Comment 20

[Dave Miller [:justdave]]

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

Comment 21

[Dave Miller [:justdave]]

r= justdave for 2.16 branch, low-risk, high-value patch, applies unmodified, and
still works.

Fixed on 2.16 branch: (for 2.16.6)
Checking in processmail;
/cvsroot/mozilla/webtools/bugzilla/Attic/processmail,v  <--  processmail
new revision: 1.81.2.2; previous revision: 1.81.2.1
done

Comment 22

[Matthew Tuck [:CodeMachine]]

Bug #271666 filed about sanity check checking this.