WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 2 months ago

#17904 assigned defect (bug)

Multisite has more restrictions on user login character set

Reported by: duck_ Owned by: jeremyfelt
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Login and Registration Keywords: has-patch
Focuses: multisite Cc:

Description

Multisite has more restrictions on the characters allowed in a user's login name compared to single site. This seems unnecessary and confusing. It was also the root of a recent bug in the importer, see this forum thread and the workaround.

I haven't worked up a patch yet since there seem to be a few locations where these restrictions are enforced and I don't know if I have found them all yet:

  • wpmu_validate_user_signup() uses the regex /[a-z0-9]+/
  • ms-default-filters.php adds strtolower to sanitize_user

Relevant: http://mu.trac.wordpress.org/changeset/1689 [12948]

Attachments (6)

17904.diff (1.1 KB) - added by martythornley 2 years ago.
17904.2.diff (12.7 KB) - added by jeremyfelt 2 years ago.
17904.unittests.patch (4.1 KB) - added by imath 7 months ago.
17904.3.patch (11.5 KB) - added by imath 7 months ago.
17904.4.patch (13.5 KB) - added by imath 2 months ago.
17904.5.patch (13.6 KB) - added by imath 2 months ago.
Moves the blogname characters check in signup_blog()

Download all attachments as: .zip

Change History (46)

comment:1 @danielbachhuber4 years ago

  • Cc d@… added

comment:3 @mercime3 years ago

  • Cc mercijavier@… added

comment:4 @mordauk3 years ago

  • Cc pippin@… added

I'd love to get some more traction on this. It's causing an issue for me with several plugins when running them on MS installs. Here's a bug report for one of the plugins: https://github.com/pippinsplugins/Easy-Digital-Downloads/issues/643

comment:5 @chriscct73 years ago

  • Cc chriscct7@… added
  • Version 3.0 deleted

comment:6 @chriscct73 years ago

  • Version set to trunk

comment:7 follow-up: @SergeyBiryukov3 years ago

  • Version changed from trunk to 3.0

comment:8 in reply to: ↑ 7 ; follow-up: @chriscct73 years ago

Replying to SergeyBiryukov:Isn't the version supposed to be the latest version the bug applies to? Or at least that's what I always assumed it was for....

comment:9 @SergeyBiryukov3 years ago

#23124 was marked as a duplicate.

comment:10 in reply to: ↑ 8 ; follow-up: @SergeyBiryukov3 years ago

Replying to chriscct7:

Isn't the version supposed to be the latest version the bug applies to?

Version number indicates when the issue was initially introduced/reported.

comment:11 in reply to: ↑ 10 @chriscct73 years ago

Oh, thats the total opposite of what I thought it was for. Good to know!

Replying to SergeyBiryukov:

Replying to chriscct7:

Isn't the version supposed to be the latest version the bug applies to?

Version number indicates when the issue was initially introduced/reported.

comment:12 @SergeyBiryukov3 years ago

#23452 was marked as a duplicate.

comment:14 follow-up: @Ipstenu3 years ago

I was under the understanding that the restrictions are in place because the username is used for the default sitename slug. So to prevent bad URLS, we sanitized everything to match.

If that can be broken out of the username check and only applied to the sitename, that would be okay.

comment:15 follow-up: @boonebgorges3 years ago

I was under the understanding that the restrictions are in place because the username is used for the default sitename slug.

Ah, I didn't know that - can you point to documentation of that fact, or are you hypothesizing?

If that can be broken out of the username check and only applied to the sitename, that would be okay.

It'd be straightforward to use, eg, the user_nicename for slugs (that's what it's for, after all). The only potential problem I see with that is that when there are user_nicename clashes (ip@stenu and ipsten@u, for example), they're cleared up with trailing counters (so the second one to get there is ipstenu2, etc). That might be less than ideal from a UX point of view, because URLs would not match usernames. But this is an edge case, and seems to me less important than consistency and flexibility in user_login.

comment:16 @Ipstenu3 years ago

I swear I read it somewhere, or it was mentioned on IRC, but I can;t find where I saw it. Maybe I got that idea from Otto? (Blame Otto!)

I think the UX loss is more important than mucking with user_login too, though. If there's no other check on sitename validations, we'll have to make sure something's put into place :)

(I swear this came up when someone was kvetching that he couldn't name a site foo.bar for a subfolder site, since that gets wiggy with subdomains.)

comment:17 @jeremyfelt2 years ago

  • Keywords dev-feedback added
  • Milestone changed from Future Release to 3.7

Moving to 3.7 for discussion

comment:18 in reply to: ↑ 14 @martythornley2 years ago

Replying to Ipstenu:

I was under the understanding that the restrictions are in place because the username is used for the default sitename slug. So to prevent bad URLS, we sanitized everything to match.

If that can be broken out of the username check and only applied to the sitename, that would be okay.

The create_new_blog() function uses sanitize_user() to sanitize the sitename, so the same filters used for users would be used for sites.

So it might be a question of whether you would want to use the same validations for sites as users? Or should there be a separate sanitize_sitename()?

But I am not seeing anywhere that there is a fallback to the actual username. During wpmu_validate_blog_signup() it checks new site names and does its own separate lower string check.

It seems that removing the strtolower from sanitize user does the trick as long as that is the desired effect?

Last edited 2 years ago by martythornley (previous) (diff)

@martythornley2 years ago

comment:19 @jeremyfelt2 years ago

Ran into this today and compared what rules single site and multisite enforced. I'm wondering if there's a way we can abstract the username logic into a common function and plug it into both edit_user() and wpmu_validate_user_signup(). It's entirely possible to filter the final results with wpmu_validate_user_signup, but that seems ugly.

Single Site edit_user()

When adding a user in single site through wp-admin/user-new.php, edit_user() is used and does the following:

  1. Process with sanitize_user(), but do not compare to original POST data
  2. Check if ( $user_login == '' )
  3. Check validate_username()
  4. Check username_exists()

Multisite wpmu_validate_user_signup()

When adding a user in multisite through wp-admin/user-new.php or wp-admin/network/user-new.php, wpmu_validate_user_signup() is used and does the following:

  1. Process with preg_replace( '/\s+/', '', sanitize_user( $user_name, true ) ) and compare the result to original POST data
  2. Accept only a-z, 0-9 with preg_match( '/[^a-z0-9]/', $user_name )
  3. Check if empty()
  4. Check if in_array( $username, $illegal_names ) to filter out www, web, root, etc...
  5. Check if strlen( $user_name ) < 4 )
  6. Check `if strpos( ' '. $user_name, '_' ) != false )
  7. Check if ( preg_match( '/^[0-9]*$/', $user_name ) )
  8. Check username_exists()
  9. Check DB tables for any matching, pending signups

@jeremyfelt2 years ago

comment:20 @jeremyfelt2 years ago

  • Keywords has-patch added; needs-patch removed

17904.2.diff introduces wp_validate_user_login() in an attempt to consolidate the user login restrictions between single site and multisite installations.

New wp_validate_user_login() requirements:

  • minimum of 4 characters
  • only contains (case-insensitive) characters: a-z 0-9 _ . - @
  • no whitespace
  • not on blacklist of illegal names
  • contains at least one letter
  • must be unique
  • not pending signup

The original criteria for wpmu_validate_user_signup() was a character set of a-z and 0-9. This has been loosened to match the criteria in edit_site().

register_new_user(), edit_site(), and wpmu_validate_user_signup() have all been modified to use the new wp_validate_user_login() function rather relying on their individual (and different) logic.

I've pieced together the original error strings from edit_user() and wpmu_validate_user_signup(), though I'm a bit confused on the naming convention between user_name and user_login and some others. It may be worth revisiting the messaging here?

It's entirely possible that we can deprecate validate_username() as it is no longer used anywhere in core as of this patch and does not exactly perform the described function. We may want to replace with an option to return bool from wp_validate_user_login() instead or ...

No changes in single site or multisite tests at the moment, though it looks like not much is actually tested around this.

comment:21 @DrewAPicture2 years ago

  • Cc xoodrew@… added

comment:22 @r-a-y2 years ago

  • Cc r-a-y@… added

comment:23 @nacin2 years ago

  • Milestone changed from 3.7 to 3.8

Per conversation with jeremyfelt, pushing this to 3.8.

comment:24 @jeremyfelt22 months ago

  • Keywords needs-unit-tests added
  • Milestone changed from 3.8 to Future Release

Probably too late for the 3.8 cycle. My latest patch introduced wp_validate_user_login(), which should definitely have unit tests before being used.

comment:25 @desrosj21 months ago

  • Cc desrosj@… added

comment:26 @jeremyfelt20 months ago

  • Component changed from Multisite to Login and Registration
  • Focuses multisite added

comment:27 @nafur12 months ago

I just stumbled about this issue and it seems that the patch did not made it into any release yet.

Is anyone taking care of this one? I'd really like to see this patch in the upstream version...

comment:28 @jdaviescoates11 months ago

I would LOVE this issue to be resolved asap. This has been reported since 3.0 and now we're on 4.0! When setting up stand alone installs of wordpress I nearly always use email addresses for usernames when setting new users because they tend to be both a lot longer and more varied (and hence a lot harder to guess/ more secure) than the short a-z 0-9 restricted user names currently possible with mulitisite installs. I just tried to create new user on a multisite install using an email address but hit this issue. My work around for now is to use this plugin that requires people use their email address instead of their username to login.

comment:29 @boonebgorges8 months ago

#31016 was marked as a duplicate.

comment:30 @ArkonLabs8 months ago

This is still an issue in 4.1.

I would like to have the choice of allowing users to register with first.last or first.last@… to match AD.

According to previous replies, this fix almost made it into version 3.7 and 3.8... are there any updates?

comment:31 @imath7 months ago

  • Keywords needs-unit-tests removed

17904.3.patch is a bit different than 17904.2.diff.
I've included some suggested unit tests in 17904.unittests.patch that can be used for 17904.2.diff in case my suggested patch is doing something wrong.

@imath7 months ago

@imath7 months ago

comment:32 @slackbot7 months ago

This ticket was mentioned in Slack in #core-multisite by nerrad. View the logs.

comment:33 @boonebgorges5 months ago

In 31970:

Unit tests for wpmu_validate_user_signup().

See #17904, #26784.

comment:34 @pento5 months ago

In 31978:

Unit Tests added in [31970] need to be restricted to run in Multisite only.

See #17904, #26784.

comment:35 in reply to: ↑ 15 ; follow-up: @jeremyfelt4 months ago

  • Milestone changed from Future Release to 4.3

Replying to boonebgorges:

I was under the understanding that the restrictions are in place because the username is used for the default sitename slug.

Ah, I didn't know that - can you point to documentation of that fact, or are you hypothesizing?

Continued hypothesizing only because I recently learned... In b2++, the username/sitename slug was first used as part of the table name (e.g. wp_jeremyfelt_posts) before it was switched to blog ID. This may have been a driver in some of the original sanitization.

Moving this to 4.3 because we have tests, a patch, and a full cycle ahead of us.

comment:36 @wonderboymusic2 months ago

  • Keywords needs-refresh added; dev-feedback removed

Someone please refresh this with gusto

comment:37 @wonderboymusic2 months ago

  • Owner set to jeremyfelt
  • Status changed from new to assigned

comment:38 in reply to: ↑ 35 @obenland2 months ago

  • Milestone changed from 4.3 to Future Release

Replying to jeremyfelt:

Moving this to 4.3 because we have tests, a patch, and a full cycle ahead of us.

The cycle is almost over and there hasn't been movement outside of wonderboymusic's comments. Feel free to re-milestone one there is a refreshed patch and this has a chance to go in.

comment:39 @jeremyfelt2 months ago

In 33083:

Usernames in multisite should be restricted to 60 characters or fewer.

Only 60 characters can be stored in the database for a username, which could cause lookup issues when attempting to use similar usernames of extreme length.

Props @DJPaul.
See #17904, Fixes #26784.

comment:40 @imath2 months ago

  • Keywords needs-refresh removed

17904.4.patch is a refreshed version of the patch to r33083 + some adjustments :

  • If i understand well, these characters _ - @ . are now allowed for the user login, so i've removed the contains_underscores index from the dataset of Tests_Multisite_WpmuValidateUserSignup->test_user_name
  • When a user signups with a blog, i suggest to make sure the _ - @ . characters are removed from the blogname input in show_blog_form() as the user_name is used as the default value of the blog's name.

@imath2 months ago

@imath2 months ago

Moves the blogname characters check in signup_blog()

Note: See TracTickets for help on using tickets.