0

In my website I have users registering with the email as the username

If I try to manually add a new user and I use a username (which is a mail) that already exists, WP complains and tells me that that username is already registered

If I do this programmatically through the code below, I handle this and prevent the registration

However, there's a user that I guess has clicked repeated times to the form submit button, has managed to get the exact same user duplicated in my WP user database

The question is, how is this possible?

And how can I prevent this?


From the comments, rephrasing the question in other words: If I want to replicate this, how can I do this?

So far the only explanation that makes sense is that I can only replicate it by recreating a race condition (frontend, postman, curl, whatever)

And I'm unsure about if there is a way to prevent it (it'd be like preventing a DOS attack I guess)


The code I'm using is this:

    public function register_user($request = null)
    {
        $response = array();
        $parameters = $request->get_json_params();
        $email = sanitize_text_field($parameters['email']);
        $password = sanitize_text_field($parameters['password']);
        $domain = $_SERVER['HTTP_REFERER'];
        $uri = $_SERVER['REQUEST_URI'];

        $role = 'subscriber';
        $error = new WP_Error();

        if (empty($email)) {
            $error->add(...);
            return $error;
        }
        if (!is_email($email)) {
            $error->add(...);
            return $error;
        }
        if (empty($password)) {
            $error->add(...);
            return $error;
        }

        $user_id = username_exists($email);
        if (!$user_id && email_exists($email) == false) {
            $user_id = wp_create_user($email, $password, $email);

            if (!is_wp_error($user_id)) {
                $user = get_user_by('id', $user_id);
                $user->set_role($role);
                $response['code'] = 200;
                $response['message'] = __("User '" . $user_id . "' ok ", "...");
            } else {
                return $user_id;
            }
        } else if ($user_id) {
            $error->add(...);
            return $error;
        }

        return new WP_REST_Response($response, 200);
    }

I just cannot get how this has happened

EDIT from the comments, front-end code included

        <button type="text" value="" onClick={register}>
          <div>...</div>
        </button>
  const register = async e => {
    e.stopPropagation()
    if (registering === 'working') return
    setRegistering('working')

    let id = input_id.current.value.trim()
    let pass = input_pass.current.value
    let pass2 = input_pass2.current.value

    const success = pass === pass2 ? await auth.signup(id, pass, feedback) : 'not_equal_passwords'
    setRegistering(success)
    if (success === 'yes') navigate('/welcome')
  }
  const signup = async (email, password) => {
    const register = `${domain}${website}/wp-json/.../register`
    const request = get_request('POST')
    const expression = /^([a-zA-Z0-9_.-])+@(([a-zA-Z0-9-])+.)+([a-zA-Z0-9]{2,4})+$/

    let success = password ? (email ? '' : 'no_id') : 'no_pass'
    success = expression.test(String(email).toLowerCase()) ? '' : 'no_mail'

    if (!success) {
      request.body = JSON.stringify({ email: email, password: password })
      const response = await get_response(register, request, 5, 4000, 'json')
      success = response.code === 'user_exist' ? 'user_exist' : 'yes'
    }

    return success
  }
const get_response = async (url, request, times, time = 2000, json) => {
  const rq = request || get_request('GET')
  let response = await Promise.race([fetch(url, rq), wait(time)])
  let counter = 0

  for (const _ of [...Array(times)]) {
    if (counter >= times || response !== 'timed') break
    response = await Promise.race([fetch(url, request), wait(time)])
    counter++
  }

  if (response === 'timed') return
  return json ? await response.json() : response
}
12
  • Have you checked the signup page? Are you sure it's a duplicated user and not just an existing user? Also I see you're using emails as username, did you know the login page also accepts the email as the username? This could cause problems with people changing their email addresses
    – Tom J Nowell
    Dec 19, 2019 at 14:24
  • No, it's 2 identical users, and the signup page is using the code above (only that code, WP is working headless here). Indeed I have 3 related users, one has a different email, I can see the user has had problems in subscribing, but as I say 2 users are completely identical, how has it been possible?
    – GWorking
    Dec 19, 2019 at 16:45
  • Is it possible that if the server receives two consecutive requests, they fail in detecting each other's action and proceed to the subscribing as usual? This is the only explanation I can think of, but isn't that something that is probably already taken care of?
    – GWorking
    Dec 19, 2019 at 16:51
  • It appears you have a race condition, and yes, double clicking instead of single clicking a link can sometimes fire off 2 requests, with the browser interpreting the second click as a new link clicking event. However there is no signup code logic in your question, just a very censored and restricted code snippet without context. I also question the need to check both the username and the email
    – Tom J Nowell
    Dec 19, 2019 at 22:38
  • I also hope you have dedicated code to either prevent email changing, or, change the username when the email is changed, or you're going to have some very strange problems should any email ever get reused
    – Tom J Nowell
    Dec 19, 2019 at 22:38

1 Answer 1

2

Yes, WordPress checks for duplicate emails internally, but not duplicate usernames

To test this I ran this several times via wp shell:

wp_create_user( 'test', 'password', 'test@test.test' );

The result on the second attempt was:

=> class WP_Error#1962 (2) {
  public $errors =>
  array(1) {
    'existing_user_email' =>
    array(1) {
      [0] =>
      string(42) "Sorry, that email address is already used!"
    }
  }
  public $error_data =>
  array(0) {
  }
}

When I tried to register a different email with the same username:

wp> wp_create_user( 'test', 'password', 'testing@test.test' );
=> class WP_Error#1981 (2) {
  public $errors =>
  array(1) {
    'existing_user_login' =>
    array(1) {
      [0] =>
      string(36) "Sorry, that username already exists!"
    }
  }
  public $error_data =>
  array(0) {
  }
}

I also checked the code and found this in wp_insert_user:

https://github.com/WordPress/WordPress/blob/f93ee2ca76164bcae721e4730c92ee1455fa1dd9/wp-includes/user.php#L1645-L1650

    /*
     * If there is no update, just check for `email_exists`. If there is an update,
     * check if current email and new email are the same, or not, and check `email_exists`
     * accordingly.
     */
    if ( ( ! $update || ( ! empty( $old_user_data ) && 0 !== strcasecmp( $user_email, $old_user_data->user_email ) ) )
        && ! defined( 'WP_IMPORTING' )
        && email_exists( $user_email )
    ) {
        return new WP_Error( 'existing_user_email', __( 'Sorry, that email address is already used!' ) );
    }

So What's Causing This?

Right now there's not enough information to diagnose this, and it doesn't help that your usernames are emails as it makes it easy to overlook things or treat them as if they're always the same.

My best guess at the moment is a race condition, which is more likely if you're using load balancing.

Eitherway, even if it didn't create multiple users, you still need to handle users double clicking or triple clicking on the signup button else they'll get an error that the email already exists ( because they're getting the result of the second request, and the email got used on the first )

A More Reliable Way to Check The Users

Looking at your code:

        $user_id = username_exists($email);
        if (!$user_id && email_exists($email) == false) {
            $user_id = wp_create_user($email, $password, $email);
        } else if ($user_id) {
            ... 'ERROR in Registration, user email already exists ' ...
            return $error;
        }

wp_create_user already does these checks, and returns either a user id, or an error object. In this situation:

  • You've doubled the number of checks
  • If user creation fails, your code never discovers this as there is no check on the result

So, lets simplify it:

$user_id = wp_create_user( $email, $password, $email );
if ( is_wp_error( $user_id ) ) {
    // it failed!
    ....
    return $error;
}

Some Other Things to keep in mind:

  • Don't use emails as usernames, auto-generate them and use a filter to display the email when the username is shown
  • Sanitize!!! Because you only showed a constrained snippet, it's possible that sanitising or the lack of it is involved in this, but as you won't share the surrounding code it's impossible to tell. It could be that your email had trailing spaces and other characters around it
  • You don't need to use emails as usernames for that to work. WordPress will accept both the username, and the email when logging in, you don't need to know your username if you're logging in if you know the email
  • You don't need to check if the email exists, wp_insert_user already does this internally
  • Your signup form may be more reliable if it ran via a PHP form submission rather than an AJAX request, eitherway you can't make this atomic but you can try to reduce the number of steps and parallel requests
  • Disable your signup button on the first click for 5 seconds, perhaps swap it for a progress spinner
  • wp_create_user calls wp_insert_user, so use the latter. WP has a lot of "middle men" functions that try to be helpful, but usually they have subtle behaviours for backwards compat reasons that are more annoying than helpful. Cutting out these inbetweens simplifies things and gives fewer steps to debug
  • Put a nonce on your signup endpoint, if you haven't then you're wide open to someone creating thousands of users per minute with a quick bash script and Curl
7
  • (I've included the full php function) could you develop the concept of race condition? Also, my decision to use emails as usernames is to reduce friction, what's wrong in using the email both as the email and as the string for the username?
    – GWorking
    Dec 20, 2019 at 12:54
  • Also for completion, I will delete checking redundancy and call wp_insert_user as well, and take a look at the nonce since that was my concern, I am indeed wide open to this type of attack, thanks for these tips!
    – GWorking
    Dec 20, 2019 at 12:57
  • Not sure if I can use a nounce for a wp non-user if using a headless WP with the front-end completely out of the server, AFAIS it is like I cannot, do you have any input / suggestion?
    – GWorking
    Dec 20, 2019 at 13:21
  • A race condition isn't a WP term, but a general term, it's when 2 things try to do something at the same time, but there are multiple steps to what they're doing, e.g. if you check if it's ok to create the user, and it says yes, then I check and it still says yes, then you create the user, now it's no longer ok for me, but when I checked it said yes because you hadn't created it yes, so even though I got the green light to go ahead, my step fails. Race condition. Race conditions can cause data corruption and strange behaviour, that's why we try to make steps "atomic"
    – Tom J Nowell
    Dec 20, 2019 at 14:04
  • You can also rate limit IPs poking the API, similar to the limit logins plugin, but it would be better to do that at the server level rather than the WP level
    – Tom J Nowell
    Dec 20, 2019 at 14:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.