WordPress.org

Make WordPress Core

#23190 closed enhancement (fixed)

get_user_id_from_string() is returning wrong data

Reported by: godhulii_1985 Owned by: nacin
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.5
Component: Multisite Keywords: has-patch
Focuses: Cc:

Description (last modified by DrewAPicture)

Background
I was developing my custom theme and used google-oauth for auto user login. Here, I used user's google id (not login user-id, the profile id which is totally numeric) so that I can identify the user later. To create new user I used wp_insert_user().

The newly created user can update his/her initial default password and it will fail in the wp login process so everytime I call user_signon() function I call wp_update_user() to update his/her password to default [additionally, I disable password field in wp-admin area and that works for general user but as you know it is not hacker proof]

Here begins the problem
Lets assume google says that the oauth user's id is: 123456. So, I created an user with user-id: 123456. Wordpress assigned 99 to the user (that is www.example.com/?author=99 will redirect to this user's profile)

Now, when I call get_user_id_from_string('123456'), I expect 99 but I get 123456. I think it is a security risk because user-id is the users's database primary key type id (which is 99 in this case).

I looked into the core "wp-includes => ms-functions.php => get_user_id_from_string()" and found this segment:

	elseif ( is_numeric( $string ) ) {
		$user_id = $string;
	} else {
		$user = get_user_by('login', $string);
		if ( $user )
			$user_id = $user->ID;
	}

Here, is_numeric() gets precendance and I do not get my desired id (99) as my input string (or user-login-name) was 123456 which passes is_numeric() function.

Problem defination
The developers considered wp user-login-id to be alphaneumeric (I think) but in the documentation it is mentioned that: "user_login A string that contains the user's username for logging in. " in http://codex.wordpress.org/Function_Reference/wp_insert_user page. It is not mentioned it should be alphaneumeric or not.

Right now I have solved the issue by prepending 'g' infront of the oauth codes so I'm using 'g123456' as user-login-id in wp_insert_user() but I think this issue should be considered as security risk because if there is no binding on wp_insert_user() with a numeric value (123456) as user-id then get_user_id_from_string() should also respect this choice and return 99 here instead of 123456, otherwise wrong user will be signed into in this scenario.

Attachments (2)

23190.patch (603 bytes) - added by SergeyBiryukov 15 months ago.
23190.2.patch (2.1 KB) - added by SergeyBiryukov 14 months ago.

Download all attachments as: .zip

Change History (19)

comment:1 alexvorn215 months ago

  • Component changed from General to Multisite
  • Keywords needs-patch added
  • Severity changed from major to normal
  • Type changed from defect (bug) to enhancement

comment:2 follow-up: alexvorn215 months ago

better try to use get_user_by function

comment:3 follow-up: scribu15 months ago

  • Keywords reporter-feedback added; needs-docs needs-patch removed

Lets assume google says that the oauth user's id is: 123456. So, I created an user with user-id: 123456. Wordpress assigned 99 to the user (that is www.example.com/?author=99 will redirect to this user's profile)

"user id" is quite ambiguous here. Let's agree to use the wp_users table column names, for clarity:

wp_users.ID = 99
wp_users.user_login = 123456

So, you fixed it by making user_login = g123456, correct?

comment:4 follow-up: scribu15 months ago

I tend to agree with alexvom2; if you know you're dealing with user logins, just use get_user_by('login', 123456).

if there is no binding on wp_insert_user() with a numeric value (123456) as user-id then get_user_id_from_string() should also respect this choice and return 99 here instead of 123456

I don't think that's a good idea, unless we plan to support numeric user_logins everywhere. For example, WP_User::__construct() has the same heuristic.

We should just document that get_user_id_from_string( $x ) will return $x, if $x is numeric.

Version 0, edited 15 months ago by scribu (next)

comment:5 DrewAPicture15 months ago

  • Description modified (diff)

comment:6 DrewAPicture15 months ago

  • Keywords needs-codex added

comment:7 in reply to: ↑ 3 godhulii_198515 months ago

Replying to scribu:

Lets assume google says that the oauth user's id is: 123456. So, I created an user with user-id: 123456. Wordpress assigned 99 to the user (that is www.example.com/?author=99 will redirect to this user's profile)

"user id" is quite ambiguous here. Let's agree to use the wp_users table column names, for clarity:

wp_users.ID = 99
wp_users.user_login = 123456

So, you fixed it by making user_login = g123456, correct?


Yes, you are correct. (and sorry for the ambiguous term, I tried to avoid as much as possible but missed here)

comment:8 in reply to: ↑ 4 ; follow-up: godhulii_198515 months ago

Replying to scribu:

I tend to agree with alexvom2; if you know you're dealing with user logins, just use get_user_by('login', 123456).

if there is no binding on wp_insert_user() with a numeric value (123456) as user-id then get_user_id_from_string() should also respect this choice and return 99 here instead of 123456

I don't think that's a good idea, unless we plan to support numeric user_logins everywhere. For example, WP_User::__construct() has the same heuristic.

We should just document that get_user_id_from_string( $x ) will return $x, if $x is numeric.


Okk. Ofcourse that note will make sense and additionally you can also mention get_user_by() function in the codex so that others can properly decide which function to use (for example: I have to use get_user_by() to solve my login issue)

comment:9 in reply to: ↑ 2 godhulii_198515 months ago

Replying to alexvorn2:

better try to use get_user_by function


Yes, doing that :-)

comment:10 in reply to: ↑ 8 alexvorn215 months ago

http://codex.wordpress.org/WPMU_Functions/get_user_id_from_string
May be either the user's log in (their username), or the email address registered in WordPress.

so this function should not check from ID, I mean we should remove the lines for checking for a numeric string.

comment:11 DrewAPicture15 months ago

  • Keywords reporter-feedback removed

SergeyBiryukov15 months ago

comment:12 SergeyBiryukov15 months ago

  • Keywords close added; needs-codex removed

get_user_id_from_string() was introduced in mu:884 and inherited during the merge: [12603].

Replying to scribu:

We should just document that get_user_id_from_string( $x ) will return $x, if $x is numeric.

I've added a note to both Codex articles:
http://codex.wordpress.org/WPMU_Functions/get_user_id_from_string
http://codex.wordpress.org/Function_Reference/get_user_id_from_string

We could also add a note to the inline docs (23190.patch), but I guess we should just replace the only instance of get_user_id_from_string() in core with get_user_by() (#23192) and then deprecate the former in favor of latter.

Last edited 15 months ago by SergeyBiryukov (previous) (diff)

comment:13 SergeyBiryukov14 months ago

In 1216/tests:

Test that is_user_spammy() properly recognizes a numeric username. props bananastalktome. see #23192. see #23190.

comment:14 SergeyBiryukov14 months ago

In 23431:

Replace get_user_id_from_string() with get_user_by(). props ocean90, bananastalktome. fixes #23192. see #23190.

comment:15 SergeyBiryukov14 months ago

  • Keywords close removed
  • Milestone changed from Awaiting Review to 3.6

Now that #23192 is in, I guess we should deprecate get_user_id_from_string(). A note from 23190.patch still might be helpful.

SergeyBiryukov14 months ago

comment:16 SergeyBiryukov14 months ago

  • Keywords has-patch added

comment:17 nacin14 months ago

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

In 23438:

Deprecate get_user_id_from_string() in favor of get_user_by( $field ) where $field is 'email' or 'login'. props SergeyBiryukov. fixes #23190.

Note: See TracTickets for help on using tickets.