WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#28315 closed defect (bug) (fixed)

update_user_meta() updates user with ID1 if WP_Error is passed as first argument

Reported by: javorszky Owned by: wonderboymusic
Milestone: 4.0 Priority: normal
Severity: normal Version: 3.9.1
Component: Options, Meta APIs Keywords: has-patch commit
Focuses: Cc:

Description

// Let $existing details be an array with an email address that's already in there
$id = wp_insert_user( $existing_details );
 
// At this point, because $id _SHOULD_ be an instance of WP_Error (which it is btw),
// telling me a user with that email address or username already exists.
// You kinda hope this would fail or have no effect whatsoever.
update_user_meta( $id, 'key', 'value' );
 
// Now go forth and check user #1's user meta tables...

Problem is with meta.php:136, function update_metadata.

This:

if ( !$object_id = absint($object_id) )
	return false;

should be

if ( $object_id != absint($object_id) )
	return false;

(Tested locally, worked)

Attachments (3)

28315.diff (465 bytes) - added by jacklenox 6 years ago.
Added a patch using kitchin's suggestion above.
28315.2.diff (6.6 KB) - added by SergeyBiryukov 6 years ago.
28315.3.diff (6.6 KB) - added by SergeyBiryukov 6 years ago.

Download all attachments as: .zip

Change History (28)

#1 @javorszky
6 years ago

Also applies to the table comparison.

#2 @kitchin
6 years ago

Good suggestion. The fix seems a little obscure, relying on absint() casting WP_Error differently than the comparison operator. Maybe be it should be:

// Reject $object_id if a WP_Error, other non-scalar, or empty.
if ( !is_scalar($object_id) or !$object_id = absint($object_id) )
	return false;

Also this maintains the current behavior of casting to integer.

Last edited 6 years ago by kitchin (previous) (diff)

#3 @wonderboymusic
6 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.0

There should be validation after wp_insert_user()

@jacklenox
6 years ago

Added a patch using kitchin's suggestion above.

#4 @anubisthejackle
6 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

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


6 years ago

#6 @DrewAPicture
6 years ago

  • Milestone 4.0 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

The onus is on the developer to ensure that the value type they're passing is the value type the receiving function or functions expect.

#7 @sc0ttkclark
6 years ago

I guess that makes sense, but I still don't like that it could happen.

#8 @javorszky
6 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

It doesn't make sense. Yes, the onus should be on the developer, but bugs happen because theme / plugin / core / generally developers are of varying skill. Because user 1 is the admin of the site, changing meta info on user 1 instead of the intended user, because that user fails for some reason, is just bad.

Especially so when we know about the possibility that this could happen. I'd expect WordPress to exit with an error if I passed a non-integer. The change needed in core is tiny, and it would save a ton of headaches further down the line. Defensive programming is a good thing, and bugs that arise from PHP's type casting rules aren't fun to debug either.

#9 @nacin
6 years ago

  • Milestone set to 4.0

This is pretty nasty and I'd like to fix it.

#10 @javorszky
6 years ago

@nacin, I'm happy to fix this. This'd be my first PR to WP Core, who can I bug for help / how do I get started besides reading make.wordpress.org about code contrib?

#11 @georgestephanis
6 years ago

I'd be happy to spend whatever time you need to get you up to speed.

Are you familiar with IRC? I normally idle in the WordPress channels, username georgestephanis -- or daljo628 on Skype if you don't IRC.

#12 @SergeyBiryukov
6 years ago

is_numeric() would probably be a more appropriate check here than is_scalar().

#13 follow-up: @georgestephanis
6 years ago

Eh, I'm unsure of that. Someone could inadvertently pass in a string containing the number -- some ways of getting a user id out of the db or from inputs could be strings.

It may be worth excluding booleans, though, as (true) will also flip to 1 when casted as an int.

#14 in reply to: ↑ 13 @SergeyBiryukov
6 years ago

Replying to georgestephanis:

Someone could inadvertently pass in a string containing the number -- some ways of getting a user id out of the db or from inputs could be strings.

is_numeric() would allow for that, unlike is_int().

#15 @javorszky
6 years ago

intval( abs( $thing ) ) == $thing should make sure that what I passed in is a positive integer.

#16 @georgestephanis
6 years ago

Oh, hey, another fun trick in PHP I didn't know about. is_numeric() should be perfect, then!

#17 @javorszky
6 years ago

is_numeric() works as long as we're aware that is_numeric("9.482") === true and therefore need to make sure that it's actually an integer with something else.

#18 @wonderboymusic
6 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from reopened to closed

In 29339:

Bail on update_user_meta() when $object_id is non-numeric.

Adds unit test.

Props jacklenox, wonderboymusic.
Fixes #28315.

#19 @wonderboymusic
6 years ago

  • Keywords needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

There's a killer race condition in here:

  • Tests pass when run --group 28315
  • Tests pass when run --group user
  • Test fails when all tests are run, AND 2 users are set with the same email address

FAIL WHALE.

#20 @wonderboymusic
6 years ago

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

In 29341:

Account for WP_IMPORTING being defined in the unit tests added in [29339] when all tests are run.

Glory, glory hallelujah.

Fixes #28315.

#21 @SergeyBiryukov
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

28315.2.diff does this check consistently for all meta functions, makes the validation code more readable (to avoid confusions like #23089), and adds more unit tests.

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


6 years ago

#23 @SergeyBiryukov
6 years ago

28315.2.diff causes two unit test failures:

1) Tests_Post_Meta::test_nonunique_postmeta
Failed asserting that false is true.

S:\home\wordpress\develop\tests\phpunit\tests\post\meta.php:98

2) Tests_Post_Meta::test_delete_post_meta_by_key
Failed asserting that false is true.

S:\home\wordpress\develop\tests\phpunit\tests\post\meta.php:156

delete_metadata() can accept null or false if $delete_all is true, see delete_post_meta_by_key().

28315.3.diff takes that into account and fixes the failures.

#24 @SergeyBiryukov
6 years ago

  • Keywords commit added

#25 @wonderboymusic
6 years ago

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

In 29421:

After [29339] and [29341], add more unit tests and less ambiguous type-checking before bailing in meta-related functions that expect a numeric value for $object_id.

Props SergeyBiryukov.
Fixes #28315.

Note: See TracTickets for help on using tickets.