One of my users got an "email not valid" message from Drupal when registering. After some trial & error, I figured out that she had accidentally typed a space at the end of the address. (Actually, she pasted it in after copying it from a text file.) The error message doesn't give a hint as to what's wrong with the address, so her assumption was that Drupal was broken.
Furthermore, this being html, the extra space is truncated by the browser, so when she copied and pasted the email error message and emailed it to me, the extra space was no longer there. (To see what I mean, try copying and pasting the line below into a text editor. I intentionally added extra spaces after user@example.com. View source to see them.)
The email address user@example.com is not valid.
I made a small change to user.module that I'd like to see added to the core. I added the following to user_validate_mail(), making it the first line before any validation checks.
$mail=trim($mail);
Reasoning:
- Because the error message is not specific, the average user may not understand that they've added an extra space, or that an extra space matters
- This will not have an unintended impact down the line. A user is unlikely to also enter a space when, for example, they are requesting a password reset.
Comment | File | Size | Author |
---|---|---|---|
#40 | trim_user_mail-61856-d6-40.patch | 443 bytes | Albert Volkman |
#37 | 61856-37-trim-user-mail.patch | 3.96 KB | bfroehle |
#29 | 61856-29-trim-email-address.patch | 3.54 KB | bfroehle |
#27 | 61856-27-email-trim.patch | 3.57 KB | bfroehle |
#22 | 61856_email_trim.patch | 1.23 KB | andypost |
Comments
Comment #1
elv CreditAttribution: elv commentedThis ooold issue was filed in user experience... It's still valid though, I just tried in Drupal 6 beta 2 and the end space is not trimmed.
Comment #2
darknessEnlightened CreditAttribution: darknessEnlightened commentedAdding this code to the
user_validate_mail()
seems to me to be rather a bug fixing solution than to solve the underlying problem:The problem is that some user input is not
trim()
ed before further processing. I'm sure that there are other places where problems like this hide.Comment #3
Panchorather buggish, I guess
Comment #4
blakehall CreditAttribution: blakehall commentedA change to user_validate_mail() (only) is less than ideal, since it doesn't actually trim the value that gets to the database.
There are two changes in the attached patch.
There's probably a better way of doing the save, but this was the first I could figure out ;)
Comment #5
PanchoThis is a little more straight-forward. Tested. Works.
Comment #6
blakehall CreditAttribution: blakehall commentedWorks for me too.
Comment #7
Jody LynnLet's fix it in HEAD first.
Comment #8
jredding CreditAttribution: jredding commentedUpdated for head
modifications to user_save to trim excess whitespace, also modification to user_validate_email
Comment #9
jredding CreditAttribution: jredding commentedComment #10
jredding CreditAttribution: jredding commentedComment #12
jredding CreditAttribution: jredding commentedComment #13
jredding CreditAttribution: jredding commentedComment #14
tobiasbWorks for me! 1+
Comment #15
j.somers CreditAttribution: j.somers commentedSmall, applied successful and I understand the need for it, did something alike on one of my sites.
Comment #16
cburschkaGood idea, indeed.
This still looks ready, once testbot has run the retest.
Comment #17
webchickI was going to ask if this could be done in just one place, but you need it in both for programmatically created users, I think.
Committed to HEAD. Moving to 6.x for consideration.
Comment #18
andypostHere the first patch for D6
Suppose much better to trim all string user fields before save (mail only one of the values)
Comment #19
andypostAdd tag
Comment #20
tobiasb1+
Comment #21
Gábor HojtsyLet's do a straight backport, or if trimming all values is deemed better, we should do that in D7 as well.
Comment #22
andypostGoing to D7 again:
1) user_validate_mail() should not trim data before check - this should happen in code
2) added trim email in user_account_form_validate - users can paste emails with spaces
3) added trim email before updating existing account
Comment #24
SkySy CreditAttribution: SkySy commented#22: 61856_email_trim.patch queued for re-testing.
Comment #26
bfroehle CreditAttribution: bfroehle commentedThis issue just reared it's head in the form of #994612: Duplicate e-mail addresses accepted during registration.
Comment #27
bfroehle CreditAttribution: bfroehle commentedI agree with andypost in #22.
user_validate_mail
should check for valid email address --- period. This means saying that an email address with leading or trailing spaces should be invalid.user_form_validate
alter the form submission to trim the e-mail address, to prevent issues with forgetting trim in some comparison check.If the final point is objectionable, then we could do
$mail = trim($form_state['values']['mail'])
and only work with $mail in user_form_validate.Attached is a patch which does these things, and includes tests from #994612-16: Duplicate e-mail addresses accepted during registration written by Alan Evans who deserves credit for them.
Comment #28
bfroehle CreditAttribution: bfroehle commentedShould be:
or something similar.
Powered by Dreditor.
Comment #29
bfroehle CreditAttribution: bfroehle commentedIssue Summary:
The user registration form attempts to ensure users have unique, valid e-mail addresses. Since we want to consider
'test@localhost'
and' test@localhost '
as being the same e-mail address, we (sometimes) trim the user submitted e-mail address before working with it. The existing D7 code trim's the e-mail address in the following places:user_save()
but only if it is a new user.user_validate_mail()
.However, this allows for duplicate e-mail addresses to get registered. For example, if a user first registers
'test@localhost'
and then registers' test@localhost '
the system will approve the second e-mail address as being valid (since we trim the whitespace inuser_validate_mail()
) and not a duplicate (since the whitespace differs when the database comparison is being done).Also, since the trim function is only run for new users, any user can edit his or her profile to add whitespace around their e-mail address after the account is registered.
The appropriate fix, which is in the attached patch, is to immediately trim the user supplied e-mail in
user_account_form_validate()
so that the database check anduser_validate_mail()
both receive a trimmed e-mail address. The patch then remove the trim call inuser_validate_mail()
. Lastly, the patch moves the trim call to earlier inuser_save()
so it gets executed for both new and updated users.Credit: some code by andy post & testing routines by Alan Evans.
Comment #30
bfroehle CreditAttribution: bfroehle commentedComment #31
Jody LynnGreat. It looks like you've gotten to the bottom of this one.
Comment #32
Dries CreditAttribution: Dries commentedI don't really understand this comment. Why and when would it need to be easily compared?
Powered by Dreditor.
Comment #33
bfroehle CreditAttribution: bfroehle commentedAs is, a user could register an email like 'test@localhost', then update their email to ' test@localhost'. And then could re-register using the same original email of 'test@localhost'.
One could do a more expensive comparison in SQL to check for equality with possible white space on either side of the string -- that's the 'easily' compared --- but as is we only check for (case insensitive) equality.
Comment #34
Alan Evans CreditAttribution: Alan Evans commentedMany thanks for pulling this together, bfroehle.
Comment #35
dawehnerComment #36
Dries CreditAttribution: Dries commentedI reviewed the patch and everything looks good.
The code comment that I referred to in #32 is confusing for people that don't have the context of this issue. It could be improved -- or even be removed.
However, this patch no longer applies against 8.x. It will need to be re-rolled.
Comment #37
bfroehle CreditAttribution: bfroehle commentedRevisions, as requested by Dries in #36, namely re-rolled (to account for #817482: user_save issues a warning when saving without changing mail attribute) and the confusing comment removed.
Comment #38
Dries CreditAttribution: Dries commentedThanks for incorporating the feedback. Committed to 7.x and 8.x. Thanks!
Comment #39
webchickThis could go into 6.x too.
Comment #40
Albert Volkman CreditAttribution: Albert Volkman commentedD6 backport.
Comment #41
lightweight CreditAttribution: lightweight commented22: 61856_email_trim.patch queued for re-testing.
Comment #43
thedavidmeister CreditAttribution: thedavidmeister commentedThe patch in #40 did not work for me on simplytest.me http://grab.by/wS4a
Do we need to modify
_user_edit_validate()
too?