Wrong user data leak with external object cache

[m_uysl]

Steps to reproduce

Create a new field group and add field(s). Ex: "Facebook"
Create new user and leave "Facebook" field empty.
Fill your "Facebook" field.

Create new php file and try to fetch non-exist data.

<?php
require_once 'wp-load.php';
$maybe_has_facebook = BP_XProfile_ProfileData::get_value_byid( xprofile_get_field_id_from_name('Facebook'), 2 );

Then try to edit that user, you will see your field data in there.

This problem caused by missing $field_id when creating empty cache ​https://buddypress.trac.wordpress.org/browser/tags/2.7.3/src/bp-xprofile/classes/class-bp-xprofile-profiledata.php#L492 and
​https://buddypress.trac.wordpress.org/browser/tags/2.7.3/src/bp-xprofile/classes/class-bp-xprofile-group.php#L450 fetching wrong data in there.

Tested with memcached/redis on trunk.

[r-a-y]

Nice catch, @m_uysl!

Originally committed in r7820.

[boonebgorges]

@m_uysl - Thanks a lot for the ticket and the patch! It's helped me to understand the issue a bit better. It's now clear to me why the cache corruption happens in the case where an admin is editing another user's profile. The mechanics of the more general case (as described in #6091) are still not clear to me, but I'm fairly sure they're related.

It seems to me that the underlying architectural problem here is that cached data for another user is fetched in the first place. When you call BP_XProfile_Group::get(), the field objects are populated using xprofile_get_field(). This instantiates a BP_XProfile_Field object, with the default $user_id and $get_data params. As such, these field objects contain data for the *logged-in* user, even if you are fetching the fields/groups of another user.

There are a bunch of ways to address this. The one that requires the least amount of code is 7401.2.diff​. Briefly: when fetching the field objects in BP_XProfile_Group::get(), be sure to request them in a way that doesn't fetch any user data at all. (BP_XProfile_Group::get() gets the needed user data separately.) This change requires passing around the $user_id and $get_data params, which I don't really like, but it does the trick.

@m_uysl and @r-a-y What do you think of this more general change? As far as I can see, it will ensure that this kind of cross-pollination is impossible in the future.

[m_uysl]

The mechanics of the more general case (as described in #6091) are still not clear to me, but I'm fairly sure they're related.

I think so.

This change requires passing around the $user_id and $get_data params, which I don't really like, but it does the trick.

I hate extra parameters as well but it's less terrifying than fetching wrong data :)

What do you think of this more general change?

It certainly makes sense BP_XProfile_Group::get() should always fetch data belongs to correct user.

@boonebgorges I updated your patch by adding $user_id to fake cache data. (just in case)

[r-a-y]

Looks good to me!

[boonebgorges]

In 11316:

XProfile: More consistent cache behavior when fetching user data.

• Inside of a profile group loop (BP_XProfile_Group::get()), don't fetch user data when pulling up BP_XProfile_Field objects. In the absence of finer-grained information about users, fetching a field object grabs the data associated with the logged-in user. But in many cases, the logged-in user is irrelevant to the fields being looped over, so there's no benefit to pulling up this data. (When necessary - fetch_data - the data is queried separately, later in the get() method.)
• When caching database misses for a data query (because the specifed user doesn't have anything filled in for the given field), store the field_id and user_id properties on the cached object. This ensures that values are properly associated with their fields when being displayed.

These changes resolve an issue where cached data for the logged-in user
can be shown erroneously on another user's profile, when the other user
doesn't have a value for a given field.

Props m_uysl, r-a-y.
See #6091. Fixes #7401.