Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 9 years ago

Last modified 8 years ago

#5796 closed defect (bug) (fixed)

Invalid or empty page_arg results in no-limit queries

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by: johnjamesjacoby's profile 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 10 years ago.
Low impact fix for activity template class. Could be applied to other components.
5796.2.patch (1.0 KB) - added by dustyf 10 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 10 years ago.
5796.4.patch (483 bytes) - added by thebrandonallen 9 years ago.

Download all attachments as: .zip

Change History (33)

#1 @johnjamesjacoby
10 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
10 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
10 years ago

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

#3 @johnjamesjacoby
10 years ago

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

#4 @boonebgorges
10 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
10 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
10 years ago

  • Keywords has-patch added; needs-patch removed

#7 @DJPaul
10 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 10 years ago by DJPaul (previous) (diff)

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


10 years ago

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


10 years ago

#10 @DJPaul
10 years ago

  • Milestone changed from 2.1 to 2.1.1

#11 @DJPaul
10 years ago

  • Milestone changed from 2.1.1 to 2.2

#12 @DJPaul
10 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 10 years ago by DJPaul (previous) (diff)

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


10 years ago

#14 @DJPaul
10 years ago

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

#15 @tw2113
10 years ago

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

@dustyf
10 years ago

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

#16 @DJPaul
10 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.

#17 @boonebgorges
10 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
9 years ago

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

#19 @johnjamesjacoby
9 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
9 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
9 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
9 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
9 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
9 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
9 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
9 years ago

In 9417:

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

#27 @thebrandonallen
9 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
9 years ago

In 9426:

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

#29 @DJPaul
8 years ago

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