Skip to content

Commit

Permalink
Set next_page and prev_page without leaking any info
Browse files Browse the repository at this point in the history
c2d5d72 removed total and pages if a user doesn't have
sufficient rights for security, and this makes paging
difficult because the caller doesn't know if there are
more results.

Refactor next_page to set it based on looking ahead,
but still be secure and not reveal any extra
information about counts. If next_page exists,
a caller now knows they should call for more records.

Fixes: I##37712
  • Loading branch information
cbrandtbuffalo committed Nov 7, 2023
1 parent 96ca7dd commit 6272af8
Show file tree
Hide file tree
Showing 2 changed files with 107 additions and 8 deletions.
41 changes: 33 additions & 8 deletions lib/RT/REST2/Resource/Collection.pm
Original file line number Diff line number Diff line change
Expand Up @@ -208,18 +208,29 @@ sub serialize {
$results{pages} = defined $results{total} ? ceil($results{total} / $results{per_page}) : undef;
if ( $results{pages} ) {
if ($results{page} < $results{pages}) {
my $page = $results{page} + 1;
$uri->query_form( @query_form, page => $results{page} + 1 );
$results{next_page} = $uri->as_string;
$results{next_page} = $self->get_next_page( results_ref => \%results, uri => $uri, query_form_ref => \@query_form );
}
if ($results{page} > 1) {
# If we're beyond the last page, set prev_page as the last page
# available, otherwise, the previous page.
$uri->query_form( @query_form, page => ($results{page} > $results{pages} ? $results{pages} : $results{page} - 1) );
$results{prev_page} = $uri->as_string;
}

if ( (not exists $results{next_page}) && (not defined $results{total}) ) {
# If total is undef, this collection checks ACLs in code so we can't
# use the collection count directly. Try to peek ahead to see if there
# are more records available so we can return next_page without giving
# away specific counts.

$collection->NextPage();
while (my $item = $collection->Next) {
# As soon as we get one record, we know there is a next page
$results{next_page} = $self->get_next_page( results_ref => \%results, uri => $uri, query_form_ref => \@query_form );
last;
}
}

if ( (not exists $results{prev_page}) && $results{page} > 1 ) {
$uri->query_form( @query_form, page => ($results{page} - 1) );
$results{prev_page} = $uri->as_string;
}

return \%results;
}

Expand All @@ -229,6 +240,20 @@ sub serialize_record {
return expand_uid($record);
}

sub get_next_page {
my $self = shift;
my %args = (
results_ref => undef,
uri => undef,
query_form_ref => undef,
@_
);

my $page = $args{results_ref}->{page} + 1;
$args{uri}->query_form( @{$args{query_form_ref}}, page => $args{results_ref}->{page} + 1 );
return $args{uri}->as_string;
}

# XXX TODO: Bulk update via DELETE/PUT on a collection resource?

sub charsets_provided { [ 'utf-8' ] }
Expand Down
74 changes: 74 additions & 0 deletions t/rest2/tickets.t
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,80 @@ my ($ticket_url, $ticket_id);
}
}
# Transactions with paging
{
my $res = $mech->get($ticket_url,
'Authorization' => $auth,
);
is($res->code, 200);
my $history_pages_url = $mech->url_for_hypermedia('history');
# Set low per_page to get at least two pages
$history_pages_url .= '?per_page=10';
$res = $mech->get($history_pages_url,
'Authorization' => $auth,
);
is($res->code, 200);
my $content = $mech->json_response;
is($content->{count}, 9);
is($content->{page}, 1);
is($content->{per_page}, 10);
is($content->{prev_page}, undef, 'No prev_page');
is($content->{next_page}, $history_pages_url . '&page=2');
is($content->{total}, undef, 'No total');
is(scalar @{$content->{items}}, 9);
for my $txn (@{ $content->{items} }) {
is($txn->{type}, 'transaction');
like($txn->{_url}, qr{$rest_base_path/transaction/\d+$});
}
# Get next_page
$res = $mech->get($content->{next_page},
'Authorization' => $auth,
);
is($res->code, 200);
$content = $mech->json_response;
is($content->{count}, 5);
is($content->{page}, 2);
is($content->{per_page}, 10);
is($content->{next_page}, undef, 'No next_page');
is($content->{prev_page}, $history_pages_url . '&page=1');
is($content->{total}, undef, 'No total');
is(scalar @{$content->{items}}, 5);
for my $txn (@{ $content->{items} }) {
is($txn->{type}, 'transaction');
like($txn->{_url}, qr{$rest_base_path/transaction/\d+$});
}
# Back to prev_page
$res = $mech->get($content->{prev_page},
'Authorization' => $auth,
);
is($res->code, 200);
$content = $mech->json_response;
is($content->{count}, 9);
is($content->{page}, 1);
is($content->{per_page}, 10);
is($content->{prev_page}, undef, 'No prev_page');
is($content->{next_page}, $history_pages_url . '&page=2');
is($content->{total}, undef, 'No total');
is(scalar @{$content->{items}}, 9);
for my $txn (@{ $content->{items} }) {
is($txn->{type}, 'transaction');
like($txn->{_url}, qr{$rest_base_path/transaction/\d+$});
}
}
# Ticket Reply
{
# we know from earlier tests that look at hypermedia without ReplyToTicket
Expand Down

0 comments on commit 6272af8

Please sign in to comment.