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:

  1. 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
  2. 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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

elv’s picture

Project: » Drupal core
Version: » 6.0-beta2
Component: usability » user system

This 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.

darknessEnlightened’s picture

Adding 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.

Pancho’s picture

Version: 6.0-beta2 » 6.x-dev
Category: feature » bug

rather buggish, I guess

blakehall’s picture

Status: Active » Needs review
FileSize
1.15 KB

A 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.

  • the aforementioned trim on user_validate_mail()
  • a change to user_save to also run trim on the mail value before it gets to the database

There's probably a better way of doing the save, but this was the first I could figure out ;)

Pancho’s picture

FileSize
1.05 KB

This is a little more straight-forward. Tested. Works.

blakehall’s picture

Works for me too.

Jody Lynn’s picture

Version: 6.x-dev » 7.x-dev

Let's fix it in HEAD first.

jredding’s picture

FileSize
4.08 KB

Updated for head
modifications to user_save to trim excess whitespace, also modification to user_validate_email

jredding’s picture

Status: Needs review » Active
jredding’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

jredding’s picture

FileSize
852 bytes
jredding’s picture

Status: Needs work » Needs review
tobiasb’s picture

Works for me! 1+

j.somers’s picture

Status: Needs review » Reviewed & tested by the community

Small, applied successful and I understand the need for it, did something alike on one of my sites.

cburschka’s picture

Good idea, indeed.

This still looks ready, once testbot has run the retest.

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I 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.

andypost’s picture

Status: Patch (to be ported) » Needs review
FileSize
997 bytes

Here the first patch for D6

Suppose much better to trim all string user fields before save (mail only one of the values)

andypost’s picture

Issue tags: +Needs backport to D6

Add tag

tobiasb’s picture

Status: Needs review » Reviewed & tested by the community

1+

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Let's do a straight backport, or if trimming all values is deemed better, we should do that in D7 as well.

andypost’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
1.23 KB

Going 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

Status: Needs review » Needs work
Issue tags: -Needs backport to D6

The last submitted patch failed testing.

SkySy’s picture

Status: Needs work » Needs review

#22: 61856_email_trim.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs backport to D6

The last submitted patch, 61856_email_trim.patch, failed testing.

bfroehle’s picture

This issue just reared it's head in the form of #994612: Duplicate e-mail addresses accepted during registration.

bfroehle’s picture

Status: Needs work » Needs review
FileSize
3.57 KB

I agree with andypost in #22.

  1. 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.
  2. e-mail should be trimmed always in user_save, not just when a new account is created. This is solved by moving up the $edit['mail'] = trim($edit['mail']) to earlier in the code flow (before the 'new account' vs. 'update existing account' switch).
  3. have 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.

bfroehle’s picture

Status: Needs review » Needs work
+++ modules/user/user.module
@@ -1203,6 +1208,10 @@ function user_account_form_validate($form, &$form_state) {
+    $form_state['values']['mail'] = trim($form_state['values']['mail']);

Should be:

$mail = trim($form_state['values']['mail']);
form_set_value($form['mail'], $mail, $form_state);

or something similar.

Powered by Dreditor.

bfroehle’s picture

Status: Needs review » Needs work
FileSize
3.54 KB

Issue 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:

  1. In user_save() but only if it is a new user.
  2. In 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 in user_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 and user_validate_mail() both receive a trimmed e-mail address. The patch then remove the trim call in user_validate_mail(). Lastly, the patch moves the trim call to earlier in user_save() so it gets executed for both new and updated users.

Credit: some code by andy post & testing routines by Alan Evans.

bfroehle’s picture

Status: Needs work » Needs review
Jody Lynn’s picture

Status: Needs work » Reviewed & tested by the community

Great. It looks like you've gotten to the bottom of this one.

Dries’s picture

+++ modules/user/user.module
@@ -420,6 +420,12 @@ function user_save($account, $edit = array(), $category = 'account') {
 
+    // Trim e-mail addresses before they are saved so they may be easily
+    // compared to new user registrations.

I don't really understand this comment. Why and when would it need to be easily compared?

Powered by Dreditor.

bfroehle’s picture

As 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.

Alan Evans’s picture

Many thanks for pulling this together, bfroehle.

dawehner’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
Dries’s picture

Status: Reviewed & tested by the community » Needs work

I 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.

bfroehle’s picture

Status: Needs work » Needs review
FileSize
3.96 KB

Revisions, 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.

Dries’s picture

Status: Needs review » Fixed

Thanks for incorporating the feedback. Committed to 7.x and 8.x. Thanks!

webchick’s picture

Version: 8.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)
Issue tags: -Needs backport to D7

This could go into 6.x too.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
443 bytes

D6 backport.

lightweight’s picture

22: 61856_email_trim.patch queued for re-testing.

The last submitted patch, 22: 61856_email_trim.patch, failed testing.

thedavidmeister’s picture

Issue summary: View changes
Status: Needs review » Needs work

The patch in #40 did not work for me on simplytest.me http://grab.by/wS4a

Do we need to modify _user_edit_validate() too?

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.