Skip to:

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#7683 closed defect (bug) (fixed)

friends_add_friend doesnt check if user ids exist

Reported by: modemlooper's profile modemlooper Owned by: djpaul's profile djpaul
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9.2
Component: Friends Keywords: has-patch 2nd-opinion


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.

Attachments (3)

db.png (111.2 KB) - added by modemlooper 6 years ago.
7683-ajax.diff (1.5 KB) - added by boonebgorges 6 years ago.
7683-business.diff (656 bytes) - added by boonebgorges 6 years ago.

Download all attachments as: .zip

Change History (14)

#1 @espellcaste
6 years ago

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

#3 @boonebgorges
6 years ago

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?

#4 @modemlooper
6 years ago

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?

#5 @espellcaste
6 years ago

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".

#6 @boonebgorges
6 years ago

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?

#7 @modemlooper
6 years ago

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.

6 years ago

#8 @boonebgorges
6 years ago

  • Keywords has-patch 2nd-opinion added
  • Milestone changed from Awaiting Review to 3.0
  • Owner set to djpaul
  • Status changed from new to reviewing

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.

#9 @DJPaul
6 years ago

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.

#10 @boonebgorges
6 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 11889:

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

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

Props modemlooper.
Fixes #7683.

#11 @boonebgorges
6 years ago

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.

Note: See TracTickets for help on using tickets.