friends_add_friend doesnt check if user ids exist

[modemlooper]

If you use the function friends_add_friend you can supply an initiator id and a user id to request friendship but you could supply a user id 38383838383838383838383833838 and it would place a record of the friendship in the db with anything you supply.

There should be a check that users even exist.

​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-friends/bp-friends-functions.php#L33

[espellcaste]

Is it correct to say that this function creator wished the user existence check be done outside of it?

[modemlooper]

Doesn’t check in template functions

​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-templates/bp-legacy/buddypress-functions.php#L1388

[boonebgorges]

What's the bug that we'd be fixing by doing this? @modemlooper Did you come across a situation where the current behavior caused problems?

[modemlooper]

I’m converting buddypress-functions.php functions into JSON API methods and noticed that I could pass anything to the friends_add_friends_function and it would add to db.

Not sure it’s a bug. Is it ok invalid data be added?

[espellcaste]

Not ok at all in my book, at least!

I asked because on wp-cli-buddypress, we are doing the check ourselves, so maybe, when it was created, its creator thought someone needed to do the check before using it. (Not that's ok, just confirming).

But I don't see a problem in adding a check to the users' existence. Makes total sense and might avoid this "bug".

[boonebgorges]

We don't check for valid users in many of our "business" functions. See eg groups_join_group(), bp_activity_add(), xprofile_set_field_data(). In at least some of these cases, it's possible to imagine scenarios where you'd want to put dummy IDs for $user_id. (For example, we do it pretty extensively in our unit tests.) It feels to me like these functions ought to enforce internal data consistency - don't allow someone to join a group if they're already in the group - but external consistency ought to be left up to the controllers responsible for calling these functions (AJAX handlers, API endpoints, etc).

I don't have very strong intuitions on this, though. @djpaul What do you think?

[modemlooper]

I saw other functions do not check ids, so I can go into the html changed the id to anything and then BP js will add the wrong user id without checking. Look at my attachment.

[boonebgorges]

Thanks for the additional info, @modemlooper.

The potential for harm here is pretty low. If someone inserts a dummy friendship, it won't show up in any friendship lists, because those lists - which are powered by bp_has_members() - *do* check against the users table. The only potential mischief is filling up the table with bad data.

I'm attaching two different patches. The ajax patch fixes the problem in the "client" functions, as I've suggested. The business patch fixes it in friends_add_friend(), as @modemlooper and others have suggested. I will note that, aside from my arguments above about consistency and leanness in business functions, there's another reason to go with the ajax approach: friends_add_friend() etc return a simple false value on failure, no matter the nature of the failure, so it's not possible to have a tailored error message.

[DJPaul]

I broadly agree with @boonebgorges. I think we should definitely add the 7683-ajax.diff patch.

(Nitpicking, I'm not sure we call users "users" in our strings, but I don't know if "Not a valid member" is worse English, or not. At any rate, I know it's just an error message for an edge case.)

I did find this question time-consuming and hard to answer. I think it's because I'm a bit scared at the amount of work required to audit all functions called by controllers to check we are sanitising data appropriate in this context (or how to find this information automatically) -- if we were to do that today. I don't think we are proposing that we are.

[boonebgorges]

In 11889:

Templates: Die from AJAX friend operations when friend ID is invalid.

This prevents the adding of friendship objects attached to non-existent
users.

Props modemlooper.
Fixes #7683.

[boonebgorges]

Thanks, @DJPaul.

[11889] fixes the problem in our AJAX functions. A broader audit of our AJAX and controller functions for similar issues would be worthwhile, but is outside the scope of this ticket. If anyone wants to take that project on, please open a separate ticket to track it.