Skip to:
Content

BuddyPress.org

Opened 9 years ago

Closed 9 years ago

#6506 closed defect (bug) (fixed)

Should not try to redirect in bp_has_message_threads

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by: johnjamesjacoby's profile johnjamesjacoby
Milestone: 2.4 Priority: normal
Severity: normal Version: 1.0
Component: Messages Keywords: commit
Cc:

Description

In bp_has_message_threads() is a bit of code intended to redirect a non-capable member from the "Notices" area. Unfortunately, bp_has_message_threads() is a template function, and is only ever called after headers have been sent and page output has started.

At best, in themes that rely on bp-default or bp-legacy this code is dead, as messages_screen_notices() is also wrapped in bp_current_user_can( 'bp_moderate' ) checks to avoid hooked in and/or executed.

At worst, in themes that already come with their own BuddyPress template parts, members who navigate to the notices URL manually (it's not made visible in the UI) are greeted with the following debug notice and/or an empty page:

Warning: Cannot modify header information - headers already sent by (output started at /srv/www/wordpress-develop/src/wp-includes/formatting.php:4154) in /srv/www/wordpress-develop/src/wp-includes/pluggable.php on line 1205

Attachments (3)

6506.01.patch (1.0 KB) - added by johnjamesjacoby 9 years ago.
6506.02.patch (574 bytes) - added by boonebgorges 9 years ago.
6506.03.patch (2.3 KB) - added by johnjamesjacoby 9 years ago.

Download all attachments as: .zip

Change History (14)

#1 @johnjamesjacoby
9 years ago

6506.01.patch does two things:

  • Replace an early return false; with a bp_do_404() to prevent an empty page where there should be a 404
  • Remove the bp_core_redirect( bp_displayed_user_domain() ) bit from bp_has_message_threads() since it's neither the correct location for this decision to be made, nor the right time to try and perform a redirection

#2 @johnjamesjacoby
9 years ago

  • Keywords has-patch 2nd-opinion added

This ticket was mentioned in Slack in #buddypress by jjj. View the logs.


9 years ago

#4 follow-up: @boonebgorges
9 years ago

  • Keywords 2nd-opinion removed

Regarding the bp_do_404(): This block can never be reached, because of the bp_current_user_can( 'bp_moderate' ) checks in BP_Messages_Component::setup_nav(). Technically, it could be removed altogether, will all caps checks handled during nav setup. But swapping out for bp_do_404() will do no harm.

I agree that there should not be a redirect in bp_has_messages_threads(), but simply removing the block could potentially introduce security issues when the function is being called directly without proper cap checks. I recommend returning false, which is to say, ! bp_has_messages_threads(). See 6506.02.patch.

#5 in reply to: ↑ 4 @johnjamesjacoby
9 years ago

Replying to boonebgorges:

Regarding the bp_do_404(): This block can never be reached, because of the bp_current_user_can( 'bp_moderate' ) checks in BP_Messages_Component::setup_nav(). Technically, it could be removed altogether, will all caps checks handled during nav setup. But swapping out for bp_do_404() will do no harm.

Screen functions that do not hook themselves directly into bp_screens (meaning they are hooked to bp_screens indirectly via setup_nav(), bp_core_new_nav_item(), or bp_core_new_subnav_item()) do not require any additional capability checks, so I recommend we remove it here to avoid future confusion.

The Messages component screens specifically needs to do a bunch of bp_action_variables() checks because of the way messages_screen_conversation() is loosely hooked in; most other components do not have this collision issue.

I agree that there should not be a redirect in bp_has_messages_threads(), but simply removing the block could potentially introduce security issues when the function is being called directly without proper cap checks. I recommend returning false, which is to say, ! bp_has_messages_threads(). See 6506.02.patch.

None of our other template-loop generating functions perform hard-stops on query results based on capabilities or conditions; they only generate smart defaults based on various conditions like current components, actions, action variables, displayed members, single items, etc...

I feel as if bp_has_message_threads() is the closest template function we (currently) have to openly query for messages within any set of parameters. Returning anything other than the requested results seems counter-intuitive to me, at least until we have bp_has_messages() or something that could perhaps be more of a general handler, when we can then purposely limit functions like bp_has_message_threads() to fit exactly the parameters it's intended for.

Updated patch imminent.

#6 @johnjamesjacoby
9 years ago

6506.03.patch is a little polluted with extra changes, but you'll get the gist:

  • Remove the direct in bp_has_message_threads() and put no restrictions in it's place
  • Remove the superfluous capability check in bp_current_user_can()
  • Rejig the default parameters of bp_has_message_threads() to make their calculations a bit more obvious

#7 follow-up: @boonebgorges
9 years ago

Thanks, johnjamesjacoby. But again: this change will introduce a security problem for anyone who is calling bp_has_message_threads() and expecting it to do the necessary cap checks. Perhaps no one is doing this, and perhaps we can make the change and document/advertise it, but I do want it to be noted before we just pull the check out.

#8 in reply to: ↑ 7 @johnjamesjacoby
9 years ago

Replying to boonebgorges:

Thanks, johnjamesjacoby. But again: this change will introduce a security problem for anyone who is calling bp_has_message_threads() and expecting it to do the necessary cap checks. Perhaps no one is doing this, and perhaps we can make the change and document/advertise it, but I do want it to be noted before we just pull the check out.

But it doesn't.

The current capability check only ever happens if a non-capable member manually attempts to visit /members/themself/messages/notices/ which is already not hooked into bp_screens if the member does not have the bp_moderate capability.

This means that if a developer is incorrectly using bp_has_message_threads() like you're stating, this bit of code only actually helps protect them from themselves when a logged-in non-capable member visits themself/messages/notices having core-hacked BP_Messages_Component::setup_nav() to remove the capability check.

Last edited 9 years ago by johnjamesjacoby (previous) (diff)

#9 follow-up: @boonebgorges
9 years ago

Right, I guess it only happens when bp_is_current_action( 'notices' ). I guess it's OK to pull it out.

#10 in reply to: ↑ 9 @johnjamesjacoby
9 years ago

  • Keywords commit added; has-patch removed

Replying to boonebgorges:

Right, I guess it only happens when bp_is_current_action( 'notices' ). I guess it's OK to pull it out.

I think so, too.

I have a hunch this code is a relic from before we had any bp_screens sub-action segregation, and was inaccurately put here to prevent showing the notice screen to members who should never see it; and now that's handled in a few more reliable places.

#11 @johnjamesjacoby
9 years ago

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

In 9946:

Messages: Remove unused redirect in bp_has_message_threads().

This change prevents a debug notice for themes with built-in BuddyPress support, by removing a tightly-scoped redirect attempt that only occurs after page output has already started. It also removes a superfluous bp_moderate capability check from the message notices screen function, as that function is only hooked in if the user has bp_moderate previously.

Fixes #6506 (in trunk, for 2.4)

Note: See TracTickets for help on using tickets.