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

[javorszky]

// 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)

[javorszky]

Also applies to the table comparison.

[kitchin]

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.

[wonderboymusic]

There should be validation after wp_insert_user()

[jacklenox]

Added a patch using kitchin's suggestion above.

[DrewAPicture]

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.

[sc0ttkclark]

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

[javorszky]

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.

[nacin]

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

[javorszky]

@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?

[georgestephanis]

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.

[SergeyBiryukov]

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

[georgestephanis]

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.

[SergeyBiryukov]

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().

[javorszky]

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

[georgestephanis]

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

[javorszky]

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.

[wonderboymusic]

In 29339:

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

Adds unit test.

Props jacklenox, wonderboymusic.
Fixes #28315.

[wonderboymusic]

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.

[wonderboymusic]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[wonderboymusic]

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.