WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#29696 closed defect (bug) (fixed)

user_nicename is not being sanitized when updated by wp_update_user()

Reported by: joemcgill Owned by: boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.1
Component: Users Keywords: has-patch commit
Focuses: Cc:

Description

When a user account is initially created via wp_insert_user() the user_nicename is created from user_login after it as been run through sanitize_user( $userdata['user_login'], true ); and sanitize_title( $user_login );. However, when a user is updated and the update includes a new value for the user_nicename field, that new value is not sanitized at all.

I imagine this could create all sorts of problems, but it specifically makes author archive pages 404 if the updated user_nicename includes a character that gets sanitized by query.php.

To test:

  • Create a new user
  • Use wp-cli or manually run wp_update_user() to update the user_nicename to a value that includes a '.' (e.g., john.doe).
  • See that the '.' gets passed into the database.

Additionally:

  • create some posts with the test user
  • try to access the users' author archive (i.e. /author/john.doe/) and watch the page bomb out.
  • cry

Attachments (2)

29696.patch (560 bytes) - added by joemcgill 5 years ago.
Filter user_nicename before updating the database.
29696.2.patch (1.7 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (13)

@joemcgill
5 years ago

Filter user_nicename before updating the database.

#1 @joemcgill
5 years ago

  • Keywords has-patch added

#2 @sareiodata
5 years ago

I've tested the issue and managed to replicate it.

However, the bug is explained incorrectly (the patch is correct)

It's not wp_update_user that's causing problems, it's wp_insert_user(). wp_insert_user doesn't sanitize the user_nicename if you transmit it like a parameter like so:
wp_insert_user(array('user_login'=>'johndoe','user_pass'=>'pass','user_nicename'=>'john.doe'));

Applied the patch, tested it again and the nicename is correctly sanitized. Also the code is straight forward.

The question is, since this will be used by developers primarily, SHOULD we sanitize the nicename or let the developer do that? An input from someone else would be welcomed.

#3 @joemcgill
5 years ago

Thanks for the clarification. You're correct that the bug is actually a part of wp_insert_user(), though it's usually only exposed when executed through wp_update_user(), which I admittedly didn't explain very well.

The main reason that I would lobby for fixing this is that there are now popular tools like wp-cli that are being used to access the user APIs without accessing them through the WordPress UI. In those cases, doing something like wp user update ... could lead to nicenames in the database that would fail in a user query. Furthermore, I can't think of a reason why you would not want to sanitize a user_nicename on an update using the same rules as an insert.

#4 @sareiodata
5 years ago

  • Keywords 2nd-opinion added

#5 @boonebgorges
5 years ago

  • Keywords needs-unit-tests added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to 4.1

Thanks for the report and for the patch.

The question is, since this will be used by developers primarily, SHOULD we sanitize the nicename or let the developer do that?

If we're enforcing the character restriction in one place where user_nicename is generated, we should be enforcing it all the time. In particular, since the main purpose of user_nicename is for use in URLs, if we're allowing nicenames that break URLs, then that's a bug :) So yes, the logic behind the patch seems good.

Can you point to the place in query.php where the character is getting stripped?

It would be great to get a unit test that demonstrates the problem.

#6 @boonebgorges
5 years ago

  • Owner set to boonebgorges
  • Status changed from new to reviewing

#7 @joemcgill
5 years ago

Thanks for looking at this, Boone. The line in query.php that causes the issue is line 2784:
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/query.php#L2784

$q['author_name'] = sanitize_title_for_query( $q['author_name'] );

#8 @boonebgorges
5 years ago

  • Keywords needs-unit-tests removed
  • Status changed from reviewing to accepted

29696.2.patch adds a unit test, cleans up the logic of the original patch, and adds some inline documentation. Gonna wait for the all clear from a lead before going forward with it.

This ticket was mentioned in IRC in #wordpress-dev by boonebgorges. View the logs.


5 years ago

#10 @nacin
5 years ago

  • Keywords commit added

Anyone relying on this was relying on very unexpected behavior. Sanitizing each time is also how it works for user_login.

This could break something. There's the potential that someone did want characters in a URL that would be stripped out by sanitize_title(), such as a dot. But they'd have to be doing this through the API somehow.

Wanted to document these issues, but I am fine with it.

#11 @boonebgorges
5 years ago

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

In 29819:

Always sanitize user_nicename in wp_insert_user().

Previously, a 'user_nicename' parameter passed into the function was
unsanitized. This could result in a mismatch between the sanitized nicename
generated automatically at user creation, resulting in broken author archive
permalinks.

Props joemcgill.

Fixes #29696.

Note: See TracTickets for help on using tickets.