65313 - Failure to detect some invalid email addresses

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect, P3)

Description

[Martin Baulig]

When you create a new Bugzilla account, it normally checks
whether the email address you entered is valid. This check
fails to detect some invalid email addresses.

The problem is the default regexp in defparams.pl is
q:^[^@, ]*@[^@, ]*\\.[^@, ]*$:

This regexp doesn't catch embedded newlines, tab characters,
backslashes and other weird stuff. It'd be better to explicitly
list all characters which are allowed in an email address there:
q:^[\\w\\d_\\.\\+\\-]+@[\\w\\d_\\.\\+\\-]+\\.[\\w\\d_\\.\\+\\-]+$:
or something like this.

Note that * after a character class allows zero characters, so that
it won't catch stuff like "@.", this should better be a + to
require at least one character for the set.

Martin

Comment 1

[Matthew Tuck [:CodeMachine]]

We think someone created an account on an installation with an embedded newline
and this caused an error in sanitycheck.cgi, however we haven't found a browser
we can reliably reproduce this in.

If this causes an error in sanitycheck.cgi we should check that regardless of
emailregexp (or is the check just confirming all the addresses conform to
emailregexp?).

Should be nice and easy to change the default for new installations.  For old
installations, we might want to consider asking if they want to change it in
checksetup.pl or whatever.

Comment 2

[Andreas Franke (gone)]

-> Bugzilla product, trying User Accounts component, reassigning.

Comment 3

[Gervase Markham [:gerv]]

q:^[\\w\\d_\\.\\+\\-]+@[\\w\\d_\\.\\+\\-]+\\.[\\w\\d_\\.\\+\\-]+$:

My regexps might not be up to much, but surely:

q:^[\w\.\+\-]+@[\w\.\-]+\.[\w\.\-]+$:

_ and \d are included in \w, and we don't need to escape the backslashes. + is
not allowed in domain names.

Ccing others for regexp fu.

Gerv

Comment 4

[Jacob Steenhagen]

q:^[\w\.\+\-]+@[\w\.\-]+\.[\w\-]+$:

No need for the . in the last set as with a domain name such 
as "chariot.net.au", "chariot.net" will be mached in the first charset after 
the @ and "au" will bet matched in the last one.  Also, w/the extra dot, it 
will match "chariot.net." just fine, even though that's far from valid.

Comment 5

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 6

[Gervase Markham [:gerv]]

The current default is:
^[^@]+@[^@]+\.[^@]+$

We should be using:
^[\w\.\+\-=]+@[\w\.\-]+\.[\w\-]+$
(as in previous comment, but with "=" sign in username.)

Gerv

Comment 7

[Vlad Dascalu]

Attachment: Version 1 (Gerv's proposed fix)

Tested. Appears like expected on the editparams.cgi page. Succesfully created
accounts with it.

Comment 8

[Gervase Markham [:gerv]]

Comment on attachment 142183 [details] [diff] [review]
Version 1 (Gerv's proposed fix)

The param has associated text, and is documented somewhere, I think. r=gerv if
you check for, and possibly update, any associated docs.

Gerv

Comment 9

[Vlad Dascalu]

Thanks.

There is first the text that appears in editparams.cgi. It talks about the 
regexp as trying to match any qualified email address. So we don't need to 
change that one.

I found some references to it in the docs (by searching for "email" 
and "regexp"), but nothing specific. None of them seemed in need of updates.

Comment 10

[Vlad Dascalu]

Attachment: Version 2

It seems I forgot to modify the value in checksetup.pl; we could end up with it
been validated by checksetup.pl but rejected in the web interface, so it's best
to have the same regexp in both places.

Comment 11

[Vlad Dascalu]

Comment on attachment 142211 [details] [diff] [review]
Version 2

You could rubber-stamp this one too for checkin. :)

Comment 12

[Gervase Markham [:gerv]]

Comment on attachment 142211 [details] [diff] [review]
Version 2

r=gerv. No docs, then? :-)

Gerv

Comment 13

[Vlad Dascalu]

Nope. Forgot to CC you when I posted comment #9. The relevant stuff in regard 
to the docs is:

> I found some references to it in the docs (by searching for "email" 
> and "regexp"), but nothing specific. None of them seemed in need of updates.

Thanks! :-)

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.264; previous revision: 1.263
done
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.125; previous revision: 1.124
done