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 | Owned by: | 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)
Change History (14)
This ticket was mentioned in Slack in #buddypress by jjj. View the logs.
9 years ago
#4
follow-up:
↓ 5
@
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
@
9 years ago
Replying to boonebgorges:
Regarding the
bp_do_404()
: This block can never be reached, because of thebp_current_user_can( 'bp_moderate' )
checks inBP_Messages_Component::setup_nav()
. Technically, it could be removed altogether, will all caps checks handled during nav setup. But swapping out forbp_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
@
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:
↓ 8
@
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
@
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.
#9
follow-up:
↓ 10
@
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
@
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.
6506.01.patch does two things:
return false;
with abp_do_404()
to prevent an empty page where there should be a 404bp_core_redirect( bp_displayed_user_domain() )
bit frombp_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