Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#54987 closed defect (bug) (fixed)

Check if 'user_nicename' is not too long after the 'pre_user_nicename' filter

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: davidbaumwald's profile davidbaumwald
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Users Keywords: good-first-bug has-patch
Focuses: Cc:

Description

Background: #33793, #44107.

In wp_insert_user(), there are a few checks to make sure the values are not too long to fit into the database:

  • user_login may not be longer than 60 characters.
  • user_nicename may not be longer than 50 characters.
  • user_url may not be longer than 100 characters.

Some history: [34218], [34626], [52650].

While the user_login and user_url checks run after the pre_user_login and pre_user_url filters, respectively, the user_nicename check for some reason runs before the pre_user_nicename filter.

For consistency with the other filters, and to make sure the filtered value is still no longer than 50 characters, the mb_strlen( $user_nicename ) > 50 check should run after the pre_user_nicename filter.

Attachments (6)

54987.patch (612 bytes) - added by ravipatel 3 years ago.
As per comment i have added a condiiton after pre_user_nicename filter
52650.patch (903 bytes) - added by mukesh27 3 years ago.
Patch that include another solution
54987.2.patch (903 bytes) - added by mukesh27 3 years ago.
54987.diff (2.4 KB) - added by csesumonpro 3 years ago.
Patch created
54987.2.diff (1.0 KB) - added by SergeyBiryukov 3 years ago.
54987.3.diff (1.0 KB) - added by csesumonpro 3 years ago.
Patch added

Download all attachments as: .zip

Change History (21)

@ravipatel
3 years ago

As per comment i have added a condiiton after pre_user_nicename filter

This ticket was mentioned in PR #2244 on WordPress/wordpress-develop by MuhammadFaizanHaidar.


3 years ago
#1

  • Keywords has-patch added; needs-patch removed

Added Check if 'user_nicename' is not too long after the 'pre_user_nicename' filter in user.php function wp_insert_user( $userdata )

Trac ticket: https://core.trac.wordpress.org/ticket/54987

#2 @muhammadfaizanhaidar
3 years ago

@SergeyBiryukov did not notice it already had a patch but I have removed size check from earlier location and added a more strict check after the 'pre_user_nicename' filter
Following added.

<?php
// Remove any non-printable chars from the user_nicename string to see if we have ended up with an empty username.
        $user_nicename = trim( $user_nicename );

        // user_nicename must be between 0 and 50 characters.
        if ( empty( $user_nicename ) ) {
                return new WP_Error( 'empty_user_nicename', __( 'Cannot create a user with an empty nicename.' ) );
        } elseif ( mb_strlen( $user_nicename ) > 50 ) {
                return new WP_Error( 'user_nicename_too_long', __( 'Nicename may not be longer than 50 characters.' ) );
        }

@mukesh27
3 years ago

Patch that include another solution

@mukesh27
3 years ago

#3 @mukesh27
3 years ago

Hi there!

54987.2.patch introduce an other approach for the solution. Please have a look

#4 @muhammadfaizanhaidar
3 years ago

@mukesh27 I think user_nicename check should run after after the 'pre_user_nicename' filter according to the ticket.

#5 @ravipatel
3 years ago

@mukesh27
Please review my patch as @muhammadfaizanhaidar.
What's happen when any one post user_nicename more then 50 caracter using the filter pre_user_nicename, So code condition come after the filter code.

Last edited 3 years ago by ravipatel (previous) (diff)

@csesumonpro
3 years ago

Patch created

#6 @csesumonpro
3 years ago

As per the comment, I have already checked pre_user_nicename and pre_user_url also checked pre_user_login filter. pre_user_login already validate that's why I just added validation after pre_user_nicename filter and pre_user_url filter.

This ticket was mentioned in Slack in #core by csesumonpro. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by csesumonpro. View the logs.


3 years ago

#9 @davidbaumwald
3 years ago

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

#10 follow-up: @SergeyBiryukov
3 years ago

Thanks for the patches! It looks like most of them have changes that are a bit out of scope for this ticket:

  • 54987.patch introduces a duplicate check instead of moving the existing one.
  • 54987.2.patch moves the existing check, but no longer throws a WP_Error if the nicename is too long.
  • 54987.diff has some formatting changes and renames a variable, which does not seem necessary here. It also adds validation for $user_url, but that was already added in [52650] / #44107.
  • PR #2244 introduces a new check that $user_nicename cannot be empty. I think this would be better discussed in a separate ticket.

For this ticket, I was thinking of just moving the existing check after the filter, like in 54987.2.diff.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

This ticket was mentioned in Slack in #core by csesumonpro. View the logs.


3 years ago

@csesumonpro
3 years ago

Patch added

#12 in reply to: ↑ 10 @azouamauriac
3 years ago

Replying to SergeyBiryukov:

  • PR #2244 introduces a new check that $user_nicename cannot be empty. I think this would be better discussed in a separate ticket.

I don't think this worth another ticket, there is already a control above to make sure the nicename won't be empty https://prnt.sc/B6DCuHPqSNDh

For this ticket, I was thinking of just moving the existing check after the filter, like in 54987.2.diff.

Agree. This LGTM.

#13 @davidbaumwald
3 years ago

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

In 52954:

Users: Check maximum length of user_nicename after filters are applied.

Similar to other checks on user_login and user_url, this change moves the maximum length check on user_nicename after the pre_user_nicename filter has been applied, to account for any changes to the value prior to saving.

Props SergeyBiryukov, ravipatel, muhammadfaizanhaidar, mukesh27, csesumonpro, azouamauriac.
Fixes #54987.

dream-encode commented on PR #2244:


3 years ago
#14

Thanks for the PR! This was merged into Core in https://core.trac.wordpress.org/changeset/52954.

MuhammadFaizanHaidar commented on PR #2244:


3 years ago
#15

Thanks for the PR! This was merged into Core in https://core.trac.wordpress.org/changeset/52954.

Pleasure is all mine.

Note: See TracTickets for help on using tickets.