Apologies if this is a duplicate issue, I've not found anything in search ...
If you add arbitrary amounts of whitespace to the beginning or end of your email address at registration, you can bypass the duplicate email address validation. email addresses are stored trim()med in users.mail, but untrimmed in users.init (useful for comparison). The validation does a LIKE comparison of users.mail against the untrimmed value from the form, which is case-insensitive, but does not ignore the whitespace. Simple solution would just be to do the same trim in validation that is done just before insertion into the DB.
Steps to reproduce:
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | user-mail-duplicates-test.patch | 2.97 KB | bleen |
| #15 | user-mail-duplicates-test.patch | 2.98 KB | alan evans |
| #2 | user-mail-duplicates.patch | 1.13 KB | alan evans |
Comments
Comment #1
alan evans commentedI think this could be considered major, as the system assumes that a user account as identified by users.mail (as well as users.name) should be unique (this should probably not be enforced in MySQL though, to allow modules to change form behaviour if desired).
Comment #2
alan evans commentedIf it's agreed that this is a problem and not by design, a simple solution would be the attached patch.
Comment #3
alan evans commentedComment #4
bleen commentedif we are going to consider ' foo@bar.com' a duplicate of 'foo@bar.com' (and I'm not convinced we necessarily need to) ... then this patch does not address the opposite situation.
what happens if ' foo@bar.com' already exists and we try and register with 'foo@bar.com'
Comment #7
alan evans commentedThis doesn't cover the case of existing email address ' foo@bar.com' because that can't exist in users.mail under normal circumstances (ie - if no module overrides core behaviour in this regard). user_save() trims mail before inserting it, so in effect what happens is we alter the e-mail address on insert, but validate for duplicates against an address which has not been altered in the same way (trim()), which doesn't really make sense. Given that we trim on insert, the validation really should trim as well in order to be validating against the known state of users.mail.
Further to the point of "so long as no modules override core behaviour" ... it would be equally possible for modules to allow duplicate email addresses by design, so I'd discount that consideration on the grounds that we can't handle every possible thrid party behaviour.
Comment #8
alan evans commented... Additionally, the idea of validating against the possible existence of /\s*foo@bar.com\s*/ from the form input 'foo@bar.com' is likely not possible to search efficiently in SQL, as it won't be able to use any index on the unknown-length whitespace prefix - you'd end up doing a table scan with a regex, which isn't going to be pretty.
This will be almost certainly part of the reasoning behind trimming on insert in the first place.
Comment #9
alan evans commented... Additionally, the idea of validating against the possible existence of /\s*foo@bar.com\s*/ from the form input 'foo@bar.com' is likely not possible to search efficiently in SQL, as it won't be able to use any index for the unknown-length whitespace prefix - you'd end up doing a table scan with a regex, which isn't going to be pretty.
This will be almost certainly part of the reasoning behind trimming on insert in the first place.
Comment #10
alan evans commentedComment #11
bleen commentedgiven this, I think the patch in #2 is the correct approach.
Comment #12
webchickWe should add a test for this to make sure we don't hit this again.
Comment #13
alan evans commentedWill have a test ready shortly ...
Comment #14
Tor Arne Thune commentedI can confirm this happens on the latest HEAD.
After the patch is applied, I am no longer able to register a user with an email address identical to an already created user account, using the whitespace trick.
Comment #15
alan evans commentedThis should do the trick - added a test for exact duplicate emails as well as those which are duplicates but for extra whitespace. Tests pass on my local test install. Checked that in the pre-patched state, the duplicates-with-whitespace test fails.
Comment #16
bleen commentedsame patch as #15 w/o whitespace issue
Comment #17
bfroehle commentedI don't like the solution of adding
trimeverywhere... Inevitably you'll miss a spot (like we did here) and it'll lead to the wrong behavior. Instead, I think we should fix the underlying problem which is in #61856: In user.module, trim() user-submitted email address before validation --- namely just edit the user's submission to be trim'ed immediately after the form is submitted.Tests are commendable, and should be in the final patch.
Comment #18
abhigupta commentedWe should be trimming all single line text form field values by default. I can't think of a scenario where a user needs to include leading or trailing spaces in form input.
Comment #19
Eranga commentedI tried this issue to fix using the patch above. I think It's a good simple solution for this issue. just one line has been changed.
Comment #21
bfroehle commentedI'm loathe to do this, since there is some valuable discussion in here, especially regarding tests, but I'm going to mark this as a duplicate of the much older #61856: In user.module, trim() user-submitted email address before validation where this issue of trimming user input for e-mail addresses began. The most recent patch in there does include the tests written by Alan Evans, but generally tries to solve the issue in a simpler (and hopefully more robust) way.