WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#33793 closed defect (bug) (fixed)

Adding a user with a username 52 characters or longer fails, but displays success message.

Reported by: tommarshall Owned by: boonebgorges
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.8.8
Component: Users Keywords: needs-unit-tests has-patch dev-feedback
Focuses: administration Cc:

Description

Steps to reproduce:

  1. Login
  2. Users -> Add New (/wp-admin/user-new.php)
  3. Fill out the form with following data:

Username: Any valid email with 52 characters or more, e.g. abcdefghijklmnopqrstuvwxyzabcdefghijklmn [at] example.com (without the email obfuscation). E-mail: Any Password: Any Repeat Password: Any

  1. Submit form by 'Add New User' button.
  2. WordPress redirects to Users page (/wp-admin/users.php?id=0) with a 'New user created.' success message, but the user is not visible in list, and is not present in wp_users table.

If you repeat the steps specifying a username that is 51 characters or less, then the new user is created correctly, and the success message includes an 'Edit user' link.

I've seen this broken behaviour in:

  • 4.3
  • 4.2.1
  • 3.9
  • 3.8.8

In 3.8.7 adding a user with a username >51 characters (and >60 characters) creates a new user correctly. The username is truncated to the first 60 characters when necessary. (wp_users.user_login is a varchar(60)).

I suspect the reason 51 characters succeeds and 52 does not may be to do with the wp_users.user_nicename field, which is a varchar(50). The add new user operation auto generates the user_nicename value, as a sanitised form of the user_login, stripping the @ in the process. I have not confirmed this though.

Attachments (7)

33792.diff (1.7 KB) - added by utkarshpatel 3 years ago.
Added html max-length 50 for user_login and also added check in backed for length.
33792_2.diff (1.7 KB) - added by utkarshpatel 3 years ago.
change condition to allow 50 chars
33793.diff (3.2 KB) - added by dipesh.kakadiya 3 years ago.
By default username set as user_nicename but in schema both have diffrent size.
33793_1.diff (4.0 KB) - added by dipesh.kakadiya 3 years ago.
change added related to #33820
33793.2.diff (3.7 KB) - added by dipesh.kakadiya 3 years ago.
Added extra check for checking nicename length
33793.3.diff (3.7 KB) - added by dipesh.kakadiya 3 years ago.
extra check added for length of user_nicename in wp_insert_user()
33793.4.diff (858 bytes) - added by ruud@… 3 years ago.
added furthter regression test for allowed new username length

Download all attachments as: .zip

Change History (25)

#1 @utkarshpatel
3 years ago

I can reproduce this error on my end and also getting error

[09-Sep-2015 17:45:58 UTC] PHP Notice:  Trying to get property of non-object in htdocs/wp-includes/pluggable.php on line 1723

I am running 4.3.

@utkarshpatel
3 years ago

Added html max-length 50 for user_login and also added check in backed for length.

#2 @utkarshpatel
3 years ago

We should consider increasing user_nicename length to var_char(60) or should decrease user_login length to var_char(50).

@utkarshpatel
3 years ago

change condition to allow 50 chars

@dipesh.kakadiya
3 years ago

By default username set as user_nicename but in schema both have diffrent size.

#3 @dipesh.kakadiya
3 years ago

  • Keywords has-patch added

#4 @dipesh.kakadiya
3 years ago

#33820 was marked as a duplicate.

@dipesh.kakadiya
3 years ago

change added related to #33820

#5 @dipesh.kakadiya
3 years ago

#33820 was marked as a duplicate.

#6 follow-up: @tommarshall
3 years ago

It feels like it would be better to return to the truncation behaviour present in 3.8.7 if possible, rather than introducing additional validation.

#7 in reply to: ↑ 6 @boonebgorges
3 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch removed
  • Milestone changed from Awaiting Review to Future Release

Replying to tommarshall:

It feels like it would be better to return to the truncation behaviour present in 3.8.7 if possible, rather than introducing additional validation.

The truncation that took place prior to 3.8.8 (and corresponding releases on the 3.9, 4.0, 4.1, and 4.2 branches) was an unintended consequence of shoving too much data into a field. We can't revert directly to that behavior. Moreover, silently truncating user-provided data is never a good idea - it means that a user could attempt to register with one username, but be provided with a different one, without any appropriate feedback.

user_login and user_nicename field lengths have been the same forever [2213]. It's likely that increasing them would introduce complications. If there's a pressing reason to change one or both, let's handle it as a separate enhancement ticket. We should solve the current problem with better validation.

It looks like wp_insert_user() fails silently when 'user_login' is greater than 60 chars, and when 'user_nicename' is greater than 50 chars. We'll need separate validation for each, plus unit tests.

#8 @dipesh.kakadiya
3 years ago

@boonebgorges

currently, length of user_login and user_nicename fields are different and we set user_login as user_nicename if user_nicename is empty.

So we need to change length of user_nicename field into schema file.

@dipesh.kakadiya
3 years ago

Added extra check for checking nicename length

#9 @dipesh.kakadiya
3 years ago

  • Keywords has-patch added; needs-patch removed

@boonebgorges

As your suggestion extra check added for length of user_nicename in wp_insert_user().

Last edited 3 years ago by dipesh.kakadiya (previous) (diff)

@dipesh.kakadiya
3 years ago

extra check added for length of user_nicename in wp_insert_user()

#10 @boonebgorges
3 years ago

currently, length of user_login and user_nicename fields are different and we set user_login as user_nicename if user_nicename is empty. So we need to change length of user_nicename field into schema file.

As noted above, there may be repercussions to changing the length of the field. I'd rather not do it. It's true that user_nicename is usually created from the user_login field, but let's enforce the truncation in code, rather than extending the field length.

#11 @boonebgorges
3 years ago

  • Milestone changed from Future Release to 4.4
  • Owner set to boonebgorges
  • Status changed from new to assigned

#12 @boonebgorges
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 34218:

Improve validation of user_login and user_nicename length.

The user_login field only allows 60 characters, and user_nicename allows

  1. However, there are no protections in the interface, and few in the code,

that prevent the creation of users with values in excess of these limits. Prior
to recent changes in $wpdb, users were generally created anyway, MySQL
having performed the necessary truncation. More recently, the INSERTs and
UPDATEs simply fail, with no real feedback on the nature of the failure.

This changeset addresses the issue in a number of ways:

  • On the user-new.php and network/user-new.php panels, don't allow input in excess of the maximum field length.
  • In wp_insert_user(), throw an error if the value provided for 'user_login' or 'user_nicename' exceeds the maximum field length.
  • In wp_insert_user(), when using 'user_login' to generate a default value for 'user_nicename', ensure that the nicename is properly truncated, even when suffixed for uniqueness (username-2, etc).

Props dipesh.kakadiya, utkarshpatel, tommarshall, boonebgorges.
Fixes #33793.

#13 @ruud@…
3 years ago

  • Keywords dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Hi dipesh.kakadiya, utkarshpatel, tommarshall, boonebgorges, thanks for reporting this bug and fixing it.

I had a special interest in this bug because this bug bit me too a while ago :) So I was hoping this would be something to work on at the WordCamp NL contributor day.

While looking over the code and new tests, I think we can add another more precise test to prevent possible future regression on this subject, specifically on the allowed length of the username (60chars). I'm going to try to reopen this trac-ticket to be able to add the additional test.

@ruud@…
3 years ago

added furthter regression test for allowed new username length

#14 @boonebgorges
3 years ago

Additional test looks good. Can you give me a link to your profiles.wordpress.org page, so that I can give you props? Clicking on your username above (https://profiles.wordpress.org/ruud@%E2%80%A6) doesn't seem to work, so it may be that Trac hiding part of your username :)

#15 @ruud@…
3 years ago

Hi Boone,

Yes my username is kind of wrangled because of the @ in the name. A working link is https://profiles.wordpress.org/ruudjoyo/

Thnx!

#16 @boonebgorges
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 34626:

Add unit test verifying that 60 char user_login is valid.

Props ruudjoyo.
Fixes #33793.

#17 @jeremyfelt
2 years ago

#34718 was marked as a duplicate.

#18 @pento
2 years ago

#32222 was marked as a duplicate.

Note: See TracTickets for help on using tickets.