Make WordPress Core

Opened 6 years ago

Closed 2 years ago

Last modified 2 years ago

#44107 closed defect (bug) (fixed)

wp_insert_user should return WP_Error if user_url's length is too long

Reported by: tszming's profile tszming Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.0 Priority: normal
Severity: normal Version: 2.0
Component: Users Keywords: has-patch has-unit-tests dev-feedback
Focuses: administration Cc:

Description

Just like the user_nicename checking

Consider the test below

public function test_wp_insert_user_should_reject_user_url_over_100_characters() {
    $user_url = str_repeat( 'a', 101 );

    $u             = wp_insert_user(
        array(
            'user_login'    => 'test',
            'user_email'    =>  'test@example.com',
            'user_pass'     => 'password',
            'user_url' => $user_url,
        )
    );

    $this->assertWPError( $u );
}

This also apply to the function wp_update_user

Attachments (3)

44107.diff (1.4 KB) - added by mkox 2 years ago.
Create error if user url length is > 100 characters, includes unit-test
44107.2.diff (1.3 KB) - added by mkox 2 years ago.
Fixed indentations in patch
44107.3.diff (1.4 KB) - added by mkox 2 years ago.
Remove wrong tabs

Download all attachments as: .zip

Change History (13)

#1 @sabernhardt
3 years ago

  • Component changed from General to Build/Test Tools
  • Keywords needs-patch added

#2 @mkox
2 years ago

  • Component changed from Build/Test Tools to Administration
  • Focuses administration added
  • Version set to 2.0

I tried to create a user with an user url with more than 100 characters. The user will not be created but an empty registration mail was sent to the given mail address and a "New User created" message is shown in admin area.

@mkox
2 years ago

Create error if user url length is > 100 characters, includes unit-test

#3 @mkox
2 years ago

  • Keywords has-patch has-unit-tests dev-feedback added; needs-patch removed

The patch checks for the user url length and returns a WP_Error just like at user's nicename.

Test from @tszming added accordingly.

#4 @sabernhardt
2 years ago

  • Component changed from Administration to Users
  • Milestone changed from Awaiting Review to 6.0

Thanks for the patch!

Please adjust the spacing so the equal signs line up and there are no extra spaces/tabs.

	public function test_wp_insert_user_should_reject_user_url_over_100_characters() {
		$user_url = str_repeat( 'a', 101 );
		$u        = wp_insert_user(
			array(
				'user_login' => 'test',
				'user_email' => 'test@example.com',
				'user_pass'  => 'password',
				'user_url'   => $user_url,
			)
		);

@mkox
2 years ago

Fixed indentations in patch

#5 @mkox
2 years ago

Thanks for the hint @sabernhardt!

@mkox
2 years ago

Remove wrong tabs

#6 follow-up: @SergeyBiryukov
2 years ago

Thanks for the patch and the unit test!

This looks good, I only have a few minor notes:

  • The if ( mb_strlen( $raw_user_url ) > 100 ) { ... } condition itself does not need a DocBlock, as it's not a function nor a hook. See the documentation standards for more details on what entities need a DocBlock.
  • I think the check should run after the pre_user_url filter, to make sure the filtered value is still no longer than 100 characters. That would be consistent with the mb_strlen( $user_login ) > 60 check, which runs after the pre_user_login filter, though not with the mb_strlen( $user_nicename ) > 50 check, which runs before the pre_user_nicename filter for some reason. Let's address the latter in a new ticket. Some history: [34218].
  • The unit test should also verify that the error code is user_url_too_long.

I can make these adjustments on commit.

#7 @SergeyBiryukov
2 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 52650:

Users: Return a WP_Error from wp_insert_user() if the user_url field is too long.

The user_url database field only allows up to 100 characters, and if the value is longer than that, the function should return a proper error message instead of silently failing.

This complements similar checks for user_login and user_nicename fields.

Follow-up to [45], [1575], [32299], [34218], [34626].

Props mkox, sabernhardt, tszming, SergeyBiryukov.
Fixes #44107.

#8 in reply to: ↑ 6 @SergeyBiryukov
2 years ago

Replying to SergeyBiryukov:

I think the check should run after the pre_user_url filter, to make sure the filtered value is still no longer than 100 characters. That would be consistent with the mb_strlen( $user_login ) > 60 check, which runs after the pre_user_login filter, though not with the mb_strlen( $user_nicename ) > 50 check, which runs before the pre_user_nicename filter for some reason. Let's address the latter in a new ticket.

Follow-up: #54987.

#9 @Cybr
2 years ago

I did not test the code, but just for sanity, if we now insert a few mb* chars (strlen( $user_url ) > 100 === true), this "empty registration email"-issue will no longer happen? If it does, shouldn't we use strlen() instead of mb_strlen()?

echo mb_strlen( str_repeat( chr(240) . chr(159) . chr(144) . chr(152), 100 ) ); // 100
echo    strlen( str_repeat( chr(240) . chr(159) . chr(144) . chr(152), 100 ) ); // 400

#10 @SergeyBiryukov
2 years ago

mb_strlen() is what was used for user_login and user_nicename in [34218]. I believe it is also the correct check for the user_url field, as the database field is limited to 100 characters, not 100 bytes.

Note: See TracTickets for help on using tickets.