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
}
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
}
- 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 ♦ Commented 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 Commented 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 Commented 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 ♦ Commented 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 ♦ Commented Dec 19, 2019 at 22:38
1 Answer
Reset to default 2Yes, 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', '[email protected]' );
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', '[email protected]' );
=> 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/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_useralready 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_usercallswp_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