user_nicename is not being sanitized when updated by wp_update_user()

[joemcgill]

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

[joemcgill]

Filter user_nicename before updating the database.

[sareiodata]

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.

[joemcgill]

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.

[boonebgorges]

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.

[joemcgill]

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'] );

[boonebgorges]

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.

[nacin]

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.

[boonebgorges]

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.