[buddypress-trac] [BuddyPress Trac] #5664: BP setup routine can run erroneously

[buddypress-trac]

#5664: BP setup routine can run erroneously
--------------------------+-----------------------------
 Reporter:  boonebgorges  |      Owner:
     Type:  defect (bug)  |     Status:  new
 Priority:  normal        |  Milestone:  Awaiting Review
Component:  Core          |    Version:
 Severity:  major         |   Keywords:
--------------------------+-----------------------------
 A few days back I got an emergency ticket from a client, letting me know
 that the site was whitescreening. After some debugging, it turned out that
 a number of components had been deactivated (groups, legacy forums,
 messages), causing fatal errors with custom functionality. Some more
 investigation showed that the existing directory pages for these
 components had also been deleted (along with the Register and Activity
 pages), with multiple versions of these pages regenerated in their place,
 and the Dashboard > BuddyPress > Pages settings switched to point to the
 most recently created versions (so the URLs were `members-3`, etc). The
 page authors were three different non-admin users, and the pages had all
 been created at the exact same time.

 It's hard to know for sure, but my intepretation is that the BP
 installation routine was being run incorrectly. See the `if (
 bp_is_install() )` block here
 https://buddypress.trac.wordpress.org/browser/tags/2.0.1/bp-core/bp-core-
 update.php#L199: keeping in mind that the default components are Members,
 Activity, and XProfile, running this code on a production site would
 result in just the symptoms described above.

 How could this happen? A possible trace:
 - `bp_is_install()` is true if `! bp_get_db_version_raw()`. `!
 bp_get_db_version_raw()` is true if `$bp->db_version_raw` is `empty()`,
 which would presumably happen here
 https://buddypress.trac.wordpress.org/browser/tags/2.0.1/bp-
 loader.php#L503. I'm not sure how or why this would happen, but it does
 explain how the `bp_is_install()` check would pass.
 - There is also the question of how we got to the `bp_version_updater()`
 function to begin with. It is called by `bp_setup_updater()`, but only
 when `bp_is_update()` is true; but `bp_is_update()` will return true under
 similar circumstances as `bp_is_install()`, described above.
 `bp_setup_updater()` is hooked to `'bp_admin_init'`, which suggests that
 the entire episode was triggered by these non-admin users visiting a `wp-
 admin` page at a moment when something was messing with the
 `get_site_option( '_bp_db_version' )` call or something like that.

 I'm afraid I don't have any more details, or any concrete ideas about how
 to trace it further. Part of the purpose of this ticket is to ask whether
 anyone here has ever seen anything like this, or has any ideas about where
 else I could look.

 The second reason for opening the ticket is to suggest that there could be
 more safeguards in place to prevent this sort of problem from happening. A
 couple thoughts:

 1. Bail out of `bp_version_updater()` based on a cap ('bp_moderate' or
 maybe something stronger) check. This may not have prevented the problem,
 but it would at least prevent the odd situation of directory pages being
 generated and being attributed to a non-admin user.
 2. In `bp_version_updater()`, do some sort of gentle checks before wiping
 out existing 'bp-active-components' settings and directory pages. Another
 way of putting this: do we want the absence of `db_version_raw`, all by
 itself, to trigger the deletion of existing content? Seems to me we should
 use data if we find it there; users who want to trigger a full reinstall
 by deleting `_bp_db_version` will be savvy enough to delete `bp_pages` and
 `bp-active-components` too.

 Any thoughts are welcome.

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/5664>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac