Sanity checks for member/group widget limits

[boonebgorges]

I discovered an odd issue while debugging a client site for requests that triggered excessive numbers of SQL queries.

A handful of our Members and Groups widgets allow the admin to enter a "max" count. If you enter 0 (or, say, 999999999) it gets passed along blindly to the widget constructor. On a large site, this can result in thousands of database queries.

The problem is especially notable on Multisite installations where BP is network-active. Admins of individual sites (who may not be super admins, and may not know what they're doing) have the ability to add BP widgets to their sites. So this is likely not just an education problem.

Realistically, there's no reason why anyone would ever need more than, say, 50 or 100 users in a widget. I propose we do something like the following:

• Add a gloss to the widget admin UI that says "Up to x members", where 'x' 50 or 100 or something like that
• Validate on the server (in the client too, if we can easily manage it - this might be easier in the Customizer) that the number is between 1 and the max number.
• Run the max number through a filter so that a network admin who really wants a higher number can write a plugin that sets a higher ceiling.

If this seems like overkill, we could also just silently set any large number (or 0) to 25 or 50 or some sane number.

[dcavins]

Hi Boone, this seems sane to me. I think adding the new text and adding a new filter is enough, though (client validation seems like overkill).

[boonebgorges]

Thanks, @dcavins !

8036.diff​ is a proof-of-concept for the main Members widget.

• New function bp_get_widget_max_count_limit() (awkward name for an awkward concept) provides a single point of filter if you want to change the value across the install. The class name passed to the function means you can be more fine-grained if you want. Default is 50.
• Validate that you're not exceeding the max in form() and widget() (this will catch existing instances that are upgraded to 5.0) and also in update()
• I added min and max attributes to the form input (and changed to type="number"). This gives a visual indication that it's for numbers only, and prevents you from clicking up above the limit. You can still enter a higher number, but the widget then won't save. There's no other visual feedback - it's hard to write JS that'll work well both on widgets.php and in the Customizer. Since this is likely to affect very few users, it may not be worth it.

Anyone care to weigh in on the approach before I apply across other widgets?

[dcavins]

Hi Boone-

The logic all looks good here.

Re feedback if the number is out of range: It seems like the WP widget class should display an error if the save fails, but it just doesn't run. It's weird. But, it seems like an issue that should be handled at the WP level.

When testing your code, I encountered a "no index" error and fixed that in the attached patch.

Thanks!

[dcavins]

Boone's patch plus a missing index fix.

[boonebgorges]

In 12323:

Avoid PHP notices when saving widget link_title.

Props dcavins.
See #8036.

[boonebgorges]

In 12324:

Widgets: Place an upper bound on item counts in widget forms.

This prevents widgets from triggering performance problems when initialized
with unreasonably high max item counts.

Use the bp_get_widget_max_count_limit filter to increase the default
ceiling of 50.

Fixes #8036.