Closed Bug 65313 Opened 24 years ago Closed 20 years ago

Failure to detect some invalid email addresses

Categories

(Bugzilla :: User Accounts, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: baulig, Assigned: goobix)

Details

Attachments

(1 file, 1 obsolete file)

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
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.
Target Milestone: --- → Bugzilla 2.16
Priority: -- → P3
-> Bugzilla product, trying User Accounts component, reassigning.
Assignee: tara → myk
Component: Bugzilla → User Accounts
Product: Webtools → Bugzilla
Version: other → unspecified
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


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.
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.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
The current default is:
^[^@]+@[^@]+\.[^@]+$

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

Gerv

Attached patch Version 1 (Gerv's proposed fix) (obsolete) — Splinter Review
Tested. Appears like expected on the editparams.cgi page. Succesfully created
accounts with it.
Assignee: myk → vlad
Status: NEW → ASSIGNED
Attachment #142183 - Flags: review?(gerv)
Hardware: PC → All
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
Attachment #142183 - Flags: review?(gerv) → review+
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.
Flags: approval?
Flags: approval? → approval+
Attached patch Version 2Splinter Review
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.
Attachment #142183 - Attachment is obsolete: true
Comment on attachment 142211 [details] [diff] [review]
Version 2

You could rubber-stamp this one too for checkin. :)
Attachment #142211 - Flags: review?(gerv)
Comment on attachment 142211 [details] [diff] [review]
Version 2

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

Gerv
Attachment #142211 - Flags: review?(gerv) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Summary: Fails to detect some invalid email address when creating accounts → Failure to detect some invalid email addresses
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: