Invalid or empty page_arg results in no-limit queries

[johnjamesjacoby]

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

[johnjamesjacoby]

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.

[johnjamesjacoby]

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

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

See also: #3679

[johnjamesjacoby]

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

[johnjamesjacoby]

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

[boonebgorges]

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.)

[DJPaul]

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

[DJPaul]

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

[DJPaul]

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

[tw2113]

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

[dustyf]

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

[DJPaul]

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

[boonebgorges]

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.

[johnjamesjacoby]

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.

[johnjamesjacoby]

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.

[johnjamesjacoby]

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.

[johnjamesjacoby]

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.

[johnjamesjacoby]

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.

[johnjamesjacoby]

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.

[johnjamesjacoby]

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.

[johnjamesjacoby]

In 9417:

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

[thebrandonallen]

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

[johnjamesjacoby]

In 9426:

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