Make WordPress Core

Opened 10 years ago

Closed 10 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's profile javorszky Owned by: wonderboymusic's profile 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 10 years ago.
Added a patch using kitchin's suggestion above.
28315.2.diff (6.6 KB) - added by SergeyBiryukov 10 years ago.
28315.3.diff (6.6 KB) - added by SergeyBiryukov 10 years ago.

Download all attachments as: .zip

Change History (28)

#1 @javorszky
10 years ago

Also applies to the table comparison.

#2 @kitchin
10 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 rejecting -1, if that ever happens.

Version 3, edited 10 years ago by kitchin (previous) (next) (diff)

#3 @wonderboymusic
10 years ago

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

There should be validation after wp_insert_user()

@jacklenox
10 years ago

Added a patch using kitchin's suggestion above.

#4 @anubisthejackle
10 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.


10 years ago

#6 @DrewAPicture
10 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
10 years ago

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

#8 @javorszky
10 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
10 years ago

  • Milestone set to 4.0

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

#10 @javorszky
10 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
10 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
10 years ago

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

#13 follow-up: @georgestephanis
10 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
10 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
10 years ago

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

#16 @georgestephanis
10 years ago

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

#17 @javorszky
10 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
10 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
10 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
10 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
10 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.


10 years ago

#23 @SergeyBiryukov
10 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
10 years ago

  • Keywords commit added

#25 @wonderboymusic
10 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.