WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 3 years ago

#9640 reopened defect (bug)

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

Reported by: misterbisson Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.8
Component: Users Keywords: has-patch 2nd-opinion dev-feedback
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)

9640.patch (1.0 KB) - added by hakre 5 years ago.
add_magic_quotes now takes care of the type of data passed.
9640.2.patch (1.1 KB) - added by hakre 5 years ago.
object now handeled. that should be intended as in wp_update_user()
wp-db.patch (394 bytes) - added by prettyboymp 5 years ago.
This should take care of the objects

Download all attachments as: .zip

Change History (46)

comment:1 follow-up: Denis-de-Bernardy5 years ago

  • Milestone Unassigned deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Closing as dup -- it should get serialized.

comment:2 misterbisson5 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.

comment:3 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added
  • Milestone set to 2.8
  • Version set to 2.8

comment:4 hakre5 years ago

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

hakre5 years ago

add_magic_quotes now takes care of the type of data passed.

comment:5 hakre5 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.

comment:6 Denis-de-Bernardy5 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.

comment:7 hakre5 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.

hakre5 years ago

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

comment:8 hakre5 years ago

this time with object handling.

comment:9 hakre5 years ago

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

comment:10 Denis-de-Bernardy5 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.

comment:11 follow-up: hakre5 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));

comment:12 in reply to: ↑ 11 Denis-de-Bernardy5 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.

comment:13 follow-up: hakre5 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.

comment:14 in reply to: ↑ 13 Denis-de-Bernardy5 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

comment:15 hakre5 years ago

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

comment:16 in reply to: ↑ 1 hakre5 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).

comment:17 follow-up: hakre5 years ago

  • Keywords security added

some more factual info on the case:

  1. $user = get_userdata($ID); returns an object (User DB row object).
  1. 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.

comment:18 in reply to: ↑ 17 Denis-de-Bernardy5 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.

comment:19 hakre5 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.

comment:20 hakre5 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.

comment:21 hakre5 years ago

For reference:

post.php
~ 1597

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

comment:22 Denis-de-Bernardy5 years ago

  • Milestone changed from 2.8 to Future Release

punting pending proper fix to #7540

comment:23 hakre5 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.

comment:24 Denis-de-Bernardy5 years ago

  • Milestone changed from Future Release to 2.9

comment:26 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; dev-feedback security removed

comment:27 hakre5 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.

prettyboymp5 years ago

This should take care of the objects

comment:29 prettyboymp5 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.

comment:30 prettyboymp5 years ago

  • Keywords has-patch added; needs-patch removed

comment:31 simonwheatley4 years ago

  • Cc simon@… added

comment:32 ryan4 years ago

  • Milestone changed from 2.9 to 3.0

comment:34 hakre4 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.

comment:35 sirzooro4 years ago

  • Cc sirzooro added

comment:36 hakre4 years ago

  • Keywords 2nd-opinion dev-feedback added

comment:37 ryan4 years ago

  • Milestone changed from 3.0 to 3.1

comment:38 nacin3 years ago

  • Milestone changed from Awaiting Triage to Future Release

comment:39 M66B3 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.

Version 1, edited 3 years ago by M66B (previous) (next) (diff)

comment:40 anmari3 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.

Last edited 3 years ago by anmari (previous) (diff)

comment:41 anmari3 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)

comment:42 SergeyBiryukov3 years ago

  • Version changed from 3.2.1 to 2.8

Version number is used to track when the bug was introduced/reported.

comment:43 kanuck543 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.

Note: See TracTickets for help on using tickets.