[2.x] Prevent race condition in account generation code by adding uid to name

Created on 4 April 2009, over 15 years ago
Updated 23 October 2023, about 1 year ago

In email_registration_user, if the generated username already exists, the code will issue a query to find the last numeric sequence number in use with that username, then add one to it and change the username to that. But because Drupal is generally run in a multi-process or multi-thread environment and the check and the update do not happen atomically, it is possible for another user to create an account with the same generated name, which would be assigned the same sequence number. The sequence is like this:

  1. User 1 creates account 'racer@example.com'
  2. User 2 creates account 'racer@aol.com' at about the same time
  3. email_registration_user runs for User 1, finds that username racer already exists, chooses sequence number 1
  4. email_registration_user runs for User 2, finds that username racer already exists, chooses sequence number 1
  5. email_registration_user runs for User 1, changes username to racer_1
  6. email_registration_user runs for User 2, changes username to racer_1. SQL statement fails because of duplicate username, and user gets a weird SQL error in their browser

My solution is just to always append their UID. We already know it is unique. This also simplifies the query, and make it work on Postgres (see http://drupal.org/node/421078).

Patch is attached.

πŸ› Bug report
Status

Needs work

Version

2.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States scottgifford

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡©πŸ‡ͺGermany Grevil

    This can be still reproduced in 2.x, in "hook_ENTITY_TYPE_presave()" through commenting out $new_name = email_registration_unique_username($new_name, (int) $account->id()); in line 58 in the email_registration.module.

    This is quite the edge case, but should be solved nonetheless. I am not a big fan of adding a UUID at the end of the username. Could this eventually solved by executing an SQL transaction?

  • πŸ‡¬πŸ‡§United Kingdom joachim

    > Why not just replace the @ with an underscore and put the whole email address in. There shouldn't be any collisions at all that way, plus every user name could be computed from their email address without added information.
    > There is a privacy issue with revealing the user's entire email address as their username, if their username is ever visible.

    Alternative idea: instead of appending the uid, or the email address, append a short hash of the username. That way we get something (probably!) unique, but we don't need the uid, and we don't expose the email address.

    See https://roytanck.com/2021/10/17/generating-short-hashes-in-php/ for instance for creating short hashes.

  • πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

    Great idea @joachim! Anyone willing to implement that?

Production build 0.71.5 2024