Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#5796 closed defect (bug) (fixed)

Invalid or empty page_arg results in no-limit queries

Reported by: johnjamesjacoby Owned by: johnjamesjacoby
Milestone: 2.2 Priority: high
Severity: major Version:
Component: Core Keywords: good-first-bug has-patch
Cc:

Description

Passing an invalid page argument in a URL that's listening for $_REQUEST[ $page_arg ] will empower users to query for unpaginated results. To reproduce:

  • domain.com/activity/?acpage=%27
  • domain.com/activity/?acpage=0
  • domain.com/members/?upage=%27
  • domain.com/members/?upage=0

Attachments (4)

5796.patch (938 bytes) - added by johnjamesjacoby 6 years ago.
Low impact fix for activity template class. Could be applied to other components.
5796.2.patch (1.0 KB) - added by dustyf 6 years ago.
Assigned to variables to work with PHP 5.2 and used absint() based on tw2113's suggestion.
5796.3.patch (3.0 KB) - added by boonebgorges 6 years ago.
5796.4.patch (483 bytes) - added by thebrandonallen 6 years ago.

Download all attachments as: .zip

Change History (33)

#1 @johnjamesjacoby
6 years ago

At a cursory, our intval( $_REQUEST[$page_arg] ) checks are not enough here. intval() sets an invalid result to 0, and 0 assumes unlimited results are being requested.

While I can think of reasons why this might be useful, it's problematic on large sites where querying for all content will either lock up the database or OOM PHP.

I recommend we put empty() checks in our _Template classes for our page_arg values, and force them back to 1 (or the $page default argument). This way our core functions and classes remain untouched and querying for unlimited results is still possible, and we only prevent users from passing invalid arguments around.

#2 @johnjamesjacoby
6 years ago

Worth highlighting, the following components (capable of returning paginated results) are susceptible:

  • Groups
  • Activity
  • Members
  • Notifications
  • Messages
  • Blogs
  • Friends

See also: #3679

@johnjamesjacoby
6 years ago

Low impact fix for activity template class. Could be applied to other components.

#3 @johnjamesjacoby
6 years ago

Similar issues can be found on other $_REQUEST checks, like num which is also fixed in the above patch.

#4 @boonebgorges
6 years ago

Yes, I think your technique here is good. I want to be very careful not to break any implementations that are intentionally pulling up unpaginated results by passing 0 to the function, but like you, I don't see any valid reason to allow this to be set via the URL string.

On a somewhat related note, the whole logic for grabbing these params out of the URL seems messed up to me. Values passed to the function should always take precedence. See #5796 for a similar point with respect to bp_has_groups(). In that case, we moved the $_REQUEST logic *above* the definition of the function defaults. In this case, that's not possible because of the variable pag_arg; but it does seem to me that the logic should be opposite of what it currently is:

// Check the passed arguments directly
if ( ! isset( $args['num'] ) && ! empty( $_REQUEST['num'] ) ) {
    $this->pag_num = intval( $_REQUEST['num'] );
} else {
    $this->pag_num = intval( $r['page'] );

and likewise for pag_page. (I don't think that this latter bit needs to be fixed for BP 2.1.)

#5 @DJPaul
6 years ago

  • Keywords commit added

5796.patch looks OK for 2.1. Please commit it, the others can be changed in a future release.

#6 @DJPaul
6 years ago

  • Keywords has-patch added; needs-patch removed

#7 @DJPaul
6 years ago

  • Keywords needs-patch added; 2nd-opinion commit has-patch removed

empty( intval( ... ) ) doesn't work in PHP 5.2. I think it's only valid in >= PHP 5.5.

Fatal error: Can't use function return value in write context

Last edited 6 years ago by DJPaul (previous) (diff)

This ticket was mentioned in IRC in #buddypress-dev by jjj. View the logs.


6 years ago

This ticket was mentioned in IRC in #buddypress-dev by jjj. View the logs.


6 years ago

#10 @DJPaul
6 years ago

  • Milestone changed from 2.1 to 2.1.1

#11 @DJPaul
6 years ago

  • Milestone changed from 2.1.1 to 2.2

#12 @DJPaul
6 years ago

  • Keywords needs-refresh has-patch good-first-bug added; needs-patch removed

This would make a good contribution from someone. Take John's patch, make it play nice with PHP 5.2, et voilà.

Last edited 6 years ago by DJPaul (previous) (diff)

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


6 years ago

#14 @DJPaul
6 years ago

  • Keywords needs-patch added; needs-refresh has-patch removed

#15 @tw2113
6 years ago

intval() calls need to be removed from inside empty() checks and there's validity in using absint() instead of intval()

@dustyf
6 years ago

Assigned to variables to work with PHP 5.2 and used absint() based on tw2113's suggestion.

#16 @DJPaul
6 years ago

  • Keywords commit has-patch added; needs-patch removed

Need a committer to give this a final test, but let's get this in.

@boonebgorges
6 years ago

#17 @boonebgorges
6 years ago

  • Keywords commit removed

5796.2.patch won't work because it doesn't check to see that the $_REQUEST value isset before passing to absint().

5796.3.patch makes the logic a bit more verbose. Includes unit tests for the following cases:

  • $_REQUEST['acpage'] = 5 (a valid number, which overrides the 'page' value passed to the template object)
  • $_REQUEST['acpage'] = 0 (an invalid number, in which case the value of 'page' should be used)
  • $_REQUEST['num'] = 14 (a valid number, which overrides the 'per_page' value passed to the template object)
  • $_REQUEST['num'] = 0 (an invalid number, in which case the value of 'per_page' should be used)

I think this logic correctly describes what we want here (though see my comment above about how this is not ideal - there ought to be a better way to override URL params programatically). Could I get a second set of eyes? If it's right, I think we can apply the same logic across the template classes.

#18 @johnjamesjacoby
6 years ago

  • Owner set to johnjamesjacoby
  • Status changed from new to assigned

#19 @johnjamesjacoby
6 years ago

In 9410:

Introduce bp_sanitize_pagination_arg() and related tests. This function will help sanitize our pagination request values, as they are frequently accessed via globally accessible variables for ajax requests. See #5796.

#20 @johnjamesjacoby
6 years ago

In 9411:

Use bp_sanitize_pagination_arg() in BP_Activity_Template and include related tests. This prevents pagination values from being overridden outside of anticipated boundaries. Props boonebgorges. See #5796.

#21 @johnjamesjacoby
6 years ago

In 9412:

Use bp_sanitize_pagination_arg() in BP_Core_Members_Template and include related tests. This prevents pagination values from being overridden outside of anticipated boundaries. See #5796.

#22 @johnjamesjacoby
6 years ago

In 9413:

Use bp_sanitize_pagination_arg() in BP_Blogs_Template and include related (multisite only) tests. This prevents pagination values from being overridden outside of anticipated boundaries. See #5796.

#23 @johnjamesjacoby
6 years ago

In 9414:

Use bp_sanitize_pagination_arg() in BP_Notifications_Template and include related tests. This prevents pagination values from being overridden outside of anticipated boundaries. See #5796.

#24 @johnjamesjacoby
6 years ago

In 9415:

Use bp_sanitize_pagination_arg() in BP_Messages_Box_Template and include related tests. This prevents pagination values from being overridden outside of anticipated boundaries. See #5796.

#25 @johnjamesjacoby
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 9416:

Use bp_sanitize_pagination_arg() in the following Groups classes:

  • BP_Groups_Template
  • BP_Groups_Invite_Template
  • BP_Groups_Group_Members_Template
  • BP_Groups_Membership_Requests_Template

Includes related tests for each class, as well as pag_arg assignments for classes that were previously lacking them.

This prevents pagination values from being overridden outside of anticipated boundaries. Fixes #5796.

#26 @johnjamesjacoby
6 years ago

In 9417:

Add pagination group to recent unit test additions related to pagination argument sanitization. See #5796.

#27 @thebrandonallen
6 years ago

There's an unneeded intval included with bp_sanitize_pagination_arg() since $int is absint()'d immediately before. 5796.4.patch removes this.

#28 @johnjamesjacoby
6 years ago

In 9426:

Move intval() call to outside of comparison in bp_sanitize_pagination_arg(). Props thebrandonallen. See #5796.

#29 @DJPaul
4 years ago

  • Component changed from Component - Any/All to Core
Note: See TracTickets for help on using tickets.