Opened 16 years ago
Closed 4 years ago
#9640 closed defect (bug) (worksforme)
wp_update_user() blindly calls add_magic_quotes(), even on objects
Reported by: | misterbisson | Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 2.8 |
Component: | Users | Keywords: | |
Focuses: | Cc: |
Description
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.
Attachments (3)
Change History (49)
#1
follow-up:
↓ 16
@
16 years ago
- Milestone Unassigned deleted
- Resolution set to duplicate
- Status changed from new to closed
#2
@
16 years ago
- Resolution duplicate deleted
- Status changed from closed to reopened
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.
#5
@
16 years ago
- Keywords has-patch needs-testing added; needs-patch removed
patch added, please test against 2.8 bleeding. the checks should prevent throwing any errors.
#6
@
16 years ago
-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.
#7
@
16 years ago
@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.
#9
@
16 years ago
The other Idea is to create a get_object_vars_recursive() function that can be used in wp_update_user() then.
#10
@
16 years ago
- Keywords dev-feedback added; has-patch needs-testing removed
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.
#11
follow-up:
↓ 12
@
16 years ago
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));
#12
in reply to:
↑ 11
@
16 years ago
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.
#13
follow-up:
↓ 14
@
16 years ago
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.
#14
in reply to:
↑ 13
@
16 years ago
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
#15
@
16 years ago
working with legacy code does not mean to re-write all. it's the opposite of it normally.
#16
in reply to:
↑ 1
@
16 years ago
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).
#17
follow-up:
↓ 18
@
16 years ago
- Keywords security added
some more factual info on the case:
$user = get_userdata($ID);
returns an object (User DB row object).
- 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.
#18
in reply to:
↑ 17
@
16 years ago
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.
#19
@
16 years ago
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.
#20
@
16 years ago
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.
#21
@
16 years ago
For reference:
post.php
~ 1597
// Escape data pulled from DB. $post = add_magic_quotes($post);
#23
@
16 years ago
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.
#27
@
15 years ago
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.
#29
@
15 years ago
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.
#34
@
15 years ago
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.
#39
@
13 years ago
wp_update_user seems to call add_magic_quotes for all user meta values (maybe this is new behavior in a recent version of WordPress). According to the documentation of update_user_meta "Arrays and objects will be automatically serialized".
So, either this bug should be fixed or the documentation should be updated. For the time being I made a not in the documentation.
#40
@
13 years ago
- Version changed from 2.8 to 3.2.1
Hi all,
I'd like to flag that this really should be fixed by now as it causes great confusion when wp users do not understand the cause (symptoms can occur long after the cause).
EXAMPLE: they use a plugin which creates it's own user object class stored in the user meta ("your members" does this for example, and possibly "WP-member" (judging by email I received today - I guess because of this [post]http://webdesign.anmari.com/1895/wordpress-user-editing-problems/)).
Later the wp site admin deactivates the plugin for whatever reason. They do not realise that anything is amiss until possibly much later until someone tries to edit a user that has the meta data stored. At which point the fatal error occurs:
Catchable fatal error: Object of class PHP_Incomplete_Class could not be converted to string...wp-includes/functions.php on line 1526
The user record cannot be updated and the cause is not clear to them and WordPress looks 'bad'. It is also a difficult one for non techies (increasing using WordPress and experimenting with plugins) to fix as they have to remove the associated usermeta.
I think the WordPress code should handle the situation gracefully.
I don't think that it can just be left for plugin author's to know that they need to restrict themselves to a standard class, or for web owners to deal with fixing the usermeta.
#41
@
13 years ago
Update: In 3.3 beta I get this message
Catchable fatal error: Object of class stdClass could not be converted to string in C:\web\wpbeta\wp\wp-includes\functions.php on line 1526
In 3.2.1 message is
Catchable fatal error: Object of class PHP_Incomplete_Class could not be converted to string in C:\web\wpforum\wp\wp-includes\functions.php on line 1526
I have tried to follow discussion above. somehow it seems objects are still ending up at line 1562 (same code in both versions)
#42
@
13 years ago
- Version changed from 3.2.1 to 2.8
Version number is used to track when the bug was introduced/reported.
#43
@
13 years ago
I'd just like to add my support for fixing this, as I accidentally stored a WP_Error object in the database, and then spent SEVERAL HOURS in a BLIND MURDEROUS RAGE trying to figure out why I was getting 500 Internal Server Errors left and right, despite having removed all traces of the offending code.
It's not a very common issue, but when it strikes, it strikes hard.
#45
@
10 years ago
In my experience, this bug makes it difficult to store denormalized data in meta tables using JSON formatting. When you don't need to query by something other than ID, this encourages meta bloat.
#46
@
4 years ago
- Keywords dev-feedback needs-refresh removed
- Resolution set to worksforme
- Status changed from reopened to closed
I am currently unable to reproduce this with the latest version of WordPress, so I am going to close this out. To test, I used the following code:
add_action( 'admin_init', function() { update_user_meta( 1, 'object_meta1', (object) array( 'some_property' => 'some_value', ) ); update_user_meta( 1, 'object_meta2', new stdClass()); wp_update_user( array( 'ID' => 1, 'user_email' => 'somenewemail@test.com', ) ); });
It's possible that [48205]/[48440] or an earlier change resolved this. If someone is still able to reproduce, please reopen with specific steps someone can use to cause the issue. Bonus for a unit test or two that fails demonstrating the issue.
Closing as dup -- it should get serialized.