wp_update_user() blindly calls add_magic_quotes(), even on objects

[misterbisson]

If there's an object stored in usermeta, then the call to add_magic_quotes() will lead to an error in wpdb::escape():

Catchable fatal error: Object of class stdClass could not be converted to string in /web/ven/wp-includes/wp-db.php on line 472

http://core.trac.wordpress.org/browser/trunk/wp-includes/registration.php

See also #9638. The two problems are related, but probably need to be solved independently.

Closing as dup -- it should get serialized.

Assuming you're considering this a dupe of #9638, I'd argue this ticket shouldn't be closed. add_magic_quotes () is indeed returning an error, but this behavior in registration.php is wrong:

// First, get all of the original fields
$user=get_userdata($ID);

// Escape data pulled from DB.
$user=add_magic_quotes(get_object_vars($user));

get_userdata() is returning everything unserialized, including arrays and objects. Blindly calling add_magic_quotes() on that data is causing the problem in #9638, but the call to add_magic_quote() is probably unnecessary at this point.

I will take a look into it and write a patch.

add_magic_quotes now takes care of the type of data passed.

patch added, please test against 2.8 bleeding. the checks should prevent throwing any errors.

-1. if arrays get escaped, objects should get escaped as well. plus, it feels like we're hiding the issue under the rug.

I think misterbisson is right: there's a workflow issue in there. it might be that objects should get serialized before calling that function, or something like that.

@Denis: Escape objects: I thought about doing so, but there is no luck with iterating over objects with PHP 4.3.

But more clearly spoken: there is nothing hiding under the rug, the patch just properly sanitizes the functions processes now. as you can read with the phpdoc comment (not from me), this function is for arrays only. since i use recursion I allowed mixed but from the documented description (and that should guide) I see no problems with having a properly working function here.

object now handeled. that should be intended as in wp_update_user()

this time with object handling.

The other Idea is to create a get_object_vars_recursive() function that can be used in wp_update_user() then.

mm... it seems to me that this won't escape anything in any useful way:

$array = add_magic_quotes( get_object_vars( $array ) );

it seems that an object then gets turned into an array and gets stored as such. :-|

if there is no way to iterate over an object in php 4.3, we should either close as wontfix or change the workflow entirely.

well, i would say you are right, but you need to know the circumstances, this _is_ the way to got for wordpress code:

registration.php (as noted above):

// First, get all of the original fields
$user=get_userdata($ID);

// Escape data pulled from DB.
$user=add_magic_quotes(get_object_vars($user));

Replying to hakre:

well, i would say you are right, but you need to know the circumstances, this _is_ the way to got for wordpress code: registration.php (as noted above): {{{ // First, get all of the original fields $user=get_userdata($ID); // Escape data pulled from DB. $user=add_magic_quotes(get_object_vars($user)); }}}

this is a $user and not a $user_meta, no? If anything, this means the above code is wrong too, and needs to be fixed as well -- due to the fact that the user_meta containing an object will the be smashed by add_magic_quotes as well.

No, I take the legacy code as argument why I made the changes, not as argument to change the legacy code. If you want to change the legacy code, you can start re-coding WordPress core.

Replying to hakre:

No, I take the legacy code as argument why I made the changes, not as argument to change the legacy code. If you want to change the legacy code, you can start re-coding WordPress core.

Believe me. I considered it two years ago. :D

working with legacy code does not mean to re-write all. it's the opposite of it normally.

Replying to Denis-de-Bernardy:

Closing as dup -- it should get serialized.

No, it should not. It should be explicitly unserialized (see next comment by me with a detailed analysis).

some more factual info on the case:

1. $user = get_userdata($ID); returns an object (User DB row object).

2. therefore the call to $user=add_magic_quotes(get_object_vars($user)); is perfectly valid.

what is not properly documented is the fact that get_userdata(); calls for _fill_user(). And that little piece makes a visit to my old friend (irony) maybe_unserialize(); and then putting meta_keys to that user object.

so if some user has serialized meta values then those will become (maybe) unserialized.

maybe baby, the user object then contains subobjects.

uuups, have we seen here, that we can use meta_keys to overwrite users properties? nice one... (well not really).

so whats left? what to fix first? please choose:

a) _fill_user() - prevent overwriting of a users properties with meta values.

b) add_magic_quotes() variant A - to add quotes on sub-objects by converting them to array values.

c) add_magic_quotes() variant B - to ignore sub-objects

note for add_magic_quotes(): Current state is quote like variant B but with throwing an error.

Replying to hakre:

$user=add_magic_quotes(get_object_vars($user));

the trouble with this one is it won't do the trick if a meta is an object.

as I see things: the functionality is broken but functional.

as you highlight, _fill_user() allows to override a user's fields with a meta. this might be desirable, or not. it had caught my attention too when looking into a separate ticket on a get_usermeta function overhaul: #7540.

the latter point could be addressed in 2.8 if it poses any security threat, else it's probably a bug we can live with.

on the user meta front, the current behavior works as long as you're not trying to insert objects in user_meta. a plugin dev will notice this on the spot while coding. he'll just work around the issue by storing arrays instead. in other words it's broken but functional.

imo, it's more urgent to give the API needs a good clean up (see #7540) than making add_magic_quotes() behave well for objects. the latter is really not designed for this (it's supposed to be used to quote the GET, POST, etc. variables). put otherwise, it makes more sense (to me anyway) to not use it at all in the various user_meta functions, and to rework the workflow accordingly.

my first patch ignored objects in add_magic_quotes sothat authors using objects there do not run into problems. leaving it unfixed is at least an option everytime, but since it was reported here and the userdata explicitly allows objects, there should be no error thrown. not handling objects in add_magic_quotes in the sense of not trying to escape those (just skip objects in there by decision) is most reasonable to me.

my suggestion:

1.) patch add_magic_quotes to not add quotes to objects.

that's it.

argumentation: add_magic_quotes is used with userdata that is able to contain objects by design (it is made for that).

if you have a problem with the argumentation, then a suggestion is to create a function called add_magic_quotes_to_userdata($user); and use it in that case.

this helps at least solving the dilemma. and it leaves space for the other design issues you raised as well.

ah and add_magic_quotes should return false on any call when a variable is passed that is not an array as describben in the phpdoc statement. if you like the pressure way.

For reference:

post.php ~ 1597

	// Escape data pulled from DB.
	$post = add_magic_quotes($post);

punting pending proper fix to #7540

Some Note for patch 2: wpdb->escape() is used on the string, should be add_magic_quotes(). Just for reference, there seems to be no need to update the patch currently but only in the future.

see also #3243

Docblocks and comments should be provided for clarification. Addtionally current codebase can add a check for other types then arrays. Currently there is only array and else. This is quite broad.

IMHO this should at least be documented with the add_magic_quotes() function.

see also #10419

This should take care of the objects

Looks like several issues are all thrown into this one problem with the way the _escape is handled. The issue is that there may be times where a function passes in an object and want's the object's vars escaped and other times where an array with an object in it may be passed in where the object needs to be serialized (serialized objects in usermeta). I'm curious of what occasions come up where the escape/add_magic_quotes functions need to be recursive.

see also #11509

I've just re-read the patches and some of the comments and I must admit that I'm not confident with my older suggestion any longer. I tend to say that this is a workflow issue and we should check that only intended types are passed to the function, namely strings.

For arrays and objects we could offer new functions in which their name reflects for what they are and we can more strictly define in their docblock what they do. So other developers won't miss actual features (they can switch to the new functions in case they need) and we get the concerns seperated so this is not over-generalized.