Skip to:

Opened 6 years ago

Closed 6 years ago

#8036 closed enhancement (fixed)

Sanity checks for member/group widget limits

Reported by: boonebgorges's profile boonebgorges Owned by: boonebgorges's profile boonebgorges
Milestone: 5.0.0 Priority: normal
Severity: normal Version:
Component: Core Keywords: 2nd-opinion has-patch


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.

Attachments (2)

8036.diff (3.9 KB) - added by boonebgorges 6 years ago.
8036.2.diff (4.0 KB) - added by dcavins 6 years ago.
Boone's patch plus a missing index fix.

Download all attachments as: .zip

Change History (7)

#1 @dcavins
6 years ago

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

6 years ago

#2 @boonebgorges
6 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 5.0.0

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?

#3 @dcavins
6 years ago

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.


6 years ago

Boone's patch plus a missing index fix.

#4 @boonebgorges
6 years ago

In 12323:

Avoid PHP notices when saving widget link_title.

Props dcavins.
See #8036.

#5 @boonebgorges
6 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.